fix(tmux): unify isolated cleanup across close paths
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -168,6 +168,10 @@ function createTmuxConfig(overrides?: Partial<TmuxConfig>): TmuxConfig {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function getTrackedSessions(manager: object): Map<string, { paneId: string; closePending: boolean; closeRetryCount: number }> {
|
||||||
|
return Reflect.get(manager, 'sessions') as Map<string, { paneId: string; closePending: boolean; closeRetryCount: number }>
|
||||||
|
}
|
||||||
|
|
||||||
describe('TmuxSessionManager', () => {
|
describe('TmuxSessionManager', () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
mockQueryWindowState.mockClear()
|
mockQueryWindowState.mockClear()
|
||||||
@@ -1532,6 +1536,280 @@ describe('TmuxSessionManager', () => {
|
|||||||
})
|
})
|
||||||
|
|
||||||
describe('cleanup', () => {
|
describe('cleanup', () => {
|
||||||
|
test('#given session isolation with two tracked panes #when polling closes both sessions #then it reassigns the anchor and cleans up the isolated container', async () => {
|
||||||
|
// given
|
||||||
|
mockIsInsideTmux.mockReturnValue(true)
|
||||||
|
mockQueryWindowState.mockImplementation(async (paneId: string) => {
|
||||||
|
if (paneId === '%isolated-session-ses_first') {
|
||||||
|
return createWindowState({
|
||||||
|
mainPane: {
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
width: 110,
|
||||||
|
height: 44,
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
title: 'isolated',
|
||||||
|
isActive: true,
|
||||||
|
},
|
||||||
|
agentPanes: [
|
||||||
|
{
|
||||||
|
paneId: '%mock',
|
||||||
|
width: 40,
|
||||||
|
height: 44,
|
||||||
|
left: 110,
|
||||||
|
top: 0,
|
||||||
|
title: 'omo-subagent-Second Task',
|
||||||
|
isActive: false,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
if (paneId === '%mock') {
|
||||||
|
return createWindowState({
|
||||||
|
mainPane: {
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
width: 110,
|
||||||
|
height: 44,
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
title: 'isolated',
|
||||||
|
isActive: true,
|
||||||
|
},
|
||||||
|
agentPanes: [
|
||||||
|
{
|
||||||
|
paneId: '%mock',
|
||||||
|
width: 40,
|
||||||
|
height: 44,
|
||||||
|
left: 110,
|
||||||
|
top: 0,
|
||||||
|
title: 'omo-subagent-Second Task',
|
||||||
|
isActive: false,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
return createWindowState()
|
||||||
|
})
|
||||||
|
|
||||||
|
const { TmuxSessionManager } = await import('./manager')
|
||||||
|
const manager = new TmuxSessionManager(createMockContext(), createTmuxConfig({
|
||||||
|
enabled: true,
|
||||||
|
isolation: 'session',
|
||||||
|
}), mockTmuxDeps)
|
||||||
|
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_first', 'ses_parent', 'First Task'))
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_second', 'ses_parent', 'Second Task'))
|
||||||
|
mockExecuteAction.mockClear()
|
||||||
|
|
||||||
|
const closeSessionById = Reflect.get(manager, 'closeSessionById') as (sessionId: string) => Promise<void>
|
||||||
|
|
||||||
|
// when
|
||||||
|
await closeSessionById.call(manager, 'ses_first')
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(mockExecuteAction.mock.calls[0]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_first',
|
||||||
|
})
|
||||||
|
expect(Reflect.get(manager, 'isolatedContainerPaneId')).toBe('%isolated-session-ses_first')
|
||||||
|
expect(Reflect.get(manager, 'isolatedWindowPaneId')).toBe('%mock')
|
||||||
|
|
||||||
|
// when
|
||||||
|
await closeSessionById.call(manager, 'ses_second')
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(mockExecuteAction).toHaveBeenCalledTimes(3)
|
||||||
|
expect(mockExecuteAction.mock.calls[1]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%mock',
|
||||||
|
sessionId: 'ses_second',
|
||||||
|
})
|
||||||
|
expect(mockExecuteAction.mock.calls[2]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_second',
|
||||||
|
})
|
||||||
|
expect(Reflect.get(manager, 'isolatedContainerPaneId')).toBeUndefined()
|
||||||
|
expect(Reflect.get(manager, 'isolatedWindowPaneId')).toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('#given session isolation with two tracked panes #when process shutdown cleanup runs #then it closes panes and the isolated container through the shared close path', async () => {
|
||||||
|
// given
|
||||||
|
mockIsInsideTmux.mockReturnValue(true)
|
||||||
|
mockQueryWindowState.mockImplementation(async (paneId: string) => {
|
||||||
|
if (paneId === '%isolated-session-ses_first') {
|
||||||
|
return createWindowState({
|
||||||
|
mainPane: {
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
width: 110,
|
||||||
|
height: 44,
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
title: 'isolated',
|
||||||
|
isActive: true,
|
||||||
|
},
|
||||||
|
agentPanes: [
|
||||||
|
{
|
||||||
|
paneId: '%mock',
|
||||||
|
width: 40,
|
||||||
|
height: 44,
|
||||||
|
left: 110,
|
||||||
|
top: 0,
|
||||||
|
title: 'omo-subagent-Second Task',
|
||||||
|
isActive: false,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
if (paneId === '%mock') {
|
||||||
|
return createWindowState({
|
||||||
|
mainPane: {
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
width: 110,
|
||||||
|
height: 44,
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
title: 'isolated',
|
||||||
|
isActive: true,
|
||||||
|
},
|
||||||
|
agentPanes: [
|
||||||
|
{
|
||||||
|
paneId: '%mock',
|
||||||
|
width: 40,
|
||||||
|
height: 44,
|
||||||
|
left: 110,
|
||||||
|
top: 0,
|
||||||
|
title: 'omo-subagent-Second Task',
|
||||||
|
isActive: false,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
return createWindowState()
|
||||||
|
})
|
||||||
|
|
||||||
|
const { TmuxSessionManager } = await import('./manager')
|
||||||
|
const manager = new TmuxSessionManager(createMockContext(), createTmuxConfig({
|
||||||
|
enabled: true,
|
||||||
|
isolation: 'session',
|
||||||
|
}), mockTmuxDeps)
|
||||||
|
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_first', 'ses_parent', 'First Task'))
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_second', 'ses_parent', 'Second Task'))
|
||||||
|
mockExecuteAction.mockClear()
|
||||||
|
|
||||||
|
// when
|
||||||
|
await manager.cleanup()
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(mockExecuteAction).toHaveBeenCalledTimes(3)
|
||||||
|
expect(mockExecuteAction.mock.calls[0]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_first',
|
||||||
|
})
|
||||||
|
expect(mockExecuteAction.mock.calls[1]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%mock',
|
||||||
|
sessionId: 'ses_second',
|
||||||
|
})
|
||||||
|
expect(mockExecuteAction.mock.calls[2]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_second',
|
||||||
|
})
|
||||||
|
expect(Reflect.get(manager, 'isolatedContainerPaneId')).toBeUndefined()
|
||||||
|
expect(Reflect.get(manager, 'isolatedWindowPaneId')).toBeUndefined()
|
||||||
|
})
|
||||||
|
|
||||||
|
test('#given an isolated anchor close that fails once #when retryPendingCloses succeeds on retry #then it reassigns the isolated anchor through the shared cleanup path', async () => {
|
||||||
|
// given
|
||||||
|
mockIsInsideTmux.mockReturnValue(true)
|
||||||
|
mockQueryWindowState.mockImplementation(async (paneId: string) => {
|
||||||
|
if (paneId === '%isolated-session-ses_first') {
|
||||||
|
return createWindowState({
|
||||||
|
mainPane: {
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
width: 110,
|
||||||
|
height: 44,
|
||||||
|
left: 0,
|
||||||
|
top: 0,
|
||||||
|
title: 'isolated',
|
||||||
|
isActive: true,
|
||||||
|
},
|
||||||
|
agentPanes: [
|
||||||
|
{
|
||||||
|
paneId: '%mock',
|
||||||
|
width: 40,
|
||||||
|
height: 44,
|
||||||
|
left: 110,
|
||||||
|
top: 0,
|
||||||
|
title: 'omo-subagent-Second Task',
|
||||||
|
isActive: false,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
return createWindowState()
|
||||||
|
})
|
||||||
|
|
||||||
|
let closeAttemptCount = 0
|
||||||
|
mockExecuteAction.mockImplementation(async (action: PaneAction) => {
|
||||||
|
if (action.type === 'close' && action.sessionId === 'ses_first') {
|
||||||
|
closeAttemptCount += 1
|
||||||
|
if (closeAttemptCount === 1) {
|
||||||
|
return { success: false }
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return { success: true }
|
||||||
|
})
|
||||||
|
|
||||||
|
const { TmuxSessionManager } = await import('./manager')
|
||||||
|
const manager = new TmuxSessionManager(createMockContext(), createTmuxConfig({
|
||||||
|
enabled: true,
|
||||||
|
isolation: 'session',
|
||||||
|
}), mockTmuxDeps)
|
||||||
|
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_first', 'ses_parent', 'First Task'))
|
||||||
|
await manager.onSessionCreated(createSessionCreatedEvent('ses_second', 'ses_parent', 'Second Task'))
|
||||||
|
mockExecuteAction.mockClear()
|
||||||
|
|
||||||
|
const closeSessionById = Reflect.get(manager, 'closeSessionById') as (sessionId: string) => Promise<void>
|
||||||
|
const retryPendingCloses = Reflect.get(manager, 'retryPendingCloses') as () => Promise<void>
|
||||||
|
|
||||||
|
// when
|
||||||
|
await closeSessionById.call(manager, 'ses_first')
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(getTrackedSessions(manager).get('ses_first')?.closePending).toBe(true)
|
||||||
|
expect(Reflect.get(manager, 'isolatedWindowPaneId')).toBe('%isolated-session-ses_first')
|
||||||
|
|
||||||
|
// when
|
||||||
|
await retryPendingCloses.call(manager)
|
||||||
|
|
||||||
|
// then
|
||||||
|
expect(getTrackedSessions(manager).has('ses_first')).toBe(false)
|
||||||
|
expect(Reflect.get(manager, 'isolatedContainerPaneId')).toBe('%isolated-session-ses_first')
|
||||||
|
expect(Reflect.get(manager, 'isolatedWindowPaneId')).toBe('%mock')
|
||||||
|
expect(mockExecuteAction.mock.calls[0]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_first',
|
||||||
|
})
|
||||||
|
expect(mockExecuteAction.mock.calls[1]?.[0]).toEqual({
|
||||||
|
type: 'close',
|
||||||
|
paneId: '%isolated-session-ses_first',
|
||||||
|
sessionId: 'ses_first',
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
test('closes all tracked panes', async () => {
|
test('closes all tracked panes', async () => {
|
||||||
// given
|
// given
|
||||||
mockIsInsideTmux.mockReturnValue(true)
|
mockIsInsideTmux.mockReturnValue(true)
|
||||||
|
|||||||
@@ -283,9 +283,11 @@ export class TmuxSessionManager {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private async tryCloseTrackedSession(tracked: TrackedSession): Promise<boolean> {
|
private async closeTrackedSessionPane(args: {
|
||||||
const state = await this.queryWindowStateSafely()
|
tracked: TrackedSession
|
||||||
if (!state) return false
|
state: WindowState
|
||||||
|
}): Promise<boolean> {
|
||||||
|
const { tracked, state } = args
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const result = await executeAction(
|
const result = await executeAction(
|
||||||
@@ -309,6 +311,37 @@ export class TmuxSessionManager {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private async finalizeTrackedSessionClose(args: {
|
||||||
|
tracked: TrackedSession
|
||||||
|
state: WindowState
|
||||||
|
isolatedPaneAlreadyClosed: boolean
|
||||||
|
}): Promise<void> {
|
||||||
|
const { tracked, state, isolatedPaneAlreadyClosed } = args
|
||||||
|
this.removeTrackedSession(tracked.sessionId)
|
||||||
|
await this.cleanupIsolatedContainerAfterSessionDeletion(
|
||||||
|
tracked,
|
||||||
|
isolatedPaneAlreadyClosed,
|
||||||
|
state,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
private async closeTrackedSession(tracked: TrackedSession): Promise<boolean> {
|
||||||
|
const state = await this.queryWindowStateSafely()
|
||||||
|
if (!state) return false
|
||||||
|
|
||||||
|
const closed = await this.closeTrackedSessionPane({ tracked, state })
|
||||||
|
if (!closed) {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
|
||||||
|
await this.finalizeTrackedSessionClose({
|
||||||
|
tracked,
|
||||||
|
state,
|
||||||
|
isolatedPaneAlreadyClosed: true,
|
||||||
|
})
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
private async retryPendingCloses(): Promise<void> {
|
private async retryPendingCloses(): Promise<void> {
|
||||||
const pendingSessions = Array.from(this.sessions.values()).filter(
|
const pendingSessions = Array.from(this.sessions.values()).filter(
|
||||||
(tracked) => tracked.closePending,
|
(tracked) => tracked.closePending,
|
||||||
@@ -327,14 +360,13 @@ export class TmuxSessionManager {
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
const closed = await this.tryCloseTrackedSession(tracked)
|
const closed = await this.closeTrackedSession(tracked)
|
||||||
if (closed) {
|
if (closed) {
|
||||||
log("[tmux-session-manager] retried close succeeded", {
|
log("[tmux-session-manager] retried close succeeded", {
|
||||||
sessionId: tracked.sessionId,
|
sessionId: tracked.sessionId,
|
||||||
paneId: tracked.paneId,
|
paneId: tracked.paneId,
|
||||||
closeRetryCount: tracked.closeRetryCount,
|
closeRetryCount: tracked.closeRetryCount,
|
||||||
})
|
})
|
||||||
this.removeTrackedSession(tracked.sessionId)
|
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -825,8 +857,11 @@ export class TmuxSessionManager {
|
|||||||
|
|
||||||
const closeAction = decideCloseAction(state, event.sessionID, this.getSessionMappings())
|
const closeAction = decideCloseAction(state, event.sessionID, this.getSessionMappings())
|
||||||
if (!closeAction) {
|
if (!closeAction) {
|
||||||
this.removeTrackedSession(event.sessionID)
|
await this.finalizeTrackedSessionClose({
|
||||||
await this.cleanupIsolatedContainerAfterSessionDeletion(tracked, false, state)
|
tracked,
|
||||||
|
state,
|
||||||
|
isolatedPaneAlreadyClosed: false,
|
||||||
|
})
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -854,12 +889,11 @@ export class TmuxSessionManager {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
this.removeTrackedSession(event.sessionID)
|
await this.finalizeTrackedSessionClose({
|
||||||
await this.cleanupIsolatedContainerAfterSessionDeletion(
|
|
||||||
tracked,
|
tracked,
|
||||||
isolatedPaneAlreadyClosed,
|
|
||||||
state,
|
state,
|
||||||
)
|
isolatedPaneAlreadyClosed,
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
@@ -882,13 +916,11 @@ export class TmuxSessionManager {
|
|||||||
paneId: tracked.paneId,
|
paneId: tracked.paneId,
|
||||||
})
|
})
|
||||||
|
|
||||||
const closed = await this.tryCloseTrackedSession(tracked)
|
const closed = await this.closeTrackedSession(tracked)
|
||||||
if (!closed) {
|
if (!closed) {
|
||||||
this.markSessionClosePending(sessionId)
|
this.markSessionClosePending(sessionId)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
this.removeTrackedSession(sessionId)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
createEventHandler(): (input: { event: { type: string; properties?: unknown } }) => Promise<void> {
|
createEventHandler(): (input: { event: { type: string; properties?: unknown } }) => Promise<void> {
|
||||||
|
|||||||
Reference in New Issue
Block a user