Răsfoiți Sursa

Fix background job reconciliation edge cases

Alvin Unreal 3 luni în urmă
părinte
comite
a728337b63

+ 45 - 18
src/hooks/task-session-manager/index.test.ts

@@ -440,7 +440,7 @@ describe('task-session-manager hook', () => {
     });
   });
 
-  test('dedupes anonymous synthetic completions by session message and part index', async () => {
+  test('dedupes anonymous synthetic completions by content hash even when message index changes', async () => {
     const board = new BackgroundJobBoard();
     const { hook } = createHook({ backgroundJobBoard: board });
 
@@ -464,16 +464,26 @@ describe('task-session-manager hook', () => {
         '</task_result>',
       ].join('\n'),
     };
-    const firstMessage = {
-      info: { role: 'user', agent: 'orchestrator', sessionID: 'parent-1' },
-      parts: [completionPart],
+
+    // First transform - message at index 0
+    const firstMessages = {
+      messages: [
+        {
+          info: { role: 'user', agent: 'orchestrator', sessionID: 'parent-1' },
+          parts: [completionPart],
+        },
+      ],
     };
 
-    await hook['experimental.chat.messages.transform'](
-      {},
-      { messages: [firstMessage] },
-    );
+    await hook['experimental.chat.messages.transform']({}, firstMessages);
 
+    expect(board.get('child-1')).toMatchObject({
+      state: 'completed',
+      terminalUnreconciled: true,
+      resultSummary: 'same result',
+    });
+
+    // Relaunch the task
     board.registerLaunch({
       taskID: 'child-1',
       parentSessionID: 'parent-1',
@@ -481,20 +491,37 @@ describe('task-session-manager hook', () => {
       description: 'map hooks again',
     });
 
-    const secondMessage = {
-      info: { role: 'user', agent: 'orchestrator', sessionID: 'parent-1' },
-      parts: [completionPart],
+    expect(board.get('child-1')).toMatchObject({
+      state: 'running',
+      terminalUnreconciled: false,
+    });
+
+    // Second transform - same completion content but at different message index (1 instead of 0)
+    // With stable content hash, this should still be deduped (not processed again)
+    const secondMessages = {
+      messages: [
+        {
+          info: {
+            role: 'assistant',
+            agent: 'orchestrator',
+            sessionID: 'parent-1',
+          },
+          parts: [{ type: 'text', text: 'some other message' }],
+        }, // New message at index 0
+        {
+          info: { role: 'user', agent: 'orchestrator', sessionID: 'parent-1' },
+          parts: [completionPart], // Same completion now at index 1
+        },
+      ],
     };
 
-    await hook['experimental.chat.messages.transform'](
-      {},
-      { messages: [firstMessage, secondMessage] },
-    );
+    await hook['experimental.chat.messages.transform']({}, secondMessages);
 
+    // Should still be running because the same anonymous completion was deduped
+    // (not re-processed just because message index changed)
     expect(board.get('child-1')).toMatchObject({
-      state: 'completed',
-      terminalUnreconciled: true,
-      resultSummary: 'same result',
+      state: 'running',
+      terminalUnreconciled: false,
     });
   });
 

+ 55 - 10
src/hooks/task-session-manager/index.ts

@@ -70,6 +70,58 @@ const BACKGROUND_COMPLETION_COMPLETED = /^Background task completed: /;
 const BACKGROUND_COMPLETION_FAILED = /^Background task failed: /;
 const MAX_PROCESSED_INJECTED_COMPLETIONS = 500;
 
+/**
+ * Simple deterministic string hash for stable occurrence IDs.
+ * Uses DJB2 algorithm - fast and good distribution for short strings.
+ */
+function djb2Hash(str: string): string {
+  let hash = 5381;
+  for (let i = 0; i < str.length; i++) {
+    hash = (hash << 5) + hash + str.charCodeAt(i); // hash * 33 + char
+  }
+  // Convert to unsigned 32-bit and then to hex
+  return (hash >>> 0).toString(16).padStart(8, '0');
+}
+
+/**
+ * Create a stable occurrence ID for synthetic completion deduplication.
+ * Prefers part.id, then message.info.id + partIndex, then content-derived hash.
+ */
+function createOccurrenceId(
+  part: ChatMessagePart,
+  message: ChatMessage,
+  partIndex: number,
+): string {
+  // Prefer explicit part.id if available
+  if (typeof part.id === 'string') {
+    return part.id;
+  }
+
+  // Fall back to message.info.id + partIndex
+  if (typeof message.info.id === 'string') {
+    return `${message.info.id}:${partIndex}`;
+  }
+
+  // Final fallback: content-derived hash from sessionID + parsed taskID/state/result
+  // This ensures the same anonymous synthetic completion is deduped
+  // even when its message index changes between transform calls
+  const sessionID = message.info.sessionID ?? 'unknown';
+  const content = typeof part.text === 'string' ? part.text : '';
+
+  // Parse task status to get stable identifiers
+  const status = parseTaskStatusOutput(content);
+  if (status) {
+    // Use taskID + state + result for a stable hash
+    const stableKey = `${sessionID}:${status.taskID}:${status.state}:${status.result ?? ''}`;
+    const hash = djb2Hash(stableKey);
+    return `anon:${hash}`;
+  }
+
+  // Fallback to hashing the full content if parsing fails
+  const hash = djb2Hash(`${sessionID}:${content}`);
+  return `anon:${hash}`;
+}
+
 function isAgentName(value: unknown): value is AgentName {
   return typeof value === 'string' && AGENT_NAME_SET.has(value as AgentName);
 }
@@ -220,7 +272,7 @@ export function createTaskSessionManagerHook(
   function updateFromInjectedCompletion(
     part: ChatMessagePart,
     message: ChatMessage,
-    messageIndex: number,
+    _messageIndex: number,
     partIndex: number,
   ): BackgroundJobRecord | undefined {
     if (part.type !== 'text' || typeof part.text !== 'string') {
@@ -243,14 +295,8 @@ export function createTaskSessionManagerHook(
     if (isCompleted && status.state !== 'completed') return undefined;
     if (isFailed && status.state !== 'error') return undefined;
 
-    // Dedupe by synthetic message occurrence using part.id if available,
-    // fallback to message.info.id + part index, then message/part index.
-    const occurrenceId =
-      typeof part.id === 'string'
-        ? part.id
-        : typeof message.info.id === 'string'
-          ? `${message.info.id}:${partIndex}`
-          : `${message.info.sessionID ?? 'unknown'}:${messageIndex}:${partIndex}`;
+    // Dedupe by synthetic message occurrence using stable occurrence ID
+    const occurrenceId = createOccurrenceId(part, message, partIndex);
 
     if (processedInjectedCompletions.has(occurrenceId)) return undefined;
 
@@ -612,7 +658,6 @@ export function createTaskSessionManagerHook(
       if (!sessionId) return;
 
       sessionManager.dropTask(sessionId);
-      backgroundJobBoard.drop(sessionId);
       sessionManager.clearParent(sessionId);
       backgroundJobBoard.clearParent(sessionId);
       terminalJobsInjectedByParent.delete(sessionId);

+ 76 - 0
src/utils/background-job-board.test.ts

@@ -208,6 +208,7 @@ describe('BackgroundJobBoard', () => {
       [
         'task_id: ses_1',
         'state: cancelled',
+        '',
         '<task_error>',
         'cancelled by user',
         '</task_error>',
@@ -220,4 +221,79 @@ describe('BackgroundJobBoard', () => {
       resultSummary: 'cancelled by user',
     });
   });
+
+  test('stale status updates cannot reopen already reconciled jobs', () => {
+    const board = new BackgroundJobBoard();
+    board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'oracle',
+      description: 'review plan',
+    });
+    board.updateStatus({ taskID: 'ses_1', state: 'completed' });
+    board.markReconciled('ses_1', 300);
+
+    // Stale status updates should not reopen the reconciled job
+    const staleCompleted = board.updateStatus({
+      taskID: 'ses_1',
+      state: 'completed',
+      resultSummary: 'stale result',
+    });
+    expect(staleCompleted).toMatchObject({
+      state: 'reconciled',
+      terminalUnreconciled: false,
+    });
+
+    const staleError = board.updateStatus({
+      taskID: 'ses_1',
+      state: 'error',
+      resultSummary: 'stale error',
+    });
+    expect(staleError).toMatchObject({
+      state: 'reconciled',
+      terminalUnreconciled: false,
+    });
+
+    const staleCancelled = board.updateStatus({
+      taskID: 'ses_1',
+      state: 'cancelled',
+    });
+    expect(staleCancelled).toMatchObject({
+      state: 'reconciled',
+      terminalUnreconciled: false,
+    });
+
+    // Job should remain hidden from prompt
+    expect(board.formatForPrompt('parent-1')).toBeUndefined();
+  });
+
+  test('registerLaunch can reset a reconciled job to running', () => {
+    const board = new BackgroundJobBoard();
+    board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'oracle',
+      description: 'review plan',
+      now: 100,
+    });
+    board.updateStatus({ taskID: 'ses_1', state: 'completed' });
+    board.markReconciled('ses_1', 300);
+
+    // Relaunch should reset the reconciled job to running
+    const relaunched = board.registerLaunch({
+      taskID: 'ses_1',
+      parentSessionID: 'parent-1',
+      agent: 'oracle',
+      description: 'review plan again',
+      now: 400,
+    });
+
+    expect(relaunched).toMatchObject({
+      state: 'running',
+      terminalUnreconciled: false,
+      completedAt: undefined,
+      resultSummary: undefined,
+      updatedAt: 400,
+    });
+  });
 });

+ 5 - 0
src/utils/background-job-board.ts

@@ -100,6 +100,11 @@ export class BackgroundJobBoard {
     const existing = this.jobs.get(input.taskID);
     if (!existing) return undefined;
 
+    // Guard: stale status updates cannot reopen already reconciled jobs
+    if (existing.state === 'reconciled') {
+      return existing;
+    }
+
     const now = input.now ?? Date.now();
     const terminal = TERMINAL_STATES.has(input.state);
     const updated: BackgroundJobRecord = {