Repository navigation
Attribute dependency waiting, the swarm's largest unexplained cost - #362
Conversation
claim_telemetry.idle_millis is computed as the remainder of a coordination window after the four measured phases, so it totals waiting without naming what was waited on. It is also the largest single line item in the swarm: 61.5% of all worker time on epoch 21, against 3.4% for the scheduling scan and 2.3% for lock waits. telemetry.dependency_wait records one row per cooperative_solve episode that reached the wait loop: the dependency's spine, size and budget, how the episode divided between claiming that branch, helping elsewhere and being stuck, and how it ended. holders_at_first_block and unclaimed_at_first_block are the columns the worker-cap question turns on. A worker in that loop tries the branch alone, then anywhere else, then a pair onto the branch, and only then sleeps; holders at MAX_WORKERS_PER_BRANCH means the cap refused the pair, while zero unclaimed candidates means the branch had nothing left to hand out and the wait is for its finalize. Those are different problems with different fixes and idle_millis cannot separate them. They are sampled once, at the first blocked iteration: that path already runs every 50 ms on a starving worker, and re-reading them per poll would put two queries on the branch everyone is waiting for. branch_claim_holders is the single-branch form of claim_holders_by_branch, so the sample costs one indexed query rather than a map of every branch. The table is created by _TELEMETRY_SCHEMA_SQL with CREATE TABLE IF NOT EXISTS, which runs on every open, so an existing telemetry file gains it without a migration step to sequence. Workers fork from the supervisor, so deploying still needs a swarm stop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98413fefed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings on the first-block sample, both of which defeated the distinction the columns exist to draw. unclaimed_at_first_block used n_candidates - done, which counts a candidate held in an unfinished claim as available. That is the ordinary finalize-wait shape — one rival holding every remaining slot — so the pair attempt would find no bundle while telemetry reported unclaimed work, reading as a cap refusal on a branch with nothing left to hand out. branch_unclaimed_candidates counts slots no claim row covers, so a freed position still counts as claimable and an in-flight one does not. The recursion-capped path sampled after its 50 ms poll rather than before it. A holder that finished mid-sleep read as zero holders, so both columns described a moment that blocked nothing. It now samples first, as the uncapped path already did. test_candidates_held_in_flight_do_not_read_as_unclaimed and test_the_capped_path_snapshots_before_it_sleeps each fail with their fix reverted. The second drives the capped path directly, with a patched _idle_wait that finishes the rival mid-sleep, because no existing test reaches that branch and the ordering is invisible without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
|
@codex review Both P2s are fixed in 14b30b4:
Each has a test verified to fail with the fix reverted, including one that drives the capped branch directly — it had no coverage before, so the ordering change passed the suite untested until that test existed. |
* origin/main: Require 25 effective samples before a cost-model cell reads warm
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b30b459a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more review findings, both on cases the columns claim to name. _await_rival_finalize ends in the same 50 ms poll but was called outside the accounting, so an episode that lost the finalize race put that sleep in episode_millis while blocked_millis read zero and both first-block counters stayed NULL. That is the wait-for-finalize case these columns exist to identify, and it is the common one. It now samples before the call and returns whether it polled, so the takeover path — which reopens a dead finalizer's row and completes the finalize — is counted as the work it is rather than as waiting. branch_unclaimed_candidates trusted a registry id as evidence the branch exists. delete_branch deliberately keeps the append-only branches row so branch_id stays stable across a re-promotion, while dropping every candidate_claims row, so a finished branch counted zero claims and reported all n_candidates as claimable: completed work described as untouched, and a cap refusal where nothing remains to claim. It now requires a live active_branches row. Each fix has a test verified to fail with it reverted, including one that pins the takeover path as unblocked so the poll and the takeover cannot be conflated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
|
@codex review Both P2s from the second pass are fixed in 4f51b53:
Three tests, each verified to fail with its fix reverted — including Also merged |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f51b53bc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The liveness check added for the deleted-branch case was a separate autocommit select from the claim count, so the two described different database states. A rival finalizing between them leaves the branch present and its claim rows already deleted, which reports a completed branch as fully claimable — the same telemetry error, reached by timing rather than by order. Both now come from one statement, which sees one snapshot. The race is not reproducible from a sequential fixture, so the guard is structural: test_unclaimed_reads_liveness_and_claims_in_one_snapshot traces the connection and requires a single SELECT naming both tables. It fails when the query is split back in two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
|
@codex review Fixed in 988bd15. Liveness and the claim count now come from one statement, so they describe one snapshot: SELECT EXISTS(SELECT 1 FROM active_branches WHERE branch_id = ?),
(SELECT COUNT(*) FROM candidate_claims WHERE branch_id = ?)Since a sequential fixture cannot exercise the race, the guard is structural — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 988bd150f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Making liveness and the claim count atomic inside one method did not make the two diagnostic columns atomic with each other. _record_first_block still called branch_claim_holders and branch_unclaimed_candidates separately, so a rival could finish, reclaim or finalize between them and leave a holder count from one state beside an unclaimed count from another. That pair is read against itself — holders at MAX_WORKERS_PER_BRANCH versus nothing left to claim — so a mismatched pair does not blur the answer, it inverts it. branch_block_snapshot replaces both methods and returns liveness, the holder count and the unclaimed count from a single statement, which sees a single snapshot. The race is not reachable from a sequential fixture, so the guard stays structural: the trace test now covers all three reads and fails when the query is split. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
|
@codex review Fixed in d62ba30. SELECT EXISTS(SELECT 1 FROM active_branches WHERE branch_id = ?),
(SELECT COUNT(DISTINCT claimed_by) FROM candidate_claims
WHERE branch_id = ? AND done = 0
AND (? IS NULL OR claimed_by IS NOT ?)),
(SELECT COUNT(*) FROM candidate_claims WHERE branch_id = ?)You were right that atomicity inside the second call was not the property that mattered — the pair is interpreted against itself, so a mismatched pair inverts the classification rather than blurring it. The structural trace guard now covers all three reads and fails when the query is split. AGENTS.md updated to state the one-statement requirement rather than just the sample-once one. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Both first-block columns reconstructed, from outside and after the fact, a decision claim_next_bundle already makes atomically inside its own BEGIN IMMEDIATE. That reconstruction was the source of four separate defects: a sample taken after the sleep it described, a deleted branch read as fully claimable, liveness and a claim count taken in two statements, and two counters taken in two calls. Each fix closed one instance of the same mistake. Occupancy is counted against the claims the claim transaction is about to create, so only that transaction can say whether the cap refused a claim. It now records which of its five return-None sites it took, and last_claim_decline reports it. dependency_wait's holders_at_first_block and unclaimed_at_first_block are replaced by four counters — blocks_worker_cap, blocks_no_candidates, blocks_awaiting_finalize, blocks_help_capped — one per cause, counted over the whole episode rather than sampled once. Two come from the claim transaction, two from the branch condition the worker is standing in. Nothing on this path reads live state, so no counter can disagree with the moment it describes, and the race class is gone by construction rather than by vigilance. branch_block_snapshot and _record_first_block are deleted along with their query: the attribution is now free. test_a_served_claim_clears_the_previous_decline asserts the clear after a real decline; on a fresh connection the field is already None and removing the clear proved nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
Redesigned: report the decision instead of sampling for itPushed as 738c196. The six review findings were two mistakes, and four of them were one mistake — reconstructing, from outside and after the fact, a decision
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 738c196ef5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
wait.blocks counted whatever reason came back, but only four of them had a column. A claim transaction can also report branch_gone, budget_mismatch or owner_mismatch when the dependency changes identity under the waiter, and an exhausted retry loop reports nothing at all. Those sleeps were counted and then dropped, leaving blocked_millis holding time no counter accounted for — which is the defect idle_millis has and this table exists to avoid repeating. blocks_other carries them, so every sleep increments exactly one counter and the five sum to the episode's sleep count. A counter that is a partition can be audited; a counter that is a selection cannot. The re-loop-without-sleeping alternative is deliberately not taken here: it changes what a blocked worker does, which is a scheduling change and wants its own justification rather than arriving inside instrumentation. _assert_blocks checks every column on every test rather than the one under test, because the first version of these tests passed with other_blocks summing named reasons too — the only reasons present were unnamed ones, so a double-count was indistinguishable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rawcKNbdKoVpUW1Avr4sS
1 participant
Summary
claim_telemetry.idle_millisis not measured — it is the remainder of a coordination window after the four measured phases, computed withmax(0, …). So it totals waiting without saying what was waited on. It is also the swarm's largest single cost:(Measured against worker wall time. Summing
coordination_millis + candidate_evaluation_millisinstead accounts for 124% of the available worker-hours, becausecandidate_evaluation_milliscounts nested help at every level it appears.)What this adds
telemetry.dependency_wait, one row percooperative_solveepisode that reached the wait loop: the dependency's spine, size and budget; how the episode divided between claiming that branch, helping elsewhere, and being stuck; and how it ended.Why each sleep happened
A worker in that loop tries the branch alone, then anywhere else, then a pair onto the branch, and only then sleeps. Four counters say which:
blocks_worker_capMAX_WORKERS_PER_BRANCHblocks_no_candidatesblocks_awaiting_finalizeblocks_help_cappedMAX_HELP_RECURSION_DEPTHforbade scanningidle_milliscannot tell these apart, and only the first is reached by the worker-cap change this measurement exists to decide.Reported, not sampled
Every reason comes from the code that made the decision.
blocks_worker_capandblocks_no_candidatescome fromERDQueue.last_claim_decline(), set inside the claim transaction at each of itsreturn Nonesites. The other two are the branch condition the worker is standing in. Nothing on this path reads live state.That is the design, not an optimization. Occupancy is counted against the claims the claim transaction is about to create, so only that transaction can say whether the cap refused a claim. An earlier revision of this PR sampled holders and unclaimed-slot counts from outside afterwards, and that reconstruction produced four separate defects — a sample taken after the sleep it described, a deleted branch read as fully claimable, liveness and a count taken in two statements, two counters taken in two calls. Each fix closed one instance of one mistake. Reporting the decision instead removes the class by construction, deletes a query, and makes the answer exact rather than inferred.
An episode answered before the loop opens writes nothing, so write volume tracks contention rather than throughput.
Deployment
The table is created by
_TELEMETRY_SCHEMA_SQLwithCREATE TABLE IF NOT EXISTS, which runs on every open, so the existing telemetry file gains it with no migration to sequence. Workers fork from the supervisor, so deploying still needs a swarm stop.Tests
TestClaimDeclineReasonpins each of the five decline reasons against the transaction that decides it;TestDependencyWaitAttributionpins the episode accounting. Every guard is verified to fail with its behaviour reverted, including:no_candidatestest_the_worker_cap_is_named_as_the_reason,test_a_pair_refused_by_the_cap_is_recorded_as_worker_captest_a_served_claim_clears_the_previous_declineworker_captest_the_recursion_cap_names_its_own_blocktest_a_finalize_takeover_is_not_charged_as_blocked_time_record_dependency_waitmade a no-optest_a_solved_dependency_names_the_branch_it_waited_onshould_recordalways Truetest_an_episode_that_never_iterates_writes_no_rowThree tests in this PR's history passed under mutation on the first attempt and were rewritten: the cache-hit test could not reach
should_record, no test reached the recursion-capped branch, and the clear-on-success test ran on a fresh connection where the field was alreadyNone.Follow-up, not included
No
viewreport yet — the table is queryable directly, and a reporting surface belongs in its own change alongside whatever the first read of this data says is worth showing.