Repository navigation
Release the surface-ids test's lock in one rename so the waiting allocator cannot land mid-removal - #1249
Conversation
…cator cannot land mid-removal
Deploying mouseterm with
|
| Latest commit: |
bbc021f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e3009635.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-ci-39457380049.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
CI hasn't run the test this PR fixes yet, so the fix is still unverified. Build & Test failed in pnpm test:other at its first step: spec-lint reports docs/specs/security-remote.md (7117 words against a budget of 7100) and docs/specs/security.md (2669 against 2650) over their word budgets. The && chain stopped there, before the vscode-ext tests ran. Main has the same failure and also has security-hosted.md over budget (run 39458825365). It comes from the base branch, not this diff, and #1250 re-baselines those budgets. Once main is green, a merge or rerun will give the real result for surface-ids.test.ts.
|
The fix is now verified in CI. I merged |
Fixes a flaky test,
surface-ids.test.ts > waits out a lock a live holder keeps, and takes it once released. It failed on main in run 39457380049 withENOTEMPTY: directory not empty, rmdir '…/surface-ids.json.lock'. The race is in the test, not in the allocator.The test releases the other window's lock with
rm(lock(), { recursive: true }), which deletesowner.jsonand then callsrmdiron the directory. Meanwhile, the waiting allocator keeps retryingrename(mine, lock)every few milliseconds. On Linux, a directory rename onto an empty directory replaces it. If a retry lands between the unlink and thermdir, the allocator takes the lock and thermdirhits itsowner.json. The secondENOTEMPTYin the log, on the temp dir, follows from the first: the test threw while the allocator was still writing.Production code already avoids this.
removeLockinvscode-ext/src/surface-ids.tsmoves the lock aside in a single rename before deleting it. The test now releases the lock the same way, through areleaseLock()helper. The otherrm(lock())call, in "releases only a lock it still holds", is left unchanged: it runs while the only allocator holds the lock, so nothing is waiting to race it.Verification: I couldn't run vitest here because the CI sandbox has no network for
pnpm install, so this PR's CI is the test run. In plain Node, I confirmed thatrenameSync(dir, emptyDir)succeeds and that the followingrmdirSyncthrowsENOTEMPTY, matching the CI error.