fix: yield slots during external waits - #1195
Conversation
Signed-off-by: Search <search@example.com>
|
🔗 Paperclip issue: PEN-2074 |
1 similar comment
|
🔗 Paperclip issue: PEN-2074 |
![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: 31bb91c
Looks good. I found no high-confidence correctness, error-handling, or lifecycle regressions in the external-wait yield path.
Critical Issues (0)
Important Issues (0)
Strengths
- The cancellation path persists terminal state before external teardown and retains the issue lock until the exact runtime reservation is released.
- The stale-lock and deferred-wake paths distinguish external-service resumption signals from ordinary activity, with focused regression coverage.
Recommended Action
- Safe to merge after the repository's required checks pass.
|
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 |
Signed-off-by: Search <search@example.com>
|
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 |
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: 49a636e
Looks good. I found no high-confidence correctness, error-handling, lifecycle, or type-design regressions in the external-wait yield and recovery paths.
Critical Issues (0)
Important Issues (0)
Strengths
- The route persists the monitor before yielding, while the cancellation path keeps the execution lock until the external reservation is safely released.
- The deferred-wake and stale-lock paths distinguish external-service resume signals from ordinary activity, with targeted regression coverage.
Recommended Action
- Safe to merge after the repository required checks pass.
Signed-off-by: Search <search@example.com>
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: 826ffdb
Critical Issues (0)
Important Issues (1)
- [native-codex]
server/src/services/heartbeat.ts:510— Agithub_pr_synchronizedwake is not considered an external-wait resume signal, so a push to the monitored PR is written asissue_external_wait_wake_suppressedatserver/src/services/heartbeat.ts:25373instead of waking the waiting issue. This leaves the issue idle until its timer fires even though its GitHub state changed and the existing review scheduling code explicitly treats synchronize as a fresh-head event (server/src/services/heartbeat.ts:7984).- Include
github_pr_synchronizedin the external-wait resume reasons and cover the resulting wake path with a focused test.
- Include
Suggestions (0)
Strengths
- The cancellation path persists the yield before terminating an external runtime and holds the issue lock until the exact runtime reservation is safely released.
- The added tests cover delayed Job deletion, deferred-wake retirement, and terminal-lock cleanup races.
Recommended Action
- Address the Important issue this cycle.
Signed-off-by: Search <search@example.com>
|
Addressed the |
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: fd8eeb5
Looks good. I found no high-confidence correctness, error-handling, lifecycle, or type-design regressions in the external-wait yield and recovery paths.
Prior Findings Dispositioned (1)
- prior:826ffdb important 1 — fixed —
server/src/services/heartbeat.ts:780—github_pr_synchronizedis now an external-wait resume signal; the current-head lifecycle regression test exercises its resulting wake atserver/src/__tests__/heartbeat-process-recovery.test.ts:5626.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- Yielding persists the external monitor before cancelling and keeps the issue lock until the matching external reservation is released.
- Recovery prevents ordinary deferred wakes from bypassing the persisted external monitor, while allowing the relevant GitHub and timer resumption signals.
Recommended Action
- Safe to merge after the repository required checks pass.
…ternal-wait-slots Signed-off-by: Search <search@example.com> # Conflicts: # server/src/services/heartbeat.ts
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: 5797d3c
Looks good. The external-wait yield path, the terminal-cleanup-pending gating in the wake path, and the reservation-aware lock-release predicates all hold up. I found no high-confidence correctness, lifecycle, or type-design regressions.
Critical Issues (0)
Important Issues (0)
Suggestions (4)
- [native-codex]
server/src/services/heartbeat.ts:18602— In the reservation reconciler's new terminal-run cleanup block,startNextQueuedRunForAgent(run.agentId)is the only unguardedawait, while the two sibling calls immediately above it (releaseIssueExecutionAndPromote,finalizeAgentStatus) are explicitly.catch()-wrapped. Because this sits insidefor (const { reservation, run } of pending), a throw here aborts the sweep and leaves the remaining pending reservations unreconciled. This matches the unguarded house style used elsewhere, so it may be deliberate — but the local asymmetry inside a batch loop suggests wrapping it in the same defensive.catch()as its neighbours. - [gstack/review]
server/src/services/recovery/service.ts:241—EXTERNAL_WAIT_RESUME_WAKE_REASONSis defined twice: here as a hardcoded 8-entry literal, and atserver/src/services/heartbeat.ts:561asGITHUB_STATE_CHANGE_WAKE_REASONSspread plus 5 extras. The two sets are identical today, but they must stay in lockstep or a resume signal will wake one path and be suppressed by the other. Sinceheartbeat.tsalready re-exports from./recovery/service.js,recovery/service.tscan own the constant andheartbeat.tscan import it. - [pr-review-toolkit/code]
server/src/services/issues.ts:6524—if (run && !TERMINAL_HEARTBEAT_RUN_STATUSES.has(run.status)) return false;is immediately followed byif (run && !await isTerminalOrMissingHeartbeatRun(...)) return false;, and the callee re-reads the run row and re-applies the same terminal check atserver/src/services/issues.ts:6024. The first line is fully subsumed by the second and costs an extra round trip. Same pattern atserver/src/services/issues.ts:6720andserver/src/services/issues.ts:6732. - [pr-review-toolkit/types]
server/src/services/issues.ts:6017—isTerminalOrMissingHeartbeatRunnow also requires that no unreleasedexternalRuntimeReservationsrow exists, so the name no longer describes what it returns; a caller reading only the signature would expect a pure status predicate. Something likeisReleasedTerminalOrMissingHeartbeatRunwould keep the four call sites honest.
Strengths
- The cancellation path is now durable-first: status is written via
setRunStatusIfCurrentStatusCAS before process termination, with a bounded re-entry when the CAS loses to a concurrent transition, so a lost race no longer silently drops the cancellation. repairTerminalReleasemakes the yield idempotent — re-invokingcancelRunon an already-cancelledrun with the sameerrorCodere-runs cleanup, andpersistCancellationArtifactsdedupes the lifecycle event rather than appending a duplicate.- The reservation-aware predicates are applied consistently across all three lock-release surfaces (issue service, recovery sweep, wake path), so a terminal run holding an unreleased runtime slot can no longer have its issue lock stolen mid-teardown.
terminalRunCleanupPendingcorrectly keepsactiveExecutionRunnon-null and forces deferral instead of admitting a new run while teardown is in flight, with the reconciler as the backstop if cleanup stalls.
Recommended Action
- No Critical or Important issues — safe to merge once the repository required checks pass.
- Consider the Suggestions opportunistically; the duplicated wake-reason set is the one most likely to bite later.
…ble (PEN-2383) cancelRunInternal's cleanupExternalRuntime pre-checks the active reservation and returns early, so it never reached deleteExactExternalRuntimeJob, which owned the debug log for the benign no-reservation path. The code was intact but unreachable, silencing the signal heartbeat-external-runtime-retry asserts on. Extract the log into logNoActiveReservationSkip and emit it from both the early-return and the delete-refusal site, so skipping the pointless delete call does not also skip the signal. Signed-off-by: Search <search@example.com>
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current 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: 4338f17
Looks good. The external-wait yield, durable cancellation, reservation-aware lock release, deferred-wake suppression, and stale-lock recovery changes are covered by focused tests. I found no high-confidence correctness, error-handling, lifecycle, or type-design regressions in the current head.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
server/src/services/heartbeat.ts:18593— The reservation reconciler catches issue-lock cleanup and agent finalization failures but leavesstartNextQueuedRunForAgentas an unguarded await inside its batch loop. Consider isolating that dispatch failure so one agent's queue-start error cannot abort reconciliation of later pending reservations.
Strengths
- The cancellation path persists terminal state before external teardown and retains the issue lock until the matching runtime reservation is released.
- External-wait resume reasons are carried through coalescing, deferred-wake recovery, and normal wake dispatch, including synchronized PR pushes.
- Focused lifecycle and recovery tests cover delayed Job deletion, deferred-wake retirement, stale-lock protection, and queued-run promotion.
Recommended Action
- Safe to merge after the repository's required checks pass.
Thinking Path
Linked Issues or Issue Description
What Changed
master, including detached queued-run recovery and the explicitissue_assignedauto-checkout contract.Verification
pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts server/src/__tests__/issue-execution-policy-routes.test.ts server/src/__tests__/recovery-stale-issue-lock-sweep.test.ts server/src/__tests__/heartbeat-context-summary.test.ts(335/335passed after merging currentmaster).pnpm --filter @paperclipai/server typecheckpassed.pnpm --filter @paperclipai/server buildpassed.git diff --checkpassed.Risks
31bb91c21and merge-resolution commit49a636e07.Model Used
openai/gpt-5.6-sol, reasoning-capable coding model with tool use and local code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template