refactor(dispatch): F-CONC audit batch (5 concurrency findings)#623
Merged
Conversation
…udit Three audit findings (#494 F-CONC-4, #498 F-CONC-8, #490 F-CONC-11) were re-validated against current code and confirmed as correct-by- construction; what was missing was an in-source explanation of why. This commit captures those invariants where a future refactor would otherwise reintroduce the patterns the audit flagged. - `cancel_and_find_parent` (signals.rs): document the `subleads` → `worker_layer_index` → `workers.cancels` lock-acquisition order as a project invariant, naming every site that currently respects it (#494). - `register_sublead` (state.rs): explain why insert → install_watcher → cascade_to is load-bearing — the proposed reorder from the audit would have CAUSED a real bug because `install_cascade_cancel_watcher` is fire-once on root cancel. The eager cascade at step 3 is what closes the post-watcher-fired window for late-registered subleads (#498). - `control/server.rs` broadcast site: annotate the op-reply broadcast as intentional, complementing the existing PR-Q routing block. The audit re-validation confirmed `OpUnknownState.current_state` carries only the coarse enum already published via `WorkerStatus`, so the "subscriber visibility" surface is not a leak (#490). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes the F-CONC-7 (#497) operator-surprise gap: after a single Ctrl-C the watcher would drain and then loop forever waiting for a second Ctrl-C. If an operator walked away and a worker hung with no timeout, the run would never wind down. Adds `PITBOSS_CTRLC_DRAIN_ESCALATE_SECS=<n>` as an opt-in env var. When set to a positive integer, the watcher races a sleep timer against a follow-up Ctrl-C after the 5-second second-SIGINT window expires; whichever fires first escalates to terminate via `TerminateReason::OperatorCtrlC` (matching the existing double-Ctrl-C path). Default behaviour is preserved: with the env var unset, empty, zero, or unparseable, the watcher continues to drain forever (the pre-fix contract). Operators who want auto-cleanup can opt in. A `#[serial(env)]` unit test pins the env-parsing contract (none for unset/empty/zero/unparseable, Some(Duration) for positive integers with surrounding whitespace tolerated). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F-CONC-12 #491: the per-`WorkerRegistry` broadcast channel that fans worker-completion events out to `wait_for_*` subscribers was sized at 64 — fine for the historical single-layer flat mode, but post depth-2 every root subscriber sees every worker's completion across the whole tree. In a mass-cancel scenario across a saturated tree, the slowest subscriber can lag the channel and fall back to `refresh_in_flight_from_maps`, which holds nested read locks on `subleads` + per-sublead `workers.states` across the recovery scan. Bumping to 1024 absorbs the storm without changing the fallback's correctness guarantee — the `Lagged` → `refresh_in_flight_from_maps` path remains in place as belt-and-braces, not the expected fast path. The capacity change is ~8 KiB additional memory per `WorkerRegistry` (tokio broadcast allocates the ring eagerly). Doc-comment updated to explain the sizing rationale so a future refactor that questions the value has the context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SDS-Mode
enabled auto-merge (rebase)
May 27, 2026 01:30
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.
Summary
Bundles the five open
F-CONC-*concurrency-audit findings into a single PR. Each finding was independently re-validated by a multi-agent review team against current code (post the F-ARCH batch merge) before any edits landed.cancel_and_find_parentPITBOSS_CTRLC_DRAIN_ESCALATE_SECSenv varregister_subleaddone_txLaggedtriggers nested-lock rescanThree commits routed by
cliff.toml:docs(dispatch,control)— covers F-CONC-4:cancel_and_find_parentacquiressubleadsandworker_layer_indexread locks simultaneously #494, F-CONC-8: Race window inregister_subleadbetween insert and cascade #498, F-CONC-11: Op-reply broadcast is visible to all clients, not just the requester #490 (skipped from CHANGELOG)feat(dispatch)— covers F-CONC-7: Ctrl-C watcher continues to loop after drain expires without terminate #497 (Added)perf(dispatch)— covers F-CONC-12:done_txbroadcastLaggedinawait_workers_drainedtriggers a full map rescan #491 (Changed)Notable findings from validation
F-CONC-8 turned out to be the most valuable validation. The audit's "no action needed" verdict was right, but the audit also flirted with a reorder (cascade → insert → install_watcher). Tracing
install_cascade_cancel_watchershowed that reorder would actually introduce a bug: that watcher is fire-once on root cancel, so the eagercascade_toat step 3 is what closes the post-watcher-fired window for late-registered subleads. The doc-comment now spells out why the current order is load-bearing so a future refactorer doesn't try the "obvious" cleanup.F-CONC-7 is opt-in for back-compat safety. Default behaviour is unchanged (loop forever in drain mode). Operators who want auto-cleanup set
PITBOSS_CTRLC_DRAIN_ESCALATE_SECS=<n>. A unit test pins the env-parsing contract.F-CONC-11 is documentation-only. Validation confirmed
OpUnknownState.current_stateonly carries the coarse string enum already published viaWorkerStatus— there is no leak. The existing PR-Q routing block atserver.rs:619-635is the design rationale; the new inline comment marks the broadcast site so a reader at that line knows the visibility is intentional, not a bug.F-CONC-12 capacity bump from 64 → 1024. Pre-depth-2 the channel served a single layer; post depth-2 the root subscriber sees completions across the whole tree. ~8 KiB additional memory per
WorkerRegistry, well within budget. TheLagged→refresh_in_flight_from_mapsrecovery path remains in place as belt-and-braces.Test plan
cargo lintclean (workspace clippy-D warnings)cargo tidyclean (fmt --check)cargo test -p pitboss-cli --test cancel_cascade_flows— 4/4 pass (pins F-CONC-8: Race window inregister_subleadbetween insert and cascade #498 invariants)cargo test -p pitboss-cli --test control_flows— 20/20 pass (incl.op_replies_broadcast_to_subscriberswhich pins F-CONC-11: Op-reply broadcast is visible to all clients, not just the requester #490 behaviour)cargo test -p pitboss-cli --test sublead_flows— 34/34 passcargo test -p pitboss-cli --test hierarchical_flows— 7/7 pass#[serial(env)]unit testdrain_escalate_timeout_is_opt_inpins the F-CONC-7 env-parsing contractWhat's NOT in this PR
The
register_sublead"race window" (#498) is not closed by code change because closing it would require introducing the bug the audit reviewer flagged. The window is benign (sublead subprocess hasn't launched yet); the documentation makes that explicit so future maintainers don't trade it for the worse failure mode.Closes #490
Closes #491
Closes #494
Closes #497
Closes #498
🤖 Generated with Claude Code