fix(cache): never hold a cache semaphore across an API call [CORE-4248]

[jamm3e3333]

What is broken

When a client asks the SDK for data, the SDK also shares that answer with its other caches. It is a good idea — one request warms several caches. But it shares the answer on the same thread, while the first cache still holds its own lock.

So two caches can end up holding the lock the other one needs. Like two people each standing in a doorway, waiting for the other door to open. Neither moves, ever.

It takes two normal calls on two threads while the caches are still cold:

• sport.Tournaments takes the sport lock, then waits for the tournament lock
• tournament.GetNameAsync() takes the tournament lock, then waits for the sport lock

For the client the feed simply goes quiet. The connection stays up, messages keep being acked, nothing is written to any log. Only a restart clears it.

This is the same bug that was fixed in the Java SDK in oddin-gg/javasdk#58. Its locks are synchronized, which is re-entrant, so the failure modes differ. CORE-4248.

What this changes

One rule, in the six caches that subscribe to API responses: never hold the semaphore across an API call. Each loader now fetches first, then takes the lock only to write.

Each observer is also wrapped in try/catch. Without it, one bad response from the server switches that cache off for the rest of the process, silently.

Trade-off

The cache check and the load are no longer one step. Previously the semaphore serialised cold loads, so N concurrent callers asking for the same missing item produced exactly one HTTP call. Now they produce N.

That is safe where a response is replaced, which is every cache but one. SportDataCache appends tournament ids, so a double load needed care: the collection is now a set, it is published copy-on-write so a handed-out instance is never mutated, and RefreshOrInsertItem now actually adds the id it was given — previously it created an empty set and never filled it, which left ISport.Tournaments empty on every call after the first.

Getting the coalescing back without reintroducing the deadlock needs an in-flight map (ConcurrentDictionary<key, Lazy<T>>) on top of this change. Follow-up, not this PR.

Behaviour that changes

No public type or signature changes. Three things a client could observe:

• sport.Tournaments used to return the list once and then empty on every later call, because the ids were never actually stored. It now returns them consistently, in the order the server sent, de-duplicated.
• A tournament whose id the server sends malformed used to surface as an ArgumentException out of sport.Tournaments regardless of ExceptionHandlingStrategy. It is now logged and skipped, so the list is shorter rather than the call failing.
• Tournaments with an empty id used to produce placeholder entries that threw on any property access, and could create a nameless sport. They are skipped.

Tests

The tests were pushed before the fix so CI recorded them failing first:

tests only	4 failed, 45 passed — the deadlock test hung the full 30 s
with the fix	0 failed, 49 passed, suite 30 s → 217 ms

Now 70 tests. Every fix in this PR was checked by reverting it alone and confirming a named test turns red.

• ConcurrentColdSportAndTournamentLoadsDoNotDeadlock — forces both loads to sit inside their API calls at the same moment, and asserts that overlap happened, so a green run cannot be empty. Uses dedicated threads, because on a failing run both block forever.
• CacheDoesNotHoldItsSemaphoreWhileTheApiCallIsInFlight — the rule itself, over all six caches, plus the async GetSports loader. No timing involved.
• SideLoadDeliveryTests — pins the contract the design rests on: RestClient delivering on the calling thread, and the two call sites that read the cache straight after their fetch.
• CacheObserverResilienceTests — for each of the six: a bad response does not kill the subscription, and one malformed item in a batch does not drop its siblings.
• SportDataCacheTournamentIdsTests — ids are added, deduplicated, and never mutated after publication.

Why not the javasdk approach

javasdk moved delivery onto a scheduler. That does not port cleanly, and it would not satisfy "no lock held across I/O" anyway — the lock would still be held across the HTTP call. It also breaks three things that rely on delivery being inline:

• MatchStatusCache.GetMatchStatus reads the cache right after fetching
• MatchCache.LoadFixture discards its result; only the observer writes it, and Match.FetchTournament reads the cache on the next line — this would silently revert the earlier fix that lets a match resolve its sport and tournament without a successful summary
• SportDataProvider.GetListOfMatches relies on the schedule side-load landing inline, otherwise up to 1000 extra calls

SideLoadDeliveryTests exists so that this cannot be broken silently later.

Not in this PR

• The other three caches. MarketDescriptionCache, MarketVoidReasonsCache and LocalizedStaticDataOfMatchStatusCache hold a lock across their own HTTP calls, the same shape this PR removes from the six. No cycle exists today — none of the six observers reads a market description, and Monitor is re-entrant so there is no self-deadlock either — so this is a follow-up, not a blocker.
• Semaphore(1,1) → lock. After this change no lock region spans I/O or an await, so the kernel semaphore now costs two syscalls per warm read and buys nothing, and it is not re-entrant. Worth converting, but it is a change of primitive rather than of where the lock is held, and it would invalidate the reflection-based test that pins this PR's invariant. Its own PR.
• winner_id (the .NET and Java feed models disagree — needs the schema owner), the disposal chain (Feed resolves no IDisposable), and the unbounded growth of SportDataCache entries, which are NotRemovable with no expiry.

🤖 Generated with Claude Code

[dsaiko]

Changes requested

• 5 unresolved finding(s) at high or above: i1, i6, i8, i9, i13
• 3 of 3 reviewer(s) completed every lens (quorum 2)

Blocking (5)

HIGH · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/SportDataCache.cs:271

RefreshOrInsertItem initializes TournamentIds but never adds the tournamentId

GetSportTournaments calls RefreshOrInsertItem(id, culture, tournamentId: tournamentId) once per tournament, but the only thing that overload does with tournamentId is localizedSport.TournamentIds ??= new HashSet<URN>() — it never adds the id. The sport is therefore left with an empty, non-null set. Sport.FetchTournaments (Sport.cs:74) reads sport.TournamentIds?.ToList() ?? _cache.GetSportTournaments(...), so the null-check that used to fall through to the API now sees an empty collection instead. The defect predates this PR, but the line is changed by it (List -> HashSet) and it directly contradicts the PR's stated goal of making ISport.Tournaments return a correct tournament list.

Suggested: Add the id in the tournamentId branch, e.g. if (tournamentId != null) (localizedSport.TournamentIds ??= new HashSet<URN>()).Add(tournamentId); — HashSet gives the same dedup guarantee the PR relies on in HandleTournamentData.

HIGH · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/Entities/Sport.cs:74 · reported by 2 reviewers

TournamentIds HashSet is copied outside the semaphore while observers Add to it in place

SportDataCache.HandleTournamentData (SportDataCache.cs:204) mutates LocalizedSport.TournamentIds in place via sportTournaments.Add(...) while holding the sport semaphore. Sport.FetchTournaments reads the same instance with no lock: GetSport -> Read(id) releases the semaphore before returning the entity, and only then does line 74 run sport.TournamentIds?.ToList(). The added comment claims the copy makes this safe, but the copy is the racing read. Enumerable.ToList on an ICollection<T> goes through new List<URN>(source), which reads Count, allocates URN[count], then calls HashSet<URN>.CopyTo(array, 0) — CopyTo copies the set's current _count, not the count that sized the array. This is the one collection in the six caches still mutated in place; MatchCache.ExtraInfo and TournamentsCache.CompetitorIds are both swapped copy-on-write for exactly this reason. Both writers are on ordinary paths (TournamentInfoModel from every cold TournamentsCache.GetTournament, and TournamentScheduleModel), and this PR deliberately raises the append rate, so the window widens rather than narrows.

Suggested: Make the publication copy-on-write in SportDataCache.HandleTournamentData so no published instance is ever mutated, matching the MatchCache.ExtraInfo / TournamentsCache.CompetitorIds idiom: build the new set under the semaphore and swap the reference — var updated = sport.TournamentIds is null ? new HashSet<URN>() : new HashSet<URN>(sport.TournamentIds); updated.Add(tournamentId); sport.TournamentIds = updated;. Then Sport.cs:74 can drop its .ToList() and its comment, since the reference it reads is immutable after publication. The alternative is to keep the mutation and have SportDataCache expose a snapshot taken under the semaphore (e.g. an ISportDataCache.GetSportTournamentIds(URN) that copies inside the lock); either way the snapshot must not be taken by the caller outside the lock.

HIGH · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/Entities/Sport.cs:74

TournamentIds HashSet enumerated without the SportDataCache lock

Sport.FetchTournaments does sport.TournamentIds?.ToList() on a LocalizedSport obtained via SportDataCache.Read, which releases the SportDataCache semaphore before returning. SportDataCache.HandleTournamentData (under that same semaphore) mutates the HashSet via sportTournaments.Add(tournamentId) and reassigns sport.TournamentIds (lines 200-205 of SportDataCache.cs). Because HashSet<T> is not thread-safe, concurrent enumeration and Add can throw InvalidOperationException or return a torn view. The PR added the .ToList() to copy on reassignment, but did not address that Add under the lock vs. enumeration after lock release still races. The read path is reached whenever a client enumerates Sport.Tournaments after an API side-load has begun firing.

Suggested: Either snapshot the HashSet under the SportDataCache semaphore (e.g. expose a ReadTournamentIds(URN) that copies inside the lock) or change HandleTournamentData to swap a fresh immutable snapshot (ConcurrentDictionary<URN,byte> or copy-on-write ImmutableHashSet/ImmutableArray) so readers never enumerate a HashSet that another thread is mutating.

The panel disagreed about this one.

HIGH · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/SportDataCache.cs:203 · reported by 2 reviewers

TournamentIds dedup fix ships with no test

Commit 1664a71 changed LocalizedSport.TournamentIds from List<URN> to HashSet<URN> to fix client-visible duplicates: HandleTournamentData appends on every TournamentInfoModel/TournamentScheduleModel response, the sport entry is NotRemovable with no expiry, and ISport.Tournaments therefore handed the same tournament back once per repeated side-load while the collection grew unboundedly. No test in the branch touches TournamentIds. Every new test would stay green if the field went back to a List, or if a future refactor reintroduced an ordered collection, so the fix for the round-1 finding has no regression guard at all.

Suggested: Add a SportDataCache test using the existing PublishOnlyApiClientProxy pattern: publish the same TournamentInfoModel (tournament od:tournament:1, sport od:sport:1) twice with culture "en", then assert (await cache.GetSport(new URN("od:sport:1"), new[]{Culture})).TournamentIds.Count == 1 and that it contains that id. After the first publish Name["en"] is set, so LoadedLocals covers "en" and GetSport makes no API call — the assertion is deterministic.

HIGH · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk.Tests/API/CacheSideLoadDeadlockTests.cs:39 · reported by 2 reviewers

Dedup invariant in SportDataCache has no test

The1664a71 fix changed SportDataCache.HandleTournamentData to use HashSet<URN> for TournamentIds, with the explicit rationale that two concurrent cold loads would otherwise put the same tournament id in the list twice and ISport.Tournaments would hand the client the same tournament back N times. The only existing concurrent-load test (ConcurrentColdSportAndTournamentLoadsDoNotDeadlock) asserts the loads finish and overlap, but never asserts the resulting ISport.Tournaments collection contains each id exactly once. A future regression that drops the HashSet (e.g. someone re-introducing a List<T> to make a different test pass) would not be caught.

Suggested: Add a test that triggers two concurrent cold loads of the same sport/tournament and asserts ISport.Tournaments returns each distinct id exactly once. The existing test scaffolding (Worker + SideLoadApiClientProxy) is sufficient; the gap is the assertion, not the apparatus.

The panel disagreed about this one.

Other findings (5)

MEDIUM · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/TournamentsCache.cs:110

URN parse sits outside the per-item try, so one bad id drops the rest of the batch

Commit 1664a71 added per-item catches to HandleTournamentData, HandlePlayersData and HandleTeamData because the new response-level catch would otherwise discard the remainder of a batch on a single malformed item, and states TournamentsCache already matched that shape. It does not: new URN(tournament.id) at line 110 is evaluated before the try block, so an ArgumentException from a malformed server id escapes HandleTournamentsData, is caught only by the new response-level handler in the subscription, and every tournament after the bad one in that response is never cached. The three sibling caches now behave correctly here; this one does not.

Suggested: Move the URN construction inside the existing try, matching SportDataCache.HandleTournamentData / PlayerCache.HandlePlayersData / CompetitorCache.HandleTeamData, and include the raw id in the log message.

MEDIUM · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/SportDataCache.cs:119

GetSportTournaments returns a deferred Select; URN parsing bypasses its own per-item catch

Commit 1664a71 claims both deferred projections were 'materialized inside the lock'. GetSports got .ToList(); GetSportTournaments did not — line 119 is still a lazy Select. Two consequences. First, the try/catch inside the foreach at lines 126-133 exists to log-and-continue on a bad tournament, but the most likely thrower, new URN(t.id), runs in the enumerator's MoveNext at line 124, outside that try, so a single malformed id aborts the entire insert loop and propagates out of GetSportTournaments into Sport.FetchTournaments / ISport.Tournaments as an unhandled ArgumentException regardless of ExceptionHandlingStrategy. Second, the returned sequence is enumerated again by the caller, re-running URN construction for every element and keeping the whole TournamentsModel response alive.

Suggested: Materialize the projection per item inside the loop's try (parse the id and insert in one guarded step), and return a materialized List<URN> so the caller does not re-run URN construction over the live response.

MEDIUM · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/PlayerCache.cs:166

Per-item catches added for batch survival are untested

Commit 1664a71 added per-item try/catch to HandlePlayersData (PlayerCache.cs:166), HandleTeamData (CompetitorCache.cs:242) and HandleTournamentData (SportDataCache.cs:192) specifically so the new response-level catch would not drop a whole batch because of one malformed id. CacheObserverResilienceTests only covers the response-level guard, using players = null — a payload with no valid items at all. Nothing asserts that a batch containing one bad item still caches the good ones, so removing any of the three per-item catches leaves the suite green while a single malformed id silently discards every sibling in the response.

Suggested: Extend CacheObserverResilienceTests: publish a competitorProfileEndpoint whose players are [{id="bogus"},{id="od:player:1"}] and assert GetPlayer(od:player:1) is non-null; mirror it for CompetitorCache (teams) and SportDataCache (TournamentScheduleModel with one unparseable tournament key). All three fail if the per-item catch is removed, because the response-level catch aborts the loop.

MEDIUM · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk/API/MatchStatusCache.cs:37

Observer guard tested for one of six caches; MatchStatusCache's reachable throw is uncovered

Commit 8965106 added an identical observer guard to all six caches, but CacheObserverResilienceTests exercises only PlayerCache. MatchStatusCache is the highest-value miss and is trivially reachable: HandleResponse passes summary.sport_event_status to RefreshOrInsertApiItem, which dereferences summary.status (MatchStatusCache.cs:145) and NREs when the endpoint returns a summary with no status. The deadlock suite's own MatchSummary() fixture (CacheSideLoadDeadlockTests.cs:284) is exactly that payload, so the case occurs in the branch already but is never asserted. Before the guard this exception escaped OnNext, permanently disposed the subscription and rethrew into the GetMatchSummary caller; if the guard is later removed from MatchStatusCache, SportDataCache, TournamentsCache, MatchCache or CompetitorCache, nothing fails.

Suggested: Add a case per remaining cache to CacheObserverResilienceTests using the PublishOnlyApiClientProxy: for MatchStatusCache publish a MatchSummaryModel with sport_event_status = null, then one with a valid status, and assert no exception reached the publisher and GetMatchStatus returns the second status. Use TournamentInfoModel with a null tournament for SportDataCache/TournamentsCache and a FixturesEndpointModel with a null fixture for MatchCache/CompetitorCache.

LOW · src/Oddin.OddsFeedSdk/Oddin.OddsFeedSdk.Tests/API/CacheSideLoadDeadlockTests.cs:105

SportData semaphore theory omits its async loader

The SportData case exercises only GetSportTournaments. It never calls GetSports or GetSport, whose shared async LoadAndCacheItem path was separately changed in 8965106 to release the semaphore across IApiClient.GetSports. The test named for the cache-wide invariant therefore stays green if that async path again holds the semaphore across I/O.

Suggested: Add an async SportData case that pauses GetSports while in flight, probes the semaphore, completes the response, and asserts that the returned sport data was written.

---

Reviewed by claude, codex, minimax-ollama over pull request # 66.

🤖 Reviewed by AI panel · claude, codex, minimax-ollama · run 20260910-203814