Skip to content

fix: add stop-acknowledgement timeout to prevent stale recording state #9024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jasonnnz merged 1 commit into from
Feb 25, 2026
Merged

fix: add stop-acknowledgement timeout to prevent stale recording state #9024

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions assistant/src/daemon/handlers/recording.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ import { log, findSocketForSession, defineHandlers, type HandlerContext } from '
import * as conversationStore from '../../memory/conversation-store.js';
import { uploadFileBackedAttachment, linkAttachmentToMessage } from '../../memory/attachments-store.js';

// ─── Constants ───────────────────────────────────────────────────────────────

/** How long to wait (ms) for a client to acknowledge a recording_stop before
* automatically cleaning up stale map entries. Prevents a missing client ack
* from permanently blocking all future recordings. */
const STOP_ACK_TIMEOUT_MS = 30_000;

// ─── Deterministic maps ──────────────────────────────────────────────────────
// These ensure stop resolves the exact active recording for a conversation,
// prevent ambiguous cross-thread stop behavior, and maintain conversation
Expand All @@ -18,6 +25,9 @@ const standaloneRecordingConversationId = new Map<string, string>();
/** Maps conversationId -> recordingId (active recording). */
const recordingOwnerByConversation = new Map<string, string>();

/** Pending stop-acknowledgement timeouts keyed by recordingId. */
const pendingStopTimeouts = new Map<string, NodeJS.Timeout>();

// ─── Start ───────────────────────────────────────────────────────────────────

/**
Expand Down Expand Up @@ -110,12 +120,31 @@ export function handleRecordingStop(
recordingId,
});

// Start a timeout so that if the client never acknowledges the stop (e.g.
// client bug, app freeze), we automatically clean up the maps and unblock
// future recordings.
const timeoutHandle = setTimeout(() => {
pendingStopTimeouts.delete(recordingId);
log.warn({ recordingId, conversationId: ownerConversationId, timeoutMs: STOP_ACK_TIMEOUT_MS }, 'Stop-acknowledgement timeout fired — cleaning up stale recording state');
cleanupMaps(recordingId, ownerConversationId);
}, STOP_ACK_TIMEOUT_MS);
pendingStopTimeouts.set(recordingId, timeoutHandle);

log.info({ recordingId, conversationId }, 'Standalone recording stop sent');
return recordingId;
}

// ─── Internal helpers ─────────────────────────────────────────────────────────

/** Cancel a pending stop-acknowledgement timeout for a recording, if any. */
function cancelStopTimeout(recordingId: string): void {
const handle = pendingStopTimeouts.get(recordingId);
if (handle) {
clearTimeout(handle);
pendingStopTimeouts.delete(recordingId);
}
}

/** Remove a recording from both deterministic maps. */
function cleanupMaps(recordingId: string, conversationId: string | undefined): void {
standaloneRecordingConversationId.delete(recordingId);
Expand Down Expand Up @@ -151,6 +180,9 @@ function handleRecordingStatus(
return;
}

// The client acknowledged this recording — cancel any pending stop timeout.
cancelStopTimeout(recordingId);
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Stop-ack timeout cancelled by non-terminal started status, defeating the safety mechanism

cancelStopTimeout(recordingId) at line 184 is called unconditionally for every recording_status message, including status: 'started'. A started status is the client acknowledging it began recording — not that it stopped. This creates a race condition that defeats the entire purpose of this PR.

Race condition scenario
  1. Server calls handleRecordingStart → sends recording_start to client
  2. Server calls handleRecordingStop → sends recording_stop to client → starts 30s timeout at recording.ts:126-131
  3. Client's started ack arrives (was in-flight or delayed) → handleRecordingStatus runs with status: 'started' → cancelStopTimeout(recordingId) at line 184 cancels the timeout
  4. Client is frozen/crashed → never sends stopped → timeout was already cancelled → maps are never cleaned up → future recordings permanently blocked

The fix should only cancel the timeout for terminal statuses (stopped or failed), not for started. The cancelStopTimeout call should be moved inside the case 'stopped' and case 'failed' branches of the switch statement, or guarded with if (msg.status !== 'started').

Impact: The exact scenario this PR aims to prevent (stale recording state blocking future recordings) can still occur when a started ack races with a stop request.

Prompt for agents 
In assistant/src/daemon/handlers/recording.ts, move the `cancelStopTimeout(recordingId)` call from line 184 (before the switch statement) into the two terminal status branches where it belongs:

1. Remove line 184: `cancelStopTimeout(recordingId);`
2. Add `cancelStopTimeout(recordingId);` at the beginning of the `case 'stopped'` block (around line 196, before the log.info call)
3. Add `cancelStopTimeout(recordingId);` at the beginning of the `case 'failed'` block (around line 312, before the log.warn call)

This ensures the timeout is only cancelled when the client actually acknowledges the stop (with a terminal status), not when a `started` ack races with a stop request.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Cancel stop timeout only on terminal recording statuses

handleRecordingStatus clears the stop-ack timeout before checking msg.status, so a late started event can cancel the watchdog that was just armed by handleRecordingStop. In a fast start→stop sequence (or any event-loop delay), this leaves no timeout guarding the stop acknowledgement; if the client then never emits stopped/failed, the recording maps remain stuck indefinitely and future recordings are blocked again. The timeout cancellation should be limited to terminal statuses that actually acknowledge stop completion.

Useful? React with 👍 / 👎.


// Use the reporting socket (which delivered this message) as the primary
// recipient. Fall back to session-based lookup if the user switched sessions.
const notifySocket = reportingSocket ?? findSocketForSession(conversationId, ctx);
Expand Down Expand Up @@ -319,6 +351,7 @@ export function cleanupRecordingsOnDisconnect(
// or if the owner conversation has no socket bound at all.
if (!ownerSocket || ownerSocket === disconnectedSocket) {
log.warn({ conversationId: convId, recordingId: recId }, 'Cleaning up recording state for disconnected socket');
cancelStopTimeout(recId);
standaloneRecordingConversationId.delete(recId);
recordingOwnerByConversation.delete(convId);
}
Expand All @@ -329,6 +362,10 @@ export function cleanupRecordingsOnDisconnect(

/** Reset module-level state. Only for use in tests. */
export function __resetRecordingState(): void {
for (const handle of pendingStopTimeouts.values()) {
clearTimeout(handle);
}
pendingStopTimeouts.clear();
standaloneRecordingConversationId.clear();
recordingOwnerByConversation.clear();
}
Expand Down