feat: osrtc implementation

[raghavcast]

No description provided.

[raghavcast]

@ClaudeWeb review this please

[khuzema786]

PR Review: feat: osrtc implementation

Solid implementation overall — the cache structure, token refresh lifecycle, and background task follow the existing ChaloVehicleCache pattern well. Two real bugs to fix before merge, plus a few things worth knowing.

---

⚠️ Warnings

1. Logic bug: unwrap_or fallback causes 500 when OSRTC is unconfigured

Files: src/handlers/routes.rs — get_stops (~line 584) and get_stop (~line 645)
Confidence: 92

Both handlers use this pattern:

if gtfs_id == app_state.config.osrtc_feed_key.as_ref().unwrap_or(&"odisha_osrtc".to_string()).to_string()

When osrtc_feed_key = None (credentials not configured), the fallback silently hardcodes "odisha_osrtc". Any request with gtfs_id = "odisha_osrtc" then enters the OSRTC block, hits osrtc_cache = None, and returns a 500 Internal Error — instead of falling through to the regular GTFS service.

Fix: Only enter the OSRTC path if the feed key is explicitly configured:

if app_state.config.osrtc_feed_key.as_deref() == Some(gtfs_id.as_str()) {

This way, None → condition is false → falls through to GTFS as expected.

---

2. Race condition in get_valid_token (TOCTOU)

File: src/services/osrtc_station_cache.rs — get_valid_token
Confidence: 85

async fn get_valid_token(&self) -> AppResult<String> {
    {
        let token = self.token.read().await;
        if let Some(t) = &*token {
            if t.expires_at > Utc::now() + Duration::minutes(5) {
                return Ok(t.access_token.clone());
            }
        }
    } // <-- read lock dropped here
    self.refresh_token().await  // <-- N concurrent callers all reach this
}

Multiple concurrent station refreshes (or the initial initialize() + background task race) can all observe an expired token simultaneously, drop the read lock, and then all call refresh_token() in parallel — hammering the OSRTC auth endpoint and potentially hitting rate limits.

Fix: Use a tokio::sync::Mutex for the token so check-and-refresh is atomic:

token: Arc<tokio::sync::Mutex<Option<OsrtcToken>>>,

async fn get_valid_token(&self) -> AppResult<String> {
    let mut token_guard = self.token.lock().await;
    if let Some(t) = &*token_guard {
        if t.expires_at > Utc::now() + Duration::minutes(5) {
            return Ok(t.access_token.clone());
        }
    }
    // Refresh while holding the lock — only one caller proceeds
    let new_token = self.fetch_token().await?;
    *token_guard = Some(new_token);
    Ok(token_guard.as_ref().unwrap().access_token.clone())
}

---

3. External UAT URL hardcoded in committed dev config

File: dhall-configs/dev/gtfs_in_memory_server_rust.dhall
Confidence: 82

osrtc_base_url = Some "https://osrtcuatthirdpartyapi.amnex.com",

Committing a third-party partner's UAT endpoint to version control means rotating it requires a code change, and it may violate the partner's acceptable-use policy. Consider moving it to the secrets dhall or at minimum using a placeholder like Some "OSRTC_BASE_URL" in dev, consistent with how other URLs are handled.

---

💡 Potential Issues

4. get_all_stations double-copies the entire station map on every request

File: src/services/osrtc_station_cache.rs — get_all_stations
Confidence: 72

pub async fn get_all_stations(&self) -> Vec<OsrtcStation> {
    let stations = self.stations.read().await;
    stations.values().cloned().collect()  // full clone
}
// then in routes.rs:
let mappings: Vec<RouteStopMapping> = stations.iter().map(...).collect(); // second allocation

For large station lists this is two full heap allocations per request. Consider storing a pre-built Arc<Vec<RouteStopMapping>> and swapping it atomically on each refresh — then get_all_stations is a single Arc::clone.

---

5. stop_point serializes as (0.0, 0.0) — real coordinates

File: src/services/osrtc_station_cache.rs — osrtc_station_to_route_stop_mapping
Confidence: 68

stop_point: LatLong { lat: 0.0, lon: 0.0 }, // Doesn't matter, and won't be consumed.

(0°N, 0°E) is a valid coordinate (Gulf of Guinea) and serializes into the JSON response. Any client that renders stop markers on a map will place all OSRTC stations off the coast of Africa. If stop_point is nullable in the broader schema, using Option<LatLong> here would be safer and self-documenting. If not, this assumption ("won't be consumed") should be verified with the consuming client team.

---

✅ What's good

• Token refresh with a 5-minute buffer is a solid approach.
• Graceful fallback (None cache → disabled, with an info log) is clean.
• HashMap::with_capacity on station refresh avoids reallocation.
• Background task pattern matches the existing ChaloVehicleCache convention.
• Credentials kept in the secrets dhall, not the dev config.

Bottom line: Fix issues #1 and #2 before merge. Issue #1 in particular is a correctness bug that will silently 500 in any environment where OSRTC isn't fully configured.

[khuzema786]

I'll fetch the PR details, diff, and existing comments in parallel.The token lacks read:org scope. Let me try fetching the PR data directly via the API.Good, I have the file list. Now let me fetch the diff and read the key new files in parallel.Now let me look at the workspace for additional context on the models and how other caches handle similar patterns.I have a full picture of the changes. Now let me post the review.Review posted to PR #92. Here's a summary of what I found:

2 bugs to fix before merge:

1. Logic bug in routes.rs — The unwrap_or(&"odisha_osrtc".to_string()) fallback means that when OSRTC credentials aren't configured (osrtc_feed_key = None), requests to /stops/odisha_osrtc still enter the OSRTC code path, hit osrtc_cache = None, and 500. Fix: app_state.config.osrtc_feed_key.as_deref() == Some(gtfs_id.as_str()).

2. TOCTOU race in get_valid_token — The read lock is dropped before refresh_token is called, so multiple concurrent callers can all see an expired token and hammer the OSRTC auth endpoint simultaneously. Fix: swap RwLock → Mutex on the token field so check-and-refresh is atomic.

3 lower-priority items: external UAT URL hardcoded in dev config, double-clone on every get_all_stations call, and stop_point: (0.0, 0.0) serializing as real coordinates that map clients will render off the coast of Africa.