fix(autofix-pr): 修复 taskId 不一致导致 monitor lock dangling
问题:createAutofixTeammate 生成 teammate UUID 作为 monitor lock 的 key, 但 registerRemoteAgentTask 内部生成的 framework taskId 是另一个 UUID。 CCR session 自然完成时框架调 clearActiveMonitor(frameworkTaskId) guard 失败,lock 永不释放,导致后续 /autofix-pr 报 "already monitoring"。 修复(Phase 1 of remote-agent completion loop): - monitorState 新增 updateActiveMonitor(partial) 原子更新 - callAutofixPr 在 register 后 swap lock 的 taskId 到 framework 分配的 id - RemoteAgentTask 引入 registerCompletionHook 注册式 API(参考已有的 registerCompletionChecker 模式),在 5 个完成路径调 runCompletionHook - autofix-pr 命令模块自己注册 cleanup hook,避免 framework 反向依赖 command 模块 测试: - monitorState 新增 4 个测试(updateActiveMonitor 行为 + bug 复现/修复) - launchAutofixPr 新增 3 个端到端回归测试(taskId swap + hook 触发 + subsequent launch 不报 already monitoring) 完整分析与 Phase 2/3 改造方案见 docs/features/remote-agent-completion-analysis.md。
This commit is contained in:
parent
ea399f1862
commit
cca5102f15
|
|
@ -46,7 +46,7 @@ mock.module('src/utils/teleport.js', () => ({
|
|||
}))
|
||||
|
||||
const registerMock = mock(() => ({
|
||||
taskId: 'task-abc',
|
||||
taskId: 'framework-task-id',
|
||||
sessionId: 'session-123',
|
||||
cleanup: () => {},
|
||||
}))
|
||||
|
|
@ -56,10 +56,14 @@ const checkEligibilityMock = mock(() =>
|
|||
const getSessionUrlMock = mock(
|
||||
(id: string) => `https://claude.ai/session/${id}`,
|
||||
)
|
||||
const registerCompletionHookMock = mock<
|
||||
(taskType: string, hook: (taskId: string, metadata?: unknown) => void) => void
|
||||
>(() => {})
|
||||
|
||||
mock.module('src/tasks/RemoteAgentTask/RemoteAgentTask.js', () => ({
|
||||
checkRemoteAgentEligibility: checkEligibilityMock,
|
||||
registerRemoteAgentTask: registerMock,
|
||||
registerCompletionHook: registerCompletionHookMock,
|
||||
getRemoteTaskSessionUrl: getSessionUrlMock,
|
||||
formatPreconditionError: (e: { type: string }) => e.type,
|
||||
}))
|
||||
|
|
@ -375,6 +379,63 @@ describe('callAutofixPr', () => {
|
|||
})
|
||||
})
|
||||
|
||||
// Regression suite for the taskId-mismatch latent bug + completion hook wiring.
|
||||
// Before this fix, createAutofixTeammate generated a teammate UUID, that UUID
|
||||
// was used to acquire the singleton monitor lock, and registerRemoteAgentTask
|
||||
// generated a *different* framework taskId. When the framework eventually
|
||||
// called clearActiveMonitor(frameworkTaskId) on natural completion, the guard
|
||||
// failed (active.taskId !== frameworkTaskId) and the lock stayed acquired,
|
||||
// blocking any subsequent /autofix-pr invocations in the same process.
|
||||
describe('callAutofixPr · completion hook wiring (taskId mismatch regression)', () => {
|
||||
test('updateActiveMonitor swaps lock taskId to framework-assigned id after register', async () => {
|
||||
await callAutofixPr(onDone, makeContext(), '42')
|
||||
const monitor = getActiveMonitor() as { taskId: string } | null
|
||||
expect(monitor).not.toBeNull()
|
||||
// registerMock returns 'framework-task-id'; before the fix this would be
|
||||
// a teammate-generated random UUID instead.
|
||||
expect(monitor?.taskId).toBe('framework-task-id')
|
||||
})
|
||||
|
||||
test('framework hook → clearActiveMonitor releases lock on natural completion', async () => {
|
||||
await callAutofixPr(onDone, makeContext(), '42')
|
||||
expect(getActiveMonitor()).not.toBeNull()
|
||||
|
||||
// Find the hook the module registered at import time. We grab the last
|
||||
// call so re-imports across tests don't break this — only the most recent
|
||||
// registration is what the framework would invoke now.
|
||||
const calls = registerCompletionHookMock.mock.calls
|
||||
expect(calls.length).toBeGreaterThan(0)
|
||||
const lastCall = calls[calls.length - 1]
|
||||
expect(lastCall?.[0]).toBe('autofix-pr')
|
||||
const hook = lastCall?.[1] as (id: string, metadata?: unknown) => void
|
||||
expect(typeof hook).toBe('function')
|
||||
|
||||
// Simulate the framework invoking the hook with the framework taskId
|
||||
// after a terminal transition. Before the fix this would no-op against
|
||||
// a lock keyed by the teammate UUID.
|
||||
hook('framework-task-id', { owner: 'acme', repo: 'myrepo', prNumber: 42 })
|
||||
expect(getActiveMonitor()).toBeNull()
|
||||
})
|
||||
|
||||
test('subsequent /autofix-pr succeeds after framework hook clears the lock', async () => {
|
||||
await callAutofixPr(onDone, makeContext(), '42')
|
||||
// Simulate natural completion via the registered hook
|
||||
const calls = registerCompletionHookMock.mock.calls
|
||||
const hook = calls[calls.length - 1]?.[1] as (
|
||||
id: string,
|
||||
metadata?: unknown,
|
||||
) => void
|
||||
hook('framework-task-id', { owner: 'acme', repo: 'myrepo', prNumber: 42 })
|
||||
|
||||
onDone.mockClear()
|
||||
await callAutofixPr(onDone, makeContext(), '99')
|
||||
const firstArg = onDone.mock.calls[0]?.[0] as string
|
||||
// Should be the success path, not "already monitoring"
|
||||
expect(firstArg).not.toMatch(/already monitoring/i)
|
||||
expect(firstArg).toMatch(/Autofix launched/)
|
||||
})
|
||||
})
|
||||
|
||||
// Cover ../index.ts load() — placed in this test file so all the heavy mocks
|
||||
// (teleport / detectRepository / RemoteAgentTask / bootstrap-state / analytics /
|
||||
// skillDetect) are already registered when load() dynamically imports
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import {
|
|||
isMonitoring,
|
||||
setActiveMonitor,
|
||||
trySetActiveMonitor,
|
||||
updateActiveMonitor,
|
||||
} from '../monitorState.js'
|
||||
|
||||
function makeState(
|
||||
|
|
@ -76,4 +77,41 @@ describe('monitorState', () => {
|
|||
// First state remains
|
||||
expect(getActiveMonitor()?.prNumber).toBe(1)
|
||||
})
|
||||
|
||||
test('updateActiveMonitor returns false when no active monitor', () => {
|
||||
expect(updateActiveMonitor({ taskId: 'task-x' })).toBe(false)
|
||||
expect(getActiveMonitor()).toBeNull()
|
||||
})
|
||||
|
||||
test('updateActiveMonitor merges partial fields into the active monitor', () => {
|
||||
setActiveMonitor(makeState({ taskId: 'tentative-uuid' }))
|
||||
expect(updateActiveMonitor({ taskId: 'framework-task-id' })).toBe(true)
|
||||
const after = getActiveMonitor()
|
||||
expect(after?.taskId).toBe('framework-task-id')
|
||||
// Other fields untouched
|
||||
expect(after?.owner).toBe('acme')
|
||||
expect(after?.repo).toBe('myrepo')
|
||||
expect(after?.prNumber).toBe(42)
|
||||
})
|
||||
|
||||
test('updateActiveMonitor with new taskId makes clearActiveMonitor recognise framework taskId', () => {
|
||||
// Reproduce the latent bug scenario: lock acquired with one taskId,
|
||||
// framework assigns a different one. Before the fix, the framework's
|
||||
// clearActiveMonitor(frameworkTaskId) would no-op because guard fails.
|
||||
setActiveMonitor(makeState({ taskId: 'teammate-uuid' }))
|
||||
// Framework cleanup using its own taskId — would fail guard before the fix
|
||||
clearActiveMonitor('framework-uuid')
|
||||
expect(getActiveMonitor()).not.toBeNull()
|
||||
// After updateActiveMonitor swaps the taskId, framework cleanup works
|
||||
updateActiveMonitor({ taskId: 'framework-uuid' })
|
||||
clearActiveMonitor('framework-uuid')
|
||||
expect(getActiveMonitor()).toBeNull()
|
||||
})
|
||||
|
||||
test('updateActiveMonitor does not change abortController identity', () => {
|
||||
const ac = new AbortController()
|
||||
setActiveMonitor(makeState({ abortController: ac, taskId: 'tentative' }))
|
||||
updateActiveMonitor({ taskId: 'updated' })
|
||||
expect(getActiveMonitor()?.abortController).toBe(ac)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ import {
|
|||
checkRemoteAgentEligibility,
|
||||
formatPreconditionError,
|
||||
getRemoteTaskSessionUrl,
|
||||
registerCompletionHook,
|
||||
registerRemoteAgentTask,
|
||||
type BackgroundRemoteSessionPrecondition,
|
||||
} from '../../tasks/RemoteAgentTask/RemoteAgentTask.js'
|
||||
|
|
@ -26,10 +27,21 @@ import {
|
|||
getActiveMonitor,
|
||||
isMonitoring,
|
||||
trySetActiveMonitor,
|
||||
updateActiveMonitor,
|
||||
} from './monitorState.js'
|
||||
import { parseAutofixArgs } from './parseArgs.js'
|
||||
import { detectAutofixSkills, formatSkillsHint } from './skillDetect.js'
|
||||
|
||||
// Release the singleton monitor lock when the framework transitions the
|
||||
// autofix task to a terminal state. Without this, the lock — keyed by the
|
||||
// framework-assigned taskId (after callAutofixPr's updateActiveMonitor swap)
|
||||
// — would dangle past natural completion, blocking subsequent /autofix-pr
|
||||
// invocations until the process restarts. Registered at module load; the
|
||||
// framework's runCompletionHook invokes it once per terminal transition.
|
||||
registerCompletionHook('autofix-pr', taskId => {
|
||||
clearActiveMonitor(taskId)
|
||||
})
|
||||
|
||||
function makeErrorText(message: string, code: string): string {
|
||||
logEvent('tengu_autofix_pr_result', {
|
||||
result:
|
||||
|
|
@ -277,8 +289,15 @@ export const callAutofixPr: LocalJSXCommandCall = async (
|
|||
// 4.9 register task. If this throws, release the lock so the user can
|
||||
// retry — the remote CCR session is already created so we surface a
|
||||
// dedicated error code.
|
||||
//
|
||||
// After registration succeeds, swap the lock's taskId from the tentative
|
||||
// teammate UUID (used to acquire the lock atomically before teleport) to
|
||||
// the framework-assigned taskId. Without this swap, the framework's own
|
||||
// cleanup path (clearActiveMonitor(frameworkTaskId) on natural completion)
|
||||
// would no-op against a lock keyed by teammate.taskId, leaving the
|
||||
// singleton lock dangling and blocking future /autofix-pr invocations.
|
||||
try {
|
||||
registerRemoteAgentTask({
|
||||
const { taskId: frameworkTaskId } = registerRemoteAgentTask({
|
||||
remoteTaskType: 'autofix-pr',
|
||||
session,
|
||||
command: `/autofix-pr ${prNumber}`,
|
||||
|
|
@ -286,6 +305,7 @@ export const callAutofixPr: LocalJSXCommandCall = async (
|
|||
isLongRunning: true,
|
||||
remoteTaskMetadata: { owner, repo, prNumber },
|
||||
})
|
||||
updateActiveMonitor({ taskId: frameworkTaskId })
|
||||
} catch (regErr: unknown) {
|
||||
clearActiveMonitor(teammate.taskId)
|
||||
const regMsg = regErr instanceof Error ? regErr.message : String(regErr)
|
||||
|
|
|
|||
|
|
@ -46,6 +46,20 @@ export function clearActiveMonitor(taskId?: string): void {
|
|||
active = null
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically merges partial updates into the active monitor. Returns true if
|
||||
* applied, false if no active monitor. Used when the caller needs to swap the
|
||||
* lock's taskId after the framework assigns a different one than the
|
||||
* tentative one used to acquire the lock — without this the framework's
|
||||
* cleanup (clearActiveMonitor with the framework taskId) would no-op against
|
||||
* a lock keyed by the caller's tentative id.
|
||||
*/
|
||||
export function updateActiveMonitor(partial: Partial<MonitorState>): boolean {
|
||||
if (!active) return false
|
||||
active = { ...active, ...partial }
|
||||
return true
|
||||
}
|
||||
|
||||
export function isMonitoring(
|
||||
owner: string,
|
||||
repo: string,
|
||||
|
|
|
|||
|
|
@ -114,6 +114,38 @@ export function registerCompletionChecker(remoteTaskType: RemoteTaskType, checke
|
|||
completionCheckers.set(remoteTaskType, checker);
|
||||
}
|
||||
|
||||
/**
|
||||
* Called after the task transitions to a terminal state and the notification
|
||||
* has been enqueued. Used by command modules to release singleton locks,
|
||||
* clear cached state, or perform other cleanup the framework cannot see.
|
||||
* Hooks must be synchronous and best-effort — errors are logged but never
|
||||
* propagate.
|
||||
*/
|
||||
export type RemoteTaskCompletionHook = (taskId: string, remoteTaskMetadata: RemoteTaskMetadata | undefined) => void;
|
||||
|
||||
const completionHooks = new Map<RemoteTaskType, RemoteTaskCompletionHook>();
|
||||
|
||||
/**
|
||||
* Register a completion hook for a remote task type. Invoked once after the
|
||||
* task reaches a terminal state in any of the framework's completion branches
|
||||
* (archived session, completionChecker, stableIdle, result). Use this to
|
||||
* release command-module state (e.g. singleton locks) without forcing the
|
||||
* framework to reverse-import from the command package.
|
||||
*/
|
||||
export function registerCompletionHook(remoteTaskType: RemoteTaskType, hook: RemoteTaskCompletionHook): void {
|
||||
completionHooks.set(remoteTaskType, hook);
|
||||
}
|
||||
|
||||
function runCompletionHook(taskId: string, task: RemoteAgentTaskState): void {
|
||||
const hook = completionHooks.get(task.remoteTaskType);
|
||||
if (!hook) return;
|
||||
try {
|
||||
hook(taskId, task.remoteTaskMetadata);
|
||||
} catch (e) {
|
||||
logError(e);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Persist a remote-agent metadata entry to the session sidecar.
|
||||
* Fire-and-forget — persistence failures must not block task registration.
|
||||
|
|
@ -681,6 +713,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () =>
|
|||
enqueueRemoteNotification(taskId, task.title, 'completed', context.setAppState, task.toolUseId);
|
||||
void evictTaskOutput(taskId);
|
||||
void removeRemoteAgentMetadata(taskId);
|
||||
runCompletionHook(taskId, task);
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -694,6 +727,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () =>
|
|||
enqueueRemoteNotification(taskId, completionResult, 'completed', context.setAppState, task.toolUseId);
|
||||
void evictTaskOutput(taskId);
|
||||
void removeRemoteAgentMetadata(taskId);
|
||||
runCompletionHook(taskId, task);
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
|
@ -853,6 +887,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () =>
|
|||
enqueueRemoteReviewNotification(taskId, reviewContent, context.setAppState);
|
||||
void evictTaskOutput(taskId);
|
||||
void removeRemoteAgentMetadata(taskId);
|
||||
runCompletionHook(taskId, task);
|
||||
return; // Stop polling
|
||||
}
|
||||
|
||||
|
|
@ -870,12 +905,14 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () =>
|
|||
enqueueRemoteReviewFailureNotification(taskId, reason, context.setAppState);
|
||||
void evictTaskOutput(taskId);
|
||||
void removeRemoteAgentMetadata(taskId);
|
||||
runCompletionHook(taskId, task);
|
||||
return; // Stop polling
|
||||
}
|
||||
|
||||
enqueueRemoteNotification(taskId, task.title, finalStatus, context.setAppState, task.toolUseId);
|
||||
void evictTaskOutput(taskId);
|
||||
void removeRemoteAgentMetadata(taskId);
|
||||
runCompletionHook(taskId, task);
|
||||
return; // Stop polling
|
||||
}
|
||||
} catch (error) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user