Repository navigation
Conversation
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: api-compat, tests, docs, compat-flags, design-simplicity, jsg-gc, kj-style, memory-safety (no author changes in their files since the last review) Reviewed commit: 731a02d8 · github run |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Addressed in ca861d7: |
What's up with the PR description? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7644 +/- ##
==========================================
- Coverage 38.46% 38.46% -0.01%
==========================================
Files 864 866 +2
Lines 267594 267874 +280
Branches 25265 25286 +21
==========================================
+ Hits 102928 103030 +102
- Misses 150539 150702 +163
- Partials 14127 14142 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
883f94c to
cbbc329
Compare
|
The generated output of |
Record delivered callee-side JSRPC method exceptions when the method body throws or its returned promise rejects. Keep session teardown cancellation distinct and cover both paths with a streaming-tail regression test.
Bind delivered method failures to the originating session observer and tracer, release observation state on canceled calls, and strengthen cancellation and synchronous-error coverage.
Use the direct no-call session test to assert no failure reporting, and keep the streaming test focused on a delivered pending method canceled during teardown.
Keep the streaming regression focused on delivered exceptions while asserting that pure workerd capability release remains successful and exception-free.
Use separate service bindings and assert one callee invocation per method so sync, async, and pending-session traces cannot contaminate each other.
Directly verify exact-once failure reporting and post-cancellation rejection suppression, consolidate delivered-exception observation, and remove the release-only hanging trace fixture.
Exercise exact-once failure reporting and late-rejection suppression directly, consolidate exception observers, and remove the release-only hanging trace fixture.
Assert synchronous and asynchronous method failures on their observed separate callee invocations.
Keep exact-once assertions for the intentional sync and async failures while tolerating runtime-generated exceptions on the invocation.
Account for the runtime cancellation event explicitly and document why trace extraction failures do not replace delivered exceptions.
Accept the exact sync exception sequence with or without the known cleanup cancellation while rejecting every other exception shape.
Clone the current shared tracer claim for delivered-exception reporting after mapAddRef was removed.
Select Maybe's mutable map overload so the shared tracer reference can be cloned.
dc777bf to
731a02d
Compare
| : [ | ||
| { | ||
| name: 'Error', | ||
| message: HUNG_REQUEST_MESSAGE, |
There was a problem hiding this comment.
This is the bug we want to get rid of and this asserts that it's expected behaviour (...sometimes).
I'm not at all convinced this is directionally the right fix. I think we need to add a Rejected state in worker-rpc.h: kj::OneOf<Pending, Resolved, Disposed> state -> kj::OneOf<Pending, Resolved, Disposed, Rejected> state where Rejected drops the PendingPipeline reference that Pending is currently holding onto even after receiving a rejection. That is what's keeping the RPC session and the callee's request context alive leading to the hang detector kicking in on the callee.
4 participants
Summary
Record delivered JSRPC method exceptions on the callee invocation while preserving cancellation semantics for calls that never deliver an exception.
EdgeWorker integration MR: https://gitlab.cfdata.org/cloudflare/ew/edgeworker/-/merge_requests/15291
Final workerd SHA:
731a02d8aa4a988ada35b5fb9e8ea0f04732b038Rebased EdgeWorker SHA:
7ff2d455a3d56bae485845b68344e0a5eca86bfaRoot cause
When a JSRPC method threw and its exception was delivered to the caller, the callee invocation still followed the broader session lifetime. Graceful session teardown could therefore finalize the callee trace as
canceledwithout recording the application exception, even though the caller received and caught that exception.Exception reporting also needed to remain scoped to a live invocation. A rejection arriving after session cancellation must not report a request failure or mutate the canceled invocation.
Changes
canceledwith no delivered application exception.Behavior
Before:
outcome = canceledand no exception.After:
outcome = exception.canceledand does not report a failure.Tests
The final GitHub checks cover:
//src/workerd/api:jsrpc-call-plan-test@//src/workerd/api/tests:jsrpc-error-trace-test@The EdgeWorker MR adds production-harness buffered and streaming coverage through
//src/edgeworker/api-tests:js-rpc-exception-logging.ew-test@and its generated variants.Out of scope
Raw EdgeWorker streaming traces contain one entrypoint
jsRpcCallspan and one method call. Any duplicaterpcMethodsorrpcCallCountproduced later by WOBS aggregation is a separate downstream issue and is not changed here.