Don't try to roll back the database on a failed message delivery, we … #2231
Conversation
…must allow the error so we have an audit of what happened. Otherwise we have to kill all trace of it, which just creates a grand mystery.
|
Warning Rate limit exceeded@danfunk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adjusts error handling in the MessageService by removing the automatic database rollback and instead marking related message instances as "failed" while preserving the failure reason. A comment is added to highlight a potential issue with deleting the message instance in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Test
participant MS as MessageService
participant DB as Database
participant MIS as MessageInstance(s)
participant Log as Logger
U->>MS: Trigger message processing
MS->>DB: Attempt to process message
DB-->>MS: Exception/Error
MS->>MIS: Set status to "failed" (send instance)
alt If message_instance_receive exists
MS->>MIS: Set status to "failed" (receive instance)
end
MS->>Log: Log error ("error after starting")
MS->>DB: Delete message instance in run_process_model_from_message
sequenceDiagram
participant C as Client
participant BPMN as BPMN Engine
participant SE as Start Event
participant ST as Script Task
participant EE as End Event
C->>BPMN: Trigger "MJE_Process_test"
BPMN->>SE: Initiate process via message event ("test_bad_process")
SE->>ST: Flow to script task (contains placeholder script)
ST->>EE: Complete process execution
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: 1
🧹 Nitpick comments (1)
spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_message_service.py (1)
100-140: LGTM! Test case effectively verifies error handling.The test case correctly:
- Sets up a process that will fail
- Verifies that the error note is preserved
Consider these improvements:
- Remove unused
payloadvariable- Remove unused
message_instancevariable- Break long line 126 into multiple lines for better readability
- payload = { - "customer_id": "Sartography", - "po_number": 1001, - "description": "We built a new feature for messages!", - "amount": "100.00", - } # Load up the definition for the receiving process # It has a message start event that should cause it to fire when a unique message comes through # Fire up the first process load_test_spec( "test_group/message-start-with-error", process_model_source_directory="message-start-with-error", bpmn_file_name="message-start-with-error.bpmn", ) # Now send in the message user = self.find_or_create_user() - message_triggerable_process_model = MessageTriggerableProcessModel.query.filter_by(message_name="test_bad_process").first() + message_triggerable_process_model = ( + MessageTriggerableProcessModel.query + .filter_by(message_name="test_bad_process") + .first() + ) assert message_triggerable_process_model is not None - message_instance = MessageInstanceModel( - message_type="send", - name="test_bad_process", - payload={}, - user_id=user.id, - ) g.user = user🧰 Tools
🪛 Ruff (0.8.2)
108-108: Local variable
payloadis assigned to but never usedRemove assignment to unused variable
payload(F841)
126-126: Line too long (131 > 130)
(E501)
129-129: Local variable
message_instanceis assigned to but never usedRemove assignment to unused variable
message_instance(F841)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spiffworkflow-backend/src/spiffworkflow_backend/services/message_service.py(3 hunks)spiffworkflow-backend/tests/data/message-start-with-error/message-start-with-error.bpmn(1 hunks)spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_message_service.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_message_service.py
108-108: Local variable payload is assigned to but never used
Remove assignment to unused variable payload
(F841)
126-126: Line too long (131 > 130)
(E501)
129-129: Local variable message_instance is assigned to but never used
Remove assignment to unused variable message_instance
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: typeguard 3.12 / ubuntu-latest sqlite
- GitHub Check: typeguard 3.11 / ubuntu-latest sqlite
- GitHub Check: tests 3.11 / macos-latest sqlite
- GitHub Check: tests 3.10 / ubuntu-latest sqlite
- GitHub Check: tests 3.12 / ubuntu-latest sqlite
- GitHub Check: tests 3.11 / ubuntu-latest sqlite
- GitHub Check: tests 3.12 / ubuntu-latest postgres
- GitHub Check: tests 3.11 / ubuntu-latest postgres
- GitHub Check: tests 3.12 / ubuntu-latest mysql
- GitHub Check: tests 3.11 / ubuntu-latest mysql
- GitHub Check: check_docker_start_script
- GitHub Check: build
- GitHub Check: pixeebot[bot]
🔇 Additional comments (2)
spiffworkflow-backend/tests/data/message-start-with-error/message-start-with-error.bpmn (1)
1-45: LGTM! Test file is well-structured for error handling verification.The BPMN file is correctly configured with:
- An executable process with a message start event
- A script task with invalid code to trigger a failure
- Proper sequence flows and message definitions
spiffworkflow-backend/src/spiffworkflow_backend/services/message_service.py (1)
151-164: LGTM! Improved error handling by preserving error information.The changes correctly implement the PR's objective by:
- Preventing database rollback to preserve error information
- Setting message instance statuses to "failed"
- Recording failure causes
- Adding a descriptive error note
spiffworkflow-backend/src/spiffworkflow_backend/services/message_service.py
Outdated
Show resolved
Hide resolved
…must allow the error so we have an audit of what happened. Otherwise we have to kill all trace of it, which just creates a grand mystery.
Summary by CodeRabbit
Bug Fixes
New Features
Tests