fix(shim): reduce critical sections in local sandbox

[austinvazquez]

localsandbox serialized every RPC behind one mutex, including two calls
with no deadline of their own: Stop's call into instance.Shutdown and
StartStream's guest handshake. A guest that stopped acking streams, or a
slow Shutdown, held that mutex for as long as the stall lasted, blocking
every other RPC against the sandbox. The same problem existed one layer
down in the libkrun vm.Instance: StartStream shared no lock with Shutdown
at all, and its handshake had no deadline, so a stalled guest could wedge
an individual exec for the VM's lifetime even once the sandbox-level fix
stopped it from blocking unrelated RPCs.

This change, across four commits:

• Releases localsandbox's s.mu before the blocking calls in Stop and
  StartStream, holding it only to read/mutate localsandbox's own state.
  A stopping flag makes concurrent Stop calls fail fast instead of
  racing each other, and makes Client/StartStream fail fast instead of
  racing a call against an in-flight Shutdown. Client also now rejects a
  nil client from the instance as unavailable, rather than silently
  returning (nil, nil).
• Has the libkrun vm.Instance track in-flight StartStream handshakes so
  Shutdown can close them, unblocking a handshake stalled on a guest
  that never acks instead of leaving it to hang for the life of the VM.
  A shuttingDown flag serializes concurrent Shutdown calls and lets
  Shutdown release its own lock before the unbounded VM teardown.
• Bounds the handshake's ack read relative to the stream ID's length,
  closing a guest-controlled unbounded-allocation path.
• Bounds the handshake itself with a deadline (min of an internal
  timeout and the caller's context deadline), and separately watches
  for context cancellation without a deadline (e.g. a disconnected
  ttrpc request), so a wedged guest or a canceled caller can no longer
  hold a stream handshake open indefinitely.

Tested with go build ./..., go vet ./..., gofmt -l, and go test
on both changed packages; each fix has a dedicated regression test
verified against a temporarily reverted version of the fix.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR reduces time spent holding the localsandbox mutex during Stop and StartStream to prevent unrelated calls (notably Client()) from blocking behind long-running guest operations.

Changes:

• Move Shutdown() and StartStream() calls outside of the localsandbox critical section by copying s.instance under lock.
• Add concurrency-focused unit tests to ensure Client() is not blocked by in-flight StartStream() handshakes or Stop() shutdowns.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
internal/shim/sandbox/vm/vm.go	Shrinks mutex-held regions in Stop and StartStream by calling into the VM instance outside the lock.
internal/shim/sandbox/vm/vm_test.go	Adds tests that simulate blocking VM calls and verify Client() remains responsive; checks Client() fails after Stop().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new tests can leak/block goroutines on failure paths (before inst.release is closed), which can hang the overall test run and should be hardened with cleanup unblocking.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

internal/shim/sandbox/vm/vm_test.go:100

• If this test fails before reaching close(inst.release), the Stop goroutine will remain blocked, potentially hanging the test suite. Add a t.Cleanup that closes inst.release if it isn’t already closed.

	s := &localsandbox{instance: inst}

internal/shim/sandbox/vm/vm_test.go:146

• This test can hang the suite on early failure because the first Stop goroutine blocks on inst.release until it’s closed. Add a cleanup close for inst.release so t.Fatal paths still unblock the goroutine.

	s := &localsandbox{instance: inst}

internal/shim/sandbox/vm/vm_test.go:187

• If any assertion fails before close(inst.release), the Stop goroutine will remain blocked in Shutdown and may hang the overall test run. Add a t.Cleanup to close inst.release if needed.

	s := &localsandbox{instance: inst}

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Stop can leave an instance usable after Shutdown returns an error, contradicting the vm.Instance lifecycle contract and risking undefined behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[Copilot]

🟡 Changes recommended

The shutdown state transition is not atomic, and oversized stream IDs can be rejected after guest acceptance.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Shutdown can hold v.mu during teardown while StartStream waits to untrack its connection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Shutdown still holds the VM mutex during slow teardown, potentially blocking StartStream indefinitely.

Review details

Suppressed comments (1)

internal/vm/libkrun/instance.go:538

• Shutdown still holds v.mu while this closes the tracked connections and performs the rest of teardown. A handshake that is unblocked here returns from completeStreamHandshake and then blocks in untrackStreamConn waiting for the same mutex, so StartStream can remain stuck until vmc.Shutdown/dlClose finish (or indefinitely if VM teardown hangs). Detach/clear the in-flight set under the lock and release the lock before the slow teardown, while preserving a shutdown state for the post-handshake check.

	// Run before the VM teardown below, so an in-flight handshake unblocks
	// with an error instead of racing that teardown.
	v.closeStreamConnsLocked()

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved VM teardown and stream/lifecycle synchronization issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/vm/libkrun/instance.go:608

• The v.handler = 0 immediately above this return is now written without v.mu, while a concurrent Shutdown can read it under beginShutdown. This data race was introduced by moving the lock out of Shutdown and can make lifecycle state nondeterministic under -race; assign the zero value while holding the same mutex.

	return nil

internal/vm/libkrun/instance.go:428

• This only derives a deadline from ctx.Deadline(). If the caller cancels a context that has no deadline while the guest is waiting to ack, completeStreamHandshake never observes that cancellation and remains blocked for the full vmStartTimeout; interrupt the tracked connection on ctx.Done() as well.

			deadline := time.Now().Add(vmStartTimeout)
			if dl, ok := ctx.Deadline(); ok && dl.Before(deadline) {
				deadline = dl
			}

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Shutdown coordination, cancellation handling, retry wake-up, and fail-fast test coverage need improvement.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

internal/shim/sandbox/vm/vm.go:195

• activeInstance only protects the lookup; after it returns, Stop can set stopping and finish instance.Shutdown while this StartStream call is still returning. In the libkrun implementation the handshake connection is untracked immediately before this return, so that interleaving can yield (conn, nil) for a VM that has already been torn down. Add an in-flight operation lease (or otherwise wait for StartStream to return) before allowing Stop to call Shutdown.

	return instance.StartStream(ctx, streamID)

internal/vm/libkrun/instance.go:417

• Shutdown is only observed here after os.Stat succeeds and a new socket connection is dialed. If an in-flight StartStream is still in the retry loop when streaming.sock disappears during teardown, it never reaches trackStreamConn and waits through the 10–990 ms sleeps (about 49.5 seconds) before returning, so Shutdown does not unblock it. Check the shutdown state while retrying and provide a wake-up path instead of checking only after dialing.

			if !v.trackStreamConn(conn) {
				// Shutdown has already closed every tracked stream
				// connection and is tearing down (or has torn down) the
				// VM; don't hand back a connection racing that teardown.
				conn.Close()
				return nil, errdefs.ErrUnavailable.WithMessage("vm instance is shutting down")

internal/vm/libkrun/instance_test.go:131

• This assertion does not verify the documented “fail fast” behavior: an implementation that reaches the guest handshake and blocks could still return a context-deadline error within 5 seconds and pass. Measure the call duration (or synchronize on the server to prove no handshake was attempted) and assert it returns well before the test deadline.

	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
	defer cancel()
	if _, err := v.StartStream(ctx, "test-stream"); err == nil {
		t.Fatal("expected StartStream to fail fast once inFlightHandshakes is nil")
	}

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite