CompletionResolver: 9 sites maintain the CAS-remove invariant, only 2 are asserted (from the #577 sweep) #581
Closed
opened 2026-09-12 15:05:24 +02:00 by ltms
·
3 comments
No Branch/Tag Specified
main
worker/fleetd-612-unita-87807e-1
worker/612-b3-mcpwirings-da2b58-3
worker/612-b2-cb185-176d3a-2
worker/612-b1-completion-457459-1
worker/612-agaps-73a926-2
worker/608-sleeps-3a64ff-3
worker/621-b4520b-1
worker/618-b83894-2
worker/fleetd-615-e05481-5
worker/lead-autocompact-5f1ab2-3
worker/fleetd-613-f85deb-3
worker/fleetd-608-flaky-nudge-test-d0c2d1-3
worker/lead-context-gauge-ad404f-1
worker/gauge-wiring-9158c1-4
worker/redeploy-slowstart-ead0e5-5
worker/charter-bytes-13668c-6
worker/rollover-outcome-291483-2
worker/589-f64303-2
worker/593-1a8025-5
worker/589-fcd2aa-1
worker/568-9fdaa2-3
worker/571-attempted-outcome-5739f7-2
worker/581-completionresolver-cas-sites-0542b7-6
worker/562-loop-health-wiring-test-99611c-5
worker/562-surface-loop-health-7df5cc-4
worker/575-waiter-cleanup-sites-62ad80-1
worker/572-answer-lock-release-46a9ae-5
worker/567-probe-channel-leak-a38fc5-6
worker/551-record-before-send-7cbf56-1
worker/561-listener-fanout-survives-a-throw-61d538-2
worker/555-redeploy-main-flow-seam-65c2f5-2
worker/556-injector-owns-registration-e027a5-1
worker/552-post-restart-mktemp-abort-bc2672-4
worker/553-onstatus-completion-leak-0da881-2
worker/550-shasum-linux-196132-1
worker/538-loop-dies-on-error-4a5eeb-6
worker/426-health-coverage-ef1fd4-4
worker/504-failed-reported-clean-3cfd66-3
worker/537-capturedlog-close-e4c437-2
worker/459-broken-link-targets-cadc17-5
worker/535-appender-leak-fe74c1-1
worker/512-part2-shutdown-detection-434701-9
worker/529-logger-level-sweep-2a5533-8
worker/528-drain-gate-call-site-5de83d-7
charter/forge-mcp-vs-token
worker/521-swap-guard-unpinned-28e931-5
worker/519-probe-test-harness-d25ab8-4
worker/525-logger-level-leak-1b4eb0-6
worker/518-fleetmcp-resolver-wiring-8ef96c-1
worker/512-drain-complete-line-7edd71-3
worker/517-abort-branch-and-jar-id-41b641-2
worker/500-9e52c9-3
worker/509-4912f4-2
worker/511-9a4b23-1
worker/493-479f45-2
worker/505-03f8b2-1
worker/492-followup-detect-unclear
worker/501-a31fa0-7
worker/498-451d1c-5
worker/494-1015ce-2
worker/492-209647-1
worker/489-001902-2
worker/480-relative-handover-path-906323-1
worker/480-b-handover-skill-45bf1f-5
worker/474-followup-source-pin-f54a55-17
worker/474-charter-check-on-reload-f54a55-17
worker/466-quarantine-repeatcount-report
worker/393-opencode-skill-seeding-71854b-13
worker/469-canonical-tool-names-2a472a-16
worker/466-quarantine-escalation-5ae9c1-15
worker/446-hot-exhausted-pattern-0af580-6
worker/464-charter-tool-name-guard-a85635-12
worker/463-listfleet-default-fails-open-f1c76c-11
worker/458-invariant-5-by-purpose-862f9a-10
worker/439-coordinator-row-gate-bc032a-8
worker/449-herdr-protocol-576015-4
worker/450-abstract-spawn-599e1c-5
worker/437-ack-refuses-177d91-1
worker/444-placement-window-feb56a-2
worker/440-helddurable-derived-d462d7-13
worker/425-rework-placement-resolve-c58ba1-9
worker/421-lead-peek-held-msgs-cdbad2-10
worker/435-fixed-policy-cap-fe11de-12
worker/422-gate-state-observability-9e79d6-11
worker/431-memberregistry-live-readers-cdbad2-10
worker/424-architect-slot-hot-038b41-7
worker/422-model-gate-spawn-c29f48-6
worker/425-default-profile-live-f55534-8
worker/415-coverage-wording-2cbf9c-5
worker/416-3ad1da-1
worker/418-588283-3
worker/deterministic-stamp-race-409-3cb7b6-10
worker/armed-reads-live-config-404-ed931f-9
worker/reply-peer-refusal-391-5a34bd-7
worker/models-allowlist-aa9e9b-3
worker/ttl-stamp-race-399-f1122f-8
worker/scrub-receipt-400-316b3e-5
worker/exhaustion-detection-395-105105-6
worker/scrub-abort-394-316b3e-5
fix/scrub-uid-abort
worker/task-scrub-517574-2
worker/t386-clock-bd5b78-4
worker/t384-scrub-813790-5
worker/t381-cc-748314-2
worker/t373-336973-2
worker/t365-3920c5-3
worker/t358-6e989b-1
worker/t355-8b321c-1
worker/fleetd-369-hermetic-git-tests-e8b19a-3
worker/fleetd-368-stale-lead-binding-f5682e-2
worker/fleetd-360-deploy-units-0d3793-1
worker/359-dead-lead-tabs-f1253b-4
worker/362-worktree-skills-c03e51-3
worker/361-coord-visibility-655144-1
362-plugin-visibility-and-drift
worker/errscan-bed2ca-2
worker/amqp-log-identity-bed2ca-2
worker/withdefaults-guard-561704
worker/sleepguard-82076d-1
worker/fd334-9ee1b6-5
worker/fd348-f1ab27-4
worker/fd335-a71c35-1
worker/fd342-174a17-2
worker/fd345-490d0f-3
worker/fleetd-337-5ec7d4-21
worker/fleetd-341-af5a6b-24
worker/fleetd-339-5ca0a2-23
worker/fleetd-338-83a4a1-22
worker/fleetd-333-281f46-18
worker/fleetd-329-11bdbb-16
worker/fleetd-330-2770fb-17
worker/fix-326-50506e-15
worker/fix-324-3e9bbf-14
worker/fix-323-b8287d-13
worker/fix-316b-bd0860-11
worker/fix-318-76ca36-9
worker/fix-317-486aec-8
worker/fix-315-ce47c5-6
worker/fix-307-275890-6
worker/fix-308-b4f664-7
worker/fix-309-ec3939-8
worker/fix-310-7a3974-9
worker/fix-302-52ad0e-9
worker/fix-298-ce1acb-8
worker/fix-297-66bd11-7
worker/fix-296-104622-6
worker/fix-293-bare-closetab-eb22b5-3
worker/fix-280-gone-ask-lapse-bca98e-2
worker/fix-290-reapidle-guard-coverage-9b0dd1-1
worker/fix-285-trust-seed-8f3565-10
worker/fix-284-backend-error-seat-85912c-11
worker/fix-282-chained-ask-e6d0bb-8
worker/fix-283-teardown-leaks-f40dfa-9
worker/fix-281-pin-handler-actions-4921ac-7
worker/audit-rendezvous-lifecycle-d072ae-2
worker/audit-health-placement-1a2476-6
worker/audit-teardown-exits-e207a5-3
worker/audit-launcher-asymmetry-27e370-4
worker/audit-rest-authz-6ca53c-5
worker/investigate-275-abandon-asking-fdef52-8
worker/fix-274-worktree-leak-b0095d-7
worker/fix-273-exhausted-pattern-9665b5-6
worker/fleetd-267-model-check-bd8068-1
worker/fleetd-131-archunit-18b834-7
worker/fleetd-266-sshagent-rename-a014ff-6
worker/fleetd-184-uid-claim-8e1f31-4
worker/fleetd-184-warn-b381ee-10
worker/fleetd-184-docs-be1d12-9
worker/fleetd-257-9bf010-7
worker/fleetd-103-23a113-6
worker/fleetd-247-342356-5
worker/fleetd-116-04dea8-4
worker/fleetd-252-a830e0-3
worker/fleetd-111-7e8673-9
worker/fleetd-155c-f8ef4b-8
worker/fleetd-176-b928ca-3
worker/fleetd-249-7a7878-2
worker/cb248-composition-root-b-9acdf7-15
worker/cb148-envrc-default-fa6c82-12
worker/cb201-unit5-wiring-6c12e6-8
worker/cb241-fallback-echo-1175e9-11
worker/cb149-trust-dialog-2392a5-9
worker/cb134-148-overlay-visible-c9b986-10
worker/cb234-session-id-keyed-04e1fc-1
worker/cb201-unit3-nudge-abdf5c-6
worker/cb201-unit2-policy-c1102c-5
worker/cb201-unit4-outcome-a13bfa-7
worker/cb201-unit1-classifier-91b9b1-4
worker/cb201-227-refine-831980-3
worker/cb175-model-readback-0f085f-1
worker/cb222-charter-tmpdir-17f013-1
worker/cb226-architect-slot-race-cd3aa8-3
worker/cb224-worktree-root-group-024523-2
worker/cb-123-role-demotion-c600f7-2
worker/cb-219-opencode-roots-1f677e-1
worker/cb214-claude-session-id-b9eab4-4
worker/cb213-zdotdir-wrong-process-dd6de4-3
worker/cb211-exhaustion-classification-9546e0-2
worker/cb137-ambiguous-task-4df3d8-4
worker/cb209-agentsessionid-4dfdb6-2
worker/cb185-hostenvnames-2692b5-3
worker/cb206-opencode-sqlite-128718-2
worker/cb185-worktree-group-fc0c99-1
worker/cb-137-ask-ticket-e7760c-2
worker/cb-172-broker-uri-d36ae4-4
worker/cb-175-model-readback-76ead6-3
worker/cb-161-pane-ancestry-293510-1
worker/cb-164-rebase-885863-8
worker/cb-164-empty-scrape-false-success-1a80af-3
fix/cb-197-ticket-ttl-from-completion
worker/cb-189-remote-url-coverage-4692f3-1
worker/cb-185-blockers-027756-4
worker/cb-192-gap-log-11b631-2
worker/cb-633-fix-5f4396-3
worker/cb185-router-d6436d-3
worker/cb185-router-routing-gaps-9e9d33-3
worker/cb185-paneids-992586-2
worker/cb-633-allow-list-union-ed374b-1
worker/cb-157-credential-in-remote-url-496e44-2
worker/cb-641-health-herdr-evidence-8f1f54-6
worker/cb-640-health-msg-evidence-99c9cd-1
worker/cb-642-fleets-status-skill-bbbc40-5
cb-634-ide-mcp
worker/lead-comms-wiring-c014b9-7
worker/lead-mailbox-c19577-6
worker/autocompact-window-82bc2f-5
worker/cb-634-probe-18056f-4
worker/cb635-broker-urienv
worker/cb-632-config-retry-8e0efa-7
lead/cb-622e-claude-md
lead/cb-622-followup
worker/cb-622a-165dff-1
lead/cb-622d-opencode-mount
worker/cb-622b-717c67-2
worker/cb-622c-ab7759-3
worker/cb-617b2-20ca4b-3
worker/cb-617a-5c2f4a-1
worker/cb596-4e49ef-3
worker/cb586-10500c-1
worker/cb-606-b9343a-25
worker/cb604-1445f8-24
worker/cb582-477374-21
worker/cb584-8c2281-22
worker/cb600-e6b9a9-20
worker/cb602-ce257f-19
worker/cb601-b42837-18
worker/cb598-6c7ba7-17
worker/cb599-740fe4-16
worker/cb597-282224-15
worker/cb590fix-185e9a-10
worker/cb528-recovery-race
worker/cb594-96bead-8
worker/cb590-916766-2
worker/cb527-997d99-3
worker/cb592-env-leak-3cbf9c-1
worker/cb588-async-ticket-nudge-3218f7-5
worker/cb578b-9dcb13-6
worker/cb581-d24826-5
worker/m2-u5-ef8c42-15
worker/cb578a-516499-2
worker/cb576-01a04b-17
worker/cb579-lead-tab-acba06-20
worker/cb580-terminal-health-ed6058-21
worker/cb577-f36fdc-18
worker/cb573b-3db06f-16
worker/cb568c-f36fdc-18
worker/cb568-drop-cause-c3ac1c
worker/cb575-cancelled-notification-c3ac1c
worker/m4-sol-a2cbec-3
worker/cb574-async-ask-c3ac1c
worker/cb573-health-model-8ca857-14
worker/cb572-unknown-target-7f2e35-13
worker/u4-700706-9
worker/u3-b9fcb6-6
worker/u2-ef5b68-4
worker/u1-469dce-1-clean
worker/u1-469dce-1
worker/cb-564-health-events-70cf7e-2
worker/cb-565-recycle-drops-role-98e58f-3
worker/cb-563-missing-reply-df2866-1
worker/cb-562-readiness-gate-silent-6c23c9-3
worker/cb-560-architect-presence-da8155-1
worker/cb-561-architect-silent-off-a71cab-2
worker/cb-548-bind-architect-slot-fe1b8c-1
worker/parity-overlay-settings-5fb711-1
secrets-central-store
cb-559-hot-key-correction
cb-557-fleet-role-pools
worker/cb-553-maxload-explicit-spawn-305ee3-6
worker/cb-551-idle-lead-heartbeat-f1633c-1
worker/cb-544-drain-preserves-worktree-925fad-3
worker/cb-552-docs-sync-1cb9cf-4
worker/cb-548-rendezvous-guard-rebased
worker/cb-548-rendezvous-guard-116b53-10
worker/cb-548-authz-v2-586df6-8
worker/cb-548-authz-264363-5
salvage/cb-528b-codex-home
salvage/cb-528a-codex-launcher
CB-518-primary-flow
feature/peer-launcher-spi
cb-103-injector
v1.1.0
v1.0.0
Labels
Clear labels
blocked
needs-live-proof
ready-to-delegate
silent-default
Cannot start until something else lands. The body says what.
Merged and green, but never shown working on the running daemon. Not the same as done.
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: fleet/fleetd#581
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Found by the #577 per-site assertion sweep. Both survivors below were proven by mutation, not by reading. Severity 1 of the two findings that sweep returned.
The invariant
When a turn's completion resolves, the code must remove its own entry from
inFlightwith the compare-and-remove form:The class javadoc in
CompletionResolver.javanames the exact danger: if a later turn has already registered under the same target, a blindinFlight.remove(target)deletes the new turn's entry by mistake. That is the CB-116 bug this code exists to prevent.The gap
Measured with
grep -n "inFlight.remove"infleetd/src/main/java/dev/ltms/fleet/inject/CompletionResolver.java:register()/captureBaseline(), reached only whenwaiter == null. That is a different case and does not count against this invariant.Tests in
CompletionResolverTest.javathat guard "must not evict a successor's registration" — there are exactly 3, and they cover 2 sites:aSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration(531)aSupersededTurnsEchoedNoReportSubPathMustNotEvictItsSuccessorsRegistration(565)aSupersededTurnsFailMustNotEvictItsSuccessorsRegistration(600)7 of 9 sites have no test that would catch a regression to the unsafe form: 335, 405, 432, 505, 525, 549, 609.
What makes this the sharpest instance of the per-site rule so far: this is not a forgotten invariant. Each covering test's own comment spells out why the CAS form matters ("a one-arg
remove(target)here would evict it even though the map no longer holds turnA"). The authors understood the risk precisely and guarded 2 of 9 sites.Mutation proof (two sites, both survived)
Mutation 1 — line 405, in
resolve()'sBACKEND_EXHAUSTEDsuccess branch. AnchorinFlight.remove(target, turn);counts 5 withgrep -Fxc(shared by 405, 432, 505, 525, 609). Changed to the 1-arg form with a line-anchoredsed; recount 5 → 4, so the edit hit exactly one line.Mutation 2 — line 549, the early-return guard in
fail(). Anchor made unique by its trailing comment, count 1 → 0.Both survived. Both restored,
sha256matched pristine9b5f2f6d810fa45ee838d098256e472cf3af8bd668d20213e1a3dc8ffac8c8be,git status --shortclean.The other two explanations were ruled out, not assumed:
BACKEND_EXHAUSTEDcompletion whererendezvous.resolveExhaustedsucceeds; existing tests drive that path and still pass after the mutation — which proves the line executes and that nothing checks which turn was removed. Line 549 runs wheneverfail()is called with no live waiter, also driven by existing tests.mvn -o clean installon the default profile (excludedGroups=contract,pom.xml:264).CompletionResolverTestis not taggedcontract, so all 62 of its tests ran in both runs — the 1766 total is identical both times.Why severity 1
If any of the 7 unguarded lines is "simplified" back to the 1-arg form during a refactor, a live turn's waiter can be evicted by a stale one. The caller then hangs or receives the wrong answer, and no test fails.
The work
Add one assertion per unguarded site: 335, 405, 432, 505, 525, 549, 609. Follow the three existing tests as the model — they are the correct shape, there are just not enough of them.
Acceptance:
inFlight.remove(target, turn)withinFlight.remove(target), show the named failure, restore, and paste the matchingsha256. A test that is not red under that exact change is not the test being asked for.NOTE — an alternative fix was proposed and measured. The brief is UNCHANGED; here is why, so nobody re-opens it.
The fleet01 lead proposed a better-looking remedy than seven tests: extract one private
removeIfCurrent(target, turn), so the raw 1-arg call appears at no business site and one test guards all nine forever. Their argument is sound in general — a per-site rule should find the defect, not dictate a per-site remedy, and seven near-identical tests is the kind of ceremony someone deletes in two years because they cannot tell which one is load-bearing.I measured it before acting, and it does not hold here. Three findings:
1. A private helper does not make the unsafe form unrepresentable
inFlightis aprivate final ConcurrentHashMapdeclared atCompletionResolver.java:145, with 19 raw map calls inside the class. A private helper sits beside that field, not in front of it. Any future author working in the same class can still writeinFlight.remove(target), because the map is right there.So the helper would reduce how many sites you can see, not how many exist — which is precisely the warning already written into #577's own ticket body: "extracting the duplicated lines into one method does not reduce the number of call sites that must maintain the invariant; it only reduces how many of them you can see."
2. Real encapsulation is possible — and still would not enforce it
inFlightis confined to this class (the only two other mentions,Injector.java:643and:715, are comments). So the map could be wrapped in a small type that exposes only named operations.It still would not work, because the unsafe form is a LEGITIMATE operation at two sites.
register()at:252andcaptureBaseline()at:278use the plain 1-arginFlight.remove(target)correctly — they run whenwaiter == null, which is a different case.So the wrapper would have to expose both
removeIfCurrent(target, turn)andremoveAny(target). The choice between them then exists at every single site, exactly as it does today. The best available outcome is two better-named methods — a readability win, not an enforcement mechanism. "Make the unsafe form unrepresentable" is not reachable when the unsafe form is somebody's correct answer.3. The nine sites are nine branches, not nine copies
They sit in three methods:
resolve(String, InFlight)noReportMessage(String)fail(String, InFlight, String)Each is a distinct exit path needing its own arrangement to reach. They will look similar in their assertion and differ in their setup. That is not boilerplate; that is nine reachability scenarios, and the two already-covered sites (445, 566) are covered by three tests written exactly that way.
Conclusion
The brief stands: one test per unguarded site, each red under its own mutation. Tests remain the only thing that can enforce this invariant, because the type system cannot be made to.
Two refinements worth taking from the proposal, neither of which changes the acceptance:
Merged. Seven sites, seven tests, each red under its own mutation.
Merged to
mainas part of634d33b, via PR #585. Test-only, 143 insertions, one file — confirmed bygit diff --stat, not taken on report.My own verification:
Tests run: 1784, Failures: 0, Errors: 0, from theResults:block and an independent sum over 132 report files.remove(target)at site 405 — and it is killed byCompletionResolverTest.aSupersededExhaustedTurnMustNotEvictItsSuccessorsRegistration:662. Exactly one test. The anchor count dropped 5 → 4 as it should, since that exact line is shared by five sites.The worker named each test after the branch it pins, which is what the earlier note asked for. The seven names read as a list of the actual exit paths — done, exhausted, backend-error, raw-exhausted, raw-backend-error, done-failed, too-fast-backend-error — so a future reader can tell which one is load-bearing for which site. That was the real substance of the objection recorded in the note above, and it is answered.
The alternative fix stays rejected, and the reason is on the record in the note above: a private helper does not make the unsafe form unrepresentable, because
inFlightis a private field with 19 raw map calls in the same class; and real encapsulation does not help either, because the 1-arg form is the correct answer atregister():252andcaptureBaseline():278. The general rule that came out of it — "make the wrong form unrepresentable" is unavailable when the wrong form is somebody's correct answer — is worth more than this ticket.The count is now five. Instances of the per-site shape: #561, #572, #575,
3f8c38f(merged as #567), and this one plus #582.Closing.
Follow-up measurement: all NINE CAS sites are pinned, and none of them is an equivalent mutant
The fleet01 lead asked the right question after this merged: were the survivors checked for
equivalence? An equivalent mutant is a mutation that no test can ever kill, because the two
forms behave identically on every reachable input. If any of the nine
inFlight.remove(target, turn)sites sat in a branch where no successor can be registered, the CAS there is inert, a one-arg
mutation is unkillable, and a test for it would be asserting on something that cannot fail.
I measured it instead of answering from the brief. Two builds on
main634d33b, each mutatingtwo sites from the two-argument form to the one-argument form, in-place inside the call so the
statement structure cannot change:
Result — every site kills at least one test, and the kills are narrow
:335resolve()early return (waiter == null || isDone())aSupersededDoneTurnMustNotEvictItsSuccessorsRegistration:643:405resolveExhausted:432resolveFailure:445resolveCompletionsuccess armaSupersededTurnsPlainCompletionMustNotEvictItsSuccessorsRegistration:555and…EchoedNoReportSubPath…:592:505:525:549fail()early returnaSupersededDoneFailedTurnMustNotEvictItsSuccessorsRegistration:738:566fail()resolveFailuresuccess armaSupersededTurnsFailMustNotEvictItsSuccessorsRegistration:620:609failTooFastbackend errorMutating
:335and:549together produced exactly two named failures. Mutating:445and:566together produced exactly three —:445takes two tests because two different paths reachthat arm. No mutation took a crowd with it. Both runs restored the file to its pristine sha
9b5f2f6d810fa45e.So: no equivalent mutants here, and the four sites this ticket did not add a test for were already
covered —
:445and:566by tests that predate it (:555,:592,:620, from the #572/#575work), and
:335/:549by two of this ticket's own seven.Two corrections to what I wrote earlier
Two of the seven (
Done,DoneFailed) pin the two early-return sites:335and:549;the other five pin
:405,:432,:505,:525,:609. Nothing is missing, but my map oftest→site was not measured when I wrote it. It is now.
:445is reached two ways. Counting assertions persite is still right; counting sites per assertion is not the same number.
The rule this adds
A SURVIVING MUTANT HAS THREE EXPLANATIONS, AND ONLY ONE OF THEM IS A TEST GAP: the test is
missing, the test is weak, or the mutation is equivalent and no test could ever kill it. The
third invalidates the question rather than answering it, and it is the one nobody checks, because
the first two both end with "write a test" and that feels like progress. Ask it before commissioning
assertions, not after. Here the answer was clean — all nine are killable — but the ticket would have
been wrong to assume that.