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

オープン
  • 何か問題がありますか？
#723 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

まだ誰も着手していません。

評価

難易度
3/5
見積もり時間
1〜2日
初心者へのやさしさ
78/100
issue の種類
バグ
明瞭さ
明確に書かれている
活発さ
活発
技術スタック
postgresql, typescript
領域
backend, databases, testing

調査の方向性

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.

索引モデルが issue の本文から書いたものです。

説明

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.
主要言語
TypeScript
スター
2
フォーク
90
平均マージ
31分
マージ済み PR（30日）
20

環境構築

  • Dockerfile または Docker Compose ファイルあり
  • プルリクエストのテンプレートなし
  • コントリビューションガイドなし

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

MergeFi/backend のほかの issue

MergeFi/backend の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。