Skip to content

Reduce data export memory usage#928

Merged
tudddorrr merged 1 commit intodevelopfrom
reduce-data-export-mem-usage
Apr 9, 2026
Merged

Reduce data export memory usage#928
tudddorrr merged 1 commit intodevelopfrom
reduce-data-export-mem-usage

Conversation

@tudddorrr
Copy link
Copy Markdown
Collaborator

No description provided.

@tudddorrr tudddorrr added the fix This pull request contains a fix label Apr 9, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.78%. Comparing base (24eab9c) to head (b0bccbc).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines Patch % Lines
src/lib/queues/data-exports/dataExportProcessor.ts 93.75% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #928      +/-   ##
===========================================
+ Coverage    97.63%   97.78%   +0.14%     
===========================================
  Files          406      406              
  Lines         6649     6634      -15     
  Branches       884      884              
===========================================
- Hits          6492     6487       -5     
+ Misses          73       64       -9     
+ Partials        84       83       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 9, 2026

PR Review: Reduce data export memory usage

This is a clean and well-motivated refactor. The core approach — pre-fetching prop keys with targeted queries and streaming entities in a single pass — is the right fix for memory-bound exports. The alias cache eviction is correctly placed after all events on a page have been yielded, the em.fork() per entity type prevents identity map cross-contamination, and the new tests cover both the new leaderboard-prop path and the re-fetch-after-eviction path.


1. Code quality and best practices

Minor — streamCSVToArchive silently depends on propKeys arriving pre-sorted

The function builds metaPropKeys / otherPropKeys by filtering without re-sorting:

const metaPropKeys = propKeys.filter((k) => k.startsWith('META_'))
const otherPropKeys = propKeys.filter((k) => !k.startsWith('META_'))

This produces deterministic column order only because all three callers use ORDER BY ... ASC in their queries. The parameter is declared as propKeys: string[] = [] with no indication of this expectation. A future caller passing an unsorted array would produce non-deterministic (but silently correct-looking) CSV headers. A doc comment on the parameter would make this contract explicit.


2. Potential bugs or issues

No issues found.


3. Performance considerations

No issues found.


4. Security concerns

No issues found.


5. Test coverage

Minor — Cache-eviction test accesses private method via prototype with unexpected this binding

const proto = Object.getPrototypeOf(exporter)
for await (const event of proto.streamEvents(dataExport, em, true)) {

Calling proto.streamEvents(...) sets this to DataExporter.prototype rather than the exporter instance. It works today because streamEvents never references this, but it is subtly fragile. The conventional alternative for accessing a private method in tests is a cast:

for await (const event of (exporter as any).streamEvents(dataExport, em, true)) {

This preserves the correct this binding and remains safe if streamEvents ever calls an instance method internally.


Overall this is a solid improvement — no blockers, just two minor nits worth noting before merge.

@tudddorrr tudddorrr force-pushed the reduce-data-export-mem-usage branch from 7035b47 to 9b1145d Compare April 9, 2026 07:56
@tudddorrr tudddorrr force-pushed the reduce-data-export-mem-usage branch from 9b1145d to b0bccbc Compare April 9, 2026 08:08
@tudddorrr tudddorrr merged commit f63f09d into develop Apr 9, 2026
9 of 11 checks passed
@tudddorrr tudddorrr deleted the reduce-data-export-mem-usage branch April 9, 2026 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix This pull request contains a fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant