Skip to content

fix(server): harden sandbox lifecycle against restarts, deletion, and provision failures - #51

Merged
awtprod merged 10 commits into
mainfrom
impl/sandbox-hardening
Aug 22, 2026
Merged

fix(server): harden sandbox lifecycle against restarts, deletion, and provision failures#51
awtprod merged 10 commits into
mainfrom
impl/sandbox-hardening

Conversation

@awtprod

@awtprod awtprod commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Six sandbox hardening fixes from the audit (P0 4-6, P1 8-10). Stacked on #50 (impl/sandbox-stop-wedge).

Problems, in plain language

  1. Mixed docker/podman threads broke each other's previews. The desktop gateway held one global preview-proxy reference; each per-runtime manager overwrote it on provision. Since a proxy only knows its own runtime's containers, provisioning a podman thread instantly broke every docker thread's preview, CDP automation, and WebRTC signaling. Now keyed per thread, cleared by removeThread.

  2. A server restart stranded every live container. Reconcile only adopted threads with an in-memory record, and restarts empty those records -- so every surviving container was reported missing, its thread marked failed, while the container kept running (it was still "expected", so orphan removal skipped it). Reconcile now verifies restart-discovered containers against the thread's label signature (same verification the export/teardown adoption path uses) and adopts the verified ones for accounting; an expected container that fails verification is stopped instead of left running unaccounted. Adoption is never cached: exec stays fail-closed.

  3. Deleting a thread left its sandbox running and its transcripts on disk. The deletion reactor stopped the provider session and terminals but never dispatched sandbox.stop, and nothing deleted the exported branch bundle / provider conversation store. It now dispatches sandbox.stop for stoppable lifecycles and best-effort deletes the thread's artifacts (logged with cause on failure). The lifecycle reactor's stop path learned to find deleted threads (the detail query hides them) and skips the branch export for them -- transcripts must not outlive the thread.

  4. Sidecars ran without resource limits. Egress, credential, and preview proxies plus service containers only had a pids ceiling. All now get conservative --memory/--cpus (256m/0.5 for proxies, 1g/1 for services), --memory-swap pinned to --memory (docker otherwise defaults swap to 2x memory -- also applied to the workspace container), and the egress sidecar gets a tmpfs /tmp (it is --read-only and had no writable scratch space).

  5. A provision failing at desktop start leaked gateway state. Preview routes, service status, and credential grants (token values readable via the read-scoped status endpoint) were registered before desktop.start but never unwound on failure -- live routes and grants pointed at destroyed containers. Every unwind now clears the thread's gateway state first.

  6. A sync throw in the provision generator skipped all unwinds. The "generated service credential ... is missing" check threw a bare Error from inside the Effect generator -- a defect that escapes tapError, leaking everything provisioned so far. Now a typed SandboxManagerError with its own unwind.

Follow-up (out of scope here)

The GET /api/thread-desktop/<id>/status endpoint returns service credential grant token values under a read scope. Fix 5 stops them from outliving a failed provision, but the payload/scope of that endpoint deserves its own review.

Verification

  • vp check exit 0, vpr typecheck exit 0
  • Full server suite: 3275 passed, 19 skipped (330 files, +2 skipped)
  • New tests: per-thread proxy resolution across runtimes, label-verified reconcile adoption + fail-closed unverified stop, deletion-triggered stop + artifact removal (real temp dir), gateway-state unwind at desktop failure, typed grant failure with executor-log teardown assertions, run-arg assertions for every new limit flag

Follow-up: three defects found by actually running it

The fixes above were written against unit tests. Building an end-to-end canary
that drives real containers surfaced three more defects that no existing test
could have caught -- every sandbox test either stubbed the lifecycle reactors or
faked the runtime manager, so the path that actually provisions had no
coverage at all
.

  1. Sandboxes could never work on a rootless-podman host. The executor spawned
    every container command with env: { PATH: process.env.PATH }. Stripping the
    host environment is the point -- it keeps provider credentials out of
    container commands -- but it also strips XDG_RUNTIME_DIR, which is how
    rootless podman finds its socket and libpod runtime root. Every command died
    at podman info, so provisioning failed in #validateRootless and the
    sandbox went straight to failed. Measured directly: PATH-only fails,
    PATH+XDG_RUNTIME_DIR succeeds. The deployment had been working around it
    with a PATH wrapper that re-injects CONTAINER_HOST
    (deploy/openclaw/sandbox/podman-wrapper.sh, documented as constraint feat: build the Command Center agentic OS foundation #1 of
    the host design). The allowlist stays; the few entries that name the runtime
    rather than carry a secret now pass through.

  2. The Provision button silently did nothing on any thread that had run once.
    sandbox.provision has two paths: one drives the sandbox to provisioning
    and emits sandbox.provisioning-started (read only by the projector --
    correct for a caller that provisions inline immediately after), the other
    emits sandbox.provision-requested, which is what SandboxLifecycleReactor
    listens for. The decider chose between them by whether a branch was
    resolvable -- but the UI sends { threadId } with no branch, and a thread
    that has run once keeps sandbox.branch across teardown. So every client
    re-provision skipped the request event and parked the thread in
    provisioning with no container, no reactor running, and no error. Branch
    presence cannot carry that meaning (ProviderCommandReactor provisions
    inline and omits the branch when re-provisioning), so the claim is now an
    explicit server-only provisionsInline, absent from
    ClientSandboxProvisionCommand -- the same shape as
    ThreadSessionStopCommand.canceledThroughSequence.

  3. Container failures threw away their own diagnosis.
    SandboxRuntimeManager.attempt rebuilt every backend failure from
    cause.message, and SandboxRuntimeError keeps the runtime's stderr in a
    separate field -- so a thread's recorded failure, the lifecycle warning, and
    the UI all showed a bare "podman network failed". This is what made the two
    above take so long to find: the canary looked like a 67% flake for five runs
    until the real text ("network name t3-net- already used") named the
    cause instantly.

The canary

apps/server/integration/sandboxCanary.integration.ts, plus a
realSandboxReactors option on the integration harness that swaps the stubs for
SandboxLifecycleReactorLive, SandboxSettleCleanupReactorLive,
ThreadDeletionReactorLive, and SandboxRuntimeManagerLive. Off by default, so
decider and projection tests stay hermetic and never touch a container.

It drives provision -> settle -> re-provision -> delete and asserts against the
host, with the provider adapter still faked (none of this needs a provider
credential):

Phase 1  PASS  sandbox reached 'ready'; workspace container running on the host
Phase 2  PASS  settled to 'stopped' -- never wedged in 'stopping'   <- #47/#50
         PASS  containers reclaimed on settle
         PASS  export artifacts written on teardown
Phase 3  PASS  re-provisioned from the export                        <- #47
Phase 4  PASS  containers torn down on thread deletion               <- this PR
         PASS  transcript artifacts removed on deletion              <- #52

Three consecutive runs, 10/10 each, exit 0. Run it with:

T3_SANDBOX_RUNTIME=podman T3_SANDBOX_DESKTOP=disabled \
T3_SANDBOX_CONTAINER_STORAGE_QUOTA=disabled \
node apps/server/integration/sandboxCanary.integration.ts

Notes for whoever runs it: the host needs digest-pinned T3_SANDBOX_IMAGE and
T3_SANDBOX_PREVIEW_PROXY_IMAGE; T3_SANDBOX_RUNTIME defaults to docker,
which this host does not have; and podman here reports no project-quota support
for volumes despite the prjquota XFS graphroot, so volume quotas must be
disabled. T3_INTEGRATION_TMPDIR places the harness root outside a private
/tmp, which the runtime's mount namespace cannot see.

Verification (updated)

  • Canary: 3 consecutive runs, 10/10 checks, exit 0, against real containers
  • Full server suite: 3274 passed, 3 failed -- all three are CodexAdapter
    tests failing on "refuses linked Git metadata outside its managed worktree
    directory", reproduced identically on a worktree without any of these commits
  • vpr typecheck exit 0; new tests for the executor allowlist and the decider's
    client path are revert-checked (each fails with its fix removed)

🤖 Generated with Claude Code (Claude Fable 5)

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L labels Aug 21, 2026
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 13.6 KiB 13.6 KiB −12 B (−0.1%) 15.1 KiB
Codex Thread snapshot wire 6.9 KiB 6.9 KiB 0 B (0.0%) 7.3 KiB
Codex Live turn WebSocket wire 6.7 KiB 6.7 KiB −12 B (−0.2%) 7.8 KiB
Codex Live turn WebSocket decoded 54.2 KiB 54.2 KiB 0 B (0.0%) 66.4 KiB
Codex Live turn messages 17 17 0 (0.0%) 21
Claude Total thread wire 13.7 KiB 13.7 KiB −12 B (−0.1%) 15.1 KiB
Claude Thread snapshot wire 7.0 KiB 7.0 KiB −6 B (−0.1%) 7.3 KiB
Claude Live turn WebSocket wire 6.7 KiB 6.7 KiB −6 B (−0.1%) 7.8 KiB
Claude Live turn WebSocket decoded 55.0 KiB 55.0 KiB 0 B (0.0%) 66.4 KiB
Claude Live turn messages 17 17 0 (0.0%) 21

Baseline: ceb12db · PR result: c27f869 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 107.8 KiB
  • Claude decoded thread snapshot: 108.6 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@awtprod
awtprod force-pushed the impl/sandbox-hardening branch from 63b9d0c to 9458301 Compare August 22, 2026 01:24
@awtprod
awtprod force-pushed the impl/sandbox-stop-wedge branch from 0888a72 to 567eb73 Compare August 22, 2026 02:08
awtprod and others added 10 commits August 22, 2026 02:23
The workspace container gets --memory/--cpus/--pids-limit from per-thread
config, but every sidecar ran with only a pids ceiling: the credential
proxy, the preview bridge, and thread service containers could each
consume unbounded host memory and cpu. Add conservative constants --
256m/0.5 cpu for the proxies, 1g/1 cpu for services -- with --memory-swap
pinned to --memory so docker's 2x-swap default cannot double the ceiling.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he egress sidecar

Two backend fixes:

Restart adoption: reconcile only adopted threads whose in-memory record
survived, and a server restart empties those records -- so every container
that outlived the restart was reported missing, its thread marked failed,
while the container kept running (removeOrphans skips expected threads,
so nothing ever reclaimed it). Reconcile now accepts per-thread label
signatures (the same sha256-derived-name + stamped-label verification the
export/teardown adoption path uses): a container that proves its identity
is adopted for reconcile accounting, and one that cannot be verified is
stopped rather than left running unaccounted. Adoption is never cached --
exec stays fail-closed on records this generation provisioned.

Egress sidecar: gains the same --memory/--memory-swap/--cpus bounds as
its siblings, plus a tmpfs /tmp -- it runs --read-only and previously had
no writable scratch space at all. The workspace container's swap is also
pinned to its memory limit (docker defaults to 2x otherwise).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eads coexist

The desktop gateway held a single global previewProxy reference, and every
per-runtime SandboxRuntimeManager overwrote it on provision and recovery.
Each ThreadPreviewProxy tracks only its own runtime's containers, so
provisioning one podman thread replaced the proxy every docker thread's
preview, CDP automation, and WebRTC signaling resolved through -- breaking
all of them at once. Store proxies per thread, set at provision/recovery
and cleared by removeThread alongside the rest of the thread's gateway
state.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rant-issuing throw

Two provision-path leaks with the same shape:

Gateway state: setServiceStatus, setServiceCredentialGrants,
setPreviewProxy, and registerPreviewRoute all run before desktop.start,
but the error unwinds only stopped containers -- a provision that failed
at the desktop stage left live preview routes and credential grants
(token values readable via the read-scoped status endpoint) pointing at
destroyed containers. Every unwind now calls desktopGateway.removeThread
first, which clears routes, grants, service status, the automation
target, and the thread's preview-proxy entry.

Sync throw: the "generated service credential ... is missing" check threw
a bare Error from inside the Effect generator -- a defect that skips every
tapError unwind, leaking everything provisioned so far. It now fails
typed as SandboxManagerError with its own unwind (revoking any grants
already issued for earlier services).

Also adds SandboxRuntimeManager.removeThreadArtifacts, deleting a
thread's exported bundle/manifest/provider-store artifacts, for the
thread-deletion retention fix that follows.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… deleted

Thread deletion stopped the provider session and closed terminals but
never touched the sandbox: the container, network, and volumes kept
running forever (reconcile counts non-terminal sandboxes as expected, so
removeOrphans never reclaimed them), and the exported branch bundle and
provider conversation store -- the thread's transcripts -- stayed on disk
indefinitely.

The deletion reactor now dispatches sandbox.stop for any stoppable
lifecycle (terminal states and missing sandboxes dispatch nothing) and
best-effort deletes the thread's exported artifacts, logging failures
with their cause rather than failing the deletion flow.

The lifecycle reactor's stop path had to learn about deleted threads: the
thread-detail query filters deleted_at IS NULL, so a deletion-triggered
sandbox.stopping previously found "no thread" and completed without
tearing anything down. It now falls back to the full snapshot (which
retains deleted rows), stops the container, and skips the branch export
-- exporting would recreate exactly the artifacts deletion removes.

The lifecycle reactor also passes label-signature adoption hints to
reconcile, activating the backend's restart adoption for threads whose
project image can still be resolved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d its runtime

