Repository navigation
review: pty stdin wedge, unfence channel, mesh bind, cuts and docs - #440
Merged
Merged
Conversation
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 betweenrenewLease'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.messageread{"error":"unknown sandbox"}; the Go SDK and the control-plane path both extract the field.config:0.0.0.0and[::]passed as a mesh bind (bb0c8390). Peers recorded an undialable address and every node defaulted to the samenode_id.Copies
silkdsession 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
mesh-epochfile is ignored.AttachOnlyfromMount, 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.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, therate_limitedsource 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.
IsContinuationnow goes throughRequestOp:AllocsPerRun0 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:PG packages ran against a real Postgres locally at
d74b812fand 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: itssandboxd, itssilkdin a guest image, cocoon v0.7.5, CH35b098c65260. 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.silkd: the full smoke passes (exec, files, session, find, replace, watch, git, pty, hibernate, fork, promote, procs, checkpoint, tree, port).silkdwedged, with no answer in 10 s; this branch's answered the next RPC in 0.04 s.mesh-epochfile and after a restart,memory_limitrefusal and lift, claim, exec, hibernate, wake, fork, checkpoint, delete broadcast.The three commits after
ead1150adid not get a hardware rerun:silkdis unchanged, and insandboxdthey change one error path's construction and drop an unreachable branch, with text and behaviour unchanged.tls_client.pyruns 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:
kill -9and repairBase 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:
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 thecorrectandperflenses in the hygiene ledger, since tests were read for style only.