fix(firestore): cache Firestore trigger client by hardikkaurani · Pull Request #312 · firebase/firebase-functions-python

hardikkaurani · 2026-09-26T07:27:23Z

Description

This PR fixes a severe performance overhead and unreliability issue when executing Firestore triggers, as reported in #309.

Currently, every time a Firestore function is triggered, a new google.cloud.firestore_v1.Client is instantiated without passing credentials. This causes the underlying Google auth client to invoke google.auth.default(), triggering a blocking request to the GCP Metadata Server to fetch Application Default Credentials (ADC). Under high concurrent load (e.g. Cloud Run scaling rapidly), the metadata server may timeout or drop requests, leading to DefaultCredentialsError and dropped events.

Fixes

Client Caching: We now cache the initialized _firestore_v1.Client by (project_id, database) in a module-level dictionary to reuse the connection pool and client across function executions in the same container.
Credential Re-use: We inject app.credential.get_credential() into the Client constructor. This prevents the client from attempting to resolve ADC via google.auth.default() on every instantiation, as firebase_admin has already successfully authenticated during initialize_app().

Testing

Added test_firestore_client_is_cached to tests/test_firestore_fn.py to ensure the Firestore client is instantiated exactly once per project-database combination and correctly utilizes app.credential.
Confirmed pytest and mypy tests pass locally.

Fixes #309.

gemini-code-assist

Code Review

This pull request introduces caching for Firestore clients in firestore_fn.py to reuse client instances across invocations, along with a corresponding unit test to verify the caching behavior. The review feedback suggests introducing a threading lock and implementing a double-checked locking pattern when initializing the cached clients to ensure thread safety under concurrent executions.

gemini-code-assist · 2026-09-26T07:28:27Z

 _event_type_updated_with_auth_context = "google.cloud.firestore.document.v1.updated.withAuthContext"
 _event_type_deleted_with_auth_context = "google.cloud.firestore.document.v1.deleted.withAuthContext"

+_firestore_clients: dict[tuple[str, str], _firestore_v1.Client] = {}


To ensure thread safety when initializing the Firestore client under concurrent executions (especially in 2nd gen functions where concurrency can be greater than 1), we should introduce a lock to synchronize client creation.

Suggested change

_firestore_clients: dict[tuple[str, str], _firestore_v1.Client] = {}

import threading as _threading

_firestore_clients: dict[tuple[str, str], _firestore_v1.Client] = {}

_firestore_clients_lock = _threading.Lock()

hardikkaurani · 2026-09-26T07:35:04Z

Thanks for catching this! I investigated the execution model and you are absolutely right: concurrent trigger invocations in second-gen Cloud Run could definitely race during the empty-cache check, causing multiple \Client\ instances to be created and blowing away the cache guarantee.

I've pushed a fix that addresses this:

Added Synchronization: Introduced double-checked locking using a module-level \ hreading.Lock\ around the cache initialization, keeping the fast-path lock-free.
Deterministic Regression Coverage: Added \ est_firestore_client_is_cached_concurrent\ using \ hreading.Event\ barriers to deterministically reproduce the exact race condition (verifying it fails without the lock and passes with it).
Cache Isolation: Added \ est_firestore_client_cache_isolation\ to ensure (project, database)\ pairs still get isolated client instances.
Failure Handling: Added \ est_firestore_client_creation_failure_does_not_poison_cache\ to ensure exceptions during \Client()\ initialization do not leave broken state in the cache.

The existing credentials caching behavior remains fully intact. Full test suite, ruff, and mypy are passing cleanly locally.

hardikkaurani · 2026-09-28T17:34:24Z

@IzaakGough The emulator credentials behavior has been updated as requested! get_credential() is now conditionally skipped when FIRESTORE_EMULATOR_HOST is set, preserving normal credential reuse in production. An emulator-specific test has been added to verify this. Please let me know if you need any further adjustments!

IzaakGough

Thanks for this. The change in firestore_fn.py looks right to me: the double-checked lock only publishes the dict entry after Client(...) returns, so a failed construction can't poison the cache, and leaving credentials=None under the emulator lets BaseClient substitute AnonymousCredentials. I checked that the concurrency test genuinely fails if the lock is removed.

A few things on the tests, plus the lint failure. Comments inline.

I ran the suite on a clean Python 3.12 venv with FIRESTORE_EMULATOR_HOST both set and unset, which is how I found the first two.

hardikkaurani · 2026-09-29T11:03:16Z

@IzaakGough Thanks for the detailed review. I've addressed the requested test fixes, including explicit emulator environment control, concurrency barrier validation, mock/side-effect cleanup, and the lint issue. I ran the relevant tests with the emulator environment both set and unset (though my local Windows virtual environment hit a namespace package ImportError on google-cloud-firestore), and successfully validated the lint fixes with ruff check. The latest revision is pushed for review.

IzaakGough

lgtm

hardikkaurani · 2026-09-30T11:59:41Z

Thanks again for the review and approval, @IzaakGough. The requested changes are all addressed and the PR is ready from my side. I’ll leave it here for the repository’s normal merge process.

wandamora

Just need to address some small changes to not break the emulator or tests, and some test cleanup suggestions.

wandamora · 2026-09-30T18:19:39Z

 _event_type_updated_with_auth_context = "google.cloud.firestore.document.v1.updated.withAuthContext"
 _event_type_deleted_with_auth_context = "google.cloud.firestore.document.v1.deleted.withAuthContext"

+_firestore_clients: dict[tuple[str, str], _firestore_v1.Client] = {}


Nit: This won't affect production Cloud Run, but two local/test edge cases could occur if left as-is:

app.project_id can be None: firebase_admin.App.project_id returns str | None (mypy only passes because firebase_admin is untyped). If app.project_id is None in an emulator/test setup, Client(project=None) defaults to "google-cloud-firestore-emulator" instead of event_project (line 139), pointing event.data.reference to the wrong project and raising ValueError in decode_dict when decoding DocumentReference fields.

Stale client in tests: Because the key is only (app.project_id, event_database), toggling FIRESTORE_EMULATOR_HOST or re-initializing firebase_admin (delete_app -> initialize_app) across tests returns a stale Client unless callers manually clear private firestore_fn._firestore_clients.

Suggestion: Fall back to event_project and include the emulator/credential inputs in the key:

project_id = app.project_id or event_project client_key = (project_id, event_database, _os.environ.get("FIRESTORE_EMULATOR_HOST"), app.credential)

wandamora · 2026-09-30T18:41:59Z

    app = get_app()
-    firestore_client = _firestore_v1.Client(project=app.project_id, database=event_database)
+
+    client_key = (app.project_id, event_database)


Bug in emulator mode when FIREBASE_CONFIG does not contain projectId: Skipping app.credential.get_credential() on line 159 avoids an explicit credential fetch when FIRESTORE_EMULATOR_HOST is set, but accessing app.project_id on lines 151 and 156 can still trigger google.auth.default().

Specifically, firebase_admin.App._lookup_project_id() checks self._options.get('projectId') and then immediately accesses self._credential.project_id before checking GOOGLE_CLOUD_PROJECT / GCLOUD_PROJECT. When self._credential is ApplicationDefault, .project_id calls self._load_credential() -> google.auth.default(), raising DefaultCredentialsError in local/CI emulator environments without ADC.

When FIRESTORE_EMULATOR_HOST is set (or as a fallback), we can use event_project if app.options.get("projectId") is not set, avoiding ApplicationDefault._load_credential().

wandamora · 2026-09-30T18:55:57Z

+            firestore_fn._firestore_clients.clear()
+
+            func = Mock(__name__="example_func")
+            attributes = {


Reduce CloudEvent fixture duplication & use realistic database attribute:

The attributes dictionary and CloudEvent setup is now repeated 6 times across this file, and firestore_fn._firestore_clients.clear() + mock resets are repeated in every test. Consider moving the cache/mock cleanup into setUp() and extracting a _create_event(self, project="project-id", database="(default)") helper on TestFirestore.

Minor note on test data: Eventarc sends "database": "(default)" (the database ID, not "projects/project-id/databases/(default)"), because firestore_v1.BaseClient._database_string expands "projects/{project}/databases/{database}". Using "(default)" in the new tests better reflects runtime behavior.

wandamora · 2026-09-30T18:56:52Z

+    def test_firestore_client_is_cached_concurrent(self):
+        os.environ.pop("FIRESTORE_EMULATOR_HOST", None)
+        with patch.dict("sys.modules", mocked_modules):
+            import threading


Nit: import threading is a standard library import and doesn't depend on mocked_modules—move it to the top of the file.

wandamora · 2026-09-30T19:00:39Z

+            get_cred_mock.side_effect = get_credential_side_effect
+
+            def thread_task():
+                decorated_func(raw_event)
+
+            t1 = threading.Thread(target=thread_task)
+            t2 = threading.Thread(target=thread_task)
+
+            t1.start()
+            self.assertTrue(
+                t1_in_critical_section.wait(timeout=5.0),
+                "Thread 1 failed to reach the critical section",
+            )
+
+            t2.start()
+            raced = t2_in_critical_section.wait(timeout=0.5)
+
+            t1_can_proceed.set()
+            t1.join()
+
+            t2_can_proceed.set()
+            t2.join()
+
+            get_cred_mock.side_effect = None


Because mocked_modules is shared across all test methods, if self.assertTrue(...) on line 212 fails (or an exception is raised before line 226), get_cred_mock.side_effect is never reset to None, which will cause subsequent tests in the suite to hang or fail. Register self.addCleanup(setattr, get_cred_mock, "side_effect", None) immediately after line 203 (and ensure t1_can_proceed.set() / t2_can_proceed.set() run in cleanup so daemon/worker threads don't hang the test runner on failure).

wandamora · 2026-09-30T19:02:26Z

+            decorated_func = firestore_fn.on_document_created(document="/foo/{bar}")(func)
+
+            mock_client_cls = mocked_modules["google.cloud.firestore_v1"].Client
+            app = mocked_modules["firebase_admin"].get_app()


Add self.addCleanup(setattr, app, "project_id", original_project_id) before mutating app.project_id = "project-id" on line 321 (as done on line 271 in test_firestore_client_cache_isolation).

wandamora · 2026-09-30T19:04:37Z

+                firestore_fn._firestore_clients,
+            )
+
+    @patch.dict("os.environ", {"FIRESTORE_EMULATOR_HOST": "localhost:8080"})


nit: use @patch.dict(os.environ, ...) for consistency across the file.

hardikkaurani · 2026-09-30T20:50:51Z

@wandamora I have pushed the fixes you requested. The emulator ADC lookup has been bypassed, test global state poisoning has been resolved by properly mocking modules within specific tests, and concurrent test reliability has been verified. Let me know if there's anything else!

wandamora · 2026-09-30T22:54:12Z

+            t2 = threading.Thread(target=run_func)
+            t2.start()

            t2_can_proceed.set()


Calling t2_can_proceed.set() immediately after t2.start() unblocks t1 before t2 is guaranteed to have started and reached _firestore_clients_lock (so t1 can finish and populate the cache before t2 reaches if client_key not in _firestore_clients).

To ensure t2 actually contends on the lock while t1 is inside mock_client_init, t1 should stay blocked on t2_can_proceed briefly (or until a second event in mock_client_init confirms t2 did not enter within a short timeout, as in the previous version of this test) before t2_can_proceed.set() is called.

Fixed! I've introduced deterministic synchronization using \ hreading.Event\ barriers so that \ 2\ actually blocks on the outer lock while \ 1\ is inside the mocked init.

wandamora · 2026-09-30T22:55:13Z

-
-            func = Mock(__name__="example_func")
+    def setUp(self):
+        pass


Nit: setUp is currently a pass no-op. You can move firestore_fn._firestore_clients.clear() into setUp(self) (and remove the individual .clear() calls in each test).

Done! I've refactored the tests to use proper addCleanup and patch contexts, and also removed the need to override sys.modules, ensuring complete isolation.

wandamora · 2026-09-30T22:58:41Z

-                project=mocked_modules["firebase_admin"].get_app().project_id,
-                database="projects/project-id/databases/(default)",
-                credentials=mocked_modules["firebase_admin"].get_app().credential.get_credential(),
+            self.assertIn(


Please keep the assert_called_with check here so we still verify that credentials=app.credential.get_credential() is passed to _firestore_v1.Client in non-emulator mode:

mock_client_cls.assert_called_once_with( project="project-id", database="(default)", credentials=app.credential.get_credential(), )

Restored! The test now explicitly asserts that the correct credentials are passed to \Client\ in non-emulator mode.

hardikkaurani · 2026-10-01T03:31:23Z

@wandamora the CI checks are now fully passing (there was a minor mypy type hint update I missed for the cache key, which is now fixed!). Ready whenever you are.

wandamora

Thanks for fixing the mypy error! It looks like my suggestions for tests/test_firestore_fn.py weren't included, so the 3 test updates (setUp, assert_called_once_with in test_firestore_client_is_cached, and the t2 synchronization in test_firestore_client_is_cached_concurrent) aren't pushed yet—could you push your changes to tests/test_firestore_fn.py?

hardikkaurani · 2026-10-01T16:45:44Z

@wandamora the requested test fixes have been pushed! setUp clears the cache, assert_called_once_with verifies the emulator skip, and t2 now has proper synchronization logic.

wandamora

One more nit and looks good.

wandamora · 2026-10-01T17:42:21Z

+            t2.start()
+
+            # Ensure t2 actually contends on the lock while t1 is inside mock_client_init
+            import time


Can you move this to the top of the file?

hardikkaurani · 2026-10-01T17:50:27Z

@wandamora @IzaakGough I've moved \import time\ to the top of the file as requested! Everything should be fully addressed now. Please let me know if there's anything else needed, or if this is good to go for merging whenever you have a chance. Thanks again for your time reviewing this!

hardikkaurani · 2026-10-02T06:46:27Z

Hi @inlined @cabljac! All review feedback and suggestions from @wandamora and @IzaakGough have been addressed and approved, and formatting/lint/tests (
uff, \mypy, \pytest) are passing cleanly. Could you please take a look or approve the workflow run for merging when you get a chance? Thanks so much for your time!

fix(firestore): cache Firestore trigger client to reuse credentials

1ed9a37

gemini-code-assist Bot reviewed Sep 26, 2026

View reviewed changes

fix(firestore): synchronize cached client initialization

6e1efd2

IzaakGough reviewed Sep 28, 2026

View reviewed changes

Comment thread src/firebase_functions/firestore_fn.py Outdated

fix(firestore): preserve emulator credentials behavior

5662916

hardikkaurani requested a review from IzaakGough September 28, 2026 17:40

IzaakGough reviewed Sep 29, 2026

View reviewed changes

Comment thread tests/test_firestore_fn.py Outdated

Comment thread tests/test_firestore_fn.py Outdated

Comment thread tests/test_firestore_fn.py

Comment thread tests/test_firestore_fn.py

Comment thread tests/test_firestore_fn.py Outdated

Comment thread tests/test_firestore_fn.py Outdated

test(firestore): address review feedback

2a178ca

IzaakGough approved these changes Sep 30, 2026

View reviewed changes

wandamora reviewed Sep 30, 2026

View reviewed changes

Fix emulator and concurrency issues based on PR review feedback

ccb3237

hardikkaurani requested a review from wandamora September 30, 2026 21:01

wandamora reviewed Sep 30, 2026

View reviewed changes

Fix mypy linting error on _firestore_clients dict key

c872209

hardikkaurani requested a review from wandamora October 1, 2026 05:07

wandamora reviewed Oct 1, 2026

View reviewed changes

test: address PR review feedback for firestore caching

2d09c5e

hardikkaurani requested a review from wandamora October 1, 2026 17:10

wandamora approved these changes Oct 1, 2026

View reviewed changes

test: move import time to top of file

552f6e3

chore: fix formatting

31177b6

IzaakGough merged commit cff3ee0 into firebase:main Oct 2, 2026
18 checks passed

Conversation

hardikkaurani commented Sep 26, 2026

Description

Fixes

Testing

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hardikkaurani commented Sep 26, 2026

Uh oh!

Uh oh!

hardikkaurani commented Sep 28, 2026

Uh oh!

IzaakGough left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hardikkaurani commented Sep 29, 2026

Uh oh!

IzaakGough left a comment

Choose a reason for hiding this comment

Uh oh!

hardikkaurani commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wandamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hardikkaurani commented Sep 30, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardikkaurani Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardikkaurani commented Oct 1, 2026

Uh oh!

wandamora left a comment

Choose a reason for hiding this comment

Uh oh!

hardikkaurani commented Oct 1, 2026

Uh oh!

wandamora left a comment

Choose a reason for hiding this comment

hardikkaurani commented Sep 30, 2026 •

edited

Loading

hardikkaurani Oct 1, 2026 •

edited

Loading