fix(studio-server): the cancel route replies once the adapter's stop has settled - #7221

Requesting changes at 57e4c013. The one blocker is CI; the change itself looks right.

Blocker: the Comments workflow fails at this head (comment-ratchet, against merge-base 1fecf5b82):

• packages/studio-server/src/routes/render.ts: comment share goes from 10.4% to 10.5% (+1 line, the new // The reply follows the adapter's stop…)
• packages/studio-server/src/types.ts: comment share goes from 18.5% to 19.1% (+2 lines in the cancel JSDoc)

To fix it without losing the explanation, fold it into comments that already exist. The route's header comment ("Marks the job cancelled immediately … and invokes the adapter's abort hook") can say it also waits for the hook's promise. Then drop the inline line. Trim the JSDoc addition to stay within the existing line count. Naming the type (for example cancel?: () => void | Promise<void> is self-describing) means the extra prose isn't needed.

What I checked

• fails without the fix: line: present and accurate. I reverted only render.ts and types.ts to main and ran render.test.ts. replies only once the adapter's stop has settled failed with expected false to be true. With the PR, all 91 tests pass.
• A sync or missing cancel still behaves the same: Promise.race([undefined, late]) settles on the next microtask and the timer is cleared.
• If a stop takes longer than 45 s and then rejects, the rejection is handled. Promise.race subscribes to every input, so there's no unhandled rejection and the process doesn't crash.
• Studio's useRenderQueue.cancelRender updates the row optimistically before it awaits the POST, so a slow stop doesn't freeze the UI.

Non-blocking

• If cancel() rejects before the 45 s timeout, the route now returns a 500 from Hono's error path, even though job.status is already "cancelled". Studio shows "The render may still be running", which is arguably the right message. If that's the intent, a short test would lock it in. If not, add .catch(() => {}) and reply with the status.
• A second cancel that arrives while the first is still waiting sees status === "cancelled" and replies straight away, before the stop has settled. That's the same stale read for a concurrent caller. It's fine for now; keeping the in-flight promise on the job would close it if the Desktop host needs that.

Other CI at this head: regression, Player perf, PR captures and preview-regression are green. CI, CodeQL and Windows render verification were still running when I reviewed.

Reviewed at f5b598cdb54834bfb9050d97840b3bc61bd1cd9d.

Need: yes, allow host stop completion before replying · Size: ok, three files · Reuse: existing RenderJobState and status response · Simpler: local promise race is sufficient · Structure: fine
Risk: medium — changes cancellation across the route/host boundary; real asynchronous host integration remains untested.

The first cancellation now waits for the adapter hook to settle or for 45 seconds to expire, then responds with the current status (packages/studio-server/src/routes/render.ts:250–261). Missing/void hooks retain prompt completion; the SSE terminal state is still set immediately. Both existing OSS adapters use synchronous AbortController hooks; I traced Studio's optimistic cancel and response handling.

The earlier Genesis comment-ratchet concern is resolved: the delta since 57e4c013 removes four comment lines and changes no executable code. The body still names the exact fails without the fix: regression. The route and types retain their base comment counts (43 and 48); the ratchet's measure/judge reports no problems.

Nit — bounded timer cleanup: render.ts:257–258 allocates the 45-second timeout before evaluating job.cancel?.(). A synchronous throw escapes before .finally is installed, leaving the timer referenced until it fires. An actual-route probe observed one timer left after HTTP 500, versus zero on sync success or async rejection. Use language-level try { await Promise.race([job.cancel?.(), late]); } finally { clearTimeout(giveUp); }; that control passes all six probes. This is an exceptional extension-hook path with a bounded lifetime, so I do not block the functional improvement on it.

Scope: as Genesis already noted, a concurrent second cancel replies immediately while the first stop is pending, and an early stop rejection yields HTTP 500 despite the optimistic cancelled state. I reproduced those limitations; this is not a guarantee that every cancel reply certifies stopped work. The existing caller initiates one cancel per UI action; preserving the earlier second-call behavior does not regress that path. Shared in-flight cancellation would be the follow-up for a host requiring every concurrent caller to await the same stop.

Validation: frozen install and full root bun run build passed. Four native route/stream/deletion/finished-job files: 108 passed. Reverting only route/types to PR base makes the named regression fail (expected false to be true); restored head passes. Independent parent replay of render.test.ts plus six adversarial probes: 96 passed, one intentionally failing cleanup assertion as described above; the try/finally control passes all six probes. Timeout expiry and rejection after timeout were probed; deletion during a pending cancel was not. No cloud service or live Desktop async-stop run. Local Bun was 1.3.14 versus the manifest's 1.4.4.

Verdict: APPROVE
Reasoning: The initial async-stop response behavior and its regression are verified, and the earlier requested comment cleanup is complete. Remaining exceptional-path cleanup is a small, concrete nit; the concurrent-cancel limitation is explicitly scoped.

— Rames (hf-review)

Reviewed at f6179ee69cb479108e257f876500fb42513d2849.

Need: yes, finish cancellation timer cleanup · Size: ok, one-route delta · Reuse: existing timeout and Promise.race · Simpler: language-level try/finally · Structure: fine
Risk: low — isolated cleanup repair; response and timeout policy are unchanged.

Delta re-review from my approved f5b598cd: the delta changes only packages/studio-server/src/routes/render.ts:258–262. The hook invocation is now inside try, so finally clears the timeout on a synchronous throw as well as async fulfillment, rejection, or timeout. My prior timer nit is resolved. The earlier Genesis comment-count repair remains intact; no comments were added here.

Validation: frozen install and full root build passed; native render.test.ts plus the six prior actual-route probes pass (97 tests). Those probes cover sync return, sync throw, early async rejection, timeout followed by late rejection, plus the previously documented concurrent-cancel/retry behavior. The synchronous-throw probe failed on the prior head and now leaves zero timers. Independent delta audit and seven extracted-fragment Node probes found no blocker. The prior round's broader 108-test run was on f5b598cd, not this head. No real Desktop/cloud async-host run; concurrent second cancellation still returns immediately and early hook failure still returns HTTP 500. Local Bun 1.3.14 versus declared 1.4.4.

Verdict: APPROVE
Reasoning: The requested cleanup fix closes the reproduced exceptional path without changing cancellation semantics. No new blocker in the delta.

— Rames (hf-review)

Approving at e341d135. My earlier blocker, the Comments check, now passes at this head.

Changes since my review at 57e4c013. The update says "no code change", but the head differs from the one I reviewed in two ways. I reviewed both:

• The three added comment lines are gone, both in render.ts and in the types.ts JSDoc. That fixes the ratchet.
• The cancel route now uses try { await Promise.race(...) } finally { clearTimeout(giveUp) } instead of .finally() on the race. Before, a cancel() that threw synchronously skipped clearTimeout and left the 45 s timer pending. Now it is always cleared. That's an improvement, and the sync-throw path still returns the same 500 as on main.

What I checked

• Fails-without-the-fix line: present and accurate. With only render.ts and types.ts reverted to main, replies only once the adapter's stop has settled fails with expected false to be true. With the PR, all 91 tests in render.test.ts pass.
• A sync or missing cancel still behaves as on main. A stop that takes longer than 45 s and then rejects causes no unhandled rejection, because Promise.race subscribes to it.

Non-blocking, carried over: if the stop rejects within 45 s, the route returns a 500 even though the job is already marked cancelled. A second cancel that arrives while the first is waiting gets an immediate reply.

CI at this head: Comments, regression, PR captures and Player perf are green. CI, CodeQL, preview-regression and Windows render verification were still running at approval, and those still gate the merge.