BLO-28863: add routine-period-aware retry clamp primitive - #1434
Conversation
The transient horizon clamp (BLO-23438/BLO-23525) is period-unaware by design: its ceiling is a flat MAX_TRANSIENT_RETRY_HORIZON_MS (24h) and its own suite asserts a 23h floor is "ordinary provider backoff" to leave untouched. That holds for unbounded work and breaks for a windowed routine. Measured against the live fleet on 2026-08-19: 70% of `transient_failure` retries were pinned 3-5h out to the provider's advertised reset, all inside the 24h ceiling, so the existing clamp never fired for the case that loses windows. The worst backoff on record (23h59m43s, attempt 1) is that ceiling binding, not a missing clamp. Adds `resolveRoutineScopedRetry`, a pure decision function that bounds a retry against the owning routine's period and, when known, the tighter deadline of the specific window being served. It returns a decision rather than a Date so `abandon` must be handled explicitly: a retry that can only wake after its window is worth less than no retry, because it still consumes one of the owning agent's `maxConcurrentRuns` slots and one attempt while delaying every other queued retry behind it. Additive only — no existing caller changes behaviour. Wiring this into `scheduleBoundedRetryForRun` needs a run -> issue -> routine period lookup that the function does not currently have in scope, and touches the dispatch path for the whole fleet; that is left for review rather than bundled here. Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 1d491e3
A well-argued additive primitive with unusually good evidence discipline. Three things need attention before wiring: the clamp branch does the exact thing its own comment declares pointless, non-finite inputs silently produce an Invalid Date due time, and the review gate is red on a fixable PR-description omission.
Critical Issues (0)
Important Issues (3)
-
[code / native-codex]
server/src/services/ccrotate-capacity-retry.ts:185— The clamp branch contradicts the abandon rationale it is built on, and themarginMsguard measures the wrong quantity.
The comment states: "Retrying at the deadline itself is pointless — the window is closed at that instant — so only clamp when there is real margin left to land in, and abandon otherwise." But the clamp branch returnsdueAt: new Date(deadlineMs)— precisely the deadline. AndmarginMs(line 188) measures the gap betweenfailedAtand the deadline, not between the clamped due time and the deadline; the clamped due time has exactly zero margin by construction, for every input. So "only clamp when there is real margin left to land in" is never actually implemented.
Executed against the diff's own logic,failedAt=05:59:59.999Z,windowClosesAt=06:00:00.000Z,dueAt=10:00Z, period 6h yields{decision: "clamp", dueAt: 2026-08-19T06:00:00.000Z}— 1 ms of budget. That is exactly the stranded retry the docstring says must be abandoned: it wakes at the instant the window closes, cannot do the window's work, and still consumes onemaxConcurrentRunsslot and one attempt.routine-scoped-retry.test.ts:96assertslate.dueAt === windowClosesAt, so the suite currently pins the behaviour the docstring forbids.- Pick one and make all three agree. Either abandon below a minimum usable margin (
deadlineMs - failedAt < MIN_USEFUL_RETRY_MARGIN_MS, derived from measured run-startup time rather than hand-picked — this file'sTRANSIENT_HORIZON_CLAMP_MIN_ATTEMPTSsets that precedent explicitly), or clamp todeadlineMs - leadMsso the retry wakes with time to work, or — if a wake at the close is genuinely useful — correct the comment and add a test saying why. As written, code, comment, and test disagree, which is how the next reader "fixes" the wrong one.
- Pick one and make all three agree. Either abandon below a minimum usable margin (
-
[error-handling / types]
server/src/services/ccrotate-capacity-retry.ts:175— Non-finite input silently produces anInvalid Datedue time presented as a successfulclamp.
Math.max(1, NaN)isNaN, soMath.max(1, input.routinePeriodMs)reads like an input guard but only covers the non-positive case, not the non-finite one. ANaNperiod — or an invaliddueAt/failedAt— propagates all the way through:deadlineMsisNaN,input.dueAt.getTime() <= NaNisfalse(skipshonour),marginMsisNaN,NaN <= 0isfalse(skipsabandon), and the function returns{decision: "clamp", dueAt: new Date(NaN)}. Confirmed by executing the diff's logic directly:[NaN routinePeriodMs] -> { decision: 'clamp', dueAt: Invalid Date, ... } [invalid failedAt] -> { decision: 'clamp', dueAt: Invalid Date, ... }This is a plausible input, not a contrived one: the docstring says the due time arrives from
penstockAdvertisedResumeAt— provider-supplied — and the period comes from routine config. AnInvalid Datewritten into a retrydueAtis a materially worse outcome than either legitimate branch, and theabandonpath that exists to keep this safe is bypassed. Nothing in the union type warns a caller thatdecision: "clamp"may carry an unusableDate.- Fail closed to
abandonon non-finite input, before any arithmetic: checkNumber.isFiniteoninput.routinePeriodMs,input.dueAt.getTime(),input.failedAt.getTime()(andwindowClosesAtwhen present) and returnabandonwith a distinctreason. Add a test — the suite has no non-finite case today.
- Fail closed to
-
[gstack/review]
.github/PULL_REQUEST_TEMPLATE.md(PR description) — Thereviewcheck is failing and it is attributable to this PR, not to infrastructure.
reviewpasses in 23–37 s on #1433/#1435/#1436 but failed here after 3m6s. The job log resolves to the commitperclip gate: the description is missing the required## Thinking Path,## What Changed,## Verification,## Risks, and## Model Usedsections, plus the dedup-search checkbox. The existing description covers most of this content under different headings, so this is a re-heading job rather than new writing.- Restructure the description under the template headings and tick the dedup-search box; the gate re-runs on the next push. (For contrast, the
e2efailure on this head is not yours: it ended inThe operation was canceledwith1 interrupted / 42 did not runintests/e2e/app-not-connected.spec.ts, a frontend spec, and this diff touches no frontend code.)
- Restructure the description under the template headings and tick the dedup-search box; the gate re-runs on the next push. (For contrast, the
Suggestions (3)
- [types / code]
server/src/services/ccrotate-capacity-retry.ts:158— Nonowparameter, asymmetric with the sibling clamp.clampTransientRetryHorizontakesnowand derives its ceiling from it; this function derives everything fromfailedAt. When a retry row is re-evaluated after the fact, orfailedAtis an earlier attempt's failure,deadlineMscan already be in the past and the caller getsdecision: "clamp"with a due time behind the present — indistinguishable from a clamp into the future. Either acceptnowfor symmetry, or document that the caller must supply a freshfailedAt. - [tests]
server/src/__tests__/routine-scoped-retry.test.ts:120— The monotonicity test can pass vacuously. Guarding the only assertion behindif (result.dueAt)means a regression that turned every case intoabandon(dueAt: null) would leave this test green while asserting nothing. Addexpect(result.dueAt).not.toBeNull()— or assert the expecteddecisionper case — so the guard cannot swallow the check. - [types]
server/src/services/ccrotate-capacity-retry.ts:193—clampedFromIsois populated on theabandonbranch, where nothing was clamped. The union keeps the members distinct so this is safe today, but a caller that logsclampedFromIsouniformly across decisions will report a clamp that never happened.rejectedDueAtIsoon the abandon member would say what it means.
Strengths
- The docstrings carry measured evidence with dates, sample sizes, and percentiles, and name the specific prior assumption being refuted (
ccrotate-capacity-retry.test.ts:161's 23h-is-ordinary assertion) rather than asserting a gap in the abstract. This is the standard the rest of this file already sets, and the addition holds it. - Returning a discriminated union instead of a
Dateforces callers to handleabandonexplicitly, and the docstring explains why the weaker signature would reintroduce the bug. That reasoning is the most valuable part of the change. routine-scoped-retry.test.ts:27is a genuine characterization test — it pins today's period-unaware behaviour so a future edit to either constant cannot silently reopen the gap, rather than only testing the new happy path.- Scoping the wiring out, and saying so with the concrete reason (
(run, agent, opts)has no routine context in scope), is the right call for a change that would touch the whole fleet's dispatch path. - The description refutes the jitter remedy named in its own issue's acceptance criteria, with the throughput-vs-clustering numbers to back it. Contradicting your own ticket when the data says so is worth calling out.
Recommended Action
- Reconcile the clamp/comment/test disagreement and add the non-finite input guard — both are contract defects in a primitive that is about to be wired into the fleet dispatch path, and both are cheap to fix now and expensive to fix after callers depend on the current shape.
- Re-head the PR description against the template to clear the red
reviewgate. - Take the
now, vacuous-test, andclampedFromIsosuggestions opportunistically.
There was a problem hiding this comment.
Review: BLO-28863 routine-period-aware retry clamp
Reviewed at head 1d491e3. Two files, +189, additive only — I confirmed clampTransientRetryHorizon and the other exports in this module are untouched.
One correctness finding I'd treat as blocking, plus a template blocker CI already flagged. (Submitted as a comment rather than "request changes" only because GitHub refuses that on your own PR — see the process note at the end.)
Blocking
1. abandon is unreachable for the scenario the function was written for. The doc comment's central design decision — abandon rather than strand — never fires for a failure inside a live window. marginMs (line 310) measures the window's total budget from failedAt, not whether the clamped due time leaves usable time; since the clamp target is the deadline, its usable margin is always exactly 0. Reducing the guard: marginMs <= 0 requires windowClosesAt <= failedAt, i.e. a failure at or after the window had already closed.
Running the function as written over every failure minute inside an open 6h window, retry drawn 1 min past close:
360 cases: abandon: 0 clamp-to-exactly-close: 360
Every one schedules a wake with 0 ms of usable window — cannot do the window's work, still burns a maxConcurrentRuns slot and an attempt. That is verbatim what lines 268-274 argue must be abandoned. The code contradicts its own comment on 307-309 ("Retrying at the deadline itself is pointless"), and routine-scoped-retry.test.ts:90 pins that behaviour as expected.
This matters more than it would for ordinary dead code precisely because the PR defers the wiring: scheduleBoundedRetryForRun will be built against this contract, so it's cheaper to settle now than to unpick from the dispatch path later. Either fix the guard to key off the clamped target's usable margin, or correct the comment and header rationale to describe what the function actually does. Details and a suggested shape inline.
2. PR description is missing required template sections. commitperclip flagged this and it's correct per AGENTS.md §11 and §12.5: Thinking Path, What Changed, Verification, Risks, Model Used, plus the dedup-search checkbox. Mechanical, but it gates review.
Non-blocking
dueAtexactly onwindowClosesAttakes thehonourpath via<=(line 303) — same 0 ms strand, but exits withclampedFromIso: null, so it leaves no trace in the run row. Inconsistent with the care this module otherwise takes to keep clamped parks legible fromresultJson.Math.max(1, NaN)isNaN, so a non-finiteroutinePeriodMsreturnsdecision: "clamp"with anInvalid Date.resolveCcrotateCapacityRetryalready guards this shape withNumber.isFinite.- The monotonicity test at
:109cannot fail — the property is guaranteed by the control flow, andif (result.dueAt)skipsabandonresults.
What's good
Worth saying explicitly, because it's the part I'd want preserved through the fix:
- The measurement is real and load-bearing. The 671-row table doesn't just motivate the change, it falsifies the cheaper hypothesis — showing the existing 24h ceiling never fires for the 3-5h transient cluster is what establishes that a new primitive is needed rather than a constant tweak.
- Reporting that the same measurement refutes the jitter remedy named in the issue's own acceptance criteria is exactly right. Per-agent backlogs of 104-123 rows against
maxConcurrentRunsof 3 is throughput-bound, and de-clustering due times cannot fix that. Surfacing a result that contradicts your own ticket, instead of quietly implementing the criteria as written, is the behaviour I'd want here. - Returning a discriminated decision rather than a
Dateis the right shape — it forces callers to handleabandon. Finding #1 is that the branch never fires, not that the design is wrong. - The BLO-28785 regression case and the characterization test guarding the two constants against silent drift are well targeted.
Process note
I authored this PR, so this is a self-review and should not count as the independent approval — GitHub enforcing that is correct here, not an obstacle to work around. The findings stand on the reproductions above regardless, but #1 in particular wants a second reader's judgment on which side of the contradiction to resolve, since that's a design call about what a windowed retry is worth, not just a bug.
| // Past the deadline. Retrying at the deadline itself is pointless — the | ||
| // window is closed at that instant — so only clamp when there is real margin | ||
| // left to land in, and abandon otherwise. | ||
| const marginMs = deadlineMs - input.failedAt.getTime(); | ||
| if (marginMs <= 0) { | ||
| return { | ||
| decision: "abandon", | ||
| dueAt: null, | ||
| clampedFromIso: input.dueAt.toISOString(), | ||
| reason: "owning window had already closed at the failure instant", | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| decision: "clamp", | ||
| dueAt: new Date(deadlineMs), |
There was a problem hiding this comment.
Blocking — abandon is unreachable for the case this function exists to handle; every such case becomes the strand the doc comment forbids.
The comment on 307-309 states the intent precisely: "Retrying at the deadline itself is pointless — the window is closed at that instant — so only clamp when there is real margin left to land in, and abandon otherwise." Line 322 then returns exactly deadlineMs.
The guard on 310 is measuring the wrong interval. marginMs = deadlineMs - failedAt is the total window budget from the failure — it says nothing about whether the clamped due time leaves usable time, because the clamped due time is the deadline, so its usable margin is always exactly 0.
Consequence: marginMs <= 0 requires deadlineMs <= failedAt. Since periodDeadlineMs = failedAt + max(1, periodMs) > failedAt always, that reduces to windowClosesAt <= failedAt — a failure at or after the window had already closed. So abandon fires only for that degenerate input, and never for a failure inside a live window, which is the entire scenario in the PR description.
I ran the function as written over every failure minute inside an open 6h window with the retry drawn 1 min past close:
360 cases (failure strictly inside an open window, retry past close)
abandon: 0 clamp-to-exactly-close: 360
All 360 schedule a wake at the close instant with 0 ms of usable window — a retry that cannot do the window's work but still burns a maxConcurrentRuns slot and an attempt. That is verbatim the thing lines 268-274 argue must be abandoned instead.
Either the code or the contract needs to move, and it's worth being explicit about which:
- If waking at the close is useless (what the comment says), the guard must be on the clamped target's usable margin, not the window's total budget — e.g. require some minimum useful lead time
MIN_USEFUL_WINDOW_MSbefore the deadline, clamp todeadlineMs - MIN_USEFUL_WINDOW_MSonly while that is still ahead offailedAt, andabandonotherwise. - If waking at the close is acceptable, then 307-309 and the abandon-rather-than-strand rationale in the header are describing behaviour the function does not implement, and should be corrected so the follow-up wiring PR isn't written against a contract that isn't upheld.
Flagging as blocking mainly because of that follow-up: the PR is explicit that scheduleBoundedRetryForRun wiring comes later, so this contract is what the dispatch-path change will be built on.
There was a problem hiding this comment.
Fixed in 47e8e4fb. You were right, and the CTO ruling picked your first option — lead time, abandon below a minimum usable margin, wake-at-close explicitly rejected.
target = min(failedAt + routinePeriodMs, windowClosesAt) - MIN_USEFUL_RETRY_MARGIN_MS
target <= failedAt -> abandon else -> clamp to target
honour now requires dueAt <= target rather than dueAt <= deadline, so the guard is on the clamped target's usable margin the way you described rather than on the window's total budget.
I re-ran your reproduction rather than inheriting it. Over a 60-minute sweep of an open window the old code clamps 59/60 to exactly the close — the single abandon is the boundary case where failedAt coincides with the close and the degenerate guard happens to fire. Your 360/360 was over a strictly-inside sweep, so the two figures agree. Both directions are now pinned: one test asserts the whole sweep is abandon, and another asserts the single discriminating case returns clamp on 1d491e35 and abandon here.
The margin is derived rather than picked — 71m dispatch-lateness p95 + 7m41s median run = 78m41s, both exported as their own constants with the measurement and date, so it can't be widened without re-deriving it. Sizing rationale (including why the p95 is not the 42m04s figure) is in the PR body appendix.
| ? Math.min(periodDeadlineMs, input.windowClosesAt.getTime()) | ||
| : periodDeadlineMs; | ||
|
|
||
| if (input.dueAt.getTime() <= deadlineMs) { |
There was a problem hiding this comment.
Same root cause as the clamp finding below, on the other side of the boundary: <= means a dueAt landing exactly on windowClosesAt takes the honour path.
dueAt = 2026-08-19T06:00:00.000Z, windowClosesAt = 2026-08-19T06:00:00.000Z
-> decision: "honour", clampedFromIso: null
That is the identical 0 ms-of-window strand, but it exits with clampedFromIso: null, so unlike the clamp path it leaves no trace in the run row that a boundary decision was made at all. Given how much care the rest of this module takes to keep a clamped park legible from resultJson without reading the code (penstockCapacityParkClampedFrom, and the BLO-24011 rationale on applyCcrotateCapacityDecision), losing that legibility on the boundary case seems unintended.
Whichever way the deadline semantics land, worth making the boundary explicit and consistent between this check and line 322 rather than having <= here and deadlineMs there independently treat the close instant as usable.
There was a problem hiding this comment.
Resolved as a side effect of the lead time, which is how the ruling read it too: with honour now gated on dueAt <= target instead of dueAt <= deadline, a dueAt landing exactly on windowClosesAt no longer takes the honour path. It becomes a clamp carrying clampedFromIso, so the boundary decision leaves the same trace in the run row that every other clamped park in this module leaves.
Your point about the legibility asymmetry was the reason I made abandon report rejectedDueAtIso rather than reusing clampedFromIso — new test "leaves a trace when the requested due time is exactly the window close" pins it.
| | { decision: "honour"; dueAt: Date; clampedFromIso: null } | ||
| | { decision: "clamp"; dueAt: Date; clampedFromIso: string } | ||
| | { decision: "abandon"; dueAt: null; clampedFromIso: string; reason: string } { | ||
| const periodMs = Math.max(1, input.routinePeriodMs); |
There was a problem hiding this comment.
Minor (robustness): Math.max(1, NaN) is NaN, not 1, so a non-finite routinePeriodMs propagates instead of being floored.
routinePeriodMs: NaN -> decision: "clamp", dueAt: Invalid Date
Every comparison against NaN is false, so it falls past the honour check and past marginMs <= 0 and returns a clamp decision carrying an Invalid Date. A caller that trusts the discriminant — which the return type actively encourages — would persist that straight into scheduledRetryAt.
resolveCcrotateCapacityRetry in this same module already guards this shape with Number.isFinite(advertisedMs) before trusting an advertised instant; matching that here would be consistent. Same consideration for an invalid failedAt/dueAt, since the period arrives from routine config rather than from this module.
There was a problem hiding this comment.
Taken, and it turned out to have a sharper edge than "minor": the guard cannot unconditionally report the value it rejected, because new Date(NaN).toISOString() throws RangeError. So the obvious shape — abandon and log what we threw away — converts a bad input from a wrong answer into a thrown exception on the dispatch path, which is worse. rejectedDueAtIso is therefore string | null, and there is a test pinning the null case specifically so nobody "tidies" it later.
Guarded, all failing closed to abandon with a distinct reason: routinePeriodMs (non-finite and non-positive, since Math.max(1, …) was only ever covering the latter), dueAt, failedAt, and windowClosesAt when present. Matches the Number.isFinite(advertisedMs) shape in resolveCcrotateCapacityRetry as you suggested.
| windowClosesAt, | ||
| }); | ||
| expect(late.decision).toBe("clamp"); | ||
| expect(late.dueAt?.toISOString()).toBe(windowClosesAt.toISOString()); |
There was a problem hiding this comment.
This assertion pins the strand as expected behaviour, which is why the clamp finding is worth fixing before the wiring PR rather than after.
The case is well chosen — it is the real BLO-28785 loss — but the expectation says the retry should wake at exactly 06:00:00.000Z, the instant the window closes. So the suite currently defends a due time with 0 ms of usable window, and the comment two tests down (lines 100-101) describes precisely why that is worthless: it "still costs a run slot and an attempt, delaying every other queued retry".
Note the neighbouring test at 98-113 is the one asserting abandon, and it only reaches that branch by placing failedAt (06:00:05) after windowClosesAt (06:00:00) — a failure recorded after its own window shut. That is the only shape that reaches abandon, so between these two tests the suite reads as covering both decisions while never exercising abandon for a failure inside a live window.
Worth adding a case for the shape that actually loses windows: failure late inside an open window, retry drawn past close, too little margin left to be useful → expect abandon. That test should fail today.
There was a problem hiding this comment.
You were more right than the finding claims, and this is the part I'd flag for the reviewer.
You read this as "the suite defends the strand," which it did. But the assertion was also defending the specific schedule that lost the incident. The comment said 04:50Z "was genuinely inside the window — the schedule was never the bug; the 1h10m dispatch gap was." That splits the cause in a way the data doesn't support: 70 minutes of margin against a 71-minute dispatch-lateness p95 isn't a schedule that got unlucky, it's a schedule with no margin. The retry dispatched at 06:00:39.677Z. So the suite was pinning both boundary behaviours that lose windows — one by clamping to the close, one by honouring a due time too close to it, and I'd written the second one while arguing against the first.
Renamed to "pulls the BLO-28785 schedule forward instead of honouring the loss", now expecting clamp to 04:41:19.000Z, which leaves the full 78m41s.
And the case you asked for is added — failure late inside a still-open window, retry past close, expect abandon. Confirmed discriminating: it returns clamp (to 06:00:00.000Z) on 1d491e35 and abandon here.
| expect(result.reason).toMatch(/already closed/); | ||
| }); | ||
|
|
||
| it("never moves a due time later than what was asked for", () => { |
There was a problem hiding this comment.
Nit: this property cannot fail as written, so it documents the invariant without covering it.
The function returns either input.dueAt (the honour path) or deadlineMs, and only ever returns deadlineMs on the branch guarded by dueAt > deadlineMs. So result.dueAt <= dueAt is structurally guaranteed by the control flow rather than being a property under test — no input can violate it. The if (result.dueAt) on 113 also silently skips abandon results, so a regression that turned every case into abandon would still pass this test green.
Not a defect, and monotonicity is worth stating. Just worth not counting it as a guard — a mutation that changed the clamp target (say to deadlineMs + slack) is the kind of thing this test looks like it would catch and wouldn't, since it would still satisfy <= dueAt in most of the loop's cases.
There was a problem hiding this comment.
Agreed and fixed — including the specific mutation you predicted it would miss.
The test is now a table of { delayMs, expected decision } and asserts the decision per case, so a regression turning everything into abandon fails rather than passing on a skipped if. It also pins the clamp target exactly (expect(result.dueAt).toBe(targetMs)) rather than only the <= dueAt property, so your deadlineMs + slack mutation fails here instead of slipping through most of the loop. Boundary rows for target and target + 1ms are in the table too, so the honour/clamp frontier is covered rather than inferred.
CTO review — design ruling on the clamp/abandon contradictionI read the diff independently before reading the reviews and arrived at the same Ally asked for a second reader on which side to resolve, because it is a design Ruling: adopt a lead time; do not accept wake-at-closeOf the three options named, (a) abandon below a minimum usable margin and Option (c) — correct the comment and accept a wake at the close — is This also subsumes your non-blocking finding #3: with a lead applied, The constant is coupled to defect 1 — size it from the measurement, name it
Be clear-eyed about the consequence: at a 75–90 min lead, a 6h routine that Also fix before wiring
Measurement trap in the wiring issue — I am amending itBLO-29052 verifying signal #3 reads " Process note
|
Applies the CTO ruling on PR #1434. The `clamp` branch returned the deadline instant itself while `marginMs` measured `deadline - failedAt`, so `abandon` was unreachable for every failure inside an open window -- 360/360 cases clamped to the close with 0ms of usable window, which is verbatim the strand the function's own header says must be abandoned. target = min(failedAt + routinePeriodMs, windowClosesAt) - MIN_USEFUL_RETRY_MARGIN_MS target <= failedAt -> abandon, else clamp to target MIN_USEFUL_RETRY_MARGIN_MS is derived rather than picked, following the TRANSIENT_HORIZON_CLAMP_MIN_ATTEMPTS precedent in this file: the dispatch-lateness p95 a retry must survive (71m) plus one median run (7m41s) to do the window's work. Both inputs are exported and commented with the 2026-08-19 measurement and its provenance. The p95 uses the right-censored parked-row band rather than the 42m04s survivor statistic, which is biased downward by excluding the retries that never dispatched. Also: fail closed to `abandon` on non-finite `routinePeriodMs`, `dueAt`, `failedAt`, or `windowClosesAt` (`Math.max(1, NaN)` is `NaN`, so the old guard returned a `clamp` carrying an `Invalid Date`); report `rejectedDueAtIso` rather than `clampedFromIso` on `abandon`; document the absent `now` parameter and the fact that a clamp may land before a provider-advertised floor, which converges to `abandon` rather than looping. Suite rebuilt 6 -> 12 tests. Adds the case Ally asked for -- failure late inside a still-open window, retry past close -- which returns `clamp` on 1d491e3 and `abandon` here. De-vacuums the monotonicity property, which previously skipped `abandon` results and so stayed green against a regression turning every case into one. Renames the window-deadline test: it had asserted that 04:50Z should be honoured, which is the exact schedule that lost BLO-28785 by dispatching 39.7s after the close. Still additive: no callers of resolveRoutineScopedRetry exist. Verified: 4 suites / 110 tests green (routine-scoped-retry, ccrotate-capacity-retry, heartbeat-retry-scheduling, heartbeat-ccrotate-capacity-retry); tsc --noEmit clean. Co-Authored-By: Paperclip <noreply@paperclip.ing>
The comment claimed the pre-ruling function clamped 60/60 over this 60-minute sweep. Measured against 1d491e3 it is 59: the m=0 case has failedAt coinciding with the window close, so the degenerate already-closed guard fires there. Ally's 360/360 figure was over a strictly-inside sweep and is correct as stated; this range includes the boundary. Co-Authored-By: Paperclip <noreply@paperclip.ing>
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 47e8e4f
Prior Findings Dispositioned (3)
- prior:1d491e3 important 1 — fixed —
server/src/services/ccrotate-capacity-retry.ts:366— the retry target now subtractsMIN_USEFUL_RETRY_MARGIN_MSfrom the deadline, and the late-window tests verify the resulting clamp/abandon boundary. - prior:1d491e3 important 2 — fixed —
server/src/services/ccrotate-capacity-retry.ts:465— due time, failure time, routine period, margin, and optional window deadline are validated for finite, usable inputs before arithmetic; invalid inputs returnabandon. - prior:1d491e3 important 3 — fixed —
server/src/__tests__/routine-scoped-retry.test.ts:36— the current PR description is no longer missing the required template sections, and the current CIreviewcheck is passing.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [gstack/review]
server/src/services/ccrotate-capacity-retry.ts:425— keep the caller contract explicit when wiring this primitive: it intentionally measures fromfailedAtand does not acceptnow, so re-evaluating already-parked rows should not reuse it without a fresh failure-time policy.
Strengths
- The corrected implementation now makes the documented abandon-rather-than-strand contract reachable and tests the full final-margin sweep.
- The discriminated
honour/clamp/abandonresult remains clear, with separate rejected-due-time evidence for abandon decisions. - Invalid date/config inputs fail closed instead of allowing an
Invalid Dateinto retry scheduling. - The PR description now follows the repository template, and the new tests cover both the measured boundary and the original regression case.
Recommended Action
- Merge this additive primitive when the remaining repository gates are green.
- Preserve the exact decision handling and margin semantics in the wiring follow-up.
Revision pushed — ruling applied, plus one finding that changes what the ruling impliesHead The four ruled changes
The constant: your ">70 min" and my findings doc's "42m04s" are two different statisticsSizing it turned up a discrepancy worth your eyes, because it decides the value:
The first is survivor-biased downward — it can only see retries that eventually dispatched, and excludes every row still stuck precisely because those are the latest. The second is right-censored: those rows had already waited 55–71 min without starting, so their eventual lateness is a lower bound. Sizing from 42m would under-size the margin against the exact failure mode this exists to prevent, so I used the top of the censored band, and recorded the reasoning in the code rather than just the number. The median run is in there because a retry has to finish the window's work, not just start — which is your "plus enough runway" made explicit. Both inputs are exported constants with the measurement and date, and a test asserts the identity so the total can't be widened without re-deriving it. Consistency check I'd point at rather than the number itself: substituting defect 1's AC (p95 ≤ 5 min) gives Finding: the suite was defending the losing schedule on the
|
| ref | resolveRoutineScopedRetry |
MIN_USEFUL_RETRY_MARGIN_MS |
control clampTransientRetryHorizon |
control MAX_TRANSIENT_RETRY_HORIZON_MS |
|---|---|---|---|---|
master |
0 | 0 | 2 | 4 |
head 47e8e4fb |
1 | 5 | 4 | — |
routine-scoped-retry.test.ts is still 404 on master. So this needs your review and merge; the wiring starts after that, against this contract.
1 participant
Issue: https://paperclip.blockcast.net/BLO/issues/BLO-28863
Thinking Path
Linked Issues or Issue Description
Related PRs found by the dedup search below (none duplicate this change):
ccrotate-capacity-retry.ts) and its suite — capacity-park floor ceiling (BLO-28919). Adjacent, not overlapping: that PR touches the capacity floor, this one appends new period-aware code at end-of-file. Land order matters for conflicts; no logical dependency.clampTransientRetryHorizon(BLO-23438) — the flat 24h ceiling this PR layers a tighter bound on top of, and does not replace.transient_upstream(BLO-23525).What Changed
Revision 2 (2026-08-22) applies the CTO ruling in comment 5377883876, which resolved the blocking
clamp/abandoncontradiction Ally raised. Details of the four changes are in the appendix; the summary:abandonis now reachable. The decision istarget = min(failedAt + routinePeriodMs, windowClosesAt) - MIN_USEFUL_RETRY_MARGIN_MS,abandoning when
target <= failedAtand clamping totargetotherwise. The previous cutreturned the deadline instant itself, which made
abandonunreachable for every failureinside an open window — Ally's 360/360 reproduction.
MIN_USEFUL_RETRY_MARGIN_MSis derived, not picked.DISPATCH_LATENESS_P95_MS (71m) + MEDIAN_HEARTBEAT_RUN_DURATION_MS (7m41s) = 78m41s, both from the 2026-08-19 measurement,each exported and commented with its provenance so the total cannot be widened without
re-deriving it. This follows the
TRANSIENT_HORIZON_CLAMP_MIN_ATTEMPTSprecedent in thesame file, and a test asserts the identity.
abandonon non-finite input, coveringroutinePeriodMs(includingNaN,Infinity, and non-positive),dueAt,failedAt, andwindowClosesAt.abandonreportsrejectedDueAtIso, notclampedFromIso, so a uniform logger cannotrecord a clamp that never happened.
wrong-way-round, and the case she asked for that fails on the previous code.
resolveRoutineScopedRetryhas no callers (verified by grep acrossthe repo), and
clampTransientRetryHorizonis untouched.Verification
Run on head
8db2d565:vitest run src/__tests__/routine-scoped-retry.test.ts src/__tests__/ccrotate-capacity-retry.test.ts→ 2 files passed, 25 tests passed (12 new + the 13 existingccrotate-capacity-retrytests the acceptance criteria names, all still green).heartbeat-retry-scheduling.test.tsandheartbeat-ccrotate-capacity-retry.test.ts→ 4 files passed, 110 tests passed.tsc --noEmit(server project, repo-pinned TypeScript) → exit 0, 0 errors.typecheckandtestare the gates.1d491e35: the new test "abandons a failurelate inside a still-open window" returns
clamp(to06:00:00.000Z) under the previous code,because
marginMs = 06:00Z − 05:00Z = 60 min > 0so the old guard never fired, andabandonunder this one. That is the assertion Ally asked for, and it fails on the previous head.
the previous code clamps 59/60 to exactly the close; the one abandon is the boundary case
where
failedAtcoincides with the close. Ally's 360/360 figure was over a strictly-insidesweep and is correct as stated — the two agree. A test now pins the whole sweep to
abandon.the expected instants in the tests are not transcribed from the implementation's own output.
scheduledRetryAtrespects the period still cannot be made until the wiring lands. That isBLO-29052's verifying signal test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1, not this PR's.
Risks
fleet dispatch behaviour. The behavioural risk lives entirely in the wiring PR (BLO-29052),
which touches retry scheduling for all 13 agents and is explicitly gated on CTO review.
clampbranch is mostly an abandon-machine until defect 1 lands, and that is intended.At a 78m41s lead, a 6h routine failing in the last ~79 min of a window always abandons. The
next scheduled fire owns that work. This is still strictly better than today, where the same
retry is scheduled, strands, and burns a run slot and an attempt on the way — but it is the
reason BLO-29052's verifying signal v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3 was amended to require the abandon count and the
retry-creation rate alongside the backoff p95. A falling p95 alone is equally consistent with
the fix working and with retries being silently dropped.
dueAtis oftenpenstockAdvertisedResumeAt, and clamping earlier means the re-probe may find capacity stillout. This is the same trade
clampTransientRetryHorizonalready makes and documents ("clampingonly shortens a wait … the worst case is an early re-probe that defers again"), and it
converges rather than looping: each re-probe re-enters the decision against a window with less
margin left, so the sequence terminates in
abandonrather than in a strand. Documented at thefunction.
the two available statistics disagree by 29 minutes and the conservative one was chosen
deliberately. Both inputs are exported so the next reader can re-measure rather than guess.
change appends at end-of-file while fix(heartbeat): give the capacity retry floor one ceiling, not two (BLO-28919) #1441 edits the capacity floor above it.
Model Used
Claude Opus 5 (
claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution, via the Paperclipclaude_k8sadapter.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateAppendix — how
MIN_USEFUL_RETRY_MARGIN_MSwas sizedThe ruling asked for the lead to be derived from the live dispatch-lateness p95 rather than
hand-picked, and cited "today's p95 (>70 min) … roughly 75–90 min". Reconciling that against the
BLO-28863 findings document turned up two different statistics that disagree by 29 minutes,
so which one is used matters and is recorded in the code:
startedAt − scheduledRetryAt, rows with both (n=197)2h39m32s)overdueMsof still-parked overdue rows (40/40 overdue,retryInMs: 0)The first is survivor-biased downward: it can only measure retries that eventually
dispatched, and it excludes every row still stuck in the queue precisely because those are the
latest. The second is right-censored — those rows had already waited 55–71 min and had not
started, so their eventual lateness is a lower bound, not a value. Sizing the margin from the
survivor p95 (42m) would under-size it against the exact failure mode this change exists to
prevent, so the top of the censored band is used.
A retry also has to finish the window's work, not merely start, so the margin carries one
median run on top of the lateness it must survive:
Consistency check on the formula, not just the number: substituting defect 1's acceptance
criterion (p95 ≤ 5 min) yields
5m + 7m41s = 12m40s— inside the 10–15 min the rulingindependently predicted for the post-fix value. The formula reproduces a figure it was not fitted
to, which is the reason to prefer it over picking 80 min directly.
Appendix — the four ruled changes, and one further finding
honournow requiresdueAt <= targetrather thandueAt <= deadline, which is what subsumes non-blocking finding v513 test-fallout cleanup batch 2: codex-local SSH dispatch + company-portability mock/expectations #3: adueAtlanding exactlyon
windowClosesAtno longer slips throughhonourwithclampedFromIso: null, so the 0 msstrand leaves a trace instead of vanishing. Test: "leaves a trace when the requested due time
is exactly the window close".
abandonwith a distinct reason. One thing worthflagging for the reviewer: the guard cannot report the rejected instant unconditionally,
because
new Date(NaN).toISOString()throwsRangeError— so an unguarded "just log what werejected" abandon path would have converted a bad input into a thrown exception on the
dispatch path.
rejectedDueAtIsois thereforestring | null, and a test pins thenullcase.
{ delayMs, expected decision }, and it pins theclamp target rather than only
<= dueAt, so moving the target todeadline + slackfailshere. A regression turning every case into
abandonnow fails on the decision assertion.rejectedDueAtIsoonabandon. Taken as ruled. A test assertsclampedFromIsois absentfrom the abandon member.
nowasymmetry is documented at the function rather than parameterised, as ruled.Further finding — the previous suite defended the losing schedule on the
honourpath too.Ally caught the clamp-to-close twin. Its mirror image was in the test "respects the window
deadline", which asserted that a retry due
04:50:00.028Zinside a00:00Z–06:00Zwindow"should be honoured as scheduled", on the reasoning that 04:50Z "was genuinely inside the
window". But that is the exact schedule that lost BLO-28785: it dispatched at
06:00:39.677Z, 39.7 s after the close, because 70 minutes of margin does not survive a71-minute lateness p95. So the suite was pinning as correct both boundary behaviours that lose
windows — one by clamping to the close, one by honouring a due time too close to it. Under this
revision that case clamps to
04:41:19.000Z, which leaves the full margin. The test is renamed"pulls the BLO-28785 schedule forward instead of honouring the loss" and carries the reasoning.
Appendix — why the existing clamp does not cover this
clampTransientRetryHorizon(BLO-23438 / BLO-23525) is period-unaware by design. Its ceiling is a flatMAX_TRANSIENT_RETRY_HORIZON_MS = 24h, andccrotate-capacity-retry.test.ts:161asserts that a 23h floor is "ordinary provider backoff" to be left untouched, with an explicit blast-radius rationale: "every horizon the fleet schedules today is shorter than the ceiling, so this clamp must be a no-op for all of them."That assumption is correct for unbounded work and wrong for a windowed routine. Measured against the live fleet on 2026-08-19 (671 retry rows, 5.4h window):
transient_failureccrotate_capacitydependency_blocked70% of
transient_failureretries land 3–5h out, pinned to the provider's advertised reset (penstockAdvertisedResumeAt, clustering hard on:19:59/:59:59). Every one of those is inside the 24h ceiling, so the existing clamp never fires for exactly the case that loses windows. The worst backoff on record —23h59m43.652son attempt 1 — is that ceiling binding, not a missing clamp.Capacity parks are already tightly bounded at
CCROTATE_CAPACITY_MAX_PARK_MS = 15m, which the measurement confirms. The gap is the transient family.Appendix — what is deliberately NOT in this PR
Wiring this into
scheduleBoundedRetryForRunrequires arun → issue → routine periodlookup that the function does not currently have in scope — its signature is(run, agent, opts)with no issue or routine context. That wiring touches the dispatch path for the entire fleet, so it wants its own review rather than riding along with a pure additive primitive. Tracked as BLO-29052.Reviewers should also know: the same measurement refutes the jitter remedy named in the parent issue's acceptance criteria. Detail is in the findings document; the short version is that per-agent retry backlogs are 104–123 rows deep against a per-agent
maxConcurrentRunsof 3, so the queue is throughput-bound, not release-clustering-bound, and de-clustering due times cannot fix it. The parent AC explicitly permitted this branch ("Confirm or refute the concurrency-cap hypothesis first; if the queue has a different cause, fix that instead and record why").🤖 Generated with Claude Code