Skip to content

audit: 12 bug fixes, three decided behaviour changes, cleanups across every module - #486

Merged
CMGS merged 127 commits into
mainfrom
review/whole-repo-2026-11-06
Nov 6, 2026
Merged

CMGS merged 127 commits into
mainfrom
review/whole-repo-2026-11-06

Conversation

@CMGS

@CMGS CMGS commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Whole-repo static audit of main 1a48e2b. Every source, test, doc and infra file was read in full, across 8 logic lanes, 7 style/simplify/LOC batches, docs-vs-code and infra. No hardware or E2E runs in this round.

Bugs fixed (each with a regression test that fails without the fix)

Area Bug Commit
pool/reader A reader VM was dropped only by its idle timer, so a connection held open across a wake or release kept an uncounted, unbilled VM running the claim's disk. The reap tick now retires stale readers. pool: retire a reader whose claim woke or left…
pool/checkpoint ClaimCheckpoint took the id as the capability; a tenant holding another tenant's checkpoint id could branch it. Now scoped like list and delete (docs updated). pool: a tenant branches only its own tenant's checkpoints
pool/template resolveGolden fell back to a promoted template while a configured pool's golden was rebuilt, cloning a tenant image for a pooled key. pool: never serve a pooled key from a promoted template…
pool/refill ErrStopping during shutdown counted as a clone failure and could delete a healthy golden's stamp. pool: a clone cut off by engine shutdown never counts…
pool/checkpoint Tun state was read before the transition lock the capture holds. pool: record a checkpoint's tun state under the transition lock…
pool/archive A capacity hand-off whose peer adopted before its reply timed out answered 429 instead of 404. pool: a wake hand-off whose peer adopted…
config archive_failover hands claim tokens to gossip-learned addresses but did not require mesh.cluster_key (heal and moves already do). config: archive_failover requires mesh.cluster_key
sdk (Go + Python) A redirected claim whose reply was lost after delivery was sent to the next node, claiming a second sandbox. Only a dial failure, a redirect, or a sandboxd refusal moves on now. sdk: never resend a redirected claim…
sdk/go Run treated a stdin read error as EOF; Lookup probes were unbounded. two commits
silkd The language server was killed by pid, orphaning its children; it now runs in its own process group. silkd: kill a language server by its process group
silkd openpty set close-on-exec after opening, so a concurrent fork could inherit the pair. Now O_CLOEXEC from birth. silkd: open a pty with close-on-exec set from birth
e2e Three drivers leaked claims on error paths. e2e: release the claim on every driver error path

Behaviour changes decided in this round

  • Go stream calls (DialPort, Lsp.Request, OpenPty, Watch, Logs, Attach, WriteFile) re-send after an owner move, like the Python SDK. Run with Stdin and Push still fail once.
  • Both SDKs refuse a comma-separated seed list instead of silently keeping its first address.
  • A pty whose client disconnects gets SIGHUP (so the shell hangs up its jobs, as ssh does), then SIGKILL after 2 s.

Cleanups

Simplify / LOC-justify / comment / layout items across pool, server, store, egress, outbound, tunnet, engine, metastore, SDKs, mcp, e2e, silkd, boot/init and the Python packages. Examples:

  • generic CAS.One / CAS.All replace the per-package PG row plumbing;
  • openRelay;
  • harness.Poll;
  • borrow-first silkd signatures;
  • Client.close() in Python;
  • vacuous or real-clock race tests rewritten to exercise their interleaving (each proven by breaking the guarded code);
  • duplicated test scaffolding folded;
  • docs deduplicated and corrected where the code disagreed.

Numbers (1a48e2b..HEAD)

added removed net
production (Go, Rust, Python) 822 857 −35 (includes the bug fixes)
tests and test-helper packages 2156 2524 −368
docs 130 192 −62

Comment lines: Go/Rust 43 added, 70 removed; Python 0 added, 1 removed.

Checks run

  • make go-lint: 0 issues. ×10 (5 modules × linux/darwin).
  • asl: no rule findings. The 4 advisory forwarders (replaceArchive, skipIdle, refillGated, isZero) are kept because each names a predicate or flag combination.
  • Go: go test -race -count=1 ./... passes in every module. The PG-gated packages were also run against a real PostgreSQL in one worker pass.
  • silkd: fmt, clippy -D warnings and tests pass on macOS (178) and in a rust:1.99.1 linux/arm64 container (180). boot/init: 15 on macOS, 17 on Linux.
  • Python: ruff passes, and sdk/python has 292 passing. Adapters: 10 + 15 passing on 3.12.

Coverage

Flow Status
Claim, admission, refill, golden/template read; fixes unit-tested
Hibernate, wake, capture, archive, reap, reconcile read
Cross-node: failover, replica, live move, reader read; reader fix unit-tested
Volumes and stores read
API auth/ownership, config, usage shipping read; checkpoint ownership unit-tested
Egress, outbound, tunnet read
silkd and boot/init read; pty/lsp proven on Linux
SDK contracts read; redirect, stdin and move-retry unit-tested
Hardware E2E not run (out of scope for this round)

Known items kept without code

  • Concurrent promotes of one template key on two nodes can silently drop a tenant's template #485: concurrent promotes of one template key on two nodes can drop a tenant's template; needs a store precondition.
  • /owner lookups are not rate-limited. Each request costs one PG point lookup, and a limit would throttle lookups during failover.
  • engine.Remove keeps returning not-found; a test pins that contract.
  • TestS3BackendContractRealEndpoint does not run in CI (no S3 service); it is a hardware-round item.

CMGS added 30 commits November 6, 2026 16:07
…den is rebuilt

resolveGolden fell back to the template store whenever the pool golden was momentarily empty (first build, image change, reload), so claims on a pooled key cloned a tenant's promoted image.
During shutdown eng.Close refuses new commands with ErrStopping; counting those as clone failures could reach the streak and delete a healthy golden's stamp, so the next start rebuilt it from a cold boot.
…he node

A candidate that read the claim and then dropped the reply (timeout, reset, 502/504 edge) may have committed it; trying the next candidate or the origin claimed a second sandbox the caller never saw, billed until its TTL. Only a dial failure, a redirect, or a sandboxd refusal moves on.
…n holds it

Only the idle timer and a new dial dropped a reader, so a tenant that kept one reader connection open across a wake or release kept an uncounted, unbilled warm VM running its own guest disk. The reap tick now drops every reader whose claim no longer sleeps on the source it serves.
ClaimCheckpoint took the unguessable id as the capability, while list and delete already scoped to the caller's tenant; a tenant holding another tenant's checkpoint id could branch its memory and disk.
…nswers 404

takeBack found the row taken and dropped the claim, but handOffForRoom treated that ErrUnknownSandbox as a failure: the wake answered 429, armed the quiet period and missed wake_handoffs_total.
Every hand-off presents the claim's bearer token to an address learned from gossip; checkpoint_peer_heal and live moves already require an encrypted mesh for the same reason.
…n contract

A bridge tap survives vm hibernate and restore of the same VM (contract B11); the refusal holds because restore resumes the guest inside the call, before sandboxd can re-lock (B10).
…ptures under

publishCheckpoint read TunOff before exportSource took sb.Transition, so a SetTun enable landing in between captured a guest with tun0 up into a checkpoint whose branches start with tun off.
pumpStdin treated any read error from Cmd.Stdin as EOF, so a reader that failed mid-stream truncated the command's input and Run reported success, unlike os/exec.
A tagged 404 runs Lookup, and a released sandbox answers 404 everywhere, so a blackholed peer held the caller for the transport dial timeout when its ctx had no deadline.
The server was spawned in silkd's group and killed by pid, so a reaped, stopped or TTL-expired server orphaned its own children (cargo check, proc-macro-srv, tsserver); exec and session already group-kill.
lifecycle left an archived claim behind on any failure, which made the next run see Archived=1 at once; coldproof and demo leaked a claim to its TTL on an exec error.
…benchmarks once

The architecture diagram and lane summary live in docs/index.md.
…dirty-marker wording

Token model stays in sandboxd-api.md, the 429 causes get one Admission refusals section, the reader wire behavior and operator view split between the API and deploy pages, the systemd unit is linked rather than copied, and the archive_failover plaintext-env fact stays in security.md and the deploy table. The dirty-marker text now describes what UnmountVolume and DetachVolume check; the usage node note says a rename lands the journal again.
CMGS added 26 commits November 6, 2026 17:03
…ytes and WaitStacks through storetest, fold peer round-trip tests
…clone path, manifest env for the missing-manifest test
…ts, subtests per table row, one table for the preview failure statuses
…unting sandbox for keep-alive, git plumbing asserts the guest request
… stays on the wire

The SDK test pins that an explicit empty add/remove list is sent; omitempty dropped it.
… an owner moves

DialPort, Lsp.Request, OpenPty, Watch, Logs, Attach and WriteFile only rebound and failed once, while the Python SDK re-sends them; the move tag is raised at the dial or upgrade reply, before any frame, so a re-send is safe. Run with Stdin and Push still fail once: they stream the caller's reader.
…g it

A SIGKILLed shell cannot pass SIGHUP to its jobs, so a background job started in an abandoned web terminal kept running; ssh delivers the hang-up. The shell now gets SIGHUP and SIGKILL only after a 2 s grace, and the master keeps draining after the client is gone because a session leader exits only once its tty output drains.
openpty opened /dev/ptmx without O_CLOEXEC and set it with fcntl afterwards, so an exec forked on another connection in between inherited the pair and kept the pty alive after the shell exited. posix_openpt and the slave open now pass O_CLOEXEC; a unit test asserts both fds on Linux and macOS.
@CMGS CMGS changed the title Whole-repo audit: 12 bug fixes, three decided behaviour changes, cleanups across every module audit: 12 bug fixes, three decided behaviour changes, cleanups across every module Nov 6, 2026
@CMGS
CMGS merged commit 82607e5 into main Nov 6, 2026
4 checks passed
@CMGS
CMGS deleted the review/whole-repo-2026-11-06 branch November 6, 2026 09:38
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