fix: publish and recover multiscan locks atomically - #196
Conversation
(cherry picked from commit f6e3033)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cdc1f6880
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04f8a7be59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52fc7d944b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moved?.token !== current?.token || | ||
| (moved !== null && processIsRunning(moved.pid)) | ||
| ) { | ||
| await rename(stale, path); |
There was a problem hiding this comment.
When a live legacy supervisor replaces the stale directory after current is read and a third supervisor publishes a file lock during the vacancy created by rename(path, stale), this restoration fails because a directory cannot be renamed over the third supervisor's file. The recovery attempt then releases its marker while the live legacy lock remains stranded under .lock.stale-*; once the third supervisor rejects the live recovery owner and removes its file, .lock is vacant and another campaign can overlap the legacy campaign. Fresh evidence in this head is that the new directory-specific rename avoids hard-linking the directory but still does not handle a concurrently reoccupied destination.
Useful? React with 👍 / 👎.
| (await readLockOwner(recovery))?.token !== token || | ||
| (await readLockOwner(path))?.token !== token | ||
| ) { | ||
| await releaseLock(path, token); |
There was a problem hiding this comment.
When two contenders both validate the same stale recovery inode before either swap, contender B can publish its marker and read the stale .lock, then contender A can replace the marker, install its lock, pass the final checks, and return. B still proceeds with its earlier observation, overwrites A's lock, and reaches this cleanup because the recovery marker is no longer B's; releaseLock then removes B's replacement and leaves .lock vacant while A's campaign is running, allowing a third supervisor to start. Fresh evidence in this head is that the added final check detects the losing contender but cleans up only after that contender has already overwritten the winner's lock.
Useful? React with 👍 / 👎.
| await rename(pending, path); | ||
| removed = true; | ||
| } finally { | ||
| if (removed) await rm(stale, { recursive: true, force: true }); |
There was a problem hiding this comment.
If recursive removal of the moved legacy directory fails after rename(pending, path)—for example, because Windows reports EPERM or EBUSY for an open file—this finally propagates the cleanup error while leaving the new token installed at .lock. No unlock callback is returned, and the recovery marker is subsequently released, so retries in the same long-lived SDK process see that process's PID as live and remain locked out until it exits. Release the newly published lock when stale-directory cleanup fails, or defer nonessential cleanup until ownership has been safely returned.
Useful? React with 👍 / 👎.
| for (;;) { | ||
| try { | ||
| await link(pending, path); | ||
| const recovering = await readLockOwner(recovery); |
There was a problem hiding this comment.
If the initial hard link succeeds but inspecting .lock.recovery fails with a non-ENOENT error—for example, an unreadable preexisting marker produces EACCES—the catch rethrows without releasing the newly published .lock. The outer cleanup removes only the pending pathname, leaving the same token owned by the still-live SDK process; no unlock callback was returned, so subsequent attempts in that process report an active supervisor until the process exits. Track successful publication and release it on validation errors before propagating the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f815af6b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (contender === null) continue; | ||
| if ( | ||
| contender.token === undefined || | ||
| !occupied.isFile() || |
There was a problem hiding this comment.
When a previous-SDK supervisor creates a directory lock in the vacancy after rename(path, stale)—which it can do because it ignores .lock.recovery—rename(stale, path) fails and this !occupied.isFile() branch immediately throws. This strands the first live legacy supervisor's ownership under .lock.stale-*; after the new directory owner removes .lock, another supervisor can acquire it while the stranded supervisor is still running. Fresh evidence in the exact head is that the newly added restoration loop only evicts regular-file contenders and explicitly rejects directory contenders, so the previously reported reoccupation race remains for legacy peers.
Useful? React with 👍 / 👎.
| ); | ||
| if (occupied === null) continue; | ||
| const contender = await readLockOwner(path); | ||
| if (contender === null) continue; |
There was a problem hiding this comment.
When an old supervisor completes mkdir(.lock) in this restoration window but crashes before writing owner.json, readLockOwner(path) remains null and this unconditional continue spins forever while retaining the recovery marker. This is precisely the incomplete legacy-lock crash state the migration is intended to recover, so the occupant must be moved and validated or the restoration attempt must terminate rather than retrying until some unrelated process changes the path.
Useful? React with 👍 / 👎.
| recovering !== null && | ||
| recovering.token !== token && | ||
| processIsRunning(recovering.pid) |
There was a problem hiding this comment.
If a recoverer crashes after rename(path, stale) has vacated .lock, the next invocation can publish its file through this direct-link branch. Because the condition rejects only a recovery owner whose PID is still running, it accepts the dead foreign recovery marker and returns without ever reclaiming it, permanently leaking .lock.recovery, its pending link, and the moved .lock.stale-* directory. A later reuse of the dead PID then makes otherwise valid campaigns report that a supervisor is running, so any foreign recovery marker should be resolved before this branch returns ownership.
Useful? React with 👍 / 👎.
| const replacement = `${path}.pending-${owner.pid}`; | ||
| await acquireRecoveryMarker(replacement, pending, token); |
There was a problem hiding this comment.
Each time a process crashes after recursively publishing replacement but before renaming it over the abandoned marker, the next recovery appends another .pending-<pid> suffix to the same filename component. After enough interrupted recovery attempts, this component exceeds the filesystem's filename limit and link fails with ENAMETOOLONG; because the existing chain cannot then be collapsed, the output directory remains permanently unusable without manual cleanup. Use constant-length sibling names or otherwise bound and clean the abandoned-marker chain.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
Closes #102.
Why
The original two-step directory lock could permanently block a bulk scan after an interrupted owner write. PR #98 fixes that publication gap, but adversarial QA found that its stale-recovery path could admit several supervisors at once, overwrite a competing lock, accept unusable PID values, and leak pending files on I/O failure.
This replacement keeps the contributor's atomic hard-link publication and adds a separate, atomically published recovery claim. Stale regular locks are replaced without ever vacating the lock path; legacy directories are migrated only while a recovery claim makes competing acquisitions fail closed. Ownership release moves a candidate aside, verifies its token, and restores a replacement without clobbering it.
Regression coverage
fsyncfailure.Verification
pnpm run typespnpm run formatpnpm run buildpnpm pack --pack-destination <temporary-directory>andpnpm run check:package <tarball>— installed-package import, CLI, and all 95 bundled plugin files verified.bun test --timeout 30000 ./tests-ts/multiscan.test.ts— 19 passed.bun test --timeout 30000 --rerun-each=10 --test-name-pattern='serializes supervisors while recovering stale file and legacy locks' ./tests-ts/multiscan.test.ts— 10 repeated contention runs passed.pnpm run test— 720 passed, 5 expected platform/integration skips, 0 failed.git diff --check.