[WIP] fix(minimal2): serialize concurrent attach establishment (#588) by norrietaylor · Pull Request #590 · gominimal/minimal

norrietaylor · 2026-06-28T22:09:20Z

Mitigation for #588: a second overlapping minimal2 attach to an own-ip session wedges (no shell).

Stacked on #581 (base branch feat/networking-host-exposure) because it depends on that PR's own-ip attach path.

Root cause

Two own-ip session SSH handshakes racing through libkrun's host→guest virtio-vsock muxer leave one session wedged — its RequestShell is never delivered to session_host, so the second concurrent attach hangs. Verified the defect is in libkrun's vsock device under concurrent connections (not tokio-vsock, which is correct; not minimald logic). libkrun 1.19.0 fixed only the sequential direct-vsock case. Full investigation in #588.

Fix (in-repo workaround, pending the upstream libkrun fix)

minimal2 attach spawns a short-lived detached attach-lock helper that holds an exclusive flock for a bounded establishment window (default 4000ms, MINIMAL_ATTACH_ESTABLISH_WINDOW_MS) before exec-ing ssh:

A concurrent attach blocks on the same lock and starts its SSH handshake ~one window later, so the racy handshake/RequestShell phase is serialized instead of overlapping the muxer.
Single attaches are uncontended — the parent only waits for the lock-acquired signal, not the full window — so steady-state latency is unchanged.
exec ssh is preserved (clean interactive TTY); the helper holds the lock in the background and releases on window expiry.

Verification (macOS/HVF, DM1)

Two concurrent own-ip attaches both reach a shell with working egress: 3/3 rounds (HTTP 200 each), 20/20 establish-only. Daemon boot.log confirms both get RequestShell + attached OwnIp PTask, with session B attaching ~4s after A (the flock window).
Pre-fix: first-spawned wedged 3/3.
cargo fmt / cargo clippy -p minimal2 --all-targets -D warnings / cargo test -p minimal2 clean.

Notes

Unblocks concurrent own-ip sessions (e.g. docs/specs/03-spec-networking/test-plan.sh TC2 same-host peer) without waiting on the upstream libkrun fix.
The upstream defect remains tracked in #588; a draft upstream issue (with the build/repro notes) is prepared for containers/libkrun.

References: #588

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved attach reliability by reducing race conditions when starting interactive sessions.
- Added a short coordination window during connection setup to help prevent intermittent failures in environments with socket concurrency issues.

coderabbitai · 2026-06-28T22:09:28Z

📝 Walkthrough

Walkthrough

Adds a hidden attach-lock internal subcommand and AttachLockArgs struct to the minimal CLI, wires it into command dispatch, and updates cmd_attach to spawn this helper before exec'ing ssh, serializing concurrent attach establishment via an exclusive file lock. Adds a libc workspace dependency.

Changes

Attach Lock Serialization

Layer / File(s)	Summary
CLI command and args definitions `crates/minimal/Cargo.toml`, `crates/minimal/src/main.rs`	Adds `libc.workspace = true` dependency, a hidden `Command::AttachLock` variant, and `AttachLockArgs` struct holding a lock-file path and lock-holding window in milliseconds.
Attach flow wiring and lock helper implementation `crates/minimal/src/main.rs`	Dispatches `Command::AttachLock` to `cmd_attach_lock`; `cmd_attach` reads `MINIMAL_ATTACH_ESTABLISH_WINDOW_MS`, computes a lock path, spawns the helper before exec'ing ssh, and waits for "ok"; `cmd_attach_lock` acquires a blocking exclusive `flock`, prints "ok", sleeps for the window, then releases the lock on exit.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as minimal CLI (attach)
  participant Helper as attach-lock helper process
  participant LockFile as Lock file (flock)
  participant SSH as ssh

  CLI->>Helper: spawn "attach-lock" with lock_path, window_ms
  Helper->>LockFile: libc::flock(LOCK_EX)
  LockFile-->>Helper: lock acquired
  Helper-->>CLI: print "ok" to stdout
  CLI->>SSH: exec ssh
  Helper->>Helper: sleep(window_ms)
  Helper->>LockFile: release lock on exit

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

own-ip attach intermittently hangs forever on AF_VSOCK missed wakeup #588: Both changes address attach-time libkrun/vsock concurrency hangs by serializing attach establishment.

Suggested reviewers

evanspearman

A rabbit locks the door real tight,
One paw in, then hops just right.
No more vsock collision fright,
ssh sails smooth into the night. 🐇🔒
Hop, flock, sleep, release — delight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: serializing concurrent attach establishment.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

Concurrent own-ip session SSH handshakes racing through libkrun's host->guest vsock muxer can leave one session wedged (its RequestShell is never delivered to session_host), so an overlapping `minimal attach` occasionally hangs. The defect is a rare timing race in libkrun's vsock device under concurrent connections (~0.4%/attach at high concurrency); this is an in-repo workaround pending the upstream fix. `attach` now spawns a short-lived detached `attach-lock` helper that holds an exclusive `flock` for a bounded establishment window (default 4000ms, MINIMAL_ATTACH_ESTABLISH_WINDOW_MS) before exec-ing ssh. A concurrent attach blocks on the same lock and starts its handshake ~one window later, so the racy handshake/RequestShell phase is serialized instead of overlapping. Uncontended single attaches only wait for the lock-acquired signal (~0.24s), so steady-state latency is unchanged. Uses blocking `flock(LOCK_EX)` -- the same primitive lcache's read-tracker uses, minus LOCK_NB so a contender waits rather than failing -- which works on Darwin (BSD) and Linux alike; no new crate dependency beyond libc. Tracked upstream in #588. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/minimal/src/main.rs`:
- Around line 623-635: The attach-lock flow in the helper spawn block can hang
forever on `out.read(&mut buf)` and leaves the spawned child unreaped. Update
the attach path around the `cmd_attach_lock`/`child.stdout.take()` logic to use
a bounded wait or timeout and fall back to the unserialized path if the helper
does not report readiness promptly. Also keep a handle to the spawned child and
explicitly reap it after `ssh.exec()` (or when attach ends) so it cannot become
a zombie.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 080fe7d5-d740-4cb4-9699-b54123184660

📥 Commits

Reviewing files that changed from the base of the PR and between 4881f18 and 89883b2.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

crates/minimal/Cargo.toml
crates/minimal/src/main.rs

✅ Files skipped from review due to trivial changes (1)

crates/minimal/Cargo.toml

coderabbitai · 2026-07-01T01:06:34Z

+            Ok(mut child) => {
+                // Block until the holder reports `ok` (lock acquired) or dies.
+                if let Some(mut out) = child.stdout.take() {
+                    use std::io::Read;
+                    let mut buf = [0u8; 3];
+                    let _ = out.read(&mut buf);
+                }
+                // Leave the holder running detached; it releases after the window.
+            }
+            Err(e) => {
+                tracing::warn!("attach-lock helper failed to spawn ({e}); attaching unserialized");
+            }
+        }


🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail # Map relevant file structure first. ast-grep outline crates/minimal/src/main.rs --view expanded > /tmp/main_outline.txt || true # Show the relevant line ranges around the attach-lock flow. sed -n '580,740p' crates/minimal/src/main.rs | cat -n echo echo '--- outline ---' sed -n '1,240p' /tmp/main_outline.txt | cat -n

Repository: gominimal/minimal

Length of output: 11405

🏁 Script executed:

#!/bin/bash set -euo pipefail # Locate the attach-lock helper / flock logic and any timeout or fallback handling. rg -n -C 4 "attach-lock|flock|LOCK_EX|proceed unserialized|spawn\\(|stdout\\.take\\(|read\\(&mut buf\\)|cmd_attach_lock|unserialized" crates/minimal/src/main.rs echo echo '--- any wait/reap handling around detached child ---' rg -n -C 4 "wait\\(|try_wait\\(|kill\\(|detach|zombie|reap" crates/minimal/src/main.rs

Repository: gominimal/minimal

Length of output: 5911

Bound the attach-lock wait and reap the helper

out.read(&mut buf) can block indefinitely here because cmd_attach_lock uses a blocking flock(LOCK_EX) with no timeout; a stale holder on the lock file will stall attach instead of degrading to the unserialized path.

The spawned helper is never waited on before ssh.exec(), so if it exits during the session it can sit as a zombie until the parent process exits.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimal/src/main.rs` around lines 623 - 635, The attach-lock flow in the helper spawn block can hang forever on `out.read(&mut buf)` and leaves the spawned child unreaped. Update the attach path around the `cmd_attach_lock`/`child.stdout.take()` logic to use a bounded wait or timeout and fall back to the unserialized path if the helper does not report readiness promptly. Also keep a handle to the spawned child and explicitly reap it after `ssh.exec()` (or when attach ends) so it cannot become a zombie.

twitchyliquid64 reviewed Jun 29, 2026

View reviewed changes

Comment thread crates/minimal2/src/main.rs Outdated

Base automatically changed from feat/networking-host-exposure to main June 29, 2026 16:04

norrietaylor mentioned this pull request Jul 1, 2026

own-ip attach intermittently hangs forever on AF_VSOCK missed wakeup #588

Closed

norrietaylor force-pushed the fix/588-concurrent-attach branch from aa6b77a to 4881f18 Compare July 1, 2026 00:59

norrietaylor force-pushed the fix/588-concurrent-attach branch from 4881f18 to 89883b2 Compare July 1, 2026 01:02

coderabbitai Bot reviewed Jul 1, 2026

View reviewed changes

twitchyliquid64 approved these changes Jul 1, 2026

View reviewed changes

norrietaylor marked this pull request as draft July 1, 2026 04:05

norrietaylor changed the title ~~fix(minimal2): serialize concurrent attach establishment (#588)~~ [WIP] fix(minimal2): serialize concurrent attach establishment (#588) Jul 1, 2026

norrietaylor closed this Jul 6, 2026

norrietaylor deleted the fix/588-concurrent-attach branch July 8, 2026 22:57

norrietaylor mentioned this pull request Jul 16, 2026

established attach stream dies silently mid-session; client hangs forever with no EOF or timeout #788

Closed

This was referenced Jul 24, 2026

Updating minimal in another terminal corrupts an in-progress Claude session's terminal #952

Closed

bug: min attach without a TTY hangs indefinitely #953

Closed

Hide min session attach -c/--command from the CLI help surface #1062

Closed

Navigation Menu

Uh oh!

[WIP] fix(minimal2): serialize concurrent attach establishment (#588) - #590

[WIP] fix(minimal2): serialize concurrent attach establishment (#588)#590
norrietaylor wants to merge 1 commit into
mainfrom
fix/588-concurrent-attach

norrietaylor commented Jun 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 28, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Suggested reviewers

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

norrietaylor commented Jun 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Fix (in-repo workaround, pending the upstream libkrun fix)

Verification (macOS/HVF, DM1)

Notes

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related issues

Suggested reviewers

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

norrietaylor commented Jun 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 28, 2026 •

edited

Loading