Tunnel debug port RPC errors to callers#6507
Conversation
|
The generated output of |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR prefixes debug port assertion messages with jsg.Error: so they tunnel through capnp RPC. The tunneling mechanism itself works correctly — tunneledErrorType() will recognize the prefix and mark these as non-internal errors.
However, there's one issue that affects all the changed assertions:
- [MEDIUM] KJ assertion formatting artifacts leak into user-visible error messages. When KJ assertion macros have multiple arguments, extra args are formatted as
; varName = valuewith the variable name as a label. This means users will see messages likeService not found:; serviceName = myServiceinstead ofService not found: myService. TheJSG_REQUIRE/JSG_REQUIRE_NONNULLmacros avoid this by pre-formatting withkj::str()— the same approach should be used here.
This review was generated by an AI assistant and may contain inaccuracies.
|
The review has been posted on PR #6507. Here's a summary of the finding: [MEDIUM] KJ assertion formatting artifacts leak into user-visible error messages. The PR correctly uses the |
Merging this PR will not alter performance
Comparing 
Footnotes
|
penalosa
force-pushed
the
penalosa/debug-port-error-messages
branch
from
13cab4a to
77be143
Compare
penalosa
force-pushed
the
penalosa/debug-port-error-messages
branch
from
b4ad6fe to
88909c0
Compare
danlapid
left a comment
danlapid
left a comment
There was a problem hiding this comment.
Make sure to clean up commits before merging
penalosa
force-pushed
the
penalosa/debug-port-error-messages
branch
from
88909c0 to
efdb69b
Compare
The debug port's getEntrypoint() and getActor() methods used plain KJ_ASSERT_NONNULL for error cases (service not found, entrypoint not found, DO class not found). KJ exceptions without a jsg. prefix get sanitized by decodeTunneledException() into a generic "internal error; reference = <id>" before reaching the caller. Prefix these assertion messages with "jsg.Error:" so they tunnel through capnp RPC and the caller receives the actual error description (e.g. "Entrypoint not found: NonExistentEntrypoint").
The nonexistent-service test triggers a capnp RPC ERROR log that cannot be captured by KJ_EXPECT_LOG (the RPC layer logs through the event loop's ExceptionCallback chain, not the test's). Remove the negative test case; the positive cases (entrypoints, props, actors, JS RPC) still provide good coverage of the debug port RPC path.
Return a kj::Exception directly instead of throwing via KJ_ASSERT_NONNULL in getEntrypoint(). Throwing synchronously causes capnp RPC to log the error through an ExceptionCallback chain that KJ_EXPECT_LOG cannot intercept in tests. Returning a rejected promise avoids this while still delivering the same error to the caller.
…str() Revert the KJ_IF_SOME + return pattern — KJ_ASSERT_NONNULL with kj::str() works fine. The original test failure was a message mismatch, not a kj::str() issue. Remove kj::str() wrapping where there's no interpolation, per review.
penalosa
force-pushed
the
penalosa/debug-port-error-messages
branch
from
efdb69b to
b3f6921
Compare
2 participants
Summary
getEntrypoint()andgetActor()assertion messages withjsg.Error:so they tunnel through capnp RPC instead of being sanitized to genericinternal error; reference = <id>messages.Background
The debug port is used by the dev registry to proxy service bindings and Durable Object bindings across
wrangler dev/vite devprocesses. When a caller references a non-existent entrypoint or DO class, the KJ assertions in the debug port handler produce descriptive errors (e.g. "Entrypoint not found"), butdecodeTunneledException()treats them as internal errors and replaces the message with an opaque reference ID.The
jsg.Error:prefix marks these as user-facing errors that should be forwarded as-is, matching the pattern used elsewhere in the codebase (e.g.JSG_REQUIRE_NONNULL).