From 990236720f2b2d5e851e0ce9f71564b444adcbbf Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 4 Sep 2026 23:11:15 +0100 Subject: [PATCH 1/4] fix(review): bound the post-revision truth refresh with a caller-owned deadline --- .../composables/usePaperReviewSelectors.ts | 75 ++++- .../src/composables/useReviewProposals.ts | 46 ++- .../taskdeck-web/src/locales/en/review.ts | 2 + .../taskdeck-web/src/locales/es/review.ts | 2 + .../taskdeck-web/src/locales/it/review.ts | 2 + .../src/views/paper/PaperReviewView.vue | 313 ++++++++++++++---- 6 files changed, 366 insertions(+), 74 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts b/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts index 75f123556..32d7780f3 100644 --- a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts +++ b/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 @@ -143,6 +164,7 @@ export interface PaperReviewSelectors { waitForCoreBatch: ( proposalId: string, revisionIdentity: string | null, + options?: CoreSelectorBatchWaitOptions, ) => Promise } @@ -533,6 +555,17 @@ export function usePaperReviewSelectors( similarPastData.value = EMPTY_SIMILAR } + /** + * Cancel whatever core batch is currently in flight 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. + */ + function abortInFlightCoreBatch() { + abortController?.abort() + } + function invalidateCoreBatch() { fetchGeneration += 1 activeCoreBatch?.supersede() @@ -728,16 +761,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 { + 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((resolve) => { + onAbort = () => { + // Cancel the reads this wait is holding open before reporting, so the + // abandoned batch stops occupying the transport. + abortInFlightCoreBatch() + 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 { const promise = ensureCoreBatch(key) return promise.then((outcome) => { if ( diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 1134f50c8..6cb31b67d 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -65,7 +65,19 @@ export const REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD = 3 * intentionally keeps its historical `Promise` 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 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. + */ +export interface ProposalLoadOptions { + signal?: AbortSignal +} /** * Decision rules shared by every review surface (Paper deep-review and the @@ -448,7 +460,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 +485,9 @@ export function useReviewProposals() { } try { - const fetchedProposal = await automationApi.getProposal(proposalId) + const fetchedProposal = options?.signal + ? await automationApi.getProposal(proposalId, { signal: options.signal }) + : 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 +504,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,17 +528,26 @@ export function useReviewProposals() { await safeReplace({ name: 'workspace-review', query: route.query }) } - async function loadProposalsWithOutcome(): Promise { + async function loadProposalsWithOutcome( + options?: ProposalLoadOptions, + ): Promise { + 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 a signal, + // so every existing call site keeps its exact single-argument shape. + const loadedProposals = signal + ? await automationApi.getProposals(filters, { signal }) + : await automationApi.getProposals(filters) if (requestId !== latestProposalLoadRequestId) return 'superseded' proposals.value = loadedProposals // An explicit successful load is as trustworthy as a successful poll and @@ -534,6 +560,10 @@ 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 { @@ -541,10 +571,12 @@ export function useReviewProposals() { 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 } diff --git a/frontend/taskdeck-web/src/locales/en/review.ts b/frontend/taskdeck-web/src/locales/en/review.ts index 05ff0b2e3..f093a6fb8 100644 --- a/frontend/taskdeck-web/src/locales/en/review.ts +++ b/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.', diff --git a/frontend/taskdeck-web/src/locales/es/review.ts b/frontend/taskdeck-web/src/locales/es/review.ts index 9f256d2f7..468b2719c 100644 --- a/frontend/taskdeck-web/src/locales/es/review.ts +++ b/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.', diff --git a/frontend/taskdeck-web/src/locales/it/review.ts b/frontend/taskdeck-web/src/locales/it/review.ts index 35e279fa7..fe4031455 100644 --- a/frontend/taskdeck-web/src/locales/it/review.ts +++ b/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.', diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index d02f37c1f..d548f4551 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -1197,7 +1197,101 @@ let revisionEditEpoch = 0 let revisionReturnFocusEpoch: number | null = null let revisionReviewEpoch = 0 const revisionReviewRefreshEpochs = new Map() -const revisionReviewUnavailableKeys = ref>(new Set()) + +/** + * #2460 — the longest one post-revision truth refresh may hold the decision rail. + * + * The barrier is a COMPOSITE read: one authoritative queue read, then the six + * core evidence reads, plus at most one same-action retry of that batch. Every + * leg is bounded by the transport already, but a leg that never answers at all + * would hold the shared decision lock forever: the rail stays disabled, the + * keymap stays inert, and the reviewer has no way back to their own decision. + * So the CALLER caps the whole attempt, in the same spirit as + * `BOARD_REQUEST_TIMEOUT_MS` in `api/http.ts` caps one board read — long enough + * for a realistic composite round trip on a slow link, short enough that a + * locked rail still reads as work in progress rather than as broken. + * + * Timing out costs one more explicit action and nothing else: the per-proposal + * barrier is RETAINED, so the retry refreshes again before any decision can be + * made on pre-revision evidence. + */ +const POST_REVISION_REVIEW_DEADLINE_MS = 12_000 + +/** Race marker for {@link POST_REVISION_REVIEW_DEADLINE_MS}. */ +const REVISION_REVIEW_DEADLINE = 'post-revision-review-deadline' as const + +/** + * Why the barrier is telling the reviewer that evidence is not current. The two + * reasons need different copy: a failed read is a server or transport answer the + * reviewer cannot influence, a timed-out one is an attempt that may simply need + * longer. Collapsing them into one message would make the surface guess. + */ +type RevisionReviewUnavailableReason = 'failed' | 'timed-out' + +/** + * How one barrier attempt ended. Deliberately five distinct members rather than + * a boolean: only `refreshed` may clear the barrier, and the other four each + * call for different user-facing treatment. + */ +type RevisionReviewRefreshOutcome = + | 'refreshed' + | 'failed' + | 'timed-out' + | 'aborted' + | 'superseded' + +interface RevisionReviewAttempt { + /** Shared by the queue read and the six core selector reads. */ + signal: AbortSignal + /** Resolves with {@link REVISION_REVIEW_DEADLINE} when the attempt expires. */ + deadline: Promise + /** True once the deadline fired, which names an abort as a timeout. */ + readonly timedOut: boolean + dispose: () => void +} + +/** + * One cancellation contract per barrier attempt. + * + * Aborting is not enough on its own: a transport (or a test double) may ignore + * the signal and never settle, which is exactly the stall this guards against. + * So the deadline both aborts the shared controller AND resolves a race marker, + * and the caller releases its lock on whichever arrives first. + */ +function startRevisionReviewAttempt(): RevisionReviewAttempt { + const controller = new AbortController() + let timedOut = false + let resolveDeadline!: (value: typeof REVISION_REVIEW_DEADLINE) => void + const deadline = new Promise((resolve) => { + resolveDeadline = resolve + }) + const timer = setTimeout(() => { + timedOut = true + controller.abort() + resolveDeadline(REVISION_REVIEW_DEADLINE) + }, POST_REVISION_REVIEW_DEADLINE_MS) + return { + signal: controller.signal, + deadline, + get timedOut() { + return timedOut + }, + dispose() { + clearTimeout(timer) + }, + } +} + +/** + * Generation of the barrier attempt that currently owns the decision lock. A + * late attempt must not unlock a rail another attempt is holding, and must not + * write barrier state on behalf of a screen that has moved on. + */ +let revisionReviewAttemptGeneration = 0 + +const revisionReviewUnavailableKeys = ref>( + new Map(), +) function revisionReviewKey(proposalId: string): string { return proposalId.toLowerCase() @@ -1220,11 +1314,11 @@ function requireRevisionReviewRefresh(proposalId: string) { function setRevisionReviewUnavailable( proposalId: string, revisionIdentity: string | null, - unavailable: boolean, + reason: RevisionReviewUnavailableReason | null, ) { const key = revisionReviewUnavailableKey(proposalId, revisionIdentity) - const next = new Set(revisionReviewUnavailableKeys.value) - if (unavailable) next.add(key) + const next = new Map(revisionReviewUnavailableKeys.value) + if (reason) next.set(key, reason) else next.delete(key) revisionReviewUnavailableKeys.value = next } @@ -1240,15 +1334,33 @@ function isRevisionReviewUnavailableVisible(proposal: ApiProposal): boolean { ) } -const activeRevisionReviewUnavailable = computed(() => { - const proposal = activeProposal.value - return ( - !!proposal && - isRevisionReviewUnavailableVisible(proposal) && - revisionReviewUnavailableKeys.value.has( - revisionReviewUnavailableKey(proposal.id, proposalRevisionIdentity(proposal)), +const activeRevisionReviewUnavailableReason = computed( + () => { + const proposal = activeProposal.value + if (!proposal || !isRevisionReviewUnavailableVisible(proposal)) return null + return ( + revisionReviewUnavailableKeys.value.get( + revisionReviewUnavailableKey(proposal.id, proposalRevisionIdentity(proposal)), + ) ?? null ) - ) + }, +) + +const activeRevisionReviewUnavailable = computed( + () => activeRevisionReviewUnavailableReason.value !== null, +) + +/** + * The note the reviewer reads when the barrier could not confirm current truth. + * Both messages state the same non-negotiable fact — no decision was made — and + * differ only in why, and in whether waiting is worth another try. + */ +const revisionReviewUnavailableNote = computed(() => { + const reason = activeRevisionReviewUnavailableReason.value + if (!reason) return '' + return reason === 'timed-out' + ? t('review.toast.revisionReviewTimedOut') + : t('review.toast.revisionReviewUnavailable') }) watch( @@ -1268,7 +1380,7 @@ watch( // not inherit it. Clear it when that proposal leaves the actionable review // state, or when the active proposal itself is replaced. if (!sameProposal || !current?.eligible) { - setRevisionReviewUnavailable(previous.proposalId, previous.revisionIdentity, false) + setRevisionReviewUnavailable(previous.proposalId, previous.revisionIdentity, null) } }, ) @@ -1465,67 +1577,136 @@ function restoreApplyFocus(captured: HTMLElement | null) { }) } +/** + * Run one barrier attempt and report exactly how it ended. + * + * The barrier clears -- and the action becomes approvable on the NEXT explicit + * press -- only on `refreshed`: the authoritative proposal DTO and all six core + * evidence reads landed for one identity, status, expiry, defer state and + * effective revision. Every other ending leaves the epoch in place, so the next + * Approve or Apply refreshes again instead of deciding on pre-revision truth. + */ +async function runRevisionReviewRefresh( + proposal: ApiProposal, + requiredEpoch: number, + attempt: RevisionReviewAttempt, +): Promise { + const queueOutcome = await Promise.race([ + loadProposalsWithOutcome({ signal: attempt.signal }), + attempt.deadline, + ]) + if (queueOutcome === REVISION_REVIEW_DEADLINE) return 'timed-out' + // Only this attempt aborts that signal, so an abort it did not schedule + // itself can only be a teardown -- never a deadline the reviewer should retry. + if (queueOutcome === 'aborted') return attempt.timedOut ? 'timed-out' : 'aborted' + if (queueOutcome === 'failed') return 'failed' + if (queueOutcome !== 'landed') return 'superseded' + + const refreshed = activeProposal.value + if ( + !refreshed || + !proposalIdsEqual(refreshed.id, proposal.id) || + !isApplyActionable(refreshed) || + isProposalDeferred(refreshed) + ) return 'superseded' + + const status = normalizeProposalStatus(refreshed.status) + const revisionIdentity = proposalRevisionIdentity(refreshed) + const expiresAt = refreshed.expiresAt ?? null + const deferredUntil = refreshed.deferredUntil ?? null + const selectorOutcome = await Promise.race([ + selectors.waitForCoreBatch(refreshed.id, revisionIdentity, { signal: attempt.signal }), + attempt.deadline, + ]) + if (selectorOutcome === REVISION_REVIEW_DEADLINE) return 'timed-out' + if (selectorOutcome === 'aborted') return attempt.timedOut ? 'timed-out' : 'aborted' + if (selectorOutcome === 'failed') return 'failed' + if (selectorOutcome !== 'settled') return 'superseded' + + const verified = activeProposal.value + if ( + !verified || + !proposalIdsEqual(verified.id, proposal.id) || + normalizeProposalStatus(verified.status) !== status || + !revisionIdentitiesEqual(proposalRevisionIdentity(verified), revisionIdentity) || + (verified.expiresAt ?? null) !== expiresAt || + (verified.deferredUntil ?? null) !== deferredUntil || + !isApplyActionable(verified) || + isProposalDeferred(verified) + ) return 'superseded' + + const key = revisionReviewKey(proposal.id) + if (revisionReviewRefreshEpochs.get(key) !== requiredEpoch) return 'superseded' + revisionReviewRefreshEpochs.delete(key) + return 'refreshed' +} + +/** + * Key the barrier note to the proposal and revision now on screen. A reviewer + * who has already moved on must not inherit another screen's failure. + */ +function noteRevisionReviewOutcome( + proposal: ApiProposal, + reason: RevisionReviewUnavailableReason | null, +) { + const current = activeProposal.value + if (!current || !proposalIdsEqual(current.id, proposal.id)) return + setRevisionReviewUnavailable(current.id, proposalRevisionIdentity(current), reason) +} + +function applyRevisionReviewOutcome( + proposal: ApiProposal, + outcome: RevisionReviewRefreshOutcome, +) { + switch (outcome) { + case 'refreshed': + noteRevisionReviewOutcome(proposal, null) + toast.info(t('review.toast.revisionReviewRefreshed')) + return + case 'failed': + noteRevisionReviewOutcome(proposal, 'failed') + toast.error(t('review.toast.revisionReviewUnavailable')) + return + case 'timed-out': + noteRevisionReviewOutcome(proposal, 'timed-out') + toast.error(t('review.toast.revisionReviewTimedOut')) + return + case 'aborted': + case 'superseded': + // Nothing to tell the reviewer: the screen they were deciding on is gone + // or was replaced, and the barrier stays armed for whatever replaced it. + return + } +} + async function refreshRevisionReviewBeforeApply( proposal: ApiProposal, requiredEpoch: number, ) { const captured = applyReturnFocusEl + const generation = ++revisionReviewAttemptGeneration + const attempt = startRevisionReviewAttempt() applyGuardBusy.value = true revisionReviewRefreshBusy.value = true try { - const queueOutcome = await loadProposalsWithOutcome() - if (queueOutcome !== 'landed') return - - const refreshed = activeProposal.value - if ( - !refreshed || - !proposalIdsEqual(refreshed.id, proposal.id) || - !isApplyActionable(refreshed) || - isProposalDeferred(refreshed) - ) return - - const status = normalizeProposalStatus(refreshed.status) - const revisionIdentity = proposalRevisionIdentity(refreshed) - const expiresAt = refreshed.expiresAt ?? null - const deferredUntil = refreshed.deferredUntil ?? null - const selectorOutcome = await selectors.waitForCoreBatch( - refreshed.id, - revisionIdentity, - ) - if (selectorOutcome === 'failed') { - setRevisionReviewUnavailable(proposal.id, revisionIdentity, true) - toast.error(t('review.toast.revisionReviewUnavailable')) - return - } - if (selectorOutcome !== 'settled') return - - const verified = activeProposal.value - if ( - !verified || - !proposalIdsEqual(verified.id, proposal.id) || - normalizeProposalStatus(verified.status) !== status || - !revisionIdentitiesEqual(proposalRevisionIdentity(verified), revisionIdentity) || - (verified.expiresAt ?? null) !== expiresAt || - (verified.deferredUntil ?? null) !== deferredUntil || - !isApplyActionable(verified) || - isProposalDeferred(verified) - ) return - - const key = revisionReviewKey(proposal.id) - if (revisionReviewRefreshEpochs.get(key) !== requiredEpoch) return - revisionReviewRefreshEpochs.delete(key) - setRevisionReviewUnavailable(proposal.id, revisionIdentity, false) - toast.info(t('review.toast.revisionReviewRefreshed')) + const outcome = await runRevisionReviewRefresh(proposal, requiredEpoch, attempt) + // A late attempt reports nothing: a newer attempt owns the barrier, the + // rail and the note, and this one's answer is by definition older. + if (generation !== revisionReviewAttemptGeneration) return + applyRevisionReviewOutcome(proposal, outcome) } finally { - revisionReviewRefreshBusy.value = false - applyGuardBusy.value = false - applyReturnFocusEl = null - // The preflight consumes this action unconditionally. Return focus only - // after the shared lock releases, so a keyboard reviewer can inspect the - // refreshed evidence and deliberately invoke the current action again. - void nextTick(() => { - restoreApplyFocus(captured) - }) + attempt.dispose() + if (generation === revisionReviewAttemptGeneration) { + revisionReviewRefreshBusy.value = false + applyGuardBusy.value = false + applyReturnFocusEl = null + // The preflight consumes this action unconditionally. Return focus only + // after the shared lock releases, so a keyboard reviewer can inspect the + // refreshed evidence and deliberately invoke the current action again. + void nextTick(() => { + restoreApplyFocus(captured) + }) + } } } @@ -2354,7 +2535,7 @@ async function onClearBoardScope() { aria-atomic="true" data-testid="paper-review-evidence-unavailable" > - {{ $t('review.toast.revisionReviewUnavailable') }} + {{ revisionReviewUnavailableNote }}

Date: Fri, 4 Sep 2026 23:16:10 +0100 Subject: [PATCH 2/4] test(review): prove the post-revision refresh deadline, retry and late-answer suppression --- .../src/composables/useReviewProposals.ts | 3 + .../usePaperReviewSelectors.spec.ts | 60 ++++ .../composables/useReviewProposals.spec.ts | 58 ++++ .../paper/review/PaperReviewView.spec.ts | 291 ++++++++++++++++++ 4 files changed, 412 insertions(+) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 6cb31b67d..c0c1cc0d6 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -549,6 +549,9 @@ export function useReviewProposals() { ? await automationApi.getProposals(filters, { signal }) : 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. diff --git a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts index fb8b030f0..50ca04dac 100644 --- a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts @@ -580,6 +580,66 @@ 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(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(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('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(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([ diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index b5cc8f3b0..701cc15c4 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -697,6 +697,64 @@ describe('useReviewProposals', () => { expect(rp.proposalsLoading.value).toBe(false) }) + // #2460 -- a caller that owns a deadline needs its own cancellation told + // apart from a server failure, or a timeout would be blamed on the backend. + it('reports a caller-aborted explicit load as aborted rather than failed', async () => { + const controller = new AbortController() + let rejectRead!: (error: Error) => void + mockAutomationApi.getProposals.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectRead = reject + }), + ) + const rp = useReviewProposals() + + const load = rp.loadProposalsWithOutcome({ signal: controller.signal }) + controller.abort() + rejectRead(new Error('canceled')) + + await expect(load).resolves.toBe('aborted') + expect(mockToast.error).not.toHaveBeenCalled() + expect(mockAutomationApi.getProposals).toHaveBeenCalledWith( + expect.objectContaining({ limit: 200 }), + expect.objectContaining({ signal: controller.signal }), + ) + }) + + it('does not issue an explicit load whose caller has already given up', async () => { + const controller = new AbortController() + controller.abort() + const rp = useReviewProposals() + + await expect( + rp.loadProposalsWithOutcome({ signal: controller.signal }), + ).resolves.toBe('aborted') + expect(mockAutomationApi.getProposals).not.toHaveBeenCalled() + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it('reports an aborted deep-link leg as aborted and raises no lookup error', async () => { + mockRoute.hash = '#proposal-p-remote' + const controller = new AbortController() + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + let rejectLookup!: (error: Error) => void + mockAutomationApi.getProposal.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectLookup = reject + }), + ) + const rp = useReviewProposals() + + const load = rp.loadProposalsWithOutcome({ signal: controller.signal }) + await Promise.resolve() + await Promise.resolve() + controller.abort() + rejectLookup(new Error('canceled')) + + await expect(load).resolves.toBe('aborted') + expect(mockToast.error).not.toHaveBeenCalled() + }) + it('does not report landed until its deep-link lookup completes', async () => { mockRoute.hash = '#proposal-p-remote' mockAutomationApi.getProposals.mockResolvedValueOnce([]) diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index b2611f2b4..7411dc3b0 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -5461,4 +5461,295 @@ describe('PaperReviewView', () => { expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() }) }) + + describe('post-revision truth refresh deadline (#2460)', () => { + // Mirrors POST_REVISION_REVIEW_DEADLINE_MS in PaperReviewView.vue. Kept as a + // literal so raising the view's cap has to be a deliberate change here too. + const DEADLINE_MS = 12_000 + const TIMED_OUT_COPY = + 'Refreshing the review took too long, so it was stopped. No decision was made. Choose the current action again to retry.' + const FAILED_COPY = + 'Review evidence could not be refreshed. No decision was made. Choose the current action again to retry.' + const REFRESHED_COPY = + 'Review refreshed after your save attempt. Check the current evidence, then choose the action again.' + const REVISED_PAYLOAD = '{"operations":[{"sequence":0,"actionType":"CreateCard"}]}' + + /** Arm the post-revision barrier the way the composer does. */ + async function armBarrier( + wrapper: Awaited>, + proposalId: string, + revisionId: string, + ) { + const now = new Date().toISOString() + mocks.createRevision.mockResolvedValueOnce({ + id: revisionId, + proposalId, + revisionNumber: 1, + editorUserId: 'u-1', + revisedPayload: REVISED_PAYLOAD, + revisedAt: now, + reason: 'Deadline coverage', + createdAt: now, + }) + await wrapper.get('[data-testid="decision-edit"]').trigger('click') + await flushPromises() + wrapper.findComponent(ReviewRevisionEditor).vm.$emit('save', { + revisedPayload: REVISED_PAYLOAD, + reason: 'Deadline coverage', + }) + await flushPromises() + } + + function rejectEveryCoreRead(times: number) { + const reads = [ + mocks.getProvenance, + mocks.getConfidence, + mocks.getSideEffects, + mocks.getConflicts, + mocks.getHistory, + mocks.getSimilarPast, + ] + for (const read of reads) { + for (let attempt = 0; attempt < times; attempt += 1) { + read.mockRejectedValueOnce(new Error('core evidence unavailable')) + } + } + } + + it('releases the rail on its deadline when the authoritative queue read never answers', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'stalled-queue' }) + const refreshed = makeProposal({ + id: 'stalled-queue', + latestRevisionId: 'rev-stalled-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'stalled-queue', + status: 'Approved', + approvedRevisionId: 'rev-stalled-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'stalled-queue', 'rev-stalled-1') + + let resolveStalled!: (proposals: Proposal[]) => void + mocks.getProposals.mockImplementationOnce( + () => new Promise((resolve) => { resolveStalled = resolve }), + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + // The shared decision lock is held while the read is outstanding. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeDefined() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + // The rail is handed back with a truthful, retryable explanation. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.errorToast).toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + + // The abandoned attempt answering late changes nothing: it neither + // becomes the rendered queue nor clears the barrier it gave up on. + resolveStalled([refreshed]) + await flushPromises() + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // The barrier survived the timeout, so the next explicit action still + // refreshes rather than deciding on pre-revision evidence. + mocks.getProposals.mockResolvedValue([refreshed]) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // Only the second explicit action after a clean refresh decides. + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('releases the rail on its deadline when a core evidence read never answers', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'stalled-evidence' }) + const refreshed = makeProposal({ + id: 'stalled-evidence', + latestRevisionId: 'rev-evidence-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'stalled-evidence', + status: 'Approved', + approvedRevisionId: 'rev-evidence-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'stalled-evidence', 'rev-evidence-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + let resolveHistory!: (rows: unknown[]) => void + mocks.getHistory.mockImplementationOnce( + () => new Promise((resolve) => { resolveHistory = resolve }), + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeDefined() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.errorToast).toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + + // A late evidence answer may repopulate the panels, but it must not + // clear a barrier the timed-out attempt no longer owns. + resolveHistory([]) + await flushPromises() + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('names a partial core evidence failure as a failure, never as a timeout', async () => { + const original = makeProposal({ id: 'partial-failure' }) + const refreshed = makeProposal({ + id: 'partial-failure', + latestRevisionId: 'rev-partial-1', + }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'partial-failure', 'rev-partial-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + // One of the six rejects, through the automatic batch and its same-action retry. + mocks.getHistory + .mockRejectedValueOnce(new Error('history unavailable')) + .mockRejectedValueOnce(new Error('history still unavailable')) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(mocks.errorToast).toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Review evidence could not be refreshed', + ) + // The rail is unlocked for the retry even though the refresh failed. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('names a total core evidence failure as a failure and retains the barrier', async () => { + const original = makeProposal({ id: 'total-failure' }) + const refreshed = makeProposal({ + id: 'total-failure', + latestRevisionId: 'rev-total-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'total-failure', + status: 'Approved', + approvedRevisionId: 'rev-total-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'total-failure', 'rev-total-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + rejectEveryCoreRead(2) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(mocks.errorToast).toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(true) + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // The barrier is retained: the next action refreshes, and only the one + // after a clean refresh may approve. + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('disarms its deadline once the refresh lands, so no late timeout note appears', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'clean-refresh' }) + const refreshed = makeProposal({ + id: 'clean-refresh', + latestRevisionId: 'rev-clean-1', + }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'clean-refresh', 'rev-clean-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + }) }) From d80fb7eb7ea11e53ce4a234e5da2bc1c3b4d3339 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Fri, 4 Sep 2026 23:21:21 +0100 Subject: [PATCH 3/4] fix(review): scope the barrier abort to the key its wait held open --- .../composables/usePaperReviewSelectors.ts | 19 +++++++---- .../usePaperReviewSelectors.spec.ts | 34 +++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts b/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts index 32d7780f3..36e200840 100644 --- a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts +++ b/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts @@ -556,13 +556,18 @@ export function usePaperReviewSelectors( } /** - * Cancel whatever core batch is currently in flight 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. + * 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 abortInFlightCoreBatch() { + function abortInFlightCoreBatchForKey(key: SelectorKey) { + if (!activeCoreBatch || !selectorKeysEqual(activeCoreBatch.key, key)) return abortController?.abort() } @@ -793,7 +798,7 @@ export function usePaperReviewSelectors( onAbort = () => { // Cancel the reads this wait is holding open before reporting, so the // abandoned batch stops occupying the transport. - abortInFlightCoreBatch() + abortInFlightCoreBatchForKey(key) resolve('aborted') } signal.addEventListener('abort', onAbort, { once: true }) diff --git a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts index 50ca04dac..1a81dac64 100644 --- a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts @@ -625,6 +625,40 @@ describe('usePaperReviewSelectors', () => { 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 = [] + vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce( + (_id: string, options?: { signal?: AbortSignal }) => { + historySignals.push(options?.signal) + return new Promise(() => {}) + }, + ) + const proposal = ref(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) From 35b260b257cc260e01a243237968d9f8d04066d5 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 02:16:17 +0100 Subject: [PATCH 4/4] fix(review): guard the barrier report, cancel the attempt on unmount, skip retries --- .../src/composables/useReviewProposals.ts | 25 +++- .../composables/useReviewProposals.spec.ts | 27 +++- .../paper/review/PaperReviewView.spec.ts | 86 +++++++++++++ .../src/views/paper/PaperReviewView.vue | 117 ++++++++++++++---- 4 files changed, 221 insertions(+), 34 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index c0c1cc0d6..ab7a2240e 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -68,15 +68,22 @@ export const REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD = 3 export type ProposalLoadOutcome = 'landed' | 'failed' | 'superseded' | 'aborted' /** - * Cancellation for an explicit queue read whose caller owns a deadline. + * 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 } /** @@ -485,8 +492,11 @@ export function useReviewProposals() { } try { - const fetchedProposal = options?.signal - ? await automationApi.getProposal(proposalId, { signal: options.signal }) + 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 @@ -543,10 +553,13 @@ export function useReviewProposals() { limit: 200, boardId: activeBoardFilter.value || undefined, } - // The second argument is forwarded ONLY when a caller supplied a signal, + // The second argument is forwarded ONLY when a caller supplied options, // so every existing call site keeps its exact single-argument shape. - const loadedProposals = signal - ? await automationApi.getProposals(filters, { signal }) + 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 diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index 701cc15c4..5dc5750b0 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -721,6 +721,22 @@ describe('useReviewProposals', () => { ) }) + // A deadline-bounded caller must not spend its budget in the shared retry + // interceptor's doubling backoff and then be reported as a timeout. + it('forwards a caller opt-out of the shared retry interceptor', async () => { + const controller = new AbortController() + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + const rp = useReviewProposals() + + await expect( + rp.loadProposalsWithOutcome({ signal: controller.signal, skipRetry: true }), + ).resolves.toBe('landed') + expect(mockAutomationApi.getProposals).toHaveBeenCalledWith( + expect.objectContaining({ limit: 200 }), + expect.objectContaining({ signal: controller.signal, skipRetry: true }), + ) + }) + it('does not issue an explicit load whose caller has already given up', async () => { const controller = new AbortController() controller.abort() @@ -745,7 +761,10 @@ describe('useReviewProposals', () => { ) const rp = useReviewProposals() - const load = rp.loadProposalsWithOutcome({ signal: controller.signal }) + const load = rp.loadProposalsWithOutcome({ + signal: controller.signal, + skipRetry: true, + }) await Promise.resolve() await Promise.resolve() controller.abort() @@ -753,6 +772,12 @@ describe('useReviewProposals', () => { await expect(load).resolves.toBe('aborted') expect(mockToast.error).not.toHaveBeenCalled() + // The deep-link leg is part of the same explicit read, so it carries the + // same cancellation and retry contract rather than running unbounded. + expect(mockAutomationApi.getProposal).toHaveBeenCalledWith( + 'p-remote', + expect.objectContaining({ signal: controller.signal, skipRetry: true }), + ) }) it('does not report landed until its deep-link lookup completes', async () => { diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index 7411dc3b0..a7bcbe4bc 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -5723,6 +5723,92 @@ describe('PaperReviewView', () => { wrapper.unmount() }) + it('reports nothing when the reviewer moved to another proposal before the deadline', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const proposalA = makeProposal({ id: 'aaa-moved', summary: 'First proposal' }) + const proposalB = makeProposal({ id: 'bbb-moved', summary: 'Second proposal' }) + const wrapper = await mountView([proposalA, proposalB]) + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await armBarrier(wrapper, 'aaa-moved', 'rev-moved-1') + + mocks.getProposals.mockImplementationOnce(() => new Promise(() => {})) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + + // The queue rail is not disabled by the decision lock, so the reviewer + // can be looking at B when A's attempt ends. + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + // Neither half of the report may land on B. "Choose the current action + // again" read on B would be an instruction to approve a proposal that + // has no barrier at all. + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + + // A's barrier survived, so returning to it still refreshes before deciding. + mocks.getProposals.mockResolvedValue([ + makeProposal({ id: 'aaa-moved', summary: 'First proposal', latestRevisionId: 'rev-moved-1' }), + proposalB, + ]) + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('cancels an in-flight attempt on unmount so its deadline never reports', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'unmounted-attempt' }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'unmounted-attempt', 'rev-unmounted-1') + + let barrierSignal: AbortSignal | undefined + mocks.getProposals.mockImplementationOnce( + (_filters?: unknown, options?: { signal?: AbortSignal }) => { + barrierSignal = options?.signal + return new Promise(() => {}) + }, + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(barrierSignal?.aborted).toBe(false) + + wrapper.unmount() + // The reads the attempt was holding open are cancelled with the route. + expect(barrierSignal?.aborted).toBe(true) + + mocks.errorToast.mockClear() + mocks.infoToast.mockClear() + await vi.advanceTimersByTimeAsync(DEADLINE_MS * 2) + await flushPromises() + + // The deadline timer went with the component: nothing reports onto + // whatever page the reviewer navigated to. + expect(mocks.errorToast).not.toHaveBeenCalled() + expect(mocks.infoToast).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + it('disarms its deadline once the refresh lands, so no late timeout note appears', async () => { vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) try { diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index d548f4551..233757957 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -1211,9 +1211,19 @@ const revisionReviewRefreshEpochs = new Map() * for a realistic composite round trip on a slow link, short enough that a * locked rail still reads as work in progress rather than as broken. * - * Timing out costs one more explicit action and nothing else: the per-proposal - * barrier is RETAINED, so the retry refreshes again before any decision can be - * made on pre-revision evidence. + * What this budget assumes about retries, because they are what make a cap of + * this size tight: the queue read (and its deep-link leg) is issued with + * `skipRetry`, so it costs one round trip and reports the first honest answer. + * The six core evidence reads are NOT, because they are served by the batch + * `usePaperReviewSelectors` shares with its own automatic refresh — a per-call + * opt-out there would apply only when the barrier happened to be the batch's + * creator, which is worse than retrying uniformly. So this budget must still + * cover one shared-interceptor retry pass on the evidence leg, and a genuinely + * flaky evidence read can exhaust it. + * + * That is an acceptable trade because timing out costs one more explicit action + * and nothing else: the per-proposal barrier is RETAINED, so the retry refreshes + * again before any decision can be made on pre-revision evidence. */ const POST_REVISION_REVIEW_DEADLINE_MS = 12_000 @@ -1278,10 +1288,21 @@ function startRevisionReviewAttempt(): RevisionReviewAttempt { }, dispose() { clearTimeout(timer) + // Cancel what this attempt still holds open. Clearing the timer alone + // would leave an early failed or superseded ending -- and an unmount -- + // with reads still running for a decision nobody is waiting on. + controller.abort() }, } } +/** + * The attempt currently holding the decision lock, so teardown can cancel it. + * A barrier attempt outlives its route otherwise: its deadline timer would fire + * after the reviewer has navigated away and report on whatever page they are on. + */ +let activeRevisionReviewAttempt: RevisionReviewAttempt | null = null + /** * Generation of the barrier attempt that currently owns the decision lock. A * late attempt must not unlock a rail another attempt is holding, and must not @@ -1585,14 +1606,20 @@ function restoreApplyFocus(captured: HTMLElement | null) { * evidence reads landed for one identity, status, expiry, defer state and * effective revision. Every other ending leaves the epoch in place, so the next * Approve or Apply refreshes again instead of deciding on pre-revision truth. + * + * This decides the outcome and writes nothing. `clearRevisionReviewBarrier` is + * what actually clears the epoch, once the caller has checked that this attempt + * still owns the barrier. */ async function runRevisionReviewRefresh( proposal: ApiProposal, - requiredEpoch: number, attempt: RevisionReviewAttempt, ): Promise { const queueOutcome = await Promise.race([ - loadProposalsWithOutcome({ signal: attempt.signal }), + // `skipRetry`: the interceptor's doubling backoff can spend most of this + // attempt's budget re-asking a question the barrier would rather answer + // honestly and let the reviewer retry deliberately. + loadProposalsWithOutcome({ signal: attempt.signal, skipRetry: true }), attempt.deadline, ]) if (queueOutcome === REVISION_REVIEW_DEADLINE) return 'timed-out' @@ -1635,6 +1662,22 @@ async function runRevisionReviewRefresh( isProposalDeferred(verified) ) return 'superseded' + return 'refreshed' +} + +/** + * Clear the barrier for one attempt. This is the ONE piece of state whose loss + * matters -- everything else the attempt writes is a note or a toast -- so it is + * written last, behind the attempt-generation guard in the caller. + * + * The epoch is re-read here rather than trusted from the verification block: a + * save that landed while this attempt was resolving arms a NEWER epoch, and + * deleting that would drop a barrier this attempt never satisfied. + */ +function clearRevisionReviewBarrier( + proposal: ApiProposal, + requiredEpoch: number, +): RevisionReviewRefreshOutcome { const key = revisionReviewKey(proposal.id) if (revisionReviewRefreshEpochs.get(key) !== requiredEpoch) return 'superseded' revisionReviewRefreshEpochs.delete(key) @@ -1642,40 +1685,44 @@ async function runRevisionReviewRefresh( } /** - * Key the barrier note to the proposal and revision now on screen. A reviewer - * who has already moved on must not inherit another screen's failure. + * Report one attempt's ending to the reviewer. + * + * The note and the toast are ONE report about ONE proposal, so they share ONE + * guard. The queue rail stays clickable while the barrier reads (only the + * decision rail and the keymap are locked), so a reviewer can be looking at B + * when A's attempt ends. Every message here says "choose the current action + * again" -- addressed to B, that is an instruction to press Apply on a proposal + * with no barrier, which falls straight through to a real decision. So a report + * that cannot be attributed to what is on screen is not shown at all. + * + * Withholding it costs nothing: the barrier state itself is written separately + * and independently of what is displayed, so returning to A still refreshes. */ -function noteRevisionReviewOutcome( +function applyRevisionReviewOutcome( proposal: ApiProposal, - reason: RevisionReviewUnavailableReason | null, + outcome: RevisionReviewRefreshOutcome, ) { + // Nothing to tell the reviewer: the screen they were deciding on is gone or + // was replaced, and the barrier stays armed for whatever replaced it. + if (outcome === 'aborted' || outcome === 'superseded') return + const current = activeProposal.value if (!current || !proposalIdsEqual(current.id, proposal.id)) return - setRevisionReviewUnavailable(current.id, proposalRevisionIdentity(current), reason) -} + const revisionIdentity = proposalRevisionIdentity(current) -function applyRevisionReviewOutcome( - proposal: ApiProposal, - outcome: RevisionReviewRefreshOutcome, -) { switch (outcome) { case 'refreshed': - noteRevisionReviewOutcome(proposal, null) + setRevisionReviewUnavailable(current.id, revisionIdentity, null) toast.info(t('review.toast.revisionReviewRefreshed')) return case 'failed': - noteRevisionReviewOutcome(proposal, 'failed') + setRevisionReviewUnavailable(current.id, revisionIdentity, 'failed') toast.error(t('review.toast.revisionReviewUnavailable')) return case 'timed-out': - noteRevisionReviewOutcome(proposal, 'timed-out') + setRevisionReviewUnavailable(current.id, revisionIdentity, 'timed-out') toast.error(t('review.toast.revisionReviewTimedOut')) return - case 'aborted': - case 'superseded': - // Nothing to tell the reviewer: the screen they were deciding on is gone - // or was replaced, and the barrier stays armed for whatever replaced it. - return } } @@ -1686,16 +1733,24 @@ async function refreshRevisionReviewBeforeApply( const captured = applyReturnFocusEl const generation = ++revisionReviewAttemptGeneration const attempt = startRevisionReviewAttempt() + activeRevisionReviewAttempt = attempt applyGuardBusy.value = true revisionReviewRefreshBusy.value = true try { - const outcome = await runRevisionReviewRefresh(proposal, requiredEpoch, attempt) - // A late attempt reports nothing: a newer attempt owns the barrier, the - // rail and the note, and this one's answer is by definition older. + const outcome = await runRevisionReviewRefresh(proposal, attempt) + // A late attempt writes nothing: a newer attempt (or an unmount) owns the + // barrier, the rail and the note, and this one's answer is by definition + // older. The barrier clear sits behind this guard for that reason. if (generation !== revisionReviewAttemptGeneration) return - applyRevisionReviewOutcome(proposal, outcome) + applyRevisionReviewOutcome( + proposal, + outcome === 'refreshed' + ? clearRevisionReviewBarrier(proposal, requiredEpoch) + : outcome, + ) } finally { attempt.dispose() + if (activeRevisionReviewAttempt === attempt) activeRevisionReviewAttempt = null if (generation === revisionReviewAttemptGeneration) { revisionReviewRefreshBusy.value = false applyGuardBusy.value = false @@ -2401,6 +2456,14 @@ onMounted(() => { }) onUnmounted(() => { + // #2460: a barrier attempt outlives its route unless it is cancelled here. + // Bumping the generation first stops any outcome being applied to a surface + // that no longer exists; disposing then clears the deadline timer -- whose + // ending would otherwise toast over whatever page the reviewer moved to -- + // and aborts the reads it was holding open. + revisionReviewAttemptGeneration += 1 + activeRevisionReviewAttempt?.dispose() + activeRevisionReviewAttempt = null // Prevent an in-flight metadata open or save continuation from restoring // focus into a component that no longer owns the document. invalidateRevisionOpening()