Repository navigation
review: 7 fixes, cuts, docs against the code - #530
Merged
Merged
Conversation
…arking it mid-flight run, logs and attach parked the connection on any SilkdError, including one the caller's own on_stdout/on_stderr raised. The stream's remaining frames then sat in the reader buffer, where the quiet probe cannot see them, and the next call on the handle read them as its reply; a stdin pump blocked behind unread output also hung run's join. _deliver now aborts the connection, which unblocks the pump and makes the next take discard it. Twin of the Go fix in #524.
…ts the pump pumpStdin sent stdin_close before handing its read error back, and Run only looked at that error once stdinDone had closed. A command that exits on stdin EOF (cat > f) answers exit right after the stdin_close, so the exit frame could arrive while the pump goroutine was still between its send and close(stdinDone); Run then returned the exit code with no error for truncated input, the gap 3464794 closed. The pump now publishes the error on a buffered channel before the stdin_close, and Run reads it without needing the pump to have returned.
The fold caps a reap at its deadline only while the claim is still in a state; an archive_delete inside archived ends the span at its own t. The purge branch wrote archive_delete first, after the checkpoint delete, so every purged archive billed archived storage past its retention deadline by the reap tick, the journal fsync, the claims commit and the store delete, and by the whole downtime when the node was down across the deadline. Recording the reap first ends the span at the deadline, matching the order the restart path already uses.
purgeArchiveRow's rows all come from Sweepable, which returns rows without their claims (NULL::jsonb), so decoding rec.Claim failed on every purge: each logged a decode warning and released nothing. The address of a purged row already converges through reconcileTunAddrs, which reclaims it once no node has confirmed it for a day and its row is gone.
…aim slot dropMoveIn forgot the claim first and removed moves/<id> only after the VM removal, which can take seconds. Once the claim was forgotten AdmitMove accepted a retry of the same id and wiped and refilled the staging dir, and the old drop then deleted the retry's rounds, failing the retried move at receive. Removing the dir while the claim still holds the id keeps the retry's files, the sibling of the round and commit cases the reservation already guards.
#524 added a_user_pty_shell_owns_its_tty behind #[ignore = "needs root"], but the sudo step lists its test binaries by name and pty_e2e was not among them, so the devpts ownership check never ran in CI.
…o its rule x.examplİ.com lowercases to x.exampli.com, which *.example.com never matched, so the case passed on the rule alone and deleting the plainHost check would not fail it. x.apİ.internal lowercases to x.api.internal, the vector the forward door's test already uses.
… the release fails Both released the sandbox and then closed the Client, so a release that raised skipped the close and left the parked control connections open for as long as the closed toolkit or deleted session stayed referenced; the error-path half of the #524 adapter-close fix.
- deploy.md: balloon is in the node-wide reload list, and the unit runs /usr/local/bin/sandboxd - sandboxd-api.md: a failed archive at expiry retries a minute later; every wakes_refused reason counts a refusal whether or not a hand-off ran - performance.md: there is no proto 2 since #469 - sdk.md: disk_reserve also refuses a hibernated wake; the PutTenantCluster example keeps the egress class it would otherwise clear - silkd.md: the lsp not_found names the manifest path; the user errors quote the name - protocol/README.md: Go and Rust both pin the corpus size, and only Go checks every verb has a fixture - README.md: a fresh repo builds the whole image chain on its own; carriers are sha-pinned only when rebuilt in the run - engine: the package doc names the cocoon daemon event stream - metrics: anon_bytes help includes used swap
A preview hit dials through wakeArchived like any call, so on a node over memory_limit, max_running or disk_reserve it can hand the claim off and answer 404, after which the URL minted on the old owner never serves again. #527 made memory pressure a trigger; the preview and hand-off sections now say so.
… retried list failure as a warning, the meminfo parse error has its colon The hourly checkpoint TTL sweep logged a store list or delete failure at Error while every sibling sweep (orphan archives, archive deletes, stale moves, tun) logs the same retried failure at Warn. balloonDue's godoc repeats balloonTo's encoding of want.
balloonOnce sets balloonSeen on every tick while balloon reclaim is on, before idleOnce starts the tick's hibernates, so the only window the extra store covered is a reload that turns reclaim on and a second reload that turns it off again inside one 5 s tick, with an idle hibernate inflating in between. The test that reached it by resetting balloonSeen by hand goes with it.
…pped-frame rule, peersTimeout's godoc covers the owner probe, and volumes.go's reply no longer shadows the wire package name lease.done deliberately type-asserts *wire.ErrorResp instead of errors.As (the #524 fix); the godoc now states it so an errors.AsType sweep does not undo it.
…c clones as Arc::clone The i8 tri-state copied LANE's encoding, which only an AtomicI8 needs; has_egress now reads SILKD_NET.unwrap_or_else(DEVICE_BACKED). The two Arc clones match the crate's Arc::clone(&x) spelling everywhere else.
… its module doc no longer claims every unsafe block, and the silkd Dockerfile pin comment is one line tun_uring.rs keeps its own SAFETY-documented unsafe for the ring buffer lifetime.
… metadata JSON from its env The secret discovery step scanned os-image/<name>/*.sh and the tag dir for a single result: the tag dir holds only the Dockerfile and install-agent.sh is the only script, while any new one needs its own --mount line anyway. The merge jobs pasted steps.meta.outputs.json inside single quotes in run:, which a repo description with an apostrophe would break; DOCKER_METADATA_OUTPUT_JSON carries the same value. build-boot's sha ref reuses the kernel version step output.
The per-entry no-cache flag forced a cacheless rebuild whenever a non-Dockerfile input changed, on the premise that such files ride in as buildx secrets. Since #525 only base takes a secret, and base declares INPUTS_HASH before its RUN, so any input change already misses that layer; flavors COPY their files, which buildkit hashes by content. The depth rule also misfired on e2b-ci's systemd units, rebuilding its whole pip install from scratch on a unit edit.
…nts to their one fact
archivePool(keepAnHour) instead of the literal, mustManager and statusServer instead of hand-rolled copies, abortRecord without its one-use mustStale, a dispatchBlocked that takes no ctx its only caller derives from t, the fake silkd recording detach inside handleExec's lock, set() for the umount hook, fire/lost below the fixture types, tunnelProxy after tunnel's methods, a redundant linux build tag and tar close, a hoisted zero-chunk sum, and a tun test message that says what it checks. The removed api_test.go duplicated TestSecondsSaturatesInsteadOfWrapping through a one-line forwarder.
reapOnce hands the purge teardown to runBounded and returns, so each round re-added the same 500 ids while the last round's goroutines were still disarming, deleting checkpoints and untracking them, racing the new claims and burning CPU inside the timed region. The untimed setup now waits until every earlier purge has counted its reap.
CMGS
changed the title
…e the archive_delete The sweep of a departed owner's expired row and reconcileOwnRow's unheld expired row recorded only archive_delete. Inside archived that ends the span at its own t, so archived storage billed from the retention deadline to the purge: up to an hour of sweep lag plus the owner's downtime. A reap carrying the row's deadline ends the span there first, the order 4a0cb95 set for the local purge. The purging node cannot tell a row whose claim a restart already reaped from one still archived, so the first case now journals a second reap; in ended it closes a zero-length span and bills nothing.
…ate fails delete already closes the client in a finally (77738dd); create left it open when client.new raised or the state construction failed, so the parked control connections lived until GC.
…its on the reap counter, and setup writes fail with context Five archivePool(3600) sites a4fb52a missed now use keepAnHour; the benchmark's untimed wait built the whole Counters snapshot per spin; four bare t.Fatal(err) setup calls get the context the move tests already have.
…ther integration tests do
…'s header covers arm64 boot/Dockerfile's mirror note moves above the RUN it explains and its olddefconfig note becomes one line; sandbox.config said it overlays x86_64_defconfig only while arm64 uses it too; install-agent's header splits its facts; deploy.md's 'Its' pointed at the binary; done's godoc drops a clause the type assertion shows; DEVICE_BACKED gets the doc form its neighbours use.
…erts 513cd63) The purging node cannot tell the fold state of the row's claim. A replicated claim whose node restarted with the registry down journals only its claim, so it is still running in the fold; the reap carrying the row's retention deadline then ended that compute span at the retention deadline instead of the lease's. archive_delete outside archived already caps at the last deadline, so the row purge goes back to it alone; the archived-storage tail past the deadline is left to a fold rule change.
…hrough retention and purge The claim journals only its claim, so the purge must not end its running span at the row's retention deadline; the test now pins claim,archive_delete, which fails if a reap comes back.
…y ends in a colon
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 static audit of main 5e31104: every tracked file read whole, plus flow reads across the claim, release, hibernate/wake, archive/move, fork/checkpoint, volumes, egress/tunnet, preview, tenants, usage, memory/balloon, store, silkd, SDK wire, MCP and CI lanes. 23 commits: 7 bug fixes, 2 test fixes, docs drift, CI simplification, and cuts and layout review in their own commits. Net 75 files, +328/-395.
Bugs
on_stdout/on_stderrcallback raised aSilkdError, the connection was parked with the rest of the stream still in the reader buffer. The quiet probe cannot see buffered frames, so the next call on the handle read them as its reply._delivernow aborts the connection. This is the Python twin of the Go fix in audit: egress zoned/non-ASCII host bypass, SDK stream reuse, adapter close, doc drift, cuts #524.Runcould lose a stdin read failure.pumpStdinsentstdin_closebefore it published its error. If the exit frame arrived between that send and the pump returning,Runreturned(code, nil)for truncated input. The error now goes on a buffered channel beforestdin_close.archive_deletebeforereap, and inarchivedanarchive_deleteends the span at its ownt, uncapped (see the fold rules in sandboxd-api.md). So every purge billed the reap tick, the fsync, the commit and the store delete, and a node that was down across the deadline billed the whole downtime. The reap now comes first, the same order the restart path already uses.releaseRowTunnever released an address. Every row it got came fromSweepable, which returns rows without their claims (NULL::jsonb), so each purge logged a decode warning and released nothing. It is removed. The address still converges throughreconcileTunAddrs, as deploy.md describes.Also in this PR
x.examplİ.comlowercases to a name the rule never matched.DOCKER_METADATA_OUTPUT_JSONfrom the env instead of pasting it into single quotes;INPUTS_HASH, and flavorsCOPYtheir inputs.balloonSeenstore whose only window was two reloads inside one 5 s tick;SILKD_NETbecomes anOption<bool>;fs_statthatread_fileno longer sends.review:commits. No fix, sweep or cut commit adds a comment.Gates
make go-lint: all 5 modules x linux/darwin, 100 issues.lines, plusfmt --diff.asl -forwarder=false: 5 modules x 2 GOOS.go test -raceand one bench pass, on darwin and in a Linux arm64 container:sandboxd/utilsTestCloneFileRemovesFailedDestinationin the Linux container;goreleaser check.cargo fmt --check,clippy --all-targets -D warnings,cargo test, and the root-only list from silkd.yml with--ignored. All pass.ruff format --checkandruff check;mypy.make sh-lint.