Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: tmpdir include build id #41734

Conversation

benjamingr
Copy link
Member

This is an attempt to solve the flakiness of test-debugger-heap-profiler.js which returns EBUSY on windows builds sometimes (examples in nodejs/reliability#182 ) with:

Can't clean tmpdir: C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.816
Files blocking: [ 'node.heapsnapshot' ]

C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:54
    throw e;
    ^

Error: EBUSY: resource busy or locked, rmdir '\\?\C:\workspace\node-test-binary-windows-js-suites\node\test\.tmp.816'
    at rmdirSync (node:fs:1167:10)
    at _rmdirSync (node:internal/fs/rimraf:235:5)
    at rimrafSync (node:internal/fs/rimraf:193:7)
    at Object.rmSync (node:fs:1216:10)
    at rmSync (C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:8:6)
    at process.onexit (C:\workspace\node-test-binary-windows-js-suites\node\test\common\tmpdir.js:39:5)
    at process.emit (node:events:534:35) {
  errno: -4082,
  syscall: 'rmdir',
  code: 'EBUSY',
  path: '\\\\?\\C:\\workspace\\node-test-binary-windows-js-suites\\node\\test\\.tmp.816'
}

Node.js v18.0.0-pre

This attempts to fix it by adding the build id as part of the temporary directory in addition to the test id to be more resilient to cleanup related issues.

I also moved the file to mjs while touching it to debug locally and try to reproduce the issue.

Alternatively we can make tmpdir.refresh() retry on EBUSY more (currently 3 retries and 100ms) or try to debug why these aren't cleaned between/across builds anyway. (Though this sort of per-build "isolation" seems like a generally good idea to me to be resilient to process-freezing bugs)

@benjamingr benjamingr added test Issues and PRs related to the tests. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jan 28, 2022
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 28, 2022
@@ -14,6 +14,7 @@ const testRoot = process.env.NODE_TEST_DIR ?
// Using a `.` prefixed name, which is the convention for "hidden" on POSIX,
// gets tools to ignore it by default or by simple rules, especially eslint.
const tmpdirName = '.tmp.' +
(process.env.BUILD_ID || process.pid + '.') +
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to check this gets passed - my understanding in test.py is that the os environment variables are copied and accessible.

@aduh95
Copy link
Contributor

aduh95 commented Jan 28, 2022

This might resolve the flakiness of the CI indeed, but wouldn't that create other problems if there are temp directories that are left uncleaned? Isn't it like a memory leak but for hard drives?

@benjamingr
Copy link
Member Author

I assumed the temporary directories get cleaned up on their own (on restart/boot)?

@benjamingr benjamingr closed this Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants