fix(dispatch): close scope-check control bead before converging its scope (gc-gfoc7) - #158
Merged
Conversation
added 3 commits
August 23, 2026 16:15
…ts scope (gc-gfoc7) A graph.v2 workflow never finished. Its final step's scope-check bead stayed open after its blocker closed, so the body scope stayed open, so cleanup-worktree and workflow-finalize were never ready and the worktree was never cleaned. This is the long-standing graph.v2 husk accumulation. Root cause is an ordering cycle, not a discovery failure. The scope body carries a "blocks" dependency on every step's scope-check control bead (the graph builder's rewriteGraphStepRefs redirects downstream refs from the step to its scope-check, and the body is one of those downstream refs). The final scope-check is therefore simultaneously the bead that closes the body and the last blocker standing in the body's way. processScopeCheck converged the scope first and closed itself second, so the body close was refused with "cannot close blocked issue: <body> is blocked by [<control>]", the error propagated before the control bead was ever closed, and the dispatcher classified it transient and retried forever. One workflow logged ~240k such failures. Only the final step of a scope showed the symptom: for every earlier step hasOpenScopeMembers still reports members open, which takes the "continue" branch that just closes the control bead and never touches the body. That is why 4 of 5 scope-checks in a molecule closed normally and only submit hung — and why the closed-scope-check counts skewed 6/6/6/6 against 4 for submit. The fix closes the control bead before calling closeScopeAsPassed/abortScope in all three terminal branches, which is already the pattern every other terminal path in the dispatcher uses (close self, then reconcile the enclosing scope — see processRetryControl and recordControllerSpawnError). Doc comments on closeScopeAsPassed and abortScope now state the precondition. Verified upstream/main carries the identical ordering in both scope-pass branches, so this is an upstream defect rather than fork-local; the bead is tagged upstream_pr_candidate. Tests: the deadlock escaped CI because internal/dispatch tests run against MemStore, which closes a blocked bead happily. strictCloseStore already existed to mirror bd's guard but was referenced by zero tests, and it only guarded Close() while updateMetadataAndClose closes via Update(Status). Both gaps are fixed: strictCloseStore now guards the update path too, and a new TestProcessScopeCheckClosesScopeWhenBodyBlocksOnControl runs the same graph as the existing success test against it. That test fails on the pre-fix tree with the exact production error string and passes after. TestProcessScopeCheckKeepsControlOpenIfBodyCloseoutFails asserted the opposite ordering — control stays open "so the dispatcher can retry body closeout". That invariant is unsatisfiable, since holding the control bead open is precisely what makes every retry fail; it is renamed to TestProcessScopeCheckSurfacesBodyCloseoutFailure and now pins that a genuine closeout failure is surfaced rather than retried forever. Follow-up not taken here: if body convergence fails after the control bead closes, nothing re-drives it (previously it retried, though it could never succeed). Surfacing the error is strictly better than the infinite spin, but a durable convergence sweep for scope bodies whose members are all closed is worth filing separately.
…gc-gfoc7) Self-review catch on the ordering fix. The doc comment added to abortScope claimed the caller must always have closed its own control bead first, matching closeScopeAsPassed. That overstates the requirement and misdescribes the second caller. abortScope begins with skipOpenScopeMembers, which closes every open scope member that is not body/teardown/spec — open scope-check controls included — in dependency order. That pass is what unblocks the body. The single bead exempt from it is the one named by traceID (skipControlID), so only that bead is the caller's responsibility. This matters for reconcileTerminalScopedMember, which reaches abortScope on its failure path without any remaining-open check. Under the overstated wording that path looks broken; it is in fact correct, because the sibling controls it does not own are closed by the skip pass. No behavior change — comment only.
…gc-gfoc7) The root-cause analysis for this bead was living only in bead notes, which docs/file-structure.md (gc-toolkit pack) explicitly disallows for durable documents — bead comments are operational state, not the record, and this one needs to be readable after the bead closes. Files it as specs/gc-gfoc7/ per the bead-keyed local tier: it records what was found and decided while working this bead, not an authoritative "what is true now" topic that someone owns keeping current. Worth keeping rather than summarizing, because two of the three findings are about how the investigation goes wrong: - The two hypotheses that had to be ruled out first (discovery, metadata shape), including the bare-bd-vs-gc-bd store-resolution red herring that made the discovery theory look confirmed. - Why CI could not catch it: the fake store is more permissive than bd, the existing success test already builds the exact deadlock graph, and the strictCloseStore written to mirror bd's guard was referenced by zero tests and guarded the wrong method. - The counting trap: scope-check beads are absent from bd list entirely, so sizing this class by listing returns a false zero. Also records the two framings the evidence disproves (the retry invariant asserted by the old test, and gc-spa04's "blocked-by-design"), so neither gets re-adopted from the bead titles alone.
johnzook
approved these changes
Aug 24, 2026
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…cope (gc-gfoc7) (#158) * fix(dispatch): close the scope-check control bead before converging its scope (gc-gfoc7) A graph.v2 workflow never finished. Its final step's scope-check bead stayed open after its blocker closed, so the body scope stayed open, so cleanup-worktree and workflow-finalize were never ready and the worktree was never cleaned. This is the long-standing graph.v2 husk accumulation. Root cause is an ordering cycle, not a discovery failure. The scope body carries a "blocks" dependency on every step's scope-check control bead (the graph builder's rewriteGraphStepRefs redirects downstream refs from the step to its scope-check, and the body is one of those downstream refs). The final scope-check is therefore simultaneously the bead that closes the body and the last blocker standing in the body's way. processScopeCheck converged the scope first and closed itself second, so the body close was refused with "cannot close blocked issue: <body> is blocked by [<control>]", the error propagated before the control bead was ever closed, and the dispatcher classified it transient and retried forever. One workflow logged ~240k such failures. Only the final step of a scope showed the symptom: for every earlier step hasOpenScopeMembers still reports members open, which takes the "continue" branch that just closes the control bead and never touches the body. That is why 4 of 5 scope-checks in a molecule closed normally and only submit hung — and why the closed-scope-check counts skewed 6/6/6/6 against 4 for submit. The fix closes the control bead before calling closeScopeAsPassed/abortScope in all three terminal branches, which is already the pattern every other terminal path in the dispatcher uses (close self, then reconcile the enclosing scope — see processRetryControl and recordControllerSpawnError). Doc comments on closeScopeAsPassed and abortScope now state the precondition. Verified upstream/main carries the identical ordering in both scope-pass branches, so this is an upstream defect rather than fork-local; the bead is tagged upstream_pr_candidate. Tests: the deadlock escaped CI because internal/dispatch tests run against MemStore, which closes a blocked bead happily. strictCloseStore already existed to mirror bd's guard but was referenced by zero tests, and it only guarded Close() while updateMetadataAndClose closes via Update(Status). Both gaps are fixed: strictCloseStore now guards the update path too, and a new TestProcessScopeCheckClosesScopeWhenBodyBlocksOnControl runs the same graph as the existing success test against it. That test fails on the pre-fix tree with the exact production error string and passes after. TestProcessScopeCheckKeepsControlOpenIfBodyCloseoutFails asserted the opposite ordering — control stays open "so the dispatcher can retry body closeout". That invariant is unsatisfiable, since holding the control bead open is precisely what makes every retry fail; it is renamed to TestProcessScopeCheckSurfacesBodyCloseoutFailure and now pins that a genuine closeout failure is surfaced rather than retried forever. Follow-up not taken here: if body convergence fails after the control bead closes, nothing re-drives it (previously it retried, though it could never succeed). Surfacing the error is strictly better than the infinite spin, but a durable convergence sweep for scope bodies whose members are all closed is worth filing separately. * docs(dispatch): state abortScope's actual control-bead precondition (gc-gfoc7) Self-review catch on the ordering fix. The doc comment added to abortScope claimed the caller must always have closed its own control bead first, matching closeScopeAsPassed. That overstates the requirement and misdescribes the second caller. abortScope begins with skipOpenScopeMembers, which closes every open scope member that is not body/teardown/spec — open scope-check controls included — in dependency order. That pass is what unblocks the body. The single bead exempt from it is the one named by traceID (skipControlID), so only that bead is the caller's responsibility. This matters for reconcileTerminalScopedMember, which reaches abortScope on its failure path without any remaining-open check. Under the overstated wording that path looks broken; it is in fact correct, because the sibling controls it does not own are closed by the skip pass. No behavior change — comment only. * docs(specs): record the gc-gfoc7 scope-check deadlock investigation (gc-gfoc7) The root-cause analysis for this bead was living only in bead notes, which docs/file-structure.md (gc-toolkit pack) explicitly disallows for durable documents — bead comments are operational state, not the record, and this one needs to be readable after the bead closes. Files it as specs/gc-gfoc7/ per the bead-keyed local tier: it records what was found and decided while working this bead, not an authoritative "what is true now" topic that someone owns keeping current. Worth keeping rather than summarizing, because two of the three findings are about how the investigation goes wrong: - The two hypotheses that had to be ruled out first (discovery, metadata shape), including the bare-bd-vs-gc-bd store-resolution red herring that made the discovery theory look confirmed. - Why CI could not catch it: the fake store is more permissive than bd, the existing success test already builds the exact deadlock graph, and the strictCloseStore written to mirror bd's guard was referenced by zero tests and guarded the wrong method. - The counting trap: scope-check beads are absent from bd list entirely, so sizing this class by listing returns a false zero. Also records the two framings the evidence disproves (the retry invariant asserted by the old test, and gc-spa04's "blocked-by-design"), so neither gets re-adopted from the bead titles alone. --------- Co-authored-by: refinery costing <refinery@local>
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…cope (gc-gfoc7) (#158) * fix(dispatch): close the scope-check control bead before converging its scope (gc-gfoc7) A graph.v2 workflow never finished. Its final step's scope-check bead stayed open after its blocker closed, so the body scope stayed open, so cleanup-worktree and workflow-finalize were never ready and the worktree was never cleaned. This is the long-standing graph.v2 husk accumulation. Root cause is an ordering cycle, not a discovery failure. The scope body carries a "blocks" dependency on every step's scope-check control bead (the graph builder's rewriteGraphStepRefs redirects downstream refs from the step to its scope-check, and the body is one of those downstream refs). The final scope-check is therefore simultaneously the bead that closes the body and the last blocker standing in the body's way. processScopeCheck converged the scope first and closed itself second, so the body close was refused with "cannot close blocked issue: <body> is blocked by [<control>]", the error propagated before the control bead was ever closed, and the dispatcher classified it transient and retried forever. One workflow logged ~240k such failures. Only the final step of a scope showed the symptom: for every earlier step hasOpenScopeMembers still reports members open, which takes the "continue" branch that just closes the control bead and never touches the body. That is why 4 of 5 scope-checks in a molecule closed normally and only submit hung — and why the closed-scope-check counts skewed 6/6/6/6 against 4 for submit. The fix closes the control bead before calling closeScopeAsPassed/abortScope in all three terminal branches, which is already the pattern every other terminal path in the dispatcher uses (close self, then reconcile the enclosing scope — see processRetryControl and recordControllerSpawnError). Doc comments on closeScopeAsPassed and abortScope now state the precondition. Verified upstream/main carries the identical ordering in both scope-pass branches, so this is an upstream defect rather than fork-local; the bead is tagged upstream_pr_candidate. Tests: the deadlock escaped CI because internal/dispatch tests run against MemStore, which closes a blocked bead happily. strictCloseStore already existed to mirror bd's guard but was referenced by zero tests, and it only guarded Close() while updateMetadataAndClose closes via Update(Status). Both gaps are fixed: strictCloseStore now guards the update path too, and a new TestProcessScopeCheckClosesScopeWhenBodyBlocksOnControl runs the same graph as the existing success test against it. That test fails on the pre-fix tree with the exact production error string and passes after. TestProcessScopeCheckKeepsControlOpenIfBodyCloseoutFails asserted the opposite ordering — control stays open "so the dispatcher can retry body closeout". That invariant is unsatisfiable, since holding the control bead open is precisely what makes every retry fail; it is renamed to TestProcessScopeCheckSurfacesBodyCloseoutFailure and now pins that a genuine closeout failure is surfaced rather than retried forever. Follow-up not taken here: if body convergence fails after the control bead closes, nothing re-drives it (previously it retried, though it could never succeed). Surfacing the error is strictly better than the infinite spin, but a durable convergence sweep for scope bodies whose members are all closed is worth filing separately. * docs(dispatch): state abortScope's actual control-bead precondition (gc-gfoc7) Self-review catch on the ordering fix. The doc comment added to abortScope claimed the caller must always have closed its own control bead first, matching closeScopeAsPassed. That overstates the requirement and misdescribes the second caller. abortScope begins with skipOpenScopeMembers, which closes every open scope member that is not body/teardown/spec — open scope-check controls included — in dependency order. That pass is what unblocks the body. The single bead exempt from it is the one named by traceID (skipControlID), so only that bead is the caller's responsibility. This matters for reconcileTerminalScopedMember, which reaches abortScope on its failure path without any remaining-open check. Under the overstated wording that path looks broken; it is in fact correct, because the sibling controls it does not own are closed by the skip pass. No behavior change — comment only. * docs(specs): record the gc-gfoc7 scope-check deadlock investigation (gc-gfoc7) The root-cause analysis for this bead was living only in bead notes, which docs/file-structure.md (gc-toolkit pack) explicitly disallows for durable documents — bead comments are operational state, not the record, and this one needs to be readable after the bead closes. Files it as specs/gc-gfoc7/ per the bead-keyed local tier: it records what was found and decided while working this bead, not an authoritative "what is true now" topic that someone owns keeping current. Worth keeping rather than summarizing, because two of the three findings are about how the investigation goes wrong: - The two hypotheses that had to be ruled out first (discovery, metadata shape), including the bare-bd-vs-gc-bd store-resolution red herring that made the discovery theory look confirmed. - Why CI could not catch it: the fake store is more permissive than bd, the existing success test already builds the exact deadlock graph, and the strictCloseStore written to mirror bd's guard was referenced by zero tests and guarded the wrong method. - The counting trap: scope-check beads are absent from bd list entirely, so sizing this class by listing returns a false zero. Also records the two framings the evidence disproves (the retry invariant asserted by the old test, and gc-spa04's "blocked-by-design"), so neither gets re-adopted from the bead titles alone. --------- Co-authored-by: refinery costing <refinery@local>
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.
Summary
processScopeCheck: the scope body carries ablocksdep on every step's scope-check control bead, so the final step's scope-check was both the bead that closes the body and the body's last remaining blocker. Converging the scope first meant the body close was refused (cannot close blocked issue), so the control bead never closed and the dispatcher retried forever (~239k logged failures across two stranded workflows).processScopeCheck— matching the close-self-then-reconcile pattern every other terminal path already uses.strictCloseStore, previously referenced by zero tests).specs/gc-gfoc7/scope-check-deadlock.md.upstream_pr_candidate).Test plan
make test-fast-parallel: 10/10 jobs ok (rc=0)go vet ./...clean