The sandbox executor spawned every docker/podman command with
`env: { PATH: process.env.PATH }`. That keeps host credentials out of container
commands, which is the point -- but it also strips `XDG_RUNTIME_DIR`, and
rootless podman needs it to locate the user socket and libpod's runtime root.
Without it every command failed at `podman info` ("set sticky bit on: chmod
/run/user/<uid>/libpod"), so provisioning died in `#validateRootless` and the
thread's sandbox went straight to `failed`. Sandboxes could not work at all on a
rootless-podman host.

No test caught it because they all fake the executor, and the deployment worked
around it with a PATH wrapper that re-injects `CONTAINER_HOST`
(deploy/openclaw/sandbox/podman-wrapper.sh).

Keeps the allowlist, and adds the handful of entries that name the runtime
rather than carry a secret.
…runtime

Every sandbox test so far either stubbed the lifecycle reactors or faked the
runtime manager, so the path that actually provisions -- reactor to manager to
`podman`/`docker` -- had no coverage at all. That is how an executor bug that
made rootless podman unusable survived a full suite run.

`makeOrchestrationIntegrationHarness({ realSandboxReactors: true })` swaps the
stubs for `SandboxLifecycleReactorLive`, `SandboxSettleCleanupReactorLive`,
`ThreadDeletionReactorLive`, and `SandboxRuntimeManagerLive`. Off by default, so
the decider and projection tests stay hermetic and never touch a container.

The canary drives provision -> settle -> re-provision -> delete and asserts
against the host: containers appear, settle reclaims them without wedging the
thread in `stopping`, teardown writes the export, and deleting the thread takes
the containers and its transcript artifacts with it. The provider adapter stays
faked -- none of this needs a provider credential.
…ed thread

`sandbox.provision` has two paths and they are not interchangeable. One drives
the sandbox straight to `provisioning` and emits `sandbox.provisioning-started`,
which only the projector consumes -- correct for a caller that provisions inline
immediately afterwards. The other emits `sandbox.provision-requested`, the event
`SandboxLifecycleReactor` listens for.

The decider chose between them by whether a branch was resolvable. But the UI
sends `{ threadId }` with no branch (ChatView.tsx), and a thread that has run
once keeps `sandbox.branch` across teardown -- so every re-provision from a
client resolved a branch, skipped the request event, and parked the thread in
`provisioning` with no container, no reactor running, and no error.

Branch presence cannot carry that meaning: `ProviderCommandReactor` provisions
inline and deliberately omits the branch when re-provisioning. So the claim is
explicit and server-only, like `ThreadSessionStopCommand.canceledThroughSequence`
-- `provisionsInline` is absent from `ClientSandboxProvisionCommand`, so a
client cannot assert it over the wire.

Found by the sandbox canary: re-provision after settle hung until the harness
timed out, with `sandbox.provisioning-started` and no `provision-requested`
before it in the event log.
…n the canary

Two things the first canary run needed and did not have.

Each provision takes two subnets from the runtime's small default pool, and a
run that fails partway leaves its networks behind -- about seven runs exhaust
the pool, and the next one fails at `network create` for reasons that have
nothing to do with the code under test. Teardown now reaps `t3-*` networks with
no attached container, so it cannot disturb a live sandbox.

And when provisioning failed, the canary printed only which lifecycle it landed
in. `SandboxRuntimeError` carries the runtime's stderr but nothing logs it, so
"podman network failed" was all anyone saw. It now prints the recorded failure
stage, code, and message.
…essages

`SandboxRuntimeManager.attempt` rebuilt every backend failure from
`cause.message` alone, and `SandboxRuntimeError` keeps the runtime's stderr in a
separate field. So the reason a command failed was discarded at the first
boundary it crossed: a thread's recorded failure, the lifecycle warning, and the
UI all showed a bare "podman network failed".

That is the whole diagnosis, thrown away. The canary spent five runs looking
like a 67% flake until this change surfaced the actual text -- "network name
t3-net-<hash> already used: network already exists" -- which named the cause
immediately.

Also carries stderr through `failureMessage` in the lifecycle reactor, for
failures that reach it as a plain Error.
@awtprod
awtprod force-pushed the impl/sandbox-hardening branch from 9458301 to c27f869 Compare August 22, 2026 02:24
@awtprod
awtprod changed the base branch from impl/sandbox-stop-wedge to main August 22, 2026 02:24
@awtprod
awtprod merged commit 520c72d into main Aug 22, 2026
19 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant