fix(review): bound the post-revision truth refresh and keep it retryable

frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts

@@ -127,7 +127,28 @@ export interface SimilarPastRow {
   date: string
 }
 
-export type CoreSelectorBatchOutcome = 'settled' | 'failed' | 'superseded' | 'unavailable'
+/**
+ * How one exact-key core evidence batch ended.
+ *
+ * - `settled`    every one of the six proposal reads landed for the requested key.
+ * - `failed`     at least one read rejected on its own; the snapshot is incomplete.
+ * - `superseded` the reviewer moved to another proposal or revision mid-flight.
+ * - `unavailable` the requested key is not the one this surface is rendering.
+ * - `aborted`    the CALLER cancelled through its own signal. Kept distinct from
+ *   `failed` because nothing failed: only the caller knows why it cancelled
+ *   (a deadline, a teardown), so only the caller may name that reason.
+ */
+export type CoreSelectorBatchOutcome =
+  | 'settled'
+  | 'failed'
+  | 'superseded'
+  | 'unavailable'
+  | 'aborted'
+
+/** Caller-owned cancellation for one explicit core-batch wait. */
+export interface CoreSelectorBatchWaitOptions {
+  signal?: AbortSignal
+}
 
 export interface PaperReviewSelectors {
   provenance: ComputedRef<ProvenanceRow[]>
@@ -143,6 +164,7 @@ export interface PaperReviewSelectors {
   waitForCoreBatch: (
     proposalId: string,
     revisionIdentity: string | null,
+    options?: CoreSelectorBatchWaitOptions,
   ) => Promise<CoreSelectorBatchOutcome>
 }
 
@@ -533,6 +555,22 @@ export function usePaperReviewSelectors(
     similarPastData.value = EMPTY_SIMILAR
   }
 
+  /**
+   * Cancel the in-flight batch for ONE key without disturbing the generation
+   * bookkeeping. `invalidateCoreBatch` is the stronger neighbour: it also
+   * supersedes the batch's waiters. Here the waiter reports its own outcome, so
+   * the batch is left to reach its ordinary failure branch (which clears
+   * `loading`) instead of being declared superseded.
+   *
+   * The key check matters: a reviewer who moved to another proposal while the
+   * caller was waiting has a NEW batch in flight, and a late cancellation from
+   * the abandoned wait must not tear that one down.
+   */
+  function abortInFlightCoreBatchForKey(key: SelectorKey) {
+    if (!activeCoreBatch || !selectorKeysEqual(activeCoreBatch.key, key)) return
+    abortController?.abort()
+  }
+
   function invalidateCoreBatch() {
     fetchGeneration += 1
     activeCoreBatch?.supersede()
@@ -728,16 +766,56 @@ export function usePaperReviewSelectors(
     return promise
   }
 
+  /**
+   * Await the exact-key core batch on behalf of an explicit reviewer action.
+   *
+   * The optional `signal` belongs to the CALLER, not to this composable. When
+   * it fires the wait resolves `aborted` immediately and the in-flight batch is
+   * cancelled, so a caller holding a decision lock is released even when the
+   * transport (or a test double) ignores cancellation and never settles. The
+   * batch's own continuations still run behind their generation guard, so a
+   * late answer can neither publish evidence for a key the reviewer has left
+   * nor be mistaken for this wait's result.
+   */
   function waitForCoreBatch(
     proposalId: string,
     revisionIdentity: string | null,
+    options?: CoreSelectorBatchWaitOptions,
   ): Promise<CoreSelectorBatchOutcome> {
+    const signal = options?.signal
+    if (signal?.aborted) return Promise.resolve('aborted')
     const key = selectorKeyForProposal(activeProposal.value)
     if (
       !key ||
       !proposalIdsEqual(key.proposalId, proposalId) ||
       !nullableIdentifiersEqual(key.revisionIdentity, revisionIdentity)
     ) return Promise.resolve('unavailable')
+
+    if (!signal) return runCoreBatchWithSameActionRetry(key)
+
+    let onAbort: (() => void) | null = null
+    const aborted = new Promise<CoreSelectorBatchOutcome>((resolve) => {
+      onAbort = () => {
+        // Cancel the reads this wait is holding open before reporting, so the
+        // abandoned batch stops occupying the transport.
+        abortInFlightCoreBatchForKey(key)
+        resolve('aborted')
+      }
+      signal.addEventListener('abort', onAbort, { once: true })
+    })
+
+    return Promise.race([runCoreBatchWithSameActionRetry(key), aborted]).finally(() => {
+      if (onAbort) signal.removeEventListener('abort', onAbort)
+    })
+  }
+
+  /**
+   * One explicit wait retries a failed batch once, inside the same action
+   * (#2528). A second failure is reported honestly rather than retried again.
+   */
+  function runCoreBatchWithSameActionRetry(
+    key: SelectorKey,
+  ): Promise<CoreSelectorBatchOutcome> {
     const promise = ensureCoreBatch(key)
     return promise.then((outcome) => {
       if (

frontend/taskdeck-web/src/composables/useReviewProposals.ts

@@ -65,7 +65,26 @@ export const REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD = 3
  * intentionally keeps its historical `Promise<void>` contract for action
  * composables that only need a best-effort refresh.
  */
-export type ProposalLoadOutcome = 'landed' | 'failed' | 'superseded'
+export type ProposalLoadOutcome = 'landed' | 'failed' | 'superseded' | 'aborted'
+
+/**
+ * Cancellation and retry controls for an explicit queue read whose caller owns
+ * a deadline.
+ *
+ * `aborted` is reported instead of `failed` so a caller that cancelled its own
+ * read never mistakes it for a server or transport failure: only the caller
+ * knows why it aborted, and only the caller can tell a deadline apart from any
+ * other cancellation. The composable deliberately makes no such judgement.
+ *
+ * `skipRetry` matters for a deadline-bounded caller: the shared interceptor
+ * retries an idempotent read up to `MAX_RETRIES` times with doubling backoff,
+ * which can consume most of a caller's budget and turn a recoverable failure
+ * into a reported timeout. A bounded caller wants the first honest answer.
+ */
+export interface ProposalLoadOptions {
+  signal?: AbortSignal
+  skipRetry?: boolean
+}
 
 /**
  * Decision rules shared by every review surface (Paper deep-review and the
@@ -448,7 +467,7 @@ export function useReviewProposals() {
     }
   }
 
-  async function openProposalFromHash() {
+  async function openProposalFromHash(options?: ProposalLoadOptions) {
     if (proposalsLoading.value) return
     const proposalId = getProposalIdFromHash(route.hash)
     if (!proposalId) {
@@ -473,7 +492,12 @@ export function useReviewProposals() {
     }
 
     try {
-      const fetchedProposal = await automationApi.getProposal(proposalId)
+      const fetchedProposal = options
+        ? await automationApi.getProposal(proposalId, {
+            signal: options.signal,
+            skipRetry: options.skipRetry,
+          })
+        : await automationApi.getProposal(proposalId)
       if (!proposalIdsEqual(getProposalIdFromHash(route.hash), proposalId)) return
       // A route lookup may canonicalize GUID hex casing, but it may not return a
       // different record. Retain the hash as unavailable instead of upserting a
@@ -490,6 +514,9 @@ export function useReviewProposals() {
       await nextTick()
       await scrollToProposalFromHash()
     } catch (e: unknown) {
+      // A caller-owned cancellation is not a lookup failure: the deep-link read
+      // was cut short deliberately and its caller reports the real outcome.
+      if (options?.signal?.aborted) return
       if (!proposalIdsEqual(getProposalIdFromHash(route.hash), proposalId)) return
       if (isHttpNotFound(e)) {
         unavailableProposalId.value = proposalId
@@ -511,18 +538,33 @@ export function useReviewProposals() {
     await safeReplace({ name: 'workspace-review', query: route.query })
   }
 
-  async function loadProposalsWithOutcome(): Promise<ProposalLoadOutcome> {
+  async function loadProposalsWithOutcome(
+    options?: ProposalLoadOptions,
+  ): Promise<ProposalLoadOutcome> {
+    const signal = options?.signal
+    if (signal?.aborted) return 'aborted'
     reviewLoadPerf.start()
     const requestId = ++latestProposalLoadRequestId
     let outcome: ProposalLoadOutcome = 'landed'
 
     try {
       proposalsLoading.value = true
-      const loadedProposals = await automationApi.getProposals({
+      const filters = {
         limit: 200,
         boardId: activeBoardFilter.value || undefined,
-      })
+      }
+      // The second argument is forwarded ONLY when a caller supplied options,
+      // so every existing call site keeps its exact single-argument shape.
+      const loadedProposals = options
+        ? await automationApi.getProposals(filters, {
+            signal,
+            skipRetry: options.skipRetry,
+          })
+        : await automationApi.getProposals(filters)
       if (requestId !== latestProposalLoadRequestId) return 'superseded'
+      // An answer the caller stopped waiting for must not become the rendered
+      // authority behind its back, and proves nothing about queue freshness.
+      if (signal?.aborted) return 'aborted'
       proposals.value = loadedProposals
       // An explicit successful load is as trustworthy as a successful poll and
       // clears any older degraded indication without changing load semantics.
@@ -534,17 +576,23 @@ export function useReviewProposals() {
       if (accessWasRevoked) resumeQueueRefreshAfterPermissionRecovery()
     } catch (e: unknown) {
       if (requestId !== latestProposalLoadRequestId) return 'superseded'
+      // Cancellation by the caller's own deadline is not a queue failure. It
+      // must not raise the failure toast, and it must not be reported as
+      // `failed`, or the caller would blame the server for its own timeout.
+      if (signal?.aborted) return 'aborted'
       toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message)
       outcome = 'failed'
     } finally {
       if (requestId === latestProposalLoadRequestId) proposalsLoading.value = false
       reviewLoadPerf.end()
     }
 
+    if (signal?.aborted) return 'aborted'
     if (requestId === latestProposalLoadRequestId) {
-      await openProposalFromHash()
+      await openProposalFromHash(options)
     }
     if (requestId !== latestProposalLoadRequestId) return 'superseded'
+    if (signal?.aborted) return 'aborted'
     return outcome
   }
 

frontend/taskdeck-web/src/locales/en/review.ts

@@ -705,6 +705,8 @@ export default {
       'Review refreshed after your save attempt. Check the current evidence, then choose the action again.',
     revisionReviewUnavailable:
       'Review evidence could not be refreshed. No decision was made. Choose the current action again to retry.',
+    revisionReviewTimedOut:
+      'Refreshing the review took too long, so it was stopped. No decision was made. Choose the current action again to retry.',
     notRejectable: 'This proposal can no longer be rejected. Refresh review to see current status.',
     notEditable: 'This proposal can no longer be edited.',
     notDeferrable: 'This proposal can no longer be deferred.',

frontend/taskdeck-web/src/locales/es/review.ts

@@ -609,6 +609,8 @@ export default {
       'La revisión se actualizó después del intento de guardado. Comprueba las pruebas actuales y vuelve a elegir la acción.',
     revisionReviewUnavailable:
       'No se pudieron actualizar las pruebas de revisión. No se tomó ninguna decisión. Vuelve a elegir la acción actual para reintentarlo.',
+    revisionReviewTimedOut:
+      'Actualizar la revisión tardó demasiado y se detuvo. No se tomó ninguna decisión. Vuelve a elegir la acción actual para reintentarlo.',
     notRejectable:
       'Esta propuesta ya no se puede rechazar. Actualiza la revisión para ver el estado actual.',
     notEditable: 'Esta propuesta ya no se puede editar.',

frontend/taskdeck-web/src/locales/it/review.ts

@@ -612,6 +612,8 @@ export default {
       'Revisione aggiornata dopo il tentativo di salvataggio. Controlla le prove correnti, quindi scegli di nuovo l’azione.',
     revisionReviewUnavailable:
       'Non è stato possibile aggiornare le prove della revisione. Non è stata presa alcuna decisione. Scegli di nuovo l’azione corrente per riprovare.',
+    revisionReviewTimedOut:
+      'L’aggiornamento della revisione ha richiesto troppo tempo ed è stato interrotto. Non è stata presa alcuna decisione. Scegli di nuovo l’azione corrente per riprovare.',
     notRejectable:
       'Questa proposta non può più essere rifiutata. Aggiorna la revisione per vedere lo stato attuale.',
     notEditable: 'Questa proposta non può più essere modificata.',

frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts

@@ -580,6 +580,100 @@ describe('usePaperReviewSelectors', () => {
     expect(selectors.conflicts.value).toEqual([])
   })
 
+  // #2460 -- a caller that holds the decision lock owns the deadline. A read
+  // that never answers must release that caller, and must not be reported as a
+  // server failure it never was.
+  it('reports a caller-cancelled wait as aborted and stops the reads it held open', async () => {
+    mockAllEndpointsEmpty()
+    let historySignal: AbortSignal | undefined
+    vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce(
+      (_id: string, options?: { signal?: AbortSignal }) => {
+        historySignal = options?.signal
+        // Never settles: the exact stall a signal alone cannot recover from.
+        return new Promise(() => {})
+      },
+    )
+    const proposal = ref<ApiProposal | null>(makeProposal({ latestRevisionId: 'rev-1' }))
+    const selectors = usePaperReviewSelectors(computed(() => proposal.value))
+    const controller = new AbortController()
+
+    const wait = selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal })
+    await nextTick()
+    expect(historySignal?.aborted).toBe(false)
+
+    controller.abort()
+    await expect(wait).resolves.toBe('aborted')
+    // The abandoned batch is cancelled rather than left occupying the transport.
+    expect(historySignal?.aborted).toBe(true)
+  })
+
+  it('reports a wait whose caller has already given up as aborted without reading', async () => {
+    mockAllEndpointsEmpty()
+    const proposal = ref<ApiProposal | null>(makeProposal({ latestRevisionId: 'rev-1' }))
+    const selectors = usePaperReviewSelectors(computed(() => proposal.value))
+    await vi.waitFor(() => {
+      expect(proposalDeepReviewApi.getHistory).toHaveBeenCalled()
+    })
+    const callsBefore = vi.mocked(proposalDeepReviewApi.getHistory).mock.calls.length
+
+    const controller = new AbortController()
+    controller.abort()
+
+    await expect(
+      selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }),
+    ).resolves.toBe('aborted')
+    expect(vi.mocked(proposalDeepReviewApi.getHistory).mock.calls.length).toBe(callsBefore)
+  })
+
+  it('does not cancel the batch the reviewer moved to when an abandoned wait is cancelled', async () => {
+    mockAllEndpointsEmpty()
+    const historySignals: Array<AbortSignal | undefined> = []
+    vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce(
+      (_id: string, options?: { signal?: AbortSignal }) => {
+        historySignals.push(options?.signal)
+        return new Promise(() => {})
+      },
+    )
+    const proposal = ref<ApiProposal | null>(makeProposal({ latestRevisionId: 'rev-1' }))
+    const selectors = usePaperReviewSelectors(computed(() => proposal.value))
+    const controller = new AbortController()
+    const wait = selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal })
+    await nextTick()
+
+    vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce(
+      (_id: string, options?: { signal?: AbortSignal }) => {
+        historySignals.push(options?.signal)
+        return Promise.resolve([])
+      },
+    )
+    proposal.value = makeProposal({ latestRevisionId: 'rev-2' })
+    await nextTick()
+
+    controller.abort()
+    await expect(wait).resolves.toBe('aborted')
+
+    // The cancellation belongs to the rev-1 wait. The rev-2 batch the reviewer
+    // is now looking at must survive it intact.
+    expect(historySignals).toHaveLength(2)
+    expect(historySignals[1]?.aborted).toBe(false)
+    await expect(selectors.waitForCoreBatch('p-1', 'rev-2')).resolves.toBe('settled')
+  })
+
+  it('still reports a genuine read failure as failed when a signal is supplied', async () => {
+    mockAllEndpointsEmpty()
+    vi.mocked(proposalDeepReviewApi.getHistory)
+      .mockRejectedValueOnce(new Error('fail'))
+      .mockRejectedValueOnce(new Error('retry fail'))
+    const proposal = ref<ApiProposal | null>(makeProposal({ latestRevisionId: 'rev-1' }))
+    const selectors = usePaperReviewSelectors(computed(() => proposal.value))
+    const controller = new AbortController()
+
+    await expect(
+      selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }),
+    ).resolves.toBe('failed')
+    expect(controller.signal.aborted).toBe(false)
+  })
+
   it('drops the previous key evidence when the next batch fails', async () => {
     mockAllEndpointsEmpty()
     vi.mocked(proposalDeepReviewApi.getConflicts).mockResolvedValueOnce([