fix: publish multiscan locks atomically - #98
Conversation
|
@mldangelo-oai @ianw-oai |
|
Thank you for the original multiscan locking work. The follow-up in #196 preserves the contributor author and the same stable patch after rebasing, and is the working branch for #102. Its recovery/ownership race findings still need to be addressed, so neither branch should be merged yet; keeping this PR open preserves the original contribution and context. |
|
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: 558ad1b992
ℹ️ 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".
| try { | ||
| await rename(stale, path); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; |
There was a problem hiding this comment.
Do not overwrite a newly acquired lock during restoration
On POSIX, rename(stale, path) replaces an existing destination rather than raising EEXIST. During lock handoff, one contender can move a newly acquired lock after an earlier readLockOwner raced with the previous owner's unlink; if another contender then acquires .lock before this restoration, the rename silently overwrites that contender's lock. Both successful acquirers can consequently run against the same output directory and corrupt its ledger or artifacts. Restore the moved lock with a no-clobber operation instead of relying on rename to report EEXIST.
Useful? React with 👍 / 👎.
| if ( | ||
| error instanceof SyntaxError || | ||
| (error as NodeJS.ErrnoException).code === "ENOENT" | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
Delay reclaiming incomplete legacy lock files
When an older supervisor starts concurrently, its legacy sequence creates the .lock directory before writeFile finishes owner.json. If this reader observes the owner file after it is opened but before its JSON write completes, the SyntaxError is immediately treated as a stale lock and the directory can be renamed and removed. The older process retains its open descriptor, can finish successfully and believe it owns the lock, while this process publishes its own lock, allowing two campaigns to mutate the same output directory. Treat a freshly missing or malformed legacy owner as an in-progress publication and retry or apply a grace period before reclaiming it.
Useful? React with 👍 / 👎.
| try { | ||
| await link(pending, path); | ||
| return async () => await releaseLock(path, token); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; |
There was a problem hiding this comment.
Avoid requiring hard-link support for output directories
When --output-dir is on a filesystem that does not support hard links, such as FAT/exFAT or a mount that does not expose them, link(pending, path) fails with EPERM or ENOTSUP; this loop handles only EEXIST, so every bulk scan aborts before starting. The previous directory-based lock worked on these filesystems, and the CLI otherwise accepts arbitrary output paths on its supported operating systems. Use an atomic locking strategy with a fallback that does not require hard-link support.
Useful? React with 👍 / 👎.
Maintainer tracking
Related to #102. The follow-up in #196 preserves this original contributor-authored change.
Summary
Make multiscan lock publication atomic and recover incomplete or malformed legacy locks.
Previously, lock acquisition performed two separate operations:
.lock/;.lock/owner.json.A crash between those operations left a visible lock without ownership metadata. Subsequent campaigns attempted to read and parse
owner.jsonunconditionally, permanently preventing automatic recovery.
Truncated or structurally invalid owner files caused the same problem.
Changes
Atomic lock publication
The supervisor now:
fsync;.lock;The published owner record contains:
Because hard-link creation has no-clobber semantics, only one contender can publish
.lock. The lock never becomes visible with partiallywritten ownership metadata.
Crash behavior is now:
.lockexists, so the next campaign can start;.lockcontains a complete owner record and can be recovered using the recorded PID.Legacy lock compatibility
Recovery continues to recognize the previous directory layout: