Skip to content

fix: propagate fatal flag in promise error wrapper and fix execute args check#4193

Open
techcodie wants to merge 2 commits intosidorares:masterfrom
techcodie:fix/promise-error-propagation
Open

fix: propagate fatal flag in promise error wrapper and fix execute args check#4193
techcodie wants to merge 2 commits intosidorares:masterfrom
techcodie:fix/promise-error-propagation

Conversation

@techcodie
Copy link
Copy Markdown

Problem

Two related bugs in the promise wrapper layer:

1. fatal flag lost in promise rejections (make_done_cb.js)

When a fatal error occurs (e.g. PROTOCOL_CONNECTION_LOST), the callback API correctly sets err.fatal = true. However, makeDoneCb — used by all promise-based methods (query, execute, beginTransaction, etc.) — was not copying err.fatal to the rejected localErr.

This meant promise clients had no way to distinguish fatal errors from non-fatal ones, a silent behavioral difference from the callback API.

2. PromisePool.execute used a falsy args check (pool.js)

PromisePool.query correctly uses if (args !== undefined) but PromisePool.execute used if (args). These should be consistent.

Fix

  • lib/promise/make_done_cb.js: add localErr.fatal = err.fatal
  • lib/promise/pool.js: change if (args)if (args !== undefined)

Tests

Added test/integration/promise-wrappers/test-promise-error-properties.test.mts covering both fixes.

…gs check

- makeDoneCb now copies err.fatal to the rejected localErr, so promise
  clients receive the same fatal property that callback clients do.
  Previously, fatal errors (e.g. PROTOCOL_CONNECTION_LOST) would reject
  with an error missing the fatal flag, making it impossible for callers
  to distinguish fatal from non-fatal errors.

- PromisePool.execute used a falsy check (if (args)) instead of
  (if (args !== undefined)), inconsistent with PromisePool.query.
  This is now aligned to use strict undefined check.
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.65%. Comparing base (3adca00) to head (9d969fb).
⚠️ Report is 37 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4193   +/-   ##
=======================================
  Coverage   90.64%   90.65%           
=======================================
  Files          86       86           
  Lines       14245    14246    +1     
  Branches     1798     1798           
=======================================
+ Hits        12913    12914    +1     
  Misses       1332     1332           
Flag Coverage Δ
compression-0 89.90% <100.00%> (+<0.01%) ⬆️
compression-1 90.62% <100.00%> (+<0.01%) ⬆️
static-parser-0 88.33% <100.00%> (+<0.01%) ⬆️
static-parser-1 89.06% <100.00%> (+<0.01%) ⬆️
tls-0 90.08% <100.00%> (+<0.01%) ⬆️
tls-1 90.42% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@wellwelwel wellwelwel added the needs rebase For internal organization purpose label Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs rebase For internal organization purpose

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants