fix(logs): automated log retention and API page_size cap#1621
Conversation
- Rulebook process logs grow without bound, eventually causing API timeouts and UX failures when the table becomes too large to query - Add hourly scheduled task that purges logs older than LOG_RETENTION_DAYS (default 30, configurable via EDA_LOG_RETENTION_DAYS) - Add per-instance safety valve in DBLogger.flush() that trims oldest rows when MAX_LOG_LINES_PER_INSTANCE (default 500000) is exceeded - Both settings can be disabled by setting their value to 0 - Refactor purge_log_records management command to share common utility (delete_log_util.py) with the scheduled task - Command now defaults to LOG_RETENTION_DAYS when --date is omitted and supports an optional --audit-trail flag Resolves: AAP-77938 Assisted by: Claude Opus 4.6
- The logs endpoint accepted unbounded page_size values, allowing a
single request to load millions of rows into memory
- The EDA UI requests page_size={total_count} to fetch all logs at
once, which causes API timeouts and OOM on large log tables
- Add LogPagination class with max_page_size=1000, scoped to the
/activation-instances/{id}/logs/ endpoint only
- Other endpoints (credentials, audit rules) are unaffected
Resolves: AAP-77938
Assisted by: Claude Opus 4.6
📝 WalkthroughWalkthroughAdds configurable rulebook process log retention, hourly cleanup, audit-trail utilities, per-instance log caps, bounded activation-log pagination, updated purge command behavior, documentation, and integration tests. ChangesLog management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant purge_old_log_records
participant advisory_lock
participant delete_logs_older_than
participant RulebookProcessLog
Dispatcher->>purge_old_log_records: invoke every 3600 seconds
purge_old_log_records->>advisory_lock: acquire non-blocking lock
purge_old_log_records->>delete_logs_older_than: pass retention cutoff
delete_logs_older_than->>RulebookProcessLog: delete older records
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aap_eda/services/activation/db_log_handler.py (1)
82-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake insertion and trimming atomic.
bulk_create()commits before_enforce_max_log_lines()runs. If trimming fails, the inserted rows remain while the buffer is not cleared; retryingflush()can duplicate every buffered line. Wrap both operations in onetransaction.atomic()block.As per path instructions, focus on major issues impacting performance, readability, maintainability and security.
🤖 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 `@src/aap_eda/services/activation/db_log_handler.py` around lines 82 - 86, Make the insertion and trimming operations in the activation log flush path atomic by wrapping both bulk_create and _enforce_max_log_lines in a single transaction.atomic() block. Preserve the existing buffer handling and ensure a trimming failure rolls back the inserted rows so retrying flush() cannot duplicate them.Source: Path instructions
🧹 Nitpick comments (1)
src/aap_eda/api/views/activation.py (1)
819-860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet
pagination_classdeclaratively on the action.Setting
self.pagination_class = LogPaginationdynamically inside the method bypassesdrf-spectacular's automatic schema generation for pagination. Because the schema generator cannot see the dynamic assignment, thepage_sizeparameter was manually added to@extend_schema.Passing
pagination_class=LogPaginationdirectly to@actionapplies it declaratively. This allows DRF to handle the class appropriately anddrf-spectacularto automatically generate the correct pagination parameters (page_size,page, etc.), meaning you can safely remove the manualpage_sizeparameter from@extend_schema.♻️ Proposed refactor
`@extend_schema`( description=( "List Activation instance logs. " "Results are paginated with a maximum page_size of 1000." ), request=None, responses={ status.HTTP_200_OK: serializers.ActivationInstanceLogSerializer( many=True ) }, parameters=[ OpenApiParameter( name="id", type=int, location=OpenApiParameter.PATH, description="A unique integer value identifying this Activation Instance.", # noqa: E501 ), - OpenApiParameter( - name="page_size", - type=int, - location=OpenApiParameter.QUERY, - description="Number of results per page (default: 20, max: 1000).", # noqa: E501 - ), ], extensions={ "x-ai-description": ( "List logs for an activation instance by ID. " "Returns log records with timestamps and levels. " "Supports filtering and pagination. " "Maximum page_size is 1000." ) }, ) `@action`( detail=False, queryset=models.RulebookProcessLog.objects.order_by("id"), filterset_class=filters.ActivationInstanceLogFilter, rbac_action=Action.READ, url_path="(?P<id>[^/.]+)/logs", + pagination_class=LogPagination, ) def logs(self, request, id): - self.pagination_class = LogPagination instance_exists = (🤖 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 `@src/aap_eda/api/views/activation.py` around lines 819 - 860, Update the logs action to pass pagination_class=LogPagination declaratively in the `@action` decorator, and remove the dynamic self.pagination_class assignment from logs. Remove the manually defined page_size OpenApiParameter from the surrounding schema so drf-spectacular generates pagination parameters automatically.
🤖 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 `@src/aap_eda/core/utils/delete_log_util.py`:
- Around line 28-31: Add an index for RulebookProcessLog.log_timestamp through
the model and its migration, then update the hourly retention deletion around
cutoff_ts to remove matching records in bounded batches rather than issuing one
unbounded delete. Preserve the existing cutoff condition and deletion behavior
while limiting transaction size and lock duration.
- Around line 63-77: Update the audit log construction in the instances
comprehension so RulebookProcessLog.log_timestamp uses timezone.now() at
creation time instead of cutoff_ts; retain cutoff only in the purge message and
leave the bulk_create flow unchanged.
In `@src/aap_eda/services/activation/db_log_handler.py`:
- Around line 100-112: Update the log-retention logic around num_of_log_lines so
each flush avoids an exact full-table count; use a bounded existence/threshold
query to determine whether the cap is exceeded, then delete only the required
oldest RulebookProcessLog rows while preserving per-activation filtering and
ordering.
- Around line 95-104: Update _enforce_max_log_lines to treat
MAX_LOG_LINES_PER_INSTANCE values less than or equal to zero as disabled,
returning before calculating excess or deleting persisted logs. Preserve the
existing enforcement behavior for positive limits.
---
Outside diff comments:
In `@src/aap_eda/services/activation/db_log_handler.py`:
- Around line 82-86: Make the insertion and trimming operations in the
activation log flush path atomic by wrapping both bulk_create and
_enforce_max_log_lines in a single transaction.atomic() block. Preserve the
existing buffer handling and ensure a trimming failure rolls back the inserted
rows so retrying flush() cannot duplicate them.
---
Nitpick comments:
In `@src/aap_eda/api/views/activation.py`:
- Around line 819-860: Update the logs action to pass
pagination_class=LogPagination declaratively in the `@action` decorator, and
remove the dynamic self.pagination_class assignment from logs. Remove the
manually defined page_size OpenApiParameter from the surrounding schema so
drf-spectacular generates pagination parameters automatically.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a3ea8d28-481e-48a9-89c1-c35e68ee142e
📒 Files selected for processing (13)
docs/development.mdsrc/aap_eda/api/pagination.pysrc/aap_eda/api/views/activation.pysrc/aap_eda/core/management/commands/purge_log_records.pysrc/aap_eda/core/utils/delete_log_util.pysrc/aap_eda/services/activation/db_log_handler.pysrc/aap_eda/settings/core.pysrc/aap_eda/settings/defaults.pysrc/aap_eda/tasks/log_cleanup.pytests/integration/api/test_activation_instance.pytests/integration/commands/test_purge_log_records.pytests/integration/services/activation/test_db_log_handler.pytests/integration/tasks/test_log_cleanup.py
| cutoff_ts = int(cutoff.timestamp()) | ||
| deleted, _ = models.RulebookProcessLog.objects.filter( | ||
| log_timestamp__lt=cutoff_ts, | ||
| ).delete() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Index and batch the hourly retention deletion.
RulebookProcessLog.log_timestamp has no index in the supplied model, so this hourly filter scans the growing log table. The single unbounded DELETE can also create a large transaction and prolonged locks during initial cleanup. Add an indexed migration and delete in bounded batches.
As per path instructions, focus on major issues impacting performance, readability, maintainability and security.
🤖 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 `@src/aap_eda/core/utils/delete_log_util.py` around lines 28 - 31, Add an index
for RulebookProcessLog.log_timestamp through the model and its migration, then
update the hourly retention deletion around cutoff_ts to remove matching records
in bounded batches rather than issuing one unbounded delete. Preserve the
existing cutoff condition and deletion behavior while limiting transaction size
and lock duration.
Source: Path instructions
| audit_logs = [ | ||
| models.RulebookProcessLog( | ||
| log=( | ||
| f"All log records older than " | ||
| f"{cutoff.strftime('%Y-%m-%d')} " | ||
| f"were purged at {dt}." | ||
| ), | ||
| activation_instance_id=instance.id, | ||
| log_timestamp=cutoff_ts, | ||
| ) | ||
| for instance in instances | ||
| ] | ||
|
|
||
| if audit_logs: | ||
| models.RulebookProcessLog.objects.bulk_create(audit_logs) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Timestamp audit entries at creation time.
Line 71 assigns the retention cutoff to the audit row. Because the next hourly cutoff is newer, that audit entry is deleted on the next cleanup run. Store timezone.now() as log_timestamp while retaining cutoff only in the message.
Proposed fix
- cutoff_ts = int(cutoff.timestamp())
+ audit_ts = int(timezone.now().timestamp())
...
- log_timestamp=cutoff_ts,
+ log_timestamp=audit_ts,As per path instructions, focus on major issues impacting performance, readability, maintainability and security.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| audit_logs = [ | |
| models.RulebookProcessLog( | |
| log=( | |
| f"All log records older than " | |
| f"{cutoff.strftime('%Y-%m-%d')} " | |
| f"were purged at {dt}." | |
| ), | |
| activation_instance_id=instance.id, | |
| log_timestamp=cutoff_ts, | |
| ) | |
| for instance in instances | |
| ] | |
| if audit_logs: | |
| models.RulebookProcessLog.objects.bulk_create(audit_logs) | |
| audit_ts = int(timezone.now().timestamp()) | |
| audit_logs = [ | |
| models.RulebookProcessLog( | |
| log=( | |
| f"All log records older than " | |
| f"{cutoff.strftime('%Y-%m-%d')} " | |
| f"were purged at {dt}." | |
| ), | |
| activation_instance_id=instance.id, | |
| log_timestamp=audit_ts, | |
| ) | |
| for instance in instances | |
| ] | |
| if audit_logs: | |
| models.RulebookProcessLog.objects.bulk_create(audit_logs) |
🤖 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 `@src/aap_eda/core/utils/delete_log_util.py` around lines 63 - 77, Update the
audit log construction in the instances comprehension so
RulebookProcessLog.log_timestamp uses timezone.now() at creation time instead of
cutoff_ts; retain cutoff only in the purge message and leave the bulk_create
flow unchanged.
Source: Path instructions
| def _enforce_max_log_lines(self) -> None: | ||
| max_lines = settings.MAX_LOG_LINES_PER_INSTANCE | ||
| if not max_lines: | ||
| return | ||
|
|
||
| total = self.num_of_log_lines() | ||
| if total <= max_lines: | ||
| return | ||
|
|
||
| excess = total - max_lines |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent negative limits from deleting all instance logs.
A negative MAX_LOG_LINES_PER_INSTANCE is truthy, producing excess > total; the resulting slice deletes every persisted line. Reject negative configuration or treat all non-positive values as disabled.
Proposed guard
- if not max_lines:
+ if max_lines <= 0:
returnAs per path instructions, focus on major issues impacting performance, readability, maintainability and security.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _enforce_max_log_lines(self) -> None: | |
| max_lines = settings.MAX_LOG_LINES_PER_INSTANCE | |
| if not max_lines: | |
| return | |
| total = self.num_of_log_lines() | |
| if total <= max_lines: | |
| return | |
| excess = total - max_lines | |
| def _enforce_max_log_lines(self) -> None: | |
| max_lines = settings.MAX_LOG_LINES_PER_INSTANCE | |
| if max_lines <= 0: | |
| return | |
| total = self.num_of_log_lines() | |
| if total <= max_lines: | |
| return | |
| excess = total - max_lines |
🤖 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 `@src/aap_eda/services/activation/db_log_handler.py` around lines 95 - 104,
Update _enforce_max_log_lines to treat MAX_LOG_LINES_PER_INSTANCE values less
than or equal to zero as disabled, returning before calculating excess or
deleting persisted logs. Preserve the existing enforcement behavior for positive
limits.
Source: Path instructions
| total = self.num_of_log_lines() | ||
| if total <= max_lines: | ||
| return | ||
|
|
||
| excess = total - max_lines | ||
| oldest_ids = ( | ||
| models.RulebookProcessLog.objects.filter( | ||
| activation_instance_id=self.activation_instance_id, | ||
| ) | ||
| .order_by("id") | ||
| .values_list("id", flat=True)[:excess] | ||
| ) | ||
| models.RulebookProcessLog.objects.filter(id__in=oldest_ids).delete() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid counting the entire capped log set on every flush.
With the defaults, each 100-line flush eventually counts roughly 500,000 rows before deleting about 100. This recurring scan makes persistence increasingly expensive precisely when the safety valve activates. Use a bounded threshold query or maintain per-instance count metadata rather than an exact count per flush.
As per path instructions, focus on major issues impacting performance, readability, maintainability and security.
🤖 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 `@src/aap_eda/services/activation/db_log_handler.py` around lines 100 - 112,
Update the log-retention logic around num_of_log_lines so each flush avoids an
exact full-table count; use a bounded existence/threshold query to determine
whether the cap is exceeded, then delete only the required oldest
RulebookProcessLog rows while preserving per-activation filtering and ordering.
Source: Path instructions
1 participant
Summary
core_rulebook_process_log) grow without bound,causing API timeouts and UX failures when the table becomes too large
EDA_LOG_RETENTION_DAYS(default 30)and per-instance safety valve via
EDA_MAX_LOG_LINES_PER_INSTANCE(default 500000)
/activation-instances/{id}/logs/endpoint atmax_page_size=1000to prevent OOM from unbounded
page_sizerequestsTest Plan
task test -- tests/integration/tasks/test_log_cleanup.py -vtask test -- tests/integration/services/activation/test_db_log_handler.py -vtask test -- tests/integration/commands/test_purge_log_records.py -vtask test -- tests/integration/api/test_activation_instance.py -vpage_size=999999is capped to 1000 on running 2.7 instanceRelated
Summary by CodeRabbit
New Features
Documentation
Tests