review: phase-A round of the e2b drop-in series - #58
Merged
Merged
Conversation
WithClaimRouting joins WithWatchPollInterval instead of sitting between the SandboxdClientFactory type and its constructor. Three comments go: the section label above the synthesized-Sandbox labels, whose four constants carry their own godoc; the apiKeyHeader line, which the name and its one use carry; and the floating note in types.go, which the package doc already states by naming the OpenAPI contract.
… trimmed The two e2b logs handlers differed only in their reply, so one handler takes the reply and the mux registers it twice; the reply is built once at registration. The warm-pool driver mapped pools onto warm counts in two places and duplicated scale.PoolCapacityFromInfo in a third; warmByKey over PoolCapacity serves both call sites. The list filter's template guard goes, since poolImage of an empty string is empty. A page drops the entries at or before its cursor before sorting, which sorts less for the same result. The connect failure message is one constant. Two tests reuse pageOfList instead of their own copies of it.
…ory sources snapshot-placement.md still said the client does not chase redirects and the store answers 503 on any redirect, which #52 changed. The package doc named only the Kubernetes source; #54 added the mesh source. The ListNodes contract no longer says cache-fed, since the mesh source answers from a polled snapshot.
…ity miss The fleet HTTP client set no dial timeout. On a routed network a dial to a dead node is black-holed until the client's request timeout fires, and that error carries no dial OpError, so the store read it as a possibly delivered claim and answered a hard error after ten seconds instead of trying the next candidate; a dead node's inventory stays a candidate, so every create that sampled it paid that. A one-second dial timeout surfaces as a dial OpError, which claimUndelivered already classifies as nothing sent. Probe on the store's transport: before, took=2s undelivered=false (Client.Timeout exceeded while awaiting headers); after, took=1s undelivered=true (dial tcp 10.255.255.1:7777: i/o timeout). The table test pins the classification.
sandboxd reports owner_addr as the client origin when client_advertise is set, and the apiserver's Create stamped it through AddressIPs onto Status.PodIPs; net.SplitHostPort on https://host yields the scheme as the host, so the pod IP read https. The scheme is cut before the host is taken.
A discovered sandboxd that restarts gracefully leaves the gossip mesh on shutdown and rejoins only after its reconcile, while its sandboxes keep running. The source dropped an unnamed member at its first failed tick, so every rolling restart hid the node for about two ticks: List lost its sandboxes, watch emitted Deleted then Added, Get answered 404 and the lifecycle verbs failed to resolve the node. An unnamed non-seed now leaves the membership only after MaxStale consecutive failed ticks, the same window its snapshot survives, so a restart inside it is invisible. Two rules the tests did not pin are pinned: publish keeps the member with the fewest failures for a key, and a seed wins the dedupe over the gossiped spelling of its own node. The smoke tool shares one fleet HTTP client the way cmd/sandbox-e2b does.
--e2b-template-aliases landed in #56 without a chart value, mount or flag, so on a chart deploy the SDK's default Sandbox.create() still asked for base and got 503. apiserver.e2b.templateAliases holds the file's lines; the chart renders them into a ConfigMap, mounts it and passes the flag when the value is set.
detailFor renders startedAt as the read time when an entry carries no claimedAt, and the v2 page cursor keyed on that rendering, so such a sandbox changed its cursor key every wall-clock second: an ascending walk repeated it without end and a descending walk skipped the rest of its block. The cursor now keys on the raw creation time, where an absent one is a fixed zero, and the rendered startedAt is unchanged. Entries without claimedAt exist only for claims that outlived an upgrade from a sandboxd that predates the field.
GET /templates lists each pool image under names, which on e2b is exactly the set the alias lookup accepts, but the lookup knew only the alias table, so Template.exists(image) was false while Sandbox.create(image) worked. The lookup now also answers 200 for an image a pool advertises. The docs say reconnect by id needs SDK 2.6 or later: older JS SDKs read the envd token from the detail read, which carries no secret here.
…t and the mains The driver carried six-line justifications on a const, on a predicate and inside a goroutine; each now states its one fact. Comments that restated a name or a struct literal go, and the two-line godocs that fit one line are one line. No code changes.
The flag takes a file path, as its siblings --e2b-api-key-file and --sandboxd-token-file do, and no release carries the old name.
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
1 participant
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.
Summary
The phase-A end review round of the e2b drop-in series, run at repository scope on master
2ef6a7c(the batch #50–#56) and rebased onto4e10f5c2(#57). Four style readers over the 30 files the hygiene ledger listed as due, three adversarial judges (e2b API contract, store concurrency and protocol, meshinventory after its fix), the four /simplify lenses over the batch diff. Every finding was adjudicated against the source; the applied ones are below, the kept ones are in the round report.Twelve commits, each one thing:
review:layout move and three comment cuts (store, e2b surface).review:one logs handler for both routes;warmByKeyoverPoolCapacityreplaces two copies of the pool mapping; the list filter's redundant guard; a page drops entries before sorting; the connect failure message is one constant; two tests reusepageOfList.docs:snapshot-placement.mdsaid the client never follows redirects (feat: the claim loop follows sandboxd claim redirects #52 changed that);pkg/scale's package doc names both inventory sources;ListNodesno longer says cache-fed.fix:the fleet HTTP client gets a one-second dial timeout. Without it a black-holed dial (a dead node on a routed network) ended as the client's request timeout, which carries no dialOpError, so the store read it as a possibly delivered claim and answered 500 after ten seconds instead of trying the next node. Probe on the store's transport: beforetook=2s undelivered=false (Client.Timeout exceeded), aftertook=1s undelivered=true (dial tcp 10.255.255.1:7777: i/o timeout). Table test pins the classification.fix:AddressIPscuts a client origin's scheme; withclient_advertiseset the kube path stampedhttpsas a pod IP.fix:an unnamed mesh member leaves only afterMaxStalesilent ticks. A discovered sandboxd that restarts gracefully leaves the mesh on shutdown and rejoins after its reconcile, so "unnamed and failed ⇒ leave at once" hid the node for ~2 ticks on every rolling restart (List dropped its sandboxes, watch emitted Deleted/Added, Get 404, verbs failed). Two rules the tests did not pin now are: publish keeps the fewest-fails member per key, and a seed wins the dedupe. Each new test fails on the previous code.fix:the chart passes the template alias file (apiserver.e2b.templateAliases→ ConfigMap → flag); on a chart deploySandbox.create()still asked forbaseand got 503.fix:the v2 page cursor keys on the raw claim time; an entry withoutclaimedAtrenderedstartedAtas the read time, so its cursor key changed every second (an ascending walk repeated it, a descending one skipped its block). Regression test fails on the old key.fix:GET /templates/aliases/{alias}also answers for an image a pool advertises, thenamesthe template list reports, soTemplate.exists(image)agrees withSandbox.create(image). Docs: reconnect by id needs SDK 2.6 or later.review:one-line comments in the warm-pool driver, the sandboxd client and the mains (+25/−70).review:--e2b-template-alias-file, named like--e2b-api-key-file; no release carries the old name.docs:an unnamed mesh peer leaves after the same ticks its snapshot survives.Numbers
2ef6a7cis all PASS (.hygiene/e2b-matrix-2026-09-27-phaseA/, README table and 18 raw files); the fixes here are unit-pinned and change no verified path's behavior except the dead-node and restart cases described above.Gates (my run, GOWORK=off)
go build ./...,go vet ./...and the three tagged harnesses: okgo mod tidy -diff: cleanasl -forwarder=false ./...: 0 findings on darwin and linuxgo test -race -count=1 ./...: 10 okmake lint fmt-check: 8/80 issues.helm lint,helm templatewith and withouttemplateAliases: ok