From 96656dc707c36a7e6238f2086dda3aea6b99d62f Mon Sep 17 00:00:00 2001 From: Gavin Sonntag Date: Mon, 27 Apr 2026 16:58:21 -0700 Subject: [PATCH 1/3] fix: preserve mongo transaction errors in UpdateAfterSeen --- server/database/judge.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server/database/judge.go b/server/database/judge.go index 205e313..5148bb2 100644 --- a/server/database/judge.go +++ b/server/database/judge.go @@ -3,6 +3,7 @@ package database import ( "context" "errors" + "fmt" "server/models" "server/util" "strconv" @@ -178,7 +179,11 @@ func UpdateAfterSeen(db *mongo.Database, ctx context.Context, judge *models.Judg }, ) if err != nil { - return errors.New("error updating judge: " + err.Error()) + // Use %w so the underlying mongo.CommandError (and its + // TransientTransactionError label) survives wrapping; otherwise + // session.WithTransaction's errors.As walk fails to find the + // label and skips its WriteConflict auto-retry. + return fmt.Errorf("error updating judge: %w", err) } if judge.Track != "" { @@ -191,7 +196,7 @@ func UpdateAfterSeen(db *mongo.Database, ctx context.Context, judge *models.Judg ) } if err != nil { - return errors.New("error updating project: " + err.Error()) + return fmt.Errorf("error updating project: %w", err) } return nil From 3648629a568b3443890acfd6a305bfb3be287460 Mon Sep 17 00:00:00 2001 From: Gavin Sonntag Date: Mon, 27 Apr 2026 17:11:55 -0700 Subject: [PATCH 2/3] fix: make /judge/next concurrency-safe --- server/router/judge.go | 87 +++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/server/router/judge.go b/server/router/judge.go index cd7da51..ebd45b3 100644 --- a/server/router/judge.go +++ b/server/router/judge.go @@ -2,6 +2,7 @@ package router import ( "errors" + "fmt" "net/http" "server/database" "server/funcs" @@ -392,56 +393,90 @@ func GetNextJudgeProject(ctx *gin.Context) { // Get the judge from the context judge := ctx.MustGet("judge").(*models.Judge) - // If the judge already has a next project, return that project - if judge.Current != nil { - ctx.JSON(http.StatusOK, gin.H{"project_id": judge.Current.Hex()}) - return - } - - // Otherwise, get the next project for the judge + // Outputs from the transaction. Populated under either of two + // outcomes: the judge had a current project (returned as-is), or a + // new project was picked and assigned. pickedProjectName is set only + // in the new-pick case so we can log it once after the txn commits. + var pickedProjectId, pickedProjectName string + + // All judge-state reads/writes happen inside a single transaction. + // IMPORTANT: do NOT call ctx.JSON inside the callback. Mongo's + // session.WithTransaction retries the callback on + // TransientTransactionError (which includes WriteConflict); writing + // the response on a doomed first attempt would race the retry. err := database.WithTransaction(state.Db, func(sc mongo.SessionContext) error { - // Get options + // Reset outputs in case the callback is retried. + pickedProjectId = "" + pickedProjectName = "" + options, err := database.GetOptions(state.Db, sc) if err != nil { - return errors.New("error getting options: " + err.Error()) + return fmt.Errorf("error getting options: %w", err) } - // If the clock is paused, return an empty object - // This is to ensure that no projects are gotten if the clock is paused - state.Clock.Mutex.Lock() - if !state.Clock.State.Running || options.Deliberation { + // Re-read the judge inside the transaction so concurrent + // requests from the same judge see committed state. Without + // this, two simultaneous /judge/next requests both see + // judge.Current == nil from the stale middleware snapshot and + // each pick a different project, leaving one project with the + // judge in its seen list while the judge's current points at + // the other. + freshJudge, err := database.FindJudge(state.Db, sc, judge.Id) + if err != nil { + return fmt.Errorf("error finding judge in database: %w", err) + } + if freshJudge == nil { + return errors.New("judge not found in database") + } + + // Already assigned (either before this request started or by a + // concurrent request that committed first) — surface it. + if freshJudge.Current != nil { + pickedProjectId = freshJudge.Current.Hex() return nil } - state.Clock.Mutex.Unlock() - project, err := judging.PickNextProject(state.Db, sc, judge, state.Comps) - if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error picking next project: " + err.Error()}) + // Read the clock state under its mutex, then unlock immediately + // so the early return path can't deadlock the clock. We only gate + // new assignments here; an already-assigned current project should + // still be returned even if judging gets paused afterward. + state.Clock.Mutex.Lock() + running := state.Clock.State.Running + state.Clock.Mutex.Unlock() + if !running || options.Deliberation { return nil } - // If there is no next project, return an empty object + project, err := judging.PickNextProject(state.Db, sc, freshJudge, state.Comps) + if err != nil { + return fmt.Errorf("error picking next project: %w", err) + } if project == nil { - ctx.JSON(http.StatusOK, gin.H{}) return nil } - // Update judge and project - err = database.UpdateAfterPicked(state.Db, sc, project, judge) + err = database.UpdateAfterPicked(state.Db, sc, project, freshJudge) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error updating next project in database: " + err.Error()}) - return nil + return fmt.Errorf("error updating next project in database: %w", err) } - // Send OK and project ID - state.Logger.JudgeLogf(judge, "Picked new project %s (%s)", project.Name, project.Id.Hex()) - ctx.JSON(http.StatusOK, gin.H{"project_id": project.Id.Hex()}) + pickedProjectId = project.Id.Hex() + pickedProjectName = project.Name return nil }) if err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } + + if pickedProjectId == "" { + ctx.JSON(http.StatusOK, gin.H{}) + return + } + if pickedProjectName != "" { + state.Logger.JudgeLogf(judge, "Picked new project %s (%s)", pickedProjectName, pickedProjectId) + } + ctx.JSON(http.StatusOK, gin.H{"project_id": pickedProjectId}) } // GET /judge/projects - Endpoint to get a list of projects that a judge has seen From 232271c2ed91dc3ea299640e330ed1b29f07cee9 Mon Sep 17 00:00:00 2001 From: Gavin Sonntag Date: Mon, 27 Apr 2026 17:12:31 -0700 Subject: [PATCH 3/3] fix: make /judge/finish concurrency-safe --- server/router/judge.go | 82 ++++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 26 deletions(-) diff --git a/server/router/judge.go b/server/router/judge.go index ebd45b3..021a010 100644 --- a/server/router/judge.go +++ b/server/router/judge.go @@ -719,65 +719,95 @@ func JudgeFinish(ctx *gin.Context) { return } - // Run remaining actions in a transaction + // finishedProjId is set inside the txn once we know which project + // the judge had assigned; empty means the request was a no-op + // (already finished by a concurrent request). + var finishedProjId string + // deliberationActive lets us distinguish a 400 (operator-disabled + // scoring) from a 500 (database failure) when we surface the error + // after the transaction. + deliberationActive := false + + // IMPORTANT: do NOT call ctx.JSON inside the callback. Mongo's + // session.WithTransaction retries the callback on + // TransientTransactionError (which includes WriteConflict). If we + // wrote the response on a doomed first attempt, a successful retry + // would still leave the client looking at the 500. err = database.WithTransaction(state.Db, func(sc mongo.SessionContext) error { - // Get the options and return error if deliberations - options, err := database.GetOptions(state.Db, ctx) + // Reset outputs in case the callback is retried. + finishedProjId = "" + deliberationActive = false + + options, err := database.GetOptions(state.Db, sc) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error getting options: " + err.Error()}) - return err + return fmt.Errorf("error getting options: %w", err) } if options.Deliberation { - ctx.JSON(http.StatusBadRequest, gin.H{"error": "cannot score due to deliberation mode being enabled"}) - return err + deliberationActive = true + return errors.New("cannot score due to deliberation mode being enabled") } - // Get the project from the database - project, err := database.FindProject(state.Db, sc, judge.Current) + // Re-read the judge inside the transaction so concurrent + // finish requests see committed state. Without this, two + // simultaneous POST /judge/finish requests both read + // judge.Current from the stale middleware snapshot and both + // push the same project to seen_projects, producing + // duplicates. + freshJudge, err := database.FindJudge(state.Db, sc, judge.Id) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error finding project in database: " + err.Error()}) - return err + return fmt.Errorf("error finding judge in database: %w", err) + } + if freshJudge == nil { + return errors.New("judge not found in database") + } + if freshJudge.Current == nil { + // Already processed by a concurrent request — treat as success. + return nil + } + finishedProjId = freshJudge.Current.Hex() + + project, err := database.FindProject(state.Db, sc, freshJudge.Current) + if err != nil { + return fmt.Errorf("error finding project in database: %w", err) } - // Create the judged project object judgedProject := models.JudgeProjectFromProject(project, scoreReq.Notes, scoreReq.Starred) - // If groups are enabled and auto switch, move the judge to the next group conditionally if options.MultiGroup && options.SwitchingMode == "auto" { - err = judging.MoveJudgeGroup(state.Db, sc, judge, options) + err = judging.MoveJudgeGroup(state.Db, sc, freshJudge, options) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error moving judge group: " + err.Error()}) - return err + return fmt.Errorf("error moving judge group: %w", err) } } - // Update the judge and project - err = database.UpdateAfterSeen(state.Db, sc, judge, judgedProject) + err = database.UpdateAfterSeen(state.Db, sc, freshJudge, judgedProject) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error storing scores in database: " + err.Error()}) return err } - // Reset list of skipped projects due to busy status - err = database.ResetBusyProjectListForJudge(state.Db, sc, judge) + err = database.ResetBusyProjectListForJudge(state.Db, sc, freshJudge) if err != nil { - ctx.JSON(http.StatusInternalServerError, gin.H{"error": "error resetting busy project list in database: " + err.Error()}) - return err + return fmt.Errorf("error resetting busy project list in database: %w", err) } return nil }) if err != nil { + if deliberationActive { + ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + } else { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + } return } - // Send OK starred := "" if scoreReq.Starred { starred = " and starred project" } - projId := judge.Current.Hex() - state.Logger.JudgeLogf(judge, "Finished judging project %s%s", projId, starred) + if finishedProjId != "" { + state.Logger.JudgeLogf(judge, "Finished judging project %s%s", finishedProjId, starred) + } ctx.JSON(http.StatusOK, gin.H{"ok": 1}) }