fix(firestore-bigquery-export): scope insert retry to schema lag - #2937
Draft
IzaakGough wants to merge 5 commits into
Draft
fix(firestore-bigquery-export): scope insert retry to schema lag#2937IzaakGough wants to merge 5 commits into
IzaakGough wants to merge 5 commits into
Conversation
`isRetryableInsertionError` was declared `async` but called without `await`, so the guard evaluated an always-truthy promise. Every insert failure was retried once with `ignoreUnknownValues: true`, and its allowlist of expected errors never ran. The allowlist could not have worked regardless: it read `e.response.insertErrors.errors`, but `insertErrors` is an array on the raw `insertAll` response, so the guard never passed and the predicate always returned `true`. Together these meant any schema mismatch, not just a column we had just added, was retried with unknown fields ignored. BigQuery then accepted the row with those fields silently dropped and the write reported success. Split the predicate in two, both synchronous: a schema lag check that positively matches unknown-field errors naming columns this tracker adds to an existing table, and a transient check for failures with no partial-failure body. Only the former retries with `ignoreUnknownValues`; the latter retries with options unchanged. Also key the failed-transactions backup off whether the attempt is terminal rather than off `retry`. `retry` meant "a retry is available" at the guard but was read as "this is the second attempt" at the backup, which only coincided while the retry branch was unconditional. Without this, a non-retryable first attempt would throw without backing up.
IzaakGough
marked this pull request as draft
August 11, 2026 10:28
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the BigQuery insertion retry logic in the Firestore BigQuery Event History Tracker to safely handle schema lag and transient failures, and adds comprehensive unit tests. The reviewer noted that using substring matching (message.includes(column)) to identify unknown fields could lead to false positives and silent data loss (e.g., matching document_id_v2 against document_id), and suggested parsing the exact field name from the error message instead.
`old_data` is also added to existing tables by initializeRawChangeLogTable, so it shares the streaming-lag exposure, but the lag has never been observed for it and the allowlist governs what we are willing to silently drop. Keep it to the two columns the original allowlist named. An unlisted column now takes the terminal path: the rows are backed up, the error is thrown and the tracker reinitializes, so the schema still reconciles and the trigger retry redelivers. That is a better outcome than dropping the column's contents.
The fallback for the inlined `"no such field: document_id."` form used a substring test, so a user column whose name contains an allowlisted one, such as `document_id_v2`, matched. That column would then be retried with `ignoreUnknownValues` and silently dropped, which is the exact failure this change set out to remove. Parse the field name out of the message and compare it whole. A message with no colon and no `location` still does not match, so it takes the terminal path.
…e cause Restore `old_data` to the retry allowlist. Excluding it was a regression, not a tightening: on the current code every unknown field is tolerated by dropping it, so a row hitting the streaming-buffer lag after `old_data` is added to an existing table lands today with that column null. Excluded, the same event instead fails terminally and is lost once the caller exhausts its retries, because nothing on the write path reconciles the schema. `_initialized = false` does not achieve that: `record()` only calls `initialize()` when `!skipInit`, and both the extension and the kit set `skipInit: true`. The allowlist now covers exactly the three columns initializeRawChangeLogTable adds to a table that already exists, and a parameterised test pins all three. Also stop the terminal path from destroying the error it is reporting: - Wrap the backup write. A Firestore batch failure replaced the insert error, so the caller lost the cause it needs to decide whether to retry, and the error logging was skipped too. - Make classification defensive to match `extractInsertErrors`. A null entry in `errors`, or a non-object thrown value, raised a `TypeError` from inside the catch block, which replaced the original error and skipped the backup write entirely. The `_initialized` assertion in the existing test was vacuous, since the flag starts `false` and `initialize()` never runs under these mocks. It now sets the flag first, so it fails against an implementation that never clears it.
… retries Three gaps found in review. The allowlist was missing the user-configured partition column. addPartitioningToSchema adds it to a table that already exists, exactly as the other three are added, and getPartitionValue writes it into every row. An extension user setting TIME_PARTITIONING_FIELD on an existing changelog would hit the streaming lag on that column and, without this, lose the event instead of landing the row with the column null. Derive the list from the partitioning config so it stays complete. The two retries are now tracked separately. Sharing one budget meant a transient blip on the first attempt consumed the retry a schema lag needed on the second, so a row that lands today was lost. Each is spent at most once, bounding this layer at three attempts. bigQueryTableInsertErrors is called on the terminal path and was not defensive, so a bad entry in the library's remapped `errors` copy threw and replaced the insert error the caller needs. It now checks for arrays and reads entries optionally. This closes the same hole the earlier `e?.errors` change only half covered.
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.
What was wrong
isRetryableInsertionErrorwasasyncbut called withoutawait, so the guard evaluated an always-truthy promise. Every insert failure was retried withignoreUnknownValues: trueand the allowlist never ran. It also reade.response.insertErrors.errors, butinsertErrorsis an array on the rawinsertAllresponse, so the allowlist would have matched nothing even withawait.So any schema mismatch was retried with unknown fields ignored. BigQuery accepted the row with those fields dropped and the write reported success. Pre-existing since 2020 (
2de70201), with no test coverage.What changed
isSchemaLagInsertionErrormatches only unknown-field errors naming a column this tracker adds to an existing table, and is the only path that retries withignoreUnknownValues.isTransientInsertionErrorcovers failures with no partial-failure body and retries with options unchanged. Neither isasync, so the original bug cannot return.document_id,path_params,old_data, and the configured partition column. All four are added to tables that already exist, so all four are exposed to the BigQuery streaming-buffer lag. Omitting any one turns a row that lands today, with that column null, into a lost event.retry, which previously worked only because the retry branch was unconditional.Behaviour change
Drift in a column outside that list now throws and writes a backup row, instead of silently dropping the column. Recovery depends on the caller's retry budget, since nothing in this package reconciles the schema on the write path.
Both consumers pin the published
^2.0.4, so this changes nothing for them until a release and a dependency bump.Testing
23 tests in a new file, mocking the BigQuery client so it runs offline unlike the existing suites here. Each behavioural change is pinned by a test confirmed to fail against the commit before it.
tsc --noEmitis clean. The existing BigQuery suites need live credentials and were not run.Known gaps
isTransientInsertionErroralso treats fatal 400/403/404 as transient, and the library already retries beneath it inshouldRetryRequestand_insertWithRetry.PartialFailureError, so that contract is unpinned.TRANSFORM_FUNCTIONemitting keys with no matching column now hard-fails, andBACKUP_COLLECTIONis optional so the backup mitigation is absent when it is unset. Both want a CHANGELOG note with whichever release carries this.