fix(database): make runTransaction errors reject the returned promise - #10324
fix(database): make runTransaction errors reject the returned promise#10324bhavya091213 wants to merge 3 commits into
Conversation
Errors raised by the transaction update function, or by validation of the data it returns, could escape the promise returned by runTransaction(). When a transaction is rerun, the update function and validateFirebaseData() were called from repoRerunTransactionQueue, which runs inside server data-update callbacks. A throw there unwound through the WebSocket message handler rather than the promise, so the Deferred was never settled: the promise stayed pending forever while the error surfaced as an uncaught exception. Wrap both calls and turn a throw into a normal transaction abort, reusing the existing abort path (including reverting the pending local write) and reporting the original error through onComplete. The initial run happens synchronously inside repoStartTransaction, so errors there were thrown out of runTransaction() instead of rejecting. try/catch around an await caught them, but runTransaction(...).catch(...) did not, since the call threw before returning a promise. Reject the deferred instead. Also call unwatcher() before an initial-run error propagates. Previously the onValue listener registered for the transaction leaked whenever the update function or validation threw before the transaction was queued. Fixes firebase#7919
🦋 Changeset detectedLatest commit: d370581 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in runTransaction() to ensure that errors thrown by the transaction update function or during data validation (both in the initial synchronous run and during subsequent reruns) always reject the returned promise instead of causing uncaught exceptions or leaving the promise pending. It also adds corresponding integration tests. The reviewer noted that during a transaction rerun, an invalid .priority in the returned data is not validated inside the new try-catch block, which could still lead to an uncaught exception, and suggested validating the priority within the try-catch block.
An invalid `.priority` returned by a rerun was only rejected by
nodeFromJSON(), which ran after the update function and outside the guard
added in the previous commit. It therefore escaped the same way the NaN case
did: an uncaught INTERNAL ASSERT, with the returned promise left pending.
Verified against the emulator: `.priority: NaN` was already rejected via
validateFirebaseData, but `.priority: {}` and `.priority: true` both threw
out of nodeFromJSON.
Add the same isValidPriority() assertion the initial run already performs in
repoStartTransaction(), and move nodeFromJSON() inside the guard so any other
validation it performs is covered too.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes runTransaction() in @firebase/database to ensure that errors thrown during the transaction update function or data validation consistently reject the returned promise instead of throwing synchronously or leaking uncaught exceptions. The review feedback highlights two important edge cases where errors could still cause listener leaks or leave transactions in a broken state: first, by suggesting to call unwatcher() in the catch block of runTransaction to clean up the listener if repoStartTransaction fails synchronously; and second, by recommending that all validations (including priority checks) be performed before queueing the transaction to prevent it from getting stuck in a broken RUN state.
repoStartTransaction() pushed the transaction onto the queue before validating the priority returned by the update function. An invalid `.priority` therefore threw from the assert *after* queueing, leaving a stranded entry in the queue in RUN status that a later server update would try to rerun, and skipping the unwatcher() cleanup for the onValue() listener registered by runTransaction(). Move the priority computation and its isValidPriority() assert up next to the validateFirebaseData() call, inside the try block that unwatches and rethrows, so every initial-run validation happens before the transaction is queued. Also call unwatcher() in runTransaction()'s catch as defense in depth for anything that escapes from outside those try blocks. Removing an event registration twice is a no-op: viewRemoveEventRegistration() matches by callback identity, so the second removal finds nothing. Both issues were raised in review on firebase#10324.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes runTransaction() in @firebase/database to ensure that errors thrown during the transaction update function or data validation always reject the returned promise, rather than throwing synchronously or leaking as uncaught exceptions. It introduces proper error handling and cleanup (unwatching) during both the initial run and subsequent reruns of transactions, and includes comprehensive integration tests to verify these behaviors. There are no review comments, so I have no feedback to provide.
1 participant
Discussion
Fixes #7919 (reported Jan 2024, confirmed
reproducibleby @jbalidiong in Apr 2026).Approach was proposed and discussed first in #7919 (comment) per CONTRIBUTING.md. There is one open question from that comment that I'd still like your call on — see Open question at the bottom.
No public API surface changes: no signatures, types or exports are affected.
Problem
Errors raised by the transaction update function, or by validation of the data it returns, could escape the promise
runTransaction()returns. Two independent paths:1. The rerun path (the bug reported in #7919). When a transaction is rerun, the update function and
validateFirebaseData()were called bare insiderepoRerunTransactionQueue, which runs from server data-update callbacks (repoRerunTransactionsis invoked from six call sites inRepo.ts, all reached viaonDataUpdate). A throw there unwound through the WebSocket message handler instead of the promise:The
Deferredwas never settled, so the promise stayed pending forever — it neither resolved nor rejected — while the error surfaced as an uncaught exception. That is why the reporter'stry/catchnever fired.This also explains why the original repro must start with an empty database: the first run sees a not-yet-populated cache and returns valid data, so the transaction is queued; the
NaNis only produced on the rerun, once the server value arrives.2. The initial-run path. The first run happens synchronously inside
repoStartTransaction, so an error there was thrown out ofrunTransaction()rather than rejecting.try { await runTransaction(...) } catchcaught it, butrunTransaction(...).catch(fn)did not — the call throws before returning a promise, so.catchis never reached.Changes
core/Repo.ts— inrepoRerunTransactionQueue, wrap the update call andvalidateFirebaseDataso a throw becomes a normal transaction abort. This reuses the existing abort machinery, includingsyncTreeAckUserWrite(..., true)to revert the pending local write, so it behaves consistently with the existingmaxretry/nodataaborts. The original error is reported throughonCompletewith its stack preserved.api/Transaction.ts— catch a synchronous failure fromrepoStartTransactionand reject the deferred, so errors are always delivered through the returned promise.core/Repo.ts— callunwatcher()before an initial-run error propagates. Previously theonValuelistener registered for the transaction leaked whenever the update function or validation threw before the transaction was queued.Deliberately left alone: the argument-validation throws at the top of
runTransaction(validateWritablePath, and the.length/.keysread-only check). Those are programmer errors rather than transaction failures, and throwing synchronously is consistent with the rest of the SDK.Testing
Four regression tests added to
packages/database/test/exp/integration.test.ts. I verified each one fails before the change and passes after (for the two initial-run tests, by revertingTransaction.tsalone and watching them fail with the throw escaping atTransaction.ts:134):NaNrejects the promise — the exact scenario in runTransaction errors are not catchable #7919.catch()rather than throwing synchronouslyFull
@firebase/databasenode suite against the RTDB emulator goes from 266 passing to 270 passing, 0 failing.tsc --noEmit, eslint and prettier are all clean. Changeset included (@firebase/database: patch).Worth noting:
integration.test.tswas the only test file in the package touchingrunTransactionat all, which is likely why this went unnoticed for so long.Open question
Change 2 (initial run: synchronous throw → promise rejection) is technically a semantic change for anyone currently relying on
runTransaction()throwing synchronously. I believe rejecting is correct for a function documented to return aPromise, and it makes both paths behave identically — but I'm happy to split it into a separate PR and land only the rerun fix here if you'd rather handle that change on its own. Just say the word.