Skip to content

review: claim-id index for claims and fork children; comment budget and log levels - #49

Merged
CMGS merged 3 commits into
masterfrom
review/round-2026-09-25
Sep 25, 2026
Merged

CMGS merged 3 commits into
masterfrom
review/round-2026-09-25

Conversation

@CMGS

@CMGS CMGS commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Whole-repo review round after #46. It covers the ledger-due files; #48 is reviewed on its own branch.

Fix

a74ef7d: a claim and a fork child are indexed by claim id, so e2b lookups ask their node first.

  • Before: Claim recorded only the name key and Fork recorded nothing. The e2b surface and the envd proxy resolve by claim id, so the first by-id call after every e2b create or fork swept the fleet's inventories. Before the node had published, it also asked every node.

  • Now: Claim also records the claim id, and Fork records each child by name and by claim id, as docs/scaling-design.md already says the claim-time index does.

  • Cost: one more index write per claim and two per fork child, each a map write under an uncontended mutex. No I/O is added, and the by-id lookup that follows no longer fans out.

  • Tests:

    • TestGetByClaimIDAsksTheClaimingNodeFirst
    • TestAForkChildIsLookedUpOnItsNode

    Both fail without the fix.

Review

  • 0ac84d9: a node whose inventory cannot be read is skipped. It now logs at Warn, the level of a degraded read, with the error in the message. Error stays for failures.

  • 1d6c607: godoc and inline comments in:

    • cmd/sandbox-apiserver
    • examples/lifecycle
    • pkg/e2bcompat
    • pkg/envdproxy

    now fit the one-line budget. Contract text that already lives in docs/e2b-compat.md and docs/configuration.md is dropped from the godoc; key scoping, for example, is docs/e2b-compat.md line 31.

Numbers

  • Production: +3/−0 for the fix.
  • Review commits: +23/−100, nearly all of it comment lines.
  • Comment lines: +21/−98.
  • Tests: +40/−1.

Kept

asl's forwarder advisories for pkg/envdproxy are both kept:

  • withTarget pairs with targetFrom and keeps the context key private.
  • sandboxHost names the SDK's host format and is the inverse of routeOf.

Gates

All run with GOWORK=off:

  • make fmt-check: darwin and linux.
  • make lint: 6 × 0 issues. on each GOOS.
  • go test -race -count=1 ./...: 9 packages ok.
  • asl ./...: darwin and linux. The only findings are the two kept advisories above.

The branch and #48 merge without conflict in either order (git merge-tree).

Hardware verification

Pending: the two-host round runs the e2b and envd-proxy lanes on master + #48 + this branch.

… ask their node first

Claim recorded only the name key, and Fork recorded nothing, while the e2b surface and the envd proxy resolve by claim id. So the first by-id call after every e2b create or fork swept the fleet's inventories and, before the node published, asked every node. Claim now also records the claim id, and Fork records each child by name and by claim id, as scaling-design.md's claim-time index already states.
@CMGS

CMGS commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Two-host hardware A/B, 2026-09-25. Master 34fd93c against the record build: master plus #48 (a428ff8) plus #49, with the patch identical to 1d6c607. Both nodes had sandboxd and vk-sandbox behind request taps.

  • SBL-09. An e2b sandbox is read by id right after its create.
    • master: in 6 of 6 samples, the read also asked the node that did not claim it.
    • record build: that node was asked in 0 of 6 samples.
  • SBL-10. One name is claimed on both nodes, with the index on the paused claim. A pinned list selecting phase=Running is then issued.

The record build also carried the rest of the sandbox chain (41-sbx: 28/28 PASS) with no regression.

Observation, not new with these PRs: kubectl get --watch prints the object as ADDED twice. kubectl lists first, and the store's watch emits Added for its initial view; master has the same code. docs/usage.md does not state that a watch opened after a list replays the listed objects.

(Edited: an earlier version of this comment said the master watch reported the paused claim. It reported the running one.)

@CMGS
CMGS merged commit 40c09dc into master Sep 25, 2026
2 checks passed
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