Merge pull request #45 from asm0dey/fix/deck-close-activate-race

fix(deck): atomic conditional close prevents clobbering next slide's activate

Author: asm0dey

src/main/java/site/asm0dey/slidev/polls/core/service/PollRepository.java

@@ -58,6 +58,15 @@ public interface PollRepository {
   /** Close the currently-ACTIVE question on {@code pollId}. No-op when none is active. */
   Poll closeActiveQuestion(UUID pollId);
 
+  /**
+   * Conditional close: close the currently-ACTIVE question on {@code pollId} only when {@code
+   * expectedQuestionId} is null, or matches the question that is active. The guard is applied
+   * atomically under the poll-row lock, so a slide-leave close scoped to the question its slide
+   * opened cannot clobber a concurrent next-slide activate that already moved the active question
+   * on. No-op when none is active or the active question differs from {@code expectedQuestionId}.
+   */
+  Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId);
+
   /**
    * Replace the allowed-origins list for {@code pollId}. A non-null (even empty) list replaces the
    * current value. Throws {@link site.asm0dey.slidev.polls.core.error.NotFoundException} when the

src/main/java/site/asm0dey/slidev/polls/core/service/PollService.java

@@ -384,11 +384,15 @@ public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
     Poll before =
         repository.findById(pollId).orElseThrow(() -> new NotFoundException(pollId.toString()));
     UUID wasActive = before.activeQuestionId();
-    if (expectedQuestionId != null && !expectedQuestionId.equals(wasActive)) {
-      return before;
-    }
-    Poll after = repository.closeActiveQuestion(pollId);
-    if (wasActive != null) {
+    // The expectedQuestionId guard is applied atomically inside the repository under the poll-row
+    // lock, so this unlocked pre-read cannot let a slide-leave close clobber a concurrent
+    // next-slide activate: if the active question has moved on, the conditional UPDATE matches no
+    // row and `after` still reports the newer active question.
+    Poll after = repository.closeActiveQuestion(pollId, expectedQuestionId);
+    // Emit the closed event only when a question actually transitioned ACTIVE -> none. When the
+    // guard no-ops, `after` keeps its active question, so a stale `wasActive` cannot produce a
+    // spurious event.
+    if (wasActive != null && after.activeQuestionId() == null) {
       events.publishEvent(new PollQuestionClosedEvent(pollId, wasActive, Instant.now()));
     }
     return after;

src/main/java/site/asm0dey/slidev/polls/persistence/PollRepositoryImpl.java

@@ -17,6 +17,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.UUID;
+import org.jooq.Condition;
 import org.jooq.DSLContext;
 import org.jooq.Field;
 import org.jooq.Record;
@@ -342,24 +343,40 @@ public Poll activateQuestion(UUID pollId, UUID questionId) {
 
   @Override
   public Poll closeActiveQuestion(UUID pollId) {
+    return closeActiveQuestion(pollId, null);
+  }
+
+  @Override
+  public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
     // Same serialisation reason as activateQuestion — keeps concurrent close + activate on
     // the same poll from interleaving their per-row locks on poll_questions.
+    //
+    // The expectedQuestionId guard is evaluated INSIDE the poll-row lock: the deck fires a
+    // slide-leave close scoped to the question that slide opened, and the next slide's activate
+    // serialises behind the same lock. Folding the guard into the UPDATE's WHERE (rather than a
+    // prior unlocked read in the service) makes the close a true no-op when the active question
+    // has already moved on, so it can never clobber the newer activation (the source of the
+    // deck slide-switch flake / a real presenter-facing race on rapid navigation).
     return dsl.transactionResult(
         cfg -> {
           DSLContext tx = cfg.dsl();
           lockPollRow(tx, pollId);
           OffsetDateTime now = OffsetDateTime.now();
+          Condition where =
+              POLL_QUESTIONS
+                  .POLL_ID
+                  .eq(pollId)
+                  .and(POLL_QUESTIONS.STATUS.eq(QuestionStatus.ACTIVE.name()));
+          if (expectedQuestionId != null) {
+            where = where.and(POLL_QUESTIONS.ID.eq(expectedQuestionId));
+          }
           // poll_questions_active_timestamp_ck demands activated_at be NULL when status is not
           // ACTIVE.
           tx.update(POLL_QUESTIONS)
               .set(POLL_QUESTIONS.STATUS, QuestionStatus.CLOSED.name())
               .setNull(POLL_QUESTIONS.ACTIVATED_AT)
               .set(POLL_QUESTIONS.CLOSED_AT, now)
-              .where(
-                  POLL_QUESTIONS
-                      .POLL_ID
-                      .eq(pollId)
-                      .and(POLL_QUESTIONS.STATUS.eq(QuestionStatus.ACTIVE.name())))
+              .where(where)
               .execute();
           return findById(tx, pollId).orElseThrow(() -> new NotFoundException(pollId.toString()));
         });

src/test/java/site/asm0dey/slidev/polls/core/service/PollServiceLockTest.java

@@ -302,6 +302,11 @@ public Poll closeActiveQuestion(UUID pollId) {
       throw new UnsupportedOperationException();
     }
 
+    @Override
+    public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
+      throw new UnsupportedOperationException();
+    }
+
     @Override
     public Poll updateAllowedOrigins(UUID pollId, List<String> origins) {
       Poll existing = require(pollId);

src/test/java/site/asm0dey/slidev/polls/core/service/PollServiceTest.java

@@ -621,6 +621,15 @@ public Poll closeActiveQuestion(UUID pollId) {
       return after;
     }
 
+    @Override
+    public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
+      Poll existing = requirePresent(pollId);
+      if (expectedQuestionId != null && !expectedQuestionId.equals(existing.activeQuestionId())) {
+        return existing;
+      }
+      return closeActiveQuestion(pollId);
+    }
+
     @Override
     public Poll updateAllowedOrigins(UUID pollId, List<String> origins) {
       Poll existing = requirePresent(pollId);

src/test/java/site/asm0dey/slidev/polls/core/service/VoteServiceTest.java

@@ -335,6 +335,11 @@ public Poll closeActiveQuestion(UUID pollId) {
       throw new UnsupportedOperationException("not needed for VoteServiceTest");
     }
 
+    @Override
+    public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
+      throw new UnsupportedOperationException("not needed for VoteServiceTest");
+    }
+
     @Override
     public Poll updateAllowedOrigins(UUID pollId, java.util.List<String> origins) {
       throw new UnsupportedOperationException("not needed for VoteServiceTest");

src/test/java/site/asm0dey/slidev/polls/persistence/PollRepositoryImplTest.java

@@ -216,6 +216,26 @@ void deletePollCascadesQuestionsWithoutTriggerError() {
       assertThat(repo.findById(inserted.id())).isEmpty();
     }
 
+    @Test
+    void closeActiveQuestion_withExpectedMismatch_isNoOpAndKeepsActive() {
+      Poll poll =
+          insertPollWithQuestions(
+              "close-guard",
+              new QuestionSeed("Q1?", List.of("A", "B")),
+              new QuestionSeed("Q2?", List.of("A", "B")));
+      UUID q1 = poll.questions().getFirst().id();
+      UUID q2 = poll.questions().getLast().id();
+
+      assertThat(repo.activateQuestion(poll.id(), q2).activeQuestionId()).isEqualTo(q2);
+
+      // A slide-leave close scoped to Q1 must NOT clobber the active Q2 — this is the atomic
+      // guard that fixes the deck slide-switch race (close racing past the next slide's activate).
+      assertThat(repo.closeActiveQuestion(poll.id(), q1).activeQuestionId()).isEqualTo(q2);
+
+      // A close scoped to the actually-active Q2 closes it.
+      assertThat(repo.closeActiveQuestion(poll.id(), q2).activeQuestionId()).isNull();
+    }
+
     private record QuestionSeed(String prompt, List<String> optionLabels) {}
 
     private Poll insertPollWithQuestions(String slugSuffix, QuestionSeed... seeds) {

src/test/java/site/asm0dey/slidev/polls/realtime/sse/TallyBroadcastTest.java

@@ -395,6 +395,11 @@ public Poll closeActiveQuestion(UUID pollId) {
       throw new UnsupportedOperationException();
     }
 
+    @Override
+    public Poll closeActiveQuestion(UUID pollId, UUID expectedQuestionId) {
+      throw new UnsupportedOperationException();
+    }
+
     @Override
     public Poll updateAllowedOrigins(UUID pollId, java.util.List<String> origins) {
       throw new UnsupportedOperationException();