shim: serialize log-forward start/stop across Container clones

[lkml-likexu]

stop_log_forward and start_log_forward took an Arc and fell back to abort() when the handle was shared, skipping the IO drain pause/snapshot/destroy rely on. Share ownership through a mutexed slot and serialize start/stop with a semaphore so exactly one caller awaits clean task termination.

Co-developed-by: AlexSun alexsun@alexsun.top
Assisted-by: Claude Code:claude-opus-4-7

[cubesandboxbot]

⚠️ AI-generated review — prepared by an automated review agent; not a human approval.

Overview

This PR fixes a real bug: Container is #[derive(Clone)], and clone paths (delete_container clones the container out of the map before destroy_container; wait_container does the same) shared an Arc<tokio::task::JoinHandle<()>> for the init log-forward task. In the old code, stop_log_forward/start_log_forward used Arc::try_unwrap and fell back to handle.abort() when the Arc was shared — i.e. exactly on the clone paths — skipping the IO drain that pause/snapshot/destroy rely on. The fix replaces the per-clone Option<Arc<JoinHandle>> / Option<watch::Sender> with a single shared LogForwardHandle { slot: Arc<Mutex<LogForward>>, lifecycle: Arc<Semaphore> }, serializes start/stop with a 1-permit semaphore, and drains the task to completion through the shared slot. It also bounds the dedicated vsock connect with a 10s timeout.

Assessment: the fix is correct and well-constructed. The shared-slot + semaphore design is sound: drain() releases the slot mutex before the .await, store() is only reached after a drain, and the permit guarantees exactly one caller takes/awaits the task. The Rust unit tests are well-targeted and directly guard the invariant (clone-path drain, concurrent-stop serialization, stop-waits-for-in-flight-start). The e2e tests are honest about their scope.

Findings

1. [Medium — residual risk, explicitly documented] Unbounded drain() can stall the entire sandbox

drain() awaits the log task with no timeout. A task stuck in file.write_all — the one place the cancel signal cannot interrupt it — blocks while holding the lifecycle permit, and on the disconnect_agent/kill_container paths also the sandbox containers mutex; because the service layer serializes every RPC on the sandbox mutex, a single wedged write freezes all lifecycle operations on the sandbox indefinitely. The PR's doc comment acknowledges this, and on the delete_container clone path draining is a deliberate new blocking point (previously prompt abort()). This is an accepted trade-off (draining instead of aborting is the whole point of the fix), and local log writes rarely stall, but it is the one place the change trades liveness for correctness. See the inline comment for a concrete mitigation (make the write branch cancel-responsive in forward_init_log_*), which removes the hang rather than bounding it.

2. [Low] Connect timeout changes create-path failure semantics

start_container propagates start_log_forward errors with ? (CubeShim/shim/src/container/mod.rs:534), so a 10s connect timeout now fails container start, where the old unbounded connect would wait until the agent was reachable. The resume path tolerates it (set_client warns and continues at mod.rs:152), so the fatal-on-create / non-fatal-on-resume asymmetry is pre-existing — but the timeout makes the create failure a new, reachable outcome. Practically fine (a local vsock connect is normally instant), but worth confirming it's deliberate.

3. [Low — tests] requires_capability marker omits FILESYSTEM

test_repeated_pause_resume_keeps_commands_working uses read_file/write_file but only marks requires_capability(COMMANDS). Both current backends (cubesandbox, e2b) include FILESYSTEM in COMMON_CAPABILITIES, so there's no practical failure today, but the marker should list FILESYSTEM for precision.

4. [Nit] .expect() on semaphore acquire

acquire() uses .expect("log-forward lifecycle semaphore closed") on an owned permit. The semaphore is never closed in this codebase, so it's unreachable, but a panic there would abort the task rather than returning an error. Consider a CResult/fallback or a warnf!-and-return.

Tests

Rust unit tests (log_forward_tests) — good coverage:

• drain_completes_when_stopped_through_clone — the core invariant (a clone stops a task installed through the shared slot and the drain completes; the pre-fix abort() fallback would skip it).
• concurrent_stops_serialize_and_drain — exactly one caller drains; the other sees an empty slot.
• stop_waits_for_in_flight_start_then_drains — the semaphore serializes a stop arriving during a start's connect, so it drains the task the start installs.

The stateful watch channel makes the cancel race-free even if the task hasn't parked yet, so these tests are deterministic.

E2E tests — the docstrings explicitly acknowledge they do not observe the shim's init log-forward task (the SDK-level paths traverse envd's exec-output stream, not the vsock log forward) and would pass against pre-fix code. They're a reasonable black-box "repeated pause/resume stays healthy" smoke check, and the large-output test is correctly gated to the cubesandbox backend.

Minor

• let _ = handle.await swallows JoinError (task panic). Pre-existing behavior, but logging it would help diagnose forward-loop panics.

Conclusion

The core fix is correct, well-documented, and adequately tested. The main thing to weigh is the unbounded drain's liveness risk (finding 1); everything else is minor. No blockers.

[ls-ggg]

Great PR! I'll pick this up later。