From ab8c467480b5443736bb156904d16ea194bb86d1 Mon Sep 17 00:00:00 2001 From: CMGS Date: Tue, 22 Sep 2026 14:33:49 +0800 Subject: [PATCH 1/2] review: comment budget on the relay, e2e drivers and mcp tools heldConn's comment still named preview after dialPort went generic; keepAlive restated the const block; three restating comments on e2e constants and a helper; three wrapped comments folded to one line; a test helper carried a doc block. --- e2e/cmd/envdsmoke/main.go | 9 +++------ e2e/cmd/portsmoke/main.go | 7 ++----- mcp/tools.go | 9 +++------ sandboxd/pool/claim.go | 2 +- sandboxd/server/relay.go | 1 - silkd/tests/forward_e2e.rs | 4 +--- 6 files changed, 10 insertions(+), 22 deletions(-) diff --git a/e2e/cmd/envdsmoke/main.go b/e2e/cmd/envdsmoke/main.go index 3d163a46..fa363685 100644 --- a/e2e/cmd/envdsmoke/main.go +++ b/e2e/cmd/envdsmoke/main.go @@ -29,8 +29,7 @@ import ( const ( // envdPort is envd's fixed listener; the SDK derives its host from it. - envdPort = 49983 - // readyWait bounds the wait for envd to answer after the claim. + envdPort = 49983 readyWait = 60 * time.Second // claimTTL outlasts the run so a lease expiry cannot look like a relay fault. claimTTL = 30 * time.Minute @@ -105,8 +104,7 @@ func run(addr, token, template, wantVersion string, hold time.Duration) error { return nil } -// reportGuest asks the guest what it is running, over silkd — the native data -// plane, which must keep working alongside envd. +// reportGuest asks the guest what it runs over silkd, the native data plane that must keep working beside envd. func reportGuest(ctx context.Context, sb *sandbox.Sandbox, wantVersion string) error { version, err := sb.Exec(ctx, "/usr/local/bin/envd", "-version") if err != nil { @@ -221,8 +219,7 @@ func stepConnectH1(ctx context.Context, rt *harness.PortRelay, _ *sandbox.Sandbo return nil } -// stepProcessStart spawns through envd's process service, the half the image runs with -// -no-cgroups, and reads its Connect server stream back over HTTP/1.1. +// stepProcessStart spawns through envd's process service, which the image runs with -no-cgroups, and reads its Connect server stream back. func stepProcessStart(ctx context.Context, rt *harness.PortRelay, _ *sandbox.Sandbox) error { headers := http.Header{ contentType: []string{"application/connect+json"}, diff --git a/e2e/cmd/portsmoke/main.go b/e2e/cmd/portsmoke/main.go index 9519f270..442a6e50 100644 --- a/e2e/cmd/portsmoke/main.go +++ b/e2e/cmd/portsmoke/main.go @@ -27,11 +27,9 @@ const ( // halfClosePort greets and shuts its write side, then keeps reading. halfClosePort = 49984 // deadPort has no listener, so silkd's connect refusal must surface as 502. - deadPort = 49985 - // listenerReadyWait bounds the wait for the in-guest listener to bind. + deadPort = 49985 listenerReadyWait = 20 * time.Second - // guestServerPath is where the uploaded listener lands in the guest. - guestServerPath = "/usr/local/bin/guestserver" + guestServerPath = "/usr/local/bin/guestserver" // claimTTL outlasts the run; the node default would reap the sandbox mid-test. claimTTL = 30 * time.Minute ) @@ -167,7 +165,6 @@ func stepHTTP2(ctx context.Context, rt *harness.PortRelay, _ *sandbox.Sandbox) e return wantReport(string(out), "proto=2", "method=POST") } -// wantReport asserts guestserver reported each expected line. func wantReport(body string, want ...string) error { for _, line := range want { if !strings.Contains(body, line) { diff --git a/mcp/tools.go b/mcp/tools.go index e0694526..039898de 100644 --- a/mcp/tools.go +++ b/mcp/tools.go @@ -80,8 +80,7 @@ var ( } ) -// tool binds one MCP tool's spec to its handler, so a tool can never exist -// in the listing without a dispatch entry. +// tool binds one MCP tool's spec to its handler, so a tool can never exist in the listing without a dispatch entry. type tool struct { name string description string @@ -135,8 +134,7 @@ func (a sandboxArg) id() string { return a.SandboxID } type sandboxIDer interface{ id() string } -// parseAndBox decodes a tool's arguments and resolves their sandbox_id to a -// live handle — the prologue every sandbox-scoped tool shares. +// parseAndBox decodes a tool's arguments and resolves their sandbox_id to a live handle, the prologue every sandbox-scoped tool shares. func parseAndBox[T sandboxIDer](s *server, raw json.RawMessage) (T, *sandbox.Sandbox, error) { var args T if err := parse(raw, &args); err != nil { @@ -450,8 +448,7 @@ func (s *server) boxArg(raw json.RawMessage) (*sandbox.Sandbox, error) { return sb, err } -// checkpointArg resolves a checkpoint_id argument: a handle minted in this -// session when available, else a fresh one — checkpoints outlive sessions. +// checkpointArg resolves a checkpoint_id argument: a handle minted in this session when available, else a fresh one, because checkpoints outlive sessions. func (s *server) checkpointArg(raw json.RawMessage) (*sandbox.Checkpoint, error) { var args struct { CheckpointID string `json:"checkpoint_id"` diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 9e0d11ca..7f3fdb64 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -626,7 +626,7 @@ func clampTTL(ttl time.Duration) time.Duration { return min(ttl, maxTTL) } -// heldConn keeps its sandbox held until the preview connection closes. +// heldConn keeps its sandbox held until the guest connection closes. type heldConn struct { net.Conn release func() diff --git a/sandboxd/server/relay.go b/sandboxd/server/relay.go index 790f8941..bccf98d1 100644 --- a/sandboxd/server/relay.go +++ b/sandboxd/server/relay.go @@ -157,7 +157,6 @@ func (s *Server) trackRelay(client, guest net.Conn) (func(), bool) { }, true } -// keepAlive bounds a client that dies without a FIN; without it the splice goroutine blocks forever. func keepAlive(client net.Conn) { tcp, ok := client.(*net.TCPConn) if !ok { diff --git a/silkd/tests/forward_e2e.rs b/silkd/tests/forward_e2e.rs index f29077c6..4af809c5 100644 --- a/silkd/tests/forward_e2e.rs +++ b/silkd/tests/forward_e2e.rs @@ -64,9 +64,7 @@ async fn forward_round_trips_and_done_on_server_close() { .await .expect("test deadline"); } -/// A guest that half-closes must not take the client's write direction with it. -/// What it reads afterwards travels back to the test body: an assertion inside a -/// spawned task is swallowed, so it would pass either way. + async fn half_close_listener() -> (u16, oneshot::Receiver>) { let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); let port = listener.local_addr().expect("addr").port(); From 43a98ed5df79e2ebea237b18e95f07c4962d2819 Mon Sep 17 00:00:00 2001 From: CMGS Date: Tue, 22 Sep 2026 14:34:37 +0800 Subject: [PATCH 2/2] review: silkd layout, public items above private sysutil's private make_controlling_tty sat between two public fns; proc's State enum is vocabulary and now clusters with Chunk ahead of Table. --- silkd/src/proc.rs | 12 ++++++------ silkd/src/sysutil.rs | 36 ++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/silkd/src/proc.rs b/silkd/src/proc.rs index b496dbd6..1cfcd524 100644 --- a/silkd/src/proc.rs +++ b/silkd/src/proc.rs @@ -26,6 +26,12 @@ pub enum Chunk { Exit(i32), } +#[derive(Clone, Copy)] +enum State { + Running, + Exited(i32), +} + /// Registry of running and recently-exited processes. #[derive(Clone, Default)] pub struct Table { @@ -242,12 +248,6 @@ impl Ring { } } -#[derive(Clone, Copy)] -enum State { - Running, - Exited(i32), -} - /// Stdout/Stderr ride the reused-buffer bulk path; serde's per-chunk allocations dominate replay otherwise. pub async fn write_chunk( w: &mut W, diff --git a/silkd/src/sysutil.rs b/silkd/src/sysutil.rs index 633c243d..0a2d0459 100644 --- a/silkd/src/sysutil.rs +++ b/silkd/src/sysutil.rs @@ -124,24 +124,6 @@ pub fn adopt_controlling_tty(cmd: &mut Command) { } } -/// Makes the calling process a session leader and adopts fd 0 as its controlling terminal. -/// -/// # Safety -/// Only async-signal-safe syscalls; valid in a post-fork child. -unsafe fn make_controlling_tty() -> std::io::Result<()> { - // SAFETY: setsid and ioctl are async-signal-safe and take no pointers; the - // caller guarantees a post-fork child (see # Safety). - unsafe { - if libc::setsid() < 0 { - return Err(std::io::Error::last_os_error()); - } - if libc::ioctl(0, libc::TIOCSCTTY as _, 0) < 0 { - return Err(std::io::Error::last_os_error()); - } - Ok(()) - } -} - /// Reads from a raw fd, surfacing WouldBlock to the caller's readiness loop. pub fn read_fd(fd: RawFd, buf: &mut [u8]) -> std::io::Result { // SAFETY: fd is a live open fd; buf is a valid mutable slice of buf.len(). @@ -181,6 +163,24 @@ pub async fn wait_code(child: &mut tokio::process::Child) -> i32 { child.wait().await.map_or(-1, exit_code) } +/// Makes the calling process a session leader and adopts fd 0 as its controlling terminal. +/// +/// # Safety +/// Only async-signal-safe syscalls; valid in a post-fork child. +unsafe fn make_controlling_tty() -> std::io::Result<()> { + // SAFETY: setsid and ioctl are async-signal-safe and take no pointers; the + // caller guarantees a post-fork child (see # Safety). + unsafe { + if libc::setsid() < 0 { + return Err(std::io::Error::last_os_error()); + } + if libc::ioctl(0, libc::TIOCSCTTY as _, 0) < 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + } +} + /// Fills `b` from the OS CSPRNG without blocking a tokio worker; /dev/urandom covers the fallback. fn fill_random(b: &mut [u8]) -> bool { #[cfg(target_os = "linux")]