Shutdown/rollback/output-cap hardening for api, worker, and deploy manifests#97
Conversation
Kubernetes sends SIGTERM (not SIGINT) to stop a pod, so the api's shutdown_signal() -- which only awaited tokio::signal::ctrl_c() -- never fired under the shipped Deployment; axum's graceful shutdown was dead code in production. It now races SIGTERM against ctrl_c() on Unix (falling back to ctrl_c() alone elsewhere). The worker had no signal handling at all: a SIGTERM would hard-kill it mid-lease. It now stops claiming new leases once a shutdown signal arrives and lets any already-claimed lease run to completion, bounded by a new --drain-timeout-secs (default 300s) so a stuck job can't hang the process forever; the lease itself is left to expire and be reclaimed if that timeout fires. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
KubernetesApplyProvider::provision/fork applied PVC/Pod/NetworkPolicy/ Service manifests and then bailed out if the subsequent kubectl apply itself reported failure (it's not atomic across a multi-document apply) or wait_for_pod_ready timed out, leaking every already-applied resource. Both paths now best-effort tear down everything labeled with the sandbox/child-sandbox id via the existing teardown_args()/kubectl delete path before propagating the original error; the rollback's own failure is logged but never masks the original error. Covered by new unit tests using a fake kubectl shell script that fakes apply/wait/delete outcomes and asserts on the invocation log (no real cluster needed). Also cap the stdout/stderr captured from every kubectl invocation (configurable via --max-captured-output-bytes / provider builder, default 2MiB, mirroring sandboxwich-agent's DEFAULT_MAX_CAPTURED_OUTPUT_BYTES) so a chatty or misbehaving kubectl command can't grow WorkerJobResult / complete request payloads unboundedly. Truncated output gets a "[truncated N bytes]" marker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
api.yaml and worker.yaml now get the same pod/container hardening the rendered sandbox pods already had: runAsNonRoot, a fixed non-root uid/gid (matching the image's USER 65532:65532), seccompProfile RuntimeDefault, allowPrivilegeEscalation: false, capabilities drop ALL, and readOnlyRootFilesystem: true with an emptyDir mounted at /tmp (neither binary writes to disk in the deployed configuration; the worker's kubectl additionally gets HOME=/tmp for its now-unwritable default cache dir). terminationGracePeriodSeconds is also set explicitly here (30s for the api, 330s for the worker -- comfortably above its 300s --drain-timeout-secs default, which is now passed explicitly in the worker's args) since both land in the same pod-spec hunks as the security context above. Dockerfile: pin the builder/runtime base images by digest (refresh via `docker buildx imagetools inspect <image:tag>`) instead of a floating tag, and add a HEALTHCHECK that curls the api's /healthz -- it no-ops for the worker image (same Dockerfile builds both binaries, and the worker has no HTTP listener to probe). curl is kept in the final image rather than purged so the HEALTHCHECK has something to run; this is a deliberate size/attack-surface trade-off for a working health signal. The :latest image tags in the k8s manifests are left as-is (no release process to pin to a specific tag yet) but now carry a clearer TODO. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UqHrivhBUfLn5fmJv3XeyK
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let outcome = tokio::select! { | ||
| result = handle_future => Some(result), | ||
| _ = drain_watchdog(shutdown.clone(), drain_timeout) => None, | ||
| }; |
There was a problem hiding this comment.
📝 Info: Drain watchdog cancellation leaves spawned tasks orphaned until runtime shutdown
When drain_watchdog wins the tokio::select! at crates/sandboxwich-worker/src/main.rs:1029-1032, the handle_future is dropped mid-await. Inside handle_lease (crates/sandboxwich-worker/src/main.rs:1064-1118), two tasks are spawned: a renew_task via tokio::spawn (line 1083) and a spawn_blocking task (line 1109). When the future is dropped while awaiting spawn_blocking, the renew_task.abort() at line 1117 never executes, so the renew task continues running indefinitely. The spawn_blocking task also runs to completion (blocking threads can't be cancelled). In practice this is benign because the work loop breaks immediately after, main() returns, and the tokio runtime shutdown cancels all spawned tasks. However, if the runtime shutdown itself is slow (e.g. other tasks are also draining), the renew task could fire additional renewal API calls for a lease the worker has already abandoned. Worth documenting this as a known limitation if drain semantics become more complex.
Was this helpful? React with 👍 or 👎 to provide feedback.
| tokio::select! { | ||
| _ = sigterm.recv() => {} | ||
| _ = tokio::signal::ctrl_c() => {} | ||
| } |
There was a problem hiding this comment.
📝 Info: Worker's shutdown signal handler silently discards ctrl_c errors unlike the API's version
The worker's wait_for_shutdown_signal at line 886 uses _ = tokio::signal::ctrl_c() which silently discards any error from ctrl_c(). The API's equivalent (crates/sandboxwich-api/src/main.rs:1299-1304) explicitly checks the result and logs a warning via tracing::warn!. While the behavioral outcome is identical (both return on error, triggering shutdown), the inconsistency means a ctrl_c installation failure in the worker would be invisible in logs. Consider aligning the two implementations for debuggability.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn cap_output_bytes(bytes: &[u8], max_bytes: u64) -> String { | ||
| let max_bytes = usize::try_from(max_bytes).unwrap_or(usize::MAX); | ||
| if bytes.len() <= max_bytes { | ||
| return String::from_utf8_lossy(bytes).into_owned(); | ||
| } | ||
| let omitted = bytes.len() - max_bytes; | ||
| let mut text = String::from_utf8_lossy(&bytes[..max_bytes]).into_owned(); | ||
| text.push_str(&format!("\n[truncated {omitted} bytes]\n")); | ||
| text | ||
| } |
There was a problem hiding this comment.
📝 Info: Output byte cap slices at raw byte boundaries, relying on from_utf8_lossy for safety
The cap_output_bytes function at crates/sandboxwich-worker/src/provider.rs:1405-1414 slices &bytes[..max_bytes] which can split a multi-byte UTF-8 sequence mid-character. This is safe because String::from_utf8_lossy replaces any trailing incomplete sequence with the U+FFFD replacement character. However, the truncation marker reports omitted = bytes.len() - max_bytes (raw byte count), while the actual string produced from the first max_bytes bytes may be slightly longer or shorter than max_bytes characters due to lossy conversion. The marker is accurate in terms of raw bytes dropped, which is the right metric for a byte-budget cap, so this is correct but worth understanding.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # Must exceed --drain-timeout-secs below so Kubernetes doesn't SIGKILL | ||
| # the worker mid-drain: on SIGTERM/SIGINT the worker stops claiming new | ||
| # leases and waits for any in-flight lease to finish, bounded by that | ||
| # timeout, before exiting on its own. | ||
| terminationGracePeriodSeconds: 330 |
There was a problem hiding this comment.
📝 Info: terminationGracePeriodSeconds must stay aligned with drain-timeout-secs across two YAML files
The worker deployment sets terminationGracePeriodSeconds: 330 (deploy/kubernetes/worker.yaml:101) and --drain-timeout-secs 300 (line 222). The 30-second margin ensures Kubernetes won't SIGKILL the worker before its own drain logic completes. This invariant (grace period > drain timeout) is critical for correct graceful shutdown but is enforced only by convention across two separate values in the same YAML file. If either value is changed independently in a future PR, the worker could be force-killed mid-drain. Consider adding a comment cross-referencing the two values, or validating the relationship at startup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn rollback_applied_resources(&self, sandbox_id: SandboxId, context: &'static str) { | ||
| let args = self.teardown_args(sandbox_id); | ||
| match run_kubectl_command( | ||
| &self.kubectl, | ||
| &args, | ||
| "rollback applied resources after failed provision/fork", | ||
| self.max_captured_output_bytes, | ||
| ) { | ||
| Ok(output) if output.success => { | ||
| eprintln!( | ||
| "sandboxwich-worker: rolled back resources for sandbox {sandbox_id} after \ | ||
| failed {context}" | ||
| ); | ||
| } | ||
| Ok(output) => { | ||
| eprintln!( | ||
| "warning: rollback of sandbox {sandbox_id} resources after failed {context} \ | ||
| itself failed with {}: {} (resources may be leaked; original error is not \ | ||
| masked by this)", | ||
| output.status, output.stderr | ||
| ); | ||
| } | ||
| Err(error) => { | ||
| eprintln!( | ||
| "warning: rollback of sandbox {sandbox_id} resources after failed {context} \ | ||
| could not run kubectl: {error:#} (resources may be leaked; original error is \ | ||
| not masked by this)" | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Rollback runs synchronously inside blocking-thread provider methods, which is correct but worth noting
The rollback_applied_resources method (crates/sandboxwich-worker/src/provider.rs:1329-1359) calls run_kubectl_command synchronously. This is invoked from provision and fork which are SandboxProvider trait methods called via spawn_blocking in handle_lease (crates/sandboxwich-worker/src/main.rs:1109). Since the entire provider call chain runs on a blocking thread, the synchronous kubectl rollback is appropriate and won't block the async runtime. The rollback correctly swallows its own errors to avoid masking the original provisioning error.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Four operational fixes for the control plane's shutdown/failure/output paths:
sandboxwich-api'sshutdown_signal()only awaitedctrl_c(), so axum's graceful shutdown never fired under a real Kubernetes Deployment (Kubernetes sends SIGTERM, not SIGINT). It now races SIGTERM againstctrl_c()on Unix.sandboxwich-workerhad no signal handling at all -- it now stops claiming new leases on SIGTERM/SIGINT and lets any in-flight lease finish, bounded by a new--drain-timeout-secs(default 300s), before exiting.KubernetesApplyProvider::provision/forkapplied PVC/Pod/NetworkPolicy/Service manifests and bailed on a failed apply or await_for_pod_readytimeout, leaking every already-applied resource. Both paths now best-effort tear down everything labeled with the sandbox id via the existingteardown_args()/kubectl deletepath before propagating the original error (rollback failures are logged, never masked as the original error). Covered by new unit tests using a fakekubectlshell script.--max-captured-output-bytes, default 2 MiB, mirroringsandboxwich-agent's existing cap) instead of flowing intoWorkerJobResult/complete-request payloads unbounded. Truncated output gets a[truncated N bytes]marker.api.yaml/worker.yamlcontrol-plane pods now get the same security context the rendered sandbox pods already had (runAsNonRoot, fixed uid/gid matching the image'sUSER 65532:65532,seccompProfile: RuntimeDefault,allowPrivilegeEscalation: false,capabilities: drop: [ALL],readOnlyRootFilesystem: true+ a/tmpemptyDir), plus explicitterminationGracePeriodSeconds(30s api, 330s worker, above its 300s drain timeout). The Dockerfile now pins both base images by digest and adds aHEALTHCHECKagainst/healthz(a no-op for the worker image, which has no HTTP listener).Deviations / notes
terminationGracePeriodSecondsand the new security context land in the same Deployment pod-spec block; the rollback and output-cap changes touch the sameprovision/forkcall sites). Each commit message says exactly what it covers.docker buildcouldn't be exercised end-to-end (no running daemon in this environment); base image digests were resolved viadocker buildx imagetools inspect(registry-only, no daemon needed) and the manifests were validated offline withkubeconformandhadolint.:latestimage tags in the k8s manifests as-is per the existing TODO (no release process to pin a specific tag yet) but the Dockerfile is now digest-pinned so the build is reproducible even though the deployed tag still floats.HEALTHCHECKhas something to run; documented as a deliberate size/attack-surface trade-off.Test plan
cargo fmt --all -- --checkcargo clippy --workspace --all-targets(no warnings)cargo test --workspace(84 tests passing, including 5 new rollback/output-cap unit tests using a fake kubectl script)kubeconform -stricton both k8s manifests (9/9 resources valid)hadolint Dockerfile(only a pre-existing, unrelated warning)🤖 Generated with Claude Code