perf(metrics): move MetricsService disk I/O off the event loop

[groupthinking]

Canonical issue

Closes #1193

Outcome

MetricsService no longer performs disk I/O on the event loop.

record_metric flushes the whole metric store to disk every tenth point (metrics_service.py:145-146), and that write ran as a synchronous open()/write() directly on the loop — on a request-serving path. load_persisted_metrics had the same problem on the read side. Both now go through await asyncio.to_thread(...).

What this does and does not change

	Before	After
Disk write blocks the event loop	✅ yes, for the whole write	❌ no
json.dumps of the store runs on the loop	✅ yes	✅ still yes — see Scope
Bytes written / file format	—	unchanged
Public method signatures	—	unchanged
New dependencies	—	none

This is not a throughput win. The write costs the same wall-clock time; it simply stops stalling every other coroutine while it happens.

Scope

• _persist_metrics — write path moved off-loop via a _write_text_file static helper
• load_persisted_metrics — read + parse moved off-loop via a _read_json_file static helper
• These two helpers are now the only remaining open( sites in the module

Deliberately out of scope: export_metrics("json") (L264) still runs json.dumps(..., indent=2) on the loop. It is a public method with its own callers and its own synchronous contract; converting it would be a breaking API change. That is the larger remaining cost for a big metric store and is worth a follow-up issue — I did not want to smuggle an API break into a perf PR.

Design notes

Why to_thread and not aiofiles? aiofiles is itself a thread-pool shim — every method is await loop.run_in_executor(...), so it would cost one executor round-trip per call. Here there is a single write, so to_thread is the same mechanism with less indirection and no new dependency.

Why static helpers? asyncio.to_thread(self._write_text_file, path, text) needs a plain callable. Keeping them @staticmethod avoids capturing self in the worker thread, so the thread cannot observe a half-mutated instance.

Risk

Low. No signature, format or behaviour change. The failure mode of to_thread is that exceptions surface identically at the await point — covered by the preserved-behaviour guards below.

One genuine hazard was checked explicitly: converting a sync call to to_thread inside an object that a caller tears down in a finally creates a use-after-close race. MetricsService has no close()/teardown method and no caller disposes of it in a finally, so that pattern does not apply here.

Verification

All results below are at head b55b6e555.

78 passed

• 72 pre-existing tests pass with zero test edits
• +6 new in TestPersistenceIsSerialisedAndAtomic (this round) on top of the 7 in TestDiskIoDoesNotBlockEventLoop

Round 2 non-vacuity, by behavioural mutation. All three round-2 changes were reverted at once — lock removed, Path.exists() restored, atomic write reverted to a direct truncating open(). Exactly four tests fail, one per dimension:

FAILED test_concurrent_persists_never_overlap          (lock)
FAILED test_reader_never_sees_a_truncated_file         (atomic write)
FAILED test_load_does_not_stat_the_path_on_the_loop    (exists check)
FAILED test_write_cleans_up_temp_file_on_failure       (temp cleanup)
4 failed, 74 passed

Restoring gives 78 passed. Targeted rather than uniform failures — a uniform "everything failed" would have meant the proof was structural, not behavioural.

Round 1 non-vacuity (the original off-loop change) is unchanged: replacing await asyncio.to_thread(self._write_text_file, ...) with a direct call fails exactly the 3 heartbeat tests.

Honest note: of the 13 tests in this PR, 6 discriminate (4 above + the round-1 heartbeats) and the rest are preserved-behaviour guards that pass under both old and new code by design.

ruff: All checks passed! on both touched files, exact parity with main. Checked in-repo, not from /tmp — this repo's per-file-ignores are path-relative and exempt tests/ from E402.

Production evidence

Reachable from the primary production entrypoint (youtube_extension.main:app, root Dockerfile:93):

router.py:68 imports MetricsService → router.py:194-196 get_metrics_service() → router.py:467 Depends(...) on a live endpoint → router.py:478 TranscriptActionWorkflow → transcript_action_workflow.py:870-882 _record_metric, called at L167, 641, 646, 659, 664, 787, 792, 828, 833.

Confirmed against a transitive import-closure audit of all 7 deployed entrypoints; backend.services.metrics_service is in the closure.

⚠️ grep "\.record_metric(" alone is misleading — it matches only performance_monitor.record_metric, a different class. The wrapper indirection above is the real path.

Agent handoff

Follow-up worth filing: move export_metrics's json.dumps off the loop behind a new async method, leaving the existing sync one intact for current callers.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
v0-uvai	Canceled		Aug 1, 2026 10:39pm

[coderabbitai]

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one) (1)

• [‘architecture-gap’, ‘bug’, ‘ci-cd’, ‘ci/cd’, ‘copilot-rabbit’, ‘documentation’, ‘duplicate’, ‘enhancement’, ‘frontend’, ‘github_actions’, ‘good first issue’, ‘help wanted’, ‘high-priority’, ‘invalid’, ‘javascript’, ‘ml-model’, ‘needs-triage’, ‘pipeline-critical’, ‘placeholder-code’, ‘priority:high’, ‘python’, ‘python:uv’, ‘question’, ‘styling’, ‘tests’, ‘v0’]

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8e980f48-07a2-4c89-a4e4-04fef3fb8e9c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA b55b6e5.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

[groupthinking]

@coderabbitai review

Specific challenges rather than a general pass:

1. _write_text_file and _read_json_file are @staticmethod so asyncio.to_thread gets a plain callable and the worker thread cannot observe a half-mutated self. Is there still shared mutable state reachable from the thread — e.g. can self.metrics be mutated by another coroutine while the serialized text is being written, producing a torn file?

2. _persist_metrics is triggered from record_metric on a % 10 == 0 counter. Two concurrent requests can therefore both enter _persist_metrics and both write the same path with no lock. That race pre-exists this PR, but does moving the write off-loop widen the window enough to matter in practice?

3. I deliberately left export_metrics("json") doing json.dumps on the loop, because it is a public sync method with existing callers. Is that the right boundary for this PR, or is the remaining on-loop serialization large enough that the change is not worth much without it?

4. MetricsService.__init__ sets self.metrics_file = Path("logs/metrics.json") — a relative path, so behaviour depends on the process CWD. Not introduced here, but does moving to a worker thread interact badly with anything that changes CWD at runtime?

5. Do the 3 heartbeat tests actually discriminate the change, or is there a way they would pass against the original blocking implementation?

[github-actions]

Agent Completion Truth Gate: NOT_APPLICABLE

Evidence agrees.

Machine-readable verdict

{
  "details": {},
  "reasons": [],
  "verdict": "not_applicable"
}

Workflow evidence

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== changed files =='
git diff --name-status HEAD^ HEAD || true

echo '== metrics-service candidates =='
fd -i 'metrics_service.py' .
fd -i '*metrics*' . | head -80

echo '== outlines =='
while IFS= read -r f; do
  ast-grep outline "$f" --items all --type function,method,class --match 'MetricsService|record_metric|_persist_metrics|load_persisted_metrics|export_metrics|_write_text_file|_read_json_file|TestDiskIoDoesNotBlockEventLoop' || true
done < <(fd -i 'metrics_service.py' .; fd -i '*metrics*test*.py' .; fd -i 'test*metrics*.py' .)

echo '== implementation and test references =='
rg -n -C 5 -g '*.py' \
  'class MetricsService\b|async def _persist_metrics\b|def _write_text_file\b|def _read_json_file\b|async def load_persisted_metrics\b|async def record_metric\b|def export_metrics\b|TestDiskIoDoesNotBlockEventLoop|heartbeat|chdir\(|metrics_file' .

echo '== PR diff for relevant Python files =='
git diff HEAD^ HEAD -- '*.py'

Length of output: 1781

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

impl='src/youtube_extension/backend/services/metrics_service.py'
tests='tests/unit/test_metrics_service.py'

echo '== implementation outline =='
ast-grep outline "$impl" --items all --view expanded

echo '== implementation: relevant methods and imports =='
rg -n -C 12 \
  '^(import|from) |class MetricsService\b|async def record_metric\b|async def _persist_metrics\b|def _write_text_file\b|def _read_json_file\b|async def load_persisted_metrics\b|def export_metrics\b|metrics_file|self\.metrics' \
  "$impl"

echo '== test outline =='
ast-grep outline "$tests" --items all --view expanded

echo '== test: heartbeat and persistence tests =='
rg -n -C 18 \
  'class TestDiskIoDoesNotBlockEventLoop\b|async def test_.*(block|persist|record|load)|heartbeat|sleep\(|_write_text_file|_read_json_file|record_metric|load_persisted_metrics' \
  "$tests"

echo '== PR diff =='
git diff HEAD^ HEAD -- "$impl" "$tests"

Length of output: 16007

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== tracked metrics tests =='
git ls-files | rg -i '(^|/)(test_.*metrics.*|.*metrics.*test)\.py$' || true
echo '== filesystem metrics tests =='
fd -i --glob '*metrics*.py' . || true

echo '== heartbeat test definitions and bodies =='
rg -n -C 25 -g '*.py' \
  'TestDiskIoDoesNotBlockEventLoop|test_persist_does_not_block_event_loop|test_record_metric_flush_does_not_block|test_load_does_not_block_event_loop|heartbeat' . || true

echo '== export_metrics definition and call sites =='
rg -n -C 4 -g '*.py' \
  '\bexport_metrics\s*\(' . || true

echo '== runtime current-working-directory changes =='
rg -n -C 4 -g '*.py' \
  '\b(os\.)?chdir\s*\(' . || true

echo '== exact PR patch (no test path assumption) =='
git diff HEAD^ HEAD --name-only | while IFS= read -r f; do
  case "$f" in
    *.py) git diff HEAD^ HEAD -- "$f" ;;
  esac
done

Length of output: 48578

---

@groupthinking I found one production-relevant regression risk.

1. self.metrics is not reachable from the worker thread.
   _persist_metrics completes await self.export_metrics("json") before it calls asyncio.to_thread. The worker receives a Path and an immutable str. A later record_metric call can mutate self.metrics, but it cannot mutate the already serialized payload. This does not produce a torn file from shared metric state.

2. The off-loop change introduces a meaningful file-operation race.
   Before this PR, the synchronous write prevented another coroutine on the same event loop from entering another write or a load until the write completed. Now _persist_metrics yields at await asyncio.to_thread(...).

   Two persistence operations can overlap for different metric names, for explicit persist_metrics() calls, or after distinct % 10 == 0 boundaries. Two open(path, "w") calls can truncate and overwrite the same file out of order. A newer snapshot can also complete first, then an older snapshot can overwrite it.

   load_persisted_metrics can now also read while a worker thread truncates or writes the file. It can observe incomplete JSON and return False.

   This is a real behavior change, not only a wider pre-existing race. Add a service-level asyncio.Lock that covers serialization plus the worker-thread write. Also use atomic replacement, such as writing a temporary file and Path.replace(), if readers must never observe partial JSON. The lock must cover load_persisted_metrics too, or the atomic replacement must make concurrent reads safe.

3. Keeping json.dumps in scope is acceptable, but the stated API constraint is not accurate.
   export_metrics is already async def at metrics_service.py:264. Moving json.dumps to await asyncio.to_thread(...) would not change its async calling contract. The construction of export_data would still run on the event loop unless that work also moves.

   This PR still removes the blocking file operation. That is useful. For large stores, JSON construction and serialization can become the dominant remaining stall. I recommend a follow-up rather than expanding this PR, but the follow-up does not require an API-breaking conversion from sync to async.

4. The relative path has a new timing hazard.
   The repository search found no production chdir() call. However, Path("logs/metrics.json") resolves when the worker executes open(), not when _persist_metrics schedules the work. A runtime CWD change can cause the worker to write a different file, or fail because the new CWD has no logs/ directory. The error is swallowed after logging.

   Bind the path during initialization:

   self.metrics_file = (Path.cwd() / "logs" / "metrics.json").resolve()
   self.metrics_file.parent.mkdir(parents=True, exist_ok=True)

   This also preserves the test behavior because each test changes CWD before it creates MetricsService.

5. The three heartbeat tests do discriminate this change.
   Each test starts the heartbeat, but it does not allow a tick before the operation begins. With the old direct helper call, patched _slow_write or _slow_read blocks the event-loop thread in time.sleep(0.15). The heartbeat cannot execute, so ticks == 0. With asyncio.to_thread, the event loop remains schedulable and ticks > 0.

   Releasing the GIL inside time.sleep does not change this result. The event loop itself remains blocked in the synchronous helper call. The test design is sound.

I would address the write/load race before merge. The path binding is a low-cost hardening fix for the new worker-thread timing window.

_{You are interacting with an AI system.}

[Copilot]

Pull request overview

Moves MetricsService file reads and writes to worker threads to reduce event-loop blocking.

Changes:

• Adds threaded file I/O helpers.
• Adds responsiveness and behavior-regression tests.
• Preserves existing persistence formats and APIs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
metrics_service.py	Offloads persistence reads and writes.
test_metrics_service.py	Tests event-loop responsiveness and persistence behavior.

[groupthinking]

Reviewed at head 10542451d. Clean, well-scoped change, and the test suite is unusually rigorous — the heartbeat loop-responsiveness tests with behavioral-mutation proof (swapping to_thread for a direct call flips exactly the 3 discriminating tests while the 4 guards hold) genuinely demonstrate the property rather than just structure. to_thread over aiofiles is the right call for a single write, and the @staticmethod helpers correctly avoid capturing self in the worker thread.

One substantive point — non-blocking: concurrent flushes can now interleave on disk (a new failure mode).

Previously the write ran synchronously on the single-threaded loop, so two overlapping _persist_metrics calls serialized — each open('w')+write() completed atomically with respect to every other task, and the file always held one complete JSON document. After this change both writes are dispatched to the default thread-pool executor and can run in two worker threads simultaneously. I confirmed there is no asyncio.Lock in the class, and record_metric is reached from concurrent request paths plus get_system_metrics (which records three metrics per tick), so two points crossing the % 10 == 0 boundary close together will both flush. Two concurrent open(path, 'w') → truncate+write can leave a torn / partially-written metrics.json.

Impact is low, which is why I wouldn't block: persistence is best-effort (errors are swallowed), load_persisted_metrics discards the parsed result and catches JSONDecodeError → returns False, so a corrupt file is self-healing with no functional effect today. But since the point of the PR is hardening exactly this path, an atomic write closes it cleanly and also makes the write crash-safe (the current inline write isn't):

@staticmethod
def _write_text_file(path: Path, contents: str) -> None:
    tmp = path.with_suffix(path.suffix + ".tmp")
    with open(tmp, "w") as f:
        f.write(contents)
    os.replace(tmp, path)  # atomic on POSIX & Windows  (needs `import os`)

An asyncio.Lock around _persist_metrics would also serialize it, but temp-file + os.replace additionally survives a mid-write crash, so it's the stronger fix for the same line count.

Minor / pre-existing (not introduced here):

• load_persisted_metrics still discards the parsed JSON ("simplified … full restoration") — unchanged by this PR; just noting the read validates but doesn't apply the data.
• export_metrics("json")'s json.dumps staying on the loop is correctly called out as an intentional out-of-scope follow-up; agreed that folding an API break into a perf PR would be the wrong move.

CI: the code-quality checks are green (CodeQL, bandit, python-safety, trivy, npm-audit, dependency-review, validate, agent-completion/truth-gate, copilot reviewer). The red checks — Agent completion enforcement, PR Governance, Canonical issue and evidence, and gitleaks (working tree) — are the repo-wide governance/infra gates rather than failures of this diff (no secrets are added here; the gitleaks job scans the whole working tree, not the PR delta). These are the same gates that were red on #1108 / #1103 / #1098.

Net: LGTM on the mechanism. The concurrent-write hardening above is worth a look but is non-blocking.

---

Generated by Claude Code

[groupthinking]

Automated review — code is sound; the only real-CI red is an unrelated false positive

Change (verified against the GitHub PR diff, which is authoritative here — two-dot git diff is unreliable on this repo's rewritten history):

• _persist_metrics and load_persisted_metrics now offload their blocking open()/write()/json.load() via await asyncio.to_thread(...) behind two @staticmethod helpers.
• Confirmed the helpers' type hints resolve — asyncio, json, Path, and Any are all already imported in metrics_service.py.
• The @staticmethod choice is correct: it avoids capturing self into the worker thread, so the thread can't observe a half-mutated instance.
• load_persisted_metrics discards the parsed result under both old and new code, so behaviour is preserved (it still just validates the file parses).
• Scope call on export_metrics("json") (leaving its on-loop json.dumps for a follow-up rather than smuggling an API break into a perf PR) is the right judgement.

The PR's own mutation-based non-vacuity argument checks out: replacing the to_thread call with a direct call is exactly what should flip only the 3 heartbeat-responsiveness tests while the 4 preserved-behaviour guards stay green.

CI status: all real code checks are green — test, lint-python, lint-frontend, guards, build, validate, CodeQL, bandit, trivy, python-safety, dependency-review, and copilot-pull-request-reviewer. agent-completion/truth-gate also passes.

The one red real-CI check, gitleaks (working tree), is not caused by this PR. It fires on uv.lock:5129 — a parso PyPI sha256: artifact hash misdetected by the default square-access-token rule — and this PR never touches uv.lock. That lockfile line landed on main recently, so it now reds the gitleaks check on every new branch (PRs cut earlier, e.g. #1075, still pass it). Fix opened separately in #1197 (allowlists the generated lockfile; verified exit 1 → exit 0 with the CI's own gitleaks 8.18.4). Merging #1197 first, or rebasing this PR onto it, clears the red here.

Remaining blockers to merge are not code: the Agent completion enforcement / PR Governance / Canonical issue and evidence governance checks are red (the same evidence/publication failures seen across the open-PR backlog), and main is protected with no automerge label — so this needs a human merge decision. From a code-correctness standpoint this is ready.

---

Generated by Claude Code

[groupthinking]

@copilot Both findings were correct and both are fixed in b55b6e555. Thank you — these are exactly the class of bug I was most worried about, because I introduced both of them in this PR.

#	Finding	Verdict	Fix
1	Path.exists() is still a synchronous stat() on the loop, contradicting the PR's own claim	✅ Valid	Check removed; FileNotFoundError from the threaded open maps to False
2	Off-loop writes lose the loop's implicit serialisation; concurrent "w" opens can truncate/interleave	✅ Valid	asyncio.Lock around persistence, plus atomic temp-file + os.replace

On finding 1 — you were right that it contradicted the stated outcome, and right that dropping it also closes a TOCTOU window. The observable contract is unchanged: test_load_still_returns_false_when_file_absent pins the cold-start result, and test_load_does_not_stat_the_path_on_the_loop patches Path.exists with a counter and asserts it is called zero times. That second test fails against the previous revision.

On finding 2 — this is the more serious one and I want to be explicit that it was a regression this PR created. Before the change the write was synchronous, so the event loop serialised it for free; moving it to a worker thread silently removed that guarantee for a singleton service. I implemented both halves of your suggestion:

• Serialisation: an asyncio.Lock held across snapshot and write, so the file always reflects one coherent export. The lock is created lazily and keyed to the running loop rather than in __init__ — a process-wide singleton constructed on one loop would otherwise bind a lock to a loop that is already closed.
• Atomicity: tempfile.mkstemp in the same directory, then os.replace. The temp file is unlinked if the write fails, so a failure leaves no litter.

Proof is a mutation test that reverts all three changes simultaneously and produces exactly four targeted failures, one per dimension — see the Verification section.

One thing I did not do: I did not add a queue or coalescing. _persist_metrics fires on every 10th metric, so under sustained load callers will now serialise behind the lock rather than racing. That is correct but it does mean the write is a serialisation point. I judged a queue to be out of scope for this PR; if you disagree I will file it as a follow-up rather than expand this one.

@coderabbitai review — please re-review at b55b6e555, and specifically challenge the lock's loop-keying and whether holding it across export_metrics() (which walks every metric series) makes the critical section unnecessarily long.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

impl=$(fd -i -t f '^metrics_service\.py$' . | head -1)
tests=$(fd -i -t f '^test_metrics_service\.py$' . | head -1)

printf '== revision ==\n'
git rev-parse HEAD
printf '\n== files ==\n%s\n%s\n' "$impl" "$tests"

printf '\n== implementation outline ==\n'
ast-grep outline "$impl" --items all --view expanded

printf '\n== lock, persistence, and export implementation ==\n'
rg -n -C 18 \
  '^(import|from) |class MetricsService\b|async def record_metric\b|async def _persist_metrics\b|async def load_persisted_metrics\b|async def export_metrics\b|def _write_text_file\b|def _read_json_file\b|_persist|_lock|Lock\(|get_running_loop|metrics_file|self\.metrics' \
  "$impl"

printf '\n== relevant tests ==\n'
rg -n -C 16 \
  'class Test.*(Persistence|DiskIo)|async def test_.*(persist|load|concurrent|truncated|temp|lock)|_persist_metrics|_persist_lock|asyncio\.Lock|export_metrics|to_thread|heartbeat' \
  "$tests"

printf '\n== exact relevant diff ==\n'
git diff HEAD^ HEAD -- "$impl" "$tests"

Length of output: 19643

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== metrics-related test files ==\n'
fd -i -t f '*metrics*.py' . | sort

printf '\n== persistence and lock tests ==\n'
rg -n -C 14 -g '*.py' \
  'TestPersistenceIsSerialisedAndAtomic|TestDiskIoDoesNotBlockEventLoop|test_concurrent_persists_never_overlap|test_reader_never_sees_a_truncated_file|test_load_does_not_stat_the_path_on_the_loop|test_write_cleans_up_temp_file_on_failure|_get_persist_lock|_persist_lock_loop|new_event_loop|set_event_loop' \
  .

printf '\n== MetricsService construction and service-provider sites ==\n'
rg -n -C 7 -g '*.py' \
  'MetricsService\s*\(|get_metrics_service\s*\(|metrics_service\s*=' \
  src tests

printf '\n== event-loop and thread entry points near service use ==\n'
rg -n -C 5 -g '*.py' \
  'new_event_loop\s*\(|asyncio\.run\s*\(|run_coroutine_threadsafe\s*\(|Thread\s*\(' \
  src tests

Length of output: 1499

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== metrics-related test files ==\n'
fd -i -t f --glob '*metrics*.py' . | sort

printf '\n== persistence and lock tests ==\n'
rg -n -C 14 -g '*.py' \
  'TestPersistenceIsSerialisedAndAtomic|TestDiskIoDoesNotBlockEventLoop|test_concurrent_persists_never_overlap|test_reader_never_sees_a_truncated_file|test_load_does_not_stat_the_path_on_the_loop|test_write_cleans_up_temp_file_on_failure|_get_persist_lock|_persist_lock_loop|new_event_loop|set_event_loop' \
  . || true

printf '\n== MetricsService construction and service-provider sites ==\n'
rg -n -C 7 -g '*.py' \
  'MetricsService\s*\(|get_metrics_service\s*\(|metrics_service\s*=' \
  src tests || true

printf '\n== event-loop and thread entry points near service use ==\n'
rg -n -C 5 -g '*.py' \
  'new_event_loop\s*\(|asyncio\.run\s*\(|run_coroutine_threadsafe\s*\(|Thread\s*\(' \
  src tests || true

Length of output: 51731

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '== commit files ==\n'
git diff-tree --no-commit-id --name-status -r HEAD

printf '\n== tracked test paths ==\n'
git ls-tree -r --name-only HEAD | rg -i '(^|/)(test|tests)(/|_)|metrics.*test|test.*metrics' | head -250 || true

printf '\n== service container structure and metrics registration ==\n'
container='src/youtube_extension/backend/containers/service_container.py'
ast-grep outline "$container" --items all --view expanded
rg -n -C 14 \
  'class ServiceContainer\b|def get_service\b|def _create_metrics_service\b|metrics_service|singleton|self\._services|self\.services' \
  "$container"

printf '\n== deployed application entrypoint and container initialization ==\n'
rg -n -C 10 -g '*.py' \
  'FastAPI\s*\(|get_service_container\s*\(|ServiceContainer\s*\(|initialize.*container|container\.(initialize|register)' \
  src/youtube_extension/main.py src/youtube_extension/backend 2>/dev/null || true

printf '\n== persisted-metrics test content from commit, if present ==\n'
git ls-tree -r --name-only HEAD | rg -i 'metrics.*(test|spec)|test.*metrics' | while IFS= read -r f; do
  printf '\n--- %s ---\n' "$f"
  git show "HEAD:$f" | rg -n -C 10 \
    'TestPersistenceIsSerialisedAndAtomic|TestDiskIoDoesNotBlockEventLoop|concurrent_persists|truncated_file|does_not_stat|temp_file|persist_lock' || true
done

Length of output: 46234

---

@groupthinking Re-review complete at b55b6e555.

The persistence fix is sound for the deployed FastAPI path.

• load_persisted_metrics() no longer performs Path.exists() on the event loop.
• _write_text_file() uses a same-directory temporary file and os.replace().
• The lock covers writes and reads.
• The tests cover the important same-loop races and failure cleanup.

The lock must remain held through await asyncio.to_thread(...).

If the lock releases before the write completes, this order can occur:

1. Persist A snapshots older metrics.
2. Persist B snapshots newer metrics.
3. Persist B writes first.
4. Persist A writes last.

The file then regresses to the older snapshot. The current critical section prevents that result.

Holding the lock during export_metrics("json") does not make the critical section materially longer for concurrent coroutines. export_metrics("json") has no suspension point. It builds the snapshot and calls json.dumps() synchronously before any other coroutine can run. The event loop still pays that serialization cost, but moving it is correctly out of this PR’s disk-I/O scope.

There is one boundary to document or harden:

_get_persist_lock() is not safe for concurrent calls from two live event loops. Loop A can acquire lock A. Loop B then replaces self._persist_lock with lock B. Both persists can then proceed concurrently. The current tests cover only one event loop.

I found no evidence that the primary FastAPI deployment shares this MetricsService singleton across live event loops. The service container creates one process-local singleton, and the inspected request path uses the application event loop. Therefore, this is not a current merge blocker.

If multi-loop or cross-thread access becomes supported, do not rely on replacing one asyncio.Lock. Define one-loop ownership for MetricsService, or add synchronization that works across threads for the persistence pipeline.

A queue or coalescing mechanism is not required for correctness here. It is a valid future throughput improvement if persistence frequency becomes a production bottleneck.

🐇

_{You are interacting with an AI system.}

[linear-code]

GRV-229