Repository navigation
fix(pic): reject a deferred call's promise when the call fails - #315
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation correctly propagates failures and includes appropriate regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes deferred actor calls so execution failures reject normally instead of leaving promises pending.
Changes:
- Replaces the async Promise constructor anti-pattern with an async function.
- Adds regression coverage for rejected canister calls.
| File | Description |
|---|---|
packages/pic/src/pocket-ic-deferred-actor.ts |
Propagates deferred-call errors correctly. |
packages/pic/tests/src/create-actor.spec.ts |
Tests rejection for a missing update method. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lwshang
approved these changes
Sep 29, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Sep 30, 2026
Closes #142 `makeLive` takes an optional options object: - `artificialDelayMs`, the minimum delay between rounds. It was hard-coded to `0`. It delays calls that wait for live mode to execute them: calls through the HTTP gateway, and submitted calls polled with `ingressStatus`. Actors, deferred actors, `updateCall` and `awaitCall` execute their rounds themselves, so they aren't delayed. Measured: an update through an agent took ~700 ms with a 300 ms delay, and a polled submitted call took ~1 s with a 1 s delay, but the others took 3–10 ms either way. - `httpGateway`, the same settings as `PocketIc.create`'s `httpGateway` (IP address, port, domains, HTTPS), for the gateway `makeLive` starts. Passing options to an instance that's already live, or `httpGateway` to one created with its own gateway, throws instead of ignoring them. If the gateway fails to start, e.g. because its port is taken, auto progress is stopped again, so the instance isn't left live without a gateway. For the reentrancy case in #142, live mode isn't needed: calls submitted with a deferred actor before any is awaited interleave at their `await`s. A new `examples/reentrancy` example shows it (its test for the vulnerable withdrawal relies on #315, since the second withdrawal traps) with a bank canister: a withdrawal that checks the balance before awaiting a transfer and deducts it after pays out twice, the fixed one pays out once, and a regular actor making the calls one after another misses the bug. `running-tests.mdx` gets short sections on testing interleaved calls (linking the example) and on live mode, and the example lists now mention both Jest and Vitest. The example is built with the Motoko recipe v5.1.0, which compiles through `mops build`, so it's defined in `[canisters]` of `mops.toml`; `.gitignore` now ignores `.mops/` at any depth for its build output. The other examples move to v5 in a follow-up. The `makeLive` JSDoc said the instance needs an NNS subnet for `fetchRootKey`. That's not the case on PocketIC 16 (checked with an agent against an instance without one), so it's removed. The broken code examples of `makeLive` and `stopLive` are fixed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
A deferred actor method returns a function that executes the call. That function wrapped the call in
new Promise(async resolve => …), so when the call was rejected or trapped, the error was lost: the promise never settled and the test hung until its timeout, with the reject only surfacing as an unhandled rejection. It's now a plain async function, so the promise rejects with the call's error.Found while testing a reentrancy scenario, where the second of two interleaved calls traps. The new test calls a method the canister doesn't have and expects the rejection. Before the fix, it timed out.
🤖 Generated with Claude Code