Skip to content

fix(cloud-agent-next): own the stop cleanup deadline per hop - #6281

Merged
eshurakov merged 2 commits into
mainfrom
eshurakov/wild-bison
Sep 17, 2026
Merged

eshurakov merged 2 commits into
mainfrom
eshurakov/wild-bison

Conversation

@eshurakov

Copy link
Copy Markdown
Contributor

Summary

interruptSession could acknowledge an interruption while interrupting nothing. The cause was a deadline that one clock sets and a different clock enforces.

cleanupDeadlineAt is minted in the Worker as Date.now() + SANDBOX_CONTROL_CLEANUP_TIMEOUT_MS (10_000), then validated against another clock at two hops:

  1. admitSessionStop in the Durable Object, reached through admitControlStop;
  2. parseScopedStopMaintenance in SandboxControl, which returns undefined and makes SandboxControl.request throw Invalid scoped Stop maintenance request.

Because the minted value already contained the timeout, the upper bound at each hop (deadline <= now + TIMEOUT) left room only for the mint-to-validate latency — it reduced to workerNow > doNow. Any positive skew between the two machines rejected the stop as Stop cleanup deadline is invalid. At hop 1 that surfaced as success: false; at hop 2 the throw was swallowed by progressSessionStop's bare catch { continue; }. Either way the caller saw an accepted request and a session that never aborted.

Both hops now reject only an already-elapsed window and otherwise accept and saturate to now + SANDBOX_CONTROL_CLEANUP_TIMEOUT_MS, so the clock that enforces the bound is the clock that sets it. Request identity (sameRequest) no longer compares the deadline, which the Durable Object derives rather than trusts, so retrying the same stop maps to the same receipt instead of reporting a spurious Stop operation conflicts.

This also fixes a second defect found while tracing the first: withFields({ message }) overwrote the emitted message key (workers-tagged-logger emits Object.assign({}, log, this.getFields())), so the failure reason never reached the logs. It is now reason.

Verification

No manual tests were run, and I want to be explicit about why: reproducing the defect requires the Worker and the Durable Object to disagree on wall-clock time, which a single-machine local stack does not produce. The condition is positive skew (workerNow > doNow); a local stack shares one clock. The deployed E2E matrix is the real proof, and exercising it requires redeploying the cloud-agent-e2e-test Worker, which agents cannot do. So this change is verified at unit and wiring level only.

Automated checks on the pushed tree (services/cloud-agent-next):

  • focused suites for the touched paths: 9 files, 794 tests passed;
  • full package unit suite: 225 files, 6767 passed, 3 skipped;
  • typecheck (including the wrapper package): exit 0;
  • lint: 0 warnings, 0 errors over 558 files;
  • format:check: exit 0.

Visual Changes

N/A

Reviewer Notes

  • Intentional consequence: saturation means hop 2 may hold a shorter deadline than hop 1 persisted (min(D1, scNow + 10_000) <= D1). That shortens the downstream native-runtime retirement window (native-runtime-retirement.ts) rather than preserving it numerically. It does not renew the window on retry — an existing retirement receipt stays authoritative — and it stays compatible with receipt identity and recovery authority. Worth a look if you expect the deadline to be preserved end to end.
  • Pre-existing assertions changed (disclosed): scoped-stop-maintenance.test.ts expected { cleanupDeadlineAt: 11_001 }, now: 1_000 to be rejected; it now saturates to 11_000. lifecycle.test.ts had a table row using cleanupDeadlineAt: Date.now() that only the removed upper-bound clause rejected — the fixture is now 1, which preserves the row's elapsed-case intent. Both changes are visible in the diff; no other existing test needed changing.
  • Left alone deliberately: the wrapper's elapsed-only container-clock check (sandbox-control-handlers.ts) is the correct fail-closed direction and is out of scope. Extreme negative Worker skew (Worker clock far behind the acting clock) still rejects as elapsed, by design.
  • Not changed, worth a follow-up: hop 2 still signals a rejected stop by throwing into progressSessionStop's silent catch, so a malformed or elapsed scoped stop remains invisible to the caller. This PR makes that path hard to reach, but does not make it observable.

…of comparing clocks

`cleanupDeadlineAt` is minted on the Worker as `Date.now() + 10_000` and then
validated against a different clock at two hops: `admitSessionStop` in the
Durable Object, and `parseScopedStopMaintenance` in `SandboxControl`. Because the
minted value already includes the timeout, the upper bound only left room for the
mint-to-validate latency, so any positive skew rejected the stop as
"Stop cleanup deadline is invalid". Hop 1 surfaced that as `success: false`, and
hop 2 threw from `SandboxControl.request` into `progressSessionStop`'s bare
`catch`, so `interruptSession` reported a successful HTTP response and silently
interrupted nothing.

