fix(ingestor): advertise RankSnapshotWriter sync side-effect (#267)#514
Conversation
RankSnapshotWriter.Write mutated account.LastRankSyncAtUtc (and Score) even when it returned Unchanged, which read as a surprising side-effect on the no-op path. The timestamp bump is load-bearing rather than accidental: LastRankSyncAtUtc is the freshness gate (RankSyncFreshness, 15m) that stops AccountRefreshProcess from re-issuing the League-v4 by-puuid call and dedups against DiscoveryProcess. A fresh reading that matches the latest rank is still a successful sync, so the timestamp must advance on the Unchanged path; guarding it behind the changed-state branch would silently regress that gate and re-fetch stable-rank accounts every cycle. Per the issue's second option, advertise the side-effect instead of hiding it: rename Write -> Ingest and document on the interface that every reading advances the account's sync bookkeeping while a snapshot row is appended only when the rank actually changed. Closes #267
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
La PR est propre : le renommage est complet, tous les call sites sont mis à jour, et la documentation XML explique clairement pourquoi l'effet de bord sur Unchanged est délibéré.
Vérifications effectuées :
AccountRefreshProcess.csetDiscoveryProcess.cs: les deux appels.Write()→.Ingest()✓RankSnapshotWriter.cs: interface + implémentation renommées ✓- Tests : aucun fichier de test n'appelle
.Write()directement — les tests unitaires mockent l'interface sans configurer d'expectation sur cette méthode ; les tests d'intégration passent parprocess.RunCoreAsync()✓
Suggestions (non bloquantes)
backend/tests/TrueMain.IntegrationTests/RankSnapshotIngestionTests.cs:240— le nom de méthodeRunCoreAsync_SuccessfulRankWrite_BumpsLastRankSyncAtUtccontient encoreWrite. À renommer enRunCoreAsync_SuccessfulRankIngest_BumpsLastRankSyncAtUtcpour rester cohérent avec le nouveau vocabulaire (ne bloque pas la compilation, simple cohérence).
There was a problem hiding this comment.
Review: La PR est propre. Renommage complet, call sites mis a jour, docs XML correctes. Aucun point bloquant. Suggestions (non bloquantes) : RankSnapshotIngestionTests.cs:240 — nom de methode RunCoreAsync_SuccessfulRankWrite_BumpsLastRankSyncAtUtc contient encore Write, a renommer en RunCoreAsync_SuccessfulRankIngest_BumpsLastRankSyncAtUtc pour coherence.
Vocabulary consistency follow-up from PR review: the rank-write test name still referenced the old method name.
| // calls (see AccountRefreshProcess), and Score is the denormalised | ||
| // leaderboard sort key kept in lock-step with the latest rank so the | ||
| // leaderboard can ORDER BY it without recomputing in SQL. Both are | ||
| // idempotent — EF only writes when the value actually changes. |
There was a problem hiding this comment.
NON BLOQUANT — Le « Both are idempotent » est légèrement inexact pour LastRankSyncAtUtc. Score est bien idempotent (EF ne génère pas de SQL UPDATE si la valeur calculée est identique à celle trackée), mais LastRankSyncAtUtc = nowUtc avance à chaque appel — EF voit systématiquement un changement et émet toujours l'UPDATE. Ce ne devrait être que Score qui est qualifié d'idempotent.
| // idempotent — EF only writes when the value actually changes. | |
| // A fresh reading always advances the account's sync bookkeeping, even on | |
| // the Unchanged path: LastRankSyncAtUtc gates redundant League-v4 by-puuid | |
| // calls (see AccountRefreshProcess), and Score is the denormalised | |
| // leaderboard sort key kept in lock-step with the latest rank so the | |
| // leaderboard can ORDER BY it without recomputing in SQL. Score is | |
| // idempotent — EF only writes when the value actually changes. |
There was a problem hiding this comment.
Review - APPROUVE
Rename + documentation pur (Write vers Ingest), aucune regression comportementale. Le raisonnement qui justifie le bump sur le chemin Unchanged est solide et desormais bien documente dans l'XML doc de l'interface.
Le renommage est complet : interface, implementation concrete, deux call sites, et le nom du test d'integration concerne.
Suggestion (non bloquante) : RankSnapshotWriter.cs:53 - 'Both are idempotent' s'applique a Score mais pas a LastRankSyncAtUtc qui avance a chaque appel. Remplacer par 'Score is idempotent' eviterait la confusion. Voir le commentaire inline pour la suggestion de code.
2 participants
Summary
Resolves #267.
RankSnapshotWriter.Writemutatedaccount.LastRankSyncAtUtc(andScore) even when it returnedUnchanged, which read as a surprising side-effect on the no-op path.The issue offered two fixes: (1) guard the timestamp update behind the changed-state branch, or (2) rename the method to advertise the side-effect. I deliberately chose option 2, because option 1 would introduce a behavioral regression:
LastRankSyncAtUtcis the freshness gate inAccountRefreshProcess(RankSyncFreshness, default 15 min, seeAccountRefreshProcess.cs:139). It stops the process from re-issuing the League-v4 by-puuid call and dedups againstDiscoveryProcess.So the bump on the
Unchangedpath is load-bearing, not accidental. This PR keeps the behavior and makes it explicit instead:IRankSnapshotWriter.Write→Ingestto signal it does more than append a row.LastRankSyncAtUtc+Score) regardless of outcome, while a snapshot row is appended only when the rank actually changed.AccountRefreshProcess,DiscoveryProcess).No runtime behavior changes — this is a rename + documentation change.
Testing
dotnet build/dotnet testlocally. The change is a pure method rename plus comments; I verified by inspection that the only.Write(...)call sites on the writer were the two updated, and that all other references are to the type name. CI will compile and run the existing unit/integration suites.Closes #267
Generated by Claude Code