googleapis / googleapis/google-cloud-node

bigtable: mutateInternal builds self-referential error (err.errors contains err) — breaks pino / AggregateError-aware serializers

Open
#8,075 0 comments 0 reactions 0 assignees View on GitHub
api: bigtable
Dominant language
TypeScript
Stars
3.2k
Forks
712
Avg merge
2d 3h
Merged PRs (30d)
99

Description

### Environment

- `@google-cloud/bigtable` version: `6.5.0` (also reproduces on `main`, see below)
- Node.js: v24.14.1
- OS: Linux (Debian 12) — not OS-specific

### Summary

When `Table#mutate` / `Table#insert` encounters an RPC-level failure with entries still pending and no per-entry mutation errors, `mutateInternal` sets `err.errors` to an array that includes `err` itself, producing a **self-referential error object**.

This makes the returned error object unsafe to serialize with any tool that follows the `AggregateError` convention of recursing into `err.errors` (e.g. `pino-std-serializers`, most structured loggers, some error-reporting SDKs) — it causes `RangeError: Maximum call stack size exceeded`.

### Offending code

https://github.com/googleapis/google-cloud-node/blob/main/handwritten/bigtable/src/utils/mutateInternal.ts

(Same code also present in the now-archived `googleapis/nodejs-bigtable` repo at `src/utils/mutateInternal.ts`.)

```ts
if (err) {
/* If there's an RPC level failure and the mutation entries don't have
a status code, the RPC level failure error code will be used as the
entry failure code.
*/
(err as ServiceError & {errors?: ServiceError[]}).errors =
mutationErrors.concat(
[...pendingEntryIndices]
.filter(index => !mutationErrorsByEntryIndex.has(index))
.map(() => err), // ← pushes `err` into its own `err.errors`
);
collectMetricsCallback(err, err);
return;
}
```

For each pending entry with no per-row status, the outer `err` is pushed into `err.errors`. After this runs, `err.errors[i] === err` for every `i`, forming a cycle.

### Minimal repro

```js
const { pino } = require('pino');
const log = pino();

// Shape of what `mutateInternal` hands back on RPC-level failure with pending entries:
const err = new Error('RPC failure');
err.code = 14; // UNAVAILABLE
err.errors = [err, err, err]; // one self-reference per pending entry

log.error({ err }, 'mutateRows failed');
// RangeError: Maximum call stack size exceeded
// at errSerializer (pino-std-serializers/lib/err.js:25)
```

### Observed impact

In our production service this has fired thousands of times after a single bigtable RPC failure — each error crashes the logging pipeline before the underlying BigTable failure can be reported, so we lost all upstream diagnostic detail about the original RPC error.

### Suggested fix

Don't reuse the outer `err` as a placeholder for pending entries. Options:

1. Use a **clone** of `err` (a fresh Error with the same `message` / `code` / `details`) for each pending-entry placeholder, so the aggregate contains siblings, not self-references.
2. Use a **distinct sentinel error** (e.g. `new Error('pending entry — RPC failed with: ' + err.message)`) for each pending entry.
3. Or simply skip padding with the outer error for pending entries and keep `err.errors = mutationErrors` — the top-level `err.message`/`err.code` already communicate the RPC-level failure.

All three avoid the cycle and remain serializable by standard logging / telemetry libraries.

### Workaround for current users

Until this is fixed in a release, consumers have to add a cycle-breaking serializer in front of their logger (we did this via a custom pino `err` serializer that walks `err.errors` with a `WeakSet` and replaces cyclic references before delegating to `stdSerializers.err`).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.