Skip to content

review: pty stdin wedge, unfence channel, mesh bind, cuts and docs - #440

Merged
CMGS merged 21 commits into
mainfrom
review/post-439
Nov 4, 2026
Merged

CMGS merged 21 commits into
mainfrom
review/post-439

Conversation

@CMGS

@CMGS CMGS commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Whole-repo audit on main after #439. Each fix is its own commit with a regression test; cuts are separate commits and the message gives each reason.

Fixes

  • silkd: a connection wedged behind a pty's stdin (49b1830a). The stdin pump outlived the shell. With a background job holding the slave and not reading, stdin frames filled the pty and then the channel, the feeder parked, and that connection read no further request until the job died. The regression test fails on Linux without the fix.
  • pool: a second unfence swapped the fence channel unclosed (e97e816b). An earlier unfence landing between renewLease's two checks left a run on a lifted state. Streams on the old channel then survived the next trip, so a fenced node kept serving while a peer could adopt. The window is narrow; the fix is one early return.
  • sdk/python: a refused upgrade put raw JSON in the error (2355fc60). APIError.message read {"error":"unknown sandbox"}; the Go SDK and the control-plane path both extract the field.
  • config: 0.0.0.0 and [::] passed as a mesh bind (bb0c8390). Peers recorded an undialable address and every node defaulted to the same node_id.

Copies

silkd session exec copied each read through a second 32 KiB buffer; it now reads into the accumulator. The pty buffer left the serve future, which every vsock connection carried: 35112 bytes to 2672.

Cuts

  • The mesh epoch's persisted lease. Accepted cost: an operator setting the clock back past the last boot, then a crash restart before peers forget the node, leaves peers ignoring its state until they forget it or it restarts on a corrected clock. Placement goes stale; clients fall back. The lease also failed boot on an unwritable data dir. A leftover mesh-epoch file is ignored.
  • Redirect-cache epoch: a delete racing a fan-out can cache an owner for up to 5 s, and the peer answers 404. Prober heal singleflight, deduped upstream by the manager.
  • Per-name tenant eviction tracking: one extra PG read when a cold lookup overlaps an eviction.
  • No backward compatibility: journals no longer derive AttachOnly from Mount, a field persisted since v0.1.20. A node jumping straight from v0.1.19 or older while holding a writable attach-only claim would read it as mounted. Upgrade through v0.1.20+ or release those claims first.
  • Guards no real call site reaches, forwarders with one caller, hand-rolled helpers, duplicated silkd stream code and comments that failed the budget, across mcp, protocol/wire, types, engine, server, store, tenants, egress. Rows reported as cuts but changing behaviour were rejected.

Docs

The root token cannot use the relay, exec, owner or preview mint. A sleeping sandbox wakes on the upgrade request, not a first frame. A wake past the lease can start a fresh one. Commands also get the claim env. Only the s3 store skips the delete broadcast. A checkpoint claim no longer lists a 409. Missing SDK methods, EnvInject, the rate_limited source and MCP arguments are named.

Numbers against main

Prod +253 -627, net -374. Tests +182 -297, net -115 (test classes moved above tests count both ways). Docs +33 -26. Comment lines +18 -50. Files 90.

Hot path: no new work on the claim path. IsContinuation now goes through RequestOp: AllocsPerRun 0 on canonical frames. No claim latency A/B was run, as no claim path code changed beyond a field read replacing a method call.

Verification

Head a2277ca3:

make go-lint: rc=0, 10 × 0 issues.
make go-test: rc=0, 27 packages ok
asl: no findings on linux and darwin outside 4 advisory forwarders, none in this diff
silkd     linux 160 passed, darwin 159 passed, 3 ignored; fmt and clippy clean
boot/init linux 17 passed, darwin 15 passed; fmt and clippy clean
ruff format and check clean; mypy clean
sdk/python 277 passed on 3.12 and 3.9; sdk/openai 10 and sdk/langchain 15 passed on 3.12

PG packages ran against a real Postgres locally at d74b812f and in CI at each head.

Style read in full, against each language's standard: all 48 Go files the mechanical cuts touched, walked through the modern Go catalog per file; all 39 Python files; all 36 Rust files. The simplification review covered the whole diff. The repo has no TypeScript.

External review: three rounds on one thread, one finding, in a commit message, corrected; final verdict no remaining blockers.

Hardware

At ead1150a: its sandboxd, its silkd in a guest image, cocoon v0.7.5, CH 35b098c65260. Two bare-metal hosts with one root path, one kit-local bridge, a kit-local SeaweedFS. No leg failed. Where a leg failed first, the driver or kit was at fault and it was fixed and rerun.

  • Guest image with the new silkd: the full smoke passes (exec, files, session, find, replace, watch, git, pty, hibernate, fork, promote, procs, checkpoint, tree, port).
  • The stdin wedge, in a real guest, one run: v0.1.22's silkd wedged, with no answer in 10 s; this branch's answered the next RPC in 0.04 s.
  • Session output: 5,488,895 bytes matched by SHA-256 across 106 frames, largest 65,496 bytes, up from about 32 KiB and well under the frame cap; shell state held across calls.
  • Peer heal across two hosts: a branch pulled from the other host in about 11 s, disk and memory markers intact, also after a hibernate round trip; a delete reached the peer.
  • Egress: none lane, bridge lane with its nft lock, SOCKS5, HTTPS interception and claim credentials, each script passing, with credentials surviving a restart.
  • S3, kit-local SeaweedFS: contract test, a checkpoint branched on the other host, archive then wake with both markers intact, delete. No cloud endpoint was used.
  • Two-node basics: gossip with a stale mesh-epoch file and after a restart, memory_limit refusal and lift, claim, exec, hibernate, wake, fork, checkpoint, delete broadcast.

The three commits after ead1150a did not get a hardware rerun: silkd is unchanged, and in sandboxd they change one error path's construction and drop an unreachable branch, with text and behaviour unchanged. tls_client.py runs in CI.

Not run: a real clock step for the mesh epoch cut, a kernel-level disk fault (the journal was broken with a directory at its temp path), and a real AWS S3 endpoint.

The held items, measured

Patched arms from a scratch branch, not in this PR. Recommendation: keep all three as they are.

  • O1, rollback after a failed journal commit. Broken journal, base against arm:

    Call Base Without rollback
    release 500, VM and claim alive, succeeds after repair 204, VM destroyed at once
    renew 500, deadline unchanged 500, deadline moved
    set env 500, value unchanged 500, host value new, guest still old
    release, then kill -9 and repair claim alive, client saw 500 claim gone, client saw 204

    Base gives a clean rule, a 500 means nothing changed. Without rollback, a client sees an error for a change that took effect.

  • O3, HMAC on the HEAD probe. Without it, heal and redirects still pass, and an unsigned HEAD answers 200 or 404 instead of 401, so anyone reaching the port can test whether a checkpoint id exists. Ids are random, so the exposure is small, but the saving is about 60 lines against a documented status. A clock-skew false 404 was not reproduced: it needs a host clock change.

  • O4, the 5 s redirect cache. Asker pinned to NUMA node 0 on a quiet disk, owner on another host about 10 ms away on a disk other kits were using, arms interleaved ABBA in one run, reply latency of a redirected checkpoint claim, ms:

    healthy peers, 6 blocks x 1000:    cache p50 0.055  p90 0.070  p99 0.201   no cache p50 10.639  p90 12.216  p99 16.578
    one peer not answering, 4 x 200:   cache p50 0.056  p90 0.075  p99 0.202   no cache p50 161.688 p90 163.602 p99 170.398
    

    Sequential claims on one id only; a cold lookup costs the same on both. The cache earns its 42 lines.

Open and known

The flow readers ran on 5a3a112c; the later cuts were reviewed as diffs. Test files stay due for the correct and perf lenses in the hygiene ledger, since tests were read for style only.

CMGS added 18 commits November 4, 2026 13:04
The pump kept the client's channel after the shell exited. With a background job holding the slave and not reading, stdin frames filled the pty and then the channel, the feeder parked on its send, and the connection read no further request until the job died. exec, lsp and forward already abort theirs.
renewLease reads the fence and then takes the unfencing flag. An earlier unfence finishing between the two left a second one running on the lifted state, and its lift swapped in a new channel without closing the old. Streams holding the old channel survived the next trip, so a fenced node kept serving while a peer could adopt.
ParsedBind rejected only an empty host. 0.0.0.0 and [::] passed, memberlist advertised them as given, peers recorded an undialable address, and node_id defaulted to the same value on every node.
… pty buffer off the future

Session exec copied each read through a second 32 KiB buffer held per session. The pty buffer sat inline in the serve future, so every vsock connection's task carried it, 35112 bytes against 2672 measured. Three test files carried an unwrap allow clippy.toml already covers.
A persisted, leased epoch floor guarded one case: an operator setting the clock back past the last boot, then a crash restart before peers forget the node. Peers then ignore its state until they forget it or it restarts on a corrected clock. That costs stale placement, and a client landing on a stale peer already falls back. The lease also failed boot on an unwritable data dir and dropped publishes when a write failed. A leftover mesh-epoch file is ignored.
Heals are deduped per id by the manager before they reach HealOwners, so the prober's flight had nothing to merge. The epoch only stopped a delete racing an in-flight fan-out from caching an answer for up to 5 s; a stale redirect is already documented and the peer answers 404.
Tracking evictions per name saved one PG read when a cold lookup overlapped another tenant's eviction, and evictions come from tenant writes.
claimLoaded's Capturable check: non-archive records come from Checkpoint and heal, both refuse an egress key. resolve's empty token check: every claimed sandbox holds a token, so the constant-time compare refuses it. reapOnce's early return: the check after the lock covers it. Loading a journal no longer derives AttachOnly from Mount, a field persisted since v0.1.20.
The root token cannot use the relay, exec, owner or preview mint. A sleeping sandbox wakes on the upgrade request, not on a first frame. A wake past the lease can start a fresh one. Commands also get the claim env. Only the s3 store skips the delete broadcast. A reload with no change writes no audit record. The config digest bumps the gossip epoch. Name the missing SDK methods, EnvInject, the egress_conn_rate source and the MCP arguments.
WithNetwork, WithSize and WithAPIToken only assign a field, and both claim fields are omitempty, so the empty-string guards and the opts slice send the same request. jsonText only marshals plain maps and wire types, so its error fallback cannot fire.
bulkDecoders ran once to derive three map entries, so a literal states them directly. IsContinuation repeated RequestOp's tag and error handling; the call inlines and allocates nothing on canonical frames. Godocs reduced to one line.
…pers

Env.Hidden had one prod caller, so it folds into HiddenEnv. SetReplicated and Replicated wrapped a plain field, so it joins the other json:"-" fields and the one writer keeps the change-only write. VolumeNames takes the VolumeRWNames shape, and ReplaceFileSync's defer replaces the renamed flag and the manual closes.
archives/pg now uses the pgtype form usage/pg already has, and scan leaves wrapping to its callers, so a row-scan failure no longer doubles the prefix. config.Load already rejected every DecodedKey and ParsedBind error main checks again, and memberlist.Join with no seeds returns nil. targetsOf only feeds a comparison, and poolset/pg's unavailable had two call sites.
…rwarders

cloneArgs and cloneSnapArgs differed only in source tokens, silkdList and silkdReadFile share one collect form, and silkdWriteFile owns the timeout its callers all opened. newGuestPortConn and Server.relay were one-line forwarders. wakeGuest now calls openGuest, CloseOnFence already handles a nil channel, metadataFilter's empty map matches the same claims, and decodeOptionalBody needed two exits.
tenants/file apply takes a map so Replace needs no rebuild. dir uses os.ReadFile and a deferred close for read-only files. s3 reuses currentManifest, carries the walked size, and drops a chunk loop that never sees more than two keys. peer drops a timeout healBudget already bounds, a second not-found sentinel, an unused client seam and guards no caller reaches; safeJoin is filepath.IsLocal. storetest uses StageRecord.
…ever calls

GenerateRoot and IssueIntermediate built the same template. routeEvent returned its input unchanged, so tunnels and relay ask the router after their deny return. The proxy only calls the three Eval methods, so claim's ServesSocks and InterceptsHost go and Evaluator narrows; Resolve and Compose return PolicyEvaluator. composite's three methods lose one exit each and still skip the tenant scan after a pool deny.
goconst counts a third literal tag in the table the bulkDecoders cut wrote; the keys now come from RespType, the form reader.go's readOnlyOps uses.
@CMGS CMGS changed the title review: pty stdin wedge, unfence channel, mesh bind, cuts and docs after #439 review: pty stdin wedge, unfence channel, mesh bind, cuts and docs Nov 4, 2026
CMGS added 3 commits November 4, 2026 16:18
The agent upgrade put the raw JSON body in APIError.message, so a 404 read agent upgrade: {"error":"unknown sandbox"}. The Go SDK and the control-plane path both extract the error field.
…able Rel branch, NetShape names no engine

The collector's seven-parameter signature read as a naming failure; only the over-limit message needed the extra two. filepath.Rel cannot fail on a path WalkDir built from src, as WalkExport already assumes. A Firecracker lane exists too.
… argparse

Layout and script rules of the Python standard, with no behaviour change; the package docstrings' samples already live in the READMEs.
@CMGS
CMGS merged commit dc1c09d into main Nov 4, 2026
3 checks passed
@CMGS
CMGS deleted the review/post-439 branch November 4, 2026 08:37
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