Harden Firestore cleanup: configurable concurrency and explicit RPC timeouts

[groupthinking]

Problem

CodeRabbit's review of #1170 raised two hardening suggestions for
FirestoreStateService.cleanup_old_states() that are correct in principle but
sit outside the scope of that PR, which was a pure latency change. Filing them
here so they are not lost.

1. Make CLEANUP_DELETE_CONCURRENCY environment-configurable

src/youtube_extension/services/cloud/firestore_state.py currently hardcodes:

CLEANUP_DELETE_CONCURRENCY = 16

Suggestion was int(os.getenv("CLEANUP_DELETE_CONCURRENCY", "16")).

Deferred because:

• The equivalent constant merged in ad7e2c1 (TAG_WRITE_CONCURRENCY = 8 in
  intelligent_cache.py) is also a plain module constant. Making only this one
  configurable is inconsistent.
• A bare int(os.getenv(...)) raises ValueError at import time on a
  malformed value, turning a typo into an unimportable module. It needs a
  validating parse with a fallback, which is its own change with its own tests.

If this is done, do both constants together behind one shared helper.

2. Pass an explicit timeout to doc.reference.delete()

Suggestion was to bound each delete with a deadline so a hung RPC cannot occupy
a pool worker indefinitely.

Deferred because:

• There is no existing timeout constant in the module (grep -n 'timeout\|TIMEOUT'
  returns nothing), so this means inventing a deadline policy rather than reusing one.
• Any value chosen would sit on top of the Firestore async client's own retry and
  deadline configuration; picking one without knowing that baseline risks
  converting slow-but-successful deletes into tallied failures.
• The pre-existing sequential loop had no timeout either, so this is not a
  regression introduced by perf: delete expired Firestore states concurrently under a bound #1170.

The right fix is probably a module-level deadline derived from the client config,
applied consistently to all Firestore calls in the service, not just cleanup.

Acceptance criteria

• Concurrency constants parsed from env with validation and a safe fallback, applied to both firestore_state.py and intelligent_cache.py
• A documented timeout policy for Firestore RPCs in firestore_state.py, with tests covering the timeout-as-failure path
• No behaviour change when the env vars are unset

Scope

src/youtube_extension/services/cloud/firestore_state.py,
src/youtube_extension/backend/services/intelligent_cache.py, and their unit tests.

[linear-code]

GRV-216

[github-actions]

👋 Thanks for opening this issue! It has been automatically labeled for triage.

[coderabbitai]

🔗 Related PRs

#207 - fix(security): SECRET_KEY, CORS wildcard, SQL injection, command injection (#193 #195 #196 #197 #200) [merged]
#310 - fix(ops): pipeline timeouts + Sentry web wiring [merged]
#454 - fix(security): OWASP Top 10 fixes — billing cookie forgery + prod CORS hardening [merged]
#648 - fix: address CodeRabbit review findings on merged perf pass (#642) [merged]
#859 - fix(cost-monitor): gate budget-alert webhooks + Discord payload + document env var (supersedes #728) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[groupthinking]

Triage note — this is currently blocked on #1170, and the issue's own guidance means it should not be started as a partial fix.

Item 1 cannot be built on main today

CLEANUP_DELETE_CONCURRENCY does not exist on main. It is introduced by #1170, which is still open:

$ git fetch origin main && git grep -n "CLEANUP_DELETE_CONCURRENCY" origin/main -- src/
(no output)

$ gh pr view 1170 --json state,mergeable,mergeStateStatus
OPEN MERGEABLE BLOCKED

So the firestore_state.py half of AC-1 has no constant to make configurable yet. And since this issue explicitly says:

If this is done, do both constants together behind one shared helper.

…doing only the intelligent_cache.py half would contradict the issue's own stated rationale (that making one of the two configurable is the inconsistency being avoided). There is no useful partial slice here.

Suggested sequencing: #1170 merges → then this issue becomes buildable in a single coherent change.

Implementation notes for whoever picks this up

Two things that are non-obvious from the issue text, both verified against main (ad7e2c1):

1. The two constants have different safety properties at their use sites, so a shared parse helper is not sufficient on its own.

TAG_WRITE_CONCURRENCY is never consumed directly — it goes through _resolve_tag_write_limit() (intelligent_cache.py L50-57):

return max(1, min(TAG_WRITE_CONCURRENCY, max_connections - TAG_WRITE_POOL_RESERVE))

That already clamps to >= 1 and to the pool budget, so an operator-supplied value is bounded at the point of use regardless of what it is. For this constant, env support only needs a safe parse (reject non-integers, fall back).

CLEANUP_DELETE_CONCURRENCY (as written in #1170) has no equivalent clamp — it is used to size the semaphore directly. So it needs the parse and a floor/ceiling, or it inherits a ValueError/0-concurrency deadlock from a bad value.

If the shared helper does the clamping itself (e.g. _env_int(name, default, minimum=1, maximum=...)), both sites get the same guarantee and _resolve_tag_write_limit()'s existing clamp becomes belt-and-braces rather than the only defence.

2. AC-3 ("no behaviour change when the env vars are unset") is directly testable — assert the resolved values still equal 8 / 16 with the env cleared, and that a malformed value ("abc", "", "-1", "0") logs and falls back rather than raising at import. That last case is the actual regression this issue exists to prevent.

Heads-up on file contention

intelligent_cache.py also has an open PR against it (#1179, event-loop ownership contract for RedisCacheLayer). That PR does not touch L38-57, so the two should not conflict textually, but whoever takes this should rebase rather than assume.