Skip to content

fix(orchestrator): reject snapshot readiness after uffd exits - #3706

Closed
eren23 wants to merge 1 commit into
e2b-dev:mainfrom
eren23:fix/uffd-start-readiness
Closed

eren23 wants to merge 1 commit into
e2b-dev:mainfrom
eren23:fix/uffd-start-readiness

Conversation

@eren23

@eren23 eren23 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

The UFFD ready channel also closes when startup fails. If its exit watcher has not canceled the start context yet, loadSnapshot returns nil despite the recorded failure and the next Firecracker call reports the cause under the wrong step.

Pass the existing UFFD exit result through Resume and check it after readiness. Return the original cause if the handler already exited, including a clean exit. This preserves GuestMemoryError classification and ENOMEM for memory refusal, and leaves the channel close behavior unchanged for prefetch waiters.

A deterministic socket-stub test fails on unchanged main for failed and stopped handlers, and passes with this change. It also checks healthy readiness and typed-cause preservation. Targeted readiness tests pass three shuffled race runs. The full FC, UFFD, server, and unprivileged sandbox suites pass; one NBD-requires-root constructor case skips. The root-container version of that case hangs without NBD devices, also reproduced on unchanged upstream. A pre-existing metric-counter test fails only when repeated within one process, also reproduced on upstream. CI-style changed-lines lint and formatting pass. Independent good-cop and adversarial audits, followed by a separate judge, found no correctness blockers. No live VM or deployment validation; no claim of reproducing a successful create with a dead handler.

AI assistance: Codex implemented the fix, tests and validation from the saved readiness investigation.

@ryan-sana

Copy link
Copy Markdown
Contributor

Hi @eren23

I wonder if you have seen particular issues with this problem. While I appreciate the fix, but I would like to know if there are any serious issue with this problem at this current stage and shape.

We would be happy to look into the PR but it is necessary to know how relevant and timely the problem can be.

I close this PR but feel free to reopen and continue the discussion.

Usually the best way would be creating one issue and one related PR instead of just creating a PR.

Thanks,
Ryan

@ryan-sana ryan-sana closed this Oct 21, 2026
@eren23

eren23 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi Ryan, sorry for skipping the issue-first step. I investigated this while tracing #3689, but should have separated the readiness behavior from the user impact that the memory-refusal fix already addresses.

I reran the original kill/immediate-create scenario on a fresh Linux KVM host using Embed's pinned orchestrator v0.16.202610201713-75c97c0d1da, with CREATE_MEMORY_REFUSAL=true: 8 vCPU / 16 GiB host, 4,608 × 2 MiB hugepages, an 8 GiB sandbox, and 6 GiB of guest memory touched before each kill. All 40 immediate creates succeeded, and all 40 replacement guests ran a command successfully. The SDK HTTP log contains exactly 41 create requests including initial setup, all returning 201; there were no client retries. Orchestrator logs recorded 40 guest-memory refusals with stopping=1, followed by successful placement retries. Median create time was 456 ms, maximum 551 ms.

Separately, a controlled Unix-socket probe on public main 293df0848e25b31587d472627ada9fa91b74d628 still reproduced the narrower behavior: after a simulated successful snapshot-load HTTP response, publishing the UFFD exit error and then closing readiness let loadSnapshot return nil in 300/300 iterations across three race-enabled runs. However, the immediately following setTxRateLimit call preserved both ENOMEM and the typed GuestMemoryError in every iteration. A real malformed UFFD handshake also closed readiness while retaining its exit error. Those are component probes, not evidence of a failed public create.

So I haven't demonstrated an additional user-facing failure or urgency for this PR on the current Embed build. The live failures were attributed to snapshot load, and recovered; the later TX-rate-limit attribution appeared only in the controlled probe. I'll leave this closed rather than reopen it on the strength of the readiness contract alone. If a concrete failure turns up, I'll open an issue with that reproduction first. Thanks for asking for the impact check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants