fix(amqp): resolve match sport and tournament without a successful summary fetch

[hrubymar10]

Summary

Resolves both halves of #57 — IMatch.SportId and IMatch.Tournament returning null after a failed match-summary fetch.

Supersedes the approach in #62 (closed as superseded), which fixed only the SportId half.

• SportId — FeedMessageMapper.MapSportEvent now forwards the routing-key sport URN to BuildMatch (the signature already accepted it, and the tournament branch already did this). Match._localSportId is populated, so SportId resolves with no cache work and without touching the process-wide semaphore in MatchCache.GetMatch on the hot feed path.
• Tournament — the tournament URN is not in the routing key (TopicParsingHelper parses sport at index 4 and event at index 6, nothing tournament-shaped), so it can only come from the API. Match.FetchTournament now probes the match cache first and, only when no cache item exists at all, loads the fixture via a new internal IMatchCache.LoadFixture — a direct IApiClient.GetFixture call whose response populates MatchCache through the API client's existing response subscription — before re-reading. The sport is then resolved as routing-key sport ?? cached match sport, so the fallback repairs all construction paths, including matches built without a routing-key sport (SportDataProvider.GetMatch/GetMatches, SportDataBuilder.BuildMatches). So a summary failure no longer leaves Tournament null when the fixture endpoint answers, and await match.Tournament.GetSportAsync() stops throwing.
• Why not route the fallback through FixtureCache — FixtureCache.GetFixture short-circuits on its own cache hit and only its API path publishes into MatchCache. The two caches are independent MemoryCache instances with separate expiry and memory-pressure trimming, so warm-fixture + evicted-match would silently turn the fallback into a no-op. The direct API call is immune to that divergence; traffic stays bounded because it only fires after a MatchCache probe miss.
• Partial-update data loss — see the next section. This started as a guard for the fallback and turned out to be a pre-existing bug worth fixing at the root.
• Sport-conflict warning — the routing-key sport unconditionally outranks the API value, so a mismatch is logged with both values instead of resolved silently. The check lives in the shared FetchMatch helper (it cannot live in FetchSportId: when the routing-key sport is set, FetchSportId never touches the API value — fetching there just to compare would reinstate the hot-path summary call this PR removes), fires on any property read that loads the cached match, and logs once per Match instance.

RefreshOrInsertItem no longer destroys data on partial payloads

The update branch (MatchCache.cs) assigned every field unconditionally, so any payload omitting a field erased the cached value. Triggering a fixture fetch from Tournament made that materially more reachable: GetMatch releases its semaphore before the HTTP call, so a concurrent summary/schedule response can populate the same match during the window, and our fixture response then arrives to find an existing entry and takes the update branch. A blanked Competitors is a NullReferenceException in Match.Competitors.

Guarding on "no cache item" before the fetch is not sufficient, because the branch is chosen after the fetch. So:

• absent reference fields (RefId, SportId, TournamentId, Competitors, ExtraInfo) are preserved; an explicitly empty collection still clears, so absent and empty stay distinguishable
• ScheduledTime/ScheduledEndTime are written only under their Specified flags
• SportFormat is written only when the sport_format key is actually present, and LiveOddsAvailability only when liveodds is non-empty — these are non-nullable, so a partial payload previously flipped Race → Classic and NOT_AVAILABLE → AVAILABLE silently
• insert-branch defaults are unchanged
• Match.Competitors and GetHomeAwayCompetitor are null-safe; a null collection is treated as "fewer than two competitors" and follows the existing ArgumentException/CATCH-returns-null contract rather than introducing a new exception type
• HandleMatchData skips null ids instead of passing them to RefreshOrInsertItem, which dereferences them

Behaviour and compatibility

No public API changes — Match, IMatchCache, IFixtureCache and ISportDataBuilder are all internal.

Scenario	Before	After
Summary OK	unchanged	unchanged
Summary OK, tournament id but no sport id	Tournament null	Tournament resolves
Summary fails, fixture answers	SportId null, Tournament null	both resolve — all construction paths, including REST-built matches without a routing-key sport
Summary fails, fixture fails, CATCH	null	null (identical)
Summary fails, fixture fails, THROW, feed-built match	ItemNotFoundException("Unable to fetch match")	identical, message and id pinned by test
Summary fails, fixture fails, THROW, REST-built match	ItemNotFoundException("Cannot load sport")	ItemNotFoundException("Unable to fetch match") — the failure is now attributed to the missing match rather than the sport derived from it
Cached item without tournament id	"Cannot load tournament" under THROW	identical, no fixture call
Partial payload for a cached match	fields blanked	fields preserved
Payload carries an unrecognised sport_format	ArgumentException out of OnNext: on the summary path nothing cached for that culture, on the fixture path every cache subscribing after MatchCache starved of the whole response	warning logged; record caches. Fresh insert reports SportFormat.Unknown, an update preserves the cached known value. HomeCompetitor/AwayCompetitor then return null (or throw under THROW) via the existing IsClassic() gate, naming unknown
Malformed event id inside a multi-event schedule payload	the whole batch abandoned and later subscribers starved	only the offending event skipped and logged with its raw id; siblings cached
Payload carries an explicitly empty extra_info	cached dictionary cleared (except sport_format)	cached dictionary preserved — deliberate divergence from Competitors, where empty still clears

Cost: reading Tournament can now reach the fixture endpoint, which it previously never did. On the failure path that is 1 summary attempt plus 1 fixture attempt per access (the post-fixture re-probe is a non-loading PeekMatch, so the summary is not retried within the same access) versus 1 summary today — and today's failure path already re-attempts the REST call on every access, unbounded, because LoadAndCacheItem swallows summary errors and caches nothing. Once the fixture answers, the cache populates and further accesses cost nothing, a net reduction. Only in a full API outage is this more traffic, and then by exactly one fixture call per access.

Review items from #62 adopted and declined

Adopted from @dsaiko's review: scope claims corrected, IMatch-level tests added, sport-conflict visibility, #57 kept open.

Declined — inverting FetchSportId precedence to prefer the API value. It is the compat-safer option in isolation, but it reinstates the per-access REST retry on the failure path and, under ExceptionHandlingStrategy.THROW, FetchMatch throws before the routing-key fallback is reachable — defeating the fix for THROW users entirely. The underlying concern is addressed by the warning log instead.

Corrected — the claim that THROW diagnosis regresses (exception flipping from "Cannot load sport" to "Cannot load tournament"). Neither message is reachable when the summary fails: FetchMatch throws "Unable to fetch match" first, identically before and after. The other two only fire when a cache item exists with that one field null. A test pins this.

Review round 2 — items from @stetinatomas-oddin's review, all adopted

• CRITICAL, fallback unreachable for REST-built matches — fixed by restructuring FetchTournament: probe → fixture load on miss → re-probe → resolve sport from routing key or cached match. Regression test added.
• MAJOR, warm FixtureCache no-ops the fallback — fixed by bypassing FixtureCache entirely (IMatchCache.LoadFixture, per the review's first suggested option). End-to-end regression test: pre-warmed real FixtureCache, real MatchCache, publishing API-client proxy.
• MAJOR, warning placement — moved to the shared FetchMatch helper with once-per-instance dedup (see Summary for why it cannot live in FetchSportId literally).
• MAJOR, scripted-fake test — all fallback tests now run against a real MatchCache fed by a publishing DispatchProxy API client; the scripted sequence fake is no longer used for fallback claims.
• MAJOR, logger init race — test logger initialization moved to a [ModuleInitializer], safe under xunit parallel collections.
• MAJOR, mapper change had zero coverage — FeedMessageMapperTests and TopicParsingHelperTests restored from fix(amqp): pass sport URN from routing key when building match events #62 and extended with throwing inputs (wrong section count, non-numeric sport section) plus the odds_change no-sport case.
• MINOR ×3 — the fixture extra_info bridge no longer mutates the shared response payload (the derived value is passed alongside instead; regression test asserts the payload is untouched); duplicate extra_info keys are last-wins with a warning (null keys skipped) instead of throwing; System.Reactive and Microsoft.Extensions.Logging.Abstractions are now explicit test dependencies.
• NIT ×3 — LoadedLocals is a maintained set instead of a per-read LINQ Except; fromFixture is passed by name at call sites; the CoverageBootstrapTest stub is deleted (the suite now carries real tests).
• Unanchored — rebased onto main tip, so the previously byte-identical ci.yml/csproj-registration/bootstrap files are out of the diff; fix(amqp): pass sport URN from routing key when building match events #62 wording corrected above.

Council rounds (ragnar-cr runs 5–7): F011 — fixture updates no longer demote summary-loaded cultures (regression test pins summary→fixture→access with no refetch); F009 — the write-only FixtureOnlyLocals set is removed; F019 — SportFormat is derived from the same last-wins extra_info value as ExtraInfo, so duplicates cannot produce divergent state and an invalid-then-valid duplicate no longer aborts the update; F022 — the post-fixture re-probe uses a non-loading PeekMatch, so the fallback no longer fires a second synchronous summary attempt under the process-wide semaphore.

Review round 3 — items from @stetinatomas-oddin's review, all adopted

• MAJOR, unrecognised sport_format threw into the Rx dispatch — RefreshOrInsertItem threw on a sport_format value this SDK version does not recognise, and the two MatchCache subscription arms catch nothing, so the exception escaped OnNext and starved every cache subscribed after MatchCache (the extra_info bridge made this reachable for fixture payloads for the first time). An unknown value now logs a warning and maps to SportFormat.Unknown, left ungated (hasSportFormat stays false) so a fresh insert reports it honestly while an update preserves a previously cached known value rather than demoting it. As defence in depth for the same class — refid/sport/tournament/competitor URNs all throw on malformed input — HandleMatchData now wraps each item in try/catch, so one bad event cannot drop its siblings or the response's later subscribers.
• MINOR, SportId readers never saw the sport-conflict warning — it is now reachable from FetchSportId via a non-loading PeekMatch behind a guarded Func overload (cheap guards run before any lookup), so a consumer reading only SportId — the property the precedence actually governs — sees the conflict without a summary fetch on the hot path.
• MINOR, ExtraInfo was whole-dictionary replace — updates now merge: present keys win, omitted keys are preserved, matching the partial-update rule the other nine fields already follow. This subsumes and removes the sport_format lockstep carve-out. The merge is copy-on-write (a new dictionary is built and the reference swapped), so IMatch.ExtraInfo keeps the snapshot semantics every sibling field already has — a reference a consumer already read is never mutated underneath it. One deliberate consequence, stated rather than buried: an explicitly empty extra_info now preserves rather than clears — unlike Competitors, an empty bag from a subset-carrying endpoint is not a credible "server cleared everything" signal.
• Included beyond the review's explicit request — FixtureCache.LoadAndCacheItem built its extra_info with a bare ToDictionary, so the duplicate/null-key payload MatchCache was just taught to tolerate still threw out of FixtureCache into the consumer (its catch wraps only the API call). Both caches now share one tolerant ExtraInfoHelper.ToExtraInfoDictionary. The reviewer flagged this as not-a-change-request; it is included because this PR is what created the inconsistency.
• NIT — the gratuitous SportDataBuilder.cs re-indentation is fully reverted, including the unmentioned trailing-newline change, so the file leaves the diff entirely.
• Not widened — FixtureCache.LoadAndCacheItem's tv_channels and other bare projections are untouched; only the extra_info builder was shared.

extra_info from fixture payloads was silently discarded

fixture re-declares extra_info with its own backing field, hiding sportEvent.extra_info (FixturesEndpointModel.cs:130 — the compiler has been emitting CS0108 for this all along). RefreshOrInsertItem takes a sportEvent, so it read the base slot and saw null for every fixture payload: extra_info, and therefore sport_format, has never arrived through the fixture endpoint.

The two declarations differ by more than storage — the base carries [XmlArrayItem(IsNullable = true)] and the derived IsNullable = false — so this originates in the XSD rather than being a generator accident. Fixed at the boundary in non-generated code instead: the FixturesEndpointModel arm of the MatchCache subscription passes the derived extra_info alongside the payload into RefreshOrInsertItem, without mutating the shared response object that other subscribers see.

Consequence beyond the bug fix: fixture responses can now update a cached match's ExtraInfo and SportFormat with the data they actually carry, which includes the Match.Fixture path and not only the new fallback. The preservation rules above keep that safe — absent values preserve cached state, and an omitted sport_format entry is carried forward.

Fixture-derived entries do not suppress summary retries

Caching a fixture-derived entry created a second-order problem: RefreshOrInsertItem always writes Name[culture], LoadedLocals derives from loaded cultures, and GetMatch only loads cultures.Except(LoadedLocals). So once the fallback populated a culture, the summary was never retried for it — a transient summary failure pinned the match to fixture-derived data for the cache TTL, leaving fields the fixture does not carry absent even after the summary endpoint recovered. Before this change a failed summary retried on every access and self-healed.

LocalizedMatch tracks a single set of loaded cultures (LoadedLocalSet); MarkCultureLoaded adds a culture only when the response did not come from a fixture, so a fixture-borne culture is simply never marked loaded and stays eligible for summary retry, while a culture already loaded from a summary or schedule is never demoted by a later fixture update. Tournament therefore resolves immediately from the fixture while the summary keeps being retried until it succeeds, at which point the culture is marked loaded and retries stop naturally.

This deliberately restores per-access summary retry while the summary endpoint is unavailable — the pre-existing behaviour. Automatic recovery is worth more than the saved calls.

Tests

45 passing (was 21 at first review; 37 before round 2). IMatch-level rather than against a capturing builder. New in round 2: unrecognised-sport_format-does-not-throw-and-still-reaches-a-later-subscriber, unknown-sport_format-preserves-a-cached-known-value, malformed-id-in-a-multi-event-schedule-does-not-drop-siblings, the SportId-path conflict warning, ExtraInfo merge (payload-key-wins, omitted-key-survives, empty-preserves, and a previously-read reference not mutated by a later update), and FixtureCache duplicate/null-key tolerance. New since the first review: REST-built (localSportId: null) fallback regression, warm-FixtureCache end-to-end regression, payload-mutation regression, duplicate-extra_info-key handling, restored mapper/topic-parser coverage with throwing inputs, and the fallback tests now run against a real MatchCache fed by a publishing API-client proxy instead of a scripted cache fake. Retained from round 1: routing-key SportId with zero cache reads, both-endpoints-fail with the exact exception asserted, the hot-path skip, cached-item-without-tournament-id, tournament-without-summary-sport, conflict logging, the non-reentrant-semaphore test, and the deterministic partial-update coverage asserting all nine vulnerable fields survive while Name did update.

Also adds InternalsVisibleTo for the test assembly — main had the test project but a solution build compiled no test code at all.

Validation

45/45 tests pass on 501302a, verified locally, and each round-3 fix was mutation-tested — reverting it fails exactly the test that pins it. Branch rebased onto main tip (9c536ba), single commit.

Note this repo does not build warning-free and did not before this change: pre-existing CS8632, CS0108, CS0618, and an NU1903 advisory for Microsoft.Extensions.Caching.Memory 6.0.1. None originate here and none are addressed here.

---

Prepared by Claude (AI) on behalf of @hrubymar10.

[stetinatomas-oddin]

Verified the branch locally: 21/21 green, and the partial-update work is a real root-cause fix rather than a workaround. Two findings gate the merge for me.

Headline concerns

• Match.FetchTournament returns on a null SportId before the probe, so the new fixture fallback never runs for any Match built without a routing-key sport — SportDataProvider.GetMatch/GetMatches and SportDataBuilder.BuildMatches. The compatibility table claims otherwise for those callers.
• The fallback goes through FixtureCache, which short-circuits on a cache hit, and only its API path publishes the response that populates MatchCache. A warm fixture entry silently turns the fix back into the old null behaviour.

I confirmed both by running them rather than by reading — details inline.

Lenses run: correctness/invariants · concurrency/race · domain-correctness · design-coherence · test-coverage · maintainability. → 1 CRITICAL + 5 MAJOR + 3 MINOR + 3 NIT inline, plus the two notes below.

Without a file anchor

• The branch predates main's tip, so the diff shows ci.yml, Oddin.OddsFeedSdk.Tests.csproj and CoverageBootstrapTest.cs as changes even though all three are byte-identical to main and merge as no-ops. A rebase makes the diff match what actually lands — 8 files rather than 11.
• The description says "#62 stays open for now", but #62 is closed as superseded.

I also went through this manually alongside the structured pass and have no findings beyond the ones here.

Worth calling out as good: chasing the partial-update data loss to its root instead of guarding around it, and PartialFixtureUpdatePreservesCompleteCachedMatchFieldsAndUpdatesName asserting that Name did update — that is what stops a preservation test from passing vacuously. Documenting the declined review items with reasons, including the corrected THROW-diagnosis claim, made this much faster to review.

Co-Authored-By: Claude Opus 5 (1M context)

[hrubymar10]

@stetinatomas-oddin all 12 inline findings and both unanchored notes are addressed in 2d2d677 — replies on each thread. The two headline items: the fallback is now reachable for REST-built matches (FetchTournament probes before resolving the sport, which it takes from the routing key or the freshly cached match), and it bypasses FixtureCache entirely via a direct API load on MatchCache, so the warm-cache divergence can't no-op it.

On the unanchored notes: branch rebased onto main tip (9c536ba), so ci.yml, the csproj registration and the bootstrap stub are out of the diff (the stub is now an actual deletion); description corrected — #62 is closed as superseded — and updated for the restructured fallback, including one intentional compat-table addition: REST-built matches under THROW now fail with "Unable to fetch match" instead of "Cannot load sport" when both endpoints fail.

35/35 tests pass (was 21), verified in two independent environments. Single commit, amended.

Prepared by Claude (AI) on behalf of @hrubymar10.

[stetinatomas-oddin]

All 12 inline findings and both headline items from my previous review verified against 0d3d3e3 by reading the code, not the replies — everything is genuinely fixed. Note the replies cite 2d2d677, which the later force-push orphaned; the 63/28 delta that landed after them (the PeekMatch re-probe and the LoadedLocals redesign) is audited here too. 37/37 pass locally, and I mutation-tested six of the fixes to confirm the tests actually lock them in rather than passing vacuously.

Highlights:

• CRITICAL, fallback unreachable for REST-built matches — FetchTournament now probes, loads the fixture on a miss, re-probes, and only then resolves _localSportId ?? match?.SportId. Reinstating the early return fails TournamentFallbackWorksForMatchWithoutRoutingKeySport.
• MAJOR, warm FixtureCache no-ops the fallback — IMatchCache.LoadFixture goes straight to IApiClient.GetFixture, and the end-to-end test with a pre-warmed real FixtureCache plus a real MatchCache asserts the second API call. The divergence I demonstrated is closed.
• MAJOR, scripted-fake test — every fallback claim now runs against a real MatchCache behind a publishing DispatchProxy, and both cases I asked for are present.
• MAJOR, mapper coverage — restored and extended. One correction to the reply: reverting the mapper line fails two tests, not four; the two "-"-routing-key cases pass either way because the sport is null on both sides. The coverage concern is satisfied regardless.
• MAJOR, logger init — [ModuleInitializer] is the right shape; the uninitialized-factory throw is no longer ordering-dependent.
• PeekMatch re-probe (F022) — good catch on your side, and pinned: swapping it back to GetMatch fails four tests on the summary-call count.

Four new findings from auditing the delta, one MAJOR and three smaller, all inline except the last. None is a regression — the MAJOR is a pre-existing throw whose reach this PR extends, so it is a small hardening rather than a rework.

Without a file anchor — the description contradicts itself. "Fixture-derived entries do not suppress summary retries" still describes two maintained sets ("fixture responses add the culture to the fixture-only set, summary and schedule responses move it to the loaded set"), but F009 removed FixtureOnlyLocals and MarkCultureLoaded now simply skips fixture-borne cultures. The council-rounds bullet above it says as much, so the two sections disagree. Same section: the Match no longer takes IFixtureCache line describes a delta against the revision I first reviewed, not against main — main's Match never took it, so a future reader diffing against main will not find that change.

One observation explicitly not a change request, since the file is outside this diff: FixtureCache.LoadAndCacheItem builds its ExtraInfo with a bare extra_info?.ToDictionary(...), so it still throws on the duplicate and null keys you just made MatchCache tolerate — and its try/catch only wraps the API call, so that one reaches the consumer. Worth knowing about, not worth widening this PR for.

Worth calling out as good: the partial-update work held up under mutation testing on every field I checked, SuccessfulSummaryAfterFixtureFallbackStopsFurtherRetries is exactly the test that stops the retry semantics from silently regressing, and the deliberate choice to keep per-access summary retry — with the reasoning written into the code as a comment rather than left in the PR body — is the right call and the right place for it.

Co-Authored-By: Claude Opus 5 (1M context)

[stetinatomas-oddin]

Round 3 verified against 501302a. All four findings from my previous review are genuinely fixed, plus both unanchored notes — I read the code rather than the replies, ran the suite (45/45), and re-ran your mutation table myself: reinstating the throw, ungating Unknown, removing the per-item try/catch, reverting merge to replace, reverting copy-on-write to in-place, dropping the FetchSportId peek, and reverting FixtureCache to the bare ToDictionary each fail exactly the test that pins them. No regressions in the delta.

Taking both routes on the sport_format throw was the better call than either alone, and the reasoning is right: the unguarded new URN(...) sites made it a class rather than an instance, and per-item rather than per-batch isolation is what keeps a malformed sibling from dropping the rest — MalformedIdInScheduleBatchDoesNotDropSiblingMatches is the test that actually distinguishes the two. Gating Unknown so an insert reports it honestly while an update refuses to demote a cached known value is a nicer answer than either option I offered. And PreviouslyReturnedExtraInfoReferenceIsNotMutatedByLaterUpdate catches a real defect that my note only indirectly pointed at — the in-place merge stripping the snapshot semantics every sibling field keeps was worth finding.

Approving. Two MINOR notes inline, both consequences of this round's fixes rather than new ground, and neither gates the merge — land them here or don't, your call.

Two smaller things not worth an anchor: FixtureCache._log is declared as SdkLoggerFactory.GetLogger(typeof(TournamentsCache)), a pre-existing copy-paste, so the new duplicate-key warning will surface under the TournamentsCache category — the file is already in the diff, so it is a one-line fix if you want it. And the preservation bullet in the description still lists ExtraInfo under "an explicitly empty collection still clears", which now contradicts both the new merge bullet and the compatibility-table row you added.

Co-Authored-By: Claude Opus 5 (1M context)

[stetinatomas-oddin]

[MINOR] — The comment claims merge "keeps ExtraInfo[sport_format] aligned with the SportFormat gate above without a special case". That holds when the key is absent, but not when the value is present and unrecognised — which is the case the gate exists for. I confirmed it: a match cached from a summary as race, then updated by a fixture carrying sport_format = "future_value", ends up with SportFormat == Race and ExtraInfo["sport_format"] == "future_value". F019's "duplicates cannot produce divergent state" no longer holds in general either.

Raw passthrough in the dictionary is a defensible choice — I am not asking you to reinstate the carve-out. The two things worth fixing are that the comment currently asserts an invariant the code does not hold, and that UnknownSportFormatUpdatePreservesCachedKnownSportFormat asserts only the typed field, so whichever behaviour you intend is unpinned and free to flip.

Co-Authored-By: Claude Opus 5 (1M context)

[stetinatomas-oddin]

[MINOR] — The peek runs on every SportId read for the whole life of the instance, not only while a comparison is impossible: 100 reads produce 100 PeekMatch calls both when nothing is cached and when the cached sport agrees, because it disarms only once a conflict is actually logged. Scale first, so this is not overstated — the feed builds a Match per message, so in practice it is roughly one extra MemoryCache.Get per message; the unbounded case is a consumer holding one instance and polling it.

Your objection to a one-shot flag was that it would disarm before anything has cached the match. Disarming after the first comparison that actually saw a non-null match.SportId avoids that and bounds the cost — nothing further is learned once the two have been compared.

Also worth a line: SportIdUsesRoutingKeyWithoutReadingMatchCache no longer verifies what its name claims, since SequenceMatchCache.PeekMatch does not advance the counter it asserts on. A peek-count assertion would keep it honest.

Co-Authored-By: Claude Opus 5 (1M context)