Skip to content

feat(sandbox): record why a container stopped - #138

Open
nplusonedev wants to merge 1 commit into
mainfrom
feat/record-container-stop
Open

feat(sandbox): record why a container stopped#138
nplusonedev wants to merge 1 commit into
mainfrom
feat/record-container-stop

Conversation

@nplusonedev

Copy link
Copy Markdown
Contributor

Problem & Insight

A run whose container dies reports this, and nothing else:

ExecFailed: exec failed (exit -1): internal error; reference = <id>

Every explanation offered for that class has now been measured and ruled out on the consumer that motivated it. With the per-stage probe running: peak memory 3.5 GiB of 11.9, disk 3.6 GB used against 8.4 GB free, and death times (137s, 647s, 1284s) shorter than the successes (2128s, 2176s). Nothing is scarce and no duration is safe.

That leaves a question only the platform can answer — and the platform has been answering it all along, one layer below where anyone was looking.

@cloudflare/containers parses the runtime's own message — runtime signalled the container to exit: <n>, or container exited with unexpected exit code: <n> — into the exitCode it hands onStop. A container something killed carries a signal there; one that ran to completion carries 0. Nothing in this repo read it.

Take

The three sandbox DO classes override onStop and persist {sandbox, exitCode, reason, requested, observedAt} to container-stops/<sandbox>/<ts>.json, logged as well as written.

Three things about the SDK decide the shape, and each is a trap:

  • reason is not the discriminator, despite its type. StopParams.reason is declared 'exit' | 'runtime_signal', but runtime_signal appears nowhere in @cloudflare/containers@0.3.7 outside that declaration — both callOnStop sites pass 'exit'. Read exitCode.
  • A 0 is not proof of a clean exit. syncPendingStoppedEvents hardcodes exitCode: 0 when the container is gone but the DO state still reads healthy (container.js:1596). That is a value the SDK invented, and it is the shape an unexplained death takes. An earlier revision of this change filtered on exitCode !== 0 to bound bucket growth; it would have discarded exactly the records this exists to produce.
  • Absence of a record is not proof the container survived. When getExitCodeFromError cannot parse the runtime's message it calls setStopped(), a status matching neither branch of syncPendingStoppedEvents, so callOnStop is never reached and nothing is written.

So the filter is whether we asked. workflow.ts destroys every run through an Effect.ensuring, and those teardowns are the volume; a destroy() override sets an intent that onStop consumes. Read-and-clear rather than set-once, because destroy() does not reach onStop inline — the alarm loop delivers it later — and a flag left set would swallow every subsequent stop that instance saw, including a genuine death after a destroy() that threw.

An idle-timeout stop is correctly recorded as unrequested and is not routine volume: onActivityExpired returns before stop() unless the container is still running (container.js:748), and finalize has normally already destroyed it. So it fires only where finalize was skipped — Worker eviction, deploy mid-run — which is worth seeing.

super.onStop() runs first, since callOnStop awaits the override before writing the DO's stopped state; the R2 put is bounded at 2s so a slow bucket cannot leave that state reading healthy while the container is gone. Failure degrades to a logged no-op — silent, it would be indistinguishable from a deploy where the record never worked.

Reading the data

Keys are addressable forward only. The sandbox name is previewSafeSandboxId(executionId), which truncates to head18-sha12-digest8 above 40 chars and is not invertible — a holder of an execution id can compute the prefix, the reverse does not work.

requested: true is reliable; false is best-effort. A DO evicted between destroy() and the alarm reports a teardown we asked for as one we did not. It fails toward recording more, never toward hiding a death.

observedAt is when the stop was seen, not when the container died — onStop arrives from the alarm loop, which can be a tick later than the event.

Operational

  • CONTAINER_STOP_RECORDS: "off" in vars stops the durable writes; logging is unaffected. It does not avoid a deploy — what it buys is a reviewed one-line config change instead of a code revert.
  • No wrangler migration. No class is added, renamed or deleted; this is behaviour-only on classes already registered at tags v0/v1/v3.
  • Nothing prunes container-stops/. Volume is bounded by the filter — a healthy run writes nothing — but that is a bound, not an expiry. The lifecycle command is in the module header; set it before leaving this on indefinitely.
  • Container path only. The substrate's own DO overrides onStop and records nothing, so this must be ported before SUBSTRATE_BACKEND flips to "on".

Key actions

  • 11 unit tests on the pure functions, including both halves of the one-shot intent
  • Mutation-checked: making takeRequested a plain read fails the test that pins it
  • pnpm typecheck clean, pnpm lint clean, pnpm test 174 files / 2255 passed / 1 skipped
  • Unverified until a real container dies: whether ctx.id.name is populated, and whether a kill reports 137 or a bare signal. Nothing in the code branches on either — it records what arrives, and the first stop settles both.

A run whose container dies reports `ExecFailed: exec failed (exit -1): internal
error` and nothing at all about the container. Every explanation offered for
that class has now been measured and ruled out on the consumer that motivated
it — peak memory 3.5 GiB of 11.9, disk 3.6 GB used of 12 free, and death times
(137s, 647s, 1284s) shorter than the successes (2128s, 2176s). Nothing is
scarce and no duration is safe, which leaves a question only the platform can
answer.

The platform does answer it, one layer below where anyone was looking.
`@cloudflare/containers` parses the runtime's own message — `runtime signalled
the container to exit: <n>`, or `container exited with unexpected exit code:
<n>` — into the `exitCode` it hands `onStop`. A container something killed
carries a signal there; one that ran to completion carries 0. Nothing in this
repo was reading it.

So the three DO classes override `onStop` and persist the number under
`container-stops/<sandbox>/<ts>.json`, logged as well as written.

`reason` is NOT the discriminator, despite its type. `StopParams.reason` is
declared `'exit' | 'runtime_signal'`, but `runtime_signal` appears nowhere in
`@cloudflare/containers@0.3.7` outside that declaration — both `callOnStop`
sites pass `'exit'`. Read `exitCode`.

The write is best-effort and logged: it runs while the container is going away,
so a failure must not replace a stop we can explain with one we cannot — but a
silent no-op would be indistinguishable from a deploy where the record never
worked.

The record function lives in its own module, free of the Sandbox SDK import, so
it is testable outside the workers pool.

UNVERIFIED until a real container dies: whether `ctx.id.name` is populated, and
whether a killed container reports 137 or the bare signal. The first stop
answers both; nothing here depends on which.

@flaredispatch-fractalboxdev flaredispatch-fractalboxdev 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.

AI code review — 💬 Comment

Risk tier: full · 0 critical · 1 warnings · 0 suggestions

Reviewers: security ⚠️ · performance 1 · code-quality ⚠️ · documentation ⚠️ · release-management ⚠️ · compliance ⚠️ · agents-md 0

1. ⚠️ Warning — Timeout does not cancel the underlying R2 upload

📍 apps/dispatcher/src/container-stop.ts:174-181

'Promise.race' stops waiting after 2 seconds but does not abort 'bucket.put'. When R2 is slow or unavailable, each stop can leave an in-flight upload running while subsequent stop callbacks start more uploads, so the timeout does not actually bound network, CPU, or memory consumption. Use an abortable request/signal if supported, or otherwise avoid spawning uncancellable uploads after the timeout.

📋 View full logs & reviewed diff ↗

@nplusonedev

Copy link
Copy Markdown
Contributor Author

Validated. The observation is correct and the suggested remedy is not available on this API.

Promise.race abandons the losing bucket.put without cancelling it — true. But R2PutOptions carries no signal (checked @cloudflare/workers-types@4.20260702.1: onlyIf, httpMetadata, customMetadata, checksums, storageClass, ssecKey — nothing abortable), so there is no way to cancel an in-flight R2 put from the binding.

What the timeout is for is preserved: it bounds how long onStop blocks the SDK's teardown and the DO's stopped-state write, and that bound holds. What it does not bound is the background lifetime of an abandoned put — which only occurs on stops nobody requested (the requested filter drops every routine teardown before the put), so the accumulation scenario needs R2 degraded and a burst of unexplained container deaths at once. If R2 ever grows an abort surface on put, wiring it here is a two-line change.

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.

1 participant