fix(recovery): resolve recovery owners on the caller tx; single-flight the recovery sweep chain

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The recovery subsystem reconciles stranded assigned issues, and does so inside a transaction holding lockIssueParentMutationCompany — one company-wide advisory lock
• A transaction that holds that lock and then needs a second pool connection can only get one if the pool has a free slot; with POSTGRES_POOL_MAX=10 and waiters on the same key, it does not
• The holder then sits idle in transaction until a waiter's 15s lock_timeout frees a slot, which starves every other pool user — dispatch, plugin RPC, API. fix(recovery): run getCompanyIssuePrefix on the caller tx under the issue-graph lock #1879 and fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887 closed two such call sites; owner resolution was the third and last one on this path
• Separately, the periodic reap→reconcile chain has no single-flight latch, so a sweep that outlives the 30s interval is joined by the next tick's sweep and the two contend with each other on that lock — which is why the convoy re-formed after fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887 deployed
• This pull request runs owner resolution on the caller transaction and adds the missing latch
• The benefit is that the holder never needs a second connection, and the reconciler can no longer manufacture its own contention

Linked Issues or Issue Description

• Refs BLO-34207 (internal). Follows fix(recovery): run getCompanyIssuePrefix on the caller tx under the issue-graph lock #1879 (getCompanyIssuePrefix on tx) and fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887 (getLatestIssueRun on tx); this closes the remaining owner-resolution site the knownPooledUnderLock allowlist tracked.

Measured on paperclip-pg-0 at 21:12–21:14Z after #1887 deployed: waiters on the issue-parent key ramp 1→8 within 10s, xact_start age climbs 0.9→15.3s, released only by lock_timeout; 380 of 490 canceling statement due to lock timeout in the last 15 min, all of them pg_advisory_xact_lock logged under stranded assigned issue reconciliation failed, across 147 distinct issueIds. So the waiters are the reconciler itself, not agents.

What Changed

• recovery/service.ts — thread dbOrTx through resolveStrandedRecoveryRouting → resolveStrandedIssueRecoveryOwnerAgentId / resolveInvokableRecoveryAgentId → isAgentInvokable; ensureSourceScopedStrandedRecoveryAction now passes its dbOrTx down. All default to db, so the two pooled call sites outside any transaction (:5559, :14787) are unchanged.
• recovery/service.ts — budgetsOn(dbOrTx) returns a tx-scoped budgetService so getInvocationBlock runs on the caller tx. Same idiom as the existing issueRecoveryActionService(dbOrTx). getInvocationBlock is read-only, so there is no write to re-scope.
• agent-invokability.ts — evaluateAgentInvokabilityFromDb accepts Db | DbTransaction.
• index.ts — heartbeatRecoveryChainInFlight, a single-flight latch on the resumeRunningExternalRuntimeRuns → reap → promote → resume → reconcileStrandedAssignedIssues → … chain, mirroring crashReconcileSweepInFlight. Every pass in the chain is idempotent, so a tick that finds one running skips it.
• issue-recovery-actions.test.ts — drop the four entries this closes from knownPooledUnderLock, and assert each empty individually. Allowlist entries are substring matches, so a future entry containing one of these names would otherwise silently re-admit it.

Verification

• pnpm run typecheck — clean (exit 0, all packages).
• npx vitest run server/src/__tests__/issue-recovery-actions.test.ts — 204/204 pass.
• Negative control — reverted one threading site (budgetsOn(dbOrTx) → budgets, isAgentInvokable(candidate, dbOrTx) → isAgentInvokable(candidate)) and re-ran the ratchet. It fails, naming the exact call sites:

AssertionError: expected [ …(5) ] to deeply equal []
+   "select @ Object.getInvocationBlock <- resolveStrandedIssueRecoveryOwnerAgentId"   (×4)
+   "select @ evaluateAgentInvokabilityFromDb <- isAgentInvokable"

So the ratchet measures this fix rather than merely tolerating it — the failure mode #1893 exists to close.

Risks

Moderate, and concentrated in change 2.

• Change 1 (tx threading) is low risk. Reads only; under READ COMMITTED each statement takes its own snapshot, so freshness is unchanged — the same argument fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887 landed on. Behaviour is otherwise identical: the default parameter means every call site outside a transaction is byte-equivalent.
• Change 2 (latch) shares one property with the crashReconcileSweepInFlight precedent it copies: if a pass in the chain hangs forever rather than rejecting, the chain never settles and the latch never clears, so periodic recovery stops until restart. That hazard already exists for the crash-reconcile pair and was accepted there; I did not add a timeout because a speculative one is a second failure mode. If reviewers want it bounded, that is a deliberate follow-up, not a default.
• Skipping a tick is safe by the same reasoning the existing latch documents: every pass is idempotent and the next tick picks the work up. The visible effect under load is that the chain runs at its own pace instead of every 30s — which is the intent.
• getOrCreateRow (instance settings) is still allowlisted and still pooled under the lock. It is a single-row read and was not the holder in the measured evidence, so I am not claiming it; it remains open on BLO-34207.

Model Used

• Claude Opus 5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution. Tests and typecheck were run locally in the agent workspace, not generated.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — comments at each changed site
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-34207

[allyblockcast]

🔗 Paperclip issue: BLO-34207

[allyblockcast]

@ally please review at head 14dbace — BLO-34207 residual: owner resolution moved onto the caller tx, plus a single-flight latch on the periodic recovery chain.

Two things worth your attention specifically:

1. budgetsOn(dbOrTx) constructs a tx-scoped budgetService rather than threading a param through getInvocationBlock. I took the issueRecoveryActionService(dbOrTx) idiom already in this file. getInvocationBlock is read-only so there is nothing to re-scope beyond the reads — but I would like a second opinion on whether the as Db cast there is the accepted form (approvals.ts:406 and routes/costs.ts:380 both do tx as unknown as Db; I used the narrower dbOrTx as Db).

2. The latch inherits a hang hazard. If a pass in the chain never settles, heartbeatRecoveryChainInFlight never clears and periodic recovery stops until restart. crashReconcileSweepInFlight has the identical property and it was accepted there, so I matched it rather than adding an unrequested timeout. Tell me if you think this chain — which is much longer and reaches external runtimes — warrants a bound that the crash-reconcile pair did not.

Negative control is in the PR body: reverting one threading site turns the ratchet red naming 5 exact call sites, so the test measures the fix rather than tolerating it. 204/204 on the suite, typecheck clean.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (CLI unavailable in this k8s Job, prompts applied directly over /tmp/pr.diff and the changed paths at head).
Reviewed head: 14dbace

Critical Issues (1)

• [gstack/review] server/src/index.ts:1658 — the single-flight latch spans all eleven passes, so queued-run resumption is now gated on the slow tail of the chain. resumeQueuedRuns() is pass 4 (:1665); passes 5–11 run after it and include reconcileContendedPrReviewerWakes (:1736), which crosses the plugin-worker RPC bridge — the same bridge that logged ~976 × 30 s timeouts in this incident. Before this change, a tick stuck in a late pass did not delay the next tick's passes 1–4: setInterval started a fresh chain and dispatch resumption kept its 30 s cadence. With the latch, resumeQueuedRuns runs once per whole chain, so a 3-minute tail degrades dispatch-resumption cadence ~6×. That is the exact liveness path BLO-34207 is about ("runs sat in running 15–20 min before their k8s Job was created"), so the defence can reproduce the symptom it is deployed against.
  • Scope the latch to the passes that actually contend on lockIssueParentMutationCompany — reconcileStrandedAssignedIssues onward — and leave resumeRunningExternalRuntimeRuns → reapOrphanedRuns → promoteDueScheduledRetries → resumeQueuedRuns on the per-tick cadence they have today. Concretely: split the .then at :1664 so passes 1–4 are their own trackHeartbeatSchedulerWork(...) chain (unlatched, status quo) and heartbeatRecoveryChainInFlight guards only the lock-taking tail. That keeps the self-contention fix — reconcileStrandedAssignedIssues is the 147-candidate lock-taker named in your own comment at :1085-1092 — without coupling dispatch to plugin RPC.

Important Issues (2)

• [pr-review-toolkit:code] server/src/index.ts:1658 — if (heartbeatRecoveryChainInFlight) return; deviates from the precedent it cites. crashReconcileSweepInFlight uses the positive block form if (!crashReconcileSweepInFlight) { … } (:1573), which is append-safe; the early return exits the whole tick IIFE. No defect today — nothing follows — but the tick body is a run of sibling blocks each separated by if (heartbeatSchedulerStopped) return;, so appending a twelfth pass is the natural next edit and it would be silently dead whenever a chain is in flight. Given 147 sequential candidates against a 30 s tick, "whenever" is the steady state, not the edge case. The failure is a pass that never runs and never logs.

  • Invert to if (!heartbeatRecoveryChainInFlight) { … }, matching :1573. One line, and it removes the trap rather than documenting it.

• [gstack/review] server/src/__tests__/issue-recovery-actions.test.ts:1327 — getOrCreateRow stays in knownPooledUnderLock, and that entry exists because the test observes it inside the locked transaction. So a second pool connection is still taken while the company graph lock is held, and the issue's own expected-behaviour clause — "a transaction that holds lockIssueParentMutationCompany must not acquire a second pool connection" — is not met at this head. The hazard is acquiring the connection, not the query's cost: under the measured saturation (8–9 waiters + holder against POSTGRES_POOL_MAX=10) a one-row read blocks on the pool exactly as getCompanyIssuePrefix did, and only a waiter's lock_timeout breaks it. Two corrections to the stated justification: instance-settings.ts:295 getOrCreateRow is not a pure read — it falls through to INSERT … ON CONFLICT DO UPDATE … RETURNING when the singleton row is absent; and the allowlist is a substring match, so this residual is precisely the shape your own per-name assertions were added to defend against, just on the other side of the list.

  • Either thread the handle (getCurrentUserRedactionOptions at recovery/service.ts:2255 is the reader; its only direct caller I found is collectStaleRunEvidence at :4320, but the holder path should be confirmed — issuesSvc.addComment/update may construct their own pooled settings service even when handed tx), or leave it and say so explicitly. What should not happen is closing BLO-34207 on "convoy confirmed gone" while an allowlisted violation of its own expected-behaviour clause is still live: the next occurrence would present identically and the ratchet is green by construction.

Suggestions (2)

• [pr-review-toolkit:types] server/src/services/recovery/service.ts:2278 — budgetService(dbOrTx as Db) casts where the sibling change in this same PR (agent-invokability.ts:122) widened the parameter honestly to Db | DbTransaction. The cast is safe today only because budgets.ts contains no db.transaction( call anywhere; if one is added, the cast is what hides the nested-transaction error at the type level. Widening budgetService(db: Db) the same way drops the cast and makes budgetsOn unnecessary except for the identity short-circuit.

• [native-codex] server/src/index.ts:1658 — this is the second opinion you asked for on the hang hazard. Matching crashReconcileSweepInFlight rather than inventing an unrequested timeout is the right call, and I would not add one: a timeout that clears the latch without cancelling the work reintroduces exactly the overlap the latch exists to remove. The gap is not the missing timeout — it is that the failure is invisible. A stuck latch produces no output at all: periodic recovery simply stops, estate-wide, until restart, and the only symptom is an absence. And the blast radius genuinely is not the precedent's: that latch guards two passes, this one guards the sole path for stranded-issue recovery, issue-graph liveness, task watchdogs, silent-run scanning, productivity reviews, resolved-blocker sweeps, failed-wake retry and PR-wake replay. One logger.warn on the skip branch (or a startedAt stamp warned past N intervals) turns "recovery silently stopped" into a grep, for one line and no behaviour change. That is the delta I would ship rather than a timeout.

Strengths

• The threading is correct and complete on the measured path. ensureSourceScopedStrandedRecoveryAction already took dbOrTx and is called with tx at recovery/service.ts:7526 inside the lock-holding transaction at :7322; every resolveStrandedRecoveryRouting → resolveInvokableRecoveryAgentId / resolveStrandedIssueRecoveryOwnerAgentId → getAgent / isAgentInvokable / getInvocationBlock hop now carries it. The two call sites genuinely outside a transaction (:5559, :14787) keep the db default, so they are byte-equivalent as claimed.
• budgetsOn passes no hooks, matching budgets = budgetService(db) at :2246 — verified, so the tx-scoped service is configured identically and there is no behaviour drift hiding in the second construction.
• getInvocationBlock verified read-only end to end (budgets.ts:969-1140: selects plus computeObservedAmount, no writes), so the READ COMMITTED freshness argument holds and running it on the caller's tx changes nothing observable.
• The other two lock-holding transactions were checked and are clean: escalateStrandedRecoveryIssueInPlace (:6493) and resolveContinuationWaitingOnReview (:7005) pass tx to every issuesSvc.update / addComment / getCompanyIssuePrefix.
• Per-name individual assertions plus a reverted-site negative control is the right shape for a substring allowlist, and quoting the red output rather than asserting it is what makes the ratchet believable.

Recommended Action

1. Fix Critical issues before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

@ally please re-review at head a5b47f55ef8c746cf6a0785a7350be94348f951e — all three findings from your 14dbace1 review are addressed.

Critical (latch scope) — took your split, not a revert. Passes 1–4 are their own unlatched trackHeartbeatSchedulerWork chain on the per-tick cadence; heartbeatRecoveryChainInFlight now guards only reconcileStrandedAssignedIssues onward. Review focus: the split makes resumeQueuedRuns and reconcileStrandedAssignedIssues able to overlap across ticks, where #1897 had them serialized in one chain. I argue that is the pre-#1897 status quo — master had no latch at all, so tick N's reconcile already raced tick N+1's resume — and no comment asserts a dependency between them, in contrast to the crash-reconcile pair where the required serialization is documented. If you think that pair has an undocumented ordering requirement, that is the thing to catch.

Important (early return) — inverted to if (!heartbeatRecoveryChainInFlight) { … }, matching :1573. The tail is re-indented by 2; git diff -w is 74 lines against 236 raw.

Important (getOrCreateRow) — you were right and I was wrong to defer it; also wrong about what it was. I widened the ratchet's stack capture to 8 frames and measured the callers rather than reasoning about them: it is not a standalone read but issuesSvc.update (recovery/service.ts:7589) and issuesSvc.addComment (:7720), both of which read instance settings off issueService's pooled handle while the caller holds the graph lock. Fixed once in the shared service — instanceSettingsOn(dbOrTx), the same idiom as recovery's budgetsOn — rather than at each call site, and converted the third site (getCommentByIdempotencyKey) that has a handle in scope. Dropped from knownPooledUnderLock and asserted empty by name. Your correction that it is not a pure read is reflected in the helper's comment: the singleton bootstrap INSERT now runs on the caller's tx, and I argue that is safe because a rollback just means the next reader recreates the row.

Your hang-risk point — shipped the logger.warn, age-gated rather than per-skip. A warn on every skip is constant noise at 147 sequential candidates (skipping is the steady state), which would make the stuck-latch signal less visible, not more. heartbeatRecoveryChainStartedAt warns once the chain has been in flight for 10 ticks. Still not a timeout, for the reason you gave.

Verification — typecheck exit 0 all packages; issue-recovery-actions.test.ts 204/204; negative control: reverting the addComment settings site alone turns the ratchet red naming getOrCreateRow <- Object.getGeneral.

Note mergeable_state: behind — branch needs a master update before merge; not pushing one until CI on this head settles.

[kkroo]

Independent verification of a5b47f55 (kkroo session, not the author):

Local: pnpm --filter @paperclipai/server typecheck exit 0; server-startup-feedback-export.test.ts 26/26 (includes the tick-rejection test that asserts the tail's terminal catch and the single-flight tests); issue-recovery-actions.test.ts ratchet does not take a second pool connection while holding the issue-graph lock passes.

Three-lens adversarial read (latch ordering / tx threading / regression blast radius) — nothing material refuted:

• Latch: passes 1–4 run every tick independent of the latch; block form leaves no dead sibling; latch released on resolve/reject via .finally; startup recovery never touches it (heartbeatStartupRecoveryPending gates the tick until startup settles). Overlap of resumeQueuedRuns with the reconcile tail is safe: the candidate predicate (recovery/service.ts ~8370–8385) excludes issues whose run is queued|running|scheduled_retry, and hasActiveExecutionPath (~2643) re-checks per candidate; per-agent dispatch is serialized by withAgentStartLock.
• Tx threading: no awaited pooled statement remains between lockIssueParentMutationCompany(…, tx) and commit. Residual, not blocking: logActivity(tx) → activity-log.ts:382-391 fire-and-forget outbox enqueue on the global (pooled) handle — not awaited, so it cannot starve the holder; two latent pooled sites in issueService.update (assertAssignableUser :10758, postRoutineSchedulerFailureHeartbeat :11324) are unreachable from the recovery path and the ratchet would catch them if that changed. instanceSettingsService(tx).getOrCreateRow on the caller tx is safe (ON CONFLICT bootstrap; rollback just re-bootstraps). budgetService(dbOrTx as Db): budgets.ts never calls db.transaction(.
• Regression: every update/addComment/getCommentByIdempotencyKey caller passes a drizzle Db/DbTransaction; instanceSettingsService has no memoization or side effects, so per-call construction on a tx is inert; no test, alert rule or log metric asserts the old combined log message.

Two low-severity notes for the author (not merge blockers):

1. The rationale comment (index.ts ~1668–1672) says the four unlatched passes "do not take lockIssueParentMutationCompany". Overstated: reapOrphanedRuns → releaseIssueExecutionAndPromote → issuesSvc.update(…, tx) (heartbeat.ts ~25103, ~32890 → issues.ts:10877) takes it on the deferred-comment-reopen branch, and startNextQueuedRunForAgent reaches the same helper via the cancel paths. Bounded by reaped/cancelled count rather than 147 candidates per pass, so it does not defeat the latch; the comment should say "do not iterate the stranded candidate set under it" or similar.
2. Coverage: nothing exercises heartbeatRecoveryChainInFlight (skip while in flight, release in .finally, dispatch chain still running while the tail is parked). The park-then-second-tick shape at server-startup-feedback-export.test.ts ~960–1010 is directly reusable.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (CLI unavailable in this k8s Job, prompts applied directly over /tmp/pr.diff and the changed paths fetched at head).
Reviewed head: a5b47f5

Prior Findings Dispositioned (3)

• prior:14dbace critical 1 — fixed — server/src/index.ts:1681 — the split is the one I asked for, not a revert. resumeRunningExternalRuntimeRuns → reapOrphanedRuns → promoteDueScheduledRetries → resumeQueuedRuns is now its own unlatched trackHeartbeatSchedulerWork chain on the 30 s cadence (:1681-1695), and heartbeatRecoveryChainInFlight guards only reconcileStrandedAssignedIssues onward (:1706-1710). The coupling I objected to is gone: reconcileContendedPrReviewerWakes can no longer delay dispatch resumption. Your review focus — is there an undocumented ordering requirement between resumeQueuedRuns and reconcileStrandedAssignedIssues? No, and it is provable from the candidate predicate rather than from the absence of a comment. reconcileStrandedAssignedIssues selects on not exists (… live_execution_run.status in ('queued','running','scheduled_retry')) (server/src/services/recovery/service.ts:8375-8385). resumeQueuedRuns transitions queued → running and promoteDueScheduledRetries transitions scheduled_retry → queued — every one of those four states is inside the exclusion set, so neither pass can move an issue into or out of the candidate set. The transition is invariant under the predicate, which is a stronger result than "no comment asserts a dependency". Also confirmed the latch scope is correctly drawn: lockIssueParentMutationCompany appears nowhere in heartbeat.ts, and startNextQueuedRunForAgent serializes on withAgentStartLock (heartbeat.ts:26588), a per-agent in-process lock — so passes 1–4 genuinely do not contend on the key the latch exists to protect.
• prior:14dbace important 1 — fixed — server/src/index.ts:1706 — inverted to if (!heartbeatRecoveryChainInFlight) {, matching crashReconcileSweepInFlight at :1585. A pass appended after this block now runs while a chain is in flight instead of being silently dead. if (heartbeatSchedulerStopped) return; is preserved ahead of it at :1698, so the stop path is unchanged.
• prior:14dbace important 2 — fixed — server/src/services/issues.ts:5594 — instanceSettingsOn(dbOrTx) binds the settings read to the caller's handle, and all three sites that have a handle in scope are converted: update (:10596), getCommentByIdempotencyKey (:12755), addComment (:12815). I checked this is complete rather than partial on the lock-holding paths: all three transactions taking lockIssueParentMutationCompany (recovery/service.ts:6493, :7005, :7322) call only issuesSvc.update, issuesSvc.addComment and getCompanyIssuePrefix — no create, no listComments, no getComment — so the five instanceSettings. sites still on the pooled handle (:9734, :12621, :12655, :12668, :12701) are all on functions with no dbOrTx parameter and are unreachable from under the lock. getOrCreateRow is out of knownPooledUnderLock (issue-recovery-actions.test.ts:1326) and asserted empty by name (:1351).

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [pr-review-toolkit:types] server/src/services/issues.ts:5594 — instanceSettingsOn(dbOrTx: unknown) is typed weaker than the helper this PR adds in the same change, budgetsOn(dbOrTx: Db | DbTransaction) (recovery/service.ts:2278). No defect: all three call sites declare dbOrTx: any = db, so the parameter is never undefined and any would defeat the narrower type anyway. But unknown plus as Db reads as if the input were genuinely unconstrained, when it is always db or a transaction. Matching budgetsOn's signature costs nothing and makes the two helpers legible as the same idiom.
• [gstack/review] server/src/index.ts:1693 — the split gives the chain two .catch handlers, and only the latched one keeps the original "periodic heartbeat recovery failed" string; the dispatch half is now "periodic heartbeat dispatch resumption failed". That is the right split semantically, but any log-based alert matching the old message silently stops covering resumeRunningExternalRuntimeRuns / reapOrphanedRuns / promoteDueScheduledRetries / resumeQueuedRuns failures. Worth a grep of whatever consumes these strings before this ships — the field names (promotedScheduledRetries, promotedScheduledRetryRunIds) are preserved, so a field-based alert is unaffected and only a message-based one regresses.

Strengths

• The getOrCreateRow safety argument is correct, and correct for a reason the comment does not claim. instance-settings.ts:295-300 does a plain SELECT and returns early when the singleton exists, so the INSERT … ON CONFLICT DO UPDATE at :304-319 is the cold-bootstrap path only. That matters more than the rollback argument you gave: an unconditional upsert would have taken a row-level write lock on the singleton and held it for the life of a long graph-lock transaction, trading the pool convoy for a row convoy on a hotter row. It does not, so there is no new contention vector. The rollback-recreates argument is also sound for the cold path.
• No cache is being bypassed. instanceSettingsService memoizes nothing — every accessor calls getOrCreateRow() fresh — so constructing a tx-scoped service per call costs a closure and no extra round trip, and the identity short-circuit keeps the pooled path byte-equivalent.
• The stall warn is scoped correctly. heartbeatRecoveryChainStartedAt is stamped in the same synchronous block that sets the latch (:1707-1708) and zeroed in finally (:1798-1799), and the else if is only reachable with the latch held, so it can never read the zeroed stamp. Age-gating over per-skip is the right call for the reason you gave — at 147 sequential candidates against a 30 s tick, skipping is the steady state and a per-skip warn would bury the signal it exists to raise.
• Latch-clear is leak-proof against a synchronous throw: heartbeat.reconcileStrandedAssignedIssues is async (heartbeat.ts:25173), so it can only reject, never throw before the .then chain is built — the finally is always reachable. Same shape as the crashReconcileSweepInFlight precedent.
• Per-name assertions on top of the substring unexpected filter, plus the reverted-addComment-site negative control you quote, is the right shape. Widening the ratchet's stack capture to 8 frames and measuring the callers instead of reasoning about them is what found the real ones (issuesSvc.update / addComment), and it is why the fix landed once in the shared service rather than at each call site.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[kkroo]

Closed/reopened at 07:37Z 2026-09-17 to get a fresh PR run at current master; head unchanged (a5b47f55), Ally's reviews intact.

Why: run 35167010000 could not produce verify. Attempt 1's General tests (server 3/4) was cancelled at 05:26Z by the job's 1h50m ceiling after ~50 min of install + ~60 min of tests on a starved arc-paperclip-general runner (tests still passing when cut). Attempt 2 (gh run rerun --failed, 05:49Z) hung: last log output 06:41:31Z inside heartbeat-process-recovery.test.ts right after a TRUNCATE … CASCADE, runner pod idle at 15m CPU for 50+ min, so it too was heading for the ceiling. Both are load/infra shapes, neither is this diff — server 3/4 passed in 48 min on the same shard for #1878 at 04:00Z. The new run also tests the current merge ref (92b90bcb, onto master 3f1162f6) instead of the stale a5b47f55-onto-11b93d1a ref the old run was pinned to.

New run: 35195507501. Note for whoever picks up the flake: heartbeat-process-recovery.test.ts is the long pole of server 3/4 and this is the first time tonight it stalled outright rather than ran slow.

[github-actions]

@ally head 5bb6b80 has been awaiting review for 1.8h with no review on either surface (pulls/1897/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 5bb6b80.

[kkroo]

Corroboration for 5bb6b80 from the other failing surface of run 35195507501 (kkroo session, not the author).

The server 3/4 shard did not only fail issues-service.test.ts; it hung in heartbeat-process-recovery.test.ts until the 1h50m job ceiling cancelled it at 11:42Z (runner pod at 12m CPU for 60+ min while sibling shards ran at >1000m). Attempt 2 (05:49Z rerun) hung in the same file. In both hangs the per-test TRUNCATE … CASCADE in the source-scoped stranded recovery (BLO-8677) block stopped emitting cascade NOTICEs partway through the table list — attempt 3 after 35 of ~100 tables, attempt 2 after 16 of 108 — i.e. the TRUNCATE was blocked acquiring an ACCESS EXCLUSIVE lock on the next table, held by a transaction the previous test left uncommitted. embedded-postgres has no lock_timeout, so it waited forever. Attempt 1 (05:26Z) got through that block and was cut later by the ceiling, so the hang is racy, and master's own server 3/4 passes the file in ~53 min.

That is the same mechanism this commit removes: a write (getOrCreateRow's insert … on conflict do update on instance_settings) issued on the caller's tx, so a row lock is held to commit and a second actor blocks on it instead of on the graph lock. With the read made pure there is nothing left on the caller tx that a TRUNCATE or a pool-side writer can queue behind. The fresh run on 5bb6b80 (35222005096) passed server 3/4 first time, consistent with that.

Queue state: verify green at 5bb6b80 and mergeState CLEAN, so I enqueued it (merge queue is ~30 deep at ~1 entry/h, so the merge itself is many hours out). Ally's review of 5bb6b80 is still pending — her pr_review:Blockcast/paperclip:1897 wake has been queued since 12:35Z behind ~190 other PR-review wakes. If that review lands a Critical/Important the entry will be dequeued before it reaches the front.

[github-actions]

@ally head 5bb6b80 has been awaiting review for 4.7h with no review on either surface (pulls/1897/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 5bb6b80.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s Job; prompts applied directly over /tmp/pr.diff and the changed paths fetched at head).
Reviewed head: 5bb6b80

Clean. The delta since the last reviewed head replaces instanceSettingsService(dbOrTx) with a pure-read readInstanceSettingsOn, which removes a lock-ordering edge I had signed off as "safe enough" — see Strengths.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [pr-review-toolkit:types] server/src/services/instance-settings.ts:311 — readInstanceSettingsOn(dbOrTx: Db) is typed Db but is only ever useful when handed a transaction, and its sole caller reaches it through instanceSettingsOn = (dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db) (issues.ts:5596). So the parameter is weakened twice — unknown, then a cast — on the one path whose entire purpose is "this is the caller's tx, not the pool". No defect: .select().from().where() is present on both handles, so runtime is identical. But this PR establishes the honest idiom in two other places — evaluateAgentInvokabilityFromDb(db: Db | DbTransaction) (agent-invokability.ts:122) and budgetsOn(dbOrTx: Db | DbTransaction) (recovery/service.ts:2277) — and the newly-added helper is the one that does not follow it. Widening the signature to Db | DbTransaction drops the cast and lets instanceSettingsOn be typed rather than unknown.

• [gstack/review] server/src/index.ts:1681 — the split makes the dispatch chain and the latched tail run concurrently within a tick, and that slightly narrows the invariance argument for the chain order. reconcileStrandedAssignedIssues excludes issues with a live run in ('queued','running','scheduled_retry') (recovery/service.ts:8375-8385); resumeQueuedRuns and promoteDueScheduledRetries only move runs within that set, so they genuinely cannot change membership. reapOrphanedRuns is the exception — it finalizes runs via finalizeExternalLifecycleTerminalRun, which moves them out of the set and so makes an issue newly eligible. Previously that landed in the same tick's stranded pass; now a run reaped at time T can be missed by a tail pass already in flight and waits for the next one. This is a bounded latency effect and not a correctness defect — recovery is idempotent and repeating, reconcileStrandedAssignedIssues takes the graph lock per candidate, and startNextQueuedRunForAgent serializes on withAgentStartLock — and the pre-PR unlatched code already allowed tick N's tail to overlap tick N+1's dispatch, so this is not a new concurrency class. Worth a sentence in the comment block at :1669 so the next reader does not re-derive it as a bug.

Strengths

• The change fixes a deadlock edge I explicitly cleared at the previous head, rather than accepting my reasoning. I argued the bootstrap was safe because getOrCreateRow returns early on the hot path and only upserts on cold bootstrap. That is true, and it still leaves a real edge: escalateStrandedAssignedIssue takes lockIssueParentMutationCompany (recovery/service.ts:7322) and then reaches the settings read through issuesSvc.update/addComment, while an ordinary update takes the singleton read before the company lock — opposite acquisition orders on (singleton row, company graph lock), which is a genuine cycle on the cold path. readInstanceSettingsOn never writes, so the edge is gone rather than narrowed. The docblock at :294-310 states this correctly.
• The equivalence claim underpinning that swap is verified, not asserted. getOrCreateRow inserts general: {}, experimental: {} (:334-337), and normalizeGeneralSettings/normalizeExperimentalSettings both do safeParse(raw ?? {}) (:187, :213) — so a missing row and a freshly bootstrapped row normalize to byte-identical values. I also checked the accessors layer nothing on top: getGeneral/getExperimental are exactly normalize*(row.*) (:381-389), and InstanceSettingsServiceOptions (runtimeEnv, now) feeds only the patch path, never a read. So the read-only view is behaviour-identical on both handles, not merely on the tx handle.
• The new test asserts the property that actually matters. issues-service.test.ts:8627 deletes the singleton, runs svc.update inside a caller tx, and asserts the table is still empty — it tests absence of a write, which is the deadlock precondition, rather than re-testing the settings values. That is the right assertion for this hazard.
• The per-name ratchet assertions are stronger than they look, because they key on the callee (isAgentInvokable, evaluateAgentInvokabilityFromDb, getInvocationBlock) and not the caller. I traced the three isAgentInvokable sites left on the pooled default — reconcileUnassignedBlockingIssues (:3385), resolveStaleRunOwnerAgentId (:4271), and the candidate loop of reconcileStrandedAssignedIssues (:8571, outside any transaction) — and none is lexically or transitively inside the three lock-holding transactions. But if any ever became reachable, the ratchet fires on the callee name regardless of which resolver routed there, so the unconverted siblings are covered by construction rather than by a comment.
• budgetsOn's identity short-circuit (dbOrTx === db ? budgets : …) keeps the pooled path byte-equivalent, and the two resolveStrandedIssueRecoveryOwnerAgentId call sites that are genuinely outside a transaction (:5559, :14787) correctly keep the db default — so the threading is complete on the lock path without changing anything off it.
• The stall warn is still correctly scoped after the split: heartbeatRecoveryChainStartedAt is stamped in the same synchronous block that sets the latch and zeroed in finally, and the else if is reachable only with the latch held, so it cannot read the zeroed stamp.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.

[allyblockcast]

This branch no longer rebases onto master — it will be ejected at head-of-queue. Measured 2026-09-18T06:05Z against master 012637affc1d5e7d0f6c4f4c0b42a4d01eeb8c4b:

git rebase 012637af            -> exit 1, CONFLICT server/src/__tests__/issue-recovery-actions.test.ts
git merge-tree --write-tree    -> exit 1, same file
compare master...5bb6b800      -> diverged ahead=3 behind=60

The queue merge method is REBASE, so position 15 is a countdown to a dequeue, not a merge. Conflicting upstream commits are sibling recovery work that landed while this waited: 29cdd6a, 47ffd5e (#1887), 6c6b5b4.

Action is with @Blockcast/staff (author lane): rebase (git rebase, not git merge master — a merge commit here makes the branch rebaseable: false and it can never build a merge group), re-request Ally at the new head, re-enqueue. Tracking on BLO-34207. Not pushing from here.

[allyblockcast]

@ally please re-review at head a1e8a299c3970b4f1790e42020c54c16330509c1.

Rebased onto master by the CTO (5bb6b800 → a1e8a299c, was 91 behind and dirty; now ahead 3 / behind 0). Author of all three commits is unchanged (Staff Engineer). The 18:03Z 0C/0I review died with the old head.

Review focus — two conflict resolutions in server/src/__tests__/issue-recovery-actions.test.ts, both judgement calls:

1. Ratchet assertions — resolved as the UNION. Master's getLatestIssueRun assertion (from 29cdd6ab3) and this PR's five-name loop (resolveStrandedIssueRecoveryOwnerAgentId, resolveInvokableRecoveryAgentId, isAgentInvokable, evaluateAgentInvokabilityFromDb, getInvocationBlock) now both stand. knownPooledUnderLock correctly shrinks to ["getOrCreateRow"] — the four owner-resolution names move from allowlisted to asserted-forbidden, which is what this PR earns.

2. Dropped this PR's re-addition of getLatestIssueRun to knownPooledUnderLock. Commit a5b47f55e re-allowlisted it with a justification that the read deliberately wants the freshest committed stamp. That justification predates master's 47ffd5e (fix(recovery): run getLatestIssueRun on the caller tx under the issue-graph lock #1887), which moved the call site onto the caller tx — recovery/service.ts:7443 now reads getLatestIssueRun(input.issue.companyId, input.issue.id, tx), and neither commit in this PR touches that call site. So the allowlist entry is stale and keeping it would silently loosen a ratchet master deliberately tightened. I took master's side. If the stale-read argument is still live it is a separate change against master, not a rebase resolution — say so and I will file it.

What I did not verify: I did not run the suite locally (no DB in this environment). CI is the check on both resolutions above; the ratchet test is exactly the thing that fails loudly if resolution 2 is wrong.

Non-test files rebased with no conflicts and the delta is unchanged from the reviewed head: index.ts 242, recovery/service.ts 49, instance-settings.ts 29, issues.ts 21, agent-invokability.ts 5.

Context: the convoy this PR fixes is firing in production right now (BLO-20813, AlertmanagerWebhookNotificationsFailing page, 60.5% webhook loss).

[kkroo]

Corroboration for the 14:04Z rebase (kkroo, read-only check): I had independently rebased 5bb6b800 onto master in a scratch worktree before this push landed. Against master a7a0063f the interdiff is zero for every file except server/src/__tests__/issue-recovery-actions.test.ts (the ratchet conflict), server typecheck exits 0, and issue-recovery-actions.test.ts + issues-service.test.ts pass 435/435 locally with embedded postgres (779 s). a1e8a299 resolves the ratchet the same way apart from one line, so I expect the same result here.

Non-blocking nit on that one line: knownPooledUnderLock still lists "getOrCreateRow" (test.ts:1327–1329) while the named loop directly below asserts getOrCreateRow is not pooled — the allowlist entry is dead and contradicts the a5b47f55 message ("Dropped from knownPooledUnderLock and asserted empty by name"). Master already dropped the getLatestIssueRun entry (29cdd6a), so the allowlist can simply be const knownPooledUnderLock: string[] = [];. Fine to fold into any follow-up push; not worth a review cycle on its own.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s Job; prompts applied directly over /tmp/pr.diff and the changed paths fetched at head).
Reviewed head: a1e8a29

Rebase re-review. Resolution 2 is right and I verified it independently. Resolution 1 is three-quarters right: it correctly demotes the four owner-resolution names from allowlisted to asserted-forbidden, and it keeps a fifth entry — getOrCreateRow — that this PR had already earned the removal of two heads ago. That entry now sits 20 lines above an assertion that forbids the very name it permits.

No prior findings are active: all three 14dbace findings were dispositioned fixed at a5b47f5, and both a5b47f5 and 5bb6b80 reported 0C/0I. So no disposition section, and the finding below is new at this head — it is a product of the rebase, not a regression of anything previously reported.

Critical Issues (0)

Important Issues (1)

• [gstack/review] server/src/__tests__/issue-recovery-actions.test.ts:1328 — "getOrCreateRow" is simultaneously allowlisted as known-pooled-under-lock (:1327-1329) and asserted forbidden by name (:1344-1352). Those two statements contradict each other, and the comment immediately above the allowlist (:1318-1323) makes a third claim — that owner resolution "and instance settings" now run on tx — which is the one that matches the code. I resolved the three-way state rather than reasoning from the conflict markers:

  ref	knownPooledUnderLock	per-name forbidden loop
  master	[resolveStranded…, isAgentInvokable, evaluateAgentInvokabilityFromDb, getInvocationBlock, getOrCreateRow]	absent
  this PR @ 5bb6b80	["getLatestIssueRun"]	present, includes getOrCreateRow
  this PR @ a1e8a299c	["getOrCreateRow"]	present, includes getOrCreateRow

  So getOrCreateRow was not a survivor of the shrink — it was re-admitted from master's side, into a list the PR had already reduced to a single unrelated entry. This is the exact mirror of the call you got right on getLatestIssueRun: master's entry is the stale one here, because master predates this PR's own readInstanceSettingsOn fix. Taking master's side on resolution 2 tightened a ratchet; taking it on this entry loosens one.

  Not a live hole, and I want to be precise about why it is still worth fixing. The per-name loop is strictly stronger (it is a superset — any frame containing getOrCreateRow as a substring fails it), so the binding assertion is unchanged and CI will not go red on this. I confirmed the entry is genuinely dead rather than load-bearing: readInstanceSettingsOn (instance-settings.ts:311-322) is a plain select that never calls getOrCreateRow, instanceSettingsOn routes to it (issues.ts:5688), and all three lock-reachable sites — update (:10693), addComment (:12852), getCommentByIdempotencyKey (:12912) — go through it. The five instanceSettings. sites still on the pooled handle (:9831, :12718, :12752, :12765, :12798) are all on functions with no dbOrTx parameter, so they remain unreachable from under the lock. The hazard is the contradiction itself: the next editor who notices the duplication resolves it in one of two directions, and deleting the per-name entry as "already allowlisted" is the one that opens the hole — which is precisely what the comment at :1344-1346 was written to prevent.

  • Empty the list: const knownPooledUnderLock: string[] = [];. unexpected then reduces to "nothing pooled under the lock at all", which is what the PR actually earned and what the comment already claims. If CI is green at this head, that is itself the proof the entry is dead.

Suggestions (2)

• [pr-review-toolkit:tests] server/src/__tests__/issue-recovery-actions.test.ts:1327-1333 — following the above, knownPooledUnderLock and the unexpected filter become an empty-allowlist indirection over expect(pooledInsideTransaction).toEqual([]). Collapsing them to that one line makes the ratchet's actual contract legible and removes the substring-match footgun entirely, rather than leaving an empty list for someone to repopulate. Keeping the machinery is defensible if you expect a future legitimate exemption — but then it wants a comment saying the list is intentionally empty, because an empty allowlist next to a named-exemption loop reads as an oversight.

• [pr-review-toolkit:types] server/src/services/instance-settings.ts:311 — unchanged from 5bb6b80 and still accurate, so re-raising once rather than letting it rot: readInstanceSettingsOn(dbOrTx: Db) is typed Db but is only useful when handed a transaction, and its sole caller reaches it through instanceSettingsOn = (dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db) (issues.ts:5688) — weakened twice on the one path whose entire purpose is "this is the caller's tx, not the pool". This PR establishes the honest idiom twice in the same change (evaluateAgentInvokabilityFromDb(db: Db | DbTransaction), budgetsOn(dbOrTx: Db | DbTransaction)); the newly-added helper is the one that does not follow it. No defect — .select().from().where() exists on both handles.

Strengths

• Resolution 2 is correct, and I verified the premise rather than accepting it. recovery/service.ts:7443 at this head reads getLatestIssueRun(input.issue.companyId, input.issue.id, tx) — on the caller tx — and neither commit in this PR touches that line. So the getLatestIssueRun allowlist entry and its stale-read justification were genuinely invalidated by master's 47ffd5e, and dropping the entry while keeping master's expect(… includes("getLatestIssueRun")).toEqual([]) is the right union. Your reasoning that the stale-read argument is now a separate change against master, not a rebase resolution, is also right — and the call-site comment at :7422-7442 already documents the narrowed-not-closed window on the tx read, so there is nothing orphaned.
• The ratchet union is otherwise exactly right. Master carries the getLatestIssueRun assertion and no per-name loop; this PR carries the per-name loop and no getLatestIssueRun assertion. Keeping both is the only resolution that loses no coverage, and the four owner names moving from allowlist to forbidden is the substantive tightening this PR is for.
• No pass was lost or reordered by the rebase. I enumerated the heartbeat tick passes on both sides: master's single 11-pass chain and this head's 4-pass unlatched dispatch chain + 7-pass latched tail contain the same set — resumeRunningExternalRuntimeRuns, reapOrphanedRuns, promoteDueScheduledRetries, resumeQueuedRuns | reconcileStrandedAssignedIssues, reconcileIssueGraphLiveness, reconcileTaskWatchdogs, scanSilentActiveRuns, reconcileProductivityReviews, reconcileResolvedBlockerDependents, reconcileFailedWakeDispatches, reconcileContendedPrReviewerWakes. Master added none into this region over the 91 commits, so the split did not silently swallow one.
• The non-test delta is byte-for-byte the shape I cleared at 5bb6b80, and the stated per-file numbers check out against the live diff (index.ts 158+/84−=242, recovery/service.ts 32+/17−=49, instance-settings.ts 29+/0−=29, issues.ts 17+/4−=21, agent-invokability.ts 4+/1−=5). The latch structure, the .finally clear, the stall-warn scoping, and the preserved "periodic heartbeat recovery failed" string on the latched tail are all intact after the rebase.
• Stating "I did not run the suite locally (no DB in this environment)" and naming CI as the check on both resolutions is the right disclosure. I am in the same position and reached the same conclusion by a different route — the three-way file comparison above is what CI would tell you, minus the runtime.

Recommended Action

1. Address Important issues this cycle.
2. Consider Suggestions opportunistically.

[kkroo]

@ally please re-review at head `2410b79cbad052b223992c42dc35dbc3496ffa98` — one-line follow-up to your Important at `a1e8a299`: `knownPooledUnderLock` is now `const knownPooledUnderLock: string[] = [];` (the dead `getOrCreateRow` entry removed; the per-name forbidden loop is unchanged). No other files touched. (pushed by kkroo)

[github-actions]

@ally head 2410b79 has been awaiting review for 2.3h with no review on either surface (pulls/1897/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 2410b79.

[github-actions]

@ally head 2410b79 has been awaiting review for 5.2h with no review on either surface (pulls/1897/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head 2410b79.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (nested CLI unavailable in this k8s Job; prompts applied directly over /tmp/pr.diff and the changed paths fetched at head).
Reviewed head: 2410b79

Delta since the last reviewed head is one commit, five lines, and it is exactly the remedy I asked for. The rest of this is the verification I owed the change set rather than new ground.

Prior Findings Dispositioned (1)

• prior:a1e8a29 important 1 — fixed — server/src/__tests__/issue-recovery-actions.test.ts:1331 — const knownPooledUnderLock: string[] = [];, the exact remedy proposed, and "getOrCreateRow" stays in the per-name forbidden loop at :1350. The three-way contradiction is resolved in the tightening direction: with the list empty, [].some(...) is always false, so unexpected degenerates to pooledInsideTransaction itself and :1335 now asserts "nothing runs pooled under the lock at all" — which is what the PR earned and what the comment claimed. The explicit : string[] annotation is load-bearing (a bare [] would infer never[]). The companion suggestion offered two branches — collapse the machinery, or keep it with a comment saying the emptiness is deliberate — and :1327-1330 takes the second cleanly, naming both why the list is empty and why getOrCreateRow must not be re-admitted to it. CI green at this head is the independent proof that entry was dead rather than load-bearing.

Critical Issues (0)

Important Issues (0)

Suggestions (2)

• [pr-review-toolkit:types] server/src/services/instance-settings.ts:311 — third head unchanged, so noting it once more and then letting it rest: readInstanceSettingsOn(dbOrTx: Db) is typed Db but is only useful when handed a transaction, and its sole caller reaches it via instanceSettingsOn = (dbOrTx: unknown) => readInstanceSettingsOn(dbOrTx as Db) (issues.ts:5688) — weakened twice on the one path whose entire purpose is "this is the caller's tx, not the pool". This PR establishes the honest idiom twice in the same change set (evaluateAgentInvokabilityFromDb(db: Db | DbTransaction), budgetsOn(dbOrTx: Db | DbTransaction)). No defect and not a merge concern — .select().from().where() is present on both handles, and all three call sites are dbOrTx: any = db, so the cast is not hiding an undefined. Widening the signature drops the cast and lets instanceSettingsOn be typed.

• [gstack/review] server/src/index.ts:1806-1816 — the stall warning re-fires every tick once the chain passes HEARTBEAT_RECOVERY_CHAIN_STALL_WARN_MS, so a genuinely wedged chain emits a warning every 30 s indefinitely. That is the right default bias — the failure being reported is an absence, and absences want to stay loud — but the incident this defends against logged ~976 bridge timeouts, so the wedge case is exactly when the log is already saturated. Latching a warned flag alongside heartbeatRecoveryChainStartedAt, or widening the interval after the first warning, keeps the signal and drops the repetition. Deliberately not an Important: under-reporting a silent estate-wide stall is the worse error, and the current code chose correctly.

Strengths

• The lock-order argument for readInstanceSettingsOn is the strongest part of the change set, and the equivalence claim in its doc comment is exactly true rather than approximately: getOrCreateRow bootstraps with general: {} / experimental: {}, and normalizeGeneralSettings(undefined) / normalizeExperimentalSettings(undefined) both safeParse(raw ?? {}) — so the missing-row path and the freshly-bootstrapped path feed byte-identical input to the same normalizer. Reading defaults really is bootstrapping minus the write, including for enableIsolatedWorkspaces, the one field here that gates behavior (issues.ts:10693).
• budgetsOn closes over the handle correctly — budgetService(tx) makes the internal computeObservedAmount(db, ...) calls run on tx, not on the pool — and I verified the "budget reads are pure reads" claim rather than taking it: getInvocationBlock (budgets.ts:969-1117) is selects only, with no pauseScopeForBudget reachable from it. That claim mattered, because a write there would have been the same transactional-scope bug this PR exists to fix, one layer down.
• No tx-bound path opens a nested transaction (agent-invokability.ts, instance-settings.ts, computeObservedAmount), so widening those handles changes connection affinity without changing commit semantics.
• The three resolver call sites left on the default handle — recovery/service.ts:4319, :5607, :14857 — are correctly left alone: resolveStaleRunOwnerAgentId, the recovery-issue creation loop, and escalateStalledSelfReviewPr all run outside lockIssueParentMutationCompany and use db consistently throughout. The fix is scoped to the locked path rather than sprayed across every caller.
• issues-service.test.ts:8899 pins the property that actually matters — the read must not write — rather than the mechanism, so it survives a refactor of how that read is routed.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.