fix(api): reduce refly-api RSS from image path and stream snapshots - #2290
Conversation
Cap concurrent Sharp/base64 work, set MALLOC_ARENA_MAX=2, and debounce ResultAggregator Redis snapshots to limit glibc arena fragmentation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR limits Sharp/libvips memory usage and concurrency, and changes ChangesImage memory controls
Result persistence lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ResultAggregator
participant StepService
participant Redis
ResultAggregator->>ResultAggregator: Schedule debounced persistence
ResultAggregator->>StepService: Persist latest steps
StepService->>Redis: Write cache entry
ResultAggregator->>StepService: Clear cache after in-flight write completes
StepService->>Redis: Delete cache entry
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Deploying refly-branch-test with
|
| Latest commit: |
c46aa01
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://93b571d0.refly-branch-test.pages.dev |
| Branch Preview URL: | https://fix-api-rss-image-allocator.refly-branch-test.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eea7c9c5e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/api/src/modules/drive/drive.service.ts (1)
69-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the concurrency bounds and default.
The minimum, maximum, and fallback values are deployment-policy constants; naming them prevents the clamp/default contract from drifting.
Proposed fix
+const MIN_IMAGE_PROCESS_CONCURRENCY = 1; +const MAX_IMAGE_PROCESS_CONCURRENCY = 4; +const DEFAULT_IMAGE_PROCESS_CONCURRENCY = 2; + const rawImageConcurrency = Number.parseInt(process.env.IMAGE_PROCESS_CONCURRENCY ?? '', 10); const imageProcessConcurrency = Number.isFinite(rawImageConcurrency) - ? Math.min(4, Math.max(1, rawImageConcurrency)) - : 2; + ? Math.min(MAX_IMAGE_PROCESS_CONCURRENCY, Math.max(MIN_IMAGE_PROCESS_CONCURRENCY, rawImageConcurrency)) + : DEFAULT_IMAGE_PROCESS_CONCURRENCY;As per coding guidelines, “Avoid magic numbers and strings - use named constants in TypeScript/JavaScript.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/drive/drive.service.ts` around lines 69 - 73, Name the image processing concurrency policy values as constants for the minimum, maximum, and fallback/default, then use those constants in the clamp and fallback within the imageProcessConcurrency initialization. Keep the existing bounds and default behavior unchanged and apply the change around rawImageConcurrency and imageProcessLimit.Source: Coding guidelines
apps/api/src/utils/result.ts (1)
18-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
PERSIST_DEBOUNCE_MSfor test reuse.The constant isn't exported, so
result.spec.tshardcodes the literal200in multiple places to match it. If this value changes, tests will silently desync from the implementation.As per coding guidelines, "Avoid magic numbers and strings - use named constants in TypeScript/JavaScript."
♻️ Proposed fix
-const PERSIST_DEBOUNCE_MS = 200; +export const PERSIST_DEBOUNCE_MS = 200;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/utils/result.ts` around lines 18 - 19, Export the PERSIST_DEBOUNCE_MS constant from result.ts so result.spec.ts can import and reuse it instead of hardcoding 200, keeping the implementation and tests synchronized.Source: Coding guidelines
apps/api/src/utils/result.spec.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
jest.advanceTimersByTimeAsync()overadvanceTimersByTime()+ manualPromise.resolve()flushing.The implementation's debounce chain is several awaits deep (timer →
flushPersistSteps→runPersistLoop→persistStepsNow→setCache). A fixed count ofawait Promise.resolve()calls afterjest.advanceTimersByTime()is a known source of flaky tests once promise-chain depth changes, since syncadvanceTimersByTimedoesn't flush microtasks between timer executions. Jest 29 (already a project dependency) providesadvanceTimersByTimeAsync()specifically to flush microtasks between timer runs, making these tests more robust against implementation changes to the chain depth.♻️ Example
-jest.advanceTimersByTime(200); -await Promise.resolve(); -await Promise.resolve(); +await jest.advanceTimersByTimeAsync(200);Also applies to: 52-58, 80-99, 104-115, 119-124, 140-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/utils/result.spec.ts` at line 19, Update the timer-based tests in result.spec.ts to use jest.advanceTimersByTimeAsync() instead of synchronous timer advancement followed by manual Promise.resolve() flushing. Apply this to the cases around the existing timer setup and referenced test ranges, removing fixed microtask flushes while preserving each test’s timing and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/utils/result.ts`:
- Around line 210-215: Update addUsageItem() to honor the aborted lifecycle
established by addSkillEvent() and handleStreamContent(): return immediately
when this.aborted is true, preventing post-abort usage updates from scheduling
persistence. If usage items must still be retained after abort, preserve the
data update but guard schedulePersistSteps() so it cannot rearm a Redis-write
timer.
- Around line 162-166: Update ResultAggregator.abort() to call
flushPersistSteps() after cancelling persistTimer, ensuring mutations queued
during the debounce window are persisted before abort completes.
---
Nitpick comments:
In `@apps/api/src/modules/drive/drive.service.ts`:
- Around line 69-73: Name the image processing concurrency policy values as
constants for the minimum, maximum, and fallback/default, then use those
constants in the clamp and fallback within the imageProcessConcurrency
initialization. Keep the existing bounds and default behavior unchanged and
apply the change around rawImageConcurrency and imageProcessLimit.
In `@apps/api/src/utils/result.spec.ts`:
- Line 19: Update the timer-based tests in result.spec.ts to use
jest.advanceTimersByTimeAsync() instead of synchronous timer advancement
followed by manual Promise.resolve() flushing. Apply this to the cases around
the existing timer setup and referenced test ranges, removing fixed microtask
flushes while preserving each test’s timing and assertions.
In `@apps/api/src/utils/result.ts`:
- Around line 18-19: Export the PERSIST_DEBOUNCE_MS constant from result.ts so
result.spec.ts can import and reuse it instead of hardcoding 200, keeping the
implementation and tests synchronized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1692d7d7-0b89-4eb1-9c2e-8c2fbb63e852
📒 Files selected for processing (5)
apps/api/Dockerfileapps/api/src/modules/drive/drive.service.tsapps/api/src/utils/result.spec.tsapps/api/src/utils/result.tsdeploy/helm/refly-api/values.yaml
Re-debounce after in-flight writes instead of immediate loops, flush on abort, guard addUsageItem when aborted, and tighten related tests/constants.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd10eb27c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/utils/result.ts (1)
126-157: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winDon’t redirty
persistDirtyinside concurrent forced flushes.
abort()startsflushPersistSteps()fire-and-forget. If a caller immediately awaitsgetSteps(), both flushers enter the same in-flight write; the first caller’s post-write loop then seespersistDirtyset by the second flusher and issues a duplicate Redis write. The forcedpersistDirty = trueis not needed for theabort→getStepsfinal-persist path becauseabort()already leaves the write in-flight.♻️ Proposed fix
this.clearPersistTimer(); - // Ensure at least one attempt when explicitly flushed (e.g. getSteps). - this.persistDirty = true; while (this.persistDirty && !this.persistCleared) {Add a regression test for
abort()followed immediately bygetSteps()that assertssetCache()is called only once with final content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/utils/result.ts` around lines 126 - 157, Remove the unconditional persistDirty = true assignment from flushPersistSteps, while preserving the existing in-flight coordination and drain loop. Add a regression test covering abort() followed immediately by getSteps(), asserting setCache() is called exactly once with the final content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/utils/result.ts`:
- Around line 126-157: Remove the unconditional persistDirty = true assignment
from flushPersistSteps, while preserving the existing in-flight coordination and
drain loop. Add a regression test covering abort() followed immediately by
getSteps(), asserting setCache() is called exactly once with the final content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aff6972-58e1-4b5a-9579-f58bbf40ae38
📒 Files selected for processing (3)
apps/api/src/modules/drive/drive.service.tsapps/api/src/utils/result.spec.tsapps/api/src/utils/result.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/modules/drive/drive.service.ts
Read IMAGE_PROCESS_CONCURRENCY on first use / onModuleInit so ConfigModule .env values are honored instead of import-time defaults.
2 participants
Summary
IMAGE_PROCESS_CONCURRENCYdefault 2, clamp 1–4), setsharp.concurrency(1), and remove a redundantBuffer.fromcopy on the vision base64 path — main RSS ratchet driver is glibc arena fragmentation under concurrent libvips + multi-buffer Base64, not a JS retained-object leak.MALLOC_ARENA_MAX=2in the API Dockerfile and helm values (Sharp/glibc allocator guidance).ResultAggregatorRedis step snapshots (200ms coalesce + single-flight, terminalclearCache) to cut secondary stream allocation churn; unit tests cover debounce and clear races.Deploy notes
MALLOC_ARENA_MAXor set to4; optionally tuneIMAGE_PROCESS_CONCURRENCYin 1–4.Test plan
pnpm exec jest src/utils/result.spec.ts(apps/api) — 3/3 passrefly-apipod with new image +MALLOC_ARENA_MAX=2Summary by CodeRabbit