refactor(car-sharing): harden the trip saga (idempotency, compensation order, error mapping)#35
Merged
Merged
Conversation
…n order, error mapping)
Apply the same hardenings CodeRabbit flagged on the hris onboarding saga to the
gold-standard car-sharing saga so the two examples stay in lockstep.
- Idempotent reservation (F3): add `holder_id` to ReserveVehicleRequest + a
`holder` column to the vehicles table. ReserveVehicle now stamps the holder
and, on a held vehicle, returns success only when the SAME holder re-reserves
(a Temporal retry observing its own commit) — a different holder is still
FAILED_PRECONDITION (no double-booking). ReleaseVehicle clears the holder.
reserveVehicle is a lock-acquisition (not a create), so read-back equivalence
is unsafe here; a holder key is the correct idempotency technique.
- GetTrip error mapping (F2): the blanket `catch {}` mapped ANY query failure to
a terminal status. Narrow it: only WorkflowNotFoundError / QueryNotRegistered /
QueryRejected (run closed/gone) fall back to describe(); a transient failure is
surfaced as Code.Unavailable instead of being reported as a SETTLED/CANCELLED
trip.
- Compensation ordering (F4): register markTripCancelled (step 2) and voidTab
(step 5) BEFORE their forward calls, so an ambiguous failure that committed the
side effect is still unwound. Both comps are no-ops on uncommitted state. Step 1
and step 6 (refundCharge needs the charge id) stay register-after.
- Dockerfile (F1): replace the inaccurate "1.0.0 pinned" note with an honest
no-committed-lockfile rationale that points production users at
`--frozen-lockfile`; keep the flagship no-lockfile policy.
New regression tests: same-holder reserve is idempotent; a different holder is
rejected; GetTrip surfaces Unavailable on a transient query failure. Migration +
journal committed. typecheck + tests: 42/42 green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MdeH7fExPmiRHRirGuvGk3
📝 WalkthroughWalkthroughThe PR adds a nullable ChangesIdempotent Reservation, Saga Robustness, and Error Classification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Buf (1.71.0)car-sharing/proto/fleet/v1/fleet.protoFailure: no .proto files were targeted. This can occur if no .proto files are found in your input, --path points to files that do not exist, or --exclude-path excludes all files. 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 |
github-actions
Bot
added
the
type:chore
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@car-sharing/src/services/fleetService.ts`:
- Around line 141-142: The WHERE clause in the vehicle release update on line
141-142 only checks the vehicle ID, creating a double-booking vulnerability if
retries occur after response loss. Add a holder field to ReleaseVehicleRequest
and thread it through the activity and workflow to the releaseVehicle method.
Modify the WHERE clause to include an additional condition that verifies the
current holder matches the expected holder using eq(vehicles.holder,
expectedHolder), ensuring the update only clears the lock if held by the same
reservation that requested the release. Treat the case where holder is already
null as idempotent success to prevent errors on legitimate retries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 586cb44b-0db3-41e3-85ee-989e3a989485
📒 Files selected for processing (14)
car-sharing/Dockerfilecar-sharing/README.mdcar-sharing/drizzle/0001_sturdy_wendell_vaughn.sqlcar-sharing/drizzle/meta/0001_snapshot.jsoncar-sharing/drizzle/meta/_journal.jsoncar-sharing/proto/fleet/v1/fleet.protocar-sharing/src/db/schema.tscar-sharing/src/services/fleetService.tscar-sharing/src/services/tripService.tscar-sharing/src/temporal/activities.tscar-sharing/src/temporal/workflows.tscar-sharing/tests/activity/activities.test.tscar-sharing/tests/e2e/e2e.test.tscar-sharing/tests/workflow/tripWorkflow.test.ts
Comment on lines
+141
to
142
| .set({ available: true, status: VehicleStatus.AVAILABLE, holder: null, updatedAt: new Date() }) | ||
| .where(eq(vehicles.id, req.id)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Guard release by the reservation holder.
Line 141 clears the lock using only id in the WHERE clause. If a releaseVehicle compensation commits but its activity response is lost, a retry can run after another trip reserves the now-available vehicle and then clear that other trip’s holder, reopening double-booking. Thread the expected holder through ReleaseVehicleRequest/activity/workflow and only clear when the stored holder matches, while treating an already-available holder = null row as idempotent success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@car-sharing/src/services/fleetService.ts` around lines 141 - 142, The WHERE
clause in the vehicle release update on line 141-142 only checks the vehicle ID,
creating a double-booking vulnerability if retries occur after response loss.
Add a holder field to ReleaseVehicleRequest and thread it through the activity
and workflow to the releaseVehicle method. Modify the WHERE clause to include an
additional condition that verifies the current holder matches the expected
holder using eq(vehicles.holder, expectedHolder), ensuring the update only
clears the lock if held by the same reservation that requested the release.
Treat the case where holder is already null as idempotent success to prevent
errors on legitimate retries.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Mirrors the four CodeRabbit "Major" hardenings raised on the hris onboarding saga (#31) into the gold-standard car-sharing saga, so the two flagship examples stay in lockstep. Each is an example-grade simplification today; this makes the saga genuinely retry-safe.
What
reserveVehicleis a lock acquisition, not a create — read-back equivalence would re-admit double-booking (no holder to disambiguate). So addholder_idtoReserveVehicleRequest+ aholdercolumn tovehicles: ReserveVehicle stamps the holder and, on a held vehicle, succeeds only when the same holder re-reserves (a Temporal retry observing its own commit); a different holder staysFAILED_PRECONDITION. ReleaseVehicle clears the holder. (hris feat(hris): durable onboarding saga (Temporal) — Phase 5a #31 uses the complementary read-back technique, since its step 1 is a create-by-id.)catch {}mapped any query failure to a terminal status. Narrowed: onlyWorkflowNotFoundError/QueryNotRegisteredError/QueryRejectedError(run closed/gone) fall back todescribe(); a transient failure surfaces asCode.Unavailableinstead of a bogus SETTLED/CANCELLED trip.markTripCancelled(step 2) andvoidTab(step 5) before their forward calls, so an ambiguous failure that committed the side effect is still unwound (both comps no-op on uncommitted state). Step 1 and step 6 (refundChargeneeds the charge id) stay register-after.^1.0.0caret ranges) with an honest no-committed-lockfile rationale pointing production users at--frozen-lockfile; flagship no-lockfile policy kept.Verification
Unavailableon a transient query failure;recordTrip/openTabfailure unwinds the pre-registered compensation (proves F4's register-before).pnpm typecheck✓ ·pnpm test44/44 ✓ (was 39).Companion: the same hardenings land on hris #31 (read-back for F3 there).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests