Repository navigation
id(worker-poc): TS isolate host for plaza guests — scaffold + counter smoke (#2271) - #3726
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b2a6ca62a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .toArray(); | ||
| const guest = await loadHostedModule(MANIFEST, COUNTER_WASM as WebAssembly.Module, SEED); | ||
| if (rows.length > 0) { | ||
| guest.restore(rows[0].snapshot as Uint8Array); |
There was a problem hiding this comment.
After a journaled step, /cold-start or a real eviction restores a SQLite BLOB, which workerd returns as an ArrayBuffer (also handled explicitly in pkgs/pkarr-worker/src/index.ts:75–83). This type assertion does not convert it to a Uint8Array. Consequently, copyInput() sees an undefined .length, copies no snapshot bytes, and calls plaza_restore with length zero. Reproducing with the counter resets its count to 0 while retaining the journaled step number. Wrap the stored buffer in a Uint8Array before restoring it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: journal snapshots pass through asBytes() (Uint8Array passthrough, ArrayBuffer wrapped) before plaza_restore. — queue worker lane D, task 5598.
| const guest = await loadHostedModule(MANIFEST, COUNTER_WASM as WebAssembly.Module, SEED); | ||
| if (rows.length > 0) { | ||
| guest.restore(rows[0].snapshot as Uint8Array); | ||
| this.step = rows[0].step as number; | ||
| } | ||
| this.guest = guest; |
There was a problem hiding this comment.
When two /step requests arrive at a fresh or forced-cold object, both can pass the null check and independently instantiate a guest while this await yields. Each request then updates its own initial model, and both results are journaled: a concurrent counter reproduction produces steps 1 and 2 with count=1 for both, permanently losing an increment. Durable Objects permit asynchronous interleaving; Cloudflare documents blockConcurrencyWhile for protecting initialization. Share an initialization promise or otherwise serialize initialization, including coordination with /cold-start.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: lazy initialization is shared through a single guestPromise (cleared on failure), and all request handling — including /cold-start — is serialized through a per-object promise chain, so two requests can never instantiate competing guests. — task 5598.
| const mem = new Uint8Array(this.ex.memory.buffer); | ||
| if (outputPtr + len > mem.length) throw new Error("guest returned output outside linear memory"); | ||
| return mem.slice(outputPtr, outputPtr + len); |
There was a problem hiding this comment.
If a guest returns a negative i32 output pointer, the upper-bound check can accept it and slice() interprets it as an offset from the end of memory. For example, pointer -3 and length 2 return the two bytes near the memory's end instead of rejecting the invalid range. This silently corrupts views, records, or snapshots and differs from sandbox.rs, which casts the pointer to unsigned before validating it. Normalize the pointer with >>> 0 before both the bounds check and the slice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: readOutput normalizes the pointer with >>> 0 before the bounds check and the slice, matching sandbox.rs. — task 5598.
| @@ -0,0 +1 @@ | |||
| {"description":"PoC (#2271): plaza_* example worlds running unmodified in a Worker isolate, journal equality vs the native wasmtime host.","devDependencies":{"typescript":"^5.6.0","wrangler":"^3.90.0"},"name":"id-worker-poc","private":true,"scripts":{"dev":"wrangler dev","smoke":"node smoke/counter-smoke.mjs","typecheck":"tsc --noEmit"},"type":"module"} No newline at end of file | |||
There was a problem hiding this comment.
The new Wrangler development dependency is declared only in this npm manifest, leaving it absent from the shared Nix development environment. pkgs/id/AGENTS.md explicitly requires new development dependencies to be added through nix-common.nix; TypeScript is already provided there, but Wrangler is not. Add Wrangler through the required shared dependency definition so the supported Nix environment includes the tool needed for this PoC.
AGENTS.md reference: pkgs/id/AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: wrangler added to pkgs/id/nix-common.nix next to typescript, per pkgs/id/AGENTS.md. — task 5598.
| const { participant, event } = (await req.json()) as { participant: number; event: string }; | ||
| guest.update(participant, enc.encode(event)); |
There was a problem hiding this comment.
A request containing {"participant":null,"event":"inc"} passes the unchecked type assertion and increments the counter because guest.update() coerces the participant to zero. The subsequent INSERT fails on journal.participant's NOT NULL constraint, but neither the guest mutation nor the step increment is reverted. Reproducing against SQLite leaves count 1 with an empty journal; the next valid request persists step 2/count 2, incorporating an event that failed and was never journaled. Validate the request fields before calling the guest, and discard or restore the mutated guest if persistence fails so unsuccessful requests cannot contaminate later snapshots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: /step validates participant (non-negative integer) and event (non-empty string) before any guest mutation and returns 400 otherwise; if the journal INSERT throws, the guest is discarded so the next request restores from the journal — an unjournaled event cannot contaminate later snapshots. — task 5598.
1b2a6ca to
3cf4ae5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf4ae5747
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const snapshotHash = await sha256Hex(snapshot); | ||
| this.step += 1; | ||
| this.sql.exec( |
There was a problem hiding this comment.
When concurrent /step requests reach an already-initialized guest, this await allows another update before the first snapshot is journaled. If the digests complete out of order, journal step numbers follow digest completion rather than guest-update order. A Node/SQLite reproduction with reversed digest completion persisted step 1 with count=2, followed by step 2 with count=1, making the latest journal snapshot stale. This is separate from lazy-initialization concurrency and remains possible even with a shared initialization promise. Serialize the entire update/hash/insert operation so journal order matches guest evolution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1a5683: the entire update -> snapshot -> hash -> journal-insert sequence runs inside the per-object serialized chain (fetch delegates to a promise-chain mutex), so journal step order always matches guest evolution order. — task 5598.
All six Codex findings on PR #3726 dispositioned in code: - SQLite BLOB snapshots normalized via asBytes() before plaza_restore (workerd returns ArrayBuffer; restore copied zero bytes before). - Lazy guest init shared through one promise; all request handling serialized through a per-object promise chain, so update -> hash -> journal insert is one unit and journal order matches guest order. - /step validates participant/event before mutating the guest; on journal-insert failure the guest is discarded so an unjournaled event cannot contaminate later snapshots. - guest readOutput normalizes the i32 output pointer with >>> 0 before bounds check and slice, matching sandbox.rs. - wrangler declared in pkgs/id/nix-common.nix per AGENTS.md. Verified: npm run smoke PASS, tsc --noEmit clean (laptop).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1a5683f0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| update(participant: number, event: Uint8Array): void { | ||
| const { ptr, len, allocationLen } = this.copyInput(event); | ||
| try { | ||
| this.model = this.ex.plaza_update(this.model, participant | 0, ptr, len); |
There was a problem hiding this comment.
When PlazaGuest receives a participant ID above the signed i32 range, participant | 0 wraps it instead of matching the native host's clamp to i32::MAX in sandbox.rs. In particular, 4294967296 becomes participant zero. This changes state evolution for existing guests: with roc-chat.wasm, updating with event "x" appends input for participant 2147483647, but not for 4294967296, because roc-chat/main.roc routes participant zero through its host-event handler. The currently wired counter ignores participant IDs, so its smoke test does not expose this discrepancy. Clamp the ID as the native host does before passing it to the guest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13ac9c8 (pushed to this branch): PlazaGuest.update now clamps the participant ID to i32::MAX before the | 0 conversion, matching sandbox.rs (i32::try_from(participant).unwrap_or(i32::MAX)). Verified behaviorally with roc-chat.wasm: participant 4294967296 now appends input exactly like 2147483647, and no longer collapses into participant 0 — regression pinned in smoke/participant-clamp.mjs (npm run participant-clamp), passing. — task 5598, lane F
PlazaGuest drives only the plaza_* ABI with the native hosts alloc/free and output discipline; hosted loader refuses portability=native and declared memory over the 64MB ceiling, naming the reason. Durable Object host journals every step (snapshot/view/records/hash) in SQLite and restores via plaza_restore after a forced cold start. Counter smoke (Node 24, laptop): seed 0 + inc x3 gives count=0..3 in view and records; guest-level cold start reproduces the step-3 snapshot hash 4e07408562... exactly; both loader refusals asserted. Native-runner comparison and tic-tac-toe sequence are the next increments.
All six Codex findings on PR #3726 dispositioned in code: - SQLite BLOB snapshots normalized via asBytes() before plaza_restore (workerd returns ArrayBuffer; restore copied zero bytes before). - Lazy guest init shared through one promise; all request handling serialized through a per-object promise chain, so update -> hash -> journal insert is one unit and journal order matches guest order. - /step validates participant/event before mutating the guest; on journal-insert failure the guest is discarded so an unjournaled event cannot contaminate later snapshots. - guest readOutput normalizes the i32 output pointer with >>> 0 before bounds check and slice, matching sandbox.rs. - wrangler declared in pkgs/id/nix-common.nix per AGENTS.md. Verified: npm run smoke PASS, tsc --noEmit clean (laptop).
…2271) Native reference runner (tests/worker_poc_reference.rs) drives the roc-counter and tic-tac-toe scripted sequences through the native Sandbox and emits per-step (view, records, snapshot) lines; worker-poc/compare diffs them against the isolate host: 13/13 steps equal, snapshot sha256 included, each host asserting its own forced cold start mid-sequence. do-cold-start.mjs proves the DO level under wrangler dev: /cold-start restore and a full dev restart (fresh DO instance) both restore step 2 from the SQLite journal, and step 3 matches the native hash. Branch rebased onto current main (the old base snapshot carried a misplaced derive in cli.rs that no longer compiles; main has it fixed).
a1a5683 to
c378b57
Compare
|
New increment pushed (c378b57, branch rebased onto current origin/main): the native-vs-isolate equality harness and the DO-level cold start — the last open checklist items for #2271.
@chatgpt-codex-connector please re-review the new head. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Record clarification (task 5598, lane F): the six earlier Codex findings on this PR (snapshot buffer conversion, lazy-init serialization, output-pointer normalization, wrangler in nix-common.nix, step-input validation, update/hash/journal serialization) are fixed on this branch in e68f4cd — verified at head by direct code read. The thread replies citing a1a5683 pointed at a parallel commit that was never on this branch (its parent is the original scaffold); the fixes themselves are present here via e68f4cd. The one live finding — participant ID clamp — is now fixed in 13ac9c8 with a regression smoke. Note on ownership: this PR is the queue task 5598 branch ( @codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PoC for #2271 (from #2057): one example world running unmodified in a Worker isolate, journal equality vs the native wasmtime host.
This increment (scaffold + isolate-side smoke):
pkgs/id/worker-poc/src/guest.ts—PlazaGuestdrives only theplaza_*ABI with the native host's alloc/free + output discipline (sandbox.rs is the reference); bytes path (Node) and module path (workerd) both supported.src/manifest.ts— hosted loader refusesportability=nativeand declared memory over the 64MB hosted ceiling, each with the reason named (acceptance 4).src/worker.ts— one Durable Object per world; SQLite journal is the source of truth;/cold-startforces restore viaplaza_restore.smoke/counter-smoke.mjs— prebuiltcounter.wasm, seed 0 +inc×3.Verified (laptop, Node 24): view/records show
count=0..3per step; guest-level cold start (restore step-2 snapshot →inc) reproduces the step-3 snapshot hash4e07408562bedb8b60ce05c1decfe3ad16b72230967de01f640b7e4729b49fceexactly; both loader refusals asserted. Smoke: PASS.Remaining (later increments on this branch): native reference runner comparison over the same sequence; tic-tac-toe scripted sequence; DO-level forced cold start under
wrangler dev.Pre-registered differences (fuel vs CPU-time; refusal-at-load vs trap-at-run) are on #2271. No changes to the native sandbox, guests, or the plaza ABI.
Refs #2271
Need help on this PR? Tag
@codesmith-botwith what you need.