Check audits logs #10414
Check audits logs #10414
Conversation
📝 WalkthroughWalkthroughReworked audit batch processing to handle each project individually: the foreach now wraps per-project processing in its own try/catch/finally, derives a per-project Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.17)Note: Using configuration file /phpstan.neon. If the excluded path can sometimes exist, append (?) parameters: ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Appwrite/Platform/Workers/Audits.php (2)
43-44: Remove constructor noise (wrong severity).This logs on every worker start at error level. Drop it or guard behind a debug flag.
Apply this diff:
- Console::error('__construct Audits');
76-78: Avoid error-level for routine context; gate or downgrade.These are dev traces. Either remove or show only outside production.
Apply this diff:
- Console::error('project '.$project->getSequence()); - Console::error('lastTriggeredTime '.$this->lastTriggeredTime); + if (System::getEnv('_APP_ENV', 'production') !== 'production') { + Console::log('project ' . $project->getSequence()); + Console::log('lastTriggeredTime ' . $this->lastTriggeredTime); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Audits.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Audits.php (1)
app/realtime.php (1)
getProjectDB(76-122)
⏰ 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/Audits.php (1)
152-154: Good: snapshot then clear buffer.This prevents in-loop mutation and simplifies processing semantics.
✨ Benchmark results
⚡ Benchmark Comparison
|
| $logs = $this->logs; | ||
| $this->logs = []; | ||
|
|
||
| foreach ($logs as $projectLogs) { | ||
| try { | ||
| $projectDocument = $projectLogs['project']; | ||
| Console::log('Processing Project "' . $projectDocument->getSequence() . '" batch with ' . count($projectLogs['logs']) . ' events'); | ||
| $dbForProject = $getProjectDB($projectDocument); | ||
| $audit = new Audit($dbForProject); | ||
|
|
||
| $audit->logBatch($projectLogs['logs']); | ||
| Console::success('Audit logs processed successfully'); | ||
|
|
||
| unset($this->logs[$sequence]); | ||
| } catch (Throwable $e) { | ||
| Console::error('Error processing audit logs for Project "' . $projectDocument->getSequence() . '": ' . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Instead of resetting all, let's unset at the end of the loop as before, but also unset in the catch
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Appwrite/Platform/Workers/Audits.php (1)
134-153: Stop dropping logs on errors; unset only on success and return NoCommit if any project fails.The finally block unconditionally unsets buffered logs and the method always returns Commit. On any per-project exception, this will drop that project's logs and ack the message — a data-loss scenario. Handle failures per-project, keep failed batches in memory, and return NoCommit if any failure occurred.
Apply this diff:
- foreach ($this->logs as $sequence => $projectLogs) { - try { - Console::log('Processing Project "' . $sequence . '" batch with ' . count($projectLogs['logs']) . ' events'); - - $projectDocument = $projectLogs['project']; - $dbForProject = $getProjectDB($projectDocument); - $audit = new Audit($dbForProject); - $audit->logBatch($projectLogs['logs']); - - Console::success('Audit logs processed successfully'); - } catch (Throwable $e) { - Console::error('Error processing audit logs for Project "' . $sequence . '": ' . $e->getMessage()); - } finally { - unset($this->logs[$sequence]); - } - } + $failed = []; + foreach ($this->logs as $sequence => $projectLogs) { + try { + Console::log('Processing Project "' . $sequence . '" batch with ' . count($projectLogs['logs']) . ' events'); + /** @var Document $projectDocument */ + $projectDocument = $projectLogs['project']; + $dbForProject = $getProjectDB($projectDocument); + $audit = new Audit($dbForProject); + $audit->logBatch($projectLogs['logs']); + Console::success('Audit logs processed successfully'); + // Remove only successfully processed batches + unset($this->logs[$sequence]); + } catch (Throwable $e) { + Console::error('Error processing audit logs for Project "' . $sequence . '": ' . $e->getMessage()); + // Keep failed batches in buffer for retry + $failed[$sequence] = true; + } + } + if (!empty($failed)) { + return new NoCommit(); + }
🧹 Nitpick comments (1)
src/Appwrite/Platform/Workers/Audits.php (1)
136-146: Confirm idempotency to avoid duplicates on retry.With at-least-once delivery (NoCommit), successful batches are removed but the current message will be redelivered. Ensure
Audit::logBatchis idempotent or events have a dedupe key to prevent duplicate inserts on retries.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Audits.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Audits.php (1)
app/realtime.php (1)
getProjectDB(76-122)
⏰ 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). (6)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Linter
- GitHub Check: scan
- GitHub Check: CodeQL
2 participants
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist