Skip to content

Commit b1087fb

Browse files
committed
fix(daemon): latch terminal run status against equal-length stale snapshots
The length-based freshness check only caught snapshots that SHRANK the stored event list. But the daemon writes the terminal run_status in a separate UPDATE (no event appended), so a web snapshot captured after the final event but before that write has the SAME event count while still carrying a non-terminal status — it previously flowed through and regressed the stored status back to 'running', sticking conversations in a nonterminal state (looper review on #6418). Make terminal run status a daemon-owned latch: once the stored status is terminal (succeeded/failed/canceled), a client PUT that carries any other status is rejected for the daemon-owned fields. Combined with the existing event-shrink rule, and preserving role/runId on both paths. Adds a regression test: run completes (terminal status + events), then an equal-length snapshot with 'running' is replayed and must not regress the terminal status or ownership fields.
1 parent ae731f3 commit b1087fb

2 files changed

Lines changed: 102 additions & 16 deletions

File tree

apps/daemon/src/routes/project/conversations.ts

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { type ChatSessionMode } from '@open-design/contracts';
33
import { readAnalyticsContext } from '../../analytics.js';
44
import { backfillBrandExtractionTranscriptForProject } from '../../brands/index.js';
55
import type { RouteDeps } from '../../server-context.js';
6+
import { TERMINAL_RUN_STATUSES } from '../../runtimes/runs.js';
67
import { registerProjectCommentRoutes } from './comments.js';
78
import { cancelRunsOwnedBy } from './cancel-owned-runs.js';
89

@@ -199,33 +200,48 @@ export function registerProjectConversationRoutes(app: Express, ctx: RegisterPro
199200
// switch, then PUT after the daemon appended more events) must never regress
200201
// those fields — that's how the early `status:model` event got wiped.
201202
//
202-
// The guard is a "no regression" rule, not a blanket write-ownership rule:
203-
// run events are append-only, so a stale snapshot can only SHRINK the stored
204-
// list. We preserve the stored events/content/last-run-event-id/run-status —
205-
// plus the daemon-ownership marker (role + runId), since a snapshot captured
206-
// before `/api/runs` assigned a run id can omit `runId` and would otherwise
207-
// null `run_id` and drop the message back out of the protected path on the
208-
// next stale PUT — only when the incoming snapshot would drop
209-
// already-persisted events. A web write that carries at least as many events
210-
// still flows through — which keeps mock-agent flows working (the daemon
211-
// never persisted events there, so the web is the legitimate writer) and
212-
// lets UI metadata (feedback, comment attachments, telemetry) land on every
213-
// PUT.
203+
// The guard is a "no regression" rule, not a blanket write-ownership rule,
204+
// and it has two independent triggers:
205+
// 1. Run events are append-only, so a stale snapshot can only SHRINK the
206+
// stored list — preserve stored events/content when the incoming
207+
// snapshot would drop already-persisted events.
208+
// 2. Terminal run status is a daemon-owned latch: the daemon writes it
209+
// separately (no event appended), so a snapshot captured after the
210+
// final event but before that write has the SAME event count yet still
211+
// carries a non-terminal status. Never let it regress a terminal status.
212+
// Both paths also preserve the daemon-ownership marker (role + runId), since
213+
// a snapshot captured before `/api/runs` assigned a run id can omit `runId`
214+
// and would otherwise null `run_id` and drop the message back out of the
215+
// protected path on the next stale PUT.
216+
//
217+
// A web write that carries at least as many events and a non-regressing
218+
// status still flows through — which keeps mock-agent flows working (the
219+
// daemon never persisted events/status there, so the web is the legitimate
220+
// writer) and lets UI metadata (feedback, comment attachments, telemetry)
221+
// land on every PUT.
214222
const mergeMessageWriteForDaemonBacked = (
215223
messageId: string,
216224
incoming: Record<string, unknown>,
217225
): Record<string, unknown> => {
218226
const stored = getMessage(db, messageId);
219227
if (!stored || stored.role !== 'assistant' || !stored.runId) return incoming;
220228
const incomingEvents = Array.isArray(incoming.events) ? incoming.events : [];
221-
if (!stored.events || stored.events.length === 0 || incomingEvents.length >= stored.events.length) {
222-
return incoming;
223-
}
229+
const shrinksEvents =
230+
Boolean(stored.events) &&
231+
stored.events!.length > 0 &&
232+
incomingEvents.length < stored.events!.length;
233+
const incomingStatus =
234+
typeof incoming.runStatus === 'string' ? incoming.runStatus : null;
235+
const regressesTerminalStatus =
236+
stored.runStatus !== undefined &&
237+
TERMINAL_RUN_STATUSES.has(stored.runStatus) &&
238+
incomingStatus !== stored.runStatus;
239+
if (!shrinksEvents && !regressesTerminalStatus) return incoming;
224240
return {
225241
...incoming,
226242
role: stored.role,
227243
runId: stored.runId,
228-
events: stored.events,
244+
events: stored.events ?? [],
229245
content: stored.content ?? '',
230246
lastRunEventId: stored.lastRunEventId,
231247
runStatus: stored.runStatus,

apps/daemon/tests/stale-message-snapshot-preserves-daemon-events.test.ts

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,76 @@ describe('stale web message snapshot does not wipe daemon-owned run events', ()
164164
expect(after?.feedback?.rating).toBe(1);
165165
});
166166

167+
it('does not regress a daemon-written terminal run status from an equal-length stale snapshot', async () => {
168+
// The daemon writes the terminal run_status separately (no event appended),
169+
// so a web snapshot captured after the final event but before that write has
170+
// the SAME event count while still carrying a non-terminal status. It must
171+
// not be able to regress the stored terminal status (#6396 / looper review).
172+
binDir = await mkdtemp(path.join(os.tmpdir(), 'od-terminal-latch-bin-'));
173+
const fakeClaude = await writeCleanClaude(binDir, 'claude-terminal-latch');
174+
175+
delete process.env.POSTHOG_KEY;
176+
delete process.env.POSTHOG_HOST;
177+
delete process.env.LANGFUSE_PUBLIC_KEY;
178+
delete process.env.LANGFUSE_SECRET_KEY;
179+
delete process.env.LANGFUSE_BASE_URL;
180+
delete process.env.OPEN_DESIGN_TELEMETRY_RELAY_URL;
181+
182+
started = (await startServer({ port: 0, returnServer: true })) as StartedServer;
183+
await putConfig(started.url, {
184+
agentId: 'claude',
185+
agentCliEnv: { claude: { CLAUDE_BIN: fakeClaude } },
186+
telemetry: { metrics: true, content: false, artifactManifest: false },
187+
privacyDecisionAt: Date.now(),
188+
});
189+
190+
const { projectId, conversationId } = await createConversation(started.url);
191+
const { assistantMessageId, status } = await sendRunAndWait(
192+
started.url,
193+
projectId,
194+
conversationId,
195+
);
196+
expect(status.status).toBe('succeeded');
197+
198+
const before = await fetchAssistantMessage(
199+
started.url,
200+
projectId,
201+
conversationId,
202+
assistantMessageId,
203+
);
204+
expect(before?.runStatus).toBe('succeeded');
205+
expect(before?.events?.length).toBeGreaterThan(0);
206+
207+
// Same event count, but the pre-finalize status — the exact shape that
208+
// previously slipped through the length-based freshness check.
209+
const equalLengthSnapshot = {
210+
id: assistantMessageId,
211+
role: 'assistant',
212+
content: before?.content ?? '',
213+
runId: before?.runId,
214+
runStatus: 'running',
215+
events: before?.events ?? [],
216+
};
217+
const putResponse = await fetch(
218+
`${started.url}/api/projects/${encodeURIComponent(projectId)}/conversations/${encodeURIComponent(conversationId)}/messages/${encodeURIComponent(assistantMessageId)}`,
219+
{
220+
method: 'PUT',
221+
headers: { 'content-type': 'application/json' },
222+
body: JSON.stringify(equalLengthSnapshot),
223+
},
224+
);
225+
expect(putResponse.status).toBe(200);
226+
227+
const after = await fetchAssistantMessage(
228+
started.url,
229+
projectId,
230+
conversationId,
231+
assistantMessageId,
232+
);
233+
expect(after?.runStatus, 'terminal run status must not regress').toBe('succeeded');
234+
expect(after?.runId).toBe(before?.runId);
235+
});
236+
167237
it('lets a mock-agent flow persist events/runStatus when the daemon never wrote any', async () => {
168238
// e2e Playwright suites mock the run SSE end-to-end, so the daemon never
169239
// persists events for the assistant message — the web client is the only

0 commit comments

Comments
 (0)