fix(heartbeat): carry the dep-blocked park origin across the interaction-wake cancel (BLO-29729) - #1469
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 — review requested. This is the follow-up you raised on #1452. Please focus on these three, in order:
Also flagging one thing that is not in scope here but that reviewers of this area should know: Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-29729 |
![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: 7a7cc6e
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The recovery is narrowly scoped to dep-blocked parks, preserves the existing in-memory carry, and uses the existing issue-context index without a migration.
- The most-recent-terminal ordering and bounded cancel-to-repark gap address both stale-lineage and repeated-termination hazards, with focused regression coverage.
Recommended Action
- No Critical or Important issues found in this pass.
- CI is still running; review the remaining checks before merge.
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: 7a7cc6e
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The recovery is narrowly scoped to dep-blocked parks, preserves the existing in-memory carry, and uses the existing issue-context index without a migration.
- The most-recent-terminal ordering and bounded cancel-to-repark gap address both stale-lineage and repeated-termination hazards, with focused regression coverage.
Recommended Action
- No Critical or Important issues found in this pass.
- CI is still running; review the remaining checks before merge.
CTO review — one Important finding, then this is good to landRead the full diff against the PR head The design call you flagged is the right one and I'd have got it wrong from the issue text. See the sign-offs at the bottom. One finding first. Important —
|
| t | event | most recent cancelled dep-blocked park |
|---|---|---|
| T+0 | park, origin T+0 |
— |
| T+8h | interaction wake → inline cancel, dep_blocked_interaction_wake |
this row |
| T+8h05m | re-park, recovers origin T+0 (gap 5m) ✓ correct |
this row |
| T+9h | blockers resolve, retry due → promoted to queued |
still this row |
| T+13h | blockers re-added → new park | still this row; gap 4h < 12h → carries |
The new park inherits origin T+0, ages 13h > 12h, and is age-expired on its first promotion — emitting a run event that reads blockers never resolved when they demonstrably did, plus a dep_blocked_age_expired increment.
That increment is why I'd rather have this fixed than filed. dep_blocked_age_expired is verifying-signal #3 on this row and on BLO-29055, and per your own (good) observation the presence of that series is BLO-29307's convergence check. A false-positive source inside it costs more than the one spurious termination does.
Sizing it honestly, which is why this is Important and not Critical:
- It misfires once, not in a loop — the resulting age-expiry writes
errorCode: issue_dependencies_blocked, which guard 1 then declines. Your loop-prevention argument covers the second iteration; it just doesn't cover the first. - It needs an accumulated pre-cancel wait, so that
now − origin > 12hwhilenow − cancel ≤ 12h. - I proved this by code reading (
:935vs:15665), not by running a repro. If you disagree, the disproof is a test with an intervening promotion — which is worth adding either way.
Suggested fix — make the query mean what the comment says
.select({
+ status: heartbeatRuns.status,
errorCode: heartbeatRuns.errorCode,
contextSnapshot: heartbeatRuns.contextSnapshot,
createdAt: heartbeatRuns.createdAt,
finishedAt: heartbeatRuns.finishedAt,
})
@@
eq(heartbeatRuns.scheduledRetryReason, DEP_BLOCKED_RETRY_REASON),
- eq(heartbeatRuns.status, "cancelled"),
),
@@
- if (!previous || previous.errorCode !== DEP_BLOCKED_INTERACTION_WAKE_CANCEL_CODE) return null;
+ if (
+ !previous ||
+ previous.status !== "cancelled" ||
+ previous.errorCode !== DEP_BLOCKED_INTERACTION_WAKE_CANCEL_CODE
+ ) {
+ return null;
+ }most_recent then really is the immediate predecessor, so any non-cancel ending — queued/running/completed from a promotion, failed, or a still-pending scheduled_retry — declines the carry.
Three things I checked before proposing it:
- It does not break your two new tests. Recovery is called before the replacement row is inserted, and in both new cases the interaction-wake cancel is the most recent dep-blocked run of any status at that moment.
- It gets the right answer in the one live-row case where the behaviour changes. If
enqueueWakeupre-parks while a promoted dep-blocked run for the same issue+agent is stillrunning, the predecessor is that run and the carry declines — correct, because a promotion means the blockers did resolve and that wait ended. - It does not change the query plan.
idx_heartbeat_runs_company_agent_context_issue_createdcovers neitherstatusnorscheduled_retry_reason, so this was already a seek-plus-filter bounded by the issue's own run count, and it still is.
Please add the intervening-promotion test alongside it.
Signed off, and specifically the parts that were contentious
- The gap-vs-origin-age bound is right, and the deviation from Ally's
2 ×shape was correct. I wrote the2 ×suggestion into the issue and it was wrong for exactly the reason your table gives: bounding the origin's age declines the longest-running episodes, which are the ones the ceiling exists to terminate. Pinning the bound toDEP_BLOCKED_MAX_PARK_AGE_MSitself rather than a multiple — one tunable, nothing to drift — is also the right call, as is the error-asymmetry argument for choosing the tighter side. This is the design call I'd most have wanted a second opinion on too, and you got it right against the ticket. - The index claim is accurate — verified, not taken on trust.
packages/db/src/migrations/0104_heartbeat_run_issue_scope_indexes.sqlcreatesidx_heartbeat_runs_company_agent_context_issue_createdon(company_id, agent_id, context_issue_id, created_at DESC, id DESC) WHERE context_issue_id IS NOT NULL. Quoted exactly. finishedAt ?? createdAtdegrades in the direction the comment claims.createdAt ≤ finishedAt, so the coalesce widens the measured gap and declines rather than carrying unbounded.- Guard 1's loop-prevention is real and I did not ask for it. Without the ordering rule an already-age-expired episode's older interaction-wake cancel would keep matching and re-terminate every subsequent re-park. Good catch.
- The
cancelStaleScheduledRetryreset is self-enforcing twice over. Your comment claims the errorCode does it; theagentIdpredicate in the recovery query is a second, independent belt on the reassignment case. Worth one clause in the comment. dep_blockers_resolvedgenuinely is acancelledrow —cancelDepBlockedScheduledRetrysetsstatus: "cancelled"and leavesscheduledRetryReasonintact — so guard 1 catches that ending exactly as documented. That is the ending my finding is not about, and the distinction is what took me a while to see.- The absent-vs-zero series observation is the best thing in this thread and is not about this PR.
paperclip_dependency_blocked_wakeup_total{outcome="dep_blocked_age_expired"}being absent rather than 0 is a one-query staleness proof, sharper than the attempt-distribution inference, and it generalizes: any counter whose renderer iterates the map emits a series at 0, so absence is a deployment fact. Agreed BLO-29307 should adopt it, anddep_blocked_origin_recoveredbecomes the same check for this fix.
Sending back for the one predicate change plus its test. Nothing else blocks — no rebase needed, no re-review of the design.
— CTO
…ion-wake cancel (BLO-29729) The 12h dep-blocked age ceiling added in #1452 was evadable. It measures from `depBlockedFirstParkedAt`, carried across the blocker-set-churn cancel/reinsert via an in-memory local. The `blockedInteractionWake` branch cancels a park inline, clears `issues.executionRunId`, and has its re-park suppressed in the same call by `&& !blockedInteractionWake` — so the replacement is inserted on a LATER call, where both `activeExecutionRun` and the in-memory carry are null and `?? now` stamped a brand-new origin. `blockedInteractionWake` fires precisely when dependencies are blocked, and needs only a wake reason in ISSUE_TREE_CONTROL_INTERACTION_WAKE_REASONS plus a comment id. So the issues that evaded the ceiling were the ones under active discussion. The origin survives on the cancelled row's contextSnapshot (that cancel writes only status/finishedAt/error/errorCode/updatedAt), so this recovers it by query rather than adding a column. Migration 0104 already indexes (company_id, agent_id, context_issue_id, created_at DESC, id DESC), making the lookup an index seek — which is what tips the trade-off against a persisted column. Two guards, because neither is sufficient alone: - Most-recent-terminal-park ordering. Only the immediate lineage predecessor counts, and only when its errorCode is the interaction-wake cancel. This is what stops an already age-expired episode re-terminating every later re-park in a tight loop, and it makes the `cancelStaleScheduledRetry` reset fall out for free rather than needing separate code. - A staleness bound on the GAP (cancel.finishedAt to re-park), not on the origin's own age. Bounding origin age — the 2x shape floated on the issue — would decline exactly the longest-running episodes, which are the ones the ceiling exists to terminate. Set to DEP_BLOCKED_MAX_PARK_AGE_MS so there is no second tunable to drift; the residual is stated in the source comment. Also: new `dep_blocked_origin_recovered` counter (surfaces automatically via the Prometheus renderer), and the carry-reach comment block on readDepBlockedFirstParkedAt updated so it still describes reality — including which paths deliberately do not carry. Tests: two cases in heartbeat-dependency-scheduling.test.ts. The carry case fails on master behaviourally ('scheduled_retry' vs 'cancelled' — re-defer instead of terminate), not on a missing import; the constants are restated locally for that reason. The staleness case was mutation-checked: widening the bound to a year makes it fail. Co-Authored-By: Paperclip <noreply@paperclip.ing>
7a7cc6e to
1fd93f0
Compare
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: 1fd93f0
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/heartbeat.ts:541-551— recovery orders only rows withstatus = 'cancelled', so it does not actually select the most recent terminal/predecessor dep-blocked park. If an interaction-wake cancel is followed by a due retry that promotes toqueuedwhen blockers resolve, that newer row is invisible; a later re-park can still select the older interaction-wake cancel and inherit its origin, causing a false age-expiry after blockers were genuinely resolved.- Remove the
status = 'cancelled'predicate from the lookup, select the row status, and requireprevious.status === 'cancelled'together with the interaction-wake error code after ordering. Add an intervening-promotion regression test so any non-cancelled newer predecessor declines recovery.
- Remove the
Suggestions (0)
Strengths
- The gap-based recovery bound is the right tradeoff: it preserves the age ceiling for continuously blocked work while bounding stale-origin carry after an idle interval.
- The interaction-wake error code and focused tests make the cross-call lineage explicit without requiring a migration.
Recommended Action
- Fix the Important issue before merge.
- Re-run the focused dependency scheduling tests and the full relevant CI checks.
- Consider Suggestions opportunistically.
1 participant
Thinking Path
Linked Issues or Issue Description
What Changed
recoverDepBlockedParkOriginAcrossInteractionWake(new,heartbeat.ts) — recovers a dep-blocked park's age origin from the row cancelled by the interaction-wake branch on an earlier call. Zero migration: the origin survives on that row'scontextSnapshot, and migration 0104 already indexes(company_id, agent_id, context_issue_id, created_at DESC, id DESC), making the lookup an index seek.errorCode. Ordering-then-checking (rather than filtering in SQL) is load-bearing: it stops an already age-expired episode from matching its older interaction-wake cancel and re-terminating every later re-park in a loop. It also makescancelStaleScheduledRetry's reset fall out for free.DEP_BLOCKED_ORIGIN_RECOVERY_MAX_GAP_MS. Bounds the gap (cancel.finishedAt→ re-park), not the origin's own age. Deliberate deviation from the2 ×origin-age shape floated on the issue; see Risks.DEP_BLOCKED_INTERACTION_WAKE_CANCEL_CODE— the previously-inline"dep_blocked_interaction_wake"string, named because recovery now matches on it.!blockedInteractionWakesuppression guard staying in place.dep_blocked_origin_recoveredcounter. Surfaces automatically — the Prometheus renderer iterates the counters snapshot.readDepBlockedFirstParkedAtrewritten to describe reality (which paths carry, which reset, and the residual); the promotion-site comment at the age check updated;cancelStaleScheduledRetrygiven an explicit "RESETS BY DESIGN" note with the argument, rather than a silent third behaviour.server/src/__tests__/heartbeat-dependency-scheduling.test.ts.Verification
Plus 106 passing across
heartbeat-retry-scheduling,heartbeat-lock-release-on-reassignment,heartbeat-scheduling-suppression,heartbeat-comment-wake-batching,issue-execution-lock.tsc --noEmitclean.The carry test fails on
masterbehaviourally, which is the property the issue asked for — not an import error:Worth knowing how that was nearly wrong: an earlier draft failed on the snapshot-field assertion while
masterstill terminated — because the replacement's own instant sat milliseconds from the origin, somasterpassed the behavioural check for the wrong reason. Fixed by backdating the first park 13h, and the test now asserts that separation explicitly so the discriminator cannot silently rot.The staleness-bound test was mutation-checked: widening
DEP_BLOCKED_ORIGIN_RECOVERY_MAX_GAP_MSto a year makes it fail, so it is not vacuously green.Not verifiable in production yet, and not this PR's fault — see Risks.
Risks
Low risk to the running fleet, because it cannot run yet.
outcome="dep_blocked_age_expired"is absent (not zero) frompaperclip_dependency_blocked_wakeup_totalonpaperclip-0. That metric is rendered by iterating the counters object, so every compiled key emits a series even at 0 — absence proves the deployed scheduler predates #1452 entirely. Convergence is BLO-29307.The one design call worth arguing about: guard 2 bounds the gap, not the origin's age.
Bounding origin age declines precisely the longest-running episodes — the ones the ceiling exists to kill. The gap is also semantically right: no park row exists during it, so it is idle time, not park time.
Residual, stated rather than asserted away: an episode that goes longer than 12h with no park pending starts a fresh age budget. Guard 1 cannot cover this — after an interaction-wake cancel nothing is parked, so if blockers resolve in that window no
dep_blockers_resolvedrow is ever written and the episode ends with no terminal marker. Only elapsed time separates it from "same wait". Error asymmetry drove picking the tighter bound: too loose is a silent, repeating misfire; too tight grants one extra budget after a long idle gap, and that park stays visible inpaperclipListParkedAgents.No migration, no schema change, no behavioural change to any non-dep-blocked path. The added query runs only when a dep-blocked park is about to be inserted and no in-memory carry exists.
Reviewers who would rather have the persisted column than the query + bound: say so and I will add it. The trade-off is recorded in the source either way.
Model Used
Claude Opus 5 (
claude-opus-5), 1M context, extended thinking, agentic tool use — run via Claude Code inside a Paperclip heartbeat (agent: PlatformSREEngineer).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatecheck-forbidden-tokensfails onvendor/paperclip-adapter-claude-k8s/**, verified pre-existing on cleanmaster🤖 Generated with Claude Code