Reject only an already-elapsed window; otherwise accept and saturate to
`now + SANDBOX_CONTROL_CLEANUP_TIMEOUT_MS` at each hop, so the clock that enforces
the bound is the clock that sets it. Request identity no longer compares the
deadline, which the Durable Object derives rather than trusts, so a retry of the
same stop still maps to the same receipt instead of reporting a spurious conflict.
…essage field

`workers-tagged-logger` emits `Object.assign({}, log, fields)`, so a field named
`message` replaced the readable message text and the failure reason never reached
the logs. Use `reason`, which does not collide with the emitted base keys.
@kilo-code-bot

kilo-code-bot Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Verified the per-hop Stop cleanup deadline change: the removed cross-clock upper bound is replaced by saturation to now + SANDBOX_CONTROL_CLEANUP_TIMEOUT_MS at both the Durable Object (admitSessionStop) and SandboxControl (parseScopedStopMaintenance), while an already-elapsed deadline still fails closed. Removing cleanupDeadlineAt from sameRequest keeps retries idempotent against the derived deadline, and the withFields({ reason }) rename avoids overwriting the emitted log message. The new and adjusted tests are deterministic (fake timers) and exercise read-side and write-side paths consistently.

Files Reviewed (8 files)
  • services/cloud-agent-next/src/router.test.ts
  • services/cloud-agent-next/src/router/handlers/session-management.ts
  • services/cloud-agent-next/src/sandbox-control/lifecycle.test.ts
  • services/cloud-agent-next/src/sandbox-control/scoped-stop-maintenance.test.ts
  • services/cloud-agent-next/src/sandbox-control/scoped-stop-maintenance.ts
  • services/cloud-agent-next/src/sandbox-session/session-message-queue.test.ts
  • services/cloud-agent-next/src/sandbox-session/session-stop.test.ts
  • services/cloud-agent-next/src/sandbox-session/session-stop.ts

Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

@eshurakov
eshurakov merged commit 9a7a27e into main Sep 17, 2026
26 checks passed
@eshurakov
eshurakov deleted the eshurakov/wild-bison branch September 17, 2026 20:31
eshurakov added a commit that referenced this pull request Sep 17, 2026
…ed stack

Give the harness one implementation per scenario that runs under either the
local or the deployed profile, and drive it through an e2e-only Worker surface
that uses only mechanisms production also has.

- Deployed profile: E2E_PROFILE=deployed drives a real Cloudflare stack. Auth
  resolves env-first and then from an ordinary personal-token file, so
  E2E_USER_TOKEN + FAKE_LLM_ADMIN_TOKEN alone are enough; the path never reads
  .dev.vars, root env files or Postgres, and never mints a credential.
  assertDeployedProfileEnv owns source precedence and resolveStreamTicket owns
  the connected-stream credential for both profiles.
- Shared scenarios: cold-hot, unknown-model, auth-reject, chunked-streaming,
  empty-response, waiters-clean, llm-error, the three callback scenarios and
  nine queue/continuity/micro scenarios now have one shared definition, so 19
  of the 34 scenarios run under either profile. scenario-capabilities.ts owns
  the gate (a missing injected environment, and a local profile without the
  mandatory `sandbox` capability, are errors before any side effect; a
  declared-but-absent capability is an explicit `unsupported`; a throwing
  capability stays a failure) and run.ts owns the 1/2/0 exit policy.
- E2e surface: an e2e-only entrypoint exposing /__e2e/*, reachable only behind
  the e2e-scoped INTERNAL_API_SECRET sent as x-internal-api-key, compared
  constant-time and failing closed when unset, with the caller's JWT still
  required on every route except the callback ingest route, which is
  authenticated by its own unguessable path token. The production entrypoint
  and its routes are untouched and the surface is unreachable there by
  construction, not by a flag. Callback bodies are read as raw bytes and
  cancelled past 64 KiB, bounded to 50 records with a one-hour TTL.
- Less duplicated ownership: worktree ownership is read from getSession instead
  of a second direct cli_sessions_v2 query, the transcript reduction skips CLI
  progress parts marked `kilocode.lifecycle == transient` so the echoed payload
  stays at the end of the observed text, gate tags are run-scoped with one
  release/leak owner, and API selection has one owner that fails a conflicting
  pin instead of skipping it.
- deploy-e2e-worker.sh no longer requires INTERNAL_API_SECRET on redeploy:
  wrangler never clears an existing Worker secret, so a supplied value now
  means "rotate" and its absence reports that the deployed value is kept.

Honest gaps: test/e2e/** is outside the package typecheck; the retryable
provider-error path stays uncovered because llm-error now asserts the terminal
failure the product actually produces (its retry premise was stale); the Docker
profile's full scenario remains unverified locally for an egress-proxy reason
(dialing 127.0.0.1:3000 while this worktree's Next.js is on 8300); and the
deployed matrix currently fails five interrupt/queue scenarios. That cause is now
established - a cross-machine deadline comparison in the product Worker - and is
fixed in #6281.

No behaviour change in this commit: the harness and dev-launcher files are also
reformatted to satisfy the repo-wide `oxfmt --list-different .` check, which the
package-level formatter (src only) does not cover.
eshurakov added a commit that referenced this pull request Sep 17, 2026
…ed stack

Give the harness one implementation per scenario that runs under either the
local or the deployed profile, and drive it through an e2e-only Worker surface
that uses only mechanisms production also has.

- Deployed profile: E2E_PROFILE=deployed drives a real Cloudflare stack. Auth
  resolves env-first and then from an ordinary personal-token file, so
  E2E_USER_TOKEN + FAKE_LLM_ADMIN_TOKEN alone are enough; the path never reads
  .dev.vars, root env files or Postgres, and never mints a credential.
  assertDeployedProfileEnv owns source precedence and resolveStreamTicket owns
  the connected-stream credential for both profiles.
- Shared scenarios: cold-hot, unknown-model, auth-reject, chunked-streaming,
  empty-response, waiters-clean, llm-error, the three callback scenarios and
  nine queue/continuity/micro scenarios now have one shared definition, so 19
  of the 34 scenarios run under either profile. scenario-capabilities.ts owns
  the gate (a missing injected environment, and a local profile without the
  mandatory `sandbox` capability, are errors before any side effect; a
  declared-but-absent capability is an explicit `unsupported`; a throwing
  capability stays a failure) and run.ts owns the 1/2/0 exit policy.
- E2e surface: an e2e-only entrypoint exposing /__e2e/*, reachable only behind
  the e2e-scoped INTERNAL_API_SECRET sent as x-internal-api-key, compared
  constant-time and failing closed when unset, with the caller's JWT still
  required on every route except the callback ingest route, which is
  authenticated by its own unguessable path token. The production entrypoint
  and its routes are untouched and the surface is unreachable there by
  construction, not by a flag. Callback bodies are read as raw bytes and
  cancelled past 64 KiB, bounded to 50 records with a one-hour TTL.
- Less duplicated ownership: worktree ownership is read from getSession instead
  of a second direct cli_sessions_v2 query, the transcript reduction skips CLI
  progress parts marked `kilocode.lifecycle == transient` so the echoed payload
  stays at the end of the observed text, gate tags are run-scoped with one
  release/leak owner, and API selection has one owner that fails a conflicting
  pin instead of skipping it.
- deploy-e2e-worker.sh no longer requires INTERNAL_API_SECRET on redeploy:
  wrangler never clears an existing Worker secret, so a supplied value now
  means "rotate" and its absence reports that the deployed value is kept.

Honest gaps: test/e2e/** is outside the package typecheck; the retryable
provider-error path stays uncovered because llm-error now asserts the terminal
failure the product actually produces (its retry premise was stale); the Docker
profile's full scenario remains unverified locally for an egress-proxy reason
(dialing 127.0.0.1:3000 while this worktree's Next.js is on 8300); and the
deployed matrix currently fails five interrupt/queue scenarios. That cause is now
established - a cross-machine deadline comparison in the product Worker - and is
fixed in #6281.

No behaviour change in this commit: the harness and dev-launcher files are also
reformatted to satisfy the repo-wide `oxfmt --list-different .` check, which the
package-level formatter (src only) does not cover.
eshurakov added a commit that referenced this pull request Sep 21, 2026
…ed stack

Give the harness one implementation per scenario that runs under either the
local or the deployed profile, and drive it through an e2e-only Worker surface
that uses only mechanisms production also has.

- Deployed profile: E2E_PROFILE=deployed drives a real Cloudflare stack. Auth
  resolves env-first and then from an ordinary personal-token file, so
  E2E_USER_TOKEN + FAKE_LLM_ADMIN_TOKEN alone are enough; the path never reads
  .dev.vars, root env files or Postgres, and never mints a credential.
  assertDeployedProfileEnv owns source precedence and resolveStreamTicket owns
  the connected-stream credential for both profiles.
- Shared scenarios: cold-hot, unknown-model, auth-reject, chunked-streaming,
  empty-response, waiters-clean, llm-error, the three callback scenarios and
  nine queue/continuity/micro scenarios now have one shared definition, so 19
  of the 34 scenarios run under either profile. scenario-capabilities.ts owns
  the gate (a missing injected environment, and a local profile without the
  mandatory `sandbox` capability, are errors before any side effect; a
  declared-but-absent capability is an explicit `unsupported`; a throwing
  capability stays a failure) and run.ts owns the 1/2/0 exit policy.
- E2e surface: an e2e-only entrypoint exposing /__e2e/*, reachable only behind
  the e2e-scoped INTERNAL_API_SECRET sent as x-internal-api-key, compared
  constant-time and failing closed when unset, with the caller's JWT still
  required on every route except the callback ingest route, which is
  authenticated by its own unguessable path token. The production entrypoint
  and its routes are untouched and the surface is unreachable there by
  construction, not by a flag. Callback bodies are read as raw bytes and
  cancelled past 64 KiB, bounded to 50 records with a one-hour TTL.
- Less duplicated ownership: worktree ownership is read from getSession instead
  of a second direct cli_sessions_v2 query, the transcript reduction skips CLI
  progress parts marked `kilocode.lifecycle == transient` so the echoed payload
  stays at the end of the observed text, gate tags are run-scoped with one
  release/leak owner, and API selection has one owner that fails a conflicting
  pin instead of skipping it.
- deploy-e2e-worker.sh no longer requires INTERNAL_API_SECRET on redeploy:
  wrangler never clears an existing Worker secret, so a supplied value now
  means "rotate" and its absence reports that the deployed value is kept.

Honest gaps: test/e2e/** is outside the package typecheck; the retryable
provider-error path stays uncovered because llm-error now asserts the terminal
failure the product actually produces (its retry premise was stale); the Docker
profile's full scenario remains unverified locally for an egress-proxy reason
(dialing 127.0.0.1:3000 while this worktree's Next.js is on 8300); and the
deployed matrix currently fails five interrupt/queue scenarios. That cause is now
established - a cross-machine deadline comparison in the product Worker - and is
fixed in #6281.

No behaviour change in this commit: the harness and dev-launcher files are also
reformatted to satisfy the repo-wide `oxfmt --list-different .` check, which the
package-level formatter (src only) does not cover.
eshurakov added a commit that referenced this pull request Sep 21, 2026
…ed stack

Give the harness one implementation per scenario that runs under either the
local or the deployed profile, and drive it through an e2e-only Worker surface
that uses only mechanisms production also has.

- Deployed profile: E2E_PROFILE=deployed drives a real Cloudflare stack. Auth
  resolves env-first and then from an ordinary personal-token file, so
  E2E_USER_TOKEN + FAKE_LLM_ADMIN_TOKEN alone are enough; the path never reads
  .dev.vars, root env files or Postgres, and never mints a credential.
  assertDeployedProfileEnv owns source precedence and resolveStreamTicket owns
  the connected-stream credential for both profiles.
- Shared scenarios: cold-hot, unknown-model, auth-reject, chunked-streaming,
  empty-response, waiters-clean, llm-error, the three callback scenarios and
  nine queue/continuity/micro scenarios now have one shared definition, so 19
  of the 34 scenarios run under either profile. scenario-capabilities.ts owns
  the gate (a missing injected environment, and a local profile without the
  mandatory `sandbox` capability, are errors before any side effect; a
  declared-but-absent capability is an explicit `unsupported`; a throwing
  capability stays a failure) and run.ts owns the 1/2/0 exit policy.
- E2e surface: an e2e-only entrypoint exposing /__e2e/*, reachable only behind
  the e2e-scoped INTERNAL_API_SECRET sent as x-internal-api-key, compared
  constant-time and failing closed when unset, with the caller's JWT still
  required on every route except the callback ingest route, which is
  authenticated by its own unguessable path token. The production entrypoint
  and its routes are untouched and the surface is unreachable there by
  construction, not by a flag. Callback bodies are read as raw bytes and
  cancelled past 64 KiB, bounded to 50 records with a one-hour TTL.
- Less duplicated ownership: worktree ownership is read from getSession instead
  of a second direct cli_sessions_v2 query, the transcript reduction skips CLI
  progress parts marked `kilocode.lifecycle == transient` so the echoed payload
  stays at the end of the observed text, gate tags are run-scoped with one
  release/leak owner, and API selection has one owner that fails a conflicting
  pin instead of skipping it.
- deploy-e2e-worker.sh no longer requires INTERNAL_API_SECRET on redeploy:
  wrangler never clears an existing Worker secret, so a supplied value now
  means "rotate" and its absence reports that the deployed value is kept.

Honest gaps: test/e2e/** is outside the package typecheck; the retryable
provider-error path stays uncovered because llm-error now asserts the terminal
failure the product actually produces (its retry premise was stale); the Docker
profile's full scenario remains unverified locally for an egress-proxy reason
(dialing 127.0.0.1:3000 while this worktree's Next.js is on 8300); and the
deployed matrix currently fails five interrupt/queue scenarios. That cause is now
established - a cross-machine deadline comparison in the product Worker - and is
fixed in #6281.

No behaviour change in this commit: the harness and dev-launcher files are also
reformatted to satisfy the repo-wide `oxfmt --list-different .` check, which the
package-level formatter (src only) does not cover.
eshurakov added a commit that referenced this pull request Sep 22, 2026
…ed stack (#6284)

* test(cloud-agent-next): run the shared e2e scenarios against a deployed stack

Give the harness one implementation per scenario that runs under either the
local or the deployed profile, and drive it through an e2e-only Worker surface
that uses only mechanisms production also has.

- Deployed profile: E2E_PROFILE=deployed drives a real Cloudflare stack. Auth
  resolves env-first and then from an ordinary personal-token file, so
  E2E_USER_TOKEN + FAKE_LLM_ADMIN_TOKEN alone are enough; the path never reads
  .dev.vars, root env files or Postgres, and never mints a credential.
  assertDeployedProfileEnv owns source precedence and resolveStreamTicket owns
  the connected-stream credential for both profiles.
- Shared scenarios: cold-hot, unknown-model, auth-reject, chunked-streaming,
  empty-response, waiters-clean, llm-error, the three callback scenarios and
  nine queue/continuity/micro scenarios now have one shared definition, so 19
  of the 34 scenarios run under either profile. scenario-capabilities.ts owns
  the gate (a missing injected environment, and a local profile without the
  mandatory `sandbox` capability, are errors before any side effect; a
  declared-but-absent capability is an explicit `unsupported`; a throwing
  capability stays a failure) and run.ts owns the 1/2/0 exit policy.
- E2e surface: an e2e-only entrypoint exposing /__e2e/*, reachable only behind
  the e2e-scoped INTERNAL_API_SECRET sent as x-internal-api-key, compared
  constant-time and failing closed when unset, with the caller's JWT still
  required on every route except the callback ingest route, which is
  authenticated by its own unguessable path token. The production entrypoint
  and its routes are untouched and the surface is unreachable there by
  construction, not by a flag. Callback bodies are read as raw bytes and
  cancelled past 64 KiB, bounded to 50 records with a one-hour TTL.
- Less duplicated ownership: worktree ownership is read from getSession instead
  of a second direct cli_sessions_v2 query, the transcript reduction skips CLI
  progress parts marked `kilocode.lifecycle == transient` so the echoed payload
  stays at the end of the observed text, gate tags are run-scoped with one
  release/leak owner, and API selection has one owner that fails a conflicting
  pin instead of skipping it.
- deploy-e2e-worker.sh no longer requires INTERNAL_API_SECRET on redeploy:
  wrangler never clears an existing Worker secret, so a supplied value now
  means "rotate" and its absence reports that the deployed value is kept.

Honest gaps: test/e2e/** is outside the package typecheck; the retryable
provider-error path stays uncovered because llm-error now asserts the terminal
failure the product actually produces (its retry premise was stale); the Docker
profile's full scenario remains unverified locally for an egress-proxy reason
(dialing 127.0.0.1:3000 while this worktree's Next.js is on 8300); and the
deployed matrix currently fails five interrupt/queue scenarios. That cause is now
established - a cross-machine deadline comparison in the product Worker - and is
fixed in #6281.

No behaviour change in this commit: the harness and dev-launcher files are also
reformatted to satisfy the repo-wide `oxfmt --list-different .` check, which the
package-level formatter (src only) does not cover.

* fix(cloud-agent-next): close leaked streams and bound ticket acquisition

- chunked-streaming / empty-response: hoist the stream and close it in a
  `finally`, so a throwing `requireContainer` cannot leave an open,
  subscribed WebSocket behind
- hot: track the warm-up stream so the existing `finally` closes it on the
  allocation-failure path too
- fetchStreamTicket: bound the ticket POST with AbortSignal.timeout(10s) so
  a backend that accepts but never responds cannot hang the scenario past
  its deadline, and report a stalled body-read as a timeout rather than as
  invalid JSON; the bound is injectable so tests can use a small value
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