Spring cleaning: upstream Rook seams, fix the transformed-await gate, harden the OPFS pool - #76
Open
MiguelsPizza wants to merge 1 commit into
Open
MiguelsPizza wants to merge 1 commit into
MiguelsPizza wants to merge 1 commit into
Conversation
…ait gate, harden the OPFS pool Bugs fixed, each reproduced first with workerd as the oracle: - Transformed awaits released the input gate on every await (lost updates in concurrent storage-only read-modify-writes; implicit transaction committed at every await). src/gate.ts now resumes inline when the awaited promise settles while the actor still holds a lock in the critical section that captured it, and hops otherwise. Two new lanes run the conformance suite through the transform. - The OPFS SAH pool never rolled back a hot journal after a crash, and a retried install could delete the pool. installSqliteWasmHost() waits for the previous owner's handles and patches xCheckReservedLock. - Facet bundles bound 7 of the 12 scope-bound globals; facetScopeBanner() now generates the banner from the shared ACTOR_SCOPE_GLOBALS list. - A socket rehydrated after a Worker restart bypassed the output gate; the registry now hands the actor one stable AcceptedWebSocket wrapper. - withEnvAndExports scope leak, AlarmScheduler abort rejections, createActorContainer handle leak, FacetHost.abort unhandled rejection, stale rowsWritten after DDL, copyFrom sidecar window, MessagePortWebSocket listener skip and bridge hang. Moved upstream from Rook: facetScopeBanner, plugin resolution of the injected imports, @mcp-b/do-runtime/cloudflare-email, connectMessagePortWebSocket, offscreen ready()/replaceUnready(), createBrowserAlarmProjector + parseBrowserAlarmProjection, installSqliteWasmHost. Coverage: conformance 74 -> 81 rows on five lanes; three browser termination smoke specs; service-worker-kill alarm e2e; weak rows fixed. Maintenance: CI builds only what e2e does not; changeset:publish; sdk:pack -> .sdk-pack/; coverage-v8 dropped (the reinstall also moved chat/@chat-adapter to the vendored 4.38.0 pin and sourcemap-codec to 1.6.0 inside trace-mapping); docs corrected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This branch has not been deployed
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
Spring cleaning with three goals: move the runtime behaviour Rook (think-browser-host) still hand-writes upstream, close the coverage gaps an audit of the gating and termination paths found, and fix what those audits turned up. Every bug below was reproduced first, with real workerd as the oracle where it applies.
Bugs fixed
doRuntimeAwaitTransform, storage awaits and plain-value awaits hopped through a MessageChannel and resumed under a fresh input lock, so two concurrent storage-only read-modify-writes lost an update and the implicit transaction committed at every await. workerd holds the gate across both. The fix insrc/gate.tsresumes inline when the awaited promise settles while the actor still holds a lock in the critical section that captured it, and hops otherwise. Transformed awaits are about 7x faster as a side effect. The remaining differences from workerd are listed indocs/gating-coverage.md(Transform section). Two new conformance lanes run the whole suite through the transform on Node and Chromium.opfs-sahpoolVFS answersxCheckReservedLockwith "held" unconditionally, so a worker terminated mid-transaction left its uncommitted pages over committed rows on reopen (284 of 2304 rows in the new browser spec). NewinstallSqliteWasmHost()inbackends/sqlite-wasm.tspatches that answer for the one-connection-per-file contract this backend has.installOpfsSAHPoolVfsfails, and a busy worker keeps its sync access handles for about two seconds afterterminate(); 3 of 24 retried recoveries came back empty. The same helper waits for the previous owner's handles before installing.new WebSocketPair()in a facet fell through to the root's installed global, so a facet's frame went out while its commit was pending.facetScopeBanner()now generates the banner from the sharedACTOR_SCOPE_GLOBALSlist.getWebSockets()handed the actor the rawMessagePortWebSocket, sosend()andclose()did not wait for pending storage writes. The registry now wraps raw sockets in one stableAcceptedWebSocket.withEnvAndExportsleft a scope installed after a bad second argument;AlarmSchedulerraised two unhandled AbortErrors with a reject-on-abort timer;createActorContainerleaked open handles on a later failure; a throwingFacetHost.abortbecame an unhandled rejection;rowsWrittenreported a stale count after DDL;copyFromcould miss a journal appearing during export;MessagePortWebSocketskipped later listeners after a throwing handler and hung when the bridge threw.Moved upstream from Rook
@mcp-b/do-runtimefacetScopeBanner({ registry }),ACTOR_SCOPE_GLOBALSwxt.config.ts@mcp-b/do-runtime/gatealias@mcp-b/do-runtime/cloudflare-email(EmailMessagein Miniflare's shape)host/shims/cloudflare-email.tsconnectMessagePortWebSocket(bridge, url, fetch)(/browser)host/transport.tsready()/replaceUnready()background/host/offscreen-document.tscreateBrowserAlarmProjector(),parseBrowserAlarmProjectionalarms.worker.ts(which has the same stuck-idle-scheduler bug the review caught here)installSqliteWasmHost(sqlite3, options)(/backends/sqlite-wasm)installOpfsSAHPoolVfssitesCoverage added
blockConcurrencyWhile, sockets after hibernation, fetched bodies gated per chunk (hosts stream two chunks),deleteAlarm,list()options, plain-value awaits holding the gate. Weak rows fixed: the_cf_refusal now hits_cf_KV, rich values are asserted by content, the fixed sleeps in alarms and hibernation are gone, and the Node host installs the shared actor scope.Maintenance
CI builds only what the e2e scripts don't build themselves;
changeset:publishrunschangeset publish;sdk:packwrites to.sdk-pack/;@vitest/coverage-v8and its 14 orphans are gone from the lockfile; docs corrected (decisions 8/20, workerd-sync, gating ledger, migrations,ActorPorts).pnpm installafter the lockfile prune also movedchatand@chat-adapter/*to the 4.38.0 the vendored manifests have pinned since #69; the lockfile had drifted.Behaviour changes to know about
serveMessagePortWebSocketstakes one form,(bridge, url) => Promise<void>.stop()closes 1001 "MessagePort transport closed".NoModificationAllowedError("still held by another context"), instead of failing at once. The three example hosts install through the helper now; vibe's retry loop andsqlite-storage.tsare gone.Rook adoption
After the release, bump the catalog pin and its
minimumReleaseAgeExcludeentry together, then:wxt.config.tsfacetScopeBanner.facetScopeBanner({ registry: "__rookFacetScopes" }); keep the/worker/facets/check. The error text becomes "facet module has no scope named …" (nothing asserts it). Run the e2e: five more names are now bound in facet bundles.vite-aliases.ts3. Delete the
@mcp-b/do-runtime/gatealias (126-129); the plugin resolves it ahead of aliases.4. Point
cloudflare:email(134) at${dep("@mcp-b/do-runtime")}/dist/cloudflare-email.jsand deletehost/shims/cloudflare-email.ts. Rook's shim exposed.raw; the package keeps it under"EmailMessage::raw", and no binding reads it.5. Keep 92-101 and 130-133: the plugin does not resolve
cloudflare:*orasync_hooks, andwxt devpre-bundling only honours aliases.think-host.worker.ts6. Delete
host/transport.ts. At 1075-1077, insiderunWithWorkerActivity, callconnectMessagePortWebSocket(socket, message.url, (r) => routeAgentRequest(r, live.env, { onBeforeConnect: withWebSocketUpgrade }))from@mcp-b/do-runtime/browser. No open message is added (the relay's strict schema would close 1002 on it). Delete or update the "Agent socket routing" tests inagent-transport.integration.test.ts, which assert the old reason strings.7. Line 655:
installSqliteWasmHost(sqlite3, { name: "rook_actor", directory: …, clearOnInit: false, initialCapacity: 64 })and return the result directly. Make the same change in the three test fixtures underoffscreen/worker/host/fixtures/browser/(await-transform.worker.ts:29,web-push-delivery.worker.ts:175,persisted-state-inspection.worker.ts:25): the last one reopens a real actor directory withclearOnInit: falseinside a 10 sexpect.poll, which is exactly the retried-install hazard, and with the helper the poll becomes one await.8. Lines 515-522 need no change; the actor now receives the gated wrapper, and the
WebSocket.prototype.….call(ws)attachment workaround is no longer needed.background/host/offscreen-document.ts9. Pass the probe loop as
ready(it already rejects after 5 s) andunresponsiveLongEnoughToRebuildasreplaceUnready.ensure()becomes: maintenance check,await networkReady,offscreenDocument.ensure(). DeleteoffscreenHostPromiseand the rebuild block. Keep the 30 s pacing.alarms.worker.ts10. Keep the pool and
__rook_alarm_projection; the UPDATE-then-SELECT becomesnextGeneration(it must survive Worker restarts and draws must land in order). CreatecreateBrowserAlarmProjector({ nextGeneration, … }), wireprojectWake: wakes.projectWakeandfire(t) → wakes.acknowledge(t). Deleteprojection,projectionTail,projectedActiveDeliveries,projectionResult, the waiters list,settleWaitersandrejectWaiters. Line 67:installSqliteWasmHost(sqlite3, { name: "rook_alarms", directory: storageDirectory, clearOnInit: false, initialCapacity: 4 }).pool.11.
browserAlarmProjectionSchema(think-app/contracts/contracts.ts:199-204) is only used by tests, andBrowserAlarmCoordinatorvalidates every projection itself, so replace it withimport type { BrowserAlarmProjection } from "@mcp-b/do-runtime/browser/alarm-coordinator"(think-app already depends on do-runtime,package.json:51) and deletefixtures/alarm-bridge.contract.test.ts. No parser re-export is needed. Inalarm-worker.integration.test.tskeep one real-worker case; the projector rows are covered bysrc/browser/alarm-coordinator.test.ts. Keepalarm-facet-reentry.actor-backup.ts12. Line 62:
installSqliteWasmHost(sqlite3, { name: \rook_backup_${uuid}`, directory: …, clearOnInit: false, initialCapacity: 0 }).pool;pauseVfs,reserveMinimumCapacityand callback-formimportDb` still typecheck.docs/shim-surface.md13. Line numbers as on Rook main: delete the
host/shims/cloudflare-email.tsrow (37). In "Runtime imports" (54),cloudflare:emailnow aliases@mcp-b/do-runtime/cloudflare-emailand the gate import is resolved bydoRuntimeAwaitTransform. In "Facet bundles" (63), the banner now comes fromfacetScopeBanner; keep the row, sincecodeSplitting: falseandkeepNamesremain Rook's. Update "Agent sockets and routing" (78) and "MV3 offscreen readiness" (79) after steps 6 and 9, anddocs/prior-art.md:35, which still nameshost/transport.ts.Finally, run Rook's host browser lane (it runs the transform): any test that expected interleaving across a storage await now serialises like workerd.
Follow-ups (not in this PR)
./conformanceand./server/alarm-schedulersubpaths; makeWorkerLoadertype-only; un-export__gateandmarkWebSocketUsed; opaqueFacetTree; gateWritableStreamsinks.AlarmScheduler: a non-abort timer rejection surfaces as an unhandled rejection instead oftaskFailure()(pre-existing).rowsWrittenfor statements withRETURNINGstill uses thetotal_changes()delta (pre-existing).xCheckReservedLockand install-failure cleanup upstream; the helper's// ponytail:comment names the upgrade path.Verification
Full CI mirror run locally on the final tree (
pnpm install --frozen-lockfile,check:oracle,typecheck,test:unit,check:package, the five conformance lanes, the vibe production build,test:examples,sdk:pack): all pass. One workerd run flipped the order of two back-to-back requests in the §1.5 critical-section row; the assertion now accepts either order and rejects any interleaving, and the lane passed three further runs.🤖 Generated with Claude Code