review: billing order, tun addresses, silkd process groups, SDK cancel, docs by CMGS · Pull Request #481 · cocoonstack/sandbox

CMGS · 2026-11-06T05:42:25Z

Whole-repo static audit of main (376a7a9 plus #480). Every production file was read in full by a per-file reader, and 12 flows (claim lifecycle, warm pool, settle/hibernate, failover, capture/store, live move, live volumes, egress metering, API relay, silkd protocol, cocoon CLI + restart, advanced network) were walked window by window. No hardware E2E ran in this round.

Bugs fixed (one commit each, each with a regression test that fails on the old code unless noted)

Billing

A release takes no Transition lock, so a wake, hibernate, archive, unarchive, adopt or move_in event stamped after its claims commit could be journaled after a concurrent release and reopen a span to the deadline (forever for an archive kept forever). Each transition is now billed where it is published.
A graceful stop could lose the last hibernate's event: eng.Close returned when its CLI exited and main exited before recordHibernate. Fixed by the same change.
A crash between the claims.json commit and the egress arm restored a served claim with no claim event. The event now follows the commit; an arm rollback records release before its teardown.
A VMM restart recorded vmm_restart only after the probe (up to 90 s), so a stop in that window left a running guest billed as ended; an interrupted capture whose guest did not resume got vmm_failed without vmm_exit.
A fenced node cold-booted a replicated claim and journaled vmm_exit/vmm_restart after a peer may have adopted it.
Restart reaped (vm_gone) and freed the overlay address of a dropped claim whose move journal says the other node runs it, and freed the address of a replicated claim whose row stays adoptable.
tun frames dropped as no_exit were charged to quota and billed as egress_bytes.
A relay or exec wake queued behind a hibernate restored the guest after its caller had left.

Correctness

PUT /v1/pools accepted "tun": true and dropped it; the config-owned field list lived in three diverged copies and now has one owner.
egress_internal_allow re-admitted public addresses as internal, so the documented guard-off allow plus egress_upstream dialed every connection direct from the node.
The claim-time inject check ran before the claim's policy layer was set (200 for an inject no proxy would apply; PATCH answered 400).
An adopt whose journal write failed could race a wake that consumed the registry row: the claim was lost for good.
One failed LockedTaps at boot quarantined (removed) every live egress-lane VM.
The overlay address was freed only after the VM teardown; a crash there dropped its reuse window. A refused PG release was never retried. Two concurrent tun enables of an addressless claim got the same address, and the loser released it under the winner.
Reconcile armed a received-but-unresolved move; the staging of a dropped moved-in claim leaked until the next restart.
A move's target dropped its reservation 2 minutes after admission while the baseline capture can wait longer on the dump slots.
The dir store wrote the template digest without fsync; Reloaded read a claim's socket without the pool lock.
silkd: inotify overflow was dropped silently; exec children shared silkd's process group, so kill and the SDK cancel left a pipeline's other processes running (each exec now runs in its own group and kill signals it); a bad session cwd answered internal.
Go SDK: a call with a canceled ctx still ran its verb on a parked keepalive connection.
CI: tunnet/pg's PostgreSQL tests never ran.

Cleanups

Dead Host.ResolvedName cut; stale and restating comments removed, layout and godocs to the house rules, test helpers deduplicated, a flaky GRO test fixed, docs matched to the code (release-scoped upgrade notes and duplicates cut).

Numbers

40 commits. Production (Go/Rust/Python) +408 −510 (net −102); tests +552 −424 (net +128); docs and other −27. Comment lines added 68, removed 186.
Net production growth per bug-fix commit is the fix itself; every review/cut commit is net negative.

Gates (macOS and a linux/arm64 container)

make go-lint: 0 issues. ×10 (5 modules × linux/darwin) and fmt --diff clean.
make go-test (race, plus one pass of every benchmark) green in all 5 modules.
asl on both GOOS: no findings (4 advisory forwarders already recorded as kept).
silkd and boot/init: fmt, clippy -D warnings, cargo test green on macOS and in rust:1 linux/arm64, including the root-only --ignored suites.
tunnet/pg against PostgreSQL 16: 5/5. sdk/python ruff + 279 tests, langchain 15 tests green; the openai adapter tests were not run locally (only a comment changed there).
Outside review: Codex, three rounds on one thread. Round 1 found the arm-rollback ordering and a retry that could free a re-acquired address; round 2 led to two concurrent enables freeing the winner's address and to the rollback's event order; all fixed with tests. The restart-lost retry entry and one microsecond retry window were held as accepted. Round 3: "No blocking findings."

Coverage

Area	Evidence
All production files, 12 flows	read in full, window tables per flow
Billing windows above	unit regression tests
silkd process group, fds	Linux container test
Hardware crash injection, live move at scale, tun throughput	not covered in this round

Kept without code (to be added to #433)

tenants/pg shared 200/s cold-lookup limiter: loud 503, converges.
Peer heal budget split across remaining owners: guards a stalled peer.
s3 delete re-stamps manifests: the operator's age-based reclaim relies on it.
A refused PG address release lost across a restart before the hourly retry; a PG Assign whose reply is lost after commit.
64 KiB per queued inbound tun frame (documented bound).
A tun disable during a checkpoint, fork or replica capture sends no tun_down (documented).

Open for the owner

Should the SDKs rebind a handle with Lookup on a 404 after a move or evacuation? Today only Close does, so the MCP server and the adapters keep a stale owner until the lease ends.

notify reports an inotify overflow as an Other event with the Rescan flag, not as an error, so to_frames dropped it and the watch went on with a gap that docs/silkd.md says never happens.

…s exec does The session shell ran 'cd <dir> || exit 1' in its init line, so a missing cwd surfaced as 'internal: shell exited'. The cwd is now the child's working directory and spawn errors share exec's mapping.

kill, the SDKs' cancel and timeout kill, and a foreground client that hangs up reached only the direct child, so 'sh -c "a | b"' left a and b running after every timeout. Sessions already ran in their own group; exec and its kill now do the same.

A parked keepalive connection skipped the dial, the one step that checked the ctx, so a canceled call still ran its verb in the guest while the caller saw context.Canceled; Run started a command it would never kill.

…refuses tun The list lived in three copies (the API refusal, the seed hash, reload's diff) and #478 added tun to only one: PUT /v1/pools answered 200 for "tun": true and dropped it, and a tun edit made every restart warn that the pools differ.

The event waited for the egress arm, so a crash between the claims.json commit and the arm restored a served claim that no event billed. A batch the arm rolls back now records a release for each claim.

Release takes no Transition lock, and wake, hibernate, archive and unarchive stamped their usage event after the claims commit, so a release committing in between was journaled first and the late event reopened a span to the deadline, or forever for an archive kept forever. A graceful stop had the same gap: eng.Close returned when the last hibernate CLI exited and main exited before the hibernate event. Hibernate, wake and a settled intent now record their event inside the m.mu section that publishes them; archive and unarchive stamp the time there. An archived wake also records unarchive and arms its egress door before the checkpoint store delete, not after it.

…'s span A VMM restart recorded vmm_restart only after the probe and the claims commit, so a stop or crash during the probe (up to 90 s) left a running guest billed as ended. vmm_restart now follows the start under m.mu, and every stop after a start records vmm_exit. failLocked records vmm_exit before vmm_failed when the exit was not recorded yet, so an interrupted capture whose guest does not resume no longer stays billed running until release (docs/deploy.md promises both events).

Both events were stamped after the claims commit, so a release committing in between was journaled first and the late adopt or move_in reopened a span to the deadline. adopt takes its time and deadline under m.mu; move_in records inside the commit's publish. A moved guest that cannot start lets failLocked record its vmm_exit.

…or can adopt Reconcile ended a claim whose VM was gone with a vm_gone reap and freed its overlay address even when a move journal said the other node runs it, and freed the address of a replicated claim whose registry row stays adoptable, so a later claim could answer for the same address. Such claims now keep both; purging the row frees its address.

…enced Peers may adopt a replicated claim once the fence trips, but the liveness pass still journaled vmm_exit and cold-booted it with vmm_restart, which the cross-node fold reads after the peer's adopt: the adopted claim billed as running compute. docs/cluster.md says a fenced node only keeps the guest running until it returns.

An adopt whose claims.json write failed forgot the claim without the Transition lock, while a concurrent wake of the just-published claim took and consumed its registry row and then found the claim gone, so the checkpoint was deleted and every later Lookup answered 404. The adopt now holds the new claim's Transition across the commit, and a wake whose claim left during the take stops before consuming the row.

finalizeBatch set the claim's layer only after the inject check, so a pooled key whose pool has no egress resolved to the tenant class during the check: the claim answered 200 for an inject that no proxy would ever apply, while the same env through PATCH .../env answered 400.

destVerdict reported any address an allow entry covered as a re-admitted internal one, public addresses included, and the upstream path dials those direct. With the documented guard-off allow (0.0.0.0/0, ::/0) and egress_upstream, every connection left from the node's own address instead of the claim's upstream, and its audit named no upstream.

The PostgreSQL step's package list left out tunnet, so tunnet/pg's tests skipped and its SKIP check never saw them.

A relay or exec wake can wait on sb.Transition behind a hibernate. It then restored the guest under a detached ctx, recorded a wake and left it running and billed until the idle sweep or the deadline, with nobody connected. It now returns the ctx error once it holds the lock.

One failed LockedTaps call at boot quarantined every live egress-lane claim and removed its VM, though Relock is idempotent and settles each tap on its own. A failed listing now falls back to relocking each tap.

The target dropped a reservation 2 minutes after admission, while the source's baseline capture first waits on the dump slots it shares with hibernates and saves and then writes all guest memory, so a large guest on a slow run dir always failed its first round with 404. The idle drop now matches the 15-minute dump timeout; a retried move still replaces its reservation at once.

forward charged the claim's quota and opened and billed a flow before it looked the exit up, so frames dropped as no_exit (a PUT /tun profile not in tun.exits, or a reload that removed a label in use) were billed as egress_bytes and consumed quota.

…y read #480 touched the claim through Sandbox.Touch, which reads the clock again on every packet in both directions; TouchAt takes the time the flow check read.

The fake underlay's Recv returned whenever its channel was momentarily empty, so the router flushed the coalescer between injected segments and TestDeliverCoalescesInOrderSegmentsIntoOneGSOFrame failed intermittently under -race; the no-TSO merge test passed vacuously the same way. A batch now arrives in one Recv.

…mits Release, reap, rollback and quarantine freed the address only after the VM teardown, which can take minutes. A crash in between left the claim gone from claims.json with no release on record, so the restart treated the address as free with no reuse window and a new claim could inherit the old flows' state at the exit.

A release that failed while the shared meta store was down was only logged, and nothing swept the table, so each failure held one address for good. A refused release is now kept and retried on the hourly store sweep.

… the cocoon root Only archive-e2e.sh said so; run beside a serving sandboxd on the same root, the others wipe its claims. The release workflow comment no longer claims only dist-extra/ is gitignored.

…, test helpers lose restating docs The pty comment said the pump hangs without the drop; since the exit drain got its timeout, held slave dups only make it run to that timeout.

… private helper Multi-line godocs across sdk/go and protocol/wire fold to one line each (a stale Promote parenthetical goes); frame.go's unexported types move below the exported ones; Python drops the _post_json forwarder and makes require_field private.

…y, as the Go SDK does

…, vocabulary types ahead of volumeCatalog Stale and restating comments go (the liveness one is contradicted: the tap survives a VMM exit); a failed golden snapshot rm that blocks the next build logs at Warn; reconcile reuses parseCheckpoint; planTun returns one tunPlan so commitTun's signature fits a line; volumeHolders and catalogVolume move ahead of volumeCatalog.

…ts, mustClaim reuse

…opped one's staging Reconcile armed a MoveIn claim's doors and tun before its move record resolved, so after an aborted move both nodes announced the overlay address until dropMoveIn; the resolve arms it (a running guest on start, a stopped one through the cold restart). The staging sweep kept the guest-memory files of a MoveIn claim that the restart dropped because it checked the loaded set instead of the adopted one.

The digest went through a plain write and rename while meta.json was fsynced, so a power loss right after a publish could leave a template whose digest reads empty and whose digest-guarded delete or labels call answers 409.

…load Reloaded read sb.VsockSocket without the pool lock that a cold restart or a move writes it under; a torn string read is a crash. The socket now comes through TunState, which already reads the claim's tun fields under that lock.

Nothing outside its own test read it, and no audit record carries a name, so every kept A/AAAA answer paid a mutex and a map write for nothing.

Stale and restating comments go or are corrected (Engine.Version no longer names a master-<sha> form cocoon never prints, the outbound Resolve godoc, the address log's compaction bound); own-line comments start uppercase; vocabulary types move ahead of their users in server/move.go, tunnet/gro.go and tunnet/udp; the relay's 101 reply builds on upgradeProto; Local's one-caller compact is inlined.

…no redundant build tags

…e notes and duplicates Drift the readers found is fixed against the code (each change cites its line in the round's ledger): tun traffic resets the idle clock, downtime_ms includes the dump-slot wait, a disarm only stages the tun quota counter, a promote of a claim whose tun is off answers 409, the tun pool flag is config-owned, and the shared tun address table gets its DDL. Upgrade notes for releases older than any running fleet, the duplicated /v1/info payload and stale version history go; silkd's README lists the DNS relay, tun verbs and IMDS.

…w, De Morgan, empty loop body)

… never retry the release of an address its claim holds again Codex review round 1. An arm failure recorded the batch's release events only after the VM teardown, so a crash there left their claim spans open to the deadline; they now follow the drop commit. A release the address store refused could be retried after a later enable had assigned the same PG row back to the claim (Assign is idempotent per claim), freeing a live allocation; the retry drops an entry whose claim holds that address.

…s, and a rollback bills before it frees addresses Codex review round 2. Assign is idempotent per claim, so two concurrent enables of an addressless claim got the same address and the one that lost commitTun's state check released the address the winner had just published; it now releases only an address the claim does not hold. A rolled-back batch writes every release event before any address-store call, so a slow store cannot hold later events back.

CMGS added 30 commits November 6, 2026 12:13

silkd: end a watch with an error when the kernel event queue overflows

a0505e8

notify reports an inotify overflow as an Other event with the Rescan flag, not as an error, so to_frames dropped it and the watch went on with a gap that docs/silkd.md says never happens.

sdk/go: send nothing for a call whose ctx is already done

762bc91

A parked keepalive connection skipped the dial, the one step that checked the ctx, so a canceled call still ran its verb in the guest while the caller saw context.Canceled; Run started a command it would never kill.

pool: record a claim's usage event right after its commit

79bcb37

The event waited for the egress arm, so a crash between the claims.json commit and the arm restored a served claim that no event billed. A batch the arm rolls back now records a release for each claim.

ci: run the tun address store's PostgreSQL tests

fa54d29

The PostgreSQL step's package list left out tunnet, so tunnet/pg's tests skipped and its SKIP check never saw them.

pool: relock egress taps at startup when the lock listing fails

d0de3de

One failed LockedTaps call at boot quarantined every live egress-lane claim and removed its VM, though Relock is idempotent and settles each tap on its own. A failed listing now falls back to relocking each tap.

tunnet: stamp a packet's activity with the time its flow check alread…

9b01bb1

…y read #480 touched the claim through Sandbox.Touch, which reads the clock again on every packet in both directions; TouchAt takes the time the flow check read.

pool: retry an overlay address release the address store refused

62e596c

A release that failed while the shared meta store was down was only logged, and nothing swept the table, so each failure held one address for good. A refused release is now kept and retried on the hourly store sweep.

scripts: say that each e2e script's cleanup removes every sbx-* VM on…

fb4edb7

… the cocoon root Only archive-e2e.sh said so; run beside a serving sandboxd on the same root, the others wipe its claims. The release workflow comment no longer claims only dist-extra/ is gitignored.

review: silkd lsp keeps one stdio pair, pty states its real drain WHY…

412dc41

…, test helpers lose restating docs The pty comment said the pump hangs without the drop; since the exit drain got its timeout, held slave dups only make it run to that timeout.

sdk/python: drop the trailing ': ' from ExitError when stderr is empt…

4282424

…y, as the Go SDK does

test: pool setup through mustManager and restartReconciled, wg.Go wai…

8622a82

…ts, mustClaim reuse

CMGS added 10 commits November 6, 2026 13:03

cut: drop Host.ResolvedName and the DNS resolver's name map

0c20db2

Nothing outside its own test read it, and no audit record carries a name, so every kept A/AAAA answer paid a mutex and a map write for nothing.

test: sandboxd table and helper reuse, synctest for the address log, …

2a38d5f

…no redundant build tags

docs: a tun disable during a capture sends the guest no tun_down

ffbdf36

review: the golangci-lint findings of this round's changes (err shado…

4985c85

…w, De Morgan, empty loop body)

CMGS changed the title ~~review: whole-repo static audit after #478 and #480 (billing order, tun addresses, silkd process groups, SDK cancel, docs)~~ review: billing order, tun addresses, silkd process groups, SDK cancel, docs Nov 6, 2026

CMGS merged commit a260c15 into main Nov 6, 2026
4 checks passed

CMGS deleted the review/post-478 branch November 6, 2026 05:54

CMGS mentioned this pull request Nov 6, 2026

sdk: follow a sandbox that moved to another node #483

Merged

Navigation Menu

Repository navigation

review: billing order, tun addresses, silkd process groups, SDK cancel, docs - #481

review: billing order, tun addresses, silkd process groups, SDK cancel, docs#481
CMGS merged 40 commits into
mainfrom
review/post-478

CMGS commented Nov 6, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

CMGS commented Nov 6, 2026

Bugs fixed (one commit each, each with a regression test that fails on the old code unless noted)

Cleanups

Numbers

Gates (macOS and a linux/arm64 container)

Coverage

Kept without code (to be added to #433)

Open for the owner

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant