fix(runtime): reclaim credential-home locks whose owner names no process - #229
Open
rohanpoudel2 wants to merge 2 commits into
Open
fix(runtime): reclaim credential-home locks whose owner names no process#229rohanpoudel2 wants to merge 2 commits into
rohanpoudel2 wants to merge 2 commits into
Conversation
`recoverStaleCredentialHomeLock` consulted `process.kill(pid, 0)` for any `owner.json` whose `pid` was a number. POSIX gives two of those numbers special meanings: 0 signals the caller's own process group and -1 every process it may signal, so both always succeed and always report a live owner. A fractional or out-of-range value makes `process.kill` throw `ERR_INVALID_ARG_TYPE`, which is neither ESRCH nor EPERM and was rethrown raw out of a public API. Because the age check sits in the `else` branch, a lock naming any of these values was also exempt from it, so `acquireCodexSecurityCredentialHomeLock` waited on it forever at a 25 ms poll with no message and no timeout. Only consult `process.kill` for a positive safe integer. Anything else is an owner that cannot be identified, and is now treated like a missing one, so the existing 30 s age check reclaims the lock. This does not address a genuinely reused pid, which needs a heartbeat rather than a liveness probe and is a larger design change. That part is described in the issue. Refs openai#228
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.
Refs #228
Problem
recoverStaleCredentialHomeLockconsultedprocess.kill(pid, 0)for anyowner.jsonwhosepidwas a number:POSIX gives two of those numbers special meanings —
0signals the caller's own process group and-1every process it may signal — so both always succeed and always report a live owner. A fractional or out-of-range value makesprocess.killthrowERR_INVALID_ARG_TYPE, which is neitherESRCHnorEPERMand was rethrown raw out of a public API.The age check sits in the
elsebranch, so a lock naming any of these values was also exempt from it.acquireCodexSecurityCredentialHomeLocktherefore waited on such a lock forever, at a 25 ms poll, with no message and no timeout — while every other stale path (missing or corruptowner.json) has a 30 s escape hatch.Measured against the public API, with a 1.5 s abort as the only way out:
Change
Consult
process.killonly for a positive safe integer. Anything else is an owner that cannot be identified, and is now treated exactly like a missing one, so the existing 30 s age check reclaims the lock. After the change the same three cases acquire in 2 ms.The
EPERMhandling is unchanged and still means "the process exists but we may not signal it".Scope: what this deliberately does not fix
A genuinely reused pid is still indistinguishable from the original owner, so a lock left behind by a
SIGKILLed scan is still never reclaimed. I left that out on purpose:#runbefore runtime initialization, released in the outerfinally), and scans legitimately run well over an hour (Standard full-repository scan shows only generic heartbeat for 40+ minutes — expected behavior? #70, Detect and warn from HEAD drifting earlier #164). A plain absolute age ceiling would therefore let one scan steal a running scan's lock — strictly worse than the current behaviour.#228 documents that part, including why it is close to guaranteed in the shipped container (
Dockerfileputs the state dir under the/outputbind mount, andcompose.yamlsetsinit: true, so container pids restart from 1 each run and a leftover low pid collides). Happy to implement the heartbeat if you tell me which shape you want.Verification
New test
recovers credential-home locks whose owner names no processwalks[0, -1, 0.5, 2 ** 53], ages the lock past the 30 s threshold, and bounds the acquisition with anAbortControllerso a regression fails the test in 5 s instead of hanging it.Before:
AbortError: The operation was aborted.—0 pass / 1 failin 5002 ms.After:
1 pass / 0 failin 54 ms.Full suite: 718 pass / 5 skip / 0 fail (717 baseline plus the new test).
pnpm run typesandpnpm run formatare clean.