fix(orders): stop retention prune burning a tick on already-gone beads - #8
Open
austinborn wants to merge 2 commits into
Open
fix(orders): stop retention prune burning a tick on already-gone beads#8austinborn wants to merge 2 commits into
austinborn wants to merge 2 commits into
Conversation
The closed-order-tracking retention prune runs from two independent places that neither coordinate nor share a process: the controller's in-process retention watchdog, and the `gc order sweep-tracking` command an order can fire on its own cadence. Both list the same closed set live, so neither sees the other's deletions, and the sweeper that runs second finds every bead the first already removed. That race was survivable. Two things made it expensive. First, a delete failing with ErrNotFound was joined into the sweep error, so the loser reported thousands of failures for beads that were in exactly the state it wanted them in. One operator's supervisor log carried 3334 such failures across 3223 distinct ids. Second, the bounded sweep's budget counted only successful deletes, so a list whose beads had all been pruned by the other sweeper never reached the limit and walked the entire backlog. Each attempt costs a full deleteWorkflowBead pass, which walks both dependency directions before deleting; on the subprocess-backed store that is several bd invocations per bead. Treat an already-gone delete as already-pruned rather than a failure, and budget delete attempts rather than successes. The reported count still reflects real prunes, so the watchdog's "pruned N closed bead(s)" line keeps its meaning. This bounds the loser of the race. It does not stop the duplicated work, which needs the prune to have a single owner or to be single-flighted across both entry points; that is tracked separately. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @austinborn Co-Authored-By: <operator-factory-bot> <factory-bot@<operator-domain>.invalid>
The prior commit on this branch changed the closed-order-tracking retention budget to count delete attempts rather than successful deletions. Two doc comments were left describing the old success-based behavior. orderTrackingRetentionWatchdogDeleteBudget's comment still said it bounds the number of beads deleted per invocation. That constant is what the whole fix hinges on, so a maintainer reading only the comment could "correct" the attempt counter back to counting successes and silently reintroduce the churn the fix removes. It now states the attempt semantics and the reason for them. runOrderTrackingRetentionWatchdog's comment carried the same pre-fix framing, at the other place a reader looks to learn the budget's unit. It now says the watchdog makes at most that many delete attempts, and notes that beads a concurrent sweeper already removed spend budget without counting toward the pruned total it reports. Comment-only. No behavior change. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @austinborn Co-Authored-By: <operator-factory-bot> <factory-bot@<operator-domain>.invalid>
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
The closed-order-tracking retention prune treated "the bead I wanted gone is already gone" as a delete failure, and its per-tick budget counted only successful deletes. Together those turned a routine race into thousands of wasted
bdsubprocess spawns per controller tick.Two changes, both in the retention sweep:
beads.ErrNotFoundis now counted as already-pruned instead of being joined into the sweep error. The bead being absent is the state the prune was trying to reach.pruned N closed bead(s)line is unchanged in meaning.Why it happened
The prune runs from two independent places that neither coordinate nor share a process:
sequenceDiagram participant W as Retention watchdog (in-process, 15m) participant S as gc order sweep-tracking (separate process, 1m) participant B as Bead store W->>B: list closed tracking beads (live, uncached) S->>B: list closed tracking beads (live, uncached) S->>B: delete each bead (wins) W->>B: delete each bead B-->>W: ErrNotFound, for every bead S already removedBoth read the same closed set live, so neither sees the other's deletions. The loser then pays a full
deleteWorkflowBeadpass per already-deleted bead, which walks both dependency directions before the delete fails. On the subprocess-backed store that is severalbdinvocations each.Because the budget only counted successes, the loser never reached its limit and kept going to the end of the backlog.
Evidence
From one operator's supervisor log: 3334 delete failures across 3223 distinct bead ids, split 3303
bead not foundand 31ambiguous ID. The ambiguous variant is the same failure wearing a different message, and it fires only when a missing id's hash happens to appear as a substring inside unrelated session or wisp ids. Sampled failing ids resolve to "no issue found" in both tiers, which confirms they were genuinely deleted rather than mis-addressed.Scope
This makes the loser of the race cheap and quiet. It does not stop the duplicated work, which needs the prune to have a single owner or to be single-flighted across both entry points. That is tracked separately.
Test plan
TestSweepClosedOrderTrackingRetentionTreatsAlreadyGoneBeadAsPruned— an already-gone bead is not a sweep error.TestSweepClosedOrderTrackingRetentionBoundedCapsAttemptsWhenBeadsAlreadyGone— with 200 eligible beads, all already gone, and a budget of 5, the sweep makes exactly 5 delete calls. Against the unfixed code it walks all 200.TestSweepClosedOrderTrackingRetentionBoundedCountsOnlyRealDeletions— already-gone beads spend budget without inflating the reported prune count.go test ./cmd/gc/...package run is green.