Skip to content

fix(firestore): observe rejection of lazily-started transaction ID promise - #9122

Open
rockwotj wants to merge 2 commits into
googleapis:mainfrom
rockwotj:fix-firestore-transaction-id-unhandled-rejection
Open

fix(firestore): observe rejection of lazily-started transaction ID promise#9122
rockwotj wants to merge 2 commits into
googleapis:mainfrom
rockwotj:fix-firestore-transaction-id-unhandled-rejection

Conversation

@rockwotj

@rockwotj rockwotj commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Problem

When the first read of a transaction fails, the client can emit an unhandledRejection. Under Node's default --unhandled-rejections=throw this terminates the process, even though the application handled the error that runTransaction() returned.

withLazyStartedTransaction() derives a second promise from the first read:

const resultPromise = resultFn.call(this, param, opts);

this._transactionIdPromise = resultPromise.then(r => { ... });

return resultPromise.then(r => r.result);

The caller handles the returned promise. Nothing attaches a handler to _transactionIdPromise at creation time. Its only handler comes later, from a subsequent read, commit(), or rollback(). Two cases leave it unobserved:

  1. Read-only transactions. rollback() returns early when this._writeBatch is unset, so it never awaits _transactionIdPromise. No handler is ever attached.
  2. Read-write transactions where the callback continues after a failed read. rollback() does await the promise, but only after the callback returns. If the callback yields to the event loop in between, Node has already reported the rejection. Node also logs PromiseRejectionHandledWarning here, which confirms the handler arrived too late.

Case 1 is deterministic: for a read-only transaction, no handler is ever attached, so any failure of the first read terminates the process.

We hit case 1 in production on @google-cloud/firestore@8.7.1. The first read of a read-only transaction, a BatchGetDocuments, stalled for 293 seconds and then failed with 16 UNAUTHENTICATED. The application caught the error from runTransaction(), and the process still died on the orphaned rejection. Read-only transactions set _maxAttempts to 1, so no retry masked it.

Reproduction

Point the client at a port where nothing listens, then run a read-only transaction whose first read fails:

const firestore = new Firestore({
  projectId: 'repro-project',
  host: 'localhost:1',
  ssl: false,
});

process.on('unhandledRejection', err => console.log('unhandled:', err.code));

try {
  await firestore.runTransaction(
    async txn => {
      await txn.get(firestore.doc('things/one'));
    },
    {readOnly: true},
  );
} catch {
  // The caller handles the error, and the process still dies without the
  // `process.on` above.
}

Before this change the listener fires with code 14. After it, it does not. The same script reproduces case 2 with a read-write transaction whose callback swallows the read error and then awaits a timer.

Fix

Attach a no-op handler to _transactionIdPromise when it is created. This marks the rejection observed. Later awaiters in commit() and rollback() still reject exactly as before, so error propagation is unchanged.

Test

Adds a regression test to handwritten/firestore/dev/test/transaction.ts. It records unhandledRejection events while a read-only transaction's first read fails, and asserts none are emitted.

Verified both directions against the firestore package:

  • Without the source change: 1 failingexpected [ Error: Test Error, …(1) ] to be empty.
  • With it: the new test passes and the full transaction suite is green at 37 passing.

…omise

When the first read of a transaction fails, the derived _transactionIdPromise
can be left without a handler. Nothing awaits it for read-only transactions,
because rollback() returns early when _writeBatch is unset, and a read-write
transaction can leave it rejected across a macrotask boundary. Node reports an
unhandled rejection, which terminates the process under the default
--unhandled-rejections=throw, even though the caller handled the error from
runTransaction().

Attach a no-op handler when the promise is created. Later awaiters in commit()
and rollback() still reject, so error propagation is unchanged.
@rockwotj
rockwotj requested a review from a team as a code owner August 11, 2026 17:20
@product-auto-label product-auto-label Bot added the api: firestore Issues related to the Firestore API. label Aug 11, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes an issue where an unhandled promise rejection is reported in Node.js when the first read of a transaction fails. This is resolved by attaching a catch handler to _transactionIdPromise to observe the rejection, and a corresponding test case has been added to verify the behavior. The reviewer suggested using the void operator on the floating promise to prevent potential ESLint warnings and explicitly signal that the promise is intentionally unawaited.

Comment thread handwritten/firestore/dev/src/transaction.ts Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the Firestore API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants