Try to debug "pushed external is not a byte stream".

[kentonv]

This adds some additional context:

• Whether it's in params or a return value.
• What exact type the capability is, that apparently didn't match.

[ask-bonk]

I've completed my review. The changes are a debugging-only change - adding a debugContext string literal parameter that flows through from the RPC deserialization layer into the error message for "pushed external is not a byte stream". This is a targeted diagnostic enhancement, not a behavioral change.

Let me assess the issues:

1. Correctness: The cap variable is passed by value to getLocalServer(cap) on line 140, and since capnp clients are copyable, cap remains valid in the else branch at line 144 where capnp::ClientHook::from(cap) is called. This is correct.

2. typeid usage: The comment on lines 149-150 correctly explains why hookRef is used instead of *hook directly. The compiler warns about side effects in typeid when applied to a dereferenced smart pointer.

3. kj::LiteralStringConst: Appropriately used - these are compile-time string literals, zero-cost to pass and store.

4. Debug context strings: "return"_kjc, "params"_kjc, "paramsNonClass"_kjc are clear and distinguish the three call sites.

5. No behavioral change: Error messages are enriched but the logic path is identical. No backward compatibility concern.

6. No security concern: This is purely diagnostic logging in error paths.

7. The getDebugContext() method on RpcDeserializerExternalHandler (line 161) is not const-qualified. This is a minor style nit but not actionable per review rules.

The one thing I want to double-check: is there any concern about typeid(hookRef).name() returning a demangled vs mangled name across platforms?

That's platform-dependent behavior but acceptable for a debugging message - the point is to give developers enough info to identify what type the hook actually is. The author (kentonv) clearly knows this.

This is a clean, well-scoped debugging change. No logic bugs, no safety issues, no backward compatibility concerns.

LGTM

github run

[codspeed-hq]

Merging this PR will not alter performance

✅ 72 untouched benchmarks
⏩ 129 skipped benchmarks¹

---

_{Comparing kenton/debug-external-pusher (0d86b66) with main (b322726)²}

Footnotes

1. 129 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. ↩

2. No successful run was found on main (9e6e5cb) during the generation of this report, so b322726 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[codecov-commenter]

Codecov Report

❌ Patch coverage is 60.86957% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.44%. Comparing base (b322726) to head (0d86b66).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/workerd/io/external-pusher.c++	35.71%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6648      +/-   ##
==========================================
- Coverage   66.49%   66.44%   -0.06%     
==========================================
  Files         405      405              
  Lines      117814   116107    -1707     
  Branches    19429    19430       +1     
==========================================
- Hits        78342    77143    -1199     
+ Misses      27896    27387     -509     
- Partials    11576    11577       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.