fix(worker): never report an empty handler rejection reason - #14
Conversation
A rejected handler promise renders as
deployment dispatch failed error=Runtime(invoking deployment HTTP
handler: handler promise rejected: )
with nothing after the colon. That names neither the failure nor where to
look, and it is what an operator actually sees on a 500.
read_perry_string returns None only when the value is not a string, so a
rejection carrying an Error object -- which reads back here as a
zero-length string -- took the Some("") path and printed nothing. Empty
string and non-string are now distinguished, and the raw NaN-boxed bits
are always included so a bug report has something to correlate.
Found while investigating PerryTS/perry#8546, where two Next.js
applications in one in_process daemon leave the second serving 500 with
exactly this empty reason.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe plugin host now validates Perry string values and formats promise rejection reasons by type. Runtime initialization provides JavaScript-value stringification for non-string rejection values. ChangesPromise rejection formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR makes rejected handler failures report a non-empty diagnostic with raw value details; no actionable merge-blocking risk remains after normal checks. Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
…the real error
read_perry_string trusted a contract that does not exist. Its comment says
js_get_string_pointer_unified 'Returns 0 if not a string', but that function
deliberately returns the payload for a POINTER_TAG (0x7ffd) value too --
'used for cross-module returns' (perry value/nanbox.rs). Handing it an Error
object yields a non-null pointer that is NOT a StringHeader, and reading
through it is a wild read: it fabricated a one-character reason ("\t") out
of unrelated heap bytes, which then took the success path and printed as if
it were the rejection reason.
Check the NaN-box tag before dereferencing, and stringify a non-string
rejection with js_jsvalue_to_string so an Error reports its message.
On the case that motivated this (PerryTS/perry#8546, two Next.js apps in one
in_process daemon), the reason goes from an empty string to
handler promise rejected: TypeError: value is not a function
which is the documented signature of a GC rooting failure -- turning an
unattributable 500 into a named defect class.
1 participant
Problem
A rejected handler promise is reported as:
Nothing after the colon. On a production 500 that names neither the failure nor
where to look.
Cause
The hex fallback only fires when
read_perry_stringreturnsNone, which itdoes only for a value that is not a string at all. A rejection carrying an
Errorobject reads back here as a zero-length string, so it takes theSome("")path and prints nothing — the fallback that exists for exactly thiscase never runs.
Change
Distinguish the three outcomes and never render an empty reason:
<empty string> (raw 0x…)<non-string rejection value> (raw 0x…)The raw NaN-boxed bits are always included now, so a bug report has something to
correlate even when the value cannot be rendered.
Why now
Found while investigating PerryTS/perry#8546: two Next.js applications hosted
in_processin one daemon leave the second serving 500 with exactly this emptyreason. Both applications load and initialize; the failure is at dispatch. With
the reason blank there is nothing to attribute, and the remaining suspects are a
list of about seven process-global registries — this converts that guesswork into
a named failure.
Testing
cargo check -p coop-worker— clean.Deliberately does not include the
rusqlite0.37 -> 0.39 bump needed tobuild against current Perry
main; that has to land atomically with aperry-main.lockpin bump (bumping it alone breaks against the current pin — Iverified this, cargo refuses with the same
libsqlite3-syslinks conflict in theopposite direction). Separate PR.
Summary by CodeRabbit