Skip to content

fix(admin): bound request log export memory - #5089

Merged
chrarnoldus merged 5 commits into
mainfrom
fix/api-request-log-zip-finalization
Aug 6, 2026
Merged

fix(admin): bound request log export memory#5089
chrarnoldus merged 5 commits into
mainfrom
fix/api-request-log-zip-finalization

Conversation

@chrarnoldus

@chrarnoldus chrarnoldus commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Root Cause

The captured failed download confirms an out-of-memory termination. The 22.8 MB partial ZIP contains 818 local entry headers but only 817 completed data descriptors and no central directory or end-of-central-directory record. Its completed entries represent about 436 MB of uncompressed request-log data.

The previous readableLength check only observed Archiver's output buffer. It did not observe Archiver's internal input queue, so database pages accumulated faster than compression could process them.

Summary

  • wait until every entry in a database page has been processed and emitted before fetching the next page, bounding Archiver's internal queue
  • reduce each page from 100 to 25 rows; against the captured data this lowers the largest observed raw page from about 160 MB to about 48 MB before formatting overhead
  • add a database-backed regression test that fills the downstream stream queues, crosses a page boundary, and validates the completed ZIP

The branch includes the latest main. Timeout, compression-level, pagination-ceiling, and concurrent-insert changes are not part of this PR diff.

Verification

  • pnpm --filter web exec jest --runInBand src/app/admin/api/api-request-log/download/route.test.ts
  • pnpm --filter web typecheck
  • pnpm --filter web lint
  • pnpm format
  • git diff --check origin/main...HEAD

Visual Changes

None. Admin download endpoint only.

@chrarnoldus chrarnoldus self-assigned this Aug 6, 2026
@kilo-code-bot

kilo-code-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of commit ab7bab9 ("refactor(admin): scope request log fix to memory") plus the merge of latest main: the PR is now narrowed to the memory-bounding fix only — the pagination-ceiling, compression-level, and concurrent-insert test additions from earlier commits were reverted to main's existing behavior, and the timeout bump to maxDuration = 800 arrived via main's already-shipped #5091, so none of those appear in the net PR diff; the previously approved core changes (waitForEntries per-batch backpressure, BATCH_SIZE 25, and the database-backed regression test with 128KB rows crossing the page boundary) are unchanged, and the test cleanup is consistent with the route's ceiling removal, so no new issues were found.

Files Reviewed (2 files)
  • apps/web/src/app/admin/api/api-request-log/download/route.ts
  • apps/web/src/app/admin/api/api-request-log/download/route.test.ts
Previous Review Summaries (3 snapshots, latest commit 14b548e)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 14b548e)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of commit 14b548e ("fix(admin): reduce request log export memory"), which lowers the export page size from 100 to 25 rows and rebalances the regression test's per-row payload from 32KB to 128KB so total first-batch byte volume (and thus the backpressure invariant) is preserved; the batch-size-agnostic ceiling bounding, per-batch waitForEntries backpressure, and level-1 compression logic approved in prior reviews are unchanged, so no new issues were found.

Files Reviewed (2 files)
  • apps/web/src/app/admin/api/api-request-log/download/route.ts
  • apps/web/src/app/admin/api/api-request-log/download/route.test.ts

Previous review (commit a421e84)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of the follow-up commit a421e84 ("refactor(admin): separate request log timeout change"), which reverts the maxDuration bump to its base-branch value of 300 and drops the now-stale test assertion; the substantive ZIP finalization fix (ceiling-bounded pagination, per-batch Archiver backpressure via entry counting, level-1 compression) and its database-backed regression test are unchanged since the previous review and remain sound, so no new issues were found.

Files Reviewed (2 files)
  • apps/web/src/app/admin/api/api-request-log/download/route.ts
  • apps/web/src/app/admin/api/api-request-log/download/route.test.ts

Previous review (commit fca109a)

Status: No Issues Found | Recommendation: Merge

Executive Summary

Reviewed the request-log ZIP finalization fix (ceiling-bounded pagination, per-batch Archiver backpressure via entry counting, level-1 compression, raised 800s budget) and its database-backed regression test; the stream-coordination logic is race-free, the pagination bound deterministically excludes concurrent inserts, and the test correctly verifies a complete, extractable ZIP across a backpressured page boundary.

Files Reviewed (2 files)
  • apps/web/src/app/admin/api/api-request-log/download/route.ts
  • apps/web/src/app/admin/api/api-request-log/download/route.test.ts

Reviewed by kimi-k3 · Input: 54.8K · Output: 12.6K · Cached: 350.2K

Review guidance: REVIEW.md from base branch main

@chrarnoldus chrarnoldus changed the title fix(admin): ensure request log ZIPs finalize fix(admin): bound request log export memory Aug 6, 2026
@chrarnoldus
chrarnoldus merged commit ab86f87 into main Aug 6, 2026
16 checks passed
@chrarnoldus
chrarnoldus deleted the fix/api-request-log-zip-finalization branch August 6, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants