Repository navigation
Refuse a stale worker's fold, and stop workers going silent while they price groups - #378
Conversation
A branch can finalize, be deleted, and be re-created at another budget under the same branch_key -- the same answer set reached by a second spine of a different length. A worker whose claim on the old incarnation was reclaimed does not learn any of that; it finishes its candidate and folds the cost in. update_branch_best guarded only monotonicity, so it took that cost whenever it was lower than the new branch's best. A cost achieved at a larger budget is below what a smaller budget can reach, so the guard admitted it every time -- and the branch then finalized under its own optimum, which is the unsound direction: a parent folding that result believes in a line no strategy plays. Ownership and priority both survive the re-creation and so catch nothing. The branch's own budget is now a precondition of the update, matching the check the claim transaction already makes. A stored budget of NULL predates the column and is admitted; a caller passing no budget asks for no check. Both production callers pass the budget they evaluated at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
evaluate_candidate ticks once on entry and reaches the next tick by recursing. A candidate that prunes on its bound never recurses, so that single tick covers its whole evaluation -- and pricing one response group costs a scan of the entire guess vocabulary. Measured on rocky's worker logs: of 25,366 slow candidates, 15,767 never recursed, 9,382 of those ran past HB_TIMEOUT_SECONDS, and the worst held one tick for 10,796 seconds. reclaim_stale_claims frees an unfinished claim whose worker has not heartbeat within that window, so live workers are being reclaimed: 611 claims over two and a half days, with all six workers up. The liveness gate added in 774ac29 is sound in design; its stated premise, that live workers heartbeat every ~2s, stopped holding for this class of work. _remaining_groups_cost_lower_bounds now ticks once per group, threaded from both callers -- evaluate_candidate and the bundle's two-level prune pass. The tick cannot go through _heartbeat, which counts a node as it writes. _nodes means candidate evaluations, and the cost model, add_nodes_spent and the accuracy rows all read it as one, so a per-group signal would inflate every one of them. _liveness_tick carries the write alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4898e24ad8
ℹ️ 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".
evaluate_candidate recursed through _solve_subset without the tick, and _solve_subset evaluated each descendant candidate with only the node-counting heartbeat. A descendant pricing its own response groups therefore went silent exactly as the entry frame did, and had its live claim reclaimed -- the production failure this is meant to prevent, moved one level down. The tick now threads through _solve_subset and back into the descendant evaluate_candidate calls, the same way hint_cache reaches every frame. test_every_frame_that_prices_groups_is_given_the_tick asserts on the frames actually reached rather than on the answer: without the forwarding it names six descendant branch sizes that priced groups unticked, while every result assertion still passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
|
Review again, @codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 796b508aef
ℹ️ 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".
branch_key and idx do not identify a claim. A worker whose claim was reclaimed while it was still evaluating goes on to finish, and by then the index may have been reissued -- to another worker on this branch, or to a different incarnation of it after a finalize and re-creation. Completing by key and index alone marks that live claim done while contributing nothing to it, and the branch can then finalize without the candidate ever having been evaluated at the budget it now holds: an optimum some candidate beats, or a loss that is not one. The budget precondition on update_branch_best makes this reachable rather than merely latent. Before it, a stale fold was accepted, so the branch at least held a value for that index -- a wrong one. Now the fold is refused and the completion still lands, so the index is marked done with nothing behind it. The two have to move together. complete_candidate is therefore scoped to the claim the caller holds, using the claimed_by and bundle_id pair claim_next_bundle stamps, and returns whether it completed one. evaluate_claim passes them and reports a loss rather than counting work the branch did not take. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
|
@codex REview again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9306271095
ℹ️ 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 docstring claimed the loop runs for minutes on a large branch, and the commit that added it read `max_depth=0` in the slow-candidate log as "never recursed". Both are wrong, and the second is what produced the first. `_cand_max_depth` is reset every PROGRESS_LOG_SECONDS (120) inside `_heartbeat`, not once per candidate, so `max_depth=0` says only that no deeper frame was entered since the last progress line. It is not evidence that a candidate stayed in one frame, and the 15,767 such rows do not show what they were quoted as showing. Measured at production vocabulary instead: one group's floor is 1.4 ms on a two-word group and 44 ms on the largest group that can exist, and the whole per-candidate loop priced over the entire answer list is 0.52-0.91 s. Against HB_TIMEOUT_SECONDS of 30 that is two orders of magnitude of headroom, so this loop cannot be where a worker falls silent. The tick stays: it costs one call per group and closes a gap wherever uninterrupted work happens. It is no longer offered as the explanation for the 611 claims reclaimed from live workers, which remains undiagnosed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
|
ANother review, please, @codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bacf44ace
ℹ️ 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".
An evaluation produces five branch writes -- the taint flag, the nodes spent, the running best, the cut flag, and the completion -- and every one of them describes the branch incarnation the candidate was evaluated against. They were being guarded one at a time, which cannot converge: refusing one while accepting the rest leaves the branch describing a mixture of two incarnations, and each guard added makes the next unguarded write newly reachable. The sharpest case is the cut flag. A stale OVER_ERD_LIMIT calls mark_branch_cut against the ceiling its own incarnation carried, setting cut_occurred on a replacement that has none. maybe_finalize then takes the cut path with ceiling = None and reaches add_cut_result, whose bound column is NOT NULL -- an exception raised after try_finalize_branch has already been won, which strands the branch finalized-but-unwritten. ERDQueue.claim_is_current answers the whole set in one indexed read: the claim must still exist unfinished and belong to this caller, and the branch must still be open at the budget the caller evaluated at. evaluate_claim asks once, before the first write, and applies all five or none. The per-operation guards on update_branch_best and complete_candidate stay. They close the window between the check and the writes; this one closes the window the evaluation itself ran in. test_every_branch_write_fires_when_the_claim_is_held pins the fixture against the vacuum: without it the refusal test passes against a fixture that reached no write at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
Self-review of the incarnation guard. The guard was spelled as five `claim_is_mine and ...` conditions scattered through the result-application block, which left the worker evaluating the rest of a bundle it no longer held. Those siblings are gone too: reclaim_stale_claims frees a worker's unfinished claims together and a bundle is claimed in one instant. Carrying on re-evaluates candidates another worker now owns -- measured at a 98 s median for evaluations long enough to be reclaimed, about eight minutes for a bundle of six, and it is the two-workers-on-one-branch concentration that is a loss at this vocabulary. Now one early return beside the cancellation path, which is the same shape: nothing below it may touch the branch, and the bundle ends censored. The four scattered guards go with it -- a dead guard reads as a live one, and those sites are unreachable once the return is in place. The candidate accuracy row is kept on this path deliberately, with the reason stated: it measures an evaluation that really happened and carries its own n_words and budget, and dropping it would bias the cost model against exactly the long evaluations most likely to be reclaimed. Also covers `claim_is_current`'s unregistered-branch return, the one line the guard's tests had missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
|
@codex, review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3adc9bf5f
ℹ️ 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".
…ndles Four review findings, three of them about code added earlier in this branch. **Releasing the bundle on a lost claim was wrong.** It assumed the siblings were gone too, which holds when reclaim_stale_claims frees a worker's claims together but not for the other way a claim dies: claim_next_bundle's one-level sweep does INSERT OR REPLACE on a single row, leaving the siblings owned by this bundle. Abandoning them stranded a branch that could never finalize, because a worker that is alive and heartbeating never has its claims reclaimed for it. A lost claim now discards its own result and carries on. **Checking and then writing could not be made atomic by more guards.** The check was an autocommit SELECT and each write a later autocommit statement, so the branch could change in between and take some of the result. Guarding the writes one at a time had already produced three rounds of findings for exactly this reason. ERDQueue.apply_candidate_result takes the whole result -- nodes, taint, best, cut, completion -- re-reads the claim inside one BEGIN IMMEDIATE, and applies all of it or none. The per-write guards stay; they cost nothing and still hold for direct callers. Nodes spent moves inside that transaction, which also stops an aborted candidate being charged twice: its claim stays open for another worker to redo, and that worker charges its own nodes. **Two tests proved nothing.** The budget-guard test deleted the branch, which deletes its claim rows, so claim_is_current answered False on the missing JOIN and would have answered False with the budget clause deleted as well. It now re-claims under the new incarnation first and asserts both directions. Two test docstrings still carried the retracted "runs for minutes" reading of the group-pricing loop, contradicting the measurements this branch added. Measured, against the prediction that batching would be cheaper: it is not. One transaction costs 169.7 us per candidate against 129.1 us for the separate statements -- 1.31x, about 0.16% of total worker time at this claim rate, and roughly 4% aggregate write-lock occupancy across six workers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpacbZRqT46b4Duhxq5QNQ
|
@codex, time for another review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
Summary
Two defects on the same path: a worker whose claim is reclaimed while it is
still evaluating, and the reason that keeps happening.
A stale worker can drive a re-created branch below its own optimum
A branch can finalize, be deleted, and be re-created at another budget under
the same
branch_key— the same answer set reached by a second spine of adifferent length. A worker whose claim on the old incarnation was reclaimed
does not learn any of that; it finishes its candidate and folds the cost in.
update_branch_bestguarded only monotonicity (? < best_erd), so it tookthat cost whenever it was lower than the new branch's best. A cost achieved at
a larger budget is below what a smaller budget can reach, so the guard admitted
it every time. The branch then finalized under its own optimum — the
unsound direction, because a parent folding that result believes in a line no
strategy plays. Ownership and priority both survive the re-creation and so
catch nothing.
The branch's own budget is now a precondition of the update, matching the check
claim_next_bundlealready makes. A stored budget of NULL predates the columnand is admitted; a caller passing no budget asks for no check. Both production
callers pass the budget they evaluated at.
Pricing a candidate's response groups now proves liveness
evaluate_candidateticks once on entry and reaches the next tick byrecursing, so a frame doing uninterrupted work between recursions produces no
signal.
_remaining_groups_cost_lower_boundsnow ticks once per group,threaded from both callers and onward through
_solve_subsetinto everydescendant frame, the same way
hint_cachereaches every frame.The tick cannot go through
_heartbeat, which counts a node as it writes._nodesmeans candidate evaluations, and the cost model,add_nodes_spentandthe accuracy rows all read it as one, so a per-group signal would inflate every
one of them.
_liveness_tickcarries the write alone.Correction (commit 5). The first version of this section claimed the loop
runs for minutes and cited 15,767 slow candidates with
max_depth=0asevidence that they never recursed. That reading is wrong:
_cand_max_depthisreset every
PROGRESS_LOG_SECONDS(120) inside_heartbeat, not once percandidate, so
max_depth=0says only that no deeper frame was entered sincethe last progress line.
Measured instead, at production vocabulary:
Against
HB_TIMEOUT_SECONDSof 30 that is two orders of magnitude of headroom,so this loop is not where a worker falls silent. The tick is kept because
it costs one call per group, not because the time is spent here.
What remains true, and what does not. 611 claims were reclaimed over two
and a half days with all six workers up, so workers are going silent past 30 s.
That is measured. Where they go silent is not, and this PR no longer claims
to know — that is tracked in #379, which carries the measurements above and the
first hypothesis to test. The two correctness fixes below and above stand on their own — they
are about the claim and the fold agreeing, not about why a reclaim fires.
A completion must name the claim it completes
branch_keyandidxdo not identify a claim. The same stale worker goes onto call
complete_candidate, and by then the index may have been reissued — toanother worker on this branch, or to a different incarnation of it. Completing
by key and index alone marks that live claim done while contributing nothing to
it, and the branch finalizes without the candidate ever having been evaluated
at the budget it now holds: an optimum some candidate beats, or a loss that is
not one.
The budget precondition above is what makes this reachable. Before it, the
stale fold was accepted, so the branch at least held a value for that index — a
wrong one. Refusing the fold while still accepting the completion leaves the
index done with nothing behind it. The two have to move together, so they do.
complete_candidateis scoped to the claim the caller holds, using theclaimed_by/bundle_idpairclaim_next_bundlestamps — the same shapecomplete_bundle_two_level_erd_prunesalready uses — and returns whether itcompleted one.
evaluate_claimpasses them and reports a loss rather thancounting work the branch did not take.
One guard for the whole result, not one per write
An evaluation produces five branch writes — the taint flag, the nodes spent,
the running best, the cut flag, and the completion — and all five describe the
branch incarnation the candidate was evaluated against. The first three commits
guarded them one at a time, which cannot converge: refusing one while
accepting the rest leaves the branch describing a mixture of two incarnations,
so each guard made the next unguarded write newly reachable.
The sharpest case is the cut flag. A stale
OVER_ERD_LIMITcallsmark_branch_cutagainst the ceiling its own incarnation carried, settingcut_occurredon a replacement that has none.maybe_finalizethen takes thecut path with
ceiling = Noneand reachesadd_cut_result, whoseboundcolumn is
NOT NULL— an exception raised aftertry_finalize_branchhasbeen won, stranding the branch finalized-but-unwritten.
ERDQueue.claim_is_currentanswers the set in one indexed read: the claim muststill exist unfinished under this caller, and the branch must still be open at
the budget evaluated at.
evaluate_claimasks once, before the first write,and a lost claim leaves through a single early return beside the cancellation
path — the same shape, and for the same reason: nothing below it may touch the
branch.
complete_candidate's own scoping stays as a backstop for the windowbetween the check and the write.
Measured, so the hot path is not taken on trust.
evaluate_claimruns onceper candidate — about 50 million times over two and a half days — so a query
added here is added 50 million times:
claim_is_currentread_branch_best(already once per candidate)branch_done_candidates(already once per claim)EXPLAIN QUERY PLANagainst the production queue (93,645 claim rows) showsboth sides as indexed lookups —
active_branchesby integer primary key,candidate_claimsby its(branch_id, idx)autoindex — so there is no scanhiding behind the microbenchmark. Total cost at 50 M candidates: 0.10
worker-hours.
A lost claim now releases the bundle rather than finishing it. The siblings
are gone too —
reclaim_stale_claimsfrees a worker's unfinished claimstogether and a bundle is claimed in one instant — so carrying on re-evaluates
candidates another worker owns. Measured: evaluations long enough to be
reclaimed run a 98 s median, about eight minutes wasted per bundle of six.
Tests
Nineteen new tests; each was run against the fix disabled and fails there.
'slate' != 'crane'without it —a budget-5 cost of 1.5 displacing a budget-3 branch's correct 3.0.
0 not greater than 1with the tick removed.The fixture deliberately targets the band between the closed-form and
two-level bounds: below it the candidate is priced out by the cheap
vectorized check and the group loop never runs, so a naive fixture proves
nothing.
test_every_frame_that_prices_groups_is_given_the_tickasserts on theframes actually reached rather than on the answer. Without the recursive
forwarding it names six descendant branch sizes that priced groups unticked,
while every result assertion still passes.
_liveness_tickcount a node fails four tests.test_completing_a_candidate_reissued_to_another_worker_is_refusedreclaims a claim, reissues the index to a second worker, and has the
first finish. Without the scoping it fails as
True is not false, withthe live claim marked done.
1,041 tests green across the queue, swarm, engine-bound, packing, cache-gap,
visibility, hint-cache, cost-model and game suites.
Found while auditing the stale-reclaim path for #367 scheduling.
Merging this de-escalates #379 from P0 to P2. That issue is P0 on
maintoday because the corruption path above is live; once this is merged and the
swarm restarted onto it — workers fork from the supervisor, so merging alone
does not deploy — a reclaimed worker's result is refused rather than folded,
and what remains is wasted compute with an undiagnosed cause.