Repository navigation
Enable minimal snapshot roundtrip - #7880
Conversation
|
💬 Approved with comments — No correctness or security bugs found in the serialize, restore and link flow, and the pipeline stays behind the STARTUP_SNAPSHOT autogate. The two warnings are about missing focused tests and missing JSG guidance for the new snapshotClone() contract. Neither one states a production impact. Findings: 2 warnings · 3 info The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality:
|
9188836 to
fc87cab
Compare
|
@dcarney-cf PTAL |
|
overall looking good. I want to take a second pass through before stamping and I'm a bit concerned that the PR includes no tests. I'm not anticipating any specific issue but I'm curious if you've tested what happens when GC hits during initial startup -- essentially, just want to make sure there are no odd edges lurking. Like I said, there's nothing immediately coming to mind but the lack of tests here just makes me nervous. I'll take another review pass (both with my eyes and with an agent) before sign off. |
|
clanky review |
b4bd282 to
db2b811
Compare
|
My AI reviewer identified a handful of issues worth investigating. Consider with the appropriate caution for AI-reviews.
My note: I'm not sure I agree with that minimum fix. We should try to handle the listeners in the snapshot.
There are a couple of other smaller findings the agent flagged but this is enough for this round. Some of these may be false positives but a bit difficult to determine given the lack of tests. |
db2b811 to
63677c3
Compare
Thanks James, so
For others properties like Iterator.*. I installed special value that fails the zygote when its top level code tries to access them and remove them before taking snapshots.
About tests. The idea here was to enable snapshot for all configurations and remove the feature because it passes for all WD tests. Every test worker will create snapshot and then use it. |
63677c3 to
9cb1180
Compare
|
fwiw on the |
thanks Cormick, I'll support it then in the next one |
|
Also had a couple patches needed to get tests passing in edgeworker; I know you're already adding support for the new module registry in the next PR, so I think the only particularly interesting commit is the jsg::Function one? |
Nice! I'll check the |
9cb1180 to
1dfa6b1
Compare
4b60b57 to
107848e
Compare
The zygote Worker that evaluates top-level code for the startup snapshot reports failures into its own ErrorReporter, but a KJ exception thrown while it compiles, evaluates or serializes still propagates out of factory_new_worker() and fails the real Worker. Catch it around the zygote's script and Worker construction and log it at INFO like the ErrorReporter case, so a snapshot that cannot be produced only costs the snapshot.
docs/startup-snapshot.md walks through the pipeline end to end: when the zygote runs, what it does before and inside createSnapshotBlob(), what the Worker started from the snapshot restores and in which order, what travels in the SnapshotArtifact, how to make a type transferable, what happens on each kind of failure, how console output is deferred, and which tests cover what. The root AGENTS.md links it from "Where to Look" and "Other Documentation".
The zygote's errors said "snapshot PoC:"; they now say "startup snapshot:". Also remove a temporary INFO log from settleFinalizationRegistriesForSnapshot(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes the startup-snapshot commits needed on current main:
- KJ_IF_SOME inside JSG_WITHIN_CONTEXT_SCOPE lambdas now reads as a
dangling else, so stashPerIsolateBootstrapForSnapshot() tests the
Maybe directly and removeRestoreTimeGlobalsBeforeSnapshot() checks
restoreTimeGlobalUsed before entering the context scope.
- Drop an unused local in bootstrapFromScripts().
- snapshot-test and the guide still called
WrappablesSerializeMetadata::verify(), since renamed
dropUnreachableWrappables().
- clang-tidy: a redundant `typename`, size() > 0 for empty(), and
NOLINTs where a raw v8::TryCatch is reset right after its probe and
where apiWrapperDeserializer's `{}` is needed because its default
constructor is explicit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
failClosed() throws a JavaScript error. When the zygote's top level was being terminated, by a startup CPU limit for instance, that throw replaced the pending termination and let the top level run on. Return without throwing while execution is terminating; the recorded use still fails the zygote. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bakeSnapshot() in snapshot-test destroyed its WrappablesSerializeMetadata after leaving the isolate lock. Its entries root cppgc shims through cppgc::Persistent, which must be released by the thread holding the isolate lock once a v8::Locker has been used; V8 builds with cppgc DCHECKs abort with "IsCreationThread()". Declare it inside the lock, as Worker::Worker does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test asserted that the output mentions "fallback" anywhere, which the service list alone satisfies. Check that the skip line is the one for the `fallback` service. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bundled Node.js code commonly keeps the function createRequire()
returns at the top level. A Worker whose top level does could not be
started from a startup snapshot:
- The callback of every jsg::Function is one of FunctorCallback's two
trampolines, which were not in the isolate's external-reference
table, so CreateBlob() trapped ("Unknown external reference").
- v8::Function::New() keeps the function's data, the
WrappableFunction's opaque wrapper, in a function template of its
own, which V8 serialized in the isolate part of the snapshot: the
wrapper lost its CppHeap pointer without the API-wrapper serializer
callback being called, so the restored function aborted when called
("wrapper type mismatch"), and a debug V8 aborted while serializing.
Register the trampolines, and patch V8 (patches/v8/0052) to keep a
function created by v8::Function::New(), its template and its data in
the context snapshot. The wrapper then goes through the API-wrapper
serializer like any other, and a WrappableFunction is transferred if
the function opts in with `Impl clone() const`; one that does not, such
as AsyncLocalStorage.bind()'s, makes the zygote reject the snapshot.
createRequire()'s functions are functors that copy their referrer.
startup-snapshot-functions-test calls a top-level createRequire()
function in a Worker started from a snapshot, and checks that a
top-level AsyncLocalStorage.bind() function makes the Worker start
without one.
Adapted when picked onto the rebased PR: the V8 patch is renumbered 0052,
after main's own 0051, and the tests detect a snapshot start with
unsafe.startedFromStartupSnapshot() now that the zygote compiles every
binding.
(cherry picked from commit f0248be78ba5f73b2bcf7c7a6bb2b9a74d89c84c)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A Worker started from a startup snapshot re-registered the zygote's
top-level listeners by calling the global's addEventListener with
{once}. That had three problems:
* It ran whatever the top level left in globalThis.addEventListener. A
wrapper ran a second time, and a replacement that does nothing lost
the listener. One that is not a function, or that throws, failed
the Worker's startup outside any error handling, with no fallback.
* The passive option was dropped, so preventDefault() took effect in a
listener registered as passive.
* Re-deriving the callback re-read an object listener's handleEvent,
which the top level may have reassigned since it registered it.
The zygote now records what each registration derived (identity,
callback and receiver) together with once and passive. The restored
Worker rebuilds the callback through a type handler that
Worker::Api::tryGetEventListenerCallbackTypeHandler() supplies, and
inserts it with EventTarget::restoreSnapshotListener(). No JavaScript
runs, and the special-event warning is no longer printed a second time.
An Api without the type handler keeps a zygote that registered a
global listener from producing a snapshot.
startup-snapshot-listeners-test covers a passive listener, a replaced
addEventListener and a reassigned handleEvent.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two gaps in the handling of the properties V8 adds to a context only when restoring it from a snapshot: * Deleting a FAIL_CLOSED property (`delete globalThis.WebAssembly`) does not call its getter, so the zygote succeeded, but V8 re-added the property on restore: the restored Worker had what the top level had removed. The deletion is now recorded alongside the Error.stackTraceLimit state, and the restored context deletes what V8 added. * Temporal and Date.prototype.toTemporalInstant were missing from RESTORE_TIME_GLOBALS. V8 installs them in InitializeGlobal_harmony_temporal(), which it skips in the zygote, and with JSObject::SetAccessor(), which replaces a property of the same name: a top-level polyfill was silently overwritten on restore, and then deleted by newContext() for a Worker without the temporal flag. In a context with Temporal the names are now FAIL_CLOSED. In one without it they stay absent, as in a normal context, and a top level that defines one fails the zygote. Date.prototype joins the holders that must stay extensible, read from a new v8::Date rather than a context data slot, since the embedder-data array has no slot to spare. startup-snapshot-globals-test covers the deletion, a Temporal probe and polyfill with and without the temporal flag, and a frozen Date.prototype. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With nodejs_compat_v2 the `process` and `Buffer` globals are accessor properties whose values live on the ServiceWorkerGlobalScope, not on the global object, and a Worker started from a snapshot gets a fresh one. A top-level `globalThis.process = ...` or `globalThis.Buffer = ...` was therefore lost: the restored Worker resolved the module defaults again. The zygote now pins what its global scope holds for the two (ServiceWorkerGlobalScope::getSnapshotState()) into SnapshotArtifact::globalScopeStateIndex, and the restored global scope takes it over (restoreSnapshotState()). startup-snapshot-globals-test covers both assignments, and a top-level process.env write. That one still falls back: evaluating node:process reaches UrlUtil, a Rust JSG resource, and Rust resource types have no snapshot transfer yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Top-level code commonly keeps a TextDecoder, which made the zygote reject the snapshot. A decoder between inputs is its encoding and its options, so the clone is a fresh decoder of the same kind (ICU or legacy). A decoder that a streaming decode left with part of an input pending is not transferred; TextDecoder now tracks whether its last decode streamed. Decoder::getEncoding() becomes const so that the const snapshotClone() can read it. startup-snapshot-wrappables-test keeps a fatal UTF-8 decoder and a windows-1252 one at top level, and checks that a mid-stream decoder falls back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A restored JSG object is built afresh, by its type's constructor or a snapshotClone(), so any jsg::Ref it holds points at a new object rather than at the one behind the restored wrapper. For Crypto that broke identity: a top-level `const subtle = crypto.subtle` was no longer `crypto.subtle` at request time, because the restored Crypto constructed its own SubtleCrypto. Wrappable gains snapshotReferences(), which names the objects an instance refers to in the zygote, and snapshotRelink(), which hands a restored instance the restored instances of those once every wrapper is linked. recordLiveWrappables() resolves the references to snapshot slots (SnapshotArtifact::LiveWrappable::references), and WrappableDeserializeMetadata::relinkReferences() runs right before the slots are released. Object gets two helpers for the casts the private Wrappable base needs. Crypto uses them for `subtle`. startup-snapshot-wrappables-test checks the identity of crypto, crypto.subtle and navigator across the snapshot. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without multiple pointer-compression cages every isolate shares the default IsolateGroup's read-only heap. V8 builds it from the read-only snapshot of whichever isolate the group starts with, and rebuilds it when the group empties, as it does between a zygote's isolate and the Worker's. A read-only snapshot taken by a zygote differs from the built-in one only in its trailer, the zygote isolate's next SharedFunctionInfo unique id, but: * DEBUG builds CHECK that every isolate of a group sets the read-only heap up from a snapshot with the same checksum. A zygote created after a Worker restored from a custom snapshot rebuilt the heap therefore aborts, which is what failed the linux-arm debug job of every multi-Worker @all-autogates test. * An isolate that joins an existing read-only heap takes its next id from the snapshot the heap came from. A Worker restored from a custom snapshot while built-in isolates hold the heap, the usual case in a busy process, hands out again the ids of the SharedFunctionInfos the zygote created; V8 keys debugger bookkeeping by them. patches/v8/0053 writes the id as a fixed-width uint32, leaves it out of the read-only heap's checksum, and has an isolate that joins an existing read-only heap continue past its own snapshot's id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
setModuleFallbackCallback() is installed on the Worker's isolate only,
after the zygote has run. A zygote whose top level tolerates a failed
import, `try { await import(x) } catch {}` or a guarded require(),
therefore baked the failure in, and the Worker started from the
snapshot never consulted the fallback service. A Worker configured with
one now starts without a snapshot, which also makes the restore's
handling of a pinned module with no registry entry unconditional.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A subclass of a stateless type inherits jsgSnapshotStateless, so a startup snapshot would record it by tag and rebuild it as a fresh instance, silently dropping the subclass's own state. The snapshotClone() path already guards against the same mistake; the stateless one now fails to compile instead. The stateless types that were not final are made final; none has a subclass in workerd or edgeworker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
restoreConstructorTemplatesFromSnapshot() ran only when an Isolate constructor instantiated the default type wrapper itself. An isolate built with instantiateTypeWrapper = false and given its wrapper later through instantiateDefaultWrapper() kept fresh templates, so every wrapper restored from a snapshot would fail the receiver check of its own type's methods. instantiateDefaultWrapper() now does the restore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe the listener replay in C++, the restore-time globals' deletion replay and Temporal handling, the process/Buffer state, references between transferred objects, jsg::Function transfer, the module fallback exclusion, the final requirement on stateless types and the read-only heap patch, and list what the tests now cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
compileModules() calls tryTakeSnapshotModule() for every bundle ES module, which cloned the module's path for a lookup in the seed table even in a Worker that was not started from a snapshot, where the table is always empty. Return right away then, so that a Worker the pipeline does not touch does no extra work per module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The zygote captured the intrinsics V8 adds restore-time properties to (Error, Iterator, Iterator.prototype) in three new ContextDataSlots, which raised the highest slot to 12 and required building V8 with v8_embedder_data_array_size 13. An embedder whose V8 is built with the default 10, as edgeworker's is, then failed to compile the static_assert on ContextDataSlot::MAX_DATA_SLOT. The zygote now holds the captured intrinsics as handles on IsolateBase, tied to the context they came from, and removeRestoreTimeGlobalsBeforeSnapshot() stores them in the record it already pins into the snapshot, where replayRestoreTimeGlobalsAfterLoad() finds them for the restored context. The handles are released once the record is pinned, or with the isolate. The slots are removed and the V8 embedder data array is back to 10. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
snapshot.h only forward-declares Wrappable, but defined ~SnapshotArtifact() inline, and destroying `liveWrappables` destroys the kj::Own<Wrappable> a transfer can hold. A translation unit that includes snapshot.h without the complete Wrappable failed to compile, as does parsing the header on its own (edgeworker's build does). The destructor is now defined in snapshot.c++. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A zygote whose top level leaves a generator, async function or async
generator suspended, for instance an async ReadableStream pull() that
awaits a promise a later request resolves, could not be restored
correctly. CreateBlob(kClear) discards the bytecode of every function
it can recompile, including one a suspended generator will resume in,
so the restored Worker resumed it in a lazily recompiled copy of that
bytecode. Debug builds with --debug-code abort there ("Missing bytecode
array from function", from ResumeGeneratorTrampoline), which failed
src/tests/streams/iocontext:iocontext-ts@all-autogates downstream.
V8's own bytecode flushing never discards a resumable function's
bytecode, for this reason.
Patch V8 (patches/v8/0054) to add
SnapshotCreator::HasSuspendedGenerators(), which looks for a suspended
generator among the objects CreateBlob() would serialize, and have
createSnapshotBlob() reject the snapshot when it finds one, just before
CreateBlob(). The Worker then starts without a snapshot.
CreateBlob(kKeep) is no alternative: it keeps compiled code that the
context snapshot cannot reference.
snapshot-test checks that a suspended async function and a generator
that has not started fail the zygote, and that a generator function and
a finished generator go through.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The macOS CI job's API AST dump (//src/workerd/tools:dump_api_ast) segfaults in Apple clang 21 (Xcode 26.6) since the binding types gained their snapshotClone() overrides; upstream clang 21 dumps the same file without trouble. Defining the overrides in the .c++ files keeps their bodies out of the headers the dump parses. What they do is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Apple clang 21 (clang-2100.1.1.101) segfaults in -ast-dump=json when
it visits a requires-expression requirement whose substitution
failed: the dumper treats the stored substitution diagnostic as an
Expr. jsgRecreateForSnapshot() and kSnapshotRecreateByTag both use
`requires(Lock& lock) { U(lock); }`, which fails substitution for
types without a Lock& constructor, crashing the macOS dump_api_ast
action. Use the equivalent type traits instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Apple clang 21 (clang-2100.1.1.101) segfaults in -ast-dump, which
the macOS dump_api_ast action uses, on an instantiated
requires-expression whose compound requirement failed substitution:
the dumper reads the substitution failure as an expression.
snapshotClone()'s inline `{ f.clone() } -> std::same_as<Impl>` check
fails for every function-pointer Impl. A named concept keeps the
requires-expression out of the instantiated body.
This also reverts the previous commit, which replaced different
requires-expressions that do not trigger the crash.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
makeCloneable() refuses a class that inherits from a built-in V8 serializes itself, and built that list at bootstrap time with `typeof SharedArrayBuffer`. The bootstrap runs in the startup-snapshot zygote, where SharedArrayBuffer is a restore-time global that fails closed when touched (V8 adds it only once the snapshot is restored), so every zygote failed and no Worker was started from a snapshot. Leaving it out of the list in the zygote would instead bake a list without it into the snapshot. Recognize SharedArrayBuffer.prototype by its own @@toStringTag, which reads no global and is the same in the zygote and in a restored Worker. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test starts some 25 Workers, each through a zygote and then from its snapshot. Under edgeworker's debug build, which runs workerd's tests with a 15-second medium timeout, its @all-autogates variant takes about 5 seconds and timed out twice on loaded runners. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
59b4089 to
83b5d71
Compare
|
@dbezhetskov would you mind opening a PR with the new module registry support as soon as it's done? It's blocking some stuff downstream |
|
Enable startup snapshot consumption
Workers that pass the eligibility check now start from the startup snapshot produced by the zygote Worker instead of evaluating their top-level code again. Everything the consumer needs from the zygote is carried through the snapshot: the bindings object, the main-module namespace, resource constructor templates, the per-isolate bootstrap state, the wrapper tag internal field, and live wrappables, which are cloned via Wrappable::snapshotClone() before CreateBlob() and linked to their deserialized JS wrappers on load.
The pipeline stays behind the STARTUP_SNAPSHOT autogate and, next to the existing python / ScriptSource / wasm exclusions, now also requires the typescript_implemented_streams compatibility flag.
Any failure in the zygote, whether a KJ exception while compiling, evaluating or serializing, or an error reported to its ErrorReporter (for example a live object whose C++ type has no snapshotClone()), is logged at INFO and the Worker starts without a snapshot.