fix(firestore): do not throw when wrapping an error with a read only stack

[Om-singhaI]

fix(firestore): do not throw when wrapping an error with a read only stack

Fixes #9154

What breaks

wrapError() in handwritten/firestore/dev/src/util.ts appends the captured
callsite stack to the error it is handed:

export function wrapError(err: Error, stack: string): Error {
  err.stack += '\nCaused by: ' + stack;
  return err;
}

The compiled build/src/util.js begins with "use strict", so this compound
assignment is a strict mode write. When the incoming error carries a non
writable own stack property the write does not silently do nothing. It throws
a TypeError, and that TypeError takes the place of the real error the caller
was supposed to receive. The user loses both the failure and any hint of where
it came from.

Why it takes the process down

Where wrapError() is called decides how bad this is.

At dev/src/index.ts:1384 the call sits inside a promise .catch(), so a throw
there only turns into a rejected promise. Unpleasant, but catchable.

At dev/src/reference/query-util.ts:87 it sits inside a stream 'error'
listener:

.on('error', err => {
  reject(wrapError(err, callsiteError.stack!));
})

That listener runs long after the Promise executor returned, so a throw inside
it is not funnelled into reject. It propagates out of EventEmitter.emit,
through emitErrorNT, and becomes an uncaughtException. Two further call
sites have the identical shape and the identical exposure:
dev/src/reference/aggregate-query.ts:151 and
dev/src/pipelines/pipeline-util.ts:106.

Running a query whose backend stream fails with such an error, against main
before this change, produces:

  1) query interface
       handles stream exception whose stack is not writable:
     Uncaught TypeError: Cannot assign to read only property 'stack' of object 'Error: Expected error'
      at wrapError (dev/src/util.ts:237:12)
      at Transform.<anonymous> (dev/src/reference/query-util.ts:87:27)
      at Transform.emit (node:events:508:20)
      at emitErrorNT (node:internal/streams/destroy:170:8)
      at emitErrorCloseNT (node:internal/streams/destroy:129:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:90:21)

The fix

export function wrapError(err: Error, stack: string): Error {
  const wrappedStack = err.stack + '\nCaused by: ' + stack;
  try {
    if (Object.getOwnPropertyDescriptor(err, 'stack')?.writable === false) {
      // Some libraries define `stack` as a read only property, which makes a
      // plain assignment throw. Redefining the property still preserves the
      // callsite as long as the property stayed configurable.
      Object.defineProperty(err, 'stack', {value: wrappedStack});
    } else {
      err.stack = wrappedStack;
    }
  } catch {
    // The stack cannot be modified at all. Recording the callsite is best
    // effort, so return the error we were asked to wrap rather than let the
    // resulting TypeError take its place.
  }
  return err;
}

Two things are going on, and they are separate on purpose.

Redefining rather than assigning keeps the diagnostic. The errors that
actually trigger this in the wild are still configurable, so the callsite can be
recorded. Simply swallowing the TypeError would make wrapError() a no op for
exactly the failures where an async stack trace is most useful.

The try is the safety net, not the mechanism. A stack that is both non
writable and non configurable cannot be redefined either, and neither can one on
a frozen error. In those cases the original error is returned untouched.
Recording the callsite is a diagnostic nicety; delivering the actual error is
not optional.

Behaviour for ordinary errors is unchanged: on a normal Error, stack is an
accessor property, descriptor.writable is undefined, and the plain
assignment path runs exactly as before.

Relationship to the auth library

One producer of such errors is
core/packages/google-auth-library-nodejs/src/auth/oauth2common.ts, where
getErrorFromOAuthErrorResponse() copies stack onto a new error:

Object.defineProperty(newError, key, {
  value: (err as {} as {[index: string]: string})[key],
  writable: false,
  enumerable: true,
});

Worth noting, because it decides which fix is correct here: configurable is
not specified, and stack already exists on newError, so defineProperty
leaves the existing attribute alone. The resulting descriptor is
writable: false, configurable: true. Verified against the
google-auth-library that Firestore actually resolves:

descriptor: {"writable":false,"configurable":true}
wrapError returned: the ORIGINAL error
message: Error code invalid_grant: Bad Request
stack ends with Caused by: true

That producer is a separate defect in a separate package with a separate owner
(.github/CODEOWNERS assigns /handwritten/firestore to
@googleapis/firestore-team and /core/packages/google-auth-library-nodejs to
@googleapis/aion-team), and it is addressed on its own. Fixing the producer
does not fix this one. wrapError() still converts any error with a non
writable stack, from any source, into a TypeError or an uncaughtException,
and Firestore resolves the auth library transitively through a published
google-gax (@google-cloud/firestore-api to google-gax to
google-auth-library), so released Firestore keeps crashing until every layer
upgrades. This change makes the Firestore side robust on its own.

Testing

Three regression tests, all written before the fix and confirmed failing against
unmodified source.

dev/test/util.ts, unit coverage of the root cause:

• appends the callsite stack to the error stack guards the existing behaviour.
• appends the callsite stack when the error stack is not writable uses the
  descriptor shape that google-auth-library produces, and asserts the callsite
  is still recorded.
• returns the original error when its stack cannot be modified uses a non
  configurable stack and asserts the original error comes back untouched.

dev/test/query.ts, coverage of the reported crash path:

• handles stream exception whose stack is not writable drives a real query
  through query-util.ts with a mock backend stream that fails with an error
  whose stack was defined the way google-auth-library defines it, and
  asserts the returned promise rejects with the original error.

All three fail without the source change

With dev/src/util.ts restored to origin/main and the tests left in place:

  wrapError()
    ✔ appends the callsite stack to the error stack
    1) appends the callsite stack when the error stack is not writable
    2) returns the original error when its stack cannot be modified

  1 passing (6ms)
  2 failing

  1) wrapError()
       appends the callsite stack when the error stack is not writable:
     TypeError: Cannot assign to read only property 'stack' of object 'Error: Expected error'
      at wrapError (dev/src/util.ts:237:12)
      at Context.<anonymous> (dev/test/util.ts:129:30)
      at process.processImmediate (node:internal/timers:504:21)

  2) wrapError()
       returns the original error when its stack cannot be modified:
     TypeError: Cannot assign to read only property 'stack' of object 'Error: Expected error'
      at wrapError (dev/src/util.ts:237:12)
      at Context.<anonymous> (dev/test/util.ts:146:30)
      at process.processImmediate (node:internal/timers:504:21)

  query interface
    1) handles stream exception whose stack is not writable

  0 passing (56ms)
  1 failing

  1) query interface
       handles stream exception whose stack is not writable:
     Uncaught TypeError: Cannot assign to read only property 'stack' of object 'Error: Expected error'
      at wrapError (dev/src/util.ts:237:12)
      at Transform.<anonymous> (dev/src/reference/query-util.ts:87:27)
      at Transform.emit (node:events:508:20)
      at emitErrorNT (node:internal/streams/destroy:170:8)
      at emitErrorCloseNT (node:internal/streams/destroy:129:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:90:21)

Suites with the change applied

Run from handwritten/firestore on Node v25.6.1.

npx mocha build/test
  696 passing (9s)
  47 pending

npx mocha build/conformance
  228 passing (245ms)

No failures. Changed files pass prettier and the gts base ESLint config.
Note that npm run lint at the package level currently cannot run from a
package only install, because the repository root .eslintrc.json extends
./node_modules/gts relative to the repository root; that is unrelated to this
change.

[gemini-code-assist]

Code Review

This pull request updates the wrapError utility function to safely handle errors with non-writable stack properties (such as those generated by google-auth-library), preventing TypeError exceptions during stack trace wrapping. It also adds comprehensive unit and integration tests for these scenarios. The reviewer suggested optimizing wrapError by attempting direct assignment first and using a nested try-catch block to handle read-only or accessor properties, which avoids the overhead of Object.getOwnPropertyDescriptor on the happy path.

[gemini-code-assist]

Instead of calling Object.getOwnPropertyDescriptor on every call to wrapError (which adds overhead on the happy path) and only checking writable === false, we can use a nested try-catch block. This approach is more robust because it also handles accessor properties without setters (where writable is undefined but assignment still throws), and it avoids property descriptor lookup overhead when the stack is writable.

  const wrappedStack = err.stack + '\nCaused by: ' + stack;
  try {
    err.stack = wrappedStack;
  } catch {
    try {
      // Some libraries define `stack` as a read-only or accessor property without a setter,
      // which makes plain assignment throw. Redefining the property preserves the callsite
      // as long as the property is configurable.
      Object.defineProperty(err, 'stack', {
        value: wrappedStack,
        configurable: true,
        writable: true,
      });
    } catch {
      // The stack cannot be modified at all. Recording the callsite is best
      // effort, so return the error we were asked to wrap rather than let the
      // resulting TypeError take its place.
    }
  }
  return err;

[Om-singhaI]

Good catch, taken.

I had only checked writable === false, which misses a getter with no setter: writable is undefined there, so it falls through to the plain assignment and still throws. Switched to the try then redefine shape you suggested, which also drops the descriptor lookup off the happy path.

Added a test for that case. One thing worth knowing if anyone writes a similar test later: the set: undefined in it is load bearing. V8 installs stack as an accessor with both a getter and a setter, and a partial descriptor only overrides the attributes it names, so defining just get leaves the original setter in place and the assignment quietly succeeds. My first attempt at the test passed against the unfixed code for exactly that reason.

Reverting only util.ts makes the new test fail with the callsite missing, so it is covering the right thing. 673 passing in the firestore unit suite either way.