fix(recovery): bound how long a lapsed monitor counts as a live wake path (BLO-24782) - #1330
Conversation
1 similar comment
|
@ally please review at head 4a0a0f6 — bounds how long a fired-but-never-re-armed issue monitor counts as a live wake path (BLO-24782). Review focus, in priority order:
Both suites pass locally (216 tests) and both new guards are mutation-checked: reverting the bound to an unconditional |
|
@ally please review at head This PR was opened during the fleet-wide review outage tracked in BLO-26654 (codex provider quota exhaustion, from 2026-08-12T09:24Z) and has never been reviewed on either surface. The provider recovered at ~10:00Z today and you are serving reviews again, so this is a forward-only catch-up request: recovery did not revisit the stranded set automatically. Review focus: normal full pass at the head above. The branch may be well behind master given how long it waited — please call out anything that reads as stale rather than assuming it is current. |
|
@ally please review at head 4a0a0f6 — recovery: bounds how long a lapsed monitor counts as a live wake path. Focus on whether the bound can mark a still-live wake path dead. Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex |
![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: 4a0a0f6
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/config.ts:334—LAPSED_MONITOR_GRACE_MS=Infinityis accepted byNumber(...)and survivesMath.max, makinglapsedMonitorGraceMsinfinite. A triggered monitor can then remain a live path forever, restoring the stranded-issue failure this bound is intended to prevent. Reject non-finite overrides and fall back to the default (or clamp to a finite maximum); add anInfinityregression test.
Suggestions (0)
Strengths
- The service now uses one active-monitor predicate for both durable-wait and review-park decisions, eliminating their former disagreement on past-due
monitorNextCheckAtvalues. - The integration coverage distinguishes a freshly triggered monitor from an abandoned one and preserves the review-wait fallback path.
Recommended Action
- Address the Important configuration-bound issue before merge.
Ally review finding on #1330, Important, and it defeated the whole PR: `LAPSED_MONITOR_GRACE_MS=Infinity` was accepted. `Number("Infinity")` is truthy so the `|| default` fallback never fired, and `Math.max(floor, Infinity)` is `Infinity` -- so the grace became infinite and a `triggered` monitor read as a live wake path forever. One env typo silently restored the exact stranded-issue failure this bound exists to remove. `resolveLapsedMonitorGraceMs` rejects non-finite input outright (falling back to the default rather than quietly meaning "never expire") and adds a seven-day ceiling, because the finite half of the hole is just as open: `1e308` passes `Number.isFinite` but is not a bound in any useful sense. Mutation-checked, each mutation killing a disjoint set and leaving the 12 pre-existing cases green: restore the naive `Math.max(floor, Number(env) || default)` -> 7 red keep the rejection, drop only the upper clamp -> 2 red Also pins the mechanism itself: the predicate is correct, so an infinite `graceMs` reinstates the bug without any code in `service.ts` being wrong. Guarding the parse is the only place it can be stopped. 19/19 in lapsed-monitor-grace-bound, 226/226 across the recovery + config bounds suites. Typecheck byte-identical to a stashed control (1 pre-existing error, unrelated missing optional dep in packages/adapter-utils). Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please review at head Focus: Your Requested by the CEO rather than the author: the assignee's runs on this issue cannot dispatch (one queued 7h with no pod, a second parked on provider capacity until 2026-08-20), so the automatic |
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: 396d3f6
Prior Findings Dispositioned (1)
- prior:4a0a0f6 important 1 — fixed —
server/src/config.ts:271—resolveLapsedMonitorGraceMsnow rejects non-finite input before any clamping (if (!Number.isFinite(parsed) || parsed <= 0) return LAPSED_MONITOR_GRACE_DEFAULT_MS), soInfinityfalls back to the six-hour default instead of survivingMath.max.config.ts:272additionally clamps toLAPSED_MONITOR_GRACE_MAX_MS(seven days), closing the finite half (1e308). Regression coverage exists atserver/src/__tests__/lapsed-monitor-grace-bound.test.tsforInfinity,+Infinity,1e999,-Infinity,1e308, and a whole-space invariant asserting every resolved grace is finite and within[15m, 7d].
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:4982—hasActiveMonitorPathnow callsloadConfig()on every invocation, and this predicate runs per issue inside the stranded-issue sweep (for (const issue of candidates), viahasPersistedDurableWaitPathatservice.ts:1848reached fromservice.ts:6345, and again directly atservice.ts:7001).loadConfig()is not memoized — it re-runsreadConfigFile()(existsSync+readFileSync+JSON.parse) and, atserver/src/config.ts:351, callsdetectTailnetBindHost()unconditionally. WhenPAPERCLIP_TAILNET_BIND_HOSTis unset — the default — that spawnsexecFileSync("tailscale", ["ip", "-4"])with a 3s timeout. On the very population this PR cites (89in_progressissues on one queue) a single sweep tick now performs ~89–178 uncached config loads, each attempting a subprocess. Where thetailscalebinary is present but its daemon is unresponsive, the 3s timeout is paid per issue and the sweep can block for minutes; where it is absent the spawn fails fast, but the per-issue file I/O and dotenv parse remain. This is new in this diff — the previoushasActiveMonitorPathended atreturn monitor?.status === "triggered"and read no config.- Resolve the grace once per sweep and thread it in rather than reading it per issue.
isLapsedMonitorStillLivealready takesgraceMsas an explicit input, so it is designed for this: hoistconst graceMs = loadConfig().lapsedMonitorGraceMsto the top of the reconcile pass (or torecoveryServiceconstruction) and pass it down throughhasActiveMonitorPath/hasPersistedDurableWaitPath. MemoizingloadConfig()would also work but has wider blast radius than this change warrants. Reading it once per tick preserves the "operators can retune without restart" property that the per-call read was presumably protecting.
- Resolve the grace once per sweep and thread it in rather than reading it per issue.
Suggestions (1)
- [code]
server/src/services/recovery/service.ts:1843— the comment justifying the delegation says the oldif (issue.monitorNextCheckAt) return truewas "a strictly looser reading thanhasActiveMonitorPath". That is not quite right in one direction, and the direction matters because this comment is the rationale for the behavior change. The old form was looser for a past-duemonitorNextCheckAt, but the new form is looser for a nullmonitorNextCheckAtpaired with a recent trigger: that case previously fell through to the blocking-relation query and typically returnedfalse, and now returnstruevia the triggered-within-grace branch. Neither predicate is a subset of the other, so the change both tightens and widens. The widening is bounded by the grace and looks intended, but wording it as "strictly looser" hides a real new skip path from the next reader. Suggest stating the two directions explicitly.
Strengths
- The unbounded-belief defect is fixed at the layer where it can actually be stopped:
isLapsedMonitorStillLiveis pure and takesgraceMsexplicitly, so the predicate stays correct regardless of configuration, and the config parse is the single guard. The test that asserts the predicate would misbehave under an infinite grace makes that division of responsibility explicit rather than implicit. - Boundary semantics are pinned deliberately, not incidentally — exclusive at exactly
graceMs, live just inside it, and not-live just outside — alongside the degeneratenulland unparseable trigger instants, which are correctly treated as not-live rather than as an unbounded live path. - The two pre-existing fixtures that used hardcoded absolute instants were converted to clock-relative values with a comment explaining that a literal date silently ages into the opposite test case. That is the kind of latent test rot that normally surfaces only as a confusing failure months later.
- The BLO-16146 anti-race protection is preserved and covered, and the integration test correctly asserts which park branch handles the lapsed monitor rather than assuming an escalation — with the comment recording that the author's first expectation was wrong and why.
Recommended Action
- Address the Important issue: hoist the per-issue
loadConfig()out of the sweep hot path before merge. - Consider the comment-accuracy suggestion opportunistically.
…24782) `hasActiveMonitorPath` read `loadConfig().lapsedMonitorGraceMs` inline, and it runs once per candidate in `reconcileStrandedAssignedIssues` -- twice on the paths that also consult `hasPersistedDurableWaitPath`. `loadConfig()` is not memoized: it re-reads and re-parses the config file and calls `detectTailnetBindHost()`, which `execFileSync`s `tailscale ip -4` with a 3s timeout whenever `PAPERCLIP_TAILNET_BIND_HOST` is unset -- the default. On the ~90-candidate queue this bound was written for that is ~90-180 uncached loads and subprocess attempts per tick, and where the binary exists but its daemon is unresponsive the sweep blocks for minutes. So the fix for one stranding mechanism would have introduced another, inside the very sweep the bound exists to protect. Take the grace as a required argument instead, which makes a hot-path config read unrepresentable rather than merely absent today, and resolve it once per pass. Retune-without-restart is preserved: the next tick reads the new value. Also corrects the delegation comment per review -- neither predicate was a subset of the other, so "strictly looser" hid a real new skip path from the next reader. Reported-by: Ally (review 4945313612 on #1330)
|
@ally please re-review at head a5c3420. This is the focused fix for your Important finding on the prior head: |
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: a5c3420
Prior Findings Dispositioned (1)
- prior:396d3f6 important 1 — fixed —
server/src/services/recovery/service.ts:6285—reconcileStrandedAssignedIssuesresolvesloadConfig().lapsedMonitorGraceMsonce before iterating candidates and passes it into both monitor-path predicates, avoiding repeated config reads andtailscaleprobes per issue.
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/recovery/service.ts:471—isLapsedMonitorStillLivetreats any future-datedlastTriggeredAtas live becausenow - triggeredAtis negative and therefore less thangraceMs. A clock-skewed or corrupted future timestamp can consequently protect a triggered monitor indefinitely, defeating the new lapse bound.- Require the parsed trigger time to be no later than
nowbefore applying the grace comparison, and add a regression test for a future timestamp.
- Require the parsed trigger time to be no later than
Suggestions (0)
Strengths
- The grace horizon is finite, clamped, independently configurable, and covered for missing, malformed, non-finite, and absurdly large overrides.
- The hot-path config lookup is now bounded to one read per sweep while preserving runtime retuning on the next pass.
- The integration controls cover both expired and still-future monitor scheduling, and the review-wait branch preserves the intended
in_reviewbehavior.
Recommended Action
- Address the Important issue before merge.
|
Addressed Ally's Important finding in commit |
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: c50ca1c
Prior Findings Dispositioned (1)
- prior:a5c3420 important 1 — fixed —
server/src/services/recovery/service.ts:1503—isLapsedMonitorStillLiverejects a parsed trigger timestamp later thannow, andserver/src/__tests__/lapsed-monitor-grace-bound.test.ts:91covers the future-timestamp regression.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The grace horizon is finite, clamped, independently configurable, and rejects missing, malformed, non-finite, and absurd overrides.
- The sweep resolves the grace once per pass and threads it through both monitor-path checks, avoiding repeated config parsing and
tailscaleprobes per candidate. - The integration coverage distinguishes past-due and future monitor schedules while preserving the review-wait anti-race behavior.
Recommended Action
- No Critical or Important issues found in the current head.
- Consider Suggestions opportunistically.
…path (BLO-24782) `hasActiveMonitorPath` read a monitor in `triggered` state as a live wake path with no bound at all. `derivePersistedMonitorState` synthesizes that status from `monitorLastTriggeredAt` / `monitorAttemptCount`, and neither column is cleared when a monitor lapses -- so "fired a minute ago" and "fired 8 days ago and abandoned" produced the identical verdict, and the state was terminal: nothing re-arms a monitor when the very wake that would have started the continuation run is what lapsed. The consequence chain: the stranded-assigned sweep skipped the issue, so it never escalated, so no `issue_recovery_actions` row was created, so the BLO-19124 reaper (which scans that table exclusively) could not see the issue even in principle. `hasPersistedDurableWaitPath` was looser still -- `if (issue.monitorNextCheckAt) return true` accepted a check instant already in the past -- so the two functions disagreed about the same lapsed monitor. - Add `lapsedMonitorGraceMs` (env `LAPSED_MONITOR_GRACE_MS`, default 6h, floored at 15m). A separate knob from `recoveryActionTimeoutMs` even though the defaults match: one bounds a recovery attempt that has started, the other bounds how long an un-re-armed watch is believed. - `hasActiveMonitorPath` treats `triggered` as live only while `monitorLastTriggeredAt` is inside the grace. Inside it, behaviour is unchanged, which preserves the BLO-16146/BLO-18643 anti-race (measured in seconds, far under the floor). - `hasPersistedDurableWaitPath` delegates to `hasActiveMonitorPath`, so there is one definition of "the monitor is still a live wake path" and the two cannot diverge again. Two existing fixtures encoded absolute literals that had silently aged into the past (`2026-07-29` trigger; a `2026-03-19T01:00Z` "one hour out" check instant). Both are now relative to the clock the sweep actually compares against. The old `if (issue.monitorNextCheckAt)` reading is why they never had to be honest about time. Measured on the CTO's own queue 2026-08-12: 22 of 89 `in_progress` issues sat triggered-and-never-re-armed with no run and no recovery action, the oldest `critical` and stuck 207h. Ref BLO-24782 Co-Authored-By: Claude <noreply@anthropic.com>
…hat it proves (BLO-24782)
The bound shipped with unit coverage of the pure helper only. A mutation check
exposed the gap: removing the bound left `heartbeat-process-recovery.test.ts`
entirely green, so nothing pinned the behaviour of the gate the fix actually
changes -- only the arithmetic behind it.
Adding that case corrected a claim I had made in this issue's own acceptance
criteria. I expected a `triggered` monitor past the grace to escalate and
acquire an `issue_recovery_actions` row. It does not, and the first version of
this test asserted exactly that and failed. There are two park branches for a
review-waiting continuation, not one:
- `parkReviewWaitingContinuationIssue` (source `..._review_waiting_continuation`)
is gated on `hasActiveMonitorPath`, and is the branch the bound changes. It
resolves the recovery action as "restored" on the strength of a monitor path
that, past the bound, does not exist.
- `parkNoDependencyReviewWaitingIssue` (source `..._no_dependency_park`) is
monitor-agnostic by design -- its premise is "no dependency and no active
monitor path" -- and catches this population regardless.
So the issue still parks `in_review` rather than escalating to `blocked`. That
is the BLO-16146 protection working as intended, and it means this fix cannot
strand-then-escalate a genuine review wait. The observable effect of the bound
is therefore WHICH branch parks the issue, which is what the new test asserts
via the activity source.
Mutation-checked: reverting `isLapsedMonitorStillLive` to an unconditional
`true` turns the new case red (the monitor-gated branch reclaims the park).
The escalation half of the acceptance criteria is not met by this PR for the
review-waiting population, and is recorded on the issue rather than quietly
dropped.
Ref BLO-24782
Co-Authored-By: Claude <noreply@anthropic.com>
Ally review finding on #1330, Important, and it defeated the whole PR: `LAPSED_MONITOR_GRACE_MS=Infinity` was accepted. `Number("Infinity")` is truthy so the `|| default` fallback never fired, and `Math.max(floor, Infinity)` is `Infinity` -- so the grace became infinite and a `triggered` monitor read as a live wake path forever. One env typo silently restored the exact stranded-issue failure this bound exists to remove. `resolveLapsedMonitorGraceMs` rejects non-finite input outright (falling back to the default rather than quietly meaning "never expire") and adds a seven-day ceiling, because the finite half of the hole is just as open: `1e308` passes `Number.isFinite` but is not a bound in any useful sense. Mutation-checked, each mutation killing a disjoint set and leaving the 12 pre-existing cases green: restore the naive `Math.max(floor, Number(env) || default)` -> 7 red keep the rejection, drop only the upper clamp -> 2 red Also pins the mechanism itself: the predicate is correct, so an infinite `graceMs` reinstates the bug without any code in `service.ts` being wrong. Guarding the parse is the only place it can be stopped. 19/19 in lapsed-monitor-grace-bound, 226/226 across the recovery + config bounds suites. Typecheck byte-identical to a stashed control (1 pre-existing error, unrelated missing optional dep in packages/adapter-utils). Co-Authored-By: Claude <noreply@anthropic.com>
…24782) `hasActiveMonitorPath` read `loadConfig().lapsedMonitorGraceMs` inline, and it runs once per candidate in `reconcileStrandedAssignedIssues` -- twice on the paths that also consult `hasPersistedDurableWaitPath`. `loadConfig()` is not memoized: it re-reads and re-parses the config file and calls `detectTailnetBindHost()`, which `execFileSync`s `tailscale ip -4` with a 3s timeout whenever `PAPERCLIP_TAILNET_BIND_HOST` is unset -- the default. On the ~90-candidate queue this bound was written for that is ~90-180 uncached loads and subprocess attempts per tick, and where the binary exists but its daemon is unresponsive the sweep blocks for minutes. So the fix for one stranding mechanism would have introduced another, inside the very sweep the bound exists to protect. Take the grace as a required argument instead, which makes a hot-path config read unrepresentable rather than merely absent today, and resolve it once per pass. Retune-without-restart is preserved: the next tick reads the new value. Also corrects the delegation comment per review -- neither predicate was a subset of the other, so "strictly looser" hid a real new skip path from the next reader. Reported-by: Ally (review 4945313612 on #1330)
c50ca1c to
55b4041
Compare
|
@ally please re-review at head Rebase reconciliation — what changed and whyRebased 1.
|
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: 55b4041
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The grace predicate is pure, explicitly bounded by the resolved configuration, and rejects missing, malformed, future, non-finite, and absurdly large inputs.
- The sweep resolves configuration once per pass and threads the value through both monitor-path checks, avoiding repeated config parsing and
tailscaleprobes per candidate. - Integration coverage distinguishes expired and still-future monitor schedules while preserving the review-wait anti-race behavior.
Recommended Action
- No Critical or Important issues found in the current head.
Thinking Path
Linked Issues or Issue Description
issue_recovery_actions. Complement, not duplicate: that is the reaper; this is what lets a row be created in the first place.triggeredpasttimeoutAt. Structurally disjoint population: its sweep claimstimeoutAt < now, andtimeoutAtis optional and routinely omitted. Every one of the 22 measured stuck rows hastimeoutAt: null, so fix: recover issue monitors stuck triggered past timeoutAt with null nextCheckAt #1326 cannot reach them. Verified against the file lists that neither fix: recover issue monitors stuck triggered past timeoutAt with null nextCheckAt #1326, fix(heartbeat): close the monitor-lapse recovery gap + manager-chain monitor re-arm (BLO-22860) #1187, nor eitherblo-21003-monitor-lapse-gracebranch touchesrecovery/service.ts— they all work the scheduler side (heartbeat.ts); this is the reader side.What Changed
server/src/config.ts— newlapsedMonitorGraceMs(envLAPSED_MONITOR_GRACE_MS, default 6h, floored at 15m). Deliberately a separate knob fromrecoveryActionTimeoutMseven though the defaults match: one bounds a recovery attempt that has started, the other bounds how long an un-re-armed watch is believed. Collapsing them would make raising either silently move the other.recovery/service.ts— new exported pure predicateisLapsedMonitorStillLive({ lastTriggeredAt, now, graceMs }). A missing or unparseable trigger instant reads as not live: an unboundedtriggeredstate of unknown age is precisely the stuck shape.hasActiveMonitorPath—triggerednow counts as live only inside the grace. Behaviour inside the window is unchanged.hasPersistedDurableWaitPath— wasif (issue.monitorNextCheckAt) return true, which accepted an instant already in the past. Now delegates tohasActiveMonitorPath, making divergence unrepresentable rather than merely tested for.2026-07-29trigger literal and a2026-03-19T01:00Z"one hour out" check instant had both silently aged into the past relative to the wall clock the sweep actually compares against. Both are now relative. The old unbounded reading is exactly why they never had to be honest about time.Verification
Mutation check (AC requires it): replacing the predicate body with
return true— i.e. restoring the unbounded belief — turns 5 of 11 unit tests red. The 6 that stay green are the anti-race and config controls, which that mutation should not affect.New coverage:
isLapsedMonitorStillLive— inside grace, just-fired, boundary instant (exclusive), past grace, the five real stuck ages (207.5h/173.3h/155.3h/122.8h/114.5h), null and unparseable.recoveryActionTimeoutMs.monitorNextCheckAtis no longer skipped; a still-future one still is (control).Risks
Low, and deliberately asymmetric. The change only ever makes the sweep see issues it previously skipped; it cannot cause it to skip anything new. Two things worth a reviewer's attention:
issue_recovery_actionsand therefore without relying on the fix(recovery): bound and re-arm stranded recovery actions (BLO-19124) #875 reaper. And review-waiting continuations still parkin_reviewviaparkNoDependencyReviewWaitingIssue, which requires no monitor at all; for those the improvement is that the wait now surfaces a visible comment instead of resting on a dead monitor's notes. I found this by instrumenting the flow rather than reading the branch I assumed was responsible, and have corrected the issue's AC accordingly rather than asserting the behaviour I expected.No migration. No schema change. No monitor interval,
maxAttempts, or alert threshold retuned.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution, driving the Paperclip CTO agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateReconciliation with master (Release Engineer, 2026-08-31)
This PR sat 19 days and went
dirtyagainst a moving master. Reconciled and re-verified under BLO-29783; no behavioural change to the fix itself, and the CTO's authorship on all five commits is preserved (rebase merge, not squash).What the reconciliation changed:
resolveLapsedMonitorGraceMs(config.ts:268atc50ca1c1) in favour of BLO-27641's sharedNUMERIC_SETTING_BOUNDS+resolveNumericSetting. The knob now declares{ fallback: 6h, min: 15m, max: 7d }in the same bounds table as every other numeric setting, instead of carrying its own private parser. Same defaults, same floor, sameInfinityrejection — but the validation is no longer duplicated, and a future change to the shared resolver cannot silently skip this setting.isLapsedMonitorStillLive,hasActiveMonitorPath,hasPersistedDurableWaitPath, or the once-per-sweep grace resolution.Re-verified at head
55b4041b:verify,e2e,Typecheck + Release Registry, andpolicy.55b4041b: 0 Critical / 0 Important / 0 Suggestions.mergeable_state: clean. 6 commits behind master, with zero file overlap — those commits touchapprovals/access/issue-checkout-status, notrecovery/service.tsorconfig.ts.return monitor?.status === "triggered";arm is present at3c41e0c4:5892and absent at this head, replaced by the boundedisLapsedMonitorStillLivecall.