Auto-backfill IGDB metadata after Steam import (#132) - #137
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf1cc08ed7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Re-read the game fresh on this context, filtered to still-pending so a | ||
| // manual/concurrent IgdbId assignment made after the sweep read is | ||
| // honoured (no concurrency token, so check right before persisting). | ||
| var game = await _db.Games.FirstOrDefaultAsync(g => g.Id == gameId && g.IgdbId == null, ct); |
There was a problem hiding this comment.
When a user edits a still-pending game while the IGDB search or cover download is in flight, this tracked instance still contains the pre-edit null values. Apply then fills those stale fields and SaveChangesAsync overwrites the user's newer values, including any concurrently assigned IgdbId; the null predicate here runs before both outbound operations rather than immediately before persistence. Reload the row or add an appropriate concurrency check before applying the backfill.
Useful? React with 👍 / 👎.
| using var timer = new PeriodicTimer(_options.CurrentValue.Interval, _clock); | ||
| while (await timer.WaitForNextTickAsync(ct)) | ||
| { | ||
| await SweepOnceAsync(ct); |
There was a problem hiding this comment.
With the default configuration, the explicit delay at line 87 waits one hour, then this newly created PeriodicTimer waits another full interval before yielding its first tick. Consequently the first backfill runs two hours after startup rather than after the documented single Interval, delaying metadata after an import; run a sweep after the initial delay or construct/use the timer without the additional wait.
Useful? React with 👍 / 👎.
| string? coverPath = null; | ||
| if (!string.IsNullOrWhiteSpace(match.Result.ImageUrl)) | ||
| coverPath = await _covers.EnsureLocalAsync(match.Result.ImageUrl, ct); |
There was a problem hiding this comment.
For a Steam-imported or manually edited game whose ImagePath is already populated, this still downloads and persists the IGDB cover, but Apply immediately discards coverPath because it is fill-only. This creates an unreferenced CoverImages row for every such match and needlessly performs the download; those rows remain until the garbage collector runs on a later application restart. Only call EnsureLocalAsync when the existing image path is blank.
Useful? React with 👍 / 👎.
| else | ||
| { | ||
| // Confident "no match" — leave for manual resolution. Not a | ||
| // throttle signal, so leave consecutiveEmpty untouched. | ||
| } |
There was a problem hiding this comment.
When IGDB returns candidates but none is an exact confident match, this branch leaves consecutiveEmpty unchanged. Thus empty responses separated by successful nonempty provider responses are counted as consecutive, and an alternating sequence can eventually hit EmptyResultAbortThreshold and abort the sweep even though IGDB is responding normally. Reset the streak whenever the provider returned a nonempty result, regardless of whether the planner selected a match.
Useful? React with 👍 / 👎.
- First sweep now fires after one Interval (PeriodicTimer first tick) not two - Skip cover download when ImagePath is already set (avoids orphan CoverImages row) - Reset empty-result abort streak on any non-empty provider response - Re-check 'still pending' against the DB immediately before save so a concurrent manual IgdbId assignment is never overwritten; detach + skip if it is - Make ProviderConfigured() synchronous (was async-no-await, CS1998) - Tests: 3 new (empty-streak reset, cover-skip, concurrent-assignment guard)
|
@codex Please re-review at the new head (
Pushed commits on
Verification (run locally, not by the sandboxed reviewers):
Thank you for the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0061adb91b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var stillPending = await _db.Games | ||
| .AsNoTracking() | ||
| .AnyAsync(g => g.Id == gameId && g.IgdbId == null, ct); |
There was a problem hiding this comment.
When a user fills Description, Year, Developer, or another previously null field during the outbound work but leaves IgdbId null, this predicate still succeeds and the stale tracked entity subsequently overwrites the user's value. There is also a smaller TOCTOU window in which an IgdbId assigned after this query but before SaveChangesAsync is overwritten. The fresh evidence at 0061adb is that the fix adds only an AnyAsync check for IgdbId, without reloading the fill-only fields or making the predicate part of the update; reload and recompute the merge, then persist with an atomic concurrency condition.
Useful? React with 👍 / 👎.
| await runner.RunSweepAsync(ct, (int)(_sweepOffset % int.MaxValue)); | ||
| _sweepOffset += _options.CurrentValue.MaxGamesPerSweep; |
There was a problem hiding this comment.
When a sweep exits at EmptyResultAbortThreshold, advancing by the full configured cap skips the unattempted remainder of that window. For example, with 200 pending games, the default cap of 100, and the first 10 games in each half returning empty results, successive offsets alternate between 0 and 100 and only those 20 games are ever attempted; the other 180 remain permanently starved despite this rotation fix. Advance by the number actually attempted, or retain a cursor for the next unprocessed ID.
Useful? React with 👍 / 👎.
| var yearAligned = group | ||
| .Where(c => c.Year is { } y && Math.Abs(y - localYear) <= YearTolerance) | ||
| .ToList(); | ||
| if (yearAligned.Count > 0) group = yearAligned; |
There was a problem hiding this comment.
When the local game has a known year but every exact-title candidate falls outside the ±1 tolerance, yearAligned is empty and the code retains the original group. A single same-platform result is then accepted—for example, a local 2016 DOOM can be permanently linked to the lone 1993 result returned within IGDB's limited search response—and the assigned IgdbId prevents future correction. Treat an explicit incompatible candidate year as a reason to decline rather than falling back to it.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex Please re-review at the new head (
Pushed commits on
Verification (run locally, not by the sandboxed reviewers):
Thank you for the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b848f71a19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // filter makes an in-flight assignment turn this load into null -> skip, | ||
| // never overwriting it. All the slow I/O has already happened, so the | ||
| // load->apply->save window is microseconds with no await. | ||
| var game = await _db.Games.FirstOrDefaultAsync(g => g.Id == gameId && g.IgdbId == null, ct); |
There was a problem hiding this comment.
When another request assigns IgdbId or fills a previously null metadata field after this SELECT returns but before SaveChangesAsync executes its UPDATE, EF has no concurrency token and the backfill can still overwrite that committed value. The fresh evidence in b848f71 is that the proposed atomic fix remains two separate database commands at lines 230 and 242; having no slow I/O between them only narrows the race. Use a conditional write, transaction, or concurrency token and treat a zero-row update/concurrency conflict as a skip.
Useful? React with 👍 / 👎.
| var match = IgdbBackfillPlanner.BestMatch( | ||
| new Game { Title = seed.Title, Year = seed.Year, Platform = seed.Platform }, | ||
| candidates); |
There was a problem hiding this comment.
If a user changes the game's title, year, or platform while the search or cover download is in flight, this match remains based on the old seed values. The later reload preserves the user's edit but still applies the stale candidate's IgdbId and metadata—for example, renaming a placeholder from DOOM to Hades can link the edited row permanently to DOOM. Compare the reloaded match-relevant fields with the seed and skip or re-search when they changed.
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex Please re-review at the new head (
Pushed commits on
Verification (run locally):
Note: your security review on |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review last commit changes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aa5578f00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var byPlatform = group.Where(c => c.Platform == game.Platform).ToList(); | ||
| if (byPlatform.Count == 1) return new BackfillMatch(byPlatform[0], MatchTier.Exact); |
There was a problem hiding this comment.
When several exact/year-aligned candidates all include the local platform but only one happens to list it first, comparing the legacy singular Platform makes that candidate appear uniquely platform-compatible and permanently links its IgdbId. The provider now exposes the authoritative multi-platform set through IsOn, so use that predicate here; otherwise IGDB's platform ordering can turn an ambiguous match into an arbitrary selection.
Useful? React with 👍 / 👎.
| "gameoftheyearedition", "gameoftheyear", "completeedition", "goty", | ||
| "deluxe", "definitiveedition", "edition", "remastered", |
There was a problem hiding this comment.
For titles such as Dark Souls Remastered, these suffixes collapse a distinct re-release onto the base game's normalized title. If the limited IGDB response contains only the base release—or its year falls within the ±1 tolerance—the single-survivor path accepts it and writes an incorrect IgdbId, after which the row is never reconsidered. Do not strip qualifiers such as remastered or definitiveedition unless identity with the base release can be established independently.
Useful? React with 👍 / 👎.
| var scoped = await _provider.SearchByPlatformAsync(title, platform, ct); | ||
| if (scoped.Count > 0) return scoped; | ||
| return await _provider.SearchAsync(title, ct); |
There was a problem hiding this comment.
With cold caches and no platform-compatible hit, SearchByPlatformAsync and this fallback use different cache keys but issue the same unfiltered IGDB search body, so every miss makes two outbound requests back-to-back before the runner's pacing delay. A sweep of unmatched imports therefore doubles quota consumption and can trigger the throttling abort; cache the raw response once and filter it locally, or otherwise pace/deduplicate the fallback.
AGENTS.md reference: AGENTS.md:L162-L162
Useful? React with 👍 / 👎.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Background service sweeps games with IgdbId == null and fills IGDB developer/publisher/year/description/cover. Fill-only: existing values (Steam import or manual) are never overwritten. Genres fold into the description (no auto-tags); PC/platform-aware, exact-match-only matcher declines ambiguous or duplicate titles (DOOM 1993 vs 2016). Paced, capped, and aborts when IGDB looks throttled. Idempotent + fail-soft. Skipped when IGDB/Twitch unconfigured or disabled via Collectify:IgdbBackfill:Enabled. Claude + Codex reviewed the plan and code; findings fixed and re-verified: metadata clobber, shared-DbContext tracked-state leak, cancellation, pacing placement, duplicate-title ambiguity, unicode folding, test bugs. First hosted service in the repo; adds Microsoft.Extensions.Hosting.Abstractions.
- First sweep now fires after one Interval (PeriodicTimer first tick) not two - Skip cover download when ImagePath is already set (avoids orphan CoverImages row) - Reset empty-result abort streak on any non-empty provider response - Re-check 'still pending' against the DB immediately before save so a concurrent manual IgdbId assignment is never overwritten; detach + skip if it is - Make ProviderConfigured() synchronous (was async-no-await, CS1998) - Tests: 3 new (empty-streak reset, cover-skip, concurrent-assignment guard)
Games past MaxGamesPerSweep were never attempted when low-id titles never match (no attempted-marker by design). The runner now rotates its window by a per-sweep offset (wrapped by the pending count); the service advances the offset by MaxGamesPerSweep each sweep, so every pending game is eventually swept. Test: cap 2 with 2 unmatchable low-id games + 2 matchable high-id ones, offset rotation reaches and fills the high-id games.
…otation) - Reload the row tracked after all outbound I/O (search + cover) and apply+save with no await in between, so a concurrent user edit (Description/Year/Developer or IgdbId) during the I/O window is never overwritten; IgdbId==null filter on the reload doubles as the atomic guard (was stale-entity overwrite) - Advance rotation by Attempted (games actually processed), not the configured cap, so a throttle-aborted sweep doesn't skip its unattempted remainder - Planner: when the local year is known but every candidate carries an explicit contradictory year, decline instead of locking a wrong link; unknown candidate years still fall through - Tests: +2 planner (year-contradiction decline, unknown-years fall-through); updated cap/throttle/concurrent/rotation for the filled/attempted result; 462 total green
If a user renames the game or edits its year/platform while the backfill's search/cover I/O is in flight, the match was computed against stale seed values and would bake in a wrong IgdbId. After reloading the row (post-I/O), compare Title/Year/Platform against the seed; if changed, skip and let the cached re-sweep recompute. Test: rename DOOM->Hades during cover I/O is not stale-linked. Residual SELECT->UPDATE microsecond window (no concurrency token) documented as accepted risk for this single-user app; closing it needs a migration or a conditional ExecuteUpdate, deferred.
Fix both real-world no-match cases (Tomb Raider GOTY, Witcher 3: Wild Hunt) with a single-survivor gate throughout: - GameLookupResult.Platforms: carry the FULL mapped platform set (was first-only) so platform filtering is truthful; Platform kept for dropdown compat; add IsOn(platform) - IGameMetadataProvider.SearchByPlatformAsync(query, platform): default DIM filters in memory; IGDB overrides with a platform-scoped cache key so a PC-scoped search never reuses (or leaks into) an unscoped/other-platform cached result - Backend sweep: search platform-scoped first (PC for a Steam import) so console SKUs sharing a title don't crowd out the PC match; fall back to unfiltered when empty. Planner strips trailing edition qualifiers (Game of the Year / Complete Edition / Goty / Deluxe) so a local 'Tomb Raider Game of the Year' matches IGDB's base 'Tomb Raider' - Edit-page prefill: GET /api/lookup/games?platform=X partitions results in memory (same-platform first, rest after, single IGDB call); OnlineSearch / useLookup take an optional platform; GameForm passes the game's platform (skips Other) - Single-survivor rule preserved in Pick: only auto-links when year/platform/ edition narrowing leaves exactly one candidate Tests (+3): PlatformScopedSearch_IsolatesPcSku (Witcher 3), EditionSuffix_ MatchesBaseRelease (Tomb Raider), EditionStrip_DoesNotOverreach. 466 server + 116 client tests green.
…is found
Real-world failure confirmed via HAR + live testing: importing The Witcher 3:
Wild Hunt and Tomb Raider Game of the Year (both Steam/PC) left IgdbId null,
and the edit-page search returned only console re-releases (PS3 for Tomb
Raider; Xbox/PS4 bundles for Witcher 3) with no plain PC entry.
Two bugs fed it:
1. GamePlatformMapping.TryParse("PC (Microsoft Windows)") returned null:
the normalizer strips parentheses/spaces, collapsing IGDB's canonical PC
name to the unregistered key "pcmicrosoftwindows". So every IGDB PC SKU
mapped with an EMPTY Platforms set, and the backfill's byPlatform filter /
edit-page IsOn(PC) prioritization could never recognize it. Added the
parenthesized IGDB variants for Pc and Mac ("Apple Macintosh").
2. IGDB's fuzzy search "..." with limit 10 ranks across ALL platforms, so
re-releases (per-platform SKUs, bundles, editions) crowd the exact PC
release out of the window entirely. The platform-scoped search now appends
a source-level Apicalypse 'where platforms = (id)' clause (PC=6, PS4=48,
...) so IGDB runs the fuzzy match within just the target platform. In-memory
IsOn() filtering is kept as a safety net for platforms with no canonical
single id (Mobile, Other, SteamDeck).
Tests: +8 — PC-scoped appends where platforms=(6); id clause per known
platform; unmapped platforms skip the clause and filter in memory; scoped vs
unscoped cache keys stay distinct; repeated scoped calls hit cache. 482 server
+ 116 client tests green.
3042a9c to
a6c1554
Compare
GamePlatformMapping/Pick compared the candidate's legacy singular Platform (first-listed platform only) to the local game's platform. A candidate that listed the local platform later in its set appeared unmatched, while one whose platform was listed first could be picked arbitrarily. The provider already exposes the authoritative multi-platform set via IsOn — use it. Also hardens the single-candidate accept: a lone candidate whose known platform POSITIVELY CONTRADICTS the local game's (e.g. a lone PS3 SKU for a Steam/PC Tomb Raider import — the exact real failure) is declined rather than locked in with the wrong IgdbId forever. Other games still auto-link. Tests: planner Hit() helper now mirrors IGDB by populating Platforms from the passed platform; RunSweepAsync_FillOnly uses a same-platform (Switch) candidate so it exercises fill-only merge without tripping the (correct) platform contradiction guard. 494 server + 116 client tests green.
Live testing showed every IGDB result coming back with platforms: [] even though the committed Map() populates it. Root cause: the lookup cache is keyed by (Provider, Key) with NO schema guard, and the row for 'search:the witcher 3: wild hunt' was written BEFORE the Platforms field existed on GameLookupResult. On cache hit, deserializing that old JSON into the new DTO leaves Platforms at its default empty [] silently (no exception), so the new source-filter + platform-mapping code was never exercised — the stale result won for the 30-day TTL. Fix: prefix every IGDB cache key with a schema-version (v2). Unversioned (or older-version) rows no longer match, forcing a one-time refresh. The v2 prefix also cleanly separates scoped/unscoped/id/barcode keysets. Future DTO shape changes just bump CacheSchemaVersion. Test: SearchAsync_UnversionedStaleCacheRow_IsNotServed_AndRefreshes seeds an old unversioned row and asserts it's ignored (fresh upstream call with full Platforms). 495 server + 116 client tests green.
The /games lookup endpoint ignored the client's platform param for the actual query — it did one unscoped IGDB fuzzy search (top-10 across ALL platforms) then only reordered by IsOn in memory. Console re-releases still crowded the window, so a PC game's search surfaced PS4/Xbox SKUs and buried the right PC SKU (the source-level 'where platforms = (id)' filter added in a6c1554 was never reached by the endpoint). Now: when the client passes a known (non-Other) platform, the endpoint calls SearchByPlatformAsync so IGDB filters at the source ('where platforms = (6)') and console SKUs are excluded from the result set entirely. Falls back to an unscoped search when the scoped query returns empty (e.g. IGDB has no entry for that platform) or the platform is unset/Other. Test: SearchGames_WithPlatform_FiltersToThatPlatformOnly asserts a PC-scoped search returns only the PC SKU, not a mixed list. 496 server + 116 client tests green.
1 participant
Closes #132
What
A background service (
IgdbBackfillService : BackgroundService) sweeps all games withIgdbId == nulland fills IGDB developer/publisher/year/description/cover in the background — never in the import request.Behavior (confirmed with product owner)
IgdbIdis always written.Game.Year(±1) and the game's own platform to disambiguate; declines ambiguous/duplicate titles (DOOM 1993 vs 2016) rather than guessing.MaxGamesPerSweep, abort on consecutive empty results (IGDB throttling is surfaced as[]).Collectify:IgdbBackfill:Enabled.Review
Claude + Codex independently reviewed the plan and code; their findings (metadata clobber, shared-DbContext tracked-state leak, cancellation, pacing placement, duplicate-title ambiguity, unicode folding, test bugs) were fixed and the suite re-verified.
Verification
dotnet build: 0 errorsdotnet test: 456 passed / 0 failed (was 421 baseline; +35 new)Microsoft.Extensions.Hosting.Abstractions(+ lock files).