fix(platform): pin error identity in the host contract - #334
Conversation
Review follow-ups on the shard-boundary fix. The property the previous commit restored was pinned only by unit doubles that implement the very workerd behaviour being claimed — they cannot fail if the platform differs, and they asserted nothing for any other host. `ShardHost` also said nothing about what happens to a thrown error, so a new `platform-<target>` could regress it and stay green. The contract now states it on both `runSerialized` and `transaction`, and the conformance suite asserts it: a sentinel carrying `code`/`status` must come back by identity, and the host must still serve the next caller afterwards. Verified under real workerd via `@lunora/do`'s `runInDurableObject` project — the case fails against the unfixed adapter and passes against the fixed one, which is the assurance the doubles could not give. The in-memory reference host and the node host both already satisfy it. Also from review: - Drop the explicit type argument on `blockConcurrencyWhile`, which was discarding the closure's return type and forcing an `as never` to recover it. `as const` on the discriminant lets it infer. - Attach a failed commit/rollback as `cause` when the handler's error takes precedence, so a broken storage layer cannot vanish behind an application error. - Rename `symbolDeclaredUnreachable` to `declaredInHandlerModule`. After the previous commit the predicate returns true for exported declarations, which are reachable — the name had become wrong and its docblock existed mostly to say so. - Cut the duplicated incident narrative from the comments, keeping the platform behaviour that cannot be derived from the code and dropping the past-tense bug story. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYvgKochPxnrCCtKB1tXGf
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thank you for following the naming conventions! 🙏 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe change preserves original closure errors in Cloudflare ChangesHost error preservation
Codegen helper rename
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ShardHost
participant CloudflareHost
participant blockConcurrencyWhile
participant Closure
participant TransactionStorage
ShardHost->>CloudflareHost: invoke runSerialized or transaction
CloudflareHost->>blockConcurrencyWhile: execute serialized operation
blockConcurrencyWhile->>Closure: run callback
Closure-->>blockConcurrencyWhile: return value or original error
CloudflareHost->>TransactionStorage: commit or rollback transaction
CloudflareHost-->>ShardHost: return value or rethrow closure error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
Thank you for confirming the Contributor License Agreement! 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/platform-cloudflare/src/cloudflare-host.ts`:
- Around line 198-200: Update the error-handling flow around the thrown value to
wrap the cause check and assignment in a try block, allowing failures when
reading or setting cause on frozen or non-extensible errors. Ensure the
surrounding logic always throws thrown.value so the original closure error is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf8ca21-d39f-4204-81f9-4f769543563f
⛔ Files ignored due to path filters (1)
packages/platform-cloudflare/__tests__/cloudflare-host.transaction.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**
📒 Files selected for processing (4)
packages/codegen/src/discover-functions.tspackages/platform-cloudflare/src/cloudflare-host.tspackages/platform/src/conformance/suite.tspackages/platform/src/shard-host.ts
Merging this PR will degrade performance by 11.75%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 1 shard × 1000 rows (single round-trip) |
2.7 ms | 3.1 ms | -11.75% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/shard-host-contract (a56ddd4) with alpha (a4503b5)2
Footnotes
-
64 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
alpha(fb05fe7) during the generation of this report, so a4503b5 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Review catch on the `cause` attachment, and a good one: the line meant to stop a storage fault vanishing could itself destroy the error it was decorating. A frozen or sealed `Error` — a module-level `Object.freeze(new Error(…))` sentinel is a real pattern — rejects a `cause` write with a `TypeError` under strict mode. That `TypeError` then propagated in place of the error the handler actually threw, which is precisely the loss this whole wrapper exists to prevent. The write is now guarded and best-effort, in a named `attachCause` helper — which also keeps `transaction` under the cognitive-complexity limit that the inline nesting broke. `defineProperty` rather than assignment states the intent (add an own property to this instance) and does not read as an accidental parameter mutation. Covered both ways: a platform failure is attached as `cause` when the closure's error can carry one, and a frozen error still arrives by identity when it cannot. Verified the second test fails without the guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYvgKochPxnrCCtKB1tXGf
|
Fixed in I reproduced it before accepting: Taken with two changes to the proposed diff:
Covered both directions: a platform failure is attached as |
Review follow-ups to #332. These were pushed to that branch before it merged, but the merge took the first three commits only, so they did not land.
#332 fixed the behaviour; this fixes the assurance and the naming.
The property was pinned only by doubles that assumed it
cloudflare-host.transaction.test.tsasserts against hand-written doubles that implement the very workerd behaviours being claimed — flatten-on-rethrow and abort-on-rejection. A double cannot fail if the platform differs from the author's model of it, and it says nothing about any other host.ShardHostitself was silent on what happens to a thrown error, so a newplatform-<target>could regress it and stay green.The contract now states it on both
runSerializedandtransaction, and the conformance suite asserts it: a sentinel carryingcode/statusmust come back by identity, and the host must still serve the next caller afterwards.Verified under real workerd through
@lunora/do'srunInDurableObjectproject — the new case fails against the pre-#332 adapter and passes against the fixed one, which is the assurance the doubles could not give. The in-memory reference host and@lunora/platform-nodeboth already satisfy it unchanged.Also
blockConcurrencyWhile. It was discarding the closure's return type and then forcing anas neverto recover it;blockConcurrencyWhile<T>(cb: () => Promise<T>): Promise<T>infers fine (@cloudflare/workers-types@5.20260724.1/index.d.ts:675).as conston the discriminant removes both.cause. When the handler's error takes precedence, a broken storage layer was being discarded entirely — the opposite of the "the platform's own failure is surfaced unchanged" rule stated three lines above it.symbolDeclaredUnreachable→declaredInHandlerModule. After fix: coded errors survive the shard boundary, plus two codegen losses #332 the predicate returnstruefor exported declarations, which are reachable; the name had become wrong and most of its docblock existed to apologise for that.Checks
platform45,platform-cloudflare31,codegen1045,do519 plus 56 under workerd,platform-node52.api:checkclean — no public surface change.🤖 Generated with Claude Code
https://claude.ai/code/session_01CYvgKochPxnrCCtKB1tXGf
Summary by CodeRabbit
Bug Fixes
Tests
Documentation