Stop two tests racing the runner: the legacy-lock reclaim and the inherited-child chain - #9
Merged
Merged
Conversation
It failed once in a full-suite run and passes ten times on its own: a 200ms budget was the whole problem. acquireLockSync() checks its deadline only after a failed attempt, so on a loaded runner a single stalled attempt expires the budget in the gap between the reclaim that just succeeded and the retry that would have taken the lock. The property under test is that a legacy lock whose owner PID looks alive is reclaimed once it ages past staleMs, not how many milliseconds that takes. A 5s budget against a 30s staleMs still proves it — and reverting the reclaim branch still fails the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
CI caught the second half of the same problem: "broker rolls back child threads inherited through a failed provisional claim" went red with "delayed grandchild thread was not created". The fixture scheduled the child 100ms after turn/start and created the grandchild only if the resume had already set nestedSubagentRequested by the time that timer fired. So the test passed only when the client got from turn/start through a disconnect, an unsubscribe wait and a fresh connection to the resume in under 100ms. Reproduced exactly by putting a 300ms delay in that gap. with-resume-inherited-subagent drives the whole chain off the resume itself: child, then grandchild under it, then the resume failure — all while that claim is open, which is the scenario the test is about. With it, the test passes even with a 1500ms delay in that gap, and it still fails when the inherited threads are dropped from the rollback. The other two with-delayed-subagent tests never resume, so they keep the old behavior unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg
Edo771977
changed the title
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
2 participants
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.
Two tests that asserted timing instead of causality. Both were found by running the suite, not by guessing: one on
mainafter the Windows batch merged, the other by this PR's own CI run.328/328, full suite run twice,
tscclean.1.
stale legacy lock without process identity does not follow a reused PID foreverWent red once in a full-suite run on
main, passes ten times out of ten on its own.acquireLockSync()checks its deadline after a failed attempt, so a single stalled attempt on a loaded runner expires a 200ms budget in the gap between the reclaim that just succeeded and the retry that would have taken the lock.The property under test is that a legacy lock whose owner PID still looks alive is reclaimed once it ages past
staleMs— not how many milliseconds that takes. The budget goes to 5s against a 30sstaleMs, so returning a successor at all still proves the reclaim happened on the spot rather than by aging out. Same fix, same reasoning, as the sibling test a few lines above; this one had been left behind, and tighter.Still discriminating: removing the legacy-reclaim branch from
reclaimAbandonedLock()fails the test.2.
broker rolls back child threads inherited through a failed provisional claimThis PR's first CI run went red on it: "delayed grandchild thread was not created".
The fixture scheduled the child 100ms after
turn/start, and created the grandchild only if the resume had already setnestedSubagentRequestedby the time that timer fired. So the test passed only when the client got fromturn/startthrough a disconnect, an unsubscribe wait and a fresh connection to the resume in under 100ms — which a loaded runner does not promise. Reproduced exactly by putting a 300ms delay in that gap.The new
with-resume-inherited-subagentbehavior drives the whole chain off the resume itself: child, then grandchild under it, then the resume failure — all while that claim is open, which is the scenario the test is about. Nothing in it depends on how fast the client got there.Verified: passes with a 1500ms delay in that gap where the old version failed at 300ms; the whole file passes five runs in a row; and it still fails when the inherited threads are dropped from the rollback (
claim.inheritedThreadIdsremoved fromrollBackClaim()). The other twowith-delayed-subagenttests never resume, so their behavior is unchanged.Neither failure is a regression from the Windows batch — the first touches locking, the second broker subscriptions, and this PR's diff before these fixes was one test file.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UXfvnjSC72HsM6EEPVt2Tg