fix(backend): reduce Firestore hot-path costs ~15-20% (#5377)#5378
fix(backend): reduce Firestore hot-path costs ~15-20% (#5377)#5378
Conversation
Greptile SummaryThis PR adds a single coordination/planning file ( Key observations:
Confidence Score: 1/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR["PR #5378 (this PR)"] --> COORD[".coordination/issue-5377-ownership.yaml\n(only file changed)"]
COORD --> S1["Scope: kelvin-transcribe-cache\nstatus: planned ❌"]
COORD --> S2["Scope: kenji-pusher-storage-cache\nstatus: planned ❌"]
S1 --> F1["backend/routers/transcribe.py\n(unmodified)"]
S1 --> F2["backend/database/conversations.py\n(unmodified)"]
S1 --> F3["backend/models/transcript_segment.py\n(unmodified)"]
S1 --> T1["unit tests\n(unmodified)"]
S2 --> F4["backend/routers/pusher.py\n(unmodified)"]
S2 --> F5["backend/utils/other/storage.py\n(unmodified)"]
S2 --> T2["unit tests\n(unmodified)"]
style COORD fill:#f9f,stroke:#333
style S1 fill:#fdd,stroke:#c00
style S2 fill:#fdd,stroke:#c00
Last reviewed commit: 64d6826 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| status: planned | ||
|
|
||
| kenji-pusher-storage-cache: | ||
| owner: kenji | ||
| description: "Cache data_protection_level per session in pusher, pass to upload_audio_chunk" | ||
| files: | ||
| - backend/routers/pusher.py | ||
| - backend/utils/other/storage.py | ||
| - backend/tests/unit/test_storage_upload_audio_chunk_data_protection.py | ||
| - backend/tests/unit/test_pusher_private_cloud_data_protection.py | ||
| status: planned |
There was a problem hiding this comment.
Both implementation scopes are still planned — no code has been changed
The PR title and description claim the Firestore cost reduction has been implemented (caching conversation in-memory, removing redundant data_protection_level reads, translation persist fix, finally block cleanup, etc.), but this PR only adds this coordination YAML file. Neither scope contains any actual code changes:
kelvin-transcribe-cache:status: planned—backend/routers/transcribe.py,backend/database/conversations.py,backend/models/transcript_segment.py, and the related unit tests are unmodified.kenji-pusher-storage-cache:status: planned—backend/routers/pusher.py,backend/utils/other/storage.py, and related unit tests are unmodified.
Merging this PR as-is would close issue #5377 (Closes #5377 in the description) without any of the described fixes actually being in the codebase. The PR description and title should either be updated to reflect that this is only a planning/coordination PR, or the implementation commits need to be included before merging.
| do_not_touch: | ||
| - kelvin must not edit pusher.py or storage.py | ||
| - kenji must not edit transcribe.py, conversations.py, or transcript_segment.py |
There was a problem hiding this comment.
do_not_touch constraints are unenforceable in this coordination format
The do_not_touch entries (e.g., "kelvin must not edit pusher.py or storage.py") are free-text strings, not machine-readable constraints. Git and GitHub have no mechanism to enforce these from a YAML file. If the intention is to enforce file ownership boundaries during the collaborative implementation, consider using a CODEOWNERS file instead, which GitHub natively enforces via required reviewer rules. For example:
backend/routers/pusher.py @kenji-agent
backend/utils/other/storage.py @kenji-agent
backend/routers/transcribe.py @kelvin-agent
backend/database/conversations.py @kelvin-agent
beastoin
changed the title
E2E Validation — Firestore Cache Optimization (PR #5378)Test environment: Dev GKE ( 1. WebSocket Transcription Test (PCM linear16)
Results:
2. VAD Gate Metrics (on disconnect)3. Transcript Persistence (API verification)Sample segments:
4. Log Validation
5. Conversation RolloverConversation ID change handled correctly — old one cleaned up, new one created and used for all subsequent segments. Pass/Fail Assessment
Verdict: E2E validation PASS by AI for @beastoin |
Before/After Cost Comparison — 15-Minute Real Audio TestsTest Setup
Functional Results (Both Runs)
Both runs produced comparable transcription results with zero errors. Firestore Read Analysis — Backend-ListenBaseline (main): Every segment update cycle calls PR #5378:
Firestore Read Analysis — Pusher (kenji's changes)
Combined Savings
Log Validation
Deploy Details
Verdict99% Firestore read reduction per session with no functional regression. Target was 15-20% cost reduction; actual savings far exceed the target. by AI for @beastoin |
beastoin
force-pushed
the
collab/5377-integration
branch
from
2b6a1b7 to
360f5be
Compare
Skip per-chunk Firestore read when caller provides cached protection level. Falls back to DB read when not provided (backward compat). Part of #5377 — reduces ~720 Firestore reads/hour/user. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fetch once when private_cloud_sync is enabled, pass to all upload_audio_chunk calls. Eliminates per-chunk Firestore reads. Part of #5377. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies: skips DB when level provided, falls back when None, correct .bin/.enc paths for standard/enhanced levels. Part of #5377. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies: fetches level once when sync enabled, skips when disabled, passes cached level to all upload calls. Part of #5377. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… redundant Firestore read
…st, add finally cleanup
beastoin
force-pushed
the
collab/5377-integration
branch
from
360f5be to
5762d41
Compare
|
lgtm. |
1 participant
Summary
data_protection_levelto write functions to skip redundant Firestore readsupload_audio_chunkNotFoundsafety (non-NotFound errors still propagate)Changes
backend/database/conversations.py: Optionaldata_protection_levelparam onupdate_conversation_segments(),NotFoundsafetybackend/routers/transcribe.py: In-memory conversation cache, translate ID guard, finally block cleanupbackend/routers/pusher.py: Session-level protection level cachingbackend/utils/other/storage.py: Optionaldata_protection_levelparam onupload_audio_chunk()backend/test.shTesting
backend/test.shpasses (pre-existing failures intest_process_conversation_usage_context.pyunrelated)Review Cycles
except Exceptiontoexcept NotFound— non-NotFound errors propagatelanguage_cache.clear()→language_cache.cache.clear()(class has no.clear()method)Estimated Impact
Closes #5377
by AI for @beastoin