fix: recover multiscan locks with missing or malformed owner metadata - #170
Open
alarcritty wants to merge 1 commit into
Open
fix: recover multiscan locks with missing or malformed owner metadata#170alarcritty wants to merge 1 commit into
alarcritty wants to merge 1 commit into
Conversation
Bulk scan creates its lock in two steps: create the .lock directory, then write owner.json inside it. A crash between those steps left a lock that every later run tried to read unconditionally, so recovery failed forever with ENOENT until the directory was deleted by hand. Truncated or structurally invalid owner files failed the same way. Treat missing, malformed, or invalid owner metadata as a stale lock and reclaim it through the existing rename-aside path, mirroring the credential-home lock recovery in runtime.ts. A lock modified within the last 30 seconds is still treated as an active supervisor so a concurrent process that is mid-publication is not stolen from, and EPERM from the liveness probe now reports an active supervisor instead of crashing. Fixes openai#102
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #102.
Bulk scan creates its lock in two steps: create
<output>/.lock, then write<output>/.lock/owner.json. A process kill, machine restart, or write failure between those steps leaves a lock that every later run tries to read unconditionally, so acquisition fails forever with:Truncated or malformed
owner.jsoncontents fail the same way throughJSON.parse. The only escape was deleting.lockby hand. The existing recovery path handled a valid owner file with a dead PID, but not missing or invalid owner metadata.Changes
acquireLockinsrc/multiscan.tsnow routes contention through arecoverStaleLockhelper that mirrors the credential-home lock recovery already shipped insrc/runtime.ts(recoverStaleCredentialHomeLock):owner.json, malformed JSON, or owner metadata without a numericpidno longer crash acquisition. The lock is treated as incomplete and reclaimed through the existing rename-aside path.owner.json, a window the previous code left unguarded.EPERMfrom theprocess.kill(pid, 0)liveness probe now reports an active supervisor instead of crashing, matching the runtime lock.ESRCHstill means the owner is dead and the lock is reclaimed.ENOENTon rename) retries acquisition instead of failing, and stale quarantine directories are removed withforce: trueso concurrent recovery attempts do not race each other into an error.The owner file format is unchanged, so locks written by older versions are still recognized.
Relationship to #98
#98 proposes a broader redesign of this lock (atomic hard-link publication with ownership tokens). This change instead applies the recovery pattern the codebase already uses for the credential-home lock, keeping the directory-based format and the existing acquisition semantics. If maintainers prefer the redesign, this can serve as the minimal interim fix or be superseded.
Tests
Two new tests in
tests-ts/multiscan.test.ts:recovers locks with missing or malformed owner metadata: iterates the three shapes from the issue (empty.lockdirectory, truncated'{"pid":', and'{"pid":"invalid"}'), ages the lock past the publication grace window, and asserts each run completes, removes the lock, and leaves no.lock.stale-residue.treats a fresh lock without owner metadata as an active supervisor: a just-created empty.lockis reported as an active supervisor instead of crashing with ENOENT.Both tests fail against the previous implementation with the exact
ENOENTerror from the issue, and pass with this change. The existing exclusive-lock and dead-owner recovery tests continue to pass.Verification:
pnpm run typespnpm run test(708 pass, 0 fail)pnpm run formatpnpm run build