fix: prevent unhandledRejection in batchAppend - #505
Merged
Conversation
batchAppend builds and writes its batches inside `new Promise(async (...) =>
{ ... })`. Anything that throws in there — most easily a non-serializable
event payload reaching JSON.stringify in eventBatcher — throws while the
stream is healthy, so the "error" handler never runs and so never settles
the entry. The Promise constructor also discards an async executor's
rejection, which leaves the append orphaned: the caller awaits forever and
the failure surfaces only as an unhandledRejection, ending the process
under Node's default --unhandled-rejections=throw.
Wrap the body in a try/catch that drops the promise-bank entry and rejects
via convertToCommandError, matching the "error" handler.
A bigint payload is an easy way to hit this by accident, since the client
hands revisions and positions back as bigints.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🚨 @w1am Failed to create cherry Pick PR due to error:
Error: Commit feba98217b3d205e690be24c85cc2494e4b1886e has 2 parents. github-cherry-pick is designed for the rebase workflow and doesn't support merge commits.
at Object.<anonymous> (/home/runner/work/_actions/kurrent-io/Automations/master/lib/cherry-pick/index.js:94:13)
at Generator.next (<anonymous>)
at fulfilled (/home/runner/work/_actions/kurrent-io/Automations/master/lib/cherry-pick/index.js:9:26)
at process.processTicksAndRejections (node:internal/process/task_queues:104:5)
🚨👉 Check https://github.com/kurrent-io/KurrentDB-Client-NodeJS/actions/runs/32696051058
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased onto
masterand repointed at the part of this that is still broken.What changed since this PR was opened
The original report below was a crash in the
"data"handler, from a response arriving for a correlation id that was no longer in the promise bank. #523 has since fixed that onmaster— each stream now owns its own bank and unknown correlation ids are dropped — so the original one-line change here is obsolete and has been dropped.What is still broken
The
unhandledRejectionin the title is still reachable by a different route.batchAppendbuilds and writes its batches insidenew Promise(async (...batchPromise) => { … }). Anything that throws in there throws while the stream is perfectly healthy, so the"error"handler never runs and never settles that entry. ThePromiseconstructor also discards an async executor's rejection. So the append is orphaned twice over:awaits forever; andunhandledRejection, which ends the process under Node's default--unhandled-rejections=throw.The easiest way in is a non-serializable event payload reaching
JSON.stringifyineventBatcher. Abigintdoes it, which is an easy mistake to make against this client specifically, since it hands revisions and positions back asbigints:options.setStreamPosition(streamState.toString(10))andmessage.serializeBinary()are reachable the same way.The fix
Wrap that body in a
try/catchwhich deletes the orphaned promise-bank entry and rejects. It rejects viaconvertToCommandErrorto match the"error"handler — that is a passthrough for anything that isn't a gRPC service error, so aTypeErrorfromJSON.stringifystill surfaces as itself, while a genuine transport error raised bybackpressuredWriteis normalized the same way it would be elsewhere.Test
Added to
appendToStream-batch-append.test.ts: appends an event carrying abigintand asserts the append rejects, and that nothing escapes as anunhandledRejection.Verified against
kurrentdb26.1.2 that it fails on unpatchedmaster— the append never settles and the test dies on the 60s jest timeout — and passes with the fix. Full file: 5 passed, 1 skipped.Original report (fixed separately by #523)
We saw some unhandledRejections during client reconnect coming from this code. Stack below: