Skip to content

follow-up: off-loop reader teardown, one upload rebind, Python dial-failure parity, reap log out of the lock - #487

Merged
CMGS merged 11 commits into
mainfrom
review/audit-followup-2026-11-06
Nov 6, 2026
Merged

CMGS merged 11 commits into
mainfrom
review/audit-followup-2026-11-06

Conversation

@CMGS

@CMGS CMGS commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #486. Two independent read-only reviews of the merged diff and one hot-path performance review raised the items below. Each was checked against the source before it was changed.

Fixes (each with a regression test that fails on main)

Commit Problem on main Test
pool: retire stale readers through the bounded maintenance fan-out dropStaleReaders ran cocoon vm rm for every stale reader on the Run loop itself, so refill, liveness and reap stalled behind serialized engine removals. TestStaleReaderTeardownNeverBlocksTheCaller (on main the call blocks on a stalled removal)
sdk/go: rebind a failed upload once WriteFile ran Lookup twice on an owner-gone error: once from retryMoved, once from uploadRPC's own deferred rebind. That doubles the failure latency. TestAWriteToAGoneSandboxLooksItsOwnerUpOnce (main: 2 owner probes)
sdk/python: treat any connect-phase failure of a redirect candidate as not delivered _claim_refused accepted only refused or DNS failures, so a candidate on a dead host (EHOSTUNREACH, connect timeout) stopped the redirect walk while the Go SDK moved on. The client now connects explicitly and tags every connect failure, TLS included, as a dial error. test_claim_redirect_skips_an_unreachable_candidate
sdk/python: a refused owner still counts as gone once connect failures are tagged Found by review of this PR: tagging connect failures hid the ConnectionRefusedError that owner-gone tagging checks, so control calls stopped following a sandbox off a retired node. test_a_moved_sandbox_retries_a_control_call_at_its_new_owner[old_node_retired]
silkd: share one compiled regex across a replace and reuse fs.read's buffers Rust ownership audit: fs.replace cloned the Regex per file (a clone builds a fresh cache pool, so the lazy DFA restarted cold per file); fs.read dropped its full head buffer and frame and reallocated about 600 KiB per large read. existing read_streams_files_around_the_first_chunk_bound, replace_*
pool: log failed reap marks before taking the manager lock Reverts a #486 cleanup that moved one Error line per expiring archive under m.mu. With the checkpoint store down, that is thousands of lines per reap tick held against the claim path. Restores the code that predates #486

Flaky test fixed

a_client_gone_mid_output_releases_the_stdin_a_forked_child_holds (since #420) dropped the client right after the first stdout frame. On a loaded runner the group SIGKILL could land before the forked perl ran setpgrp, so the child that writes the result died and the test waited out its 30 s deadline. This failed once on this PR's CI. Reproduced locally by delaying setpgrp 0.5 s. The child now marks itself ready after setpgrp, the test waits for that mark, and the test passes with the injected delay.

Style

  • silkd: pty_name moved below the public helpers; tokio::process::Child imported.
  • mcp: the repeated sandbox_id description named once. PoolSpec.ValidateLimits gets a pointer receiver like its siblings.
  • One-line package docs for the e2e drivers and sandbox-mcp. egresssmoke and rpcbench stay multi-line: they carry a config example and usage the code cannot show.
  • Restating godocs dropped (openStream, waitArchived, noteArrival and three others in watermark.go, pty drain); tokio process types imported where silkd spelled them out.

Reviewed and left as is

  • Hoisting log.WithFunc to the top of handleExec, wakeGuest, router.fail and serveTun. These run per request or per connection, and building the logger only on the error branch keeps the success path allocation-free.
  • tenants/pg slices.AppendSeq([]string{}, …): an empty set must bind as '{}', not NULL.
  • claim.go's compare-then-write of sb.Replicated: the field is read without the lock.
  • Rust ownership audit verdict: no copy abuse on any hot path (framing, exec/pty/session pumps, proc ring, fs streaming, find walker, tun pumps, forward, net_egress, imds). The session exec's few per-command strings are kept: negligible next to the pipe round trip, and both rewrites trade clarity or add an allocation.

Numbers (origin/main..HEAD)

added removed net
production 114 155 −41
tests 73 4 +69

Comment lines: 20 added, 81 removed. The 20 added are one-line package docs that replace longer ones and moved lines.

Checks run

  • make go-lint: 0 issues. ×10.
  • asl on both GOOS: clean.
  • Go: go test -race -count=1 ./... passes in all modules.
  • silkd: fmt, clippy -D warnings and tests pass on macOS (178) and in a rust:1.99.1 linux/arm64 container (180).
  • Python: sdk/python has 294 passing; ruff passes.
  • Two independent read-only reviews of this branch (the second over the last three commits) end with no open finding.
  • No hardware run.

CMGS added 7 commits November 6, 2026 18:13
Folding the failed-mark filter into the keep loop moved one Error line per expiring archive under m.mu; with the checkpoint store down that is thousands of lines on every reap tick, held against the claim path.
dropStaleReaders ran cocoon vm rm for each stale reader on the Run loop itself, so a wave of wakes with open reader connections stalled refill and liveness behind serialized engine removals.
…s not delivered

_claim_refused accepted only a refused connection or a DNS failure, so a candidate on a dead host (EHOSTUNREACH, connect timeout) stopped the redirect walk while the Go SDK moved on; the client now connects explicitly and tags any connect failure, TLS included, as a dial error.
WriteFile's retryMoved and uploadRPC's own deferred rebind each ran a full Lookup when the owner was gone, doubling the failure latency; only Push, which never retries, rebinds after its upload now.
@CMGS CMGS changed the title Audit follow-up: off-loop reader teardown, one upload rebind, Python dial-failure parity, reap log out of the lock follow-up: off-loop reader teardown, one upload rebind, Python dial-failure parity, reap log out of the lock Nov 6, 2026
CMGS added 4 commits November 6, 2026 18:35
…s are tagged

Wrapping connect failures in _DialError hid the ConnectionRefusedError that _owner_answer checks, so fork, renew, hibernate, checkpoint, promote, preview and volume calls stopped following a sandbox off a retired node.
…buffers

fs.replace cloned the Regex per file, and a regex clone builds a fresh cache pool, so every file restarted the lazy DFA cold; an Arc shares one pool across the blocking tasks. fs.read dropped its full head buffer and frame and stream_data_frames allocated new ones, about 600 KiB per large read; both are passed in now.
…ng comments, trim the smoke doc and the single-lookup test
…-gone stdin test hangs up

The test dropped the client after the first stdout frame; on a loaded runner the group SIGKILL could land before perl ran setpgrp, killing the child that writes the result, and the test waited out its 30 s deadline. Reproduced by delaying setpgrp 0.5 s; the child now marks itself ready after setpgrp and the test waits for that mark.
@CMGS
CMGS merged commit ec65c93 into main Nov 6, 2026
3 checks passed
@CMGS
CMGS deleted the review/audit-followup-2026-11-06 branch November 6, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant