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

Open
  • Is something wrong?
#723 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
78/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
postgresql, typescript
Domain
backend, databases, testing

Research direction

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.

Written by the indexing model from the issue text.

Description

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.
Dominant language
TypeScript
Stars
2
Forks
90
Avg merge
31m
Merged PRs (30d)
20

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from MergeFi/backend

All issues in MergeFi/backend

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.