sql: fix the build after #32089 × #37275 (LISTEN/NOTIFY error path) - #38628
Conversation
…he connection's error path #32089 landed calling JSGlobalObject::report_active_exception_as_unhandled, which #37275 removed, so main does not compile. A NOTIFY channel/payload string the VM could not make (OOM, a worker's termination) is now this connection's failure like every other conversion in on(): on_notification returns AnyPostgresError::JSError, which takes the pending exception. No-Verification-Needed: build fix; cargo check of the full crate graph passes, the sql suite needs a Postgres server
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughNotification handling now propagates JavaScript string-conversion failures as ChangesPostgreSQL notification error propagation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward build fix routing NOTIFY string-creation failures through the same AnyPostgresError path every other on() arm uses. The inline nit about js_error_to_postgres is a convention point only; create_utf8_for_js is zero_is_throw so both spellings are behaviorally identical today.
Extended reasoning...
Overview
#32089 (LISTEN/NOTIFY) landed calling JSGlobalObject::report_active_exception_as_unhandled, which #37275 had removed, so main stopped compiling. This PR changes on_notification and its helper channel_name_js to return Result<_, AnyPostgresError> instead of swallowing/reporting the exception locally, and adds ? at the single call site inside PostgresSQLConnection::on(). Net ~30 lines in one file.
Security risks
None. No new inputs, no parsing changes, no trust-boundary changes — this only reroutes an existing OOM/termination error path from a removed reporter to the connection's existing failure path.
Level of scrutiny
Low. This is a mechanical build fix that makes the NOTIFY arm match every sibling arm in the same match (decode_internal(...)?, ok_or(...)?, etc. all already propagate AnyPostgresError out of on()). Both changed functions have exactly one caller each, both updated. The early-return Ok(()) cases (no JS wrapper / no callback registered) preserve the previous no-op semantics.
Other factors
- Verified downstream:
AnyPostgresError::JSErrorroutes througherror_jsc.rs→global.take_exception(JsError::Thrown), which is exactly what #37275's contract expects (the pending exception is left for one taker). - The one inline nit (use
crate::jsc::js_error_to_postgresinstead of|_| AnyPostgresError::JSError) is a valid convention point but not a behavior difference here —BunString__createUTF8ForJSis[[ZIG_EXPORT(zero_is_throw)]]and only ever yieldsJsError::Thrown, so theOutOfMemorybranch the helper preserves is unreachable for this call. Worth applying, not worth blocking a build fix on.
…OM mapping) No-Verification-Needed: same behaviour via the crate's helper; cargo check passes
There was a problem hiding this comment.
LGTM — the js_error_to_postgres nit from the prior review is addressed in ec710af; both conversion sites now use the crate helper.
What was reviewed:
on_notification/channel_name_jsnow returnResult<_, AnyPostgresError>and the new?inon()matches every sibling arm's error propagation.- Confirmed
js_error_to_postgresmapsJsError::{Thrown,OutOfMemory}→AnyPostgresError::{JSError,OutOfMemory}, so the pending-exception contract from #37275 holds. - Early-return guards (
js_valueunset, noonnotificationcallback) still returnOk(())— no behavior change on the happy path.
Extended reasoning...
Overview
Build fix for main: #32089 (LISTEN/NOTIFY) landed calling JSGlobalObject::report_active_exception_as_unhandled, which #37275 removed. The two call sites in PostgresSQLConnection.rs are rewritten to return Result<_, AnyPostgresError> and propagate via ? through on(), using the crate's documented .map_err(crate::jsc::js_error_to_postgres)? convention. Net diff is ~20 lines in one file.
Security risks
None. No new inputs parsed, no allocation logic changed, no trust boundaries crossed. The only reachable error case is OOM/STRING_TOO_LONG when converting a Postgres NOTIFY channel/payload to a JS string, which now fails the connection through the same path every other on() arm uses instead of being reported as an unhandled exception.
Level of scrutiny
Low. This is a mechanical build fix aligning two brand-new call sites with the crate's existing error-propagation pattern (PostgresRequest.rs, Signature.rs, DataCell.rs all use the same helper). The on() dispatch already returns Result<(), AnyPostgresError> and every sibling arm uses ?, so the new ? on on_notification is structurally identical.
Other factors
My previous review flagged that the first revision hand-rolled |_| AnyPostgresError::JSError instead of using js_error_to_postgres; ec710af addressed it and the thread is resolved. cargo check passes per the PR description; the LISTEN/NOTIFY tests run in CI's docker lane. No outstanding review comments.
1 participant
What does this PR do?
maindoes not compile: #32089 (sql.listen/notify) callsJSGlobalObject::report_active_exception_as_unhandled, which #37275 removed (bun_sql_jsc: "no method namedreport_active_exception_as_unhandled", ×2 inPostgresSQLConnection.rs).A NOTIFY channel/payload string the VM could not make (OOM, a worker's termination) is now handled the way every other JS conversion failure inside
PostgresSQLConnection::on()is:on_notificationreturnsAnyPostgresError::JSError, and the connection's error path takes the pending exception and fails the connection with it — which is also #37275's rule that anErralways leaves its exception pending for exactly one taker.How did you verify your code works?
cargo check -p bun_sql_jscandcargo check -p bun_bin(full crate graph) pass. The LISTEN/NOTIFY tests need a Postgres server (CI's docker lane).