feat(turns): server cancel seam for durable runs (G22 #816) - #927
Conversation
Stop/Esc on an attached durable run now issues a server cancel of the Workflow run via POST /api/turns/:runId/cancel -> getRun(runId).cancel(), mirroring the stream route's auth/ownership/terminal gates. An accepted cancel folds turnStatus: 'cancelling' KEEPING turnRunId; the run's own terminal persist owns the terminal status. The old turnRunId: undefined + completed stop-fold was a lie over a live run and is removed on the durable path (survives only for legacy /api/agent and the producer- cancelled SSE terminal, discriminated by the caller's abort signal). Unmount / switch / New / Clear / logout still detach only (D18 lock unchanged); persistOverlayStatus and TURN_STATUS_VALUES unchanged. - app/api/turns/[runId]/cancel/route.ts: 401/400/404/409/429/503 gates + overlayWorkerMeta 'cancelling' PATCH (LWW strictly-newer clock) - lib/turnApi.cancelTurn: typed never-throw client (accepted/terminal/ gone/failed) - lib/detachTurn: decideStopFoldPre/Post + shouldSkipCancelPost planners - HarnessHost poll: fire cancel POST once per run id, then abort reader - lib/harnessChat: G22 stop fold keeps turnRunId + folds 'cancelling' - lib/sessionCloudCaps: TURN_CANCEL_MIN_INTERVAL_MS (NEW cap, 1000ms) - docs/feature-divide.md: Stop/cancel row timeless update
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ 100bf43 · 11 files · durable-turn Stop server-cancel seam (plan #816 / G22)
Lenses run: L1, L2, L5, L6, L8 (skip: L3 no dual-chat / Wasm protocol; L4 no GHA/runner/artifact; L7 no host bind; L9 no palette)
AGENTS.md read: yes (plus docs/feature-divide.md, SECURITY.md workflows/API surface, plan #816)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | Failed cancel POST never keeps running — optimistic 'cancelling' plus shouldSkipCancelPost traps Stop. Plan #816 race table (and decideStopFoldPost) lock: network/5xx/429 → keep turnRunId + running, soft note, never a fake cancel. HarnessHost poll fires cancelTurn then immediately abort(). lib/harnessChat.ts keepCancelling folds 'cancelling' on fail.kind === 'stop' + signal.aborted before ack. The keep-running branch only setHostNote — it does not persist(running). Repeat Stop is then skipped (shouldSkipCancelPost is true for 'cancelling'). coldAttachFromSnapshot requires 'running', so F5 will not attach. C15 409 still treats 'cancelling' + live getRun as in-flight. |
Operator Stop on a live durable run. Cancel POST returns 503 (store/Workflows) or throws (Failed to fetch). Local fold is 'cancelling' + turnRunId kept. Busy clears. Repeat Stop is a no-op. Next prompt 409s. F5 does not cold-attach. Workflow spend continues until the 1h wall / natural terminal — the exact ghost-spend lie this row claimed to kill, on the failure path the plan explicitly locked. Source-lock in lib/detachTurn.test.ts only asserts the poll contains fold.kind === 'keep-running' and setHostNote( — it never asserts a persist of 'running'. |
Defender: optimistic 'cancelling' is "by design" so repeat Stop does not re-POST; C15 requires a live wf status so a stale marker cannot block a terminal run; the run still ends on its own. Fails: the locked table says failed ack keeps running so the operator can retry Stop; skip-re-POST is specified as "once per run id" after an accepted cancel (turnStatus === 'cancelling' as the ack marker), not after a failed POST. A terminal run is not the bug — a still-live run with a fake 'cancelling' is. Goal of G22 is honest Stop; this path restores the lie. |
high |
| Minor | L6 | TURN_CANCEL_MIN_INTERVAL_MS is unlocked. lib/sessionCloudCaps.ts adds the NEW 1000 ms cap; lib/sessionCloudCaps.test.ts was not updated. Wall-clock caps on the same file have explicit value locks (TURN_WALL_CLOCK_*, TURN_START_MIN_INTERVAL_MS). |
A later edit silently changes the 1s cancel window (or diverges it from the start guard) and no test fails. Not a live hole. | Defender: the route test hard-codes TURN_CANCEL_MIN_INTERVAL_MS: 1000 in the mock, so the route matrix still passes. Fails: that mock is not the exported product lock; the caps table on this file is the lock that #923 already established as the pattern. |
high |
Residual risk
Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → cancel() + overlay PATCH) match the stream route and the plan matrix; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel. In-route requireSessionUser holds even though middleware matcher is exact /api/turns (child /api/turns/:runId/cancel is not on the matcher — same gap as /stream; not introduced as a new dual-gate hole). Live getRun().cancel() behavior on Vercel Workflows, and whether a cancelled run's SSE error lands after this tab already aborted the reader, were not executed.
Merge guidance
CONCERNS — do not merge until the Major is fixed. Suggested fix is local to the host Stop fold:
- Treat
'cancelling'as the accepted-ack marker, not the pre-ack abort fold. - On
fold.kind === 'keep-running',persist({ ...liveNow, turnRunId, turnStatus: 'running' })(and drop the run id from any "already POSTed" set) so a later Stop re-POSTs. - Apply that fold at
persistTurntime as well sorunHarnessTurn's optimistic'cancelling'cannot win the race against a failed ack. - Lock
TURN_CANCEL_MIN_INTERVAL_MS === 1000 === TURN_START_MIN_INTERVAL_MSinsessionCloudCaps.test.ts.
What was not attacked
Live Vercel Workflows Run.cancel(); DO runner; production Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9).
Adversarial-review #927 CONCERNS: a failed cancel POST left the optimistic 'cancelling' fold in place and shouldSkipCancelPost then blocked a retry — ghost spend until the 1h wall. Persist the keep-running fold (and drop the posted-id) so Stop re-POSTs; apply the same fold at persistTurn so runHarnessTurn cannot beat a failed ack. Lock TURN_CANCEL_MIN_INTERVAL_MS. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ 96770a7 · 12 files · durable-turn Stop server-cancel seam (plan #816 / G22), including the first-pass follow-up
Lenses run: L1, L2, L5, L6, L8 (skip: L3 no dual-chat / Wasm protocol bump; L4 no GHA/runner/artifact; L7 no host bind; L9 no palette)
AGENTS.md read: yes (plus docs/feature-divide.md, SECURITY.md workflows/API surface, plan #816)
Note: requested URL was /pull/927. Re-reviewed HEAD 96770a7 (claimed to close the previous Major: failed cancel POST left a fake 'cancelling' and blocked Stop retry; plus the unlocked TURN_CANCEL_MIN_INTERVAL_MS). Those two are closed. This pass is leftover coupling on the cancel-ack then() vs the next runPrompt, not a restored ghost-spend lie.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L1 | Cancel-ack then() is not generation-guarded; pendingStopFoldRef is never cleared. HarnessHost poll fires cancelTurn then releaseBusyViewport (inflight=false) immediately. The ack then() persist()s applyStopFoldToSession(liveNow, …) if liveNow.id matches and turnRunId is still the stopped id or undefined. A new runPrompt does not bump epoch and does not null pendingStopFoldRef. |
Operator Stop. Workflow terminals fast (cancel landed or natural). C15 live-only 409 allows the next prompt (cancelling + terminal getRun). User Sends. New runPrompt is inflight pre-headers; sessionRef still carries the old id. Slow 409/failed/accepted ack then() persist()s clear-terminal / keep-running / cancelling onto that snapshot. Local sessionRef is stomped until onSessionPatch persistTurn of the new id. Cloud LWW usually loses (applyStopFold does not stamp updatedAt) so this is a local-window race, not the 4h class. |
Defender: liveNow.turnRunId !== stopRunId && !== undefined skips once the new id is planted; persistTurn's newer-id guard is the same; envelope LWW rejects a stale-clock PUT. Fails: the pre-header window is real (start() in flight, no new id yet), writeLocalSession does not LWW, and leftover pendingStopFoldRef is then applied by the new turn's persistTurn while the snapshot still has the old id. Plan #816 two-tab/next-prompt row assumed the ack could not land on a later generation. |
medium |
| Minor | L6 | applyStopFoldToSession plants turnRunId when the snapshot's id is undefined. Guard only skips a different defined id. { turnRunId: undefined, turnStatus: 'completed' } + leftover { kind: 'keep-running' } becomes { turnRunId: old, turnStatus: 'running' }. Unit rows cover same-id / newer-id / legacy-clear, not the cleared-id case. |
Same window as the row above: ack then() or a later persistTurn with leftover pendingFold + a snapshot that already cleared the id (409 orphan-unstick, or a pre-headers fail fold). Plants the stopped run back as running. C15 then 409s the next prompt against a terminal/absent getRun until the worker persist / a later clear. |
Defender: this-turn Stop always has an id (keepCancelling keeps it; decideStopFoldPre requires it). Fails: the plant is exactly the resurrection seam for a leftover fold after the id was cleared; the test matrix never locks the fail-safe. |
high |
Residual risk
Prior Major on this HEAD stays closed: failed cancel POST persists running and drops the posted-id so Stop re-POSTs; persistTurn applies pendingStopFoldRef so runHarnessTurn's optimistic 'cancelling' cannot beat a failed ack; TURN_CANCEL_MIN_INTERVAL_MS === 1000 === TURN_START_MIN_INTERVAL_MS is locked. Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → cancel() + overlay PATCH) match the stream route and the plan matrix; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. In-route requireSessionUser holds even though middleware matcher is exact /api/turns (child /cancel is not on the matcher — same gap as /stream). 503 on cancel() interpolates err.message — same convention as start/stream, not a new leak. Two-tab Stop before local 'cancelling' is a second POST (409/idempotent cancel) per the plan.
Merge guidance
PASS WITH NOTES — safe to merge from this attack; the two Minors should be fixed in this PR (local to HarnessHost ack then() / runPrompt start + applyStopFoldToSession) but they do not restore the ghost-spend lie.
Suggested fixes:
- Cancel-ack
then(): no-op wheninflightRef.current(a newrunPrompthas started). NullpendingStopFoldRefat the start of eachrunPrompt. applyStopFoldToSession: ifsession.turnRunId === undefined, return the snapshot unchanged (never plant a cleared id). Lock it inlib/detachTurn.test.ts.
What was not attacked
Live Vercel Workflows Run.cancel(); DO runner; production Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9).
Adversarial-review #927 PASS WITH NOTES: - Cancel-ack then() no-ops while a new runPrompt is inflight so a slow 409/failed/accepted persist cannot stomp the next prompt's pre-headers snapshot. runPrompt nulls pendingStopFoldRef at start. - applyStopFoldToSession never plants a cleared turnRunId. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ 5a3716a · 12 files · durable-turn Stop server-cancel seam (plan #816 / G22), including two claimed follow-ups
Lenses run: L1, L2, L5, L6, L8 (skip: L3 no dual-chat / Wasm protocol bump; L4 no GHA/runner/artifact; L7 no host bind; L9 no palette)
AGENTS.md read: yes (plus docs/feature-divide.md, SECURITY.md workflows/API surface, plan #816)
Note: re-reviewed HEAD 5a3716a (claimed to close the pass-2 Minors: cancel-ack then() generation-guard + applyStopFoldToSession never planting a cleared id). Those two local stomps stay closed. This pass is the pass-2 guard reopening the pass-1 Major.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L6 | Cancel-ack then() returns on inflightRef before dropping the posted-id or applying keep-running. Pass 1 locked: failed POST must persist running and drop cancelPostedRunIdsRef so Stop retries. Pass 2 added if (inflightRef.current) return at the top of the ack callback (and runPrompt nulls pendingStopFoldRef) so a slow ack cannot stomp the next prompt. That return also skips decideStopFoldPost, the posted-id delete, and the keep-running persist. releaseBusyViewport clears Busy / inflight this tick, so a new runPrompt can start while the cancel POST is still in flight. persistTurn already wrote optimistic 'cancelling'. Repeat Stop is then skipped (cancelPostedRunIdsRef.has and shouldSkipCancelPost and decideStopFoldPre requires 'running'). The source-lock in lib/detachTurn.test.ts only asserts the early return exists — it never asserts posted-id delete / keep-running still run. |
Operator Stop on a live durable run. Cancel POST will 503 / network-fail. Busy clears immediately. User Sends (composer is live) or send-while-running Stop auto-reposts: persistTurn folded 'cancelling', so shouldRepostAttachFollowUp({ turnStatus: 'cancelling' }) is now true (!== 'running'). runPrompt sets inflight=true and nulls pendingStopFoldRef. Failed ack then() no-ops entirely. Local status stuck 'cancelling', id stuck in the posted set. Repeat Stop is a no-op. C15 409s the next prompt (cancelling + live getRun). F5 will not cold-attach (coldAttachFromSnapshot requires 'running'). Workflow spend continues until the 1h wall — the exact ghost-spend lie G22 claimed to kill, restored by the pass-2 generation guard on the failure path pass 1 locked. |
Defender: pass 2 had to no-op persist while inflight or a slow 409/failed/accepted fold stomps the next prompt's pre-headers snapshot; C15 409 is the send so the user "sees" the live run; the run still ends on its own. Fails: skipping persist is right; skipping posted-id delete + pending keep-running fold is not. shouldRepostAttachFollowUp does not take operatorStop — attach Stop on main kept 'running' via attachOperatorStop and did not repost; this PR's persistTurn overlay of optimistic 'cancelling' newly enables the auto-repost, which is the most reliable way to set inflight before the ack. A terminal run is not the bug — a still-live run with a fake 'cancelling' plus a posted-id that blocks retry is. |
high |
| Minor | L1 | Ack then() calls raw persist(), bypassing discardedSessionIdsRef / decideDetachPersist. Pass 1/2 persist of the fold is persist(applyStopFoldToSession(liveNow, …)) — writeLocalSession + repo.put with no discarded/drop gate. persistTurn is the path #844 locked so Clear cannot LWW-upsert a deleted row. |
Operator Stop (cancel POST in flight). Operator Clear: discarded is marked sync, repo.remove is async, sessionRef.id is still the cleared id until resetBridge after the DELETE. Ack then(): cancelled is false (poll effect still mounted), liveNow.id === cancelSessionId, persist() PUTs the stopped snapshot onto the deleted id. Window is the cancel RTT vs DELETE RTT — cancel usually wins. |
Defender: liveNow.id !== cancelSessionId skips once the mint lands; unmount sets cancelled. Fails: Clear does not unmount the poll effect, and resetBridge is after await repo.remove. Same resurrection class as #844, on the new persist path this PR added. Bounded race, not the ghost-spend lie. |
high |
Residual risk
Prior pass-1 Major stays closed only when the cancel ack lands before the next runPrompt. Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → cancel() + overlay PATCH) still match the stream route and the plan matrix; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. In-route requireSessionUser holds even though middleware matcher is exact /api/turns (child /cancel is not on the matcher — same gap as /stream, not a new dual-gate hole). 503 on cancel() interpolates err.message — same convention as start/stream. Pre-headers Stop (no local turnRunId yet while the start route has already PATCHed 'running') still cannot POST cancel — residual ghost-spend window, not introduced here. Two-tab Stop before local 'cancelling' is a second POST (409/idempotent cancel) per the plan.
Merge guidance
CONCERNS — do not merge until the Major is fixed. Suggested fix is local to the host Stop-ack callback + shouldRepostAttachFollowUp:
- Compute
decideStopFoldPostand drop the posted-id onkeep-running/clear-terminalunconditionally (before any inflight/unmount/switch return). - Set
pendingStopFoldRefto that fold even when inflight, sopersistTurnof a 409/fail on the next prompt can restore'running'. - Skip only the snapshot
persist()when inflight, switched, a newer id is planted, or the session is discarded (discardedSessionIdsRef— also closes the Minor). shouldRepostAttachFollowUpmust takeoperatorStopand return false on Stop ('cancelling'is!== 'running'; attach Stop must not auto-POST the remapped follow-up).
What was not attacked
Live Vercel Workflows Run.cancel(); DO runner; production Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9).
Adversarial-review #927 CONCERNS: the pass-2 inflight early-return skipped posted-id delete and the keep-running fold, so a Send (or send-while-running auto-repost) before a failed cancel ack trapped Stop — optimistic cancelling + posted-id, ghost spend until the 1h wall. Split the ack: always drop the posted-id on keep-running / clear-terminal; set pendingFold even when inflight; persist the snapshot only when idle on the same session. Discarded sessions never raw-persist (Clear resurrection). shouldRepostAttachFollowUp takes operatorStop so G22 cancelling does not auto-POST the remapped follow-up. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ ed22f12 · 14 files · durable-turn Stop server-cancel seam (plan #816 / G22), including three claimed follow-ups
Lenses run: L1, L2, L5, L6, L8 (skip: L3 no dual-chat / Wasm protocol bump; L4 no GHA/runner/artifact; L7 no host bind; L9 no palette)
AGENTS.md read: yes (plus docs/feature-divide.md, SECURITY.md workflows/API surface, plan #816)
Note: re-reviewed HEAD ed22f12 (claimed to close the pass-3 Major: failed cancel + inflight runPrompt trapped Stop; plus the Clear-resurrection Minor). Same-session inflight pending-only + shouldRepostAttachFollowUp(operatorStop) stay closed. This pass is commit: 'drop' on switch/unmount reopening the pass-1 ghost-spend lie on a session the operator still owns.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L6 | Failed cancel ack commit: 'drop' on switch/unmount never writes keep-running onto cancelSessionId, and decideStopFoldPre refuses to POST unless turnStatus === 'running'. Pass 3 split the ack so posted-id delete + pendingFold run even when inflight, and skip snapshot persist on switch/unmount/discarded/newer-id. That skip is correct for liveNow (do not stomp the destination session) and for Clear (do not resurrect). It is not correct for the session that still owns the live Workflow run. Stop poll sets optimistic 'cancelling' via pendingStopFoldRef → persistTurn of the abort (epoch not bumped, decideDetachPersist → live) → repo.put the poison. Failed ack then sees liveSessionId !== cancelSessionId (or unmounted) → commit: 'drop' → return without a keep-running write. repoRef is also nulled in unmount cleanup, so even a later persist would have nowhere to go. Retry is then impossible: decideStopFoldPre maps 'cancelling' → legacy-clear (never enters the POST branch); shouldSkipCancelPost is true for 'cancelling'; coldAttachFromSnapshot requires 'running'. Source-lock in lib/detachTurn.test.ts encodes the bug: liveSessionId: 's2' / unmounted: true + fold: keep asserts commit: 'drop'. It never asserts a persist onto cancelSessionId. |
Operator Stop on a live durable run. Busy clears this tick. Abort persistTurn PUTs 'cancelling' + turnRunId onto the envelope. Operator clicks another session in the rail (or closes the tab) while the cancel POST is in flight. POST returns 503 / network fail. Ack: commit: 'drop', posted-id deleted, no keep-running write. Switch back (or F5): snapshot is 'cancelling'. Stop is a no-op. Send 409s (C15 live-only: 'cancelling' + live getRun). F5 does not cold-attach. Workflow spend continues until the 1h wall — the exact ghost-spend lie G22 claimed to kill, on the leave-site path pass 3 added commit: 'drop' to protect. |
Defender: pass 3 had to drop persist on switch or a slow fold stomps the destination pre-headers snapshot; unmount nulls repoRef so there is no put surface; C15 409 "shows" the run is live; the run still ends on its own. Fails: skipping persist onto liveNow is right; skipping persist onto cancelSessionId is not. Capture the repo + snapshot at Stop fire (same pattern as runPrompt's adversarial #844 repo capture) and PUT the keep-running/clear-terminal fold onto that id unless discarded. decideStopFoldPre requiring 'running' makes posted-id delete insufficient — a poisoned 'cancelling' can never re-enter the POST branch, same-tab or after F5. A terminal run is not the bug — a still-live run with a fake 'cancelling' on the session the operator returns to is. |
high |
Residual risk
Same-session inflight pending-only (pass 3) stays closed: a Send that 409s while the cancel POST is in flight either applies keep-running via pendingFold on persistTurn, or the idle ack persists keep-running after inflight clears. Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → cancel() + overlay PATCH) still match the stream route and the plan matrix; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. In-route requireSessionUser holds even though middleware matcher is exact /api/turns (child /cancel is not on the matcher — same gap as /stream). 503 on cancel() interpolates err.message — same convention as start/stream. Pre-headers Stop (no local turnRunId yet while start has already PATCHed 'running') still cannot POST cancel — residual ghost-spend window, not introduced here. Clear-then-failed-cancel orphans the run (D18: Clear never cancels; discarded commit=drop is correct and must not resurrect). Client PUT of meta.turnRunId can claim a foreign run id (pre-existing stream ownership model; cancel raises the impact of that confused deputy — not introduced as a new gate).
Merge guidance
CONCERNS — do not merge until the Major is fixed. Suggested fix is local to the Stop-ack planner + poll:
decideStopFoldPre: liveturnRunId+ (runningorcancelling) → POST candidate. The in-memorycancelPostedRunIdsRefis the once-per-run skip (in-flight / accepted). A poisoned'cancelling'must be able to retry Stop after a failed ack / F5.shouldSkipCancelPost: stop using session'cancelling'as the skip (that is the optimistic marker, not the accepted-ack marker). Posted-id set is sufficient same-tab; F5 recovery wants a re-POST (idempotentcancel()/ 409 terminal).decideCancelAckApply:commit: 'drop'only for discarded (Clear resurrection) or a newerturnRunIdon the same session. Switch / unmount of a failed or terminal ack → persist the fold ontocancelSessionId(captured snapshot + captured repo at Stop fire — do not useliveNoworrepoRefafter unmount cleanup). Inflight same-session stayspending-only.- Lock the new rows in
lib/detachTurn.test.ts. The current switch/unmount →commit: 'drop'assertions are the bug.
What was not attacked
Live Vercel Workflows Run.cancel(); DO runner; production Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9).
…/unmount Adversarial-review #927 CONCERNS: failed cancel ack commit:drop on switch/unmount left optimistic 'cancelling' on the abandoned session, and decideStopFoldPre refused to POST unless running — Stop retry impossible, ghost spend until the 1h wall. - decideStopFoldPre: live id + running OR cancelling is a POST candidate - shouldSkipCancelPost: posted-id set is the once-per-run skip - decideCancelAckApply: persist-detached onto cancelSessionId for switch/unmount (captured snapshot + repo); drop only for Clear or a newer id on the same session Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel (02f374f) · 14 files · G22 server-cancel seam
Lenses run: L1, L2, L3, L5, L6, L8 (skip: L4 no CI/wasm; L7 no BYO/hardcode; L9 palette unused)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md yes · plan #816 yes
Pass: 5 (prior: CONCERNS → NOTES → CONCERNS → CONCERNS). Pass 1–4 stay closed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L3 | Attach-path Stop is still D18. lib/harnessChat.ts attachOperatorStop keeps running, skips pushTurnEnd, and setFailLifecycle('detach'). The poll still POSTs G22 cancel (HarnessHost takePendingCancel). Tests 6i/6j/6k and turnAttach.test.ts source-lock the old #857 keep-running. Plan #816: Stop/Esc on an attached durable run is server cancel + Turn-ended line. |
F5 (or hot resume / send-while-running attach) on a live run → Stop. Cancel POST fires; canvas goes Ready with no Turn ended · you stopped. Next Send is decideSendAttach none (status cancelling via persistTurn) → new POST /api/turns → C15 409 A turn is already in progress. Operator thinks they detached; the run is dying; they cannot start the next prompt until the workflow terminals. |
Defender: persistTurn overlays pendingFold 'cancelling', so meta is not a fake completed. That is status-only. The ring still has no stop line, attach tests still assert running + no Turn-ended, and C15 409 is the user-visible break. #857 reader-only attach Stop is superseded by G22. |
high |
| Major | L1 | decideDetachPersist drops cancelling on switch/unmount (lib/detachTurn.ts). Pass 4 persist-detached then repo.puts the Stop-fire snapshot (cancelSnapshot), which is pre-abort (no Turn-ended, possibly missing last tokens). IdSessionRepository.put replaces c.pending, so a later ack clobbers a preserve PUT. Locked: detachTurn.test.ts expects detached+cancelling → drop. |
POST-path Stop, then Switch before ack. persistTurn has the abort snapshot (cancelling + Turn-ended) but decideDetachPersist returns drop. Ack persist-detached writes S0+fold. Switching back: no stop line; last in-memory tokens from runHarnessTurn never landed. Failed ack + persistTurn-first: S0+running overwrites a richer cancelling persist. |
Defender: old Stop fold was completed, which also dropped — so switch-after-Stop already lost the abort snapshot. True, and G22 made 'cancelling' a live carrier (C15 treats it like running). Dropping it is now wrong the same way dropping running was. Pass 4 persist-detached was meant to unstick status, not to be the transcript write. |
high |
| Minor | L2 | New mutate route is not on the middleware matcher. middleware.ts isApiProtected / config.matcher are exact /api/turns only — no /api/turns/:path*. Plan #816 claimed "middleware matcher + in-route dual gate, same as stream." Stream had the same hole; cancel is a write. In-route requireSessionUser still 401s. |
Unauth POST /api/turns/:runId/cancel skips middleware (matcher miss) and is rejected only in-route. A requireSessionUser regression would expose cancel. Sessions/skills/status all ship :path*. |
Defender: stream shipped this way; cookies + SameSite still gate the route. Residual dual-gate miss, not an open unauth cancel. | high |
| Minor | L9 | Failed-ack setHostNote(...) is not scoped to cancelSessionId. Switch/unmount still paints "Stop signal did not reach the server" on the live session. |
Stop on A, switch to B, cancel POST 503. B shows A's cancel-failure note. | Defender: soft note, not a fake cancel. Still a cross-session lie. | high |
Residual risk
run.cancel() is project-scoped; ownership is "this user's envelope claims this turnRunId" (same as stream). A client PUT can carry meta.turnRunId. Run ids are unguessable in practice; planting one is the same confused-deputy stream already has. Per-process TURN_CANCEL_MIN_INTERVAL_MS is a soft isolate guard, not a durable limit. Two tabs can 429/503 the second Stop and paint a keep-running lie while the first tab's cancel already landed. Overlay PATCH failure is non-fatal by lock (C15 live-only 409 still needs a live getRun).
Merge guidance
CONCERNS: do not merge until both Majors are fixed.
- Attach Stop = G22:
cancelling+ Turn-ended. DropattachOperatorStopfrom the keep-running / skip-turn-end branches. Update 6i/6j/6k + the #857 source-lock. - Preserve
cancellingindecideDetachPersist(same asrunning).persist-detachedmust fold onto the abort snapshot already preserved, not the Stop-fire S0 (or skip the full-snapshot PUT when persistTurn already preserved).
Minors can ride the same PR.
What was not attacked
Live Workflows Run.cancel(), Production Redis LWW under dual-tab load, DO runner, Gateway. Route unit tests mock getRun; host wiring is source-locked, not browser-driven.
Adversarial-review #927 CONCERNS (pass 5): - Attach-path Stop still treated Stop as D18 keep-running (no Turn-ended) while the poll POSTed server cancel. F5/hot-resume/send-while-running Stop left Ready with no stop line; next Send C15 409'd a dying run. harnessChat now folds cancelling + Turn-ended like POST-path Stop. - decideDetachPersist dropped cancelling on switch/unmount; persist-detached then put the Stop-fire snapshot (no abort fold). Preserve cancelling as liveness (same as running). persist-detached folds onto persistTurn's abort snapshot. Failed-ack host note stays on cancelSessionId. - Middleware matcher + isApiProtected now cover /api/turns/:path* so the cancel mutate route has the claimed dual gate. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel (475aa9b) · 16 files · G22 server-cancel seam
Lenses run: L1, L2, L3, L5, L6, L8, L9 (skip: L4 no CI/wasm; L7 no BYO/hardcode)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md yes · plan #816 yes
Pass: 6 (prior: CONCERNS → NOTES → CONCERNS → CONCERNS → CONCERNS). Pass 1–5 stay closed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | Pass 5 lastStopPersistRef is a singleton wiped on every runPrompt. HarnessHost.tsx runPrompt does lastStopPersistRef.current = null (source-locked in lib/detachTurn.test.ts) before destination attach/Send. persist-detached then misses the abort snapshot (preserved.sessionId === cancelSessionId && preserved.runId === stopRunId) and PUTs Stop-fire cancelSnapshot — the exact clobber pass 5 called Major. pendingStopFoldRef is also nulled, so a later persistTurn on A cannot refresh the slot (pendingFold != null is the only writer). |
Operator Stop on A (durable wr_1). Busy clears; cancel POST in flight; persistTurn may already have stored abort snapshot + Turn-ended in lastStopPersistRef. Switch to B. Destination runPrompt (Send, or kickColdAttach if B is running) nulls the ref. POST returns accepted. Ack persist-detached: preserved is null → base = cancelSnapshot (pre-abort, no Turn-ended, possibly missing last tokens). cancelRepo.put(A, …) replaces the preserve PUT. Switch back: no Turn ended · you stopped; last in-memory tokens gone; status is 'cancelling' so C15 still 409s a dying run. Same hole if B's runPrompt wins before A's abort persistTurn: slot stays pre-Stop onSessionPatch, never the fail-path snapshot. |
Defender: pass 2/3 must null leftover fold so pre-headers persist of the next prompt cannot re-plant the old id; applyStopFoldToSession no-ops on a mismatched turnRunId; switch-without-Send keeps the ref. Fails: pendingStopFoldRef = null is the same-session leftover-fold fix (applyStopFoldToSession would hit pre-headers that still carry the old id). lastStopPersistRef is keyed at read time by cancelSessionId+stopRunId — a destination runPrompt must not drop A's slot. kickColdAttach on a running B is runPrompt without the operator sending. The pass 5 source-lock expect(run).toContain('lastStopPersistRef.current = null') encodes the wipe, not the session guard. |
high |
| Minor | L3+L9 | Successful G22 Stop lights ember host chrome. shouldSetHostTurnNote is still turnStatus !== 'running' (lib/detachTurn.ts); test locks 'cancelling' → true. HarnessHost else if (!result.ok && shouldSetHostTurnNote(persisted.turnStatus)) setHostNote(result.error) therefore paints host: Request cancelled. in ember.muted on every durable Stop. Canvas already has Turn ended · you stopped. The same branch preempts the attach follow-up notes. |
Stop on an attached durable run. Cancel POST accepted (or still in flight, optimistic 'cancelling'). persistTurn lands 'cancelling'. Host chrome under the canvas shows ember host: Request cancelled. while the ring says the operator stopped. |
Defender: main's Stop fold was 'completed', which also lit the note (#853 only excluded 'running'); diagnostic host chrome; not a fake cancel. Fails: G22's success state is 'cancelling' (host-held liveness, not a fail). Feature-divide allows DOM error toast only for host load failures. EMBER on operator Stop is palette-wrong and dual chrome next to the in-canvas stop line. |
high |
Residual risk
Same-session inflight pending-only (pass 3) and same-tab failed-ack keep-running (pass 1/4) stay closed when the operator does not runPrompt on another session before the ack. Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → cancel() + overlay PATCH) still match the stream route; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. Client PUT of meta.turnRunId can still claim a foreign run id (pre-existing stream ownership; cancel raises the impact to Run.cancel() — not a new gate). Per-process TURN_CANCEL_MIN_INTERVAL_MS is a soft isolate guard. Pre-headers Stop (no local turnRunId yet) still cannot POST cancel. Clear-then-failed-cancel orphans the run (commit: 'drop' is correct). 503 on cancel() interpolates err.message — same convention as start/stream. coldAttachFromSnapshot still requires 'running', so F5 on an honest 'cancelling' does not re-attach (C15 live-only 409 until getRun terminals).
Merge guidance
CONCERNS — do not merge until the Major is fixed.
lastStopPersistRef: null only when the slot belongs to the session starting thisrunPrompt(lastStopPersistRef.current?.sessionId === sessionRef.current.id). Destination Send /kickColdAttachmust not drop A's abort snapshot.persistTurn: keep writing the slot whenpendingFold != null; also refresh it whenpendingFoldis null but the snapshot matches the existing slot'ssessionId+runId(fail-path persist after another session wipedpendingStopFoldRef).- Lock the session guard in
lib/detachTurn.test.ts(replace the bare= nullsource-lock). The currentlastStopPersistRef.current = nullassertion is the bug.
Minor can ride the same PR: shouldSetHostTurnNote returns false for 'cancelling' as well as 'running'; update the #853 test matrix.
What was not attacked
Live Vercel Workflows Run.cancel(); Production Redis LWW under dual-tab load; DO runner; Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9). Route unit tests mock getRun; host wiring is source-locked, not browser-driven.
Adversarial-review #927 pass 6. lastStopPersistRef is session-scoped: a destination runPrompt (switch + Send / kickColdAttach) must not drop the abandoned session's abort snapshot or persist-detached falls back to the Stop-fire capture (no Turn-ended). persistTurn refreshes a matching slot even after pendingFold is nulled. Successful G22 Stop no longer lights ember host chrome (shouldSetHostTurnNote skips cancelling). Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ 35dba0f · 16 files · G22 server-cancel seam
Lenses run: L1, L2, L3, L5, L6, L8, L9 (skip: L4 no CI/wasm; L7 no BYO/hardcode)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md yes · plan #816 yes
Pass: 7 (prior: CONCERNS → NOTES → CONCERNS ×4). Pass 1–6 stay closed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L3 | Failed cancel ack never restores Busy — Stop is Busy-only, so the posted-id drop cannot be used. Pass 1 required keep-running + drop posted-id “so Stop can retry.” releaseBusyViewport still runs on the Stop poll before ack (HarnessHost.tsx · takePendingCancel). Wasm ■ Stop / Esc fire only while Lifecycle.Busy (composer_chrome.zig, docs/harness-limits.md). Keep-running persist does not setBusy(true), does not re-attach, and the host note says the run “will end on its own.” kickColdAttach / shouldKickHotResume are not invoked (operatorStop skips hot-resume; heap was nulled because abort persist was 'cancelling'). |
Operator Stop on a live durable run. Cancel POST 503 / network / 429. Busy clears this tick; canvas has Turn ended · you stopped; host note says the run will end on its own. ■ Stop is gone (Ready). Empty ▶ promotes the queue, does not attach. Typed Send attaches only if the operator happens to submit a follow-up. Posted-id is dropped but there is no Stop control to re-POST. Workflow spend continues until the 1h wall / natural terminal — the G22 lie, on the failure path the race table locked. Source-lock in lib/detachTurn.test.ts asserts setHostNote( + posted-id delete, never kickColdAttach / setBusy(true) on keep-running. |
Defender: F5 coldAttachFromSnapshot re-attaches a 'running' snapshot; switch-back too; the note is honest; pass 1 only asked to drop the posted-id. Fails: Stop retry was the locked recovery, not F5. The note actively discourages retry. F5 is not the in-canvas Stop control. Feature-divide: Stop is Wasm while Busy — host Ready while the run is live demotes that control. Empty composer cannot recover. |
high |
Residual risk
Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → run.cancel() + overlay PATCH) still match the stream route; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. Same-session inflight pending-only (pass 3) and switch/unmount persist-detached (pass 4–6) stay closed. Client PUT of meta.turnRunId can still claim a foreign run id (pre-existing stream ownership; cancel raises impact to Run.cancel() — not a new gate). Per-process TURN_CANCEL_MIN_INTERVAL_MS is a soft isolate guard. Pre-headers Stop (no local turnRunId yet) still cannot POST cancel. Optimistic Turn ended · you stopped remains on a failed-ack transcript even after keep-running (re-attach will paint live tokens after that line). Two-tab 429 on the second Stop can still paint keep-running in the losing tab while the first tab’s cancel landed. coldAttachFromSnapshot still requires 'running', so F5 on honest 'cancelling' does not re-attach.
Merge guidance
CONCERNS — do not merge until the Major is fixed.
- On same-session idle
keep-runningack: persist'running'(already done), then re-attach (kickColdAttach/ equivalent) so Wasm Busy + ■ Stop return and a later Stop can re-POST. - Do not kick on
pending-only(destination/runPromptinflight),persist-detached(switch/unmount — switch-back already cold-attaches), ordrop(Clear / newer id). - Change the host note: it must not say the run “will end on its own.” Lock a
shouldKickCancelRetryAttachplanner + source-lockkickColdAttachin the keep-running branch.
What was not attacked
Live Vercel Workflows Run.cancel(); Production Redis LWW under dual-tab load; DO runner; Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9). Route unit tests mock getRun; host wiring is source-locked, not browser-driven.
Adversarial-review #927 CONCERNS (pass 7): keep-running dropped the posted-id but releaseBusyViewport had already cleared Busy. Wasm Stop is Busy-only, so the operator could not re-POST; the host note said the run would end on its own (1h wall). Same-session idle keep-running now kickColdAttachs; pending-only / detach / drop still skip. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ f060d7b · 16 files · G22 server-cancel seam
Lenses run: L1, L2, L3, L5, L6, L8, L9 (skip: L4 no CI/wasm; L7 no BYO/hardcode)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md yes · plan #816 yes
Pass: 8 (prior: CONCERNS → NOTES → CONCERNS ×5). Pass 1–7 stay closed.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | Per-session cancel 429 undoes Stop on the next run. lastCancelAtMs is keyed by sessionId only (app/api/turns/[runId]/cancel/route.ts). An accepted cancel burns the 1s window for every run on that session. Host maps 429 → failed → keep-running + drop posted-id + kickColdAttach (pass 7). |
Stop run wr_1 (200, window burned). getRun terminals in <1s (cancel is the point). Next prompt starts wr_2 (C15 live-only 409 no longer applies). Immediate Stop on wr_2: posted-id has wr_1 not wr_2, so the poll POSTs. Route 429s. Host keep-running + kickColdAttach restores Busy / ■ Stop. Next Stop POSTs again, still inside the window, 429s again — a Stop-undo loop until TURN_CANCEL_MIN_INTERVAL_MS elapses. Run wr_2 spends until then. Source-lock row 9 only asserts same-runId 429; no row that wr_1 accepted must not 429 wr_2. |
Defender: plan Caps lock “per session” matching the C15 start guard; 1s is generous; pass 7 kick lets them retry. Fails: start 429 prevents a start; cancel 429 prevents stopping a start that already succeeded. Repeat-Stop abuse is same-run (host posted-id already once-per-runId; server 429 is defense-in-depth). Pass 7 kick is what turns a 1s wait into a Busy↔429 loop. Plan race table lumped 429 with 5xx “cancel failed” — false for “window still hot from the previous run’s accepted cancel.” |
high |
Residual risk
Pass 1–7 stay closed on this HEAD: failed cancel persists running and drops posted-id; persistTurn applies pendingStopFoldRef; inflight ack is pending-only (not a posted-id trap); switch/unmount persist-detached; attach Stop is G22 cancelling + Turn-ended; decideDetachPersist preserves 'cancelling'; lastStopPersistRef is session-scoped; same-session idle keep-running kickColdAttachs so ■ Stop returns. Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → run.cancel() + overlay PATCH) still match the stream route; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. Client PUT of meta.turnRunId can still claim a foreign run id (pre-existing stream ownership; cancel raises impact to Run.cancel() — not a new gate). Pre-headers Stop (no local turnRunId yet) still cannot POST cancel. Two-tab Stop on the same run inside the window is the intended 429 (dying-run re-attach on the losing tab). coldAttachFromSnapshot still requires 'running', so F5 on honest 'cancelling' does not re-attach.
Merge guidance
CONCERNS — do not merge until the Major is fixed.
- Key
lastCancelAtMsby run (e.g.`${sessionId}:${cleanRunId}`orcleanRunId) so an accepted cancel ofwr_1cannot 429 Stop onwr_2. Same-run repeat-Stop still 429s.cleanRunIdis already sanitized before the guard. - Lock it: accepted cancel of
wf_athen immediate cancel ofwf_bon the samesessionId→ 200,cancel()called. Row 9 (same id, second POST 429) stays. - Do not “fix” this only on the host (skip-kick on 429): that leaves Stop Busy-less on a live
wr_2. The window key is the bug.
What was not attacked
Live Vercel Workflows Run.cancel(); Production Redis LWW under dual-tab load; DO runner; Gateway; two-tab stream-event ordering after cancel; Wasm Stop pending-flag (unchanged protocol v9). Route unit tests mock getRun; host wiring is source-locked, not browser-driven.
Adversarial-review #927 CONCERNS (pass 8): lastCancelAtMs was keyed by sessionId only, so an accepted cancel of wr_1 429'd Stop on wr_2 inside 1s. Host maps 429 to keep-running + kickColdAttach — a Stop-undo loop on the next turn until the window elapsed. Key the soft guard by sessionId:runId. Same-run repeat-Stop still 429s. Lock wr_a accepted then wr_b 200 on the same session. Refs #816
Adversarial-review follow-up (
|
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: CONCERNS
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel · 16 files · durable-turn server cancel + host Stop fold
Lenses run: L1 L2 L3 L5 L6 L7 L8 L9 (skip: L4 — diff has no .github/workflows/**, runner, or artifact/fetch-harness change)
AGENTS.md read: yes
Review path: Cursor cloud agent deep pass + host/route gate verification (adversarial-review skill)
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1 | Optimistic Stop fold commits before cancelTurn acks. HarnessHost poll sets pendingStopFoldRef = cancelling, fires a non-keepalive void cancelTurn(...), then immediately abort() + releaseBusyViewport. runHarnessTurn paints Turn ended · you stopped and persistTurn PUTs turnStatus: 'cancelling' while keeping turnRunId (lib/harnessChat.ts keepCancelling). coldAttachFromSnapshot / decideAttachClass attach only running. C15 409 requires cancelling and a still-live getRun. Plan #816 lock: failed cancel keeps running and never paints cancelled without server confirmation; lost-race 503 must re-resolve via attach. |
1) Durable turn live. 2) Stop. 3) Refresh (or crash) before POST /api/turns/:runId/cancel completes — navigation aborts the fetch. Envelope is already cancelling + turnRunId; Workflow still running. 4) Reload: no cold-attach, Stop is Busy-only so hidden, Send → permanent C15 409 A turn is already in progress. Spend continues until the run dies on its own (up to the 1h wall). Same-tab 503/network: abort already painted/persisted “you stopped”; kickColdAttach then Busy-reattaches over that line (posted-id drop cannot un-paint it). |
Defender: C15 unsticks when getRun is terminal; decideCancelAckApply rewrites failed ack to keep-running + kickColdAttach; shouldSkipCancelPost is false so Stop can retry. That fails: unstick needs a terminal run, not a live one whose cancel never arrived; retry needs Busy/attach, which ignore cancelling; F5 drops the in-memory posted-id set and never runs the ack then(). Planner tests do not drive poll → unload → boot. |
high |
Residual risk
getRun().cancel() was not executed against live Vercel Workflows. pipeRunReadable.ts already records that platform cancel does not always run the loop fail() close; C15 treats cancelled as terminal and will allow a new POST /api/turns while an in-flight toolExecuteStep may still be running. Authz on the new route mirrors stream (401 + envelope turnRunId 404, never 403) and was not broken in review. Dual-tab LWW of host cancelling vs worker running/completed overlay was not load-tested. Equal-updatedAt Redis upserts overwrite; a failed-ack keep-running PUT that does not bump updatedAt can lose to a later overlay. The 1s cancel window is per-process and does not survive isolate recycle (same class as C15).
Merge guidance
- CONCERNS: do not merge until the Major is fixed (or an operator explicitly accepts: Stop+refresh can wedge a session as
cancellingover a still-live run, with no Stop and C15 409, while spend continues). - Owners:
app/harness/HarnessHost.tsx(poll order, keepalive, attach-on-cancelling),lib/harnessChat.ts(Turn-ended /keepCancellingbefore ack),lib/turnAttach.ts(coldAttachFromSnapshot/decideAttachClass),lib/detachTurn.ts(ack vs persist). - L2/L3/L7 did not breach: cancel is cookie-gated like stream, no client secrets, no dual chat, no Wasm/protocol bump.
What was not attacked
Live Run.cancel() on Production Workflows; dual-tab Redis LWW under real clocks; DO runner / build-harness; prod Gateway; browser verification of Stop → F5; Fluid billing of a hung cancel() with maxDuration = 1800.
Suggested fix punch list
| File / area | Expected behavior | Acceptance criteria |
|---|---|---|
app/harness/HarnessHost.tsx poll |
Do not abort the reader, release Busy, or persist cancelling until cancelTurn returns accepted (or terminal/gone). Use keepalive: true (or equivalent) so unload does not drop the POST. |
Stop then F5 before ack: envelope still running or cancel reached the server; boot can attach or Send is not 409-wedged over a never-cancelled run. |
lib/harnessChat.ts stop fold |
Turn ended · you stopped and keepCancelling only after an accepted cancel (or producer Request cancelled. with no abort). Failed/503/abort-before-ack keeps running, no stop line. |
Unit: abort + {ok:false, error:'Request cancelled.'} without a prior accepted cancel does not fold cancelling. Failed cancelTurn test: session running, no stop system row. |
lib/turnAttach.ts |
If optimistic cancelling remains at all, treat it as attachable liveness (coldAttachFromSnapshot / decideAttachClass) so Stop can retry after F5. |
F5 on cancelling + live getRun cold-attaches (Busy + Stop) or the optimistic persist is gone so this branch is unreachable. |
| Tests | Behavioral race rows, not only detachTurn source-locks / planner tables. |
1) Stop + simulated unload before 200 → no stuck cancelling+live getRun. 2) 503 ack never leaves a persisted stop line. 3) 409 terminal does not claim “you stopped” over a finished run. |
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927 (supplement)
Verdict: CONCERNS (unchanged)
Adds: second Major from a parallel local pass; prior Major (optimistic cancelling before ack / F5 wedge) still stands.
Additional finding
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Major | L1+L3 | Pass-7 kickColdAttach on failed cancel ack races the still-unwinding Stop runPrompt. Stop poll calls releaseBusyViewport → inflightRef=false before runHarnessTurn returns. decideCancelAckApply / shouldKickCancelRetryAttach treat that as idle and queueMicrotask(kickColdAttach). kickColdAttach only gates on inflightRef (already false), starts a new runPrompt (attach), then the original runPrompt finally still runs inflightRef.current=false; setBusy(false) (HarnessHost.tsx ~989–991) because Stop does not bump turn epoch (detached===false). Busy/■ Stop are stomped mid-retry-attach; inflight false also lets a concurrent Send enter runPrompt. Fastest trigger: sibling-tab / same-isolate 429 (pass-8 same-run window; guard is pre-tenant/envelope) while this tab’s SSE abort + fail-fold + persistTurn are still running. Same-tab fast 503 can hit it too. |
Tab A Stop → accepted cancel (window burned). Tab B Stop → 429 → keep-running + queueMicrotask(kickColdAttach) while Tab B’s aborted runPrompt has not reached finally. Kick sets Busy/inflight; original finally clears both; attach continues under Ready; ■ Stop gone again; optional second Send interleaves. Pass-7 recovery is undone on the path pass-8 left as “intended 429”. |
Defender: cancel HTTP is usually slower than abort unwind, so kick runs after finally; kickColdAttach checks inflight; two-tab 429 was an accepted residual. Fails: (1) 429 is auth + in-memory map — often faster than harnessChat fail-fold/persist; (2) inflight was already cleared by releaseBusyViewport, so the kick gate is vacuous during unwind; (3) pass-7 locked in-canvas Stop retry without F5 — stomping Busy re-breaks that lock; (4) source-lock asserts kickColdAttach in the ack path, not “after aborted runPrompt finally”. |
high |
Expanded punch list (for Fixer)
- Optimistic Stop / F5 wedge (prior review) — wait for cancel ack (or keepalive) before abort/Busy-release/
cancellingpersist; attach-or-eliminate optimisticcancelling. - Failed-ack kick vs aborted
runPromptfinally— kick only after aborted invocation cleanup, or generation token so thatfinallycannot clear Busy/inflight for a newer prompt. Do not rely oninflightRefalone afterreleaseBusyViewport. Keep pass-7 note/commit matrix; pass-8sessionId:runIdkeying unchanged.
…elling fold Stop no longer persists cancelling or aborts the reader until cancelTurn returns. keepalive keeps the POST alive across F5. Abort-before-ack and failed 503 keep running with no stop line. cancelling stays attachable on F5. Failed-ack re-attach waits for runPrompt finally (generation token). Co-authored-by: Bjorn <btipling@users.noreply.github.com>
btipling
left a comment
There was a problem hiding this comment.
Adversarial re-review — PR #927 @ 8e85b15
Verdict: PASS WITH NOTES
Prior: CONCERNS (two Majors: optimistic cancelling before ack / F5 wedge; failed-ack kick vs runPrompt finally)
Scope: Fixer round-1 commit only + nearby Stop/cancel risk
Claim checklist
| # | Claim | Result |
|---|---|---|
| 1 | Await cancelTurn (+ keepalive) before abort / Busy release / cancelling persist |
PASS — HarnessHost Stop poll posts with keepalive: true; abort + releaseBusyViewport only inside ack then via shouldAbortReaderOnCancelAck |
| 2 | keepCancelling only after accepted cancel |
PASS — G22_ACCEPTED_ABORT_REASON + isG22AcceptedAbort; raw abort keeps running, no stop line (harnessChat + tests) |
| 3 | cancelling cold-attachable |
PASS — isLiveTurnStatus = running | cancelling in turnAttach |
| 4 | Behavioral race rows | PASS with weak row-1 — harnessChat covers abort-before-ack / 503 overlay; detachTurn “row 1 unload” is a planner tautology, not a simulated unload |
| 5 | Generation token + deferred kick | PASS (race closed) — promptGenerationRef gates finally Busy clear; kick uses decideCancelRetryKickWhen. Note: host passes inflight: originalPromptActive || newerPrompt into shouldKickCancelRetryAttach, so kickWhen === 'pending' is currently unreachable. That is OK under the new design: failed ack does not abort/release Busy, so same-tab Stop retry does not need a deferred kick. 'now' still covers finally-before-failed-ack. |
Residual notes (non-blocking)
- Dead
pendingkick branch / source-locks that imply it fires — tidy later or wireshouldKickwithout foldingoriginalPromptActiveintoinflight. - Second Stop while cancel POST in-flight is a no-op (
cancelPostedRunIdsRef); Busy stays until ack (better than the old wedge; hung cancel still holds Busy). - Full vitest / browser Stop→F5 not re-run in this re-review (Fixer reported targeted greens).
Merge guidance
Safe to merge from the two prior Majors’ perspective, subject to Bjorn’s call. No further Fixer round required unless you want the dead pending path cleaned or a stronger unload integration test.
btipling
left a comment
There was a problem hiding this comment.
Adversarial review — PR #927
Verdict: PASS WITH NOTES
Repo: btipling/invincible
Scope: main ← plan/g22-server-cancel @ 8e85b15 · 16 files · G22 server-cancel seam (plan #816)
Lenses run: L1, L2, L3, L5, L6, L8, L9 (skip: L4 no CI/wasm/artifact; L7 no BYO/hardcode)
AGENTS.md read: yes · docs/feature-divide.md yes · SECURITY.md yes · plan #816 yes
Pass: 10 (same HEAD as the punch-list re-review). Pass 1–9 Majors stay closed.
Prior punch-list claims re-checked: cancel POST is keepalive: true; abort + releaseBusyViewport only in the ack then via shouldAbortReaderOnCancelAck; keepCancelling only after G22_ACCEPTED_ABORT_REASON; 'cancelling' is attachable liveness; sessionId:runId 429 key; failed ack does not abort the live reader.
Findings
| Sev | Lens | Finding | Break scenario | Refutation attempt | Confidence |
|---|---|---|---|---|---|
| Minor | L8+L3 | docs/feature-divide.md Stop row still specifies abort + Ready this tick. Plan #816 living-docs lock: this PR owns that row. Punch-list wait-for-ack made the sentence false for durable Stop — HarnessHost poll fires cancelTurn and does not abort or releaseBusyViewport until the ack. The row still reads: host aborts AbortController and clears Busy / Ready / clocks this tick, then also issues a server cancel. The Busy-clock row still says “Stop this tick”. |
A later implementer (or this file used as the product contract) “restores” abort-this-tick to match the living doc. That is the F5-wedge Major from pass 9: envelope 'cancelling' over a still-live run, Stop Busy-only so hidden, Send → C15 409, spend until the 1h wall. The code on this HEAD does not do that; the doc tells the next edit to. |
Defender: the row also mentions the cancel POST; “this tick” is leftover wording; Wasm Stop is still protocol v9 pending-cancel the same tick. Fails: plan required a timeless Stop/cancel sentence; “this tick” is now the opposite of the host contract this PR shipped (lib/detachTurn.test.ts source-lock: no abort / Busy-release before .then(). Feature-divide is the product architecture file, not a changelog. |
high |
| Nit | L8 | Stale host comments still describe the pre-ack abort. HarnessHost.tsx detachTurn says “the poll's takePendingCancel stays a raw abort.” cancelPostedRunIdsRef still calls shouldSkipCancelPost the accepted 'cancelling' marker — that helper is unconditionally false (pass 4: posted-id set is the skip). Poll comment: “abort inflight and skip starting a turn this tick.” |
New-hire / next agent reads the helper comment, treats Stop as D18-shaped raw abort again. Not a live hole. | Defender: source-locks cover the real poll order; comments are not the contract. True — hence Nit, not Minor. | high |
Residual risk
Pass 1–9 stay closed on this HEAD: failed cancel keeps running and drops posted-id; persistTurn applies pendingStopFoldRef; inflight ack is pending-only; switch/unmount persist-detached; attach Stop is G22 cancelling + Turn-ended; decideDetachPersist preserves 'cancelling'; lastStopPersistRef is session-scoped; same-session keep-running does not abort the live reader (Busy stays until a real ack that should abort). Route gates (401 → 400 runId/sessionId → 429 → tenant/store 503 → ownership 404 → absent 404 → terminal 409 → run.cancel() + overlay PATCH) still match the stream route; overlay PATCH failure is non-fatal; D18 detach sites still do not POST cancel; queue promote gate stays armed false. shouldSkipCancelPost is a named always-false seam (source-lock keeps the call). decideCancelRetryKickWhen === 'pending' is still unreachable (inflight: originalPromptActive \|\| newerPrompt); after wait-for-ack that is OK (failed ack does not release Busy). Client PUT of meta.turnRunId can still claim a foreign run id (pre-existing stream ownership; cancel raises impact to Run.cancel() — not a new gate). Pre-headers Stop (no local turnRunId yet) still cannot POST cancel. Two-tab Stop on the same run inside the 1s window is the intended 429. getRun().cancel() was not executed against live Vercel Workflows; pipeRunReadable still records that platform cancel does not always run the loop fail() close.
Merge guidance
PASS WITH NOTES — safe to merge from this attack; the Minor should ride this PR (the living-doc row this change owns) but it does not restore ghost-spend.
Suggested copy for the Stop row: durable Stop POSTs cancel first; abort + releaseBusyViewport only after accepted / terminal / gone; failed cancel keeps running + Busy so Stop can retry; unmount/switch/New/Clear/logout remain detach-only. Drop “this tick” from the Busy-clock parenthetical (Busy clears when the host actually clears it — after ack, or on terminal/error).
What was not attacked
Live Vercel Workflows Run.cancel(); Production Redis LWW under dual-tab load; DO runner; Gateway; browser Stop → F5; Fluid billing of a hung cancel() with maxDuration = 1800. Route unit tests mock getRun; host wiring is source-locked, not browser-driven.
Living-doc Stop/cancel row still said abort + Ready this tick after the punch-list wait-for-ack. Align feature-divide and host comments with the host contract (POST cancel first; abort/Busy-release after ack). Lock the row in detachTurn tests. Refs #816
Adversarial-review follow-up (
|
Implements #816 (parent umbrella #794). One row of the restart umbrella.
What
Stop/Esc on an attached durable run now issues a server cancel of the Workflow run —
POST /api/turns/:runId/cancel→getRun(runId).cancel()— instead of only aborting the local reader. Unmount / switch / New / Clear / logout still detach only (D18 lock unchanged).Why
Today's Stop fold is a lie on the durable path:
takePendingCancel→ raw abort → host writesturnRunId: undefined+turnStatus: 'completed'while the Workflow run is still live server-side (spend continues, refresh attaches to a ghost). This row fixes that fold.Changes
app/api/turns/[runId]/cancel/route.ts(new) — mirrors[runId]/streamgate-for-gate: 401 auth → 400 invalidrunId/sessionId→ 404 ownership mismatch/absent run → 409 terminal-with-status idempotent no-op (run.cancelNOT called) → liverun.cancel(). Cancel throw → fail-closed 503. Accepted cancel →overlayWorkerMetaPATCHturnStatus: 'cancelling'(worker-owned, copy-forward, LWW strictly-newer clock); PATCH failure is non-fatal (warning header only). Per-session min-interval soft guard (TURN_CANCEL_MIN_INTERVAL_MS, NEW cap) advances only on an accepted cancel.lib/turnApi.cancelTurn— typed never-throw client:accepted/terminal/gone/failed.lib/detachTurn—decideStopFoldPre/decideStopFoldPost/shouldSkipCancelPostpure planners (race-table mapping).app/harness/HarnessHost.tsx— poll fires the cancel POST once per run id, then aborts the reader; terminal/gone ack clears the id + foldscompleted(orphan-unstick); failed ack keepsrunning+ soft note (never a fake cancel).lib/harnessChat.ts— G22 stop fold keepsturnRunId+ folds'cancelling'on a genuine operator Stop (discriminated by the caller's abort signal); legacy/api/agentand the producer-cancelled SSE terminal still clear.lib/sessionCloudCaps.ts—TURN_CANCEL_MIN_INTERVAL_MS= 1000 (NEW generous cap; no existing cap raised or lowered).docs/feature-divide.md— Stop/cancel row timeless one-liner.Locks held
persistOverlayStatus/TURN_STATUS_VALUESunchanged — a cancelled run's terminal persist stays'completed';'cancelling'is host-held liveness only, always superseded.inv_*export, no bridge bump).Gates
npm run typecheck✅node scripts/di-gate.mjs✅node_modules/vitest/vitest.mjs run✅ (169 files / 3114 tests)npm run build✅ — mandatory (the new route importsworkflow/api→ Workflows bundle gate fires;/api/turns/[runId]/cancelbundled at 227 B)