fix(site-memory): stop a stale file lock from being stolen while the owner is still alive - #496
Open
Agnik47 wants to merge 1 commit into
Open
fix(site-memory): stop a stale file lock from being stolen while the owner is still alive#496Agnik47 wants to merge 1 commit into
Agnik47 wants to merge 1 commit into
Conversation
…owner is still alive breakIfAbandoned() broke a lock whenever it was older than staleMs, regardless of whether the owning process was still running its critical section. A same-host owner confirmed alive by process.kill(pid, 0) is now decisive: staleness alone never steals its lock, only a confirmed-dead owner or an unverifiable (cross-host) owner does. Fixes agentrhq#432 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qEzPWwgbBFci2RcjPAhMn
Contributor
🟠 Maintainer review suggested — low confidenceThe automated review could not reach a fully supported conclusion. Limitations
This review is advisory and does not block merging. |
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
1 participant
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.
Bug
breakIfAbandoned()insrc/site-memory/file-lock.tsdecided a lock was abandoned using only two signals: the lock file's mtime is older thanstaleMs, OR the owning pid is confirmed dead. It did not check whether the owning process was actually still alive and working.If a critical section legitimately runs longer than
staleMs(big file, slow disk, loaded machine), a second process concludes the first one crashed, deletes its lock file, and starts its own critical section while the first is still running. Both writers race the same file, and whicheverrename()lands last silently discards the other's work.Fix
breakIfAbandoned()now checks owner liveness first: if the owning pid is checkable and confirmed alive on the same host (process.kill(pid, 0)), the lock is never broken on staleness alone. This is decisive, not just one of two OR'd signals. Locks whose owner is confirmed dead, or whose owner can't be checked (cross-host), still fall back to the existing stale-timeout behavior.Testing
Added two regression tests in
file-lock.test.ts:does not steal a stale lock from a same-host owner that is still alive— a backdated lock file owned by a live pid must still reportSITE_MEMORY_BUSYrather than being silently taken over.does not let two critical sections overlap when one runs longer than staleMs— reproduces the exact race from the issue: twowithFileLockcalls where the first's section outlivesstaleMs.Both fail against the pre-fix code and pass with the fix. Full
file-lock.test.tssuite: 9/9 passing.npm run typecheckclean.Fixes #432
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_018qEzPWwgbBFci2RcjPAhMn
🤖 Generated with Claude Code
https://claude.ai/code/session_018qEzPWwgbBFci2RcjPAhMn