Do not delete canceled tasks #2103
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
spiffworkflow-backend/tests/data/subprocess_with_manual_task/subprocess_with_manual_task.bpmn (2)
12-36: LGTM: Subprocess is well-defined with clear instructions.The subprocess "Activity_subprocess_task_in_level_1" is correctly defined with proper internal structure (start event, manual task, end event). The instructions for the end user are comprehensive and provide good guidance on how to proceed and suggestions for further modifications.
Consider adding a brief explanation of what a subprocess is and how it relates to the main process in the instructions. This could help users better understand the structure of the workflow.
32-35: LGTM: Manual task is correctly defined and connected.The manual task "manual_task_in_level_2" is properly defined within the subprocess and correctly connected to the start and end events using sequence flows.
Consider adding more specific instructions or data inputs/outputs for the manual task to provide clearer guidance on what needs to be done during this step of the process.
spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_processor.py (1)
1139-1163: LGTM! Consider adding assertions for termination state.The new test method
test_can_terminate_instance_with_subprocessis a valuable addition to ensure that process instances with subprocesses can be terminated correctly. The test setup, process model loading, and execution steps are well-structured.To further enhance the test, consider adding assertions after the termination to verify the final state of the process instance and its subprocesses. For example:
processor.terminate() assert process_instance.status == ProcessInstanceStatus.terminated.value assert len(process_instance.active_human_tasks) == 0 # Add more assertions as needed to verify the termination stateThis will provide stronger guarantees that the termination behaves as expected.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (1)
Line range hint
1947-1962: Good addition of task removal, consider adding a comment for KeyError suppression.The changes to the
terminatemethod improve the process by callingself.remove_spiff_tasks_for_termination()before setting the status to "terminated". This ensures that tasks are properly cleaned up during termination.Consider adding a brief comment explaining why
KeyErroris being suppressed in thewith suppress(KeyError):block. This would help future developers understand the rationale behind this decision.Example:
# Suppress KeyError that might occur during task removal to ensure termination completes with suppress(KeyError): self.remove_spiff_tasks_for_termination()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (2 hunks)
- spiffworkflow-backend/tests/data/subprocess_with_manual_task/subprocess_with_manual_task.bpmn (1 hunks)
- spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_process_instance_processor.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
spiffworkflow-backend/tests/data/subprocess_with_manual_task/subprocess_with_manual_task.bpmn (4)
1-3: LGTM: BPMN definitions and process declaration are correct.The XML declaration, namespaces, and process definition are properly set up. The process is correctly marked as executable, which is essential for running the workflow.
4-11: LGTM: Start and end events are properly defined and connected.The start and end events of the main process are correctly defined and properly connected to the subprocess using sequence flows. This ensures a clear flow of execution in the process.
38-80: LGTM: BPMN diagram definitions are complete and accurate.The BPMN diagram definitions for both the main process and the subprocess are correctly specified. They include proper shapes and edges with coordinates and dimensions, ensuring an accurate visual representation of the process structure.
1-81: Overall, the BPMN file is well-structured and correctly implements the required process.This new BPMN file successfully defines a process with a subprocess containing a manual task, which aligns with the PR objective of "Do not delete canceled tasks". The implementation is correct and provides a good foundation for testing the behavior of subprocesses and manual tasks.
Key strengths:
- Proper XML structure and namespace declarations
- Clear process flow with correct event connections
- Well-defined subprocess with comprehensive instructions
- Correctly implemented manual task
- Accurate BPMN diagram definitions for visual representation
The file serves its purpose as a test data file and will be valuable for validating the handling of subprocesses and manual tasks in the SpiffWorkflow backend.
spiffworkflow-backend/src/spiffworkflow_backend/services/process_instance_processor.py (2)
Line range hint
1937-1947: Simplified task removal logic looks good, but verify no information loss.The changes to the
remove_spiff_tasks_for_terminationmethod simplify the logic by directly callingcancel()onself.bpmn_process_instanceand passing an empty list toupdate_all_tasks_from_spiff_tasksinstead of tracking deleted tasks. This approach could improve performance and reduce complexity.Please verify that no critical information is lost by not explicitly tracking deleted tasks. Run the following script to check if there are any references to
deleted_tasksin other parts of the codebase that might be affected by this change:
Line range hint
1937-1962: Overall improvements to termination process, ensure thorough testing.The changes to both
remove_spiff_tasks_for_terminationandterminatemethods appear to streamline and improve the process of terminating workflow instances. The simplification of task removal logic and the addition of explicit task removal during termination are positive changes.To ensure these changes don't introduce any unintended side effects, please conduct thorough testing of the termination process, including:
- Terminating processes at various stages of completion
- Verifying that all associated tasks are properly removed or updated
- Checking database consistency after termination
- Testing error handling during termination
Consider adding or updating unit tests to cover these scenarios:
Do not delete the tasks that spiff says have been canceled. These tasks have not actually been deleted by spiff and so the database should also not delete them. This has been at least one cause for the PendingRollbackError.
This is probably the correct fix for #1861.
Summary by CodeRabbit
New Features
Bug Fixes
Tests