fix trips-for-route: handle DUPLICATED trips and add time window support

[Ahmedhossamdev]

Fixes: #660 #661, #662, #663, #664

The trips-for-route endpoint was returning significantly fewer trips compared to the Java server. In some cases it returned zero trips for routes that should have had 15+.

What was wrong

1. NULL block_id → 0 results. -> The handler only did block-based lookups to find active trips. Routes where every trip has block_id = NULL (like Sound Transit Link) got nothing back. Added a fallback batch query for null-block trips. (fix: NULL block_id trips returning 0 results - trips-for-route #661)

2. SQLite parameter binding was silently wrong -> The new null-block query used sqlc.slice() + sqlc.arg() together, but sqlc expands slices into ?NNN positional params at runtime. When the sqlc.arg() came after the slice, its position shifted and SQLite bound the wrong value. Reordered the SQL so sqlc.arg() params come first. (fix: SQLite parameter binding misalignment in null-block query - trips-for-route #662)

3. DUPLICATED trips ignored -> GTFS-RT feeds include schedule_relationship = DUPLICATED (value 6) for extra real-time runs of a trip. These only exist in real-time data with no static DB entry. Added GetDuplicatedVehiclesForRoute() and a processing loop that resolves the base trip ID for schedule data. (Include GTFS-RT DUPLICATED trips (schedule_relationship=6) #663)

4. Past-midnight trips missing -> GTFS allows stop times > 24:00:00 for trips that started the previous service day. After midnight, the handler only looked at today's service IDs and missed these. Added previous-day service lookups. (Support previous-day service for trips running past midnight (GTFS times > 24:00:00) #664)

Also in this PR

• Cross-route block filter -> Block-based lookups could pull in trips from other routes sharing the same block. Added a post-fetch route_id filter.

• Refactored BuildTripStatus -> Added an optional vehicle parameter so callers can pass a pre-resolved vehicle (used by DUPLICATED trips). When nil, it falls back to GetVehicleForTrip. Also added dbTripID fallback so synthetic DUPLICATED trip IDs that don't exist in the DB resolve to the base trip for schedule lookups. Updated all call sites (arrival-and-departure-for-stop, arrivals-and-departures-for-stop, trip-details, trip-for-vehicle, trips-for-location).

• CANCELED trip early return -> BuildTripStatus now returns immediately for CANCELED trips, skipping stop-time and shape calculations that are meaningless for a trip that's not operating.

• TripStatus fields always present (removed omitempty) -> Changed pointer fields (*float64, *int, *int64) to value types and removed omitempty from JSON tags so every field is always serialized. This matches Java OBA which never omits fields. Defaults: occupancyCapacity/occupancyCount → -1, vehicleFeatures/situationIds → [], frequency → null.

• schedule-for-route: removed redundant query -> GetOrderedStopIDsForTrip was called separately just to collect stop IDs, but GetStopTimesForTrip was called right after and returns the same stop IDs. Removed the extra query and extracted stop IDs from stop times directly.

• trips-for-location: avoid unnecessary DB round-trip in reference builder -> buildTripReferences was re-fetching each trip from the DB via GetTrip() just to get route/headsign info that was already present in the in-memory Trip model. Removed the extra query and used the existing data directly.

Still working on it.

[aaronbrethorst]

Hey Ahmed, this is an ambitious PR that tackles several real pain points in the trips-for-route endpoint -- null-block trips, past-midnight handling, DUPLICATED trip support, and the SQLite parameter binding fix are all genuinely important improvements. The investigation work and the detailed PR description are really solid. There are a few things I need you to address before we can merge:

Critical

1. DB errors conflated with "not found" in DUPLICATED trip lookup (trips_for_route_handler.go:342-347). GetTrip returns sql.ErrNoRows when a trip doesn't exist, but returns other errors for real DB failures (connection issues, SQLite busy, etc.). The current code treats any error as "trip not found" and falls back to stripping the numeric suffix. Under DB pressure, this could resolve DUPLICATED trips to the wrong base trip, producing incorrect schedule data. Use errors.Is(lookupErr, sql.ErrNoRows) to distinguish the two cases.

2. Same issue in BuildTripStatus dbTripID fallback (trips_helper.go:83-88). The GetTrip check for activeTripRawID treats any error as "not found" and falls back to tripID. A transient DB error would cause stop times, shapes, and distances to be computed against the wrong trip, producing an internally inconsistent TripStatus. Same fix: check for sql.ErrNoRows specifically.

Important

1. SQL comment contradicts actual behavior (query.sql:1047-1049). The comment says "Use time_range_start = now - 10 min and time_range_end = now + 30 min" but the Go code does now - 30 min (runningLateNanos) and now + 10 min (runningEarlyNanos). The Go code is correct (late vehicles are more common, so the backward window should be larger). Please fix the SQL comment to match.

2. Previous-day queries silently discard errors throughout. Several previous-day lookups discard errors entirely while their current-day equivalents properly surface them:

   • Line 76: prevServiceIDs, _ := ...GetActiveServiceIDsForDate(...) -- error discarded
   • Lines 120-138: Previous-day block lookups use if err == nil guards, hiding DB errors
   • Lines 151-161: Previous-day null-block trips error discarded with if err == nil
   • Line 202: GetTripsInBlock error treated same as "no trips"

   These don't need to be server errors, but they should all log at Warn level so operators can see when past-midnight data is degraded. The inconsistency between how current-day and previous-day errors are handled is a debugging blind spot.

3. No tests for the core new logic functions. selectBestTripInBlock and stripNumericSuffix are both pure functions that are trivially unit-testable, but have zero test coverage. selectBestTripInBlock in particular implements a 4-tier priority system -- a bug there silently returns the wrong active trip for entire blocks. Please add unit tests for both.

4. BuildTripStatus with non-nil vehicle is untested. Every existing test passes nil for the vehicle parameter. The non-nil path (used by DUPLICATED trips in production) has no direct coverage. Please add at least one test that verifies a pre-resolved vehicle is used correctly.

5. PR description says "Still working on it". Is this PR ready for review, or should I wait for more changes? If it's ready, please update the description.

Fit and Finish

1. selectBestTripInBlock first pass is dead code (trips_for_route_handler.go:639-644). This function is only called when GetActiveTripInBlockAtTime returns an error (no matching trip), but the first pass checks the exact same condition (minArrival <= now <= maxDeparture). It will never match. Either remove it or add a comment explaining why it's retained as a safety net.

2. Missing route_id context in null-block trip log (trips_for_route_handler.go:147). The Warn log "failed to fetch null-block trips" would be much more useful with "route_id", routeID included.

Verdict

Request changes -- please fix the two critical ErrNoRows discrimination issues, add the missing unit tests for selectBestTripInBlock and stripNumericSuffix, and confirm the PR is ready for review. The error logging improvements and the dead code cleanup can be done in the same pass.

[Ahmedhossamdev]

Hey Aaron Brethorst, thanks for the review!
I'm still testing the changes with different feeds, so it's not fully finished yet.

• I'll rebase against main once I'm done.

Thanks