Merge pull request #417 from topcoder-platform/develop

Fix for final system tests not working for one of the MM165 submissions

Author: jmgasper

README.md

@@ -300,7 +300,7 @@ Config identity semantics:
 - `POST /challenge/:challengeId` validates that the challenge exists in challenge-api and that `reviewScorecardId` resolves in review-api before persisting. If a config already exists for the challenge, the API returns `409 Conflict`.
 - `phaseConfig.phaseId` stores the canonical challenge phase definition id from challenge-api `phases[].phaseId`. Create/update requests also accept the challenge-phase row `id` and normalize it before persistence for backwards compatibility.
 - `reviewScorecardId` can be either the current review-api scorecard id or a legacy id; scoring callback processing resolves it to the canonical scorecard id before posting review summations.
-- When `relativeScoringEnabled = true`, review scores are recomputed from per-test raw scores against the best score currently held by the latest submission from each member, so final review summation scores stay within `0..100` and can change as new submissions arrive. The raw callback result is stored only as a pending placeholder until the recompute worker writes the normalized score.
+- When `relativeScoringEnabled = true`, review scores are recomputed from per-test raw scores against the best score currently held by the latest submission from each member, so final review summation scores stay within `0..100` and can change as new submissions arrive. The raw callback result is stored only as a pending placeholder until the recompute worker writes the normalized score; queued recompute passes also finalize visible pending placeholders that are not selected as the latest member submission.
 
 Important runtime behavior:
 

docs/review-phase-scoring.md

@@ -84,7 +84,7 @@ The queue is keyed by challenge ID, review ID, and submission ID so repeated pha
 
 ## Relative scoring at completion
 
-If `relativeScoringEnabled = true`, `ScoringResultService` persists the raw callback result as a pending, non-passing placeholder and queues a pg-boss `relative-scoring-recompute` job. The worker recalculates normalized aggregate scores under a challenge/phase PostgreSQL advisory lock, updates Review API, and then completes matching SYSTEM reviews with the recomputed scores. `ChallengeCompletionService.finalizeChallenge(...)` consumes those persisted review summaries; it does not recompute relative scoring itself.
+If `relativeScoringEnabled = true`, `ScoringResultService` persists the raw callback result as a pending, non-passing placeholder and queues a pg-boss `relative-scoring-recompute` job. The worker recalculates normalized aggregate scores under a challenge/phase PostgreSQL advisory lock, updates Review API, and then completes matching SYSTEM reviews with the recomputed scores. Each worker pass also finalizes any persisted `relativeScoringPending` placeholders it can see, even when a placeholder is not the selected latest submission for that member. `ChallengeCompletionService.finalizeChallenge(...)` consumes those persisted review summaries; it does not recompute relative scoring itself.
 
 The recompute queue debounces SYSTEM bursts by challenge and phase so simultaneous scorer callbacks do not all run the expensive fan-out. Retry behavior is controlled by `RELATIVE_SCORING_RECOMPUTE_RETRY_DELAY_SECONDS` (default `60`), `RELATIVE_SCORING_RECOMPUTE_RETRY_LIMIT` (default `1000`), `RELATIVE_SCORING_RECOMPUTE_DEBOUNCE_SECONDS` (default `15`), `RELATIVE_SCORING_RECOMPUTE_START_DELAY_SECONDS` (default `10`), and `RELATIVE_SCORING_RECOMPUTE_WORKER_CONCURRENCY` (default `1`).
 

docs/submission-phase-scoring.md

@@ -112,7 +112,7 @@ Relative scoring applies when:
 - `relativeScoringEnabled = true` on the Marathon Match config
 - `testScores` are present in the scorer metadata
 
-In that case, `ScoringResultService` writes the raw callback result as a pending, non-passing placeholder, then queues a pg-boss relative scoring recomputation. The worker recalculates latest-submission review scores relative to the current best result, updates the persisted aggregate scores, and keeps existing `reviewedDate` values so relative-score updates do not move historical review timestamps.
+In that case, `ScoringResultService` writes the raw callback result as a pending, non-passing placeholder, then queues a pg-boss relative scoring recomputation. The worker recalculates latest-submission review scores relative to the current best result, updates the persisted aggregate scores, and keeps existing `reviewedDate` values so relative-score updates do not move historical review timestamps. A worker pass also finalizes visible `relativeScoringPending` placeholders that are not selected as the latest member submission, so completed reruns do not remain in progress.
 
 ## Tester-change rerun
 

src/api/scoring-result/scoring-result.service.spec.ts

@@ -828,6 +828,121 @@ describe('ScoringResultService', () => {
     );
   });
 
+  it('finalizes pending relative placeholders even when they are not latest member submissions', async () => {
+    const { service, m2mService, prisma } = createService();
+    const reviewFor = (
+      submissionId: string,
+      rawScore: number,
+      relativeScoringPending = false,
+    ) => ({
+      id: `summation-${submissionId}`,
+      aggregateScore: relativeScoringPending ? 0 : rawScore,
+      isFinal: true,
+      scorecardId: 'scorecard-1',
+      metadata: {
+        ...(relativeScoringPending ? { relativeScoringPending: true } : {}),
+        testStatus: relativeScoringPending
+          ? ScoringTestStatus.InProgress
+          : ScoringTestStatus.Success,
+        testType: 'system',
+        testScores: [{ testcase: '753388858', score: rawScore }],
+      },
+    });
+
+    prisma.marathonMatchConfig.findUnique.mockResolvedValue({
+      challengeId: basePayload.challengeId,
+      name: 'Blocks',
+      submissionApiUrl: 'https://api.topcoder-dev.com/v6',
+      relativeScoringEnabled: true,
+      scoreDirection: ScoreDirection.MAXIMIZE,
+    });
+    m2mService.getM2MToken.mockResolvedValue('m2m-token');
+
+    jest
+      .spyOn(service as any, 'withRelativeScoringLock')
+      .mockImplementation(
+        async (
+          _challengeId: string,
+          _testPhase: string,
+          work: () => Promise<unknown>,
+        ) => work(),
+      );
+    jest
+      .spyOn(service as any, 'resolveScorecardId')
+      .mockResolvedValue('scorecard-1');
+    jest.spyOn(service as any, 'fetchChallengeSubmissions').mockResolvedValue([
+      {
+        id: 'submission-newer',
+        memberId: 'member-a',
+        isLatest: true,
+        submittedDate: '2026-05-01T00:00:02.000Z',
+        reviewSummation: [reviewFor('submission-newer', 100)],
+      },
+      {
+        id: 'submission-old-pending',
+        memberId: 'member-a',
+        isLatest: false,
+        submittedDate: '2026-05-01T00:00:01.000Z',
+        reviewSummation: [reviewFor('submission-old-pending', 50, true)],
+      },
+      {
+        id: 'submission-other',
+        memberId: 'member-b',
+        isLatest: true,
+        submittedDate: '2026-05-01T00:00:03.000Z',
+        reviewSummation: [reviewFor('submission-other', 80)],
+      },
+    ]);
+    const upsertReviewSummationSpy = jest
+      .spyOn(service as any, 'upsertReviewSummation')
+      .mockResolvedValue(undefined);
+    jest
+      .spyOn(service as any, 'completeSystemReviewIfNeeded')
+      .mockResolvedValue(undefined);
+    jest
+      .spyOn(service as any, 'notifyScoringCompletionEmailIfReady')
+      .mockResolvedValue(undefined);
+
+    await expect(
+      service.recomputeQueuedRelativeScoring({
+        challengeId: basePayload.challengeId,
+        submissionId: 'submission-old-pending',
+        reviewId: 'review-old-pending',
+        testPhase: 'system',
+        reviewTypeId: basePayload.reviewTypeId,
+        scorecardId: 'scorecard-1',
+        queuedAt: '2026-05-01T00:00:10.000Z',
+      }),
+    ).resolves.toBe(undefined);
+
+    const pendingPayload = upsertReviewSummationSpy.mock.calls
+      .map(
+        ([, , payload]) =>
+          payload as {
+            aggregateScore: number;
+            isPassing: boolean;
+            metadata?: Record<string, unknown>;
+            submissionId: string;
+          },
+      )
+      .find((payload) => payload.submissionId === 'submission-old-pending');
+
+    expect(pendingPayload).toEqual(
+      expect.objectContaining({
+        aggregateScore: 50,
+        isPassing: true,
+        submissionId: 'submission-old-pending',
+        metadata: expect.objectContaining({
+          testProgress: 1,
+          testStatus: ScoringTestStatus.Success,
+        }),
+      }),
+    );
+    expect(pendingPayload?.metadata).not.toHaveProperty(
+      'relativeScoringPending',
+    );
+  });
+
   it('serializes concurrent relative scoring recomputations for the same challenge phase', async () => {
     const { service, m2mService, prisma } = createService();
     const firstPayload: ScoringResultCallbackPayload = {
@@ -1642,6 +1757,83 @@ describe('ScoringResultService', () => {
     );
   });
 
+  it('uses the review matching the callback submission when reviewId is mismatched', async () => {
+    const { service, httpService, m2mService, prisma } = createService();
+
+    const systemPayload: ScoringResultCallbackPayload = {
+      ...basePayload,
+      reviewId: 'review-for-other-submission',
+      score: 100,
+      scorecardId: 'scorecard-1',
+      testPhase: 'system',
+    };
+
+    prisma.marathonMatchConfig.findUnique.mockResolvedValue({
+      challengeId: basePayload.challengeId,
+      name: 'Blocks',
+      submissionApiUrl: 'https://api.topcoder-dev.com/v6',
+      relativeScoringEnabled: false,
+      scoreDirection: ScoreDirection.MAXIMIZE,
+    });
+    m2mService.getM2MToken.mockResolvedValue('m2m-token');
+
+    jest
+      .spyOn(service as any, 'resolveScorecardId')
+      .mockResolvedValue('scorecard-1');
+    jest
+      .spyOn(service as any, 'findExistingReviewSummations')
+      .mockResolvedValue([]);
+    jest
+      .spyOn(service as any, 'createReviewSummation')
+      .mockResolvedValue(undefined);
+
+    httpService.get.mockReturnValue(
+      of({
+        data: {
+          data: [
+            {
+              challengeId: basePayload.challengeId,
+              id: 'review-for-callback-submission',
+              scorecardId: 'scorecard-1',
+              status: 'COMPLETED',
+              submissionId: basePayload.submissionId,
+            },
+          ],
+        },
+      }),
+    );
+    httpService.patch.mockReturnValue(
+      of({ data: { id: 'review-for-callback-submission' } }),
+    );
+
+    await expect(service.processScoringResult(systemPayload)).resolves.toBe(
+      undefined,
+    );
+
+    expect(httpService.patch).toHaveBeenCalledTimes(1);
+    expect(httpService.patch).toHaveBeenCalledWith(
+      'https://api.topcoder-dev.com/v6/reviews/review-for-callback-submission',
+      expect.objectContaining({
+        finalScore: 100,
+        status: 'COMPLETED',
+      }),
+      expect.any(Object),
+    );
+    expect(httpService.patch).not.toHaveBeenCalledWith(
+      'https://api.topcoder-dev.com/v6/reviews/review-for-other-submission',
+      expect.any(Object),
+      expect.any(Object),
+    );
+    expect(mockLogger.warn).toHaveBeenCalledWith(
+      expect.objectContaining({
+        message:
+          'Ignoring system reviewId because it does not match the callback submission context.',
+        reviewId: 'review-for-other-submission',
+        submissionId: basePayload.submissionId,
+      }),
+    );
+  });
+
   it('persists system summation before completing the review', async () => {
     const { service, httpService, m2mService, prisma } = createService();
     const systemPayload: ScoringResultCallbackPayload = {
@@ -1673,7 +1865,14 @@ describe('ScoringResultService', () => {
     httpService.get.mockReturnValue(
       of({
         data: {
-          data: [],
+          data: [
+            {
+              challengeId: basePayload.challengeId,
+              id: 'review-1',
+              status: 'PENDING',
+              submissionId: basePayload.submissionId,
+            },
+          ],
         },
       }),
     );
@@ -1751,7 +1950,14 @@ describe('ScoringResultService', () => {
     httpService.get.mockReturnValue(
       of({
         data: {
-          data: [],
+          data: [
+            {
+              challengeId: basePayload.challengeId,
+              id: 'review-1',
+              status: 'PENDING',
+              submissionId: basePayload.submissionId,
+            },
+          ],
         },
       }),
     );
@@ -1842,7 +2048,14 @@ describe('ScoringResultService', () => {
       .mockReturnValueOnce(
         of({
           data: {
-            data: [],
+            data: [
+              {
+                challengeId: basePayload.challengeId,
+                id: 'review-1',
+                status: 'PENDING',
+                submissionId: basePayload.submissionId,
+              },
+            ],
           },
         }),
       );