Repository navigation
review: five bugs, cuts, tests that can fail, docs - #420
Merged
Merged
Conversation
dialPort, dialPassive and the reader each wrapped a guest connection the same way, and CloseOnFence already treats a nil channel as no watch. No ticker here resets, so time.Tick fits (use-modern-go time_tick_gc).
…en settle deploy.md now says what a golden build waits for, which #415 added.
On a shared store a re-promote through another node left this node's cache stale: DELETE or PUT labels with the replaced digest passed and removed the newer build, and a template promoted elsewhere after startup answered 412 to its correct digest. The check adds one store read, only when the caller sends a digest; the claim path is untouched.
The AfterFunc that closes the raw conn on cancel can beat HandshakeContext's own check, which then returned the read error on the closed conn and failed errors.Is(err, context.Canceled).
…led plumbing Broadcaster.Client, HTTPProber.Client and leaseTiming were never set to anything but their defaults. Healer.budget goes with a synctest test. putRow's nil guard sits behind guards in every caller. dedupAddrs becomes slices.Compact over a sort, and order does not matter to either fan-out. Registry.Put rides advance, silkdSpawn and silkdStat become silkdOnce, s3 key's empty-rest branch had no caller, and mcp's released channel repeated what sync.Once already blocks on. Behaviour is unchanged, the claim path is untouched.
…hey claim Renew persist failure uses a missing store directory instead of chmod, the door-directory restart test skips as root, origin handlers are released through a once-guarded cleanup, the Hibernate/Wake race loop reports errors, the env redaction check runs while the host-only value exists, registry and journal read errors are no longer swallowed, and the lease retry on map order gets 64 attempts.
…s returns the slice
On a small listen backlog the 256 idle dials returned before the door had accepted them, so the first request met a full backlog and was refused; the test passed on that refusal and failed on the next request. The test now polls the door's slots until it holds every idle connection, asserts the first request times out, retries a full-backlog dial on EAGAIN as well as ECONNREFUSED, disarms in Cleanup, and uses SockRoot.
Subtest closures take their own t, the relay and port servers come from serveTestServer, relay dials go through dialPortConn, the wake-log test uses captureLogs, and the redirect-beats-heal case folds into the redirect test.
The delayed-serial test restored the serial from a 60ms timer; the fake now serves blank reads from a counter, so the poll count is exact. The no-peer broadcast test had no assertion; a counting transport now proves nothing is sent.
The splice allocation baseline is taken after both goroutines hold their buffers, the apex wildcard case denies a PUT so only a wrong match can allow it, the digest vector test drops its self-made reorder, the RecordEnds audit test sets it before serving, and the schema test names both indexes.
TestFetchFileInstallsOnlyACompleteDownload now holds the download on a chunk read, after the partial file exists, so installing in place fails it. The stalled-download gates release in Cleanup, the bad json case wants parse config, the meta_store cell case sets its own DSN env, the fake lister uses strings.Cut, and verbless Errorf calls become Error.
… checks its status The three intercept method tests had one body; they are one table and roundTripTLSTo folds into roundTripTLSHost. benchRoundTrip fails on a non-200 answer so a broken proxy cannot read as faster. Two fake docs that restated their bodies go.
The 100ms deadline raced the dial, upgrade and started frame; the fake guest now follows started with output and the test cancels from the stdout writer, so the pid is known when the context ends.
A str stdin started the pump thread, which died on memoryview(str) before sending stdin_close, so a command like cat never saw EOF.
The row's claim record keeps guest: false values, and an env write on an archived claim does not reach it, so an adopter after node loss starts from the value at archive time.
…ept archive's deadline
…ure refusals rt:24.04 has no make, so the complete example now claims node:24.04. A configured pool never warms a promoted template, so that clause goes. mcp/tools.go changes only description strings.
The Dockerfile and adapter changes touch one comment line each; no instruction, argument or code changes.
A forked child that inherits stdin and does not read it kept pump_stdin parked in write_all. The 16-slot client channel then filled, the feeder blocked in tx.send, and finish_feeder held the next RPC on the connection until that child exited.
create_tmp copied the mode but not the owner, so a file a user made turned root-owned on the next fs_write or fs_replace and that user's next append got EACCES. The chown runs before the chmod because chown clears setuid and setgid. Off the exec path: one stat on a write with an explicit mode, plus an fstat and fchown under root.
Interleaved A/B on Linux arm64, four processes with the order swapped: read and write through nix match the libc wrappers within run-to-run noise (read /dev/zero 1 B, old 121.3-130.9 ns, nix 121.5-129.4 ns; pipe write+read 4 KiB, old 421.8-482.5 ns, nix 422.9-470.4 ns; EAGAIN, old 128.2-140.5 ns, nix 127.2-138.6 ns). Hot-path cost: none measured.
Every reader takes bytes and subprocess_result decodes them lossily, so the String built in drain was never used as one.
… the git root test The lsp escape names now resolve to real manifests, replace_preserves_exec_bit checks the rewrite, the committer check holds on a host with a git identity, and the oversized replace compares content. The git root test is #[ignore = "needs root"] instead of a silent return. fs_e2e uses data_frames and nix mkfifo; exec_e2e uses stdout_body; the session assertion that could not fail after its equality is gone.
They were skipped on the runner, which is how the overwrite owner change and the git owner path went unexercised in CI.
CMGS
changed the title
This was referenced Nov 2, 2026
/code sets the case by position: own-line uppercase like godoc, trailing lowercase. Comments only: a comment naming the declaration below it, and a product name, keep their case. e2e is left out.
This was referenced Nov 2, 2026
Merged
This was referenced Nov 3, 2026
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.
A whole-repo review after #417: every Go test, Rust, Python and docs file read in full against the code, with the production-code volume audit on top. It fixes five bugs, removes code nothing needs, makes tests able to fail, and corrects the docs. The commits are separate, so each can be read alone.
Bugs
Each has its own commit and a test.
digest=guard read this node's cache. On a shared store, after a re-promote through another node,DELETEorPUTlabels with the replaced digest passed and removed the newer build. A template promoted elsewhere after startup answered 412 to its correct digest. The guard now reads the record: one store read, only when the caller sends a digest.root:rootafterfs_writeorfs_replace, and that user could no longer write it. The temp file now takes the destination's owner, before its mode, because chown clears setuid.run(stdin=<str>)never returned. The stdin thread died on the type error before it closed stdin. A non-bytes stdin now raisesTypeError.Removed
Behaviour is unchanged.
Broadcaster.Client,HTTPProber.Client,leaseTimingandHealer.budget, which nothing set outside one test. That test now runs undersynctest.putRow, and an empty-path branch in the S3 store'skey.dedupAddrsbecomesslices.Compactover a sort. Neither fan-out depends on order.Registry.Putridesadvance,silkdSpawnandsilkdStatbecomesilkdOnce, andmcp'sreleasedchannel goes, sincesync.Oncealready blocks.read_fd,write_fdandset_nonblockingusenix: 4 fewerunsafelines.Tests
TestEgressDoorCapsConcurrentConnectionswas flaky on macOS: the unix backlog is 128 there, against 4096 on Linux, so the first request met a full backlog, and the assertion accepted any error. WithGOMAXPROCS=1the old test failed 50 of 50, and the new one passes 300 of 300 under CPU load. A mutant with the cap doubled fails it.#[ignore]instead of returning early, and a newsilkd.ymlstep runs them undersudo.Docs
About 30 statements the code contradicted are corrected. The largest:
archive_failover, the row of an archived or replicated claim holds the claim's env,guest: falsevalues included, in plaintext in the shared PostgreSQL. The pages said it stayed node-local. A rotation on an archived claim does not reach that row. This PR corrects the docs only and does not change that behaviour.makeonrt:24.04, which has nomake.docs/benchmarks.mdadvised running the harness beside a live sandboxd, whosesbx-VMs that harness destroys.Hot path
claimandreleaseare untouched. silkd's pty reads and writes make the same syscalls throughnix: an interleaved A/B on Linux, with the order swapped, shows the arms equal within noise. The stdin abort is one call per exec.Evidence
The tree is byte-identical to the one these ran on.
gofmt, build andgo veton both GOOS, andasl, are clean.make go-linton a cold cache printed 10 ×0 issues.go testandgo test -racepass across sandboxd, andgo testacross the other modules.pgpackages pass against a real PostgreSQL (47 tests).cargo fmt,clippyandtestpass for silkd and boot/init on macOS. silkd's tests also pass as root in a Linux container, root-only ones included.ruffandmypyare clean, andpytestreports 271 passed.TestCloneFileRemovesFailedDestination, which fails only on container overlay and does not touch this PR.Not run: hardware. The silkd changes need the next image round. The new
silkd.ymlstep runs for the first time on this PR.Update
execalso stops the stdin task when the client leaves mid-output (4e9a5d37). A forked child holding stdin read 64 KiB on a real guest after the client left, against 608 KiB with the previous silkd.