Skip to content

Propagate errors that were silently swallowed across services and frontend - #17

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1785398949-error-handling-propagation
Open

Propagate errors that were silently swallowed across services and frontend#17
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1785398949-error-handling-propagation

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

Audit of error handling across the services + frontend. Several failures were being turned into apparent success, which is worse than a loud failure because Camunda/BPMN and the user both keep going as if nothing happened. This PR fixes the cases where the failure was actually lost, and adds the exception object (or a log line) everywhere a failure was intentionally tolerated but invisible.

Failures that were reported as success

BankWorkerunlockMoney/chargeMoneyBlock logged the rejected bank response and returned, so the Zeebe job auto-completed and the process moved on as if the money had been unlocked/charged (blockMoney already failed the process properly):

 if (!response.isSuccess()) {
     log.error(...);
-    return;
+    throw new BankOperationException("Unlock money failed for accountId " + accountId + ": " + ...);
 }

ProcessSessionWorker.deleteProcessSession — swallowed the exception, so an undeleted session still completed the job; now rethrown after logging (Zeebe retries → incident).

FleetSimulatorsendStep ended with .exceptionally(ex -> { log; return null; }), which completes the future successfully. Consequence: POST /monitoring/start returned 200 RENTED and the startMonitoring worker considered monitoring active even when both gRPC calls failed, and the vehicle stayed in activeSimulations forever. Same pattern hid stopTracking/stopMonitoring failures on /monitoring/stop. exceptionallywhenComplete (logs, keeps the failure in the future); registerNewRental also drops the simulation it just registered when the first step fails. The scheduled tick keeps its per-vehicle isolation but now logs the async step failure instead of discarding the future.

HttpRentalServiceCallbackClient — a failed VEHICLE_RECHARGED callback was caught and logged, so the async recharge finished "successfully" while the rental process waited for a message that would never arrive. It now throws StationOperationException, and StationsService.completeRecharge retries the callback (3 attempts, 500 ms apart) before letting the async future fail — logged with the exception via .exceptionally(...) on the CompletableFuture (the public rechargeVehicle response stays "recharge started" by design).

bank.olresposnse.errorStatus = "INSUFFICIENT_FUNDS" typo meant the insufficient-funds rejection never carried an errorStatus, so BankWorker logged null for the one error status that matters.

Failures that were logged but stripped of detail

MonitoringController had log.error("... {}", vehicleId, e.getMessage()) in all three .exceptionally(...) handlers: with one placeholder and a String (not Throwable) as the extra argument, SLF4J drops the argument entirely — the cause was never printed. Now passes e. Same treatment for FleetManagementWorker, FleetSimulator's tick, GraphHopperClient (fallback behaviour unchanged), ZeebeDeploymentConfig and SseRedisSubscriber (also narrowed to JsonProcessingException and now logs dropped emitters).

FleetManagementWorker.monitoringLogic additionally guards the newFailCommand(...) call itself, so a failure while reporting a failure isn't swallowed.

Domain rejection vs unexpected failure (rental-service)

scanQr/bookByType/undoBooking caught RuntimeException and returned success=false, message=<any exception message>, so a DB outage or NPE was presented to the user as an ordinary business rejection. Domain rejections now use a dedicated RentalOperationException and only that is converted to a failure DTO; anything else propagates. IllegalArgumentException (e.g. a non-numeric vehicleId in GET /rentals/booking) is mapped to 400 in ProcessExceptionHandler instead of surfacing as a 500.

Frontend

catchError(() => EMPTY) in the resume polling of ProcessNavigationService discarded every failure with no trace; the retry semantics are kept but each failed attempt is logged through a skipFailedPoll(operation, error) helper. In ProcessEventService the reconciliation catchError moved inside switchMap — an error there previously killed the subscription for all subsequent navigation-state-changed events — plus logging for malformed SSE payloads and EventSource errors. SessionService (bootstrap + logout) and rideCompletionGuard keep their fallback behaviour but log why.

Tests

New: BankWorkerTest, ProcessSessionWorkerTest, failure cases in FleetSimulatorTest (registerNewRental/unregisterRental propagate and don't leak state), HttpRentalServiceCallbackClientTest.failedCallbackIsReportedToTheCaller, StationsServiceTest.rechargeCallbackIsRetriedBeforeGivingUp.

mvn test green for rental-service, stations-service, fm-gateway; ng build clean. ng test has 2 failures (rideCompletionGuard redirects…, ActiveRideComponent opens the transient summary…) that reproduce identically on main — pre-existing, untouched here.

Link to Devin session: https://app.devin.ai/sessions/11592aa2fba14aebb7839320f456a2db
Requested by: @NicolasCola7

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@NicolasCola7 NicolasCola7 self-assigned this Jul 30, 2026
@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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.

1 participant