ReputationService.getLatest and computeAndSave call each other with no base case — a contributor with no snapshot sends the request into an infinite DB loop

Aperta
  • Qualcosa non va?
#723 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
3/5
Tempo stimato
1-2 giorni
Idoneità per principianti
78/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
postgresql, typescript
Ambito
backend, databases, testing

Direzione di ricerca

Start with src/reputation/reputation.service.ts and the getLatest and computeAndSave methods, then read src/reputation/reputation.service.spec.ts and the controller’s latest() route. Make the missing-snapshot path terminate, preserve the de-duplication check, and verify that computeAndSave saves once when no snapshot exists; the existing null-snapshot spec should also pass.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

bug performance

Location — src/reputation/reputation.service.ts:30 (computeAndSave, line 37 const latest = await this.getLatest(userId);) and src/reputation/reputation.service.ts:66 (getLatest, line 72 return this.computeAndSave(userId);).

Problem — The two methods are mutually recursive with no terminating condition for a user who has no snapshot row yet:

  • computeAndSave(userId) computes stats, then calls getLatest(userId) to decide whether the new values are identical to the previous snapshot (the de-duplication check at lines 37-49) before it saves anything.
  • getLatest(userId) runs snapshotRepo.findOne(...); when that returns null it calls computeAndSave(userId) to lazily create the first snapshot.

So for any user with zero rows in reputation_snapshots, the sequence is: computeAndSave -> getLatest -> findOne returns null -> computeAndSave -> getLatest -> findOne still returns null (nothing was ever saved, because the save at line 51 is never reached) -> and so on. The recursion is await-based, so it does not blow the JS stack quickly; it simply loops forever, issuing one queryContributorCoreStats multi-join aggregate plus one findOne per iteration until the request times out or the process is killed.

Why it matters — This is the first-use path for every new contributor, i.e. the common case, not an edge case. The request never returns, and while it spins it holds a connection and hammers Postgres with an expensive aggregate query in a tight loop. A handful of such requests exhausts the TypeORM pool and takes down unrelated endpoints. It also silently defeats the lazy-snapshot behaviour that was supposed to fix empty profiles: no snapshot is ever written for a first-time user.

Reproduction / failure path

  1. Create a user who has no reputation_snapshots row (any freshly registered contributor).
  2. Authenticate as that user and call either GET /reputation/:userId or POST /reputation/:userId/recompute.
  3. The request hangs. DATABASE_LOGGING=true shows the reputation_snapshots findOne and the contributor-stats aggregate repeating indefinitely.

The existing unit test already disagrees with the code and should be failing: src/reputation/reputation.service.spec.ts:122 ('returns null when the user has no snapshot yet') mocks snapshotRepo.findOne to resolve null and asserts getLatest('new-user') resolves to null, but the code path goes into computeAndSave instead. Because queryContributorCoreStats is jest.mocked and has no return value configured in that test, the spec fails with a TypeError on stats.lifetimeEarnings rather than reporting the real recursion.

Suggested fix

  • Remove the lazy-compute from getLatest and make it a pure read that returns null when there is no snapshot (matching its declared Promise<ReputationSnapshot | null> type and the existing spec).
  • Give computeAndSave its own private read for the de-duplication check — e.g. private findLatestSnapshot(userId) doing just the findOne — so it never re-enters the lazy-create path.
  • Have the controller's latest() route decide the lazy-create policy explicitly: read, and if null, call computeAndSave exactly once and return its result.
  • Keep the order: { computedAt: 'DESC' } read but add id: 'DESC' as a tie-break so two snapshots written in the same CreateDateColumn tick resolve deterministically.
  • Add a spec that calls computeAndSave with findOne resolving null and asserts snapshotRepo.save is called exactly once.
Lingua principale
TypeScript
Stelle
2
Fork
90
Merge medio
31m
PR unite (30g)
20

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Nessun modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di MergeFi/backend

Tutte le issue di MergeFi/backend

Issue simili

Altre issue su TypeScript

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.