Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRemoved per-project state cleanup from inside the usort comparator in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Change is localized to a single method, consisting of a straightforward move of cleanup logic into a Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a memory leak in the stats usage worker by relocating the unset() call that clears processed project data. The memory leak occurred because projects were being unset too early in the sorting comparison function, preventing proper cleanup after database operations completed.
Key Changes:
- Moved the
unset($this->projects[$sequence])statement from inside the sorting comparison function to after the database upsert operation
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Workers/StatsUsage.php (1)
487-549: Apply the same memory leak fix to$this->statDocuments.The TODO comment on line 544 highlights a similar memory leak issue. The
$this->statDocumentsarray accumulates stat documents but is never cleared after writing to logs DB, causing the same memory leak pattern that this PR fixes for$this->projects.Apply this diff to clear the array after writing:
$dbForLogs->upsertDocumentsWithIncrease( 'stats', 'value', $this->statDocuments ); Console::success('Usage logs pushed to Logs DB'); - /** - * todo: Do we need to unset $this->statDocuments? - */ - + $this->statDocuments = []; } catch (Throwable $th) { Console::error($th->getMessage()); + } finally { + $this->statDocuments = []; }
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/StatsUsage.php (1)
456-456: Redundant unset - already handled by finally block.This unset is redundant since the finally block (line 461) always executes and performs the same cleanup. Remove this line to simplify the code.
Apply this diff to remove the redundant line:
$dbForProject->upsertDocumentsWithIncrease('stats', 'value', $projectStats['stats']); - unset($this->projects[$sequence]); Console::success('Batch successfully written to DB');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/StatsUsage.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
src/Appwrite/Platform/Workers/StatsUsage.php (1)
460-462: Excellent addition - ensures cleanup on exception paths.The finally block guarantees that per-project state is cleared regardless of success or failure, directly addressing the memory leak mentioned in the PR title.
✨ Benchmark results
⚡ Benchmark Comparison
|
| }); | ||
|
|
||
| $dbForProject->upsertDocumentsWithIncrease('stats', 'value', $projectStats['stats']); | ||
| unset($this->projects[$sequence]); |
There was a problem hiding this comment.
but the unset is already done in the finally block below 🧐
f427c91 to
18371eb
Compare
No description provided.