Enhancement/cancel triggers #505
Conversation
- Add DELETE /graph/{graph_name}/triggers endpoint
- Implement cancel_triggers controller with support for PENDING and TRIGGERING statuses
- Add mark_as_cancelled function to update trigger status and set expires_at
- Include CANCELLED status in TTL index for automatic cleanup
- Return count and status message in response
- Add test_cancel_triggers.py with 9 controller tests covering: - Cancellation of PENDING and TRIGGERING triggers - No triggers found scenario - Settings integration and retention hours - Error handling for database and mark errors - Batch processing multiple triggers - Update test_trigger_cron.py to include mark_as_cancelled in parametrized tests for consistency with mark_as_triggered and mark_as_failed
SafeDep Report Summary
No dependency changes detected. Nothing to scan. This report is generated by SafeDep Github App |
|
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 CodeRabbit
WalkthroughAdds a DELETE API endpoint to cancel PENDING/TRIGGERING triggers for a graph, a controller that queries and concurrently marks found triggers as CANCELLED using retention hours, a response model CancelTriggerResponse, a mark_as_cancelled helper, TTL index extended to include CANCELLED, and unit tests for these flows. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as "DELETE /graph/{graph_name}/triggers"
participant Route as cancel_triggers_route
participant Controller as cancel_triggers()
participant DB as DatabaseTriggers
participant Settings as SettingsService
participant Task as mark_as_cancelled()
Client->>API: DELETE + API key
API->>Route: forward request
Route->>Controller: namespace, graph, request_id
Controller->>DB: query triggers where status IN [PENDING, TRIGGERING]
DB-->>Controller: list of triggers
alt triggers found
Controller->>Settings: get_settings() -> trigger_retention_hours
Settings-->>Controller: retention_hours
Controller->>Task: asyncio.gather mark_as_cancelled(trigger, retention_hours) for each
Task->>DB: update trigger (status=CANCELLED, expires_at)
DB-->>Task: update results
else no triggers
Controller-->>Route: CancelTriggerResponse(cancelled_count=0, message)
end
Controller-->>Route: CancelTriggerResponse(cancelled_count=n, message)
Route-->>API: response 200
API-->>Client: returns response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @Kalra-V, 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 introduces a significant enhancement by providing the capability to cancel active graph triggers. It establishes a new API endpoint for users to programmatically halt graph executions that are currently in a 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 introduces a new endpoint to cancel pending and triggering triggers, along with corresponding controller logic, database model updates, and comprehensive tests. The implementation is solid and well-tested. My feedback focuses on improving performance by using concurrent execution for cancellations, enhancing code clarity by removing unused imports and duplicated logic, and strengthening the tests with more specific assertions. Overall, this is a great enhancement.
| # Check that all three conditions are in the call | ||
| # The call should include namespace, graph_name, and In for trigger_status | ||
| assert call_args is not None |
There was a problem hiding this comment.
The assertion assert call_args is not None is quite weak. It only confirms that the find method was called, but not with which arguments. To make this test more robust, you should assert on the specific filters being passed to the find method. A simple but effective improvement is to check the number of filter arguments passed.
| # Check that all three conditions are in the call | |
| # The call should include namespace, graph_name, and In for trigger_status | |
| assert call_args is not None | |
| # Verify find was called with the correct number of filter arguments | |
| # (namespace, graph_name, and trigger_status). | |
| assert len(call_args.args) == 3 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
state-manager/app/controller/cancel_triggers.py(1 hunks)state-manager/app/models/cancel_trigger_models.py(1 hunks)state-manager/app/models/db/trigger.py(1 hunks)state-manager/app/routes.py(2 hunks)state-manager/app/tasks/trigger_cron.py(1 hunks)state-manager/tests/unit/controller/test_cancel_triggers.py(1 hunks)state-manager/tests/unit/tasks/test_trigger_cron.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
state-manager/tests/unit/controller/test_cancel_triggers.py (4)
state-manager/app/controller/cancel_triggers.py (1)
cancel_triggers(15-67)state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)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/controller/cancel_triggers.py (6)
state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)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)state-manager/app/config/settings.py (1)
get_settings(34-38)state-manager/app/tasks/trigger_cron.py (2)
trigger_cron(97-101)mark_as_cancelled(103-112)
state-manager/app/routes.py (3)
state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)state-manager/app/controller/cancel_triggers.py (1)
cancel_triggers(15-67)state-manager/app/utils/check_secret.py (1)
check_api_key(11-16)
state-manager/tests/unit/tasks/test_trigger_cron.py (2)
state-manager/app/tasks/trigger_cron.py (1)
mark_as_cancelled(103-112)state-manager/app/models/trigger_models.py (1)
TriggerStatusEnum(9-14)
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)
🔇 Additional comments (11)
state-manager/app/models/cancel_trigger_models.py (1)
1-9: LGTM!The
CancelTriggerResponsemodel is well-defined with clear field descriptions. The structure aligns with the controller implementation and follows Pydantic best practices.state-manager/app/models/db/trigger.py (1)
47-48: LGTM!The TTL index update correctly includes
CANCELLEDstatus alongsideTRIGGEREDandFAILED. This ensures that cancelled triggers are automatically cleaned up when theirexpires_attimestamp is reached, maintaining consistency with other terminal trigger states.state-manager/tests/unit/tasks/test_trigger_cron.py (3)
13-13: LGTM!The import of
mark_as_cancelledcorrectly extends the module's public interface for testing.
25-29: LGTM!The parametrized test now includes
mark_as_cancelledalongsidemark_as_triggeredandmark_as_failed, ensuring consistent behavior across all marking functions for TTL and retention logic.
67-77: LGTM!The test coverage for custom retention periods now includes
mark_as_cancelledwith various retention hour values (12, 24, 48), ensuring the function respects custom retention periods consistently with other mark functions.state-manager/app/tasks/trigger_cron.py (2)
103-112: LGTM!The
mark_as_cancelledfunction correctly follows the same pattern asmark_as_triggeredandmark_as_failed. It properly:
- Calculates
expires_atin UTC using the providedretention_hours- Updates the trigger status to
CANCELLED- Sets the
expires_atfield for TTL-based cleanup
101-101: The concurrent execution withasyncio.gatheris pre-existing code, not introduced by this PR.Based on the git history, the
asyncio.gatherline at line 101 was already present in previous commits and is not part of the changes in this PR. The only modifications totrigger_cron.pyin the last 5 commits are: (1) a newline formatting change at line 101, and (2) the addition of themark_as_cancelledfunction. This concurrent execution pattern was established several commits ago and is already tested bytest_trigger_cron(), which explicitly verifies thathandle_triggeris calledtrigger_workerstimes concurrently.Likely an incorrect or invalid review comment.
state-manager/app/controller/cancel_triggers.py (1)
27-67: Verify partial cancellation behavior on error.If
mark_as_cancelledfails partway through the loop (e.g., on the 3rd of 5 triggers), some triggers will be marked as cancelled while others remain in their original state. The exception is re-raised (Line 67), but there's no rollback mechanism.Confirm this partial cancellation behavior is acceptable, or consider implementing error recovery logic.
state-manager/tests/unit/controller/test_cancel_triggers.py (1)
1-275: LGTM!The test suite provides comprehensive coverage for the
cancel_triggerscontroller:
- Successful cancellation scenarios for PENDING and TRIGGERING statuses
- Empty result handling
- Query filter validation
- Settings integration
- Error handling for both database and marking failures
- Batch processing verification
The tests are well-structured, use appropriate mocking, and follow pytest best practices.
state-manager/app/routes.py (2)
57-60: LGTM!The import statements are well-organized with clear comments, following the existing pattern in the file.
244-261: LGTM!The new
DELETE /graph/{graph_name}/triggersendpoint follows the same implementation pattern as other secured endpoints in this file:
- Proper response model configuration
- Consistent API key validation
- Request ID extraction and passing
- Clean delegation to the controller
- Made changes that coderabbitai & gemini required
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
state-manager/tests/unit/controller/test_cancel_triggers.py (1)
137-139: Strengthen assertion to verify query filters.The assertion only checks that
findwas called but doesn't verify the query filters (namespace, graph_name, trigger_status). This was flagged in a previous review and remains unaddressed.Apply this diff to verify the filter arguments:
# Verify find was called with correct arguments mock_db.find.assert_called_once() call_args = mock_db.find.call_args - # Check that all three conditions are in the call - # The call should include namespace, graph_name, and In for trigger_status - assert call_args is not None + # Verify that find was called with three filter arguments: + # namespace, graph_name, and trigger_status (In operator) + assert len(call_args.args) == 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
state-manager/app/controller/cancel_triggers.py(1 hunks)state-manager/tests/unit/controller/test_cancel_triggers.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
state-manager/tests/unit/controller/test_cancel_triggers.py (3)
state-manager/app/controller/cancel_triggers.py (1)
cancel_triggers(15-67)state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)
state-manager/app/controller/cancel_triggers.py (6)
state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)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)state-manager/app/config/settings.py (1)
get_settings(34-38)state-manager/app/tasks/trigger_cron.py (2)
trigger_cron(97-101)mark_as_cancelled(103-112)
🔇 Additional comments (2)
state-manager/app/controller/cancel_triggers.py (1)
1-67: LGTM! Past review suggestions have been addressed.The controller correctly implements concurrent cancellation using
asyncio.gatherand simplifies the counter logic withlen(triggers). The structure, error handling, and integration with settings and mark_as_cancelled are all sound.state-manager/tests/unit/controller/test_cancel_triggers.py (1)
1-274: Comprehensive test coverage.The test suite covers all major scenarios including success cases, error handling, settings integration, and edge cases. Well-structured and thorough.
- Fixed naming convent suggested by coderabbitai Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
state-manager/tests/unit/controller/test_cancel_triggers.py (1)
137-139: The assertion is too weak and doesn't verify the actual filter conditions.This assertion only confirms that
findwas called, but doesn't validate the namespace, graph_name, or trigger_status filters. This issue was previously flagged.Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
state-manager/tests/unit/controller/test_cancel_triggers.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/tests/unit/controller/test_cancel_triggers.py (3)
state-manager/app/controller/cancel_triggers.py (1)
cancel_triggers(15-67)state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)
🔇 Additional comments (6)
state-manager/tests/unit/controller/test_cancel_triggers.py (6)
53-87: LGTM!This test properly verifies that
mark_as_cancelledis called with the correct trigger and retention_hours arguments.
89-110: LGTM!This test correctly verifies the no-triggers-found path, including the early return that skips calling
get_settingsandmark_as_cancelled.
142-168: LGTM!This test properly verifies that the
retention_hoursfrom settings (48) is correctly passed tomark_as_cancelled.
170-182: LGTM!This test correctly verifies that database errors are propagated to the caller.
184-210: LGTM!This test correctly verifies that errors during
mark_as_cancelledare propagated. Since the controller usesasyncio.gather, exceptions will bubble up appropriately.
246-273: LGTM!This test correctly verifies that
get_settingsis called when triggers exist and that the retention_hours from settings are properly passed tomark_as_cancelled.
| # Verify mark_as_cancelled was called for each trigger | ||
| assert mock_mark_cancelled.call_count == 2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Verify the arguments passed to mark_as_cancelled, not just the call count.
The test confirms mark_as_cancelled was called twice but doesn't verify that each trigger was passed with the correct retention_hours (24). This could miss bugs where incorrect arguments are used.
Consider adding assertions to verify the arguments:
- # Verify mark_as_cancelled was called for each trigger
- assert mock_mark_cancelled.call_count == 2
+ # Verify mark_as_cancelled was called for each trigger with correct retention_hours
+ assert mock_mark_cancelled.call_count == 2
+ mock_mark_cancelled.assert_any_call(mock_trigger1, 24)
+ mock_mark_cancelled.assert_any_call(mock_trigger2, 24)📝 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.
| # Verify mark_as_cancelled was called for each trigger | |
| assert mock_mark_cancelled.call_count == 2 | |
| # Verify mark_as_cancelled was called for each trigger with correct retention_hours | |
| assert mock_mark_cancelled.call_count == 2 | |
| mock_mark_cancelled.assert_any_call(mock_trigger1, 24) | |
| mock_mark_cancelled.assert_any_call(mock_trigger2, 24) |
🤖 Prompt for AI Agents
state-manager/tests/unit/controller/test_cancel_triggers.py around lines 49-50:
the test only asserts mock_mark_cancelled.call_count == 2 but doesn't verify the
actual arguments; update the test to assert that mark_as_cancelled was called
with each trigger and retention_hours=24 by checking
mock_mark_cancelled.call_args_list or using assert_has_calls with two expected
calls (trigger1, retention_hours=24) and (trigger2, retention_hours=24) in the
correct order (or unordered if order is not guaranteed).
| async def test_cancel_triggers_multiple_triggers_batch(): | ||
| """Test that multiple triggers are cancelled in batch""" | ||
| namespace_name = "test_namespace" | ||
| graph_name = "test_graph" | ||
| x_exosphere_request_id = "test_request_id" | ||
|
|
||
| mock_trigger1 = MagicMock(spec=DatabaseTriggers) | ||
| mock_trigger1.id = "trigger_id_1" | ||
| mock_trigger2 = MagicMock(spec=DatabaseTriggers) | ||
| mock_trigger2.id = "trigger_id_2" | ||
| mock_trigger3 = MagicMock(spec=DatabaseTriggers) | ||
| mock_trigger3.id = "trigger_id_3" | ||
|
|
||
| with patch('app.controller.cancel_triggers.DatabaseTriggers') as mock_db, \ | ||
| patch('app.controller.cancel_triggers.get_settings') as mock_get_settings, \ | ||
| patch('app.controller.cancel_triggers.mark_as_cancelled') as mock_mark_cancelled: | ||
|
|
||
| mock_query = MagicMock() | ||
| mock_query.to_list = AsyncMock(return_value=[mock_trigger1, mock_trigger2, mock_trigger3]) | ||
| mock_db.find.return_value = mock_query | ||
|
|
||
| mock_settings = MagicMock() | ||
| mock_settings.trigger_retention_hours = 24 | ||
| mock_get_settings.return_value = mock_settings | ||
|
|
||
| result = await cancel_triggers(namespace_name, graph_name, x_exosphere_request_id) | ||
|
|
||
| # Verify correct count | ||
| assert result.cancelled_count == 3 | ||
|
|
||
| # Verify all triggers were processed | ||
| assert mock_mark_cancelled.call_count == 3 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name improvement looks good, but consider verifying arguments.
The renamed test correctly reflects batch processing without implying ordering guarantees. However, similar to the first test, it only verifies the call count without checking that each trigger is passed with the correct retention_hours.
Consider adding assertions to verify the arguments:
# Verify all triggers were processed
assert mock_mark_cancelled.call_count == 3
+ mock_mark_cancelled.assert_any_call(mock_trigger1, 24)
+ mock_mark_cancelled.assert_any_call(mock_trigger2, 24)
+ mock_mark_cancelled.assert_any_call(mock_trigger3, 24)📝 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.
| async def test_cancel_triggers_multiple_triggers_batch(): | |
| """Test that multiple triggers are cancelled in batch""" | |
| namespace_name = "test_namespace" | |
| graph_name = "test_graph" | |
| x_exosphere_request_id = "test_request_id" | |
| mock_trigger1 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger1.id = "trigger_id_1" | |
| mock_trigger2 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger2.id = "trigger_id_2" | |
| mock_trigger3 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger3.id = "trigger_id_3" | |
| with patch('app.controller.cancel_triggers.DatabaseTriggers') as mock_db, \ | |
| patch('app.controller.cancel_triggers.get_settings') as mock_get_settings, \ | |
| patch('app.controller.cancel_triggers.mark_as_cancelled') as mock_mark_cancelled: | |
| mock_query = MagicMock() | |
| mock_query.to_list = AsyncMock(return_value=[mock_trigger1, mock_trigger2, mock_trigger3]) | |
| mock_db.find.return_value = mock_query | |
| mock_settings = MagicMock() | |
| mock_settings.trigger_retention_hours = 24 | |
| mock_get_settings.return_value = mock_settings | |
| result = await cancel_triggers(namespace_name, graph_name, x_exosphere_request_id) | |
| # Verify correct count | |
| assert result.cancelled_count == 3 | |
| # Verify all triggers were processed | |
| assert mock_mark_cancelled.call_count == 3 | |
| async def test_cancel_triggers_multiple_triggers_batch(): | |
| """Test that multiple triggers are cancelled in batch""" | |
| namespace_name = "test_namespace" | |
| graph_name = "test_graph" | |
| x_exosphere_request_id = "test_request_id" | |
| mock_trigger1 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger1.id = "trigger_id_1" | |
| mock_trigger2 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger2.id = "trigger_id_2" | |
| mock_trigger3 = MagicMock(spec=DatabaseTriggers) | |
| mock_trigger3.id = "trigger_id_3" | |
| with patch('app.controller.cancel_triggers.DatabaseTriggers') as mock_db, \ | |
| patch('app.controller.cancel_triggers.get_settings') as mock_get_settings, \ | |
| patch('app.controller.cancel_triggers.mark_as_cancelled') as mock_mark_cancelled: | |
| mock_query = MagicMock() | |
| mock_query.to_list = AsyncMock(return_value=[mock_trigger1, mock_trigger2, mock_trigger3]) | |
| mock_db.find.return_value = mock_query | |
| mock_settings = MagicMock() | |
| mock_settings.trigger_retention_hours = 24 | |
| mock_get_settings.return_value = mock_settings | |
| result = await cancel_triggers(namespace_name, graph_name, x_exosphere_request_id) | |
| # Verify correct count | |
| assert result.cancelled_count == 3 | |
| # Verify all triggers were processed | |
| assert mock_mark_cancelled.call_count == 3 | |
| mock_mark_cancelled.assert_any_call(mock_trigger1, 24) | |
| mock_mark_cancelled.assert_any_call(mock_trigger2, 24) | |
| mock_mark_cancelled.assert_any_call(mock_trigger3, 24) |
🤖 Prompt for AI Agents
In state-manager/tests/unit/controller/test_cancel_triggers.py around lines 213
to 245, the test checks only that mark_as_cancelled was called three times but
does not verify the actual arguments; update the test to assert that each
mark_as_cancelled call includes the correct trigger instance and the
retention_hours pulled from mock_get_settings
(mock_settings.trigger_retention_hours) and that the x_exosphere_request_id is
forwarded; do this by inspecting mock_mark_cancelled.call_args_list and either
asserting each expected call (using unittest.mock.call with the trigger and
keyword retention_hours=mock_settings.trigger_retention_hours and
x_exosphere_request_id) is present or by iterating over call_args_list to check
trigger identity and the expected keyword values, without relying on call order.
|
@Kalra-V requesting to add apt tests for the code to increase the coverage. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
state-manager/app/controller/cancel_triggers.py(1 hunks)state-manager/tests/unit/test_routes.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
state-manager/app/controller/cancel_triggers.py (6)
state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)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)state-manager/app/config/settings.py (1)
get_settings(34-38)state-manager/app/tasks/trigger_cron.py (2)
trigger_cron(97-101)mark_as_cancelled(103-112)
state-manager/tests/unit/test_routes.py (2)
state-manager/app/models/cancel_trigger_models.py (1)
CancelTriggerResponse(4-8)state-manager/app/routes.py (2)
manual_retry_state_route(194-203)cancel_triggers_route(251-260)
🔇 Additional comments (8)
state-manager/tests/unit/test_routes.py (5)
12-12: LGTM!The import of
CancelTriggerResponseis necessary for the new test cases and follows the existing import structure.
41-41: LGTM!The route path assertion correctly validates the presence of the new cancel triggers endpoint.
300-329: LGTM!The model validation tests comprehensively cover both successful cancellation scenarios and the zero-count edge case. The tests follow the established patterns in this file.
353-373: LGTM!The handler existence checks properly validate that
cancel_triggers_routeis defined and callable, consistent with checks for other route handlers.
1156-1221: LGTM!The three test functions comprehensively cover the cancel triggers route:
- Valid API key scenario with proper controller invocation
- Invalid API key scenario with expected 401 response
- Missing request ID scenario with UUID generation
The tests follow the established patterns and properly mock dependencies.
state-manager/app/controller/cancel_triggers.py (3)
1-13: LGTM!All imports are used and necessary. The asyncio import supports concurrent cancellation, and the logger initialization follows the standard pattern.
27-62: LGTM! Concurrent cancellation properly implemented.The controller correctly:
- Queries for PENDING/TRIGGERING triggers with proper filtering
- Returns early with a clear message when no triggers are found
- Cancels triggers concurrently using
asyncio.gather(addressing past feedback)- Uses
len(triggers)directly for the count (addressing past feedback)- Logs outcomes at appropriate levels
The concurrent implementation significantly improves performance when cancelling multiple triggers.
64-67: LGTM!Error handling is appropriate: logging provides context for troubleshooting, and re-raising allows the route handler to properly format HTTP error responses.
| # Cancel each trigger concurrently | ||
| cancelled_count = len(triggers) | ||
| cancellation_tasks = [mark_as_cancelled(trigger, retention_hours) for trigger in triggers] | ||
| await asyncio.gather(*cancellation_tasks) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Consider partial-failure visibility.
The current asyncio.gather uses fail-fast behavior: if any mark_as_cancelled call fails, the entire batch fails and the user receives no indication of how many triggers were successfully cancelled before the failure.
For better user experience, consider using return_exceptions=True to handle partial failures:
# Cancel each trigger concurrently
cancellation_tasks = [mark_as_cancelled(trigger, retention_hours) for trigger in triggers]
results = await asyncio.gather(*cancellation_tasks, return_exceptions=True)
# Count successful cancellations
cancelled_count = sum(1 for r in results if not isinstance(r, Exception))
failures = [r for r in results if isinstance(r, Exception)]
if failures:
logger.warning(f"Failed to cancel {len(failures)} trigger(s) for graph {graph_name}",
x_exosphere_request_id=x_exosphere_request_id)
# Optionally: raise if no triggers were cancelled successfullyThe current all-or-nothing approach is acceptable for ensuring consistency, but partial success information may be valuable for troubleshooting.
🤖 Prompt for AI Agents
In state-manager/app/controller/cancel_triggers.py around lines 50 to 53, the
code uses asyncio.gather without return_exceptions so a single failure aborts
the whole batch and hides partial successes; change to await
asyncio.gather(*tasks, return_exceptions=True), collect results, count
non-Exception results as cancelled_count, and log/collect Exception instances as
failures (warning with graph_name and request id), optionally raising only if
zero succeeded.
There was a problem hiding this comment.
@Kalra-V this suggestion seems valid. Lets incorporate this.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| triggers = await DatabaseTriggers.find( | ||
| DatabaseTriggers.namespace == namespace_name, | ||
| DatabaseTriggers.graph_name == graph_name, | ||
| In(DatabaseTriggers.trigger_status, [TriggerStatusEnum.PENDING, TriggerStatusEnum.TRIGGERING]) |
There was a problem hiding this comment.
If a Database trigger is in TRIGGERING state, I don't think we should cancel in that state. Why? Because it might be immediately picked by the process to move to TRIGGERED. This will create an inconsistent state where actually the job is Triggered but it shows TRIGGERING.
| # Get retention hours from settings | ||
| settings = get_settings() | ||
| retention_hours = settings.trigger_retention_hours |
| # Cancel each trigger concurrently | ||
| cancelled_count = len(triggers) | ||
| cancellation_tasks = [mark_as_cancelled(trigger, retention_hours) for trigger in triggers] | ||
| await asyncio.gather(*cancellation_tasks) |
There was a problem hiding this comment.
this sends a number of queries, can we use some bulk update operation here? Also can we merge and do something like bulk get and update.
| # Cancel each trigger concurrently | ||
| cancelled_count = len(triggers) | ||
| cancellation_tasks = [mark_as_cancelled(trigger, retention_hours) for trigger in triggers] | ||
| await asyncio.gather(*cancellation_tasks) |
There was a problem hiding this comment.
@Kalra-V this suggestion seems valid. Lets incorporate this.
| from app.models.trigger_models import TriggerStatusEnum | ||
| from app.singletons.logs_manager import LogsManager | ||
| from app.config.settings import get_settings | ||
| from app.tasks.trigger_cron import mark_as_cancelled |
There was a problem hiding this comment.
I will not recommend importing from tasks in controller. This might lead to confusing dependencies going forward.
| async def mark_as_cancelled(trigger: DatabaseTriggers, retention_hours: int): | ||
| expires_at = datetime.now(timezone.utc) + timedelta(hours=retention_hours) | ||
|
|
||
| await DatabaseTriggers.get_pymongo_collection().update_one( | ||
| {"_id": trigger.id}, | ||
| {"$set": { | ||
| "trigger_status": TriggerStatusEnum.CANCELLED, | ||
| "expires_at": expires_at | ||
| }} | ||
| ) |
2 participants
Add DELETE /graph/{graph_name}/triggers endpoint
Implement cancel_triggers controller with support for PENDING and TRIGGERING statuses
Add mark_as_cancelled function to update trigger status and set expires_at
Include CANCELLED status in TTL index for automatic cleanup
Return count and status message in response
Add test_cancel_triggers.py with 9 controller tests covering:
Update test_trigger_cron.py to include mark_as_cancelled in
parametrized tests for consistency with mark_as_triggered and
mark_as_failed
API endpoint response when cancelling 1 trigger:
API endpoint response when no PENDING or TRIGGERING trigger is there: