Preload fanout snapshot start pages safely - #324
Merged
Conversation
sjmiller609
marked this pull request as ready for review
August 6, 2026 14:54
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f53443d. Configure here.
hiroTamada
approved these changes
Aug 6, 2026
hiroTamada
left a comment
Contributor
There was a problem hiding this comment.
reviewed — the ordering and overall approach make sense. a few things worth tightening:
Tests
server/cmd/wrapper/snapshot_start_page.go:17-81andserver/cmd/wrapper/main.go:242-257— could we add lifecycle coverage proving that seed Envoy and its bootstrap are always removed before snapshot readiness, and that the applied marker is withheld when any post-fork readiness probe fails? These are the two core invariants introduced here, but the current tests stop below them.
Performance
server/cmd/wrapper/main.go:232-257— this adds fresh Envoy startup to the synchronous fork path. Could we include before/after p50/p95 fork-to-ready measurements to validate the expected latency and confirm adequate headroom inside the 30-second handoff timeout?
Nits
server/cmd/wrapper/fork_identity.go:50— consider naming the five-second response margin alongsideApplyTimeoutrather than leaving the timing relationship as a magic subtraction.
sjmiller609
added a commit
that referenced
this pull request
Aug 6, 2026
## Summary - guarantee seed Envoy cleanup on every snapshot-start-page exit and cover process/bootstrap teardown - cover the applied-marker gate across each post-fork readiness probe - name the five-second fork identity response margin alongside the shared apply timeout ## Performance 20 sequential headless Firecracker forks per version (8 vCPU), measuring the fork request through the successful fork-identity response and verifying public CDP returned 200 on every run. The Envoy fixture used a static listener to isolate local process startup/readiness from xDS and network variance. | version | fork-to-ready p50 | fork-to-ready p95 | identity handoff p50 | identity handoff p95 | | --- | ---: | ---: | ---: | ---: | | before `759635d` | 293 ms | 492 ms | 69 ms | 96 ms | | after #324 (`0503e9f`) | 1,564 ms | 1,708 ms | 1,340 ms | 1,498 ms | All runs passed (20/20 per version). The after p95 leaves 28.3 seconds of headroom in the 30-second API timeout and 23.3 seconds inside the wrapper's 25-second readiness deadline. But it's OK that this is slower because we are migrating off of envoy and the hot pools avoid hitting this case. Also we have another tier of idle pool which we can start filling with fanout. ## Testing - `cd server && go vet ./...` - `cd server && go test -race $(go list ./... | grep -v /e2e$)` - `cd server && make build` - `git diff --check main...HEAD` <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches fork handoff timing and when the applied marker is written; incorrect gating could stall or falsely signal fork readiness to the API. > > **Overview** > Tightens **fork-identity readiness** so the applied marker is only written when every post-fork probe succeeds, and documents the **5s response margin** next to `ApplyTimeout` in `forkidentity`. > > `writeForkIdentityAppliedMarker` centralizes that gate; readiness polling is split into **`waitProbesReady`** so tests can assert per-probe behavior (cdp, chromedriver, envoy) without a full boot. > > **Snapshot start page** seed Envoy teardown is guaranteed on all exit paths via a defer, with **`cleanupSeedEnvoyWith`** for unit tests of stop + bootstrap removal. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 688cd10. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: sjmiller609 <7516283+sjmiller609@users.noreply.github.com>
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
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
about:blankfallbackWhy
Snapshot forks inherit Chromium's rendered tab state. Preparing that state while the template is built avoids external navigation and stale network error pages on the synchronous fork path.
Testing
cd server && go vet ./...cd server && go test -race $(go list ./... | grep -v /e2e$)cd server && make buildgit diff --check main...HEADNote
Medium Risk
Touches VM boot ordering, fork identity control plane, and Envoy/CDP startup paths where mis-timed handoffs could block forks or leave wrong S2 identity; changes are localized but operationally sensitive.
Overview
Preloads DuckDuckGo into Chromium while building a fork-wait template so snapshot forks inherit a loaded tab instead of doing external navigation on the hot path. The wrapper may briefly run seed Envoy, navigates via new CDP helpers (
BrowserWebSocketURL,DispatchStartURLAndWait), then tears down seed Envoy and its bootstrap before continuing.Fork identity sequencing changes: stale payload/applied state is cleared and the ready file is written in
armForkIdentityWaitonly after snapshot prep and beforekernel-images-apiand public CDP are exposed. The applied marker is written after identity env apply, post-identity Envoy init, and concurrent readiness probes succeed within a deadline derived from sharedforkidentity.ApplyTimeout(also used by the API handoff wait). Readiness probing now fails the boot if required services do not come up in that window.Docs/comments for
appliedS2Streamare updated to match the new ready-file semantics.Reviewed by Cursor Bugbot for commit f53443d. Bugbot is set up for automated code reviews on this repo. Configure here.