fix(bigquery-firestore-export): stop retrying deploy-time misconfigurations - #2983
Open
IzaakGough wants to merge 3 commits into
Open
fix(bigquery-firestore-export): stop retrying deploy-time misconfigurations#2983IzaakGough wants to merge 3 commits into
IzaakGough wants to merge 3 commits into
Conversation
…ations The upsert task retried every failure five times before dead-lettering, including misconfigurations that no retry can fix, such as clearing PARTITIONING_FIELD on an existing transfer config. Cloud Tasks retries any non-2xx and the lifecycle task has no deploy-status channel, so those paths now throw PermanentConfigurationError, which the handler logs at error level before returning.
![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 introduces a new PermanentConfigurationError to handle deploy-time misconfigurations that cannot be resolved by retrying, preventing unnecessary Cloud Tasks retries. It refactors error handling in dts.ts and handlers.ts to throw this new error and gracefully abort execution, and adds corresponding unit tests. The reviewer suggests also updating the transfer config lookup failure in constructUpdateTransferConfigRequest to throw PermanentConfigurationError instead of a generic Error to fully align with this goal.
…s permanent Building the handler context ran outside the upsert catch, so every param validation error was retried five times before dead-lettering. The context now arrives as a factory and resolveConfig reports validation failures as PermanentConfigurationError. A vanished transfer config threw a bare error from the update path and was retried the same way. It now names the stale config document and what to do about it.
… failures The invalid-structure error told users to point TRANSFER_CONFIG_NAME at a scheduled query or clear it, but that path only runs when the param is already unset, so following the advice changed nothing. It now says to delete the deployment's document from the configs collection. The linked-config-not-found error offered to clear the param so the deployment creates its own. Clearing it alone sends a previously linked config, which storeTransferConfig stamped with extInstanceId, down the update branch and rewrites its query, schedule and topic. The message now names the document that also has to be deleted.
IzaakGough
marked this pull request as ready for review
August 27, 2026 10:45
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 broken
upsertTransferConfigretried five times over 2.5 minutes before dead-lettering, even for failures no retry can fix: a missingTRANSFER_CONFIG_NAME, a stored config document with noname, an unparseable transfer config, a scheduled query deleted out from under a stored document, and any param that fails validation. The logs never said what to change.What changed
Those paths throw
PermanentConfigurationError.handleUpsertTransferConfigcatches only that type, logs it at error level and returns, so Cloud Tasks sees a 2xx and stops. Everything else still throws and still retries.How it was verified
32 unit tests and a clean build. Deployed to a real project, attempts counted in Cloud Logging:
b8cc1113DATASET_IDNotes
Treating a vanished config as permanent matches the client's retry policy:
GetTransferConfigretries onDEADLINE_EXCEEDEDandUNAVAILABLEonly, so a transient blip surfaces as one of those and still retries.The extension reported these through the Extensions runtime, so the message showed in the console. Kits have lifecycle hooks but no status channel, since
setProcessingStateis scoped to an extension instance viaEXT_INSTANCE_ID. The message goes to Cloud Logging at error level with the remediation steps in it, so a deploy that stops without acting is easier to miss.PARTITIONING_FIELDis also on the permanent list, but that guard is itself wrong (#2985). Not fixed here.Tracked in #2974 (parity analysis), rows 7a to 7c.