fix: address 5 edge cases from review-work findings
- C3: include command args in auto-slash-command dedup key - H2: track completed task summaries for ALL COMPLETE message - H9: increment tmux close retry count on re-mark - H8: detect stale MCP connections after disconnect+reconnect race - H8: guard disconnectedSessions growth for non-MCP sessions - C1: await tmux cleanup in plugin dispose lifecycle
This commit is contained in:
@@ -121,6 +121,7 @@ export class BackgroundManager {
|
|||||||
private queuesByKey: Map<string, QueueItem[]> = new Map()
|
private queuesByKey: Map<string, QueueItem[]> = new Map()
|
||||||
private processingKeys: Set<string> = new Set()
|
private processingKeys: Set<string> = new Set()
|
||||||
private completionTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
|
private completionTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
|
||||||
|
private completedTaskSummaries: Map<string, Array<{id: string, description: string}>> = new Map()
|
||||||
private idleDeferralTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
|
private idleDeferralTimers: Map<string, ReturnType<typeof setTimeout>> = new Map()
|
||||||
private notificationQueueByParent: Map<string, Promise<void>> = new Map()
|
private notificationQueueByParent: Map<string, Promise<void>> = new Map()
|
||||||
private rootDescendantCounts: Map<string, number>
|
private rootDescendantCounts: Map<string, number>
|
||||||
@@ -1379,6 +1380,14 @@ export class BackgroundManager {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!this.completedTaskSummaries.has(task.parentSessionID)) {
|
||||||
|
this.completedTaskSummaries.set(task.parentSessionID, [])
|
||||||
|
}
|
||||||
|
this.completedTaskSummaries.get(task.parentSessionID)!.push({
|
||||||
|
id: task.id,
|
||||||
|
description: task.description,
|
||||||
|
})
|
||||||
|
|
||||||
// Update pending tracking and check if all tasks complete
|
// Update pending tracking and check if all tasks complete
|
||||||
const pendingSet = this.pendingByParent.get(task.parentSessionID)
|
const pendingSet = this.pendingByParent.get(task.parentSessionID)
|
||||||
let allComplete = false
|
let allComplete = false
|
||||||
@@ -1398,10 +1407,13 @@ export class BackgroundManager {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const completedTasks = allComplete
|
const completedTasks = allComplete
|
||||||
? Array.from(this.tasks.values())
|
? (this.completedTaskSummaries.get(task.parentSessionID) ?? [{ id: task.id, description: task.description }])
|
||||||
.filter(t => t.parentSessionID === task.parentSessionID && t.status !== "running" && t.status !== "pending")
|
|
||||||
: []
|
: []
|
||||||
|
|
||||||
|
if (allComplete) {
|
||||||
|
this.completedTaskSummaries.delete(task.parentSessionID)
|
||||||
|
}
|
||||||
|
|
||||||
const statusText = task.status === "completed"
|
const statusText = task.status === "completed"
|
||||||
? "COMPLETED"
|
? "COMPLETED"
|
||||||
: task.status === "interrupt"
|
: task.status === "interrupt"
|
||||||
@@ -1740,6 +1752,7 @@ Use \`background_output(task_id="${task.id}")\` to retrieve this result when rea
|
|||||||
this.queuesByKey.clear()
|
this.queuesByKey.clear()
|
||||||
this.processingKeys.clear()
|
this.processingKeys.clear()
|
||||||
this.taskHistory.clearAll()
|
this.taskHistory.clearAll()
|
||||||
|
this.completedTaskSummaries.clear()
|
||||||
this.unregisterProcessCleanup()
|
this.unregisterProcessCleanup()
|
||||||
log("[background-agent] Shutdown complete")
|
log("[background-agent] Shutdown complete")
|
||||||
|
|
||||||
|
|||||||
@@ -87,7 +87,16 @@ async function cleanupIdleClients(state: SkillMcpManagerState): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export async function disconnectSession(state: SkillMcpManagerState, sessionID: string): Promise<void> {
|
export async function disconnectSession(state: SkillMcpManagerState, sessionID: string): Promise<void> {
|
||||||
state.disconnectedSessions.add(sessionID)
|
let hasPendingForSession = false
|
||||||
|
for (const key of state.pendingConnections.keys()) {
|
||||||
|
if (key.startsWith(`${sessionID}:`)) {
|
||||||
|
hasPendingForSession = true
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if (hasPendingForSession) {
|
||||||
|
state.disconnectedSessions.add(sessionID)
|
||||||
|
}
|
||||||
const keysToRemove: string[] = []
|
const keysToRemove: string[] = []
|
||||||
|
|
||||||
for (const [key, managed] of state.clients.entries()) {
|
for (const [key, managed] of state.clients.entries()) {
|
||||||
|
|||||||
@@ -154,11 +154,11 @@ describe("getOrCreateClient disconnect race", () => {
|
|||||||
expect(createdClients[0]?.close).not.toHaveBeenCalled()
|
expect(createdClients[0]?.close).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
it("#given no pending connections #when disconnectSession is called #then no errors occur and the session is added to disconnectedSessions", async () => {
|
it("#given no pending connections #when disconnectSession is called #then no errors occur and session is not added to disconnectedSessions", async () => {
|
||||||
const state = createState()
|
const state = createState()
|
||||||
|
|
||||||
await expect(disconnectSession(state, "session-a")).resolves.toBeUndefined()
|
await expect(disconnectSession(state, "session-a")).resolves.toBeUndefined()
|
||||||
expect(state.disconnectedSessions.has("session-a")).toBe(true)
|
expect(state.disconnectedSessions.has("session-a")).toBe(false)
|
||||||
expect(state.pendingConnections.size).toBe(0)
|
expect(state.pendingConnections.size).toBe(0)
|
||||||
expect(state.clients.size).toBe(0)
|
expect(state.clients.size).toBe(0)
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -29,9 +29,16 @@ export async function getOrCreateClient(params: {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const expandedConfig = expandEnvVarsInObject(config)
|
const expandedConfig = expandEnvVarsInObject(config)
|
||||||
const connectionPromise = (async () => {
|
let currentConnectionPromise!: Promise<Client>
|
||||||
|
currentConnectionPromise = (async () => {
|
||||||
const client = await createClient({ state, clientKey, info, config: expandedConfig })
|
const client = await createClient({ state, clientKey, info, config: expandedConfig })
|
||||||
|
|
||||||
|
const isStale = state.pendingConnections.has(clientKey) && state.pendingConnections.get(clientKey) !== currentConnectionPromise
|
||||||
|
if (isStale) {
|
||||||
|
try { await client.close() } catch {}
|
||||||
|
throw new Error(`Connection for "${info.sessionID}" was superseded by a newer connection attempt.`)
|
||||||
|
}
|
||||||
|
|
||||||
if (state.disconnectedSessions.has(info.sessionID)) {
|
if (state.disconnectedSessions.has(info.sessionID)) {
|
||||||
await forceReconnect(state, clientKey)
|
await forceReconnect(state, clientKey)
|
||||||
throw new Error(`Session "${info.sessionID}" disconnected during MCP connection setup.`)
|
throw new Error(`Session "${info.sessionID}" disconnected during MCP connection setup.`)
|
||||||
@@ -40,13 +47,13 @@ export async function getOrCreateClient(params: {
|
|||||||
return client
|
return client
|
||||||
})()
|
})()
|
||||||
|
|
||||||
state.pendingConnections.set(clientKey, connectionPromise)
|
state.pendingConnections.set(clientKey, currentConnectionPromise)
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const client = await connectionPromise
|
const client = await currentConnectionPromise
|
||||||
return client
|
return client
|
||||||
} finally {
|
} finally {
|
||||||
if (state.pendingConnections.get(clientKey) === connectionPromise) {
|
if (state.pendingConnections.get(clientKey) === currentConnectionPromise) {
|
||||||
state.pendingConnections.delete(clientKey)
|
state.pendingConnections.delete(clientKey)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -23,5 +23,6 @@ export function markTrackedSessionClosePending(tracked: TrackedSession): Tracked
|
|||||||
return {
|
return {
|
||||||
...tracked,
|
...tracked,
|
||||||
closePending: true,
|
closePending: true,
|
||||||
|
closeRetryCount: tracked.closePending ? tracked.closeRetryCount + 1 : tracked.closeRetryCount,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -125,7 +125,7 @@ export function createAutoSlashCommandHook(options?: AutoSlashCommandHookOptions
|
|||||||
input: CommandExecuteBeforeInput,
|
input: CommandExecuteBeforeInput,
|
||||||
output: CommandExecuteBeforeOutput
|
output: CommandExecuteBeforeOutput
|
||||||
): Promise<void> => {
|
): Promise<void> => {
|
||||||
const commandKey = `${input.sessionID}:${input.command.toLowerCase()}`
|
const commandKey = `${input.sessionID}:${input.command.toLowerCase()}:${input.arguments || ""}`
|
||||||
if (sessionProcessedCommandExecutions.has(commandKey)) {
|
if (sessionProcessedCommandExecutions.has(commandKey)) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -74,6 +74,7 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
|
|||||||
const dispose = createPluginDispose({
|
const dispose = createPluginDispose({
|
||||||
backgroundManager: managers.backgroundManager,
|
backgroundManager: managers.backgroundManager,
|
||||||
skillMcpManager: managers.skillMcpManager,
|
skillMcpManager: managers.skillMcpManager,
|
||||||
|
tmuxSessionManager: managers.tmuxSessionManager,
|
||||||
disposeHooks: hooks.disposeHooks,
|
disposeHooks: hooks.disposeHooks,
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
@@ -16,6 +16,7 @@ describe("createPluginDispose", () => {
|
|||||||
const dispose = createPluginDispose({
|
const dispose = createPluginDispose({
|
||||||
backgroundManager,
|
backgroundManager,
|
||||||
skillMcpManager,
|
skillMcpManager,
|
||||||
|
tmuxSessionManager: { cleanup: async (): Promise<void> => {} },
|
||||||
disposeHooks: (): void => {},
|
disposeHooks: (): void => {},
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -38,6 +39,7 @@ describe("createPluginDispose", () => {
|
|||||||
const dispose = createPluginDispose({
|
const dispose = createPluginDispose({
|
||||||
backgroundManager,
|
backgroundManager,
|
||||||
skillMcpManager,
|
skillMcpManager,
|
||||||
|
tmuxSessionManager: { cleanup: async (): Promise<void> => {} },
|
||||||
disposeHooks: (): void => {},
|
disposeHooks: (): void => {},
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -69,6 +71,7 @@ describe("createPluginDispose", () => {
|
|||||||
skillMcpManager: {
|
skillMcpManager: {
|
||||||
disconnectAll: async (): Promise<void> => {},
|
disconnectAll: async (): Promise<void> => {},
|
||||||
},
|
},
|
||||||
|
tmuxSessionManager: { cleanup: async (): Promise<void> => {} },
|
||||||
disposeHooks: (): void => {
|
disposeHooks: (): void => {
|
||||||
disposeCreatedHooks({
|
disposeCreatedHooks({
|
||||||
runtimeFallback,
|
runtimeFallback,
|
||||||
@@ -104,6 +107,7 @@ describe("createPluginDispose", () => {
|
|||||||
const dispose = createPluginDispose({
|
const dispose = createPluginDispose({
|
||||||
backgroundManager,
|
backgroundManager,
|
||||||
skillMcpManager,
|
skillMcpManager,
|
||||||
|
tmuxSessionManager: { cleanup: async (): Promise<void> => {} },
|
||||||
disposeHooks: disposeHooks.run,
|
disposeHooks: disposeHooks.run,
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
@@ -7,9 +7,12 @@ export function createPluginDispose(args: {
|
|||||||
skillMcpManager: {
|
skillMcpManager: {
|
||||||
disconnectAll: () => Promise<void>
|
disconnectAll: () => Promise<void>
|
||||||
}
|
}
|
||||||
|
tmuxSessionManager: {
|
||||||
|
cleanup: () => Promise<void>
|
||||||
|
}
|
||||||
disposeHooks: () => void
|
disposeHooks: () => void
|
||||||
}): PluginDispose {
|
}): PluginDispose {
|
||||||
const { backgroundManager, skillMcpManager, disposeHooks } = args
|
const { backgroundManager, skillMcpManager, tmuxSessionManager, disposeHooks } = args
|
||||||
let disposePromise: Promise<void> | null = null
|
let disposePromise: Promise<void> | null = null
|
||||||
|
|
||||||
return async (): Promise<void> => {
|
return async (): Promise<void> => {
|
||||||
@@ -20,7 +23,10 @@ export function createPluginDispose(args: {
|
|||||||
|
|
||||||
disposePromise = (async (): Promise<void> => {
|
disposePromise = (async (): Promise<void> => {
|
||||||
backgroundManager.shutdown()
|
backgroundManager.shutdown()
|
||||||
await skillMcpManager.disconnectAll()
|
await Promise.all([
|
||||||
|
skillMcpManager.disconnectAll(),
|
||||||
|
tmuxSessionManager.cleanup(),
|
||||||
|
])
|
||||||
disposeHooks()
|
disposeHooks()
|
||||||
})()
|
})()
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user