Skip to content

fix: return a failure reason from POST /ride - #30

Merged
guytepper merged 1 commit into
mainfrom
fix/tag-ride-start-failures
Sep 3, 2026
Merged

fix: return a failure reason from POST /ride#30
guytepper merged 1 commit into
mainfrom
fix/tag-ride-start-failures

Conversation

@guytepper

Copy link
Copy Markdown
Member

Why

A user reported a live ride that wouldn't start on Android. The tap worked — the sheet appeared and then reverted a second later — but nothing surfaced anywhere: no error on screen, nothing in Sentry, and no way to tell from the server side which rejection it was.

startRideNotifications answers { success: false } for every failure, so route_not_found, ride_in_past and ride_in_future are indistinguishable to the client. On top of that, RideNotInTimeError was deliberately skipped by the logger, so the two most likely causes left no trace at all in this handler.

What changed

  • RideNotInTimeError / NotFoundRouteForRide now carry a reason, and rideFailureReason() maps anything else to internal_error.
  • POST /ride returns { success: false, reason }. reason is one of route_not_found | ride_in_past | ride_in_future | internal_error.
  • In-time rejections log at warn (they aren't server faults, but they need to be greppable); everything else keeps logging at error, now with the reason attached.

Status codes are unchanged, so shipped clients are unaffected — they only check success, and axios rejects a 500 either way.

Follow-up

The app side reads this reason and attaches it to a Sentry tag (ride_start_reason) alongside the failure stage and HTTP status — that PR is separate.

Not included here, worth deciding on:

  • ride_in_future still answers 500, which inflates any server error-rate metric. A 409/422 would be more honest and wouldn't break clients.
  • POST /ride is rate limited to 10 requests / 10 min per cf-connecting-ip (src/routes/api.ts). On Israeli cellular CGNAT many subscribers share one public IP, so a user on mobile data can be rejected by other people's traffic. Once the client tags are live we'll see whether 429 shows up in practice.

Testing

bun test — 101 pass, 0 fail. tsc --noEmit clean.

@guytepper
guytepper requested a review from planecore September 2, 2026 22:01
@planecore
planecore changed the base branch from gtfs-migration to main September 3, 2026 07:06
A rejected ride start answered with a bare `{ success: false }`, so every
cause looked identical to the app - it just reverted to the route details
screen with no way to tell a missing route from a ride outside the startable
window. Return a machine-readable `reason` the client can tag its error
report with, and log the in-time rejections (which previously logged nothing
at this level) at warn.
@planecore
planecore force-pushed the fix/tag-ride-start-failures branch from 38ccfa1 to 060e2b5 Compare September 3, 2026 07:08
@guytepper
guytepper merged commit c5358b3 into main Sep 3, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants