feat(firestore-bigquery-export): reinstate the Cloud Tasks write buffer - #3103
Closed
cabljac wants to merge 8 commits into
Closed
feat(firestore-bigquery-export): reinstate the Cloud Tasks write buffer#3103cabljac wants to merge 8 commits into
cabljac wants to merge 8 commits into
Conversation
Contributor
Author
|
Live E2E checklist (dev-extensions-testing), to run before marking ready for review:
|
![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 reinstates the Cloud Tasks write buffer for failed inline BigQuery writes via a new syncBigQuery task queue, and restores explicit function placement derived from DATABASE_REGION. Feedback on the changes highlights a critical parameter mismatch bug in logFailedEventAction where a custom message shifts the subsequent arguments, leading to corrupted error logs. Additionally, it is recommended to calculate the random jitter inside the retry backoff loop in tasks.ts rather than keeping it constant across all attempts.
cabljac
force-pushed
the
feat/kits-fbe-cloud-tasks-buffer
branch
from
September 2, 2026 19:38
367bdc7 to
451332e
Compare
cabljac
commented
Sep 7, 2026
cabljac
left a comment
cabljac
left a comment
Contributor
Author
There was a problem hiding this comment.
Self-review before this goes out for review. Seven items: five are code changes, two are doc corrections. The tracker one is unverified and needs settling before merge.
This started as an AI-assisted review; I've verified the findings below against the source myself, except the two I've explicitly marked unverified.
Port the extension's syncBigQuery queue into the kit (option D, #3031): a failed inline write enqueues the serialized change onto a task queue (5 attempts, 60s min backoff, MAX_DISPATCHES_PER_SECOND throttle) instead of replaying through Eventarc for 24h. The task handler self-heals (ensureInitialized) before re-attempting and rethrows so Cloud Tasks retries; tracker 2.1.0 backs the row up to BACKUP_COLLECTION on every terminal insert failure. The trigger keeps retry: true so a failed enqueue - logged at error level and rethrown, unlike the extension's silent swallow - is redelivered rather than dropped. Enqueue targets the bare function name on firebase-admin ^14.2.0, which resolves the kit-<instance>- prefix from FIREBASE_KIT_INSTANCE_ID (Firebase CLI 15.28.0+); queue region derives from DATABASE_REGION with FUNCTION_REGION as fallback. MAX_DISPATCHES_PER_SECOND (default 100) and MAX_ENQUEUE_ATTEMPTS (default 3) keep their extension names so migrated .env values carry over.
…pin buffer safety properties A library consumer passing maxEnqueueAttempts <= 0 made enqueueSyncTask resolve without enqueueing, so the trigger logged success for an event buffered nowhere; the budget is now clamped to at least one attempt. New tests pin the backup wiring (toTrackerConfig.backupTableId carries backupCollectionId - the property that keeps queue exhaustion durable) and that the syncBigQuery task never re-enqueues (the trigger-queue loop seed).
…ry failure path - Attempt the buffered write even when provisioning fails. The tracker only parks a row in BACKUP_COLLECTION from its insert failure path, so throwing before the write dropped the row instead of backing it up. - Publish the success event outside the insert try. Rethrowing after the row had landed made Cloud Tasks retry past the insertId dedupe window and duplicate it. - Enqueue with a task id derived from the event id and treat task-already-exists as success, so an Eventarc redelivery after a failed enqueue cannot buffer the same event twice. - Drop the FUNCTION_REGION fallback. It is a gen1 variable and is not set on gen2 runtimes; resolve the region from DATABASE_REGION only. - Route MAX_ENQUEUE_ATTEMPTS and MAX_DISPATCHES_PER_SECOND through optionalInt so an unset param reaches resolveExportConfig as undefined and gets the documented default rather than 0.
cabljac
force-pushed
the
feat/kits-fbe-cloud-tasks-buffer
branch
from
September 7, 2026 08:38
8f0b2e1 to
0f30873
Compare
…s code firebase-admin reports the duplicate-task error as functions/task-already-exists, so the bare comparison never matched and a redelivered enqueue rethrew instead of resolving. The test mock now carries the prefixed code as the SDK does.
…N_REGION first The Firebase CLI sets FUNCTION_REGION on every deployed gen2 function (cloudfunctionsv2.js sets it from endpoint.region), so it is the region the queue actually lives in. Dropping it in 0f30873 was based on a wrong claim that the variable is never set. With DATABASE_REGION empty, which the README documents as supported, every failed inline write threw before the first enqueue attempt and went back to Eventarc for redelivery; on a first interactive deploy the queue path named the DATABASE_REGION target while the functions were in us-central1. Also clamp a NaN or non-integer attempt budget to one attempt: Math.max(1, NaN) is NaN and skipped the enqueue loop entirely.
… on the trigger The extension declares no retry policy on fsexportbigquery and, when the enqueue itself fails, logs at error level, publishes onError, and drops the event. The kit kept retry: true from before the buffer landed and rethrew on enqueue exhaustion, which redelivered the event through Eventarc for up to 24 hours; it also redelivered on every failure before the write was attempted (serialization, onStart publish), which the PR text did not say. Drop retry: true and stop rethrowing on enqueue exhaustion so the failure behaviour and cost profile match the extension.
…ilures Transform-function failures throw before the insert and are never backed up, on the kit and the extension alike.
…ler's hot path Drop provisioning from handleSyncBigQueryTask. The extension's queue handler goes straight to the write with skipInit on the tracker; the kit ran tracker.initialize() before every buffered write on a cold instance, which under a recovery burst fanned six to eight BigQuery metadata calls, and with TABLE_PARTITIONING set on an unpartitioned table a table.setMetadata, across up to 500 concurrent instances. Provisioning stays in the lifecycle tasks. Give syncBigQuery maxInstances equal to its maxConcurrentDispatches. The extension's handler is gen1 with no instance cap; gen2 defaults to 100, so dispatches beyond that would 429 at the cap and consume queue attempts.
Contributor
Author
|
Closing in favour of the same change split into a reviewable stack (stack #3132), bottom to top:
The top of the stack is identical to this branch on |
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
1 participant
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.
Ports the extension's Cloud Tasks write buffer into the kit, per the option D decision on #3031: migrating users keep the failure behaviour and cost profile they have today.
A failed inline write enqueues onto a new
syncBigQuerytask queue (5 attempts, 60s minimum backoff, throttled byMAX_DISPATCHES_PER_SECOND, default 100) after up toMAX_ENQUEUE_ATTEMPTSin-process attempts (default 3). The task handler runsensureInitializedas a self-heal, attempts the write even if that fails, and rethrows so Cloud Tasks retries; tracker 2.1.0 writes the rows toBACKUP_COLLECTIONon every terminal insert failure. The trigger declares no retry policy and a failed enqueue is logged, published asonError, and dropped, both exactly as the extension does. Enqueue targets the bare function name (firebase-admin 14.2.0+ resolves theFIREBASE_KIT_INSTANCE_IDprefix, Firebase CLI 15.28.0+), in the CLI-setFUNCTION_REGION, with a task id derived from the event id so a retried enqueue cannot buffer the same event twice. One deliberate change: the success event is published after the insert and swallowed on failure, closing a duplicate-row route the extension has.91 unit tests pass. No live queue E2E has been run: that the deployed env carries
FIREBASE_KIT_INSTANCE_IDandFUNCTION_REGION, that the gen2 task function accepts the SDK's ID token, queue creation with the resolved throttle, and the task size limit on large documents are all unverified. The tracker pins firebase-admin ^13, so a second admin copy is nested under it; a follow-up should widen that range.Fixes #3031