Conversation
Contributor
Author
|
Live E2E checklist (dev-extensions-testing), to run before marking ready for review:
|
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
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 |
cabljac
added a commit
that referenced
this pull request
Sep 8, 2026
…er (#3130) Fourth layer of the Cloud Tasks write buffer stack and the behaviour change, per the option D decision on #3031: migrating users keep the failure behaviour and cost profile they have today. Supersedes #3103. A failed inline write now enqueues onto a new `syncBigQuery` task queue (5 attempts, 60s minimum backoff, throttled by `MAX_DISPATCHES_PER_SECOND`) after up to `MAX_ENQUEUE_ATTEMPTS` in-process attempts, instead of self-healing once and rethrowing to the trigger retry policy. The task handler re-attempts the write and rethrows so Cloud Tasks retries; the tracker parks rows whose insert is rejected in `BACKUP_COLLECTION`, on the inline attempt and on each queue attempt. A change that cannot be enqueued at all is logged first, then published as `onError`, and dropped, as the extension does; the log-before-publish order is deliberate so the only trace of a dropped row survives a failing events channel. The trigger drops its retry policy. The function needs `roles/cloudtasks.enqueuer`. The queue function sets `maxInstances` to 500 to match the gen1 handler, since the gen2 default of 100 would 429 dispatches and burn attempts. The queue's `rateLimits.maxDispatchesPerSecond` is the CEL ternary `params.MAX_DISPATCHES_PER_SECOND < 1 ? 100 : params.MAX_DISPATCHES_PER_SECOND`: a blank `.env` value reaches deploy-time CEL as 0, and the ternary restores the same default the runtime falls back to. `REQUIRED_APIS` keeps `firestore.googleapis.com` from #3119, now pinned by `tests/required-apis.test.ts`. One deliberate change from the extension: the success event is published after the insert and swallowed on failure, closing a duplicate-row route. The README rewrite for the new export and failure path is the next layer, #3131. 96 tests pass on this layer. Verified live on a test project with Firebase CLI 15.29.0 from the top of the stack: the deployed env carries `FIREBASE_KIT_INSTANCE_ID` and `FUNCTION_REGION`, the gen2 task function accepted the SDK's ID token, the queue was created with the resolved throttle, and with the changelog table removed the trigger enqueued, the tracker wrote backup documents keyed by event id, and the queue drained without duplicate rows once the table was re-provisioned. Not exercised live: the Cloud Tasks task size limit. A change near Firestore's 1 MB document cap, doubled by `oldData` on an update, exceeds the task body limit, so the enqueue fails and the row is logged and dropped. This is parity: the extension enqueues the same payload through the same path, and `EXCLUDE_OLD_DATA` is its mitigation. Listed under known limits in the README in #3131. Fixes #3031.
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