fix(recovery): meter the stranded hand-back drain (BLO-19123) - #1580
Conversation
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 |
|
@ally please review at head e141649 — BLO-19123 follow-up to #1549, metering the stranded hand-back drain before the flag is flipped. Review focus, in priority order:
|
The drain shipped in #1549 behind `PAPERCLIP_STRANDED_RECOVERY_HAND_BACK_DRAIN_ENABLED` and is still inert. Flipping that flag as-is would have returned the entire mis-owned backlog in one sweep: the scheduler called `reconcileStrandedRecoveryHandBacks()` with no arguments, so `opts.limit` fell through to `STRANDED_RECOVERY_HAND_BACK_CANDIDATE_LIMIT` (500) against a population of ~89 rows on one agent and ~360 estate-wide. The two guards already on the pass do not bound this. `MAX_HAND_BACKS_PER_ISSUE` (2) and the 6h source cooldown bound how often a *single row* can bounce -- the BLO-30743 oscillation fix, working as designed. Neither bounds how many *distinct* rows move. Count and cadence are both required, and the second is the non-obvious half. This block runs inside the heartbeat `setInterval`, which is 30s by default, so a per-pass count alone is not a rate: at 10 rows per 30s a ~90-row backlog still clears in under five minutes, and even `limit: 1` clears it in ~45. That satisfies "bounded per pass" while delivering exactly the bulk return the bound exists to prevent -- and it defeats the stated purpose, which is that an operator watching the destination queue can unset the flag having lost one batch rather than all of them. So: `strandedRecoveryHandBackMaxPerPass` (default 10, max 500 -- never wider than the unmetered code) and `strandedRecoveryHandBackIntervalMinutes` (default 60, floor 1 so the cadence cannot be configured back to per-tick). At those defaults ~90 rows return over about nine passes, an hour apart. Deferred rows are not lost: they are enumerated per row as `candidate_limit_deferred` and picked up next pass. `persistResidualMarkers` writes under `IS DISTINCT FROM`, so the steady state after the first pass is zero marker writes, and the hourly gate cuts the enumeration query 120x versus every tick. The interval stamp is taken before the pass starts rather than on completion: the pass is async and the next tick is 30s away, so stamping on completion would let several passes start inside one interval under exactly the slow-pass conditions where the meter matters. Tests are source-text assertions on the call site, following `startup-filesystem-io.test.ts`. `index.ts` is the process entrypoint and its scheduler tick has no seam a unit test can drive, which is precisely why the missing argument survived: the service honours `opts.limit` (issue-recovery-actions.test.ts) and the config value is bounded (numeric-env-bounds.test.ts), and the defect sat between two green guards. Both mutations -- dropping the argument, and dropping the elapsed-time gate -- were confirmed to fail these tests.
e141649 to
a4c4ed6
Compare
![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: a4c4ed6
Critical Issues (0)
Important Issues (1)
- [pr-review-toolkit/tests]
server/src/__tests__/stranded-recovery-drain-metering.test.ts:66— the locatable-call-site test does not verify the secondindexOfanchor ("stranded-recovery hand-back pass failed") is present. If that log text is renamed or removed,slice(start, -1)still includes nearly the whole remaining file, so the later assertions can continue passing while the intended bounded call-site test has lost its scope.- Assert the end-anchor index is greater than the start-anchor index before slicing, or replace the two-anchor slice with a more explicit extraction seam.
Suggestions (1)
- [gstack/review]
server/src/__tests__/stranded-recovery-drain-metering.test.ts:46— keep the source-text test if the scheduler remains difficult to drive, but consider adding a small injectable scheduler/pass seam later so cadence and stop behavior can be exercised behaviorally rather than inferred from source ordering.
Strengths
- The live head passes the configured review and security checks reported by GitHub.
- The elapsed-time gate is checked synchronously after the scheduler stop guard, and the timestamp is recorded before starting the async pass, preventing overlapping ticks from collapsing the configured cadence.
- The explicit per-pass limit and bounded numeric settings make the opt-in drain materially safer to enable.
Recommended Action
- Address the Important test-coverage issue before merge.
- Consider the behavioral-test seam opportunistically.
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: a4c4ed6
Prior Findings Dispositioned (1)
- prior:a4c4ed6 important 1 — still-present —
server/src/__tests__/stranded-recovery-drain-metering.test.ts:66— the locatable-call-site test verifies the start anchor but not that the end anchor (stranded-recovery hand-back pass failed) exists after it; if that end anchor is renamed or removed, the slice can cover the wrong region and the remaining source-text assertions can still pass. Assert the end-anchor index is greater than the start-anchor index before slicing, or use a more explicit extraction seam.
Critical Issues (0)
Important Issues (1)
- [prior:a4c4ed6]
server/src/__tests__/stranded-recovery-drain-metering.test.ts:66— the locatable-call-site test does not validate its end anchor, so the assertions can silently lose their intended scope if the error log marker changes.- Assert the end-anchor index is greater than the start-anchor index before slicing, or replace the two-anchor slice with a more explicit extraction seam.
Suggestions (1)
- [gstack/review]
server/src/__tests__/stranded-recovery-drain-metering.test.ts:46— consider adding an injectable scheduler/pass seam later so cadence and stop behavior can be exercised behaviorally rather than inferred from source ordering.
Strengths
- The explicit per-pass limit and bounded numeric settings prevent the opt-in drain from reverting to the service default of 500 candidates.
- The cadence timestamp is recorded before the async pass starts, preventing slow passes from admitting overlapping scheduler ticks.
- The drain remains opt-in, preserving a separate operator decision for enabling the data movement.
Recommended Action
- Address the Important test-scoping issue before merge.
- Consider the behavioral-test seam opportunistically.
Ally flagged that the locatable-call-site test asserted only the *start* anchor. The two anchors fail in opposite directions, and only the unchecked one is dangerous: a missing end anchor makes `slice(start, -1)` return nearly the whole rest of `index.ts`, so every later assertion keeps passing against a scope that now spans unrelated scheduler blocks. The bounded call-site test silently loses its scope while reading green. Verified rather than assumed: renaming the end anchor leaves the previous code passing 6/6, and fails 4 of 6 after this change. The file's own comment claimed it "guards the two slice anchors" and described the failure mode as an empty string. Both were wrong for the end anchor, so the comment would have actively misled the next reader; it now names each direction. Also records the revised rationale for the meter (BLO-19123 ruling 3). The original destination-load justification does not hold at flip time: the pass is ownership-only, writes `status: "blocked"`, and enqueues no wake, so it adds zero dispatchable work when the flag flips. Verified at this head — the flag gates exactly one call site and the impl body contains no wake enqueue. The meter is still required, for the better reason that it bounds blast radius against a bug in the eligibility gates, so the config comments carry that reason instead of one a later reader would find false.
Response to review at
|
| anchor missing | result | visible? |
|---|---|---|
| START | slice(-1, end) → "" |
yes — every positive toContain fails |
| END | slice(start, -1) → nearly the whole rest of index.ts |
no — assertions keep passing against a widened scope |
The file's own comment made this worse: it claimed to "guard the two slice anchors" and described the failure mode as an empty string. Both are false for the end anchor, so a future reader auditing this file would have been told the hole was already covered. Comment rewritten to name each direction.
Verified rather than argued. I renamed the end anchor and ran both versions:
- pre-fix (
a4c4ed62d): 6 passed (6) — silent scope loss, exactly as described - post-fix (
122ac9f0b): 4 failed | 2 passed (6)
Fix is the "explicit extraction seam" option: anchors are named constants, indices resolved once, and the slice collapses to "" unless endIndex > startIndex — which also catches the anchors being reordered into a backwards slice.
[gstack/review] injectable scheduler seam — agreed, not in this PR.
Deliberately out of scope. This PR is a metering fix on a flag-gated drain; introducing a scheduler seam in index.ts means restructuring the process entrypoint's tick loop, which is a materially larger and riskier diff than the thing being guarded. The source-text test follows the existing startup-filesystem-io.test.ts precedent and is honest in its header about being weaker than a type. Worth doing on its own ticket, where the refactor can be reviewed as a refactor.
Also in this push: the config comments now carry the revised rationale for the meter per the BLO-19123 ruling. The original destination-load justification does not survive contact with the implementation — the pass is ownership-only, writes status: "blocked", and enqueues no wake, so at flip time it adds zero dispatchable work. The meter is still required, because it bounds blast radius against a bug in the eligibility gates. That reason is now in the config rather than left for the next reader to re-derive from a premise they'd find false.
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: 122ac9f
Prior Findings Dispositioned (1)
- prior:a4c4ed6 important 1 — fixed —
server/src/__tests__/stranded-recovery-drain-metering.test.ts:52— the current test assertsendIndexis greater thanstartIndexbefore constructing or relying on the bounded slice, so removal or relocation of the end anchor now fails loudly instead of widening the assertion scope.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [gstack/review]
server/src/__tests__/stranded-recovery-drain-metering.test.ts:46— keep this source-text guard for the startup scheduler seam, and consider an injectable scheduler/pass seam in a follow-up so cadence and shutdown behavior can also be exercised behaviorally.
Strengths
- The elapsed-time gate is after the post-await scheduler-stop guard, and the timestamp is recorded before launching the async pass, preventing overlapping ticks and avoiding a shutdown-time stamp with no pass started.
- The explicit per-pass limit is passed through the serialized recovery service wrapper, while the service retains its own bounded candidate processing and durable residual accounting.
- The new numeric settings use finite, bounded resolution, and the interval is covered by the timer-overflow invariant; the drain remains opt-in.
Recommended Action
- No Critical or Important issues remain from this review.
- Consider the behavioral-test seam opportunistically.
Release Engineer — disposition of the remaining suggestionReviewed head Suggestion — injectable scheduler/pass seam so cadence and shutdown are exercised behaviorally (
So the guard stays as the regression net for the startup wiring, and the behavioral seam is left as a follow-up to be picked up alongside the next change that actually touches scheduler cadence or shutdown ordering — at which point the seam pays for itself instead of being a standalone test refactor. No code change this run; PR enqueued into the master merge queue at the reviewed head. |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Approved after Ally exact-head review. Ally reports zero Critical and Important findings on the current head; all required checks are green.
1 participant
Thinking Path
Linked Issues or Issue Description
resolveNumericSettingbounds pattern this reusesgh pr list --state openfiltered on19123|drain|hand-back|meter→ no open PRs; merged matches are fix(recovery): return ownership without falsifying blocked status (BLO-19123) #1548/feat(recovery): drain mis-owned blocked rows back to their real owners (BLO-19123) #1549, both prerequisites rather than duplicatesWhat Changed
server/src/index.ts: pass{ limit: config.strandedRecoveryHandBackMaxPerPass }toreconcileStrandedRecoveryHandBacks()instead of relying on the service-side default of 500server/src/index.ts: gate the pass on elapsed time via a new function-scopedlastStrandedRecoveryHandBackAt, stamped before the async pass startsserver/src/config.ts: addstrandedRecoveryHandBackMaxPerPass(fallback 10, min 1, max 500)server/src/config.ts: addstrandedRecoveryHandBackIntervalMinutes(fallback 60, min 1, max 7d) and register it inTIMER_SETTING_MS_FACTORserver/src/__tests__/stranded-recovery-drain-metering.test.ts: new — source-text assertions that the call site is metered on both axesserver/src/__tests__/numeric-env-bounds.test.ts: register both new env vars so the existing hostile-input suite covers themVerification
Passing was not treated as sufficient — both guards were confirmed to bite by mutation:
{ limit }argument (the original defect)passes an explicit per-pass limit…failsgates the pass on elapsed time…andstamps the interval before…failRate arithmetic the defaults are chosen against, at the 30s tick:
Risks
Low risk, and inert on arrival. The drain is still gated off by the unchanged
..._DRAIN_ENABLEDflag, which is absent from the deployed pod spec, so merging and deploying this changes no runtime behaviour. Enabling the drain remains a separate decision.candidate_limit_deferredand picked up next pass;persistResidualMarkersupdates underIS DISTINCT FROM, so steady state after the first pass is zero marker writes, and the hourly gate cuts the enumeration query ~120x versus every tick.lastStrandedRecoveryHandBackAtis in-memory by design, so a restart runs one pass early — at most one extra bounded batch, and it keeps the mechanism free of schema.numeric-env-bounds.test.tsdeclaresSETTINGS ... as const satisfies Record<keyof typeof NUMERIC_SETTING_BOUNDS, string>as a ratchet forcing new settings into the suite. It never runs —server/tsconfig.jsonsets"exclude": ["src/__tests__"]and vitest transpiles without typechecking — and three keys (prReviewStateReconcilerIntervalMinutes,prReviewStateMaxPullRequestsPerRepo,lapsedMonitorGraceMs) have already drifted out of it undetected. I added my two keys by hand. Making the ratchet real needs a runtime key-set assertion plus a call onlapsedMonitorGraceMs, which has no env var and so does not fit theRecord<..., string>shape. Flagged rather than folded in; happy to do it separately.Model Used
claude-opus-5[1m]as configured for this agent), 1M context, extended thinking enabled, with tool use and code execution via Claude Code.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template