Skip to content

fix(cloud-agent-next): recover leaked sandbox wrappers safely - #3555

Merged
eshurakov merged 1 commit into
mainfrom
polite-suggestion
Jun 1, 2026
Merged

fix(cloud-agent-next): recover leaked sandbox wrappers safely#3555
eshurakov merged 1 commit into
mainfrom
polite-suggestion

Conversation

@eshurakov

@eshurakov eshurakov commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Prevent shared Cloudflare sandbox disk exhaustion from leaked wrapper processes by durably leasing physical wrappers, fencing launch/reuse/deletion on verified cleanup, and retrying teardown with persisted backoff.
  • Introduce the Cloudflare-only AgentSandbox seam as the provider boundary for physical discovery, stop, keep-alive, and capacity operations; runtime, supervisor, deletion, interrupt, terminal, and router flows now converge on that lifecycle rather than performing ad-hoc sandbox teardown.
  • Make cold workspace admission and rolling deployment handling fail-safe: low capacity triggers conservative stale cleanup plus recheck, the new capacity-admission recovery gate is restricted to explicit unusable-filesystem evidence, and legacy wrappers are identified through observable environment markers. Pre-existing recovery destruction for preparation infrastructure failures remains unchanged.

Verification

  • Automated smoke test
  • Manual cloud agents test in a local browser

@kilo-code-bot

kilo-code-bot Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: 1 Issue Remaining (carried forward, intentional by design) | Recommendation: Merge

Executive Summary

This PR introduces a durable physical-wrapper lease FSM to fence sandbox process ownership, routes all stop/discovery through a new AgentSandbox provider seam, and gates session deletion on verified wrapper absence. The architecture is sound, all prior review findings are resolved, and the one carried-forward WARNING remains intentional by design.

Overview

Severity Count
WARNING 1 (carried forward, acknowledged)
Issue Details (click to expand)

RESOLVED (from prior rounds)

File Issue
src/session/agent-runtime.ts delivery_accepted lease write now swallowed on error; maintenance reconciles via reconcilePhysicalCleanup
src/agent-sandbox/cloudflare/cloudflare-agent-sandbox.ts Warm workspace onProgress callback now passed before bootstrap

WARNING (carried forward — author acknowledged as intentional)

File Line Issue
src/persistence/CloudAgentSession.ts ~1407 !metadata + non-none lease blocks deletion without driving cleanup

WARNING — finalizeSessionDeletion: no-metadata + pending lease reschedules alarm but cannot advance cleanup

When the metadata key is missing but a wrapper_lease row persists in a non-none state, the alarm is rescheduled every second but runMaintenance is never called so the lease cannot transition toward none. Explicit deleteSession calls continue to throw 'Session deletion pending physical wrapper cleanup' until the alarm path independently clears the lease.

The PR author has acknowledged this as intentional fail-closed behavior: without metadata the DO cannot reconstruct the provider sandbox to verify physical wrapper absence, so retaining the non-none lease is the safest course. The WARNING is carried forward for visibility but does not block merge.

Additional Observations (not blocking)

discoverSessionWrappers container inspection — no per-call timeout

In src/kilo/wrapper-manager.ts, the sandbox.exec calls for docker exec (both ps -eo pid=,args= and /proc/{pid}/environ) have no explicit timeout wrapper. The outer WRAPPER_STOP_ATTEMPT_TIMEOUT_MS (45 s) and the Cloudflare sandbox internal deadline act as backstops, but a slow docker exec could delay the entire stop attempt. This matches the pattern of inspectWrapperContainers and is consistent with the existing codebase; raising it as a future hardening opportunity only.

stopObservedWrappers removes docker kill fallback for legacy sessions

The new implementation always uses docker exec <id> pkill inside containers rather than docker kill <id> as a last resort. For old sessions that have a container but no devcontainer metadata, docker kill is no longer attempted. This is intentional per the PR description (session-marker pkill is more precise), but could leave a small set of pre-PR devcontainer sessions with containers that cannot be stopped via the new path if the wrapper process is not named with the expected marker.

WorkspaceCapacityInspectionUnavailableError and WorkspaceCapacityAdmissionRejectedError intentionally bypass sandbox destruction

These new error types propagate through withPreparationInfrastructureRecovery without destroying the sandbox — they are not classified in getPreparationInfrastructureFailure. This is correct by design: these errors indicate shared-sandbox capacity constraints (not filesystem corruption), so sandbox destruction is inappropriate. The SandboxCapacityInspectionError (ENOSPC) path correctly triggers destruction.

Files Reviewed (46 files)
  • src/agent-sandbox/cloudflare/cloudflare-agent-sandbox.ts — new provider seam; correct
  • src/agent-sandbox/cloudflare/cloudflare-agent-sandbox.test.ts — comprehensive coverage
  • src/agent-sandbox/factory.ts — correct
  • src/agent-sandbox/factory.test.ts — correct
  • src/agent-sandbox/protocol.ts — clean interface definitions
  • src/execution/orchestrator.ts — delegates to AgentSandbox; correct
  • src/execution/orchestrator.test.ts — updated
  • src/kilo/wrapper-client.ts — lease identity propagation + rolling-deploy compatibility; correct
  • src/kilo/wrapper-client.test.ts — updated
  • src/kilo/wrapper-manager.tsdiscoverSessionWrappers, stopObservedWrappers, inspectWrapperContainers; correct
  • src/kilo/wrapper-manager.test.ts — updated
  • src/persistence/CloudAgentSession.ts — 1 issue (acknowledged)
  • src/router/handlers/session-management.ts — correct
  • src/router/handlers/session-questions.ts — correct
  • src/sandbox-recovery.tsSandboxCapacityInspectionError recovery path added; correct
  • src/sandbox-recovery.test.ts — updated
  • src/session-service.ts — updated
  • src/session-service.test.ts — updated
  • src/session/agent-runtime.tsauthorizePhysicalWrapper + lease FSM integration; correct
  • src/session/agent-runtime.test.ts — updated
  • src/session/wrapper-runtime-state.tsWrapperLease FSM + reduceWrapperLease; correct
  • src/session/wrapper-runtime-state.test.ts — updated
  • src/session/wrapper-supervisor.tsreconcilePhysicalCleanup + requestPhysicalWrapperStop; correct
  • src/session/wrapper-supervisor.test.ts — updated
  • src/terminal/access.ts — delegates to AgentSandbox; correct
  • src/terminal/access.test.ts — updated
  • src/workspace-errors.ts — new error types; correct
  • src/workspace.ts — admission gate + cleanupStaleWorkspaces returns; correct
  • src/workspace.test.ts — updated
  • wrapper/src/main.tswrapperInstanceId/wrapperInstanceGeneration args; correct
  • wrapper/src/server.ts — health endpoint reports identity; correct
  • wrapper/src/server.test.ts — updated
  • test/e2e/lifecycle.ts — updated
  • test/integration/session/callback-outbox.test.ts — updated
  • test/integration/session/deletion-lifecycle.test.ts — updated
  • test/integration/session/disconnect-and-reaper.test.ts — updated
  • test/integration/session/execute-directly-failure.test.ts — updated
  • test/integration/session/execution-id-removal.test.ts — updated
  • test/integration/session/hot-delivery.test.ts — updated
  • test/integration/session/idle-reconciliation.test.ts — updated
  • test/integration/session/message-terminalization.test.ts — updated
  • test/integration/session/pending-messages.test.ts — updated
  • test/unit/execution/orchestrator.test.ts — updated
  • src/router.test.ts — updated
  • src/server.test.ts — updated

Fix these issues in Kilo Cloud


Reviewed by claude-sonnet-4.6 · 8,949,567 tokens

Review guidance: REVIEW.md from base branch main

@eshurakov

Copy link
Copy Markdown
Contributor Author

Addressed the actionable warm-bootstrap progress finding in 078978f90: ordinary bootstrap now reports the kilo_server startup step on both warm and cold workspaces, with regression coverage for warm delivery.

I did not change the missing-metadata deletion branch. The public delete route short-circuits when session metadata is absent, and the Durable Object cannot reconstruct the provider sandbox or verify physical wrapper absence without that metadata. Retaining a non-none lease in that corrupted/orphaned state is intentionally fail-closed rather than erasing the only durable cleanup signal.

@eshurakov
eshurakov force-pushed the polite-suggestion branch from e2e56e6 to 8e63f2d Compare June 1, 2026 07:13
Fence physical wrapper ownership, verify cleanup before reuse or deletion, and reject unsafe capacity recovery paths so shared sandboxes do not retain leaked wrapper processes.
@eshurakov
eshurakov force-pushed the polite-suggestion branch from 8e63f2d to a6fe757 Compare June 1, 2026 08:22
@eshurakov
eshurakov merged commit 8a69c9b into main Jun 1, 2026
13 checks passed
@eshurakov
eshurakov deleted the polite-suggestion branch June 1, 2026 09:26
St0rmz1 added a commit that referenced this pull request Jul 30, 2026
* fix(cloud-agent-next): skip container wake on idle-timeout stops

  The idle sweep requests a physical wrapper stop only after confirming via
  DO state that no wrapper runtime or pending work remains. stopWrappers then
  inspected the container to re-derive that, and listProcesses is a container
  fetch, so a sleeping container was cold-booted to look for processes that
  activity expiry had already SIGTERMd. It then idled to the 900s sleepAfter.
  In 90 minutes of production logs this path was 233 inspections, all absent,
  none present.

  Confirm absence from container state instead when the container is stopped.
  A wrapper is a process and cannot outlive its container, so a stopped
  container cannot hide a leaked wrapper. Scoped to idle-timeout: every other
  stop reason still inspects, preserving the leaked wrapper recovery added in
  #3555.

* fix minor observability-duplication
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