perf(video): run enhanced-result persistence off the event loop

[groupthinking]

Head: fd5f49ede

Canonical issue

Closes #1202

Outcome

EnhancedVideoProcessor._save_enhanced_result was async def with no await in its body — mkdir, both file writes, and json.dump all ran on the event loop. It is reached in production from process_video (line 189) via service_container.py:253 → video_processor_factory.get_video_processor().

This change does	This change does NOT
Stop the save from stalling the event loop	Make saving faster in wall-clock terms
Pay json.dumps off-loop as well as the writes	Make concurrent saves parallel
Cost one thread hop for the whole save, not one per syscall	Add batching, queuing or coalescing
Make each write atomic, so a reader never sees a partial file	Change the on-disk layout or filenames
Clean up the temp file when a write fails	Change any public signature

json.dump(obj, f) writes to the file object incrementally, so a large metadata dict became many small write() syscalls. It is now a single json.dumps + one write, performed in the worker thread.

Scope

• src/youtube_extension/backend/enhanced_video_processor.py — two new static helpers (_atomic_write_text, _write_result_files); _save_enhanced_result now issues one asyncio.to_thread dispatch. Added contextlib and threading imports.
• tests/unit/test_enhanced_video_processor.py — one new test class, 6 tests. No existing test was modified.

Explicitly out of scope: the other json.loads sites in this module (lines 476/484/547/553/704) parse small model responses on the loop; and enhanced_video_processor.py:329's open() is already inside a nested def _transcribe() dispatched via asyncio.to_thread — correctly off-loop, deliberately untouched.

Design notes

Why one dispatch, not three. to_thread per syscall would cost three thread hops. _write_result_files groups mkdir + both writes so the save costs one.

Why the writes had to become atomic in the same change. Review on #1194 established the rule: moving a write off the event loop removes the loop's implicit serialisation. open(path,'w') truncates first, so once two saves can genuinely overlap, a reader can observe an empty or short file. _atomic_write_text writes "{path}.{pid}.{tid}.tmp" and os.replaces it into place — atomic on POSIX — and unlinks the temp file on any failure. The pid/tid suffix means two writers cannot collide on the temp name.

No caller teardown hazard. Unlike #1190, _save_enhanced_result holds no instance resource across the await; it opens and closes files entirely inside the worker thread, so there is no finally: close() race in any caller.

Risk

Low. The 98 pre-existing tests in this file pass unmodified, including the three that already exercise _save_enhanced_result by patching builtins.open and pathlib.Path.mkdir. The failure contract is unchanged: any exception is still logged and still returns "".

The one behavioural difference a caller could observe: the target file now appears atomically rather than growing incrementally. Nothing in the repo tails these files while they are written.

Verification

104 passed in 3.30s

= 98 pre-existing (zero edits) + 6 new in TestSaveDoesNotBlockEventLoop.

Non-vacuity — both dimensions mutated simultaneously (atomic write → plain truncating open, and to_thread → direct on-loop call):

FAILED ...::test_save_does_not_stall_the_event_loop
FAILED ...::test_json_serialisation_also_runs_off_the_loop
FAILED ...::test_temp_file_is_cleaned_up_when_the_write_fails
FAILED ...::test_reader_never_observes_a_partially_written_file
4 failed, 100 passed

Restoring the source → 104 passed. One failure per dimension, so no new test duplicates another.

Honest split: 4 of the 6 new tests discriminate; 2 are guards (test_save_writes_readable_markdown_and_metadata, test_no_temp_files_remain_after_a_successful_save) that pass under both variants and exist to pin content correctness and cleanliness.

ruff: exact parity with origin/main — the same 6 pre-existing findings (1 × E402, 5 × F841) before and after; zero introduced.

Review round 2 — pair atomicity (fd5f49ede)

@Copilot correctly found that the two writes were individually atomic but not atomic as a pair: with one-second filename precision two saves of the same video in the same second target the same paths and can interleave, pairing one save's markdown with another's metadata. Both writes are now held together under a module-level threading.Lock (_RESULT_WRITE_LOCK), taken inside the worker thread so the event loop is still never blocked.

106 passed  (104 + 2 new in TestConcurrentSavesWriteMatchedPairs)

Non-vacuity — removing the lock:

1 failed, 105 passed   # only test_concurrent_saves_never_mix_markdown_with_foreign_metadata

The single-writer guard test passes under both variants, as a guard should.

Production evidence

enhanced_video_processor and video_processor_factory are both in the transitive import closure of the deployed entrypoints (youtube_extension.main:app, root Dockerfile:93). Call chain: service_container.py:253 → video_processor_factory.py:63-65 → EnhancedVideoProcessor() → process_video (L140) → _save_enhanced_result (L189).

Agent handoff

Known coverage gap, named deliberately: the heartbeat tests prove the loop keeps running for _save_enhanced_result specifically. They do not prove anything about the json.loads sites elsewhere in this module, which remain on-loop by design.

[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:58pm

[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: a9f0c390-5447-4009-8291-8c5b2c82544c

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.}

[groupthinking]

@coderabbitai review

Please review head 4d8d250e5. Specific challenges I would like attacked rather than a general pass:

1. Atomicity claim. _atomic_write_text writes "{path}.{pid}.{tid}.tmp" then os.replace. Is the pid+thread-id suffix actually sufficient to prevent temp-name collisions, given _save_enhanced_result can be entered concurrently and to_thread reuses threads from a bounded pool? Is there a sequence where two saves for the same video_id within the same one-second timestamp window still corrupt each other?

2. Cancellation. await asyncio.to_thread(...) is not cancellation-safe — this exact issue was a real regression on perf(cloud-tasks): move blocking gRPC calls off the event loop #1190. I claim it is harmless here because _save_enhanced_result holds no instance resource across the await and no caller does finally: X.close() around process_video. Please verify that claim independently rather than take it.

3. except BaseException in the cleanup path. I re-raise after unlinking. Is contextlib.suppress(OSError) the right width there, or does it mask a real failure signal? Note I deliberately rejected a suppress(BaseException) hardening on perf(cloud-tasks): move blocking gRPC calls off the event loop #1190 because I could not write a test that discriminated it.

4. Test honesty. I claim 4 of 6 new tests discriminate and 2 are guards, proven by a simultaneous two-dimension mutation (4 failures / 100 passed). Is test_reader_never_observes_a_partially_written_file genuinely deterministic, or is it timing-dependent and liable to flake in CI? It relies on a reader task sampling a 400KB file while a writer loops 12 times through to_thread.

5. Did I miss a blocker? _save_enhanced_result still constructs Path objects and formats a timestamp on the loop. Are either of those meaningful, or is datetime.now().strftime genuinely negligible next to what I moved?

Context: this is the 9th in a series of event-loop-blocking fixes. Reviewers have caught real correctness bugs in #1186, #1188, #1190 and #1194 — including two regressions I introduced myself — so please be adversarial.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== changed paths and diff ==='
git diff --find-renames origin/main...HEAD -- \
  src/youtube_extension/backend/enhanced_video_processor.py \
  tests/unit/test_enhanced_video_processor.py

echo '=== enhanced processor outline ==='
ast-grep outline src/youtube_extension/backend/enhanced_video_processor.py \
  --items all --view expanded

echo '=== focused enhanced processor source ==='
nl -ba src/youtube_extension/backend/enhanced_video_processor.py | sed -n '130,230p'
nl -ba src/youtube_extension/backend/enhanced_video_processor.py | sed -n '1,80p'

echo '=== save and process callers/usages ==='
rg -n -C 5 --glob '*.py' \
  '(?:\._save_enhanced_result\s*\(|\bprocess_video\s*\(|get_video_processor\s*\()' \
  src tests

echo '=== new test class and nearby fixtures/helpers ==='
rg -n -C 8 --glob '*.py' \
  '(?:class TestSaveDoesNotBlockEventLoop\b|test_reader_never_observes_a_partially_written_file\b|test_save_does_not_stall_the_event_loop\b|test_json_serialisation_also_runs_off_the_loop\b)' \
  tests/unit/test_enhanced_video_processor.py

Length of output: 14841

---

The first inspection reached the diff, but the environment lacks nl. I will use sed and rg to inspect the missing ranges and cancellation boundaries.

_{You are interacting with an AI system.}

[Copilot]

Pull request overview

Moves enhanced-result serialization and atomic file persistence off the event loop.

Changes:

• Dispatches directory creation, JSON serialization, and writes through one worker thread.
• Adds atomic temporary-file replacement and cleanup.
• Adds six persistence and responsiveness tests.

Reviewed changes

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

File	Description
src/youtube_extension/backend/enhanced_video_processor.py	Adds off-loop, atomic result persistence.
tests/unit/test_enhanced_video_processor.py	Tests responsiveness, content, cleanup, and atomic reads.

[groupthinking]

@copilot — valid finding, accepted and fixed in fd5f49ede.

You were right that atomicity per-file is not the same as atomicity of the pair. With one-second filename precision, two saves of the same video in the same second resolve to the same two paths, and the interleaving you described (A-markdown, B-markdown, B-metadata, A-metadata) leaves B's markdown next to A's metadata.

Fix: both writes are now held together under a module-level _RESULT_WRITE_LOCK.

• It is a threading.Lock, not an asyncio.Lock, because the writes execute inside the to_thread worker — an asyncio lock would not exclude a second worker thread.
• It is held across only the two file writes, and only inside a worker thread, so it never blocks the event loop — the point of the PR is preserved.
• I chose a single global lock over a path-keyed map: saves are a once-per-video-completion operation, so global serialisation of two file writes costs nothing measurable, and a path-keyed dict would grow unboundedly.

Proof it discriminates — new test test_concurrent_saves_never_mix_markdown_with_foreign_metadata deterministically drives exactly your interleaving (writer B waits for A to land its markdown; A stalls between its own two writes):

Code	Result
with _RESULT_WRITE_LOCK	106 passed
lock removed	1 failed, 105 passed — only the pairing test

A second test (test_pair_write_still_produces_both_files) guards that serialising the pair did not change the single-writer result; it passes under both variants, as a guard should.

Thank you — this is the second correctness bug review has caught in my to_thread refactors here (after the lost-serialisation race in #1194). The pattern is consistent: moving work off the loop removes the loop's implicit serialisation, and I need to ask what was serialising a shared resource before every such change.

[groupthinking]

Independent review of 4d8d250e5 — verified against the 5 challenges

Re-ran the branch locally (PYTHONPATH=src): 104 passed in 3.08s, matching the claim. I attacked each numbered point rather than restating the PR body.

1. Atomicity / temp-name collision — holds per file; one honest gap on the pair.

• Temp-name collision is genuinely prevented: to_thread runs each _write_result_files on a distinct pool thread, and a thread executes only one write at a time, so two concurrent saves always have distinct threading.get_ident() → distinct .{pid}.{tid}.tmp names. No collision even for the same video_id/timestamp.
• Each individual file is atomic (os.replace), so no reader ever observes a partial .md or .json. Solid.
• The gap: the .md and the .json are two independent os.replaces, not one atomic unit. Two saves for the same video_id that land in the same one-second timestamp bucket and truly overlap can interleave as A.md → B.md → B.json → A.json, leaving markdown from generation B beside metadata from generation A — each file internally complete, but the pair mismatched. This is newly possible because the previous on-loop version was implicitly serialized by the event loop; moving to to_thread removes that serialization (the same reasoning that forced the atomic-write change here). It sits inside the stated non-goal ("does NOT make concurrent saves parallel") and is low-severity (needs same video, same second, real overlap), so not a blocker — but it's the one property this change regresses versus the serialized original. Worth a one-line comment in _save_enhanced_result naming it as accepted, or bumping timestamp granularity if you want to close it.

2. Cancellation — claim verified. The surrounding handler is except Exception as e (line 803). asyncio.CancelledError is a BaseException, not Exception, so a cancelled await asyncio.to_thread(...) is not swallowed into a "" return — it propagates. The worker thread finishes its write (a completed, atomic save the caller no longer awaits — benign), and no instance resource is held across the await, so there is no finally: close() hazard like #1190.

3. except BaseException + suppress(OSError) — right width. Catching BaseException in the cleanup unlinks the temp file even on KeyboardInterrupt/CancelledError mid-write, and re-raising preserves the original signal. unlink can only raise OSError-family, so suppress(OSError) is exactly wide enough to stop a cleanup failure from masking the real error, and no wider. No change needed.

4. Test determinism — not flaky. Ran the three timing-sensitive tests (..._partially_written_file, ..._does_not_stall_the_event_loop, ..._json_serialisation_also_runs_off_the_loop) 25× in a loop: 0 failures. The partial-file assertion is not timing-dependent — os.replace guarantees every observation is a complete generation regardless of scheduling; the only timing-sensitive part (reader sampled ≥ once) is asserted separately with wide headroom (sleep(0) reader vs. 12 × to_thread writer). The heartbeat tests carry ~20× margin (0.10s block vs. 0.005s ticks). CI-safe.

5. Residual on-loop work — negligible. datetime.now().strftime and the Path joins are O(1) microsecond-scale string ops; they don't scale with payload size the way json.dumps + the writes do. Leaving them on the loop is correct.

Net: no blocking findings. Core CI is green (test, lint-python, build, bandit, guards). The red checks are the repo-wide governance/scan gates (truth-gate, gitleaks working-tree, trivy, Security Scan, validate, dependency-review, label, Agent completion enforcement) and a canceled Vercel deploy — none implicate this diff. The only actionable item is the optional cross-file pair-atomicity note in (1).

(Review posted from the author account, so this registers as a comment rather than a formal approval.)

---

Generated by Claude Code

[linear-code]

GRV-232

[github-actions]

Agent Completion Truth Gate: NOT_APPLICABLE

Evidence agrees.

Machine-readable verdict

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

Workflow evidence