fix: wire mark_as_cancelled into trigger cancellation flow & replace destructive deletes with TTL-based cleanup #590
Conversation
…e delete_many with TTL-based cleanup
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces immediate deletion of CRON triggers with marking them as CANCELLED and assigning an expires_at computed from settings.trigger_retention_hours; updates TTL index to include CANCELLED; adds helpers to mark/cancel triggers and runs a startup task to bulk-cancel old triggers. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Controller as Controller\n(upsert_graph_template)
participant Settings as Settings\n(get_settings)
participant DB as MongoDB\n(Triggers collection)
participant Task as Init Task\n(mark_old_triggers_cancelled)
participant TTL as TTL Worker
rect rgb(220,240,220)
Note over Controller,Settings: During upsert of graph template
Controller->>Settings: get trigger_retention_hours
Settings-->>Controller: retention_hours
Controller->>DB: update_many(filter=CRON triggers for graph/namespace, set {trigger_status: CANCELLED, expires_at})
DB-->>Controller: update result
end
rect rgb(220,240,240)
Note over Task,Settings: Startup cleanup
Task->>Settings: get trigger_retention_hours
Settings-->>Task: retention_hours
Task->>DB: update_many(filter=old TRIGGERED/FAILED with expires_at null, set {trigger_status: CANCELLED, expires_at})
DB-->>Task: update result
end
rect rgb(240,230,220)
Note over DB,TTL: TTL-based expiration
TTL->>DB: remove documents where expires_at <= now() and status in [TRIGGERED, FAILED, CANCELLED]
DB-->>TTL: deletion complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-09-28T13:35:42.862ZApplied to files:
📚 Learning: 2025-10-22T14:23:18.774ZApplied to files:
📚 Learning: 2025-08-03T16:46:04.030ZApplied to files:
📚 Learning: 2025-08-06T14:04:44.515ZApplied to files:
🧬 Code graph analysis (2)state-manager/app/tasks/trigger_cron.py (4)
state-manager/app/tasks/init_tasks.py (4)
🔇 Additional comments (13)
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 |
Summary of ChangesHello @Brijesh-Thakkar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request finalizes the implementation of a robust TTL-based cleanup strategy for database triggers. It addresses previous gaps where Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly transitions from destructive trigger deletion to a TTL-based cleanup by marking them as CANCELLED and setting an expires_at timestamp. The changes to the MongoDB index and the logic in upsert_graph_template.py are well-implemented. However, I've identified critical issues in the newly added code in init_tasks.py and trigger_cron.py. In both files, enum members are passed directly to raw PyMongo queries instead of their string values, which will likely cause these database operations to fail. My review includes suggestions to fix these issues by using the .value attribute on the enums, ensuring the TTL cleanup mechanism works as intended.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
state-manager/app/config/settings.py(1 hunks)state-manager/app/controller/upsert_graph_template.py(3 hunks)state-manager/app/models/db/trigger.py(1 hunks)state-manager/app/tasks/init_tasks.py(1 hunks)state-manager/app/tasks/trigger_cron.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: NiveditJain
Repo: exospherehost/exospherehost PR: 428
File: state-manager/app/tasks/verify_graph.py:4-5
Timestamp: 2025-09-28T13:35:42.862Z
Learning: In the cron trigger scheduling logic for state-manager/app/tasks/verify_graph.py, the system intentionally schedules at least one trigger beyond the trigger_ahead_time window to ensure continuity of scheduled executions. The current logic of appending an event then breaking is by design to guarantee "at least one next" trigger.
📚 Learning: 2025-09-28T13:35:42.862Z
Learnt from: NiveditJain
Repo: exospherehost/exospherehost PR: 428
File: state-manager/app/tasks/verify_graph.py:4-5
Timestamp: 2025-09-28T13:35:42.862Z
Learning: In the cron trigger scheduling logic for state-manager/app/tasks/verify_graph.py, the system intentionally schedules at least one trigger beyond the trigger_ahead_time window to ensure continuity of scheduled executions. The current logic of appending an event then breaking is by design to guarantee "at least one next" trigger.
Applied to files:
state-manager/app/tasks/trigger_cron.pystate-manager/app/controller/upsert_graph_template.py
📚 Learning: 2025-10-22T14:23:18.774Z
Learnt from: NiveditJain
Repo: exospherehost/exospherehost PR: 464
File: state-manager/app/tasks/init_tasks.py:16-20
Timestamp: 2025-10-22T14:23:18.774Z
Learning: In state-manager/app/tasks/init_tasks.py, the init_tasks() function intentionally uses asyncio.gather with a list pattern (even with a single task) to provide an extensible structure for easily adding future startup tasks without refactoring.
Applied to files:
state-manager/app/tasks/init_tasks.py
📚 Learning: 2025-08-03T16:46:04.030Z
Learnt from: NiveditJain
Repo: exospherehost/exospherehost PR: 132
File: state-manager/app/controller/upsert_graph_template.py:11-27
Timestamp: 2025-08-03T16:46:04.030Z
Learning: In the exospherehost codebase, for upsert operations on graph templates, the team prioritizes API idempotency over avoiding race conditions in the database layer implementation. The approach of separate find and insert/update operations is acceptable when the API behavior remains consistent.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
📚 Learning: 2025-08-06T14:04:44.515Z
Learnt from: NiveditJain
Repo: exospherehost/exospherehost PR: 158
File: state-manager/app/controller/upsert_graph_template.py:49-49
Timestamp: 2025-08-06T14:04:44.515Z
Learning: In the exospherehost codebase, the team prefers to run graph template verification on every upsert operation rather than optimizing by skipping verification when templates haven't changed, prioritizing comprehensive validation over performance optimization.
Applied to files:
state-manager/app/controller/upsert_graph_template.py
🧬 Code graph analysis (3)
state-manager/app/tasks/trigger_cron.py (2)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)
state-manager/app/models/db/trigger.py (1)
state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)
state-manager/app/tasks/init_tasks.py (4)
state-manager/app/config/settings.py (1)
get_settings(34-38)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)state-manager/app/singletons/logs_manager.py (2)
LogsManager(9-66)get_logger(65-66)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (4)
state-manager/app/config/settings.py (1)
7-9: LGTM!Minor formatting adjustment. The
trigger_retention_hourssetting is correctly defined and will be used by other modules in this PR for TTL-based cleanup.state-manager/app/models/db/trigger.py (1)
43-51: LGTM!The TTL index partial filter now correctly includes
CANCELLEDalongsideTRIGGEREDandFAILED, ensuring all terminal states are subject to TTL-based cleanup. This aligns with the PR objective of replacing destructive deletes with TTL expiration.state-manager/app/tasks/init_tasks.py (1)
19-24: Verify filter scope is intentional.The filter targets triggers with
TRIGGEREDorFAILEDstatus but without anexpires_atvalue. This appears to be a migration path for legacy triggers that predate TTL support.However, this filter does not include
namespaceorgraph_name, so it will affect all matching triggers across the entire database. Confirm this global scope is intentional for the startup cleanup task.state-manager/app/tasks/trigger_cron.py (1)
48-61: Verify thatmark_as_cancelledis wired into the trigger cancellation flow.The function implementation is sound and follows established patterns, but its integration point needs confirmation. Ensure this function is called from the appropriate cancellation handler in the trigger processing logic.
There was a problem hiding this comment.
Pull request overview
This PR aims to complete the TTL-based cleanup implementation for CANCELLED triggers by wiring in the mark_as_cancelled() helper and replacing destructive delete_many() operations with TTL-aware updates. However, there are critical issues with the implementation that need to be addressed.
Key intended changes:
- Add
mark_as_cancelled()helper function for consistent cancellation handling - Replace
delete_many()withupdate_many()to preserve audit history - Ensure all CANCELLED triggers receive
expires_attimestamps for MongoDB TTL cleanup
Critical issues identified:
- The new
mark_as_cancelled()function is defined but never actually used—cancellation logic is still implemented inline - The init task incorrectly marks already-completed triggers (TRIGGERED/FAILED) as CANCELLED instead of just setting their
expires_at - Missing test coverage for the new function despite existing test patterns
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| state-manager/app/tasks/trigger_cron.py | Adds mark_as_cancelled() helper function, but it's not integrated anywhere |
| state-manager/app/tasks/init_tasks.py | Converts delete_many() to update_many(), but incorrectly transitions TRIGGERED/FAILED to CANCELLED status |
| state-manager/app/models/db/trigger.py | Updates TTL index to include CANCELLED status (correct) |
| state-manager/app/controller/upsert_graph_template.py | Replaces delete_many() with inline cancellation logic using update_many() |
| state-manager/app/config/settings.py | Minor whitespace-only change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ed across init, cron, and upsert_graph_template
1 participant
Summary
This change completes the implementation of TTL-based cleanup for
CANCELLEDtriggers by integrating the existingmark_as_cancelled()helper into the actual cancellation flow and removing all remaining
delete_many()operations that bypassed TTL handling.With this update:
CANCELLEDtriggers now receive a validexpires_attimestampThis finalizes the cleanup strategy introduced in PR #464.
Background
PR #464 added TTL cleanup for
TRIGGEREDandFAILEDtriggers andintroduced
mark_as_cancelled(), but the helper was never actually used.As a result:
CANCELLEDtriggers hadexpires_at = nulldelete_many(), permanently deletingdocuments outside the retention lifecycle
This commit addresses those gaps.
Changes Included
1. Integrates
mark_as_cancelled()into the cancellation flowEvery cancellation now applies:
trigger_status = CANCELLEDexpires_at = now + retention_hours2. Removes all manual
delete_many()logicReplaced with TTL-aware updates in:
upsert_graph_template.pyinit_tasks.py3. Ensures consistent retention behavior
All terminal states (
TRIGGERED,FAILED,CANCELLED) now follow thesame TTL-based lifecycle.
4. Improves logging
Adds clearer logs around cancellation and state transitions.
Testing
Manual verification was performed by:
PENDINGtrigger{ "trigger_status": "CANCELLED", "expires_at": "<future timestamp>" }