fix: saga orch ublishes saga started event, fixing concurr problems in saga routes#148
fix: saga orch ublishes saga started event, fixing concurr problems in saga routes#148
Conversation
📝 WalkthroughWalkthroughAdds SagaStartedEvent publishing in the saga orchestrator and a NotificationCreatedEvent publish step in the notification service; test helpers and tests updated to wait for saga-start and notification-created events. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SagaOrchestrator
participant SagaStore as "Saga Store"
participant Producer as "Event Producer"
participant TestWaiter as "Test EventWaiter"
Client->>SagaOrchestrator: ExecutionRequestedEvent
SagaOrchestrator->>SagaStore: Persist saga instance
SagaStore-->>SagaOrchestrator: Persisted confirmation
SagaOrchestrator->>Producer: Publish SagaStartedEvent
Producer-->>SagaOrchestrator: Acknowledgement (or error)
Producer->>TestWaiter: SagaStartedEvent (consumed by tests)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/e2e/conftest.py (1)
244-246:⚠️ Potential issue | 🟡 MinorStale assertion message — still references
CREATE_POD_COMMANDinstead ofSAGA_STARTED.The fixture now waits on
SAGA_STARTED(line 241), but the assertion failure message on line 245 still says"despite CREATE_POD_COMMAND received".🐛 Proposed fix
assert doc is not None, ( - f"No saga document for {created_execution.execution_id} despite CREATE_POD_COMMAND received" + f"No saga document for {created_execution.execution_id} despite SAGA_STARTED received" )
🧹 Nitpick comments (2)
backend/app/services/saga/saga_orchestrator.py (1)
316-336: Error swallowed silently — consider whether callers need to know.The
except Exceptionon line 335 logs but does not re-raise, which is intentional (publish failure shouldn't block saga execution). This is consistent with_publish_saga_cancelled_event. However, the log level iserrorwithoutexc_info=True, so the traceback is lost.🔧 Optional: preserve traceback for debugging
except Exception as e: - self.logger.error(f"Failed to publish saga started event: {e}") + self.logger.error(f"Failed to publish saga started event: {e}", exc_info=True)backend/tests/e2e/conftest.py (1)
105-113: Missingtype: ignoreone.execution_id— inconsistent withwait_for_result.Line 100 uses
# type: ignore[union-attr]for the samee.execution_idaccess pattern, but line 110 omits it. If mypy is enforced, this will produce a type error for the same reason.🔧 Suggested fix
return await self.wait_for( lambda e: ( e.event_type == EventType.CREATE_POD_COMMAND - and e.execution_id == execution_id + and e.execution_id == execution_id # type: ignore[union-attr] ), timeout=timeout, )
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/e2e/conftest.py (1)
255-258:⚠️ Potential issue | 🟡 MinorStale assertion message still references
CREATE_POD_COMMAND.Line 257 says
"despite CREATE_POD_COMMAND received"but the fixture now waits onSAGA_STARTED. Update the message for consistency.Proposed fix
doc = await SagaDocument.find_one(SagaDocument.execution_id == created_execution.execution_id) assert doc is not None, ( - f"No saga document for {created_execution.execution_id} despite CREATE_POD_COMMAND received" + f"No saga document for {created_execution.execution_id} despite SAGA_STARTED received" )
🧹 Nitpick comments (2)
backend/tests/e2e/conftest.py (2)
125-134:e.tagsaccess may raiseAttributeErrorif a non-notification event is evaluated.
DomainEventis a union type; not all variants have atagsattribute. If a non-NOTIFICATION_CREATEDevent somehow passes the first check (unlikely due to short-circuitand), or if the union discriminator doesn't narrow the type, the baree.tagsaccess could fail. In practice theevent_typeguard protects this at runtime, but consider addinghasattr/getattrfor safety and add a# type: ignore[union-attr]comment for consistency with line 100.
95-123: Consistent type-ignore comments across predicate lambdas.
wait_for_result(line 100) has# type: ignore[union-attr]on thee.execution_idaccess, butwait_for_saga_command(line 110) andwait_for_saga_started(line 120) accesse.execution_idwithout it. Consider adding the annotation uniformly to suppress mypy warnings.
1 participant
Summary by cubic
Publish SagaStartedEvent and NotificationCreatedEvent right after DB persistence to remove race conditions in saga routes and notification flows. E2E tests now wait on these events for deterministic reads.
Written for commit 948f71f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests