Merge pull request #400 from topcoder-platform/PM-4299-1

Fix for scoring race condition seen in MM

Author: jmgasper

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

@@ -1094,7 +1094,7 @@ describe('ScoringResultService', () => {
     );
   });
 
-  it('does not persist system summation when completing the review fails', async () => {
+  it('persists system summation before completing the review', async () => {
     const { service, httpService, m2mService, prisma } = createService();
     const systemPayload: ScoringResultCallbackPayload = {
       ...basePayload,
@@ -1156,12 +1156,26 @@ describe('ScoringResultService', () => {
         },
       }),
     );
-    expect(findExistingReviewSummationsSpy).not.toHaveBeenCalled();
-    expect(createReviewSummationSpy).not.toHaveBeenCalled();
+    expect(findExistingReviewSummationsSpy).toHaveBeenCalledWith(
+      'm2m-token',
+      systemPayload.submissionId,
+      'system',
+    );
+    expect(createReviewSummationSpy).toHaveBeenCalledWith(
+      'm2m-token',
+      expect.objectContaining({
+        aggregateScore: 100,
+        isFinal: true,
+        submissionId: systemPayload.submissionId,
+      }),
+    );
     expect(updateReviewSummationSpy).not.toHaveBeenCalled();
+    expect(createReviewSummationSpy.mock.invocationCallOrder[0]).toBeLessThan(
+      httpService.patch.mock.invocationCallOrder[0],
+    );
   });
 
-  it('does not persist currentReview summation when completing the review fails', async () => {
+  it('persists currentReview summation before completing the review', async () => {
     const { service, httpService, m2mService, prisma } = createService();
     const systemPayload: ScoringResultCallbackPayload = {
       ...basePayload,
@@ -1220,10 +1234,21 @@ describe('ScoringResultService', () => {
         },
       }),
     );
-    expect(upsertFromLegacyReviewPayloadSpy).not.toHaveBeenCalled();
+    expect(upsertFromLegacyReviewPayloadSpy).toHaveBeenCalledWith(
+      'm2m-token',
+      expect.objectContaining({
+        fallbackScore: 100,
+        fallbackSubmissionId: systemPayload.submissionId,
+        legacyReview: systemPayload.currentReview,
+        testPhase: 'system',
+      }),
+    );
+    expect(
+      upsertFromLegacyReviewPayloadSpy.mock.invocationCallOrder[0],
+    ).toBeLessThan(httpService.patch.mock.invocationCallOrder[0]);
   });
 
-  it('does not persist relative summations when completing the review fails', async () => {
+  it('persists relative summations before completing the review', async () => {
     const { service, httpService, m2mService, prisma } = createService();
     const systemPayload: ScoringResultCallbackPayload = {
       ...basePayload,
@@ -1300,7 +1325,19 @@ describe('ScoringResultService', () => {
         },
       }),
     );
-    expect(upsertReviewSummationSpy).not.toHaveBeenCalled();
+    expect(upsertReviewSummationSpy).toHaveBeenCalledWith(
+      'm2m-token',
+      'system',
+      expect.objectContaining({
+        aggregateScore: 100,
+        isFinal: true,
+        submissionId: systemPayload.submissionId,
+      }),
+      undefined,
+    );
+    expect(upsertReviewSummationSpy.mock.invocationCallOrder[0]).toBeLessThan(
+      httpService.patch.mock.invocationCallOrder[0],
+    );
   });
 
   it('returns a bad request error when review-api rejects a nonexistent submissionId', async () => {

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

@@ -216,7 +216,7 @@ export class ScoringResultService {
 
   /**
    * Processes one scorer callback payload after verifying the challenge config exists,
-   * then completes any SYSTEM review before final summation writes.
+   * then writes terminal summations before completing any SYSTEM review.
    */
   async processScoringResult(
     payload: ScoringResultCallbackPayload,
@@ -279,18 +279,6 @@ export class ScoringResultService {
         payload.score,
       );
 
-      await this.completeSystemReviewIfNeeded(
-        token,
-        payload.reviewId,
-        currentReviewScore,
-        normalizedPhase,
-        {
-          challengeId: payload.challengeId,
-          scorecardId: fallbackScorecardId,
-          submissionId: payload.submissionId,
-        },
-      );
-
       await this.upsertFromLegacyReviewPayload(token, {
         legacyReview: payload.currentReview,
         fallbackSubmissionId: payload.submissionId,
@@ -311,6 +299,18 @@ export class ScoringResultService {
         });
       }
 
+      await this.completeSystemReviewIfNeeded(
+        token,
+        payload.reviewId,
+        currentReviewScore,
+        normalizedPhase,
+        {
+          challengeId: payload.challengeId,
+          scorecardId: fallbackScorecardId,
+          submissionId: payload.submissionId,
+        },
+      );
+
       await this.notifyScoringCompletionEmailIfReady(
         token,
         payload,
@@ -328,6 +328,7 @@ export class ScoringResultService {
       testPhase: normalizedPhase,
     });
 
+    await this.upsertReviewSummation(token, normalizedPhase, reviewPayload);
     await this.completeSystemReviewIfNeeded(
       token,
       payload.reviewId,
@@ -339,7 +340,6 @@ export class ScoringResultService {
         submissionId: payload.submissionId,
       },
     );
-    await this.upsertReviewSummation(token, normalizedPhase, reviewPayload);
     await this.notifyScoringCompletionEmailIfReady(
       token,
       payload,
@@ -452,6 +452,7 @@ export class ScoringResultService {
       testPhase: normalizedPhase,
     });
 
+    await this.upsertReviewSummation(token, normalizedPhase, reviewPayload);
     await this.completeSystemReviewIfNeeded(
       token,
       input.reviewId,
@@ -463,7 +464,6 @@ export class ScoringResultService {
         submissionId: input.submissionId,
       },
     );
-    await this.upsertReviewSummation(token, normalizedPhase, reviewPayload);
   }
 
   /**
@@ -755,7 +755,7 @@ export class ScoringResultService {
   /**
    * Recomputes relative scores for the latest submission from each member while
    * holding a challenge/phase advisory lock for the read-compute-write cycle.
-   * Completes the current SYSTEM review before writing the recomputed summations.
+   * Writes recomputed summations before completing the current SYSTEM review.
    * Returns undefined when relative scoring cannot be applied and the caller
    * should fall back to direct review upserts.
    */
@@ -894,18 +894,6 @@ export class ScoringResultService {
       return undefined;
     }
 
-    await this.completeSystemReviewIfNeeded(
-      token,
-      payload.reviewId,
-      currentReviewPayload.payload.aggregateScore,
-      testPhase,
-      {
-        challengeId: payload.challengeId,
-        scorecardId: fallbackScorecardId,
-        submissionId: payload.submissionId,
-      },
-    );
-
     for (let index = 0; index < relativeReviewPayloads.length; index += 1) {
       const reviewPayload = relativeReviewPayloads[index];
       const reviewId = this.asString(reviewPayload.reviewObject.id);
@@ -930,6 +918,18 @@ export class ScoringResultService {
       );
     }
 
+    await this.completeSystemReviewIfNeeded(
+      token,
+      payload.reviewId,
+      currentReviewPayload.payload.aggregateScore,
+      testPhase,
+      {
+        challengeId: payload.challengeId,
+        scorecardId: fallbackScorecardId,
+        submissionId: payload.submissionId,
+      },
+    );
+
     return currentReviewPayload?.payload.aggregateScore;
   }
 
@@ -2801,7 +2801,7 @@ export class ScoringResultService {
   }
 
   /**
-   * Completes the originating review record before final SYSTEM summations are persisted.
+   * Completes the originating review record after final SYSTEM summations are persisted.
    */
   private async completeSystemReviewIfNeeded(
     token: string,