Verify trip status

[Ahmedhossamdev]

Fix: #108
Replaces: #408

[Ahmedhossamdev]

Hey @aaronbrethorst

I think this branch is ready to be merged now. I suggest we do a squash and merge so our main branch stays linear, since rebasing was too complex to resolve.

[aaronbrethorst]

Hey Ahmed, thanks for taking the time to make this substantial improvement to the trip status and vehicle status logic. The overall direction here is excellent: aligning with the Java OBA server's actual semantics for schedule relationships, introducing stale detection, consolidating duplicated code across handlers, and adding thorough test coverage (~1,400 lines of new tests!) are all meaningful wins. The StaleDetector is particularly well-designed, and the bug fixes (timezone time.Local to loc, missing agency prefix on TripID, schedule deviation nanoseconds-to-seconds) are important corrections. Before we can merge this, I will need you to make a couple changes:

Critical

1. VehicleID no longer includes agency prefix -- behavioral regression. In trips_helper.go:35, the new code sets status.VehicleID = vehicle.ID.ID (e.g., "v123"), but the old code used utils.FormCombinedID(agencyID, vehicle.ID.ID) (e.g., "25_v123"). The OBA API convention uses combined IDs throughout. This is a breaking change for API consumers. Please either restore the combined ID format:

   if vehicle.ID != nil {
       status.VehicleID = utils.FormCombinedID(agencyID, vehicle.ID.ID)
   }

   or, if the raw ID is intentional (to match vehicles_for_agency_handler.go), document it as a deliberate change.

2. fillStopsFromSchedule can overwrite vehicle-derived ClosestStop. In trips_helper.go:117-119, the guard is:

   if status.ClosestStop == "" || status.NextStop == "" {
       api.fillStopsFromSchedule(...)
   }

   But inside fillStopsFromSchedule, both ClosestStop and NextStop are unconditionally set. If the vehicle-based stop-finding successfully set ClosestStop but not NextStop (e.g., vehicle is at the last stop of a trip), the more accurate vehicle-derived value gets overwritten with a schedule-based guess. Please guard the individual fields:

   if i > 0 && status.ClosestStop == "" {
       status.ClosestStop = ...
   }
   if status.NextStop == "" {
       status.NextStop = ...
   }

3. BuildTripStatus uses mismatched trip IDs for stop times vs shape queries. GetStopTimesForTrip on line 63 uses activeTripRawID (which may differ from the requested trip if the vehicle was reassigned within a block), but GetShapePointsByTripID on line 121 uses the original tripID. If the vehicle is on a different trip within the block, the shape data and stop times will be for different trips, producing incorrect position/distance calculations. Please use a consistent trip ID for both, and add a comment explaining the choice.

Important

4. Missing RLock comment on BuildTripStatus. The main branch has // IMPORTANT: Caller must hold manager.RLock() before calling this method. above this function, but the PR dropped it. Since BuildTripStatus calls GetSituationIDsForTrip (which documents the same requirement), please restore the comment.

5. MockAddVehicle and MockAddVehicleWithOptions don't acquire realTimeMutex. The new MockAddTripUpdate and MockResetRealTimeData both correctly lock m.realTimeMutex, but the vehicle mock methods don't. This inconsistency could cause flaky tests. Please add locking to be consistent.

6. getFirstStopOfNextTripInBlock silently swallows three database errors. This new function (trips_helper.go:951-982) returns nil for GetTrip, GetTripsByBlockIDOrdered, and GetStopTimesForTrip failures with zero logging. Please add slog.Warn logging consistent with the pattern used in calculateBlockTripSequence right above it.

7. Inconsistent error handling for BuildTripSchedule failure. In trip_details_handler.go:151, a BuildTripSchedule failure returns a server error response. In trip_for_vehicle_handler.go:102, the same failure is logged as a warning and the handler continues with nil schedule. These should behave consistently -- either both should degrade gracefully or both should return a server error.

8. Missing test coverage for negative schedule deviation (early vehicles). Every test uses positive deviation (late vehicle). The effectiveScheduleTime calculation in findStopsByScheduleDeviation flips sign for negative deviations, which could produce unexpected results. Please add at least one test with a negative deviation.

9. Missing test for getFirstStopOfNextTripInBlock success path. TestFindNextStopBySequence_StoppedAtLastStop only exercises the failure case (trip not in DB). There's no test verifying that the block continuation logic correctly finds the first stop of the next trip in a block. This is a critical real-world scenario.

10. findNextStopBySequence has a redundant serviceDateForBlock parameter. The function signature has both serviceDate time.Time and serviceDateForBlock time.Time, but the call site always passes the same value for both. If they're always identical, simplify the signature to avoid confusion.

Fit and Finish

11. Missing doc comments on exported functions. GetScheduleDeviation and GetStopDelaysFromTripUpdates in trip_updates_helper.go are exported but have no doc comments. Please add brief comments explaining what they return (seconds, priority of trip-level vs stop-level delay, etc.).

12. scheduleRelationshipStatus silently maps UNSCHEDULED, REPLACEMENT, and DELETED to "SCHEDULED". While this matches the Java behavior, a DELETED trip being reported as "SCHEDULED" is semantically concerning. Please add a comment to the default case noting the intentional omission.

13. Comment in BuildVehicleStatus claims avoiding a duplicate query, but getVehicleDistanceAlongShapeContextual makes its own GetShapePointsByTripID call. The comment on vehicles_helper.go:128 ("avoiding a duplicate GetShapePointsByTripID query") is inaccurate. Please either fix the comment or refactor to actually avoid the duplicate query.

14. OPTIONAL: Java line number references are fragile. Comments referencing specific line numbers in Java source files (e.g., "line 252-253") will drift as the Java codebase evolves. Consider referencing method names only (e.g., TripStatusBeanServiceImpl.getBlockLocationAsStatusBean()).

15. shapeRowsToPoints drops ShapeDistTraveled without documenting it. The conversion intentionally discards distance data from the database rows. Please note this in the comment so future developers understand distances are recomputed via preCalculateCumulativeDistances.

16. Empty phase for canceled trips should be documented. GetVehicleStatusAndPhase returns "" for phase when the trip is canceled. Add a brief comment explaining this matches Java's null-phase behavior for canceled trips.

Thanks again, and I look forward to merging this change.

[Ahmedhossamdev]

BuildTripStatus uses mismatched trip IDs for stop times vs shape queries. GetStopTimesForTrip on line 63 uses activeTripRawID (which may differ from the requested trip if the vehicle was reassigned within a block), but GetShapePointsByTripID on line 121 uses the original tripID. If the vehicle is on a different trip within the block, the shape data and stop times will be for different trips, producing incorrect position/distance calculations. Please use a consistent trip ID for both, and add a comment explaining the choice.

For this I think for this we should use the activeTripId because it's changing based of the GTFS-RT data.

[Ahmedhossamdev]

@aaronbrethorst Done