Repository navigation
review: closing round 2026-09-28 - #79
Merged
Merged
Conversation
The logger-local rule binds a local only for two or more calls in one function.
The field was decoded and never read; the decoder ignores unknown fields, and the docs already say force is accepted and ignored.
… refused The store routes any key no warm pool serves to a node that advertises it as a promoted template and knows no namespace, so a key holder in one namespace could claim another namespace's built template by naming its full key, or clone it through a build's fromImage; the bare-name branch, which is namespace-scoped, is the only way to a built template.
The guest's stdout and stderr reached the same callback, so a shell warning printed before the printf ran (setlocale on an image without the locale) became part of the value and of every later RUN's environment and the template's defaults; stderr now goes to the build log only.
… promote Two builds of one name whose promotes both landed before either publish read the fleet deleted each other's template and both reported ready, leaving no holder; a publish now keeps every holder promoted after its own, so concurrent builds converge on the newest, and it fails when its own holder is gone. liveHolders returns the holders after Wait instead of in the same return statement, whose operand order the spec leaves open.
…lient bounded by ten minutes, not the request timeout The shared client's 10 s timeout covered a promote's snapshot, export and digest, which grow with guest memory (1.3 s for a 519 MiB record, past 10 s for a large tier or an S3 store), so the build failed at promote while the node finished it and swapped the template on one node with nothing published; the capture verbs now use a copy of the client whose bound is ten minutes, the callers' contexts bounding them further.
…t the sandbox's own token DialPort dialed url.Parse(base).Host in cleartext, so a node addressed by an https client_advertise origin, which every other verb reaches over TLS, could not be relayed at all (missing port, or cleartext into a TLS listener); it now takes the base as a host:port or an http(s) origin, defaults the scheme's port and wraps the connection in TLS for https. Client.DialPort takes the relay token: empty keeps the node token and the passive relay, the sandbox's own token wakes it and stamps activity.
…is never idle-hibernated Every build call went through the passive root-token relay, which never stamps activity, so a build longer than its pool's idle_hibernate_seconds was hibernated in the first gap between two calls and every later call answered 409 until the build timeout; the guest-port seam now carries a relay token, the build presents the claim's, and the create, metrics and fork hand-over keep the passive relay.
…ranger's signature never wakes a paused sandbox The signed path was placed by Locate and relayed with the sandbox's own claim token, which sandboxd treats as active: any request carrying a signature query, with no access token, woke a paused or archived sandbox and stamped activity before envd rejected the signature. The proxy now presents the node api_token on that path, and sandboxd's passive relay answers 409 for a paused sandbox without waking it.
…rn loops, one logger per function /code R9: LineFunc, ArchiveFunc and PublishFunc replace the func types spelled twice across e2bbuild and e2bcompat, managerBuilder the one spelled twice in sandbox-apiserver, serverOption the test option spelled five times; R1: FilesHash precedes the unexported var; R8: globBase uses slices.IndexFunc, the record sweep maps.DeleteFunc, the test fake slices.Clone; R4: createSandbox binds its logger once for two calls.
…, an informer-only inventory cache Claim read NodeCapacities once for the warm scan, again for the template scan and once more per redirect target; the candidate and address lookups are free functions over the one snapshot. The three template verbs converted PoolKey field by field between two structs of the same shape. The kubeinventory cache tuned its Get/List reader with UnsafeDisableDeepCopy and ReaderFailOnMissingInformer, but Source subscribes to informer events and nothing reads the cache, so the bench's copy and nocopy arms measured the same code.
… its own claim reader, the transport dials the relay itself probe carried the found Owner out of FirstHit through a mutex-guarded side variable although FirstHit is generic over the Owner value; NewResolver took a placed store every caller built from the inventory it also passed, then type-asserted it back; guestDialer and dialGuest were a type and constructor whose one instance forwarded one call to sandboxd.DialPort. The refusal comment and its test case named a wrong token, but the relay opens with the claim token the edge read from the node, so a 404 there is a stale cached owner.
…template views over NodeCapacities, a status page that converts only what it returns The relay environment lived in the e2b layer and reached e2bbuild through a Spec field its one caller always set to that map; it is sandboxd.RelayEnv beside NetRouteRelay. Spec.TTLSeconds was always the executor's own timeout. The template surfaces rebuilt the fleet view with ListNodes plus one NodeInventory read per node while every consumer reads Node, Pools and Templates, which NodeCapacities holds as one shared slice, so a stale node's templates now drop out of the list as they already dropped out of claims; resolveTemplate groups only the name it resolves. A build status poll converted every retained log line before paging, and Status copied the append-only log. The list filter now rejects on metadata before rendering a detail. The COPY archive path was spelled at the writer and the reader. Restated: the package doc, the fork result and Spec docs.
sandbox-apiserver, sandbox-e2b and sandbox-envd-proxy each registered --sandboxd-token and --sandboxd-token-file with their own help text; sandboxd.AddTokenFlags registers the pair beside TokenFrom, the way AddEnvdSecretFlag serves the envd secret.
…'s port The signed URL is envd's own /files download; on any other port a stranger's signature would have opened the node-token relay to whatever the sandbox serves there.
…re a build or snapshot runs sandboxd refuses a promote or checkpoint whose name is outside NameRe, so a build named outside it ran every step and failed at promote; StampedName now answers 400 up front with the same grammar.
…nd never tags a build a newer promote replaced Two builds of one name promoting on two nodes inside one publish's window could each delete the other's fresh promote by key and both report ready; the delete now names the digest the publish observed and sandboxd (91697b9) refuses a replaced record with 412, which the publish takes as already converged. On one node a newer promote replaces the record under the same key before the older build's publish, which then tagged the newer content with its own digest; the publish now checks the holder's digest against its promote and reports the build replaced.
…and the cancel hook closes the raw connection url.Host of https://[2001:db8::1] kept its brackets and JoinHostPort added a second pair, so every relay dial to such a node failed; the cancel hook captured the conn variable the TLS handshake then reassigned, a data race the race detector reports under a cancellation.
…replaced generation as 412
Between publishBuild's digest check and its label write a newer promote on the same node could replace the record, and the write then relabeled the newer content with the older digest; the write now carries the promote's digest, sandboxd (ad18c53) refuses a replaced record with 412, and the build reports itself replaced. Tag assign and delete pass each holder's observed digest for the same reason, and a failed publish tells the build status why.
TestAPublishSkipsAHolderReplacedSinceItWasObserved, TestABuildReplacedOnItsOwnNodeWritesNoTags and TestABuildWhoseTagWriteFindsANewerGenerationReportsReplaced; the first two were left out of that commit.
a164694 appended the publish error to the failure a caller sees, which names nodes; TestAFailedBuildNamesItsPhase pins the opposite. The reason stays in the process log, and the two new tests assert the phase.
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.
Closing quality round after the e2b drop-in series: fixes from the adversarial review, one review commit, the simplify pass and one docs line. Each commit stands alone.
Fixes
e2b/key in a create or a build's fromImage is refused, so a namespace never claims another namespace's built templateReview and simplify
sandboxd.RelayEnv), the build lease from the executor, template views over NodeCapacities (a stale node's templates drop out of the list as they already dropped out of claims), a status page that converts only what it returnsforcefield of the build start bodyDocs
Gates at the head: build/vet/asl clean on darwin and linux,
make lint8×0 issues.,make fmt-checkclean,go test -race11 packages ok, tidy/helm/generate clean. Kit: the full e2b matrix 113/113, the per-fix checks with -cb negative controls, create→201 A/B within noise.