diff --git a/docs/design/2026-08-06-active-work-health.md b/docs/design/2026-08-06-active-work-health.md index 7c9672b9dc7..6375f75d338 100644 --- a/docs/design/2026-08-06-active-work-health.md +++ b/docs/design/2026-08-06-active-work-health.md @@ -63,10 +63,10 @@ The cache decides _when_ it is worth asking. It never authorizes destruction, be ``` qwen/control/session/close { sessionId, onlyIfUnheld: true } - → { closed: true, holds: [] } | { closed: false, holds: [...] } + → { sessionId, closed: true } | { sessionId, closed: false, holds: [...] } ``` -The child evaluates it under its own close gate, before anything destructive runs. It rejects known holds immediately, drains any turn that was already active when the gate closed, then evaluates the unfiltered collector again. The second read matters because an already-running out-of-scope turn such as cron can register a background shell while it drains. With the gate still held no new turn can start after that final read, so a hold cannot appear between final authorization and teardown **on the child side**. If either read finds holds, the gate is released and they are handed back; the daemon adopts them and backs off. +The child evaluates it under its own close gate, before anything destructive runs. It rejects known holds immediately, drains any turn that was already active when the gate closed, then evaluates the unfiltered collector again. The second read matters because an already-running out-of-scope turn such as cron can register a background shell while it drains. With the gate still held no new turn can start after that final read, so a hold cannot appear between final authorization and teardown **on the child side**. If either read finds holds, the gate is released and they are handed back. The daemon adopts the returned hold set only when it stays within the same 1,024-hold per-Session bound as snapshots; an oversized refusal still retains the Session but does not replace the last valid cache. The daemon side needs its own cover, because the round trip is an await of up to ten seconds. A Session with a conditional close outstanding is marked in-flight, and every admission path — attach, prompt, rewind — refuses it exactly as it refuses one that is already closing. Without that, a prompt accepted during the round trip is lost when the teardown it raced completes; the previous synchronous guard-then-teardown sequence got this for free, and splitting it is what created the need to say so explicitly. @@ -74,9 +74,9 @@ On timeout the daemon cannot tell whether the child closed. It does not retry in Explicit close, kill, shutdown, and channel exit keep their force semantics and do not go through this path. -## One guard model, four triggers +## One guard model, every trigger -Four things can decide it is time to look at a Session: the last client detaching, a prompt settling, a terminal notification settling, and the idle reaper's TTL. Each brings its own policy, and none of them may weaken the shared part: +Six event families can decide it is time to look at a Session: the last client detaching, a prompt settling, a terminal notification settling, an attach registration rolling back, a full child snapshot reporting the Session idle or omitting it, and the idle reaper's TTL. Each brings its own policy, and none of them may weaken the shared part: | Guard | Why it is shared | | ------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | diff --git a/docs/design/daemon-global-deep-health.md b/docs/design/daemon-global-deep-health.md index 0cac12b09ee..ed98804b070 100644 --- a/docs/design/daemon-global-deep-health.md +++ b/docs/design/daemon-global-deep-health.md @@ -24,7 +24,7 @@ that are draining but have not completed bridge cleanup. | `pendingPermissions` | Sum | | `activePrompts` | Sum | | `activeWork` | True when any managed runtime reports active work | -| `activeWorkReporting` | Worst grade across runtimes (`full`/`partial`/`none`) | +| `activeWorkReporting` | Grade once after summing coverage counts across runtimes | | `activeWorkStaleMs` | Age of the oldest snapshot behind it; `0` when uncovered | | `connectedClients` | Existing daemon-wide REST SSE count | | `channelAlive` | True when any managed runtime channel is live | diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 9cd4fb0841a..92ff49a4649 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -769,6 +769,93 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it.each([ + { + count: ACTIVE_WORK_MAX_SESSION_HOLDS, + expectedActiveWork: true, + }, + { + count: ACTIVE_WORK_MAX_SESSION_HOLDS + 1, + expectedActiveWork: false, + }, + ])( + 'bounds close-refusal adoption at $count holds', + async ({ count, expectedActiveWork }) => { + const refusalHolds = Array.from({ length: count }, (_unused, index) => + agentHold(`h${index}`), + ); + const handle = makeChannel({ + initializeImpl: () => activeWorkInitializeResponse(), + extMethodImpl: async (method, params) => { + if (method !== SERVE_CONTROL_EXT_METHODS.sessionClose) return {}; + if (params?.[ACTIVE_WORK_CLOSE_IF_UNHELD_PARAM] === true) { + return { closed: false, holds: refusalHolds }; + } + return { closed: true }; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + sessionReapIntervalMs: 0, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + await sendActiveWorkSnapshot(handle, 1, [ + { sessionId: session.sessionId, holds: [] }, + ]); + + await bridge.detachClient(session.sessionId, session.clientId); + + expect(bridge.sessionCount).toBe(1); + expect(bridge.activeWork).toBe(expectedActiveWork); + expect(reportingGrade(bridge)).toBe('full'); + + await bridge.shutdown(); + }, + ); + + it('retains a stale non-empty cache after an oversized close refusal', async () => { + const refusalHolds = Array.from( + { length: ACTIVE_WORK_MAX_SESSION_HOLDS + 1 }, + (_unused, index) => agentHold(`h${index}`), + ); + const handle = makeChannel({ + initializeImpl: () => activeWorkInitializeResponse(), + extMethodImpl: async (method, params) => { + if (method !== SERVE_CONTROL_EXT_METHODS.sessionClose) return {}; + if (params?.[ACTIVE_WORK_CLOSE_IF_UNHELD_PARAM] === true) { + return { closed: false, holds: refusalHolds }; + } + return { closed: true }; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + sessionReapIntervalMs: 0, + }); + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + await sendActiveWorkSnapshot(handle, 1, [ + { sessionId: session.sessionId, holds: [agentHold('cached-agent')] }, + ]); + + vi.useFakeTimers(); + try { + vi.setSystemTime( + Date.now() + + ACTIVE_WORK_HEARTBEAT_INTERVAL_MS * ACTIVE_WORK_STALE_INTERVALS + + 1_000, + ); + await bridge.detachClient(session.sessionId, session.clientId); + + expect(bridge.sessionCount).toBe(1); + expect(bridge.activeWork).toBe(true); + expect(reportingGrade(bridge)).toBe('partial'); + } finally { + vi.useRealTimers(); + } + + await bridge.shutdown(); + }); + it('leaves the session in place when the close request never resolves', async () => { let closeAttempts = 0; const handle = makeChannel({ diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index e933175c66a..f559eac52a0 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -133,6 +133,7 @@ import { ACTIVE_WORK_HEARTBEAT_META_KEY, ACTIVE_WORK_HEARTBEAT_VERSION, ACTIVE_WORK_HOLD_CATEGORIES, + ACTIVE_WORK_MAX_SESSION_HOLDS, ACTIVE_WORK_STALE_INTERVALS, clampActiveWorkIntervalMs, type ActiveWorkHeartbeatCapabilityV1, @@ -3133,10 +3134,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // The child is done with it; only local teardown remains. return true; } - // Refused: adopt the hold set it handed back so the cache reflects the - // reason we are backing off rather than the stale set that sent us here. + // Refused: adopt a bounded hold set so the cache reflects the reason we + // are backing off rather than the stale set that sent us here. const holds = response['holds']; - if (Array.isArray(holds)) { + if ( + Array.isArray(holds) && + holds.length <= ACTIVE_WORK_MAX_SESSION_HOLDS + ) { const adopted = new Map(); for (const hold of holds) { if (typeof hold !== 'object' || hold === null) continue; diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index dc754494396..b43939ec37f 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -281,11 +281,12 @@ export const ACTIVE_WORK_CLOSE_TIMEOUT_MS = 10_000; export function sessionCloseDrainBudgetMs(outerWaitMs: number): number { return Math.max(1, Math.floor(outerWaitMs * 0.8)); } -/** Bounds on a single snapshot. Generous next to any real deployment — they - * exist so a version-skewed or buggy child cannot make the daemon walk an - * unbounded structure per report, not to constrain legitimate use. A packet - * over either bound is discarded whole, like any other malformed one. */ +/** Bounds on a single snapshot. Generous next to any real deployment — it + * exists so a version-skewed or buggy child cannot make the daemon walk an + * unbounded Session list per report. An oversized packet is discarded whole. */ export const ACTIVE_WORK_MAX_SNAPSHOT_SESSIONS = 1024; +/** Shared per-Session hold bound. Oversized snapshots are discarded whole; + * oversized close refusals retain the Session without replacing its cache. */ export const ACTIVE_WORK_MAX_SESSION_HOLDS = 1024; export const WORKTREE_MCP_DEFER_META_KEY = 'qwen.session.deferMcpDiscovery';