Skip to content

Run keyed playwright and REPL executions at most once - #491

Open
rgarcia wants to merge 3 commits into
mainfrom
hypeship/execute-idempotency-key
Open

rgarcia wants to merge 3 commits into
mainfrom
hypeship/execute-idempotency-key

Conversation

@rgarcia

@rgarcia rgarcia commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Playwright execute and the browser REPL can now be retried safely. Both endpoints accept an optional Idempotency-Key header, and each key runs at most once.

The Stainless SDKs retry on 408, 409, 429, 5xx, connection errors and client timeouts. The TS and Python SDKs default to a 60s timeout, and execute also defaults to timeout_sec: 60. As a result, a single execute call could run the user's code up to three times, and the caller only ever saw a timeout error.

Behavior

  • A repeat call with the same key returns the first call's response, waiting for it if it is still running.
  • Reusing a key with a different request returns 422. The request is compared on executor, code and timeout for playwright, and on code, reset and timeout for REPL.
  • Only 200 responses are kept. A 400, 409 or 500 (or a Go error) means the code never reached the daemon, so the key is released and a retry runs fresh.
  • A keyed call keeps running while any caller waits on its key, plus 10 seconds after the last one disconnects, so a retry can collect its result. After that it is cancelled like an unkeyed call, so aborting a hung REPL cell still frees the REPL.
    • It keeps its own scale-to-zero hold while it runs.
  • Memory is bounded:
    • Kept responses: at most 16 for playwright and 4 for REPL, each for 10 minutes.
    • Keyed calls waiting or running: at most 32 for playwright and 8 for REPL. A new key past that limit gets a 429 before anything runs, and the SDKs retry it.
  • A panic in a keyed call is recovered into an error. The call runs outside the router's panic recovery.

Compatibility

Declaring the header changes the generated client signatures. ExecutePlaywrightCode* and ExecuteBrowserRepl* now take a *Params argument. The e2e callers are updated, with nil params. Downstream consumers need the same one-argument change when they pick this up. API side: kernel/kernel#5031.

Testing

  • go vet ./... is clean.
  • go test -race passes for all non-e2e packages. The REPL tests run against real Node and esbuild. lib/devtoolsproxy failed once on an unrelated temp-dir cleanup race with Chromium and passed 5 of 5 on re-run.
  • New tests:
    • unit tests for the idempotency helper: run once, key reuse, key release, caller cancellation, abandoned-call cancellation, retention and cap, panic recovery, pending cap
    • a playwright handler test: disconnect then retry, 422, 409 releases the key, the scale-to-zero hold
    • REPL tests: a disconnect mid-execution doesn't kill the REPL and the retry gets the result; an abandoned hung cell is terminated after the grace period, so a reset gets a fresh REPL
  • Mutation checks:
    • Removing the detach from the caller's context makes the REPL and cancellation tests fail.
    • Removing the scale-to-zero hold makes the playwright test fail.
  • e2e tests compile but were not run, since they need the Docker images.

Reviewed with sol-loop: 2 rounds (before the grace-period change from review) with openai-codex/gpt-6.1-sol on extra-high reasoning. Sol verdict: approved.

@rgarcia
rgarcia requested a review from tnsardesai October 27, 2026 14:59
@rgarcia

rgarcia commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Picked tnsardesai to review these related PRs (#491, kernel/kernel#5031): recent commits in e2e; 1 pending

@tnsardesai tnsardesai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

  • Blocking compatibility break — aborting a REPL call no longer stops a hung cell once SDKs key every request — server/cmd/api/api/browser_repl.go:L669-L672
    • Problem: the keyed path runs Execute on context.WithoutCancel, so disconnecting no longer kills the running cell. kernel/kernel#5031 turns on Stainless idempotency, which attaches a key to every SDK POST. So the "no key → disconnect kills the REPL" path this PR keeps is effectively gone for SDK users. A follow-up reset: true doesn't help, because it waits on the same admission channel behind the running cell.
    • Impact: an agent or user that aborts a stuck cell (for example, await on a promise that never resolves) and then resets or retries is blocked for up to timeout_sec (max 300s), and the VM stays awake. Today they get a fresh REPL right away.
    • Fix: give keyed calls a way to cancel. One option: reset: true (or a dedicated cancel endpoint) cancels the in-flight keyed cell instead of queuing behind admission. Another: keep the at-most-once guarantee but cancel the detached call once no caller has been waiting on the key for some grace period.

@rgarcia

rgarcia commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Fixed in 00d6058 using the grace-period approach.

  • What changed: a keyed call now keeps running only while some caller is waiting on its key, plus 10 seconds after the last one leaves. Our SDKs wait at most 8s between retries, so a retry still attaches and collects the result.
    • After 10s with no caller, the call's context is cancelled. For REPL, that terminates the hung cell, the same as an unkeyed disconnect does today.
    • The scale-to-zero hold is released as soon as the call ends, so the VM doesn't stay awake.
    • A retry that attaches during the grace period stops the timer.
  • Your scenario: abort a hung keyed cell, then send reset: true. The reset now gets a fresh REPL after about 10s (worst case) instead of timeout_sec (up to 300s). TestBrowserReplAbandonedKeyedCellIsTerminated covers this with a shortened grace period: the reset returned a new repl_id in under a second.
  • Not changed: reset: true still queues behind a running cell. That was already true for unkeyed calls from a live caller, so I left it alone here. If you'd like reset to cancel the in-flight cell immediately, I can do that as a follow-up.
  • Tests: TestIdempotentCallsCancelAbandonedCall checks that a waiting retry keeps the call alive and that the abandoned call is then cancelled and its key released. The full go test -race suite passes.
  • Docs: the header description now states the 10-second behavior in both specs (kernel/kernel#5031 updated to match).

@rgarcia
rgarcia requested a review from tnsardesai October 28, 2026 02:11

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 00d6058. Configure here.

}
} else {
delete(c.calls, key)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abandoned calls consume idempotency keys

Medium Severity

After abandonGrace, cancel turns into a 200 in both handlers: Playwright maps admission or setup cancel to execution failed before any code runs, and REPL maps a mid-read cancel to a terminated 200. keep then stores that abort, so a later retry with the same key receives the cached failure instead of running again.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 00d6058. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants