perf: batch structured log writes into a single aiofiles call

[groupthinking]

Canonical issue

Closes #1181

Outcome

LoggingService._write_to_files() now issues one write() per output file
instead of one per log entry.

aiofiles is a thread-pool shim, not a native-async file layer. Every method on
its file objects is generated by _make_delegate_method:

async def method(self, *args, **kwargs):
    cb = functools.partial(getattr(self._file, attr_name), *args, **kwargs)
    return await self._loop.run_in_executor(self._executor, cb)

Each await f.write(...) is therefore a full executor round-trip -- submit,
context-switch to a worker thread, write, hop back to the loop. Cost scales with
the number of calls, not the number of bytes.

LoggingService defaults to 'buffer_size': 100 and flush_logs() drains the
whole buffer at once, so a single flush previously cost:

	before	after
structured_logs.jsonl	100 executor round-trips	1
error_logs.jsonl (all-ERROR batch)	100 executor round-trips	1

Flushes fire on a 30s timer and whenever the buffer fills, so this is the
steady-state path.

Scope

One function, src/youtube_extension/backend/services/logging_service.py:372.

structured_payload = ''.join(f'{log.json()}\n' for log in logs)
async with aiofiles.open(structured_path, 'a') as f:
    await f.write(structured_payload)

What this does not change: output bytes, file paths, 'a' append mode,
ERROR/CRITICAL routing to the separate file, or the surrounding try/except.
Only the number of write() calls changes.

Design notes

Why not add an empty-batch guard around the structured write?
The per-entry loop still opened structured_logs.jsonl for an empty batch,
which creates the file as a side effect. Guarding the open would change that
observable behaviour for a path that already costs nothing (''.join([]) is
'', and a single write('') is one no-op round-trip). test_empty_batch_does_not_raise
pins the safety of that path without changing it. The existing if error_logs:
guard on the error file is untouched.

Why ''.join(...) rather than writelines()?
aiofiles' writelines is delegated the same way, and CPython's writelines
loops internally -- so it is one executor round-trip either way, but ''.join
makes the single-call property explicit and directly assertable in a test.

Memory. The batch is bounded by buffer_size (default 100 entries), which is
already fully materialised in log_buffer before this function is called. Joining
it adds one transient string of the same order as the data already in memory.

Risk

Low. No public API change, no behaviour change beyond call count.
The failure mode of the old code (partial batch written, then an exception) is
strictly worse than the new one -- a single write() cannot interleave a
partial batch with a concurrent flush.

Verification

At head 60ae14e84d937235520a4470918a2e4658932ad9:

PYTHONPATH=src pytest tests/unit/test_logging_service.py \
                      tests/unit/test_logging_service_models.py \
                      tests/unit/test_misc_services.py
=> 223 passed

ruff check src/.../logging_service.py tests/unit/test_logging_service.py
=> 1 error (pre-existing F401 at logging_service.py:27, identical on main)

Non-vacuity — the new tests were run against the per-entry loop they replace
and confirmed to fail:

E  AssertionError: _write_to_files issued 25 write() calls for 25 log entries;
   each aiofiles write is a separate thread-pool round-trip, so the batch must
   be serialised into a single write

E  AssertionError: _write_to_files issued 24 write() calls for 12 ERROR entries;
   expected exactly one batched write per file

2 failed, 5 passed, 57 deselected

The 24 in the second case is the point of the change: 12 entries × 2 files.

Tests added (TestLoggingServiceWriteToFiles):

Test	Pins
test_batches_structured_logs_into_one_write	exactly 1 write() for 25 entries
test_batches_error_logs_into_one_write	exactly 2 write() for an all-ERROR batch
test_writes_every_entry_exactly_once	every entry lands, in order, parseable as JSON
test_appends_rather_than_truncating	3 + 2 flushes → 5 lines
test_error_logs_routed_to_separate_file	mixed batch → 5 structured / 3 error
test_no_error_content_written_when_no_error_logs	INFO-only batch writes no error rows
test_empty_batch_does_not_raise	empty flush is safe

Production evidence

logging_service has 11 importers, including request-path middleware:

src/youtube_extension/backend/middleware/error_handling_middleware.py:27
src/youtube_extension/backend/services/health_monitoring_service.py:25

Enclosing function confirmed genuinely async (line 372), so the awaits are real
event-loop yields rather than a run_in_executor worker where blocking would be
correct.

Agent handoff

Reviewed by @coderabbitai. Second in a series of bounded-IO performance fixes;
follows #1152 (bounded Redis tag writes) and #1170 (bounded Firestore cleanup).

[vercel]

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

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

[coderabbitai]

Warning

Review limit reached

@groupthinking, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6e4ba004-d38a-4052-beb1-455dfc78c84a

📥 Commits

Reviewing files that changed from the base of the PR and between ad7e2c1 and 60ae14e.

⛔ Files ignored due to path filters (1)

• tests/unit/test_logging_service.py is excluded by !tests/**

📒 Files selected for processing (1)

• src/youtube_extension/backend/services/logging_service.py

---

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 60ae14e.

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 things I'd like challenged:

1. Is the perf claim actually right? My argument is that aiofiles
write() is loop.run_in_executor(...) per call (see _make_delegate_method),
   so N writes = N executor round-trips regardless of byte count. If aiofiles
   buffers or coalesces somewhere I've missed, the win is smaller than claimed
   and I'd rather correct the PR body than overstate it.

2. The empty-batch open. I deliberately did not guard the structured-file
   open on if logs:, because the old loop also opened (and therefore created)
   the file for an empty batch. Is preserving that side effect the right call, or
   is creating an empty structured_logs.jsonl itself the bug?

3. Memory. ''.join(...) materialises the batch. Bounded by buffer_size
   (default 100) which is already in log_buffer, so I judged this a non-issue.
   Is there a config path that can make this batch unbounded?

4. Prior rounds on perf: issue Redis tag-set writes concurrently on cache set #1152/perf: delete expired Firestore states concurrently under a bound #1170 flagged unbounded task allocation under
   gather. There's no fan-out here at all -- one sequential write per file --
   so I believe that class of finding doesn't apply. Please confirm.

[github-actions]

Agent Completion Truth Gate: NOT_APPLICABLE

Evidence agrees.

Machine-readable verdict

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

Workflow evidence

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

service='src/youtube_extension/backend/services/logging_service.py'
tests='tests/unit/test_logging_service.py'

printf '%s\n' '--- logging service outline ---'
ast-grep outline "$service" --items all --view expanded || true

printf '%s\n' '--- _write_to_files and surrounding code ---'
rg -n -C 18 'async def _write_to_files\b|def _write_to_files\b|buffer_size|log_buffer|flush_logs|asyncio\.gather|create_task' "$service" || true

printf '%s\n' '--- configuration and instantiation references ---'
rg -n -C 4 '\bLoggingService\s*\(|["'\'']buffer_size["'\'']|logging.*buffer|buffer.*logging' src tests -g '*.py' || true

printf '%s\n' '--- write-path tests ---'
if [ -f "$tests" ]; then
  ast-grep outline "$tests" --items all --view expanded || true
  rg -n -C 8 'WriteToFiles|batches_|empty_batch|appends_|error_logs_routed|write\(' "$tests" || true
fi

printf '%s\n' '--- aiofiles dependency metadata ---'
rg -n -C 3 'aiofiles' pyproject.toml requirements*.txt setup.cfg setup.py 2>/dev/null || true

Length of output: 20498

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- PR diff for logging-service files ---'
git diff --find-renames --unified=80 'HEAD^' 'HEAD' -- \
  src/youtube_extension/backend/services/logging_service.py || true

printf '%s\n' '--- locate logging service tests ---'
fd -HI -t f '.*logging.*service.*\.py$|test_.*logging.*\.py$' . \
  -E .git -E node_modules -E dist -E build | sort

printf '%s\n' '--- configuration injection and direct construction ---'
rg -n -C 8 '\bLoggingService\s*\(' . \
  -g '*.py' -g '!**/.git/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || true

printf '%s\n' '--- test class and direct _write_to_files calls anywhere in checkout ---'
rg -n -C 6 'TestLoggingServiceWriteToFiles|_write_to_files\s*\(|buffer_size.*:' . \
  -g '*.py' -g '!**/.git/**' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || true

printf '%s\n' '--- aiofiles version resolution inputs ---'
fd -HI -t f '(^|.*\/)(uv\.lock|poetry\.lock|pdm\.lock|requirements.*\.txt|pyproject\.toml)$' . \
  -E .git -E node_modules -E dist -E build -x sh -c 'echo "--- $1"; rg -n -C 2 "aiofiles" "$1" || true' sh {}

Length of output: 18194

---

🤖 web_search executed:

aiofiles 23.2.1 source _make_delegate_method write run_in_executor GitHub

Length of output: 5090

---

@groupthinking I found no fan-out issue in LoggingService._write_to_files().

1. Performance claim: correct with one wording correction.
   aiofiles documents write() as an executor-delegated coroutine. Its wrapper awaits loop.run_in_executor(...). Therefore, each awaited f.write(...) creates a separate executor submission and event-loop suspension. aiofiles does not coalesce separate write() coroutine calls. aiofiles documentation

   The PR should not say total cost scales only with call count. Serialization, Python text buffering, kernel I/O, and payload size still scale with bytes. A precise statement is:

   Each write() adds fixed executor scheduling and event-loop coordination overhead. Batching removes that per-entry overhead. It does not guarantee one kernel write(2) call or make byte-processing cost constant.

2. Empty batch: preserve the open, but skip write('').
   Preserving creation of structured_logs.jsonl is reasonable because the old implementation opened the file for an empty batch.

   However, the old empty loop made zero write() calls. The new code makes one empty write. This adds an executor round-trip and changes the method-call behavior without preserving any additional file-system behavior.

   Keep the unconditional aiofiles.open(...), but conditionally write the payload:

   structured_payload = ''.join(f'{log.json()}\n' for log in logs)
   async with aiofiles.open(structured_path, 'a') as f:
       if logs:
           await f.write(structured_payload)

   Add a test that confirms an empty batch creates the structured file and issues zero write() calls.

3. Memory: the default is bounded, but the configuration is not.
   Line 192 merges arbitrary caller configuration. There is no type check or upper limit for buffer_size. A caller can construct LoggingService({'buffer_size': 10_000_000}). The batch and joined payload can then be very large.

   This does not block the focused performance change. It does invalidate a claim that the implementation has a universal bounded-memory guarantee. State that the default batch is 100 entries, or validate and cap buffer_size in a separate change.

4. No gather issue exists in this write path.
   Line 382 awaits the structured-file write. Lines 386-390 then build and await the error-file write. The operations are sequential. _write_to_files() creates no tasks and uses no asyncio.gather(). The prior unbounded task-allocation finding does not apply here.

5. The claimed tests are absent from this PR checkout.
   The PR diff contains only src/youtube_extension/backend/services/logging_service.py. I could not find tests/unit/test_logging_service.py or TestLoggingServiceWriteToFiles in the checkout. Please include the stated write-count and output-integrity tests in the PR. They are necessary to protect the one-write-per-nonempty-file contract.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Copilot]

Pull request overview

Optimizes structured logging by batching each flush into one asynchronous write per output file.

Changes:

• Serializes structured and error log batches before writing.
• Adds regression tests for batching, ordering, append behavior, routing, and empty batches.

Reviewed changes

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

File	Description
src/youtube_extension/backend/services/logging_service.py	Replaces per-entry writes with batched writes.
tests/unit/test_logging_service.py	Tests write counts and preserved output behavior.

[groupthinking]

Automated triage note on the CodeRabbit review — two corrections worth flagging before this is read as blocked:

1. CodeRabbit finding #5 ("the claimed tests are absent from this PR checkout") is a false positive. The test file is present at head 60ae14e:

• tests/unit/test_logging_service.py → class TestLoggingServiceWriteToFiles with all 7 tests the PR body lists (test_batches_structured_logs_into_one_write, test_batches_error_logs_into_one_write, test_writes_every_entry_exactly_once, test_appends_rather_than_truncating, test_error_logs_routed_to_separate_file, test_no_error_content_written_when_no_error_logs, test_empty_batch_does_not_raise).

The cause is CodeRabbit's own path filter: its comment lists tests/unit/test_logging_service.py under "Files ignored due to path filters (1) — excluded by !tests/**", so it reviewed only the service file and then couldn't see the tests it was told existed. Copilot's reviewer, without that filter, correctly reports "2 out of 2 changed files" including the test file. No action needed on #5 — please disregard the "please include the stated tests" instruction.

2. The only failing check is a cancelled deploy, not a code failure. The commit status shows failure overall, but that's solely Vercel — "Canceled from the Vercel Dashboard". agent-completion/truth-gate ✅, CodeRabbit ✅, and Vercel Deployments ✅ are all green. This shouldn't be read as a test/lint regression.

The remaining CodeRabbit points are non-blocking and yours to weigh: #1 (soften the "cost scales only with call count" wording) and #3 (the bounded-memory claim holds for the default buffer_size: 100 but not for an arbitrarily large caller-supplied value) are doc-precision nits; #2 (guarding the empty-batch write('')) is a reasonable alternative to the deliberate choice you already documented, not a defect; #4 (no gather fan-out) is confirmed a non-issue.

Terminal state: awaiting merge approval — mergeable once the Vercel status is re-run/dismissed. Not auto-merging: no automerge label, base is protected main, and this is an unattended run.

---

Generated by Claude Code

[groupthinking]

Review — batched log writes

Code: correct and behaviour-preserving. The per-entry await f.write(...) loop is replaced by a single joined payload and one write() per file. f'{log.json()}\n' is byte-identical to the previous log.json() + '\n'; append mode, file paths, ERROR/CRITICAL routing, the if error_logs: guard, and the surrounding try/except are all untouched. Empty-batch behaviour (structured file still opened/created) is preserved and pinned by test_empty_batch_does_not_raise. Since aiofiles delegates every write() to the thread-pool executor, collapsing ~100 round-trips per flush to 1 is a real steady-state win with no downside — and a single write() can no longer interleave a partial batch with a concurrent flush, so the failure mode is strictly better than before.

Tests: comprehensive and non-vacuous. The 7 new tests cover call-count, per-file routing, append semantics, ordering/JSON-parseability, and the empty batch. The PR demonstrates they fail against the pre-batch loop (25→1, 24→2 write assertions), so they genuinely pin the new behaviour rather than restating it.

CI — two red checks, neither introduced by this PR:

• gitleaks (working tree) ❌ — false positive on uv.lock:5129: a package sha256: hash matched the square-access-token rule (entropy 3.88). uv.lock is not in this PR's diff (only logging_service.py + test_logging_service.py), and the scan runs --no-git over the whole tree, so this also fails on main. The fix belongs in the .gitleaks.toml allowlist, not in this PR.
• Agent completion enforcement ❌ — the known repo-wide missing_trusted_publication gate; same failure on fix(deps): raise transitive override floors to patched versions #1098 / fix(deps): clear @hono/node-server advisory via MCP SDK 1.30.0 #1103 / fix: honor declared picomatch override in lockfile #1108, which merged regardless. The agent-completion/truth-gate variant passed here.
• Everything else green/skipped: CodeQL, bandit, python-safety, npm-audit, validate, PR Governance, copilot reviewer (trivy was still running at scan time).

Verdict: LGTM on the code — well-scoped, low-risk, well-tested. I can't register a GitHub approval (own-PR restriction), and I am not auto-merging: main is protected, this is an unattended scheduled run, and mergeable_state is unstable because of the two red checks above. Both are pre-existing / false-positive, so the merge call is yours — either an admin-merge/waiver, or add an allowlist entry for the uv.lock hash so gitleaks goes green first.

---

Generated by Claude Code

[groupthinking]

Remediation review — verdict: sound, one config blocker to clear

Automated pass over this PR (diff, CI checks, CodeRabbit thread). The change itself is correct and well-tested; the only thing standing between it and merge is a pre-existing CI false positive unrelated to your diff.

Change assessment ✅

The batching is right, and CodeRabbit's confirmation of the perf reasoning matches the aiofiles internals — each write() is a distinct run_in_executor round-trip, so collapsing the per-entry loop into one write() per file removes real per-entry overhead. No fan-out / gather concern exists in this path (both writes are sequential and awaited).

CI: gitleaks (working tree) red is a false positive, not your change

The failing scan reports one finding:

RuleID:  square-access-token
File:    uv.lock   Line: 5129
Finding: ...gz", hash = "sha256:REDACTED, size = 401824, ...

That's a sha256: package hash in the lockfile, not a credential (entropy 3.88 tripped the token rule). This PR doesn't touch uv.lock, so the same scan fails on main. It's a repo-config gap, not a defect here — clear it with a .gitleaks.toml allowlist for lockfile hashes, e.g.:

[allowlist]
paths = ['''uv\.lock''']
# or, narrower — allow sha256 hash literals only:
regexes = ['''sha256:[a-f0-9]{64}''']

The other reds are noise: agent-completion/truth-gate passed (NOT_APPLICABLE), and Trivy is neutral. mergeable_state is unstable, not blocked, so none of these are required checks.

CodeRabbit findings — triage

• "Tests absent from the checkout" (finding Add LiteRT-LM MCP Server #5): false alarm. CodeRabbit's own run config lists tests/unit/test_logging_service.py as "ignored due to path filters (!tests/**)" — it never saw the file. The 7 new TestLoggingServiceWriteToFiles tests are present in this PR's diff. No action needed.
• Empty-batch write('') (finding Implement Nightly Audit & Ruthless Remediation Agent #2): valid but optional. Guarding if logs: around the structured write avoids one no-op executor round-trip on empty flushes while preserving the file-creation side effect. Micro-optimization; not a correctness issue.
• Perf-claim wording / buffer_size bound (findings chore(deps): bump the npm_and_yarn group across 10 directories with 7 updates #1, Implement Nightly Audit & Ruthless Remediation Agent #3): doc-only / future work. Both fair; neither blocks this focused change.

Terminal state: HALTED(awaiting_merge_approval)

Nothing to fix in the diff. Merge is gated only on: (1) a human clearing the protected-main publish gate — no automerge label is set — and (2) the .gitleaks.toml allowlist above if you want that check green (it's non-required today). This run does not auto-merge to protected main and does not push to this branch.

---

Generated by Claude Code

[linear-code]

GRV-219