mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 10:14:19 -05:00
fix(query): add activity-aware query guard leases (#1686)
* fix(query): add activity-aware query guard leases * test(query): cover tool query activity lifecycle * fix(shell): align runtime and lease timeouts * fix(query): cap shell timeouts to query budget
This commit is contained in:
+11
@@ -11,6 +11,10 @@ import type { z } from 'zod/v4'
|
||||
import type { Command } from './commands.js'
|
||||
import type { CanUseToolFn } from './hooks/useCanUseTool.js'
|
||||
import type { ThinkingConfig } from './utils/thinking.js'
|
||||
import type {
|
||||
QueryGuardLease,
|
||||
QueryGuardLeaseInput,
|
||||
} from './utils/QueryGuard.js'
|
||||
|
||||
export type ToolInputJSONSchema = {
|
||||
[x: string]: unknown
|
||||
@@ -92,6 +96,11 @@ export type QueryChainTracking = {
|
||||
depth: number
|
||||
}
|
||||
|
||||
export type QueryActivity = {
|
||||
registerActivity(reason: string): void
|
||||
acquireLease(input: QueryGuardLeaseInput): QueryGuardLease
|
||||
}
|
||||
|
||||
export type ValidationResult =
|
||||
| { result: true }
|
||||
| {
|
||||
@@ -230,6 +239,8 @@ export type ToolUseContext = {
|
||||
setInProgressToolUseIDs: (f: (prev: Set<string>) => Set<string>) => void
|
||||
/** Only wired in interactive (REPL) contexts; SDK/QueryEngine don't set this. */
|
||||
setHasInterruptibleToolInProgress?: (v: boolean) => void
|
||||
/** Only wired in guarded REPL turns. Lets streaming/tool work update QueryGuard. */
|
||||
queryActivity?: QueryActivity
|
||||
setResponseLength: (f: (prev: number) => number) => void
|
||||
/** Ant-only: push a new API metrics entry for OTPS tracking.
|
||||
* Called by subagent streaming when a new API request starts. */
|
||||
|
||||
+14
-6
@@ -2503,7 +2503,7 @@ export function REPL({
|
||||
reject
|
||||
}]);
|
||||
}), []);
|
||||
const getToolUseContext = useCallback((messages: MessageType[], newMessages: MessageType[], abortController: AbortController, mainLoopModel: string): ProcessUserInputContext => {
|
||||
const getToolUseContext = useCallback((messages: MessageType[], newMessages: MessageType[], abortController: AbortController, mainLoopModel: string, queryGeneration?: number): ProcessUserInputContext => {
|
||||
// Read mutable values fresh from the store rather than closure-capturing
|
||||
// useAppState() snapshots. Same values today (closure is refreshed by the
|
||||
// render between turns); decouples freshness from React's render cycle for
|
||||
@@ -2522,6 +2522,12 @@ export function REPL({
|
||||
if (!mainThreadAgentDefinition) return merged;
|
||||
return resolveAgentTools(mainThreadAgentDefinition, merged, false, true).resolvedTools;
|
||||
};
|
||||
const queryActivity = queryGeneration === undefined ? undefined : {
|
||||
registerActivity: (reason: string) => {
|
||||
queryGuard.registerActivity(reason, queryGeneration);
|
||||
},
|
||||
acquireLease: (input) => queryGuard.acquireLease(input, queryGeneration)
|
||||
} satisfies ProcessUserInputContext['queryActivity'];
|
||||
return {
|
||||
abortController,
|
||||
options: {
|
||||
@@ -2623,6 +2629,7 @@ export function REPL({
|
||||
setHasInterruptibleToolInProgress: (v: boolean) => {
|
||||
hasInterruptibleToolInProgressRef.current = v;
|
||||
},
|
||||
queryActivity,
|
||||
resume,
|
||||
setConversationId,
|
||||
setActiveSessionAgent: agent => {
|
||||
@@ -2654,7 +2661,7 @@ export function REPL({
|
||||
contentReplacementState: contentReplacementStateRef.current,
|
||||
syncToolResultReplacements
|
||||
};
|
||||
}, [commands, combinedInitialTools, mainThreadAgentDefinition, debug, initialMcpClients, ideInstallationStatus, dynamicMcpConfig, theme, allowedAgentTypes, store, setAppState, reverify, addNotification, setMessages, onChangeDynamicMcpConfig, resume, requestPrompt, disabled, customSystemPrompt, appendSystemPrompt, setConversationId, syncToolResultReplacements]);
|
||||
}, [commands, combinedInitialTools, mainThreadAgentDefinition, debug, initialMcpClients, ideInstallationStatus, dynamicMcpConfig, theme, allowedAgentTypes, store, setAppState, reverify, addNotification, setMessages, onChangeDynamicMcpConfig, resume, requestPrompt, disabled, customSystemPrompt, appendSystemPrompt, setConversationId, syncToolResultReplacements, queryGuard]);
|
||||
|
||||
// Session backgrounding (Ctrl+B to background/foreground)
|
||||
const handleBackgroundQuery = useCallback(() => {
|
||||
@@ -2786,7 +2793,7 @@ export function REPL({
|
||||
void removeTranscriptMessage(tombstonedMessage.uuid);
|
||||
}, setStreamingThinking, undefined, onStreamingText);
|
||||
}, [setMessages, setResponseLength, setStreamMode, setStreamingToolUses, setStreamingThinking, onStreamingText]);
|
||||
const onQueryImpl = useCallback(async (messagesIncludingNewMessages: MessageType[], newMessages: MessageType[], abortController: AbortController, shouldQuery: boolean, additionalAllowedTools: string[], mainLoopModelParam: string, effort?: EffortValue) => {
|
||||
const onQueryImpl = useCallback(async (messagesIncludingNewMessages: MessageType[], newMessages: MessageType[], abortController: AbortController, shouldQuery: boolean, additionalAllowedTools: string[], mainLoopModelParam: string, queryGeneration: number, effort?: EffortValue) => {
|
||||
// Prepare IDE integration for new prompt. Read mcpClients fresh from
|
||||
// store — useManageMCPConnections may have populated it since the
|
||||
// render that captured this closure (same pattern as computeTools).
|
||||
@@ -2872,7 +2879,7 @@ export function REPL({
|
||||
setAbortController(null);
|
||||
return;
|
||||
}
|
||||
const toolUseContext = getToolUseContext(messagesIncludingNewMessages, newMessages, abortController, mainLoopModelParam);
|
||||
const toolUseContext = getToolUseContext(messagesIncludingNewMessages, newMessages, abortController, mainLoopModelParam, queryGeneration);
|
||||
const querySessionId = getSessionId();
|
||||
const queryAutoCompactTracking = getAutoCompactTrackingForSession(querySessionId);
|
||||
// getToolUseContext reads tools/mcpClients fresh from store.getState()
|
||||
@@ -2938,6 +2945,7 @@ export function REPL({
|
||||
}
|
||||
}
|
||||
})) {
|
||||
queryGuard.registerActivity(`query_event:${event.type}`, queryGeneration);
|
||||
onQueryEvent(event);
|
||||
}
|
||||
if (isBuddyEnabled()) {
|
||||
@@ -2955,7 +2963,7 @@ export function REPL({
|
||||
|
||||
// Signal that a query turn has completed successfully
|
||||
await onTurnComplete?.(messagesRef.current);
|
||||
}, [initialMcpClients, resetLoadingState, getToolUseContext, toolPermissionContext, setAppState, customSystemPrompt, onTurnComplete, appendSystemPrompt, canUseTool, mainThreadAgentDefinition, onQueryEvent, sessionTitle, titleDisabled, getAutoCompactTrackingForSession, setAutoCompactTrackingForSession, setAutoCompactTrackingForSessionIfUnchanged]);
|
||||
}, [initialMcpClients, resetLoadingState, getToolUseContext, toolPermissionContext, setAppState, customSystemPrompt, onTurnComplete, appendSystemPrompt, canUseTool, mainThreadAgentDefinition, onQueryEvent, sessionTitle, titleDisabled, getAutoCompactTrackingForSession, setAutoCompactTrackingForSession, setAutoCompactTrackingForSessionIfUnchanged, queryGuard]);
|
||||
const onQuery = useCallback(async (newMessages: MessageType[], abortController: AbortController, shouldQuery: boolean, additionalAllowedTools: string[], mainLoopModelParam: string, onBeforeQueryCallback?: (input: string, newMessages: MessageType[]) => Promise<boolean>, input?: string, effort?: EffortValue): Promise<void> => {
|
||||
// If this is a teammate, mark them as active when starting a turn
|
||||
if (isAgentSwarmsEnabled()) {
|
||||
@@ -3025,7 +3033,7 @@ export function REPL({
|
||||
return;
|
||||
}
|
||||
}
|
||||
await onQueryImpl(latestMessages, newMessages, abortController, shouldQuery, additionalAllowedTools, mainLoopModelParam, effort);
|
||||
await onQueryImpl(latestMessages, newMessages, abortController, shouldQuery, additionalAllowedTools, mainLoopModelParam, thisGeneration, effort);
|
||||
} finally {
|
||||
// queryGuard.end() atomically checks generation and transitions
|
||||
// running→idle. Returns false if a newer query owns the guard
|
||||
|
||||
@@ -0,0 +1,331 @@
|
||||
import type { ToolUseBlock } from '@anthropic-ai/sdk/resources/index.mjs'
|
||||
import { afterEach, describe, expect, test, vi } from 'vitest'
|
||||
import { z } from 'zod/v4'
|
||||
import type { CanUseToolFn } from '../../hooks/useCanUseTool.js'
|
||||
import {
|
||||
buildTool,
|
||||
type Tool,
|
||||
type ToolUseContext,
|
||||
type Tools,
|
||||
} from '../../Tool.js'
|
||||
import { BASH_TOOL_NAME } from '../../tools/BashTool/toolName.js'
|
||||
import { POWERSHELL_TOOL_NAME } from '../../tools/PowerShellTool/toolName.js'
|
||||
import { createAssistantMessage } from '../../utils/messages.js'
|
||||
import { createToolQueryLeaseInput } from './queryActivityLease.js'
|
||||
import { type MessageUpdateLazy, runToolUse } from './toolExecution.js'
|
||||
|
||||
const ORIGINAL_BASH_DEFAULT_TIMEOUT_MS = process.env.BASH_DEFAULT_TIMEOUT_MS
|
||||
const ORIGINAL_BASH_MAX_TIMEOUT_MS = process.env.BASH_MAX_TIMEOUT_MS
|
||||
const shellInputSchema = z.object({
|
||||
command: z.string(),
|
||||
timeout: z.number().optional(),
|
||||
run_in_background: z.boolean().optional(),
|
||||
})
|
||||
type ShellInputSchema = typeof shellInputSchema
|
||||
type FakeShellTool = Tool<ShellInputSchema, string>
|
||||
|
||||
function setShellTimeoutEnv(defaultTimeoutMs = '180000', maxTimeoutMs = '600000') {
|
||||
process.env.BASH_DEFAULT_TIMEOUT_MS = defaultTimeoutMs
|
||||
process.env.BASH_MAX_TIMEOUT_MS = maxTimeoutMs
|
||||
}
|
||||
|
||||
function createPowerShellTool(call: FakeShellTool['call']): FakeShellTool {
|
||||
return buildTool({
|
||||
name: POWERSHELL_TOOL_NAME,
|
||||
inputSchema: shellInputSchema,
|
||||
maxResultSizeChars: Infinity,
|
||||
async description() {
|
||||
return 'Run a PowerShell command'
|
||||
},
|
||||
async prompt() {
|
||||
return ''
|
||||
},
|
||||
call,
|
||||
mapToolResultToToolResultBlockParam(content, toolUseID) {
|
||||
return {
|
||||
type: 'tool_result',
|
||||
tool_use_id: toolUseID,
|
||||
content,
|
||||
}
|
||||
},
|
||||
renderToolUseMessage() {
|
||||
return null
|
||||
},
|
||||
renderToolResultMessage() {
|
||||
return null
|
||||
},
|
||||
})
|
||||
}
|
||||
|
||||
function createQueryActivityHarness() {
|
||||
const release = vi.fn()
|
||||
const acquireLease = vi.fn(
|
||||
(input: Parameters<NonNullable<ToolUseContext['queryActivity']>['acquireLease']>[0]) => ({
|
||||
id: `lease:${input.owner}:${input.id}`,
|
||||
release,
|
||||
}),
|
||||
)
|
||||
const registerActivity = vi.fn((_reason: string) => {})
|
||||
|
||||
return {
|
||||
queryActivity: {
|
||||
acquireLease,
|
||||
registerActivity,
|
||||
},
|
||||
acquireLease,
|
||||
registerActivity,
|
||||
release,
|
||||
}
|
||||
}
|
||||
|
||||
function createToolUseContext(
|
||||
tools: Tools,
|
||||
queryActivity: NonNullable<ToolUseContext['queryActivity']>,
|
||||
): ToolUseContext {
|
||||
return {
|
||||
abortController: new AbortController(),
|
||||
getAppState: () => ({
|
||||
fastMode: false,
|
||||
mcp: { tools: {}, clients: [] },
|
||||
sessionHooks: new Map(),
|
||||
settings: {},
|
||||
toolPermissionContext: { mode: 'default' },
|
||||
}),
|
||||
setAppState: () => {},
|
||||
options: {
|
||||
commands: [],
|
||||
debug: false,
|
||||
thinkingConfig: { type: 'disabled' },
|
||||
tools,
|
||||
verbose: false,
|
||||
mcpClients: [],
|
||||
mcpResources: {},
|
||||
isNonInteractiveSession: false,
|
||||
agentDefinitions: { activeAgents: [], allowedAgentTypes: undefined },
|
||||
mainLoopModel: 'gpt-4o',
|
||||
},
|
||||
messages: [],
|
||||
queryActivity,
|
||||
setInProgressToolUseIDs: () => {},
|
||||
setResponseLength: () => {},
|
||||
updateFileHistoryState: () => {},
|
||||
updateAttributionState: () => {},
|
||||
} as unknown as ToolUseContext
|
||||
}
|
||||
|
||||
async function runFakeToolUse(
|
||||
tool: FakeShellTool,
|
||||
queryActivity: NonNullable<ToolUseContext['queryActivity']>,
|
||||
) {
|
||||
const toolUse = {
|
||||
type: 'tool_use',
|
||||
id: 'toolu_lifecycle',
|
||||
name: POWERSHELL_TOOL_NAME,
|
||||
input: {
|
||||
command: 'npm test',
|
||||
timeout: 120_000,
|
||||
},
|
||||
} as ToolUseBlock
|
||||
const context = createToolUseContext([tool], queryActivity)
|
||||
const assistantMessage = createAssistantMessage({ content: 'run tool' })
|
||||
const canUseTool: CanUseToolFn = async (_tool, input) => ({
|
||||
behavior: 'allow',
|
||||
updatedInput: input,
|
||||
})
|
||||
const updates: MessageUpdateLazy[] = []
|
||||
|
||||
for await (const update of runToolUse(
|
||||
toolUse,
|
||||
assistantMessage,
|
||||
canUseTool,
|
||||
context,
|
||||
)) {
|
||||
updates.push(update)
|
||||
}
|
||||
|
||||
return updates
|
||||
}
|
||||
|
||||
describe('query activity leases for tools', () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
|
||||
if (ORIGINAL_BASH_DEFAULT_TIMEOUT_MS === undefined) {
|
||||
delete process.env.BASH_DEFAULT_TIMEOUT_MS
|
||||
} else {
|
||||
process.env.BASH_DEFAULT_TIMEOUT_MS = ORIGINAL_BASH_DEFAULT_TIMEOUT_MS
|
||||
}
|
||||
|
||||
if (ORIGINAL_BASH_MAX_TIMEOUT_MS === undefined) {
|
||||
delete process.env.BASH_MAX_TIMEOUT_MS
|
||||
} else {
|
||||
process.env.BASH_MAX_TIMEOUT_MS = ORIGINAL_BASH_MAX_TIMEOUT_MS
|
||||
}
|
||||
})
|
||||
|
||||
test('foreground Bash with explicit timeout gets a bounded lease', () => {
|
||||
setShellTimeoutEnv()
|
||||
|
||||
const leaseInput = createToolQueryLeaseInput(BASH_TOOL_NAME, 'toolu_1', {
|
||||
command: 'bun test',
|
||||
timeout: 600_000,
|
||||
run_in_background: false,
|
||||
})
|
||||
|
||||
expect(leaseInput).toEqual({
|
||||
owner: 'bash',
|
||||
id: 'toolu_1',
|
||||
timeoutMs: 600_000,
|
||||
description: BASH_TOOL_NAME,
|
||||
})
|
||||
})
|
||||
|
||||
test('foreground PowerShell with explicit timeout gets a bounded lease', () => {
|
||||
setShellTimeoutEnv()
|
||||
|
||||
const leaseInput = createToolQueryLeaseInput(POWERSHELL_TOOL_NAME, 'toolu_ps', {
|
||||
command: 'npm test',
|
||||
timeout: 120_000,
|
||||
})
|
||||
|
||||
expect(leaseInput).toEqual({
|
||||
owner: 'powershell',
|
||||
id: 'toolu_ps',
|
||||
timeoutMs: 120_000,
|
||||
description: POWERSHELL_TOOL_NAME,
|
||||
})
|
||||
})
|
||||
|
||||
test('foreground Bash without explicit timeout uses the safe default timeout', () => {
|
||||
setShellTimeoutEnv()
|
||||
|
||||
const leaseInput = createToolQueryLeaseInput(BASH_TOOL_NAME, 'toolu_2', {
|
||||
command: 'bun run build',
|
||||
})
|
||||
|
||||
expect(leaseInput).toEqual({
|
||||
owner: 'bash',
|
||||
id: 'toolu_2',
|
||||
timeoutMs: 180_000,
|
||||
description: BASH_TOOL_NAME,
|
||||
})
|
||||
})
|
||||
|
||||
test('foreground Bash explicit timeout is clamped to the configured maximum', () => {
|
||||
setShellTimeoutEnv('120000', '300000')
|
||||
|
||||
const leaseInput = createToolQueryLeaseInput(BASH_TOOL_NAME, 'toolu_clamped', {
|
||||
command: 'bun run slow-check',
|
||||
timeout: 900_000,
|
||||
})
|
||||
|
||||
expect(leaseInput).toEqual({
|
||||
owner: 'bash',
|
||||
id: 'toolu_clamped',
|
||||
timeoutMs: 300_000,
|
||||
description: BASH_TOOL_NAME,
|
||||
})
|
||||
})
|
||||
|
||||
test('foreground Bash invalid timeout values fall back to the safe default', () => {
|
||||
setShellTimeoutEnv('150000', '600000')
|
||||
|
||||
for (const timeout of [0, -1, Number.NaN, Number.POSITIVE_INFINITY, '60000']) {
|
||||
const leaseInput = createToolQueryLeaseInput(BASH_TOOL_NAME, `toolu_${String(timeout)}`, {
|
||||
command: 'bun test',
|
||||
timeout,
|
||||
})
|
||||
|
||||
expect(leaseInput).toEqual({
|
||||
owner: 'bash',
|
||||
id: `toolu_${String(timeout)}`,
|
||||
timeoutMs: 150_000,
|
||||
description: BASH_TOOL_NAME,
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
test('explicit background shell commands skip foreground query leases', () => {
|
||||
setShellTimeoutEnv()
|
||||
|
||||
const leaseInput = createToolQueryLeaseInput(POWERSHELL_TOOL_NAME, 'toolu_3', {
|
||||
command: 'Start-Sleep -Seconds 60',
|
||||
run_in_background: true,
|
||||
})
|
||||
|
||||
expect(leaseInput).toBeNull()
|
||||
})
|
||||
|
||||
test('non-shell tools skip query leases', () => {
|
||||
const leaseInput = createToolQueryLeaseInput('Read', 'toolu_4', {
|
||||
file_path: 'README.md',
|
||||
})
|
||||
|
||||
expect(leaseInput).toBeNull()
|
||||
})
|
||||
|
||||
test('non-record tool inputs skip query leases', () => {
|
||||
expect(createToolQueryLeaseInput(BASH_TOOL_NAME, 'toolu_array', [])).toBeNull()
|
||||
expect(createToolQueryLeaseInput(BASH_TOOL_NAME, 'toolu_null', null)).toBeNull()
|
||||
})
|
||||
|
||||
test('successful shell tool execution reports the full query activity lease lifecycle', async () => {
|
||||
setShellTimeoutEnv()
|
||||
const harness = createQueryActivityHarness()
|
||||
const tool = createPowerShellTool(vi.fn(async (
|
||||
_input,
|
||||
_context,
|
||||
_canUseTool,
|
||||
_parentMessage,
|
||||
onProgress,
|
||||
) => {
|
||||
onProgress?.({
|
||||
toolUseID: 'toolu_lifecycle',
|
||||
data: { type: 'powershell_progress', text: 'running' },
|
||||
})
|
||||
|
||||
return { data: 'ok' }
|
||||
}))
|
||||
|
||||
const updates = await runFakeToolUse(tool, harness.queryActivity)
|
||||
|
||||
expect(updates.length).toBeGreaterThan(0)
|
||||
expect(harness.acquireLease).toHaveBeenCalledTimes(1)
|
||||
expect(harness.acquireLease).toHaveBeenCalledWith({
|
||||
owner: 'powershell',
|
||||
id: 'toolu_lifecycle',
|
||||
timeoutMs: 120_000,
|
||||
description: POWERSHELL_TOOL_NAME,
|
||||
})
|
||||
expect(harness.registerActivity.mock.calls.map(([reason]) => reason)).toEqual([
|
||||
`tool:${POWERSHELL_TOOL_NAME}:start`,
|
||||
`tool:${POWERSHELL_TOOL_NAME}:progress`,
|
||||
`tool:${POWERSHELL_TOOL_NAME}:end`,
|
||||
])
|
||||
expect(harness.release).toHaveBeenCalledTimes(1)
|
||||
expect(harness.release.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
harness.registerActivity.mock.invocationCallOrder[2],
|
||||
)
|
||||
})
|
||||
|
||||
test('thrown shell tool execution still releases the query activity lease', async () => {
|
||||
setShellTimeoutEnv()
|
||||
const harness = createQueryActivityHarness()
|
||||
const tool = createPowerShellTool(vi.fn(async () => {
|
||||
throw new Error('tool exploded')
|
||||
}))
|
||||
|
||||
const updates = await runFakeToolUse(tool, harness.queryActivity)
|
||||
|
||||
expect(updates.length).toBeGreaterThan(0)
|
||||
expect(harness.acquireLease).toHaveBeenCalledTimes(1)
|
||||
expect(harness.registerActivity.mock.calls.map(([reason]) => reason)).toEqual([
|
||||
`tool:${POWERSHELL_TOOL_NAME}:start`,
|
||||
`tool:${POWERSHELL_TOOL_NAME}:end`,
|
||||
])
|
||||
expect(harness.release).toHaveBeenCalledTimes(1)
|
||||
expect(harness.release.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
harness.registerActivity.mock.invocationCallOrder[1],
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,42 @@
|
||||
import type { QueryGuardLeaseInput } from '../../utils/QueryGuard.js'
|
||||
import { getEffectiveBashTimeoutMs } from '../../utils/timeouts.js'
|
||||
import { BASH_TOOL_NAME } from '../../tools/BashTool/toolName.js'
|
||||
import { POWERSHELL_TOOL_NAME } from '../../tools/PowerShellTool/toolName.js'
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === 'object' && value !== null && !Array.isArray(value)
|
||||
}
|
||||
|
||||
function getShellTimeoutMs(input: Record<string, unknown>): number {
|
||||
// PowerShell already shares the Bash timeout helpers and env vars; keep that
|
||||
// compatibility surface stable here.
|
||||
return getEffectiveBashTimeoutMs(input.timeout)
|
||||
}
|
||||
|
||||
export function createToolQueryLeaseInput(
|
||||
toolName: string,
|
||||
toolUseID: string,
|
||||
input: unknown,
|
||||
): QueryGuardLeaseInput | null {
|
||||
if (!isRecord(input)) {
|
||||
return null
|
||||
}
|
||||
|
||||
if (
|
||||
toolName !== BASH_TOOL_NAME &&
|
||||
toolName !== POWERSHELL_TOOL_NAME
|
||||
) {
|
||||
return null
|
||||
}
|
||||
|
||||
if (input.run_in_background === true) {
|
||||
return null
|
||||
}
|
||||
|
||||
return {
|
||||
owner: toolName === BASH_TOOL_NAME ? 'bash' : 'powershell',
|
||||
id: toolUseID,
|
||||
timeoutMs: getShellTimeoutMs(input),
|
||||
description: toolName,
|
||||
}
|
||||
}
|
||||
@@ -108,6 +108,7 @@ import {
|
||||
} from '../mcp/client.js'
|
||||
import { mcpInfoFromString } from '../mcp/mcpStringUtils.js'
|
||||
import { normalizeNameForMCP } from '../mcp/normalization.js'
|
||||
import { createToolQueryLeaseInput } from './queryActivityLease.js'
|
||||
import type { MCPServerConnection } from '../mcp/types.js'
|
||||
import {
|
||||
getLoggingSafeMcpBaseUrl,
|
||||
@@ -1250,7 +1251,18 @@ async function checkPermissionsAndCallTool(
|
||||
} else if (processedInput !== backfilledClone) {
|
||||
callInput = processedInput
|
||||
}
|
||||
let queryActivityLease: { release(): void } | undefined
|
||||
try {
|
||||
const queryActivityLeaseInput = createToolQueryLeaseInput(
|
||||
tool.name,
|
||||
toolUseID,
|
||||
callInput,
|
||||
)
|
||||
queryActivityLease = queryActivityLeaseInput
|
||||
? toolUseContext.queryActivity?.acquireLease(queryActivityLeaseInput)
|
||||
: undefined
|
||||
toolUseContext.queryActivity?.registerActivity(`tool:${tool.name}:start`)
|
||||
|
||||
const result = await tool.call(
|
||||
callInput,
|
||||
{
|
||||
@@ -1262,6 +1274,9 @@ async function checkPermissionsAndCallTool(
|
||||
canUseTool,
|
||||
assistantMessage,
|
||||
progress => {
|
||||
toolUseContext.queryActivity?.registerActivity(
|
||||
`tool:${tool.name}:progress`,
|
||||
)
|
||||
onToolProgress({
|
||||
toolUseID: progress.toolUseID,
|
||||
data: progress.data,
|
||||
@@ -1704,10 +1719,15 @@ async function checkPermissionsAndCallTool(
|
||||
...hookMessages,
|
||||
]
|
||||
} finally {
|
||||
stopSessionActivity('tool_exec')
|
||||
// Clean up decision info after logging
|
||||
if (decisionInfo) {
|
||||
toolUseContext.toolDecisions?.delete(toolUseID)
|
||||
try {
|
||||
queryActivityLease?.release()
|
||||
toolUseContext.queryActivity?.registerActivity(`tool:${tool.name}:end`)
|
||||
} finally {
|
||||
stopSessionActivity('tool_exec')
|
||||
// Clean up decision info after logging
|
||||
if (decisionInfo) {
|
||||
toolUseContext.toolDecisions?.delete(toolUseID)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -42,7 +42,7 @@ import { userFacingName as fileEditUserFacingName } from '../FileEditTool/UI.js'
|
||||
import { trackGitOperations } from '../shared/gitOperationTracking.js';
|
||||
import { bashToolHasPermission, commandHasAnyCd, matchWildcardPattern, permissionRuleExtractPrefix } from './bashPermissions.js';
|
||||
import { interpretCommandResult } from './commandSemantics.js';
|
||||
import { getDefaultTimeoutMs, getMaxTimeoutMs, getSimplePrompt } from './prompt.js';
|
||||
import { getEffectiveTimeoutMs, getMaxTimeoutMs, getSimplePrompt } from './prompt.js';
|
||||
import { checkReadOnlyConstraints } from './readOnlyValidation.js';
|
||||
import { parseSedEditCommand } from './sedEditParser.js';
|
||||
import { shouldUseSandbox } from './shouldUseSandbox.js';
|
||||
@@ -894,7 +894,7 @@ async function* runShellCommand({
|
||||
timeout,
|
||||
run_in_background
|
||||
} = input;
|
||||
const timeoutMs = timeout || getDefaultTimeoutMs();
|
||||
const timeoutMs = getEffectiveTimeoutMs(timeout);
|
||||
let fullOutput = '';
|
||||
let lastProgressOutput = '';
|
||||
let lastTotalLines = 0;
|
||||
|
||||
@@ -9,6 +9,7 @@ import { SandboxManager } from '../../utils/sandbox/sandbox-adapter.js'
|
||||
import { jsonStringify } from '../../utils/slowOperations.js'
|
||||
import {
|
||||
getDefaultBashTimeoutMs,
|
||||
getEffectiveBashTimeoutMs,
|
||||
getMaxBashTimeoutMs,
|
||||
} from '../../utils/timeouts.js'
|
||||
import {
|
||||
@@ -32,6 +33,10 @@ export function getMaxTimeoutMs(): number {
|
||||
return getMaxBashTimeoutMs()
|
||||
}
|
||||
|
||||
export function getEffectiveTimeoutMs(timeout: unknown): number {
|
||||
return getEffectiveBashTimeoutMs(timeout)
|
||||
}
|
||||
|
||||
function getBackgroundUsageNote(): string | null {
|
||||
if (isEnvTruthy(process.env.CLAUDE_CODE_DISABLE_BACKGROUND_TASKS)) {
|
||||
return null
|
||||
|
||||
@@ -39,7 +39,7 @@ import { buildImageToolResult, isImageOutput, resetCwdIfOutsideProject, resizeSh
|
||||
import { trackGitOperations } from '../shared/gitOperationTracking.js';
|
||||
import { interpretCommandResult } from './commandSemantics.js';
|
||||
import { powershellToolHasPermission } from './powershellPermissions.js';
|
||||
import { getDefaultTimeoutMs, getMaxTimeoutMs, getPrompt } from './prompt.js';
|
||||
import { getEffectiveTimeoutMs, getMaxTimeoutMs, getPrompt } from './prompt.js';
|
||||
import { hasSyncSecurityConcerns, isReadOnlyCommand, resolveToCanonical } from './readOnlyValidation.js';
|
||||
import { POWERSHELL_TOOL_NAME } from './toolName.js';
|
||||
import { renderToolResultMessage, renderToolUseErrorMessage, renderToolUseMessage, renderToolUseProgressMessage, renderToolUseQueuedMessage } from './UI.js';
|
||||
@@ -707,7 +707,7 @@ async function* runPowerShellCommand({
|
||||
dangerouslyDisableSandbox,
|
||||
_dangerouslyDisableSandboxApproved
|
||||
} = input;
|
||||
const timeoutMs = Math.min(timeout || getDefaultTimeoutMs(), getMaxTimeoutMs());
|
||||
const timeoutMs = getEffectiveTimeoutMs(timeout);
|
||||
let fullOutput = '';
|
||||
let lastProgressOutput = '';
|
||||
let lastTotalLines = 0;
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
} from '../../utils/shell/powershellDetection.js'
|
||||
import {
|
||||
getDefaultBashTimeoutMs,
|
||||
getEffectiveBashTimeoutMs,
|
||||
getMaxBashTimeoutMs,
|
||||
} from '../../utils/timeouts.js'
|
||||
import { FILE_EDIT_TOOL_NAME } from '../FileEditTool/constants.js'
|
||||
@@ -23,6 +24,10 @@ export function getMaxTimeoutMs(): number {
|
||||
return getMaxBashTimeoutMs()
|
||||
}
|
||||
|
||||
export function getEffectiveTimeoutMs(timeout: unknown): number {
|
||||
return getEffectiveBashTimeoutMs(timeout)
|
||||
}
|
||||
|
||||
function getBackgroundUsageNote(): string | null {
|
||||
if (isEnvTruthy(process.env.CLAUDE_CODE_DISABLE_BACKGROUND_TASKS)) {
|
||||
return null
|
||||
|
||||
@@ -44,8 +44,9 @@ describe('QueryGuard', () => {
|
||||
expect(guard.isActive).toBe(false)
|
||||
})
|
||||
|
||||
test('timeout auto force-ends after 5 minutes', () => {
|
||||
test('idle timeout auto force-ends after 5 minutes without activity', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard()
|
||||
guard.tryStart()
|
||||
expect(guard.isActive).toBe(true)
|
||||
@@ -59,8 +60,9 @@ describe('QueryGuard', () => {
|
||||
expect(guard.isActive).toBe(false)
|
||||
})
|
||||
|
||||
test('timeout notifies owner with the timed-out generation', () => {
|
||||
test('timeout notifies owner with the timed-out generation and reason', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard()
|
||||
const onTimeout = vi.fn()
|
||||
guard.setTimeoutHandler(onTimeout)
|
||||
@@ -69,12 +71,13 @@ describe('QueryGuard', () => {
|
||||
vi.advanceTimersByTime(5 * 60 * 1000)
|
||||
|
||||
expect(onTimeout).toHaveBeenCalledTimes(1)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen, 'idle')
|
||||
expect(guard.isActive).toBe(false)
|
||||
})
|
||||
|
||||
test('timeout handler cleanup prevents stale notification', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard()
|
||||
const onTimeout = vi.fn()
|
||||
const cleanup = guard.setTimeoutHandler(onTimeout)
|
||||
@@ -103,6 +106,179 @@ describe('QueryGuard', () => {
|
||||
expect(consoleError).toHaveBeenCalledWith('[QueryGuard] Timeout handler failed', handlerError)
|
||||
})
|
||||
|
||||
test('API stream activity extends the idle deadline only while progress continues', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 100,
|
||||
hardMaxQueryMs: 1_000,
|
||||
})
|
||||
const gen = guard.tryStart()!
|
||||
|
||||
vi.advanceTimersByTime(90)
|
||||
guard.registerActivity('api_stream', gen)
|
||||
vi.advanceTimersByTime(99)
|
||||
expect(guard.isActive).toBe(true)
|
||||
|
||||
vi.advanceTimersByTime(1)
|
||||
expect(guard.isActive).toBe(false)
|
||||
})
|
||||
|
||||
test('query aborts when idle timeout is reached with no activity', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 100,
|
||||
hardMaxQueryMs: 1_000,
|
||||
})
|
||||
guard.tryStart()
|
||||
|
||||
vi.advanceTimersByTime(100)
|
||||
|
||||
expect(guard.isActive).toBe(false)
|
||||
})
|
||||
|
||||
test('active bounded lease is not aborted merely because idle timeout elapses', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 500,
|
||||
hardMaxQueryMs: 1_000,
|
||||
toolLeaseGraceMs: 10,
|
||||
})
|
||||
const gen = guard.tryStart()!
|
||||
const lease = guard.acquireLease({
|
||||
owner: 'bash',
|
||||
id: 'toolu_1',
|
||||
timeoutMs: 500,
|
||||
}, gen)
|
||||
|
||||
vi.advanceTimersByTime(500)
|
||||
|
||||
expect(guard.isActive).toBe(true)
|
||||
lease.release()
|
||||
expect(guard.end(gen)).toBe(true)
|
||||
})
|
||||
|
||||
test('lease deadline aborts bounded work that exceeds its own timeout', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 500,
|
||||
hardMaxQueryMs: 1_000,
|
||||
toolLeaseGraceMs: 10,
|
||||
})
|
||||
const onTimeout = vi.fn()
|
||||
guard.setTimeoutHandler(onTimeout)
|
||||
const gen = guard.tryStart()!
|
||||
guard.acquireLease({
|
||||
owner: 'bash',
|
||||
id: 'toolu_1',
|
||||
timeoutMs: 500,
|
||||
}, gen)
|
||||
|
||||
vi.advanceTimersByTime(509)
|
||||
expect(guard.isActive).toBe(true)
|
||||
vi.advanceTimersByTime(1)
|
||||
|
||||
expect(guard.isActive).toBe(false)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen, 'lease_expired')
|
||||
})
|
||||
|
||||
test('hard maximum aborts even with active leases and activity', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 100,
|
||||
hardMaxQueryMs: 1_000,
|
||||
toolLeaseGraceMs: 10,
|
||||
})
|
||||
const onTimeout = vi.fn()
|
||||
guard.setTimeoutHandler(onTimeout)
|
||||
const gen = guard.tryStart()!
|
||||
guard.acquireLease({
|
||||
owner: 'bash',
|
||||
id: 'toolu_1',
|
||||
timeoutMs: 5_000,
|
||||
}, gen)
|
||||
|
||||
for (let elapsed = 0; elapsed < 900; elapsed += 90) {
|
||||
vi.advanceTimersByTime(90)
|
||||
guard.registerActivity('api_stream', gen)
|
||||
expect(guard.isActive).toBe(true)
|
||||
}
|
||||
|
||||
vi.advanceTimersByTime(100)
|
||||
|
||||
expect(guard.isActive).toBe(false)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen, 'hard_max')
|
||||
})
|
||||
|
||||
test('lease hard cap is relative to acquisition and capped by query remaining budget', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 500,
|
||||
hardMaxQueryMs: 1_000,
|
||||
toolLeaseGraceMs: 10,
|
||||
})
|
||||
const onTimeout = vi.fn()
|
||||
guard.setTimeoutHandler(onTimeout)
|
||||
const gen = guard.tryStart()!
|
||||
|
||||
vi.advanceTimersByTime(400)
|
||||
guard.acquireLease({
|
||||
owner: 'tool',
|
||||
id: 'toolu_late',
|
||||
timeoutMs: 500,
|
||||
hardCapMs: 300,
|
||||
}, gen)
|
||||
|
||||
vi.advanceTimersByTime(299)
|
||||
expect(guard.isActive).toBe(true)
|
||||
vi.advanceTimersByTime(1)
|
||||
|
||||
expect(guard.isActive).toBe(false)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen, 'lease_expired')
|
||||
})
|
||||
|
||||
test('stale generations cannot extend or release a newer query', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const guard = new QueryGuard({
|
||||
idleTimeoutMs: 100,
|
||||
hardMaxQueryMs: 1_000,
|
||||
toolLeaseGraceMs: 10,
|
||||
})
|
||||
const onTimeout = vi.fn()
|
||||
guard.setTimeoutHandler(onTimeout)
|
||||
|
||||
const gen1 = guard.tryStart()!
|
||||
const staleLease = guard.acquireLease({
|
||||
owner: 'bash',
|
||||
id: 'toolu_stale',
|
||||
timeoutMs: 500,
|
||||
}, gen1)
|
||||
guard.forceEnd()
|
||||
|
||||
const gen2 = guard.tryStart()!
|
||||
const liveLease = guard.acquireLease({
|
||||
owner: 'bash',
|
||||
id: 'toolu_live',
|
||||
timeoutMs: 500,
|
||||
}, gen2)
|
||||
|
||||
staleLease.release()
|
||||
vi.advanceTimersByTime(100)
|
||||
expect(guard.isActive).toBe(true)
|
||||
|
||||
liveLease.release()
|
||||
guard.registerActivity('stale_api_stream', gen1)
|
||||
vi.advanceTimersByTime(100)
|
||||
expect(guard.isActive).toBe(false)
|
||||
expect(onTimeout).toHaveBeenCalledWith(gen2, 'idle')
|
||||
})
|
||||
|
||||
test('timeout is cleared when end() is called normally', () => {
|
||||
vi.useFakeTimers()
|
||||
const guard = new QueryGuard()
|
||||
|
||||
+264
-17
@@ -18,8 +18,8 @@
|
||||
* re-entry from the queue processor during the async gap.
|
||||
*
|
||||
* Timeout:
|
||||
* If a query runs longer than QUERY_TIMEOUT_MS, the guard automatically
|
||||
* force-ends to prevent infinite spinner loops (see issue #1207).
|
||||
* The guard uses an idle timeout for stuck work, bounded leases for active
|
||||
* local/API work, and a hard maximum query lifetime that always wins.
|
||||
*
|
||||
* Usage with React:
|
||||
* const queryGuard = useRef(new QueryGuard()).current
|
||||
@@ -30,8 +30,75 @@
|
||||
*/
|
||||
import { createSignal } from './signal.js'
|
||||
|
||||
const QUERY_TIMEOUT_MS = 5 * 60 * 1000 // 5 minutes
|
||||
type QueryTimeoutHandler = (generation: number) => void
|
||||
export const DEFAULT_QUERY_IDLE_TIMEOUT_MS = 5 * 60 * 1000 // 5 minutes
|
||||
export const DEFAULT_QUERY_HARD_MAX_MS = 30 * 60 * 1000 // 30 minutes
|
||||
export const DEFAULT_TOOL_LEASE_GRACE_MS = 5_000
|
||||
|
||||
/**
|
||||
* Why QueryGuard force-ended the current query.
|
||||
* - `idle`: no progress and no valid lease existed for the idle timeout.
|
||||
* - `hard_max`: the query reached its absolute maximum lifetime.
|
||||
* - `lease_expired`: bounded active work exceeded its own lease deadline.
|
||||
*/
|
||||
export type QueryGuardTimeoutReason = 'idle' | 'hard_max' | 'lease_expired'
|
||||
|
||||
/**
|
||||
* Input for a bounded unit of active work.
|
||||
*
|
||||
* `owner` identifies the subsystem taking the lease, `id` identifies the
|
||||
* specific operation, `timeoutMs` is measured from lease acquisition, and
|
||||
* `hardCapMs` is an optional lease-local upper bound that is still capped by
|
||||
* the current query's remaining hard-maximum budget.
|
||||
*/
|
||||
export type QueryGuardLeaseInput = {
|
||||
/** Subsystem taking the lease, used for diagnostics and unique lease ids. */
|
||||
owner: 'api' | 'tool' | 'bash' | 'subagent' | string
|
||||
/** Stable operation id, for example a tool-use id. */
|
||||
id: string
|
||||
/** Lease timeout measured from acquisition time. */
|
||||
timeoutMs?: number
|
||||
/** Optional lease-local hard cap, also bounded by the query hard max. */
|
||||
hardCapMs?: number
|
||||
/** Human-readable description for diagnostics. */
|
||||
description?: string
|
||||
}
|
||||
|
||||
/**
|
||||
* Handle returned for an active lease. Call `release()` exactly once when the
|
||||
* bounded work finishes; stale or repeated releases are ignored.
|
||||
*/
|
||||
export type QueryGuardLease = {
|
||||
readonly id: string
|
||||
/** Release this lease if it still belongs to the current generation. */
|
||||
release(): void
|
||||
}
|
||||
|
||||
type QueryTimeoutHandler = (
|
||||
generation: number,
|
||||
reason: QueryGuardTimeoutReason,
|
||||
) => void
|
||||
|
||||
type QueryGuardOptions = {
|
||||
idleTimeoutMs?: number
|
||||
hardMaxQueryMs?: number
|
||||
toolLeaseGraceMs?: number
|
||||
}
|
||||
|
||||
type LeaseRecord = {
|
||||
leaseId: string
|
||||
owner: string
|
||||
id: string
|
||||
generation: number
|
||||
startedAt: number
|
||||
deadlineAt: number
|
||||
description?: string
|
||||
}
|
||||
|
||||
function positiveOrDefault(value: number | undefined, fallback: number): number {
|
||||
return typeof value === 'number' && Number.isFinite(value) && value > 0
|
||||
? value
|
||||
: fallback
|
||||
}
|
||||
|
||||
export class QueryGuard {
|
||||
private _status: 'idle' | 'dispatching' | 'running' = 'idle'
|
||||
@@ -39,6 +106,28 @@ export class QueryGuard {
|
||||
private _changed = createSignal()
|
||||
private _timeoutId: ReturnType<typeof setTimeout> | null = null
|
||||
private _timeoutHandler: QueryTimeoutHandler | null = null
|
||||
private _queryStartedAt = 0
|
||||
private _lastActivityAt = 0
|
||||
private _leaseCounter = 0
|
||||
private _activeLeases = new Map<string, LeaseRecord>()
|
||||
private readonly _idleTimeoutMs: number
|
||||
private readonly _hardMaxQueryMs: number
|
||||
private readonly _toolLeaseGraceMs: number
|
||||
|
||||
constructor(options: QueryGuardOptions = {}) {
|
||||
this._idleTimeoutMs = positiveOrDefault(
|
||||
options.idleTimeoutMs,
|
||||
DEFAULT_QUERY_IDLE_TIMEOUT_MS,
|
||||
)
|
||||
this._hardMaxQueryMs = positiveOrDefault(
|
||||
options.hardMaxQueryMs,
|
||||
DEFAULT_QUERY_HARD_MAX_MS,
|
||||
)
|
||||
this._toolLeaseGraceMs = Math.max(
|
||||
0,
|
||||
positiveOrDefault(options.toolLeaseGraceMs, DEFAULT_TOOL_LEASE_GRACE_MS),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* Reserve the guard for queue processing. Transitions idle → dispatching.
|
||||
@@ -71,6 +160,9 @@ export class QueryGuard {
|
||||
if (this._status === 'running') return null
|
||||
this._status = 'running'
|
||||
++this._generation
|
||||
this._activeLeases.clear()
|
||||
this._queryStartedAt = Date.now()
|
||||
this._lastActivityAt = this._queryStartedAt
|
||||
this._startTimeout()
|
||||
this._notify()
|
||||
return this._generation
|
||||
@@ -85,6 +177,7 @@ export class QueryGuard {
|
||||
if (this._generation !== generation) return false
|
||||
if (this._status !== 'running') return false
|
||||
this._clearTimeout()
|
||||
this._activeLeases.clear()
|
||||
this._status = 'idle'
|
||||
this._notify()
|
||||
return true
|
||||
@@ -99,11 +192,95 @@ export class QueryGuard {
|
||||
forceEnd(): void {
|
||||
if (this._status === 'idle') return
|
||||
this._clearTimeout()
|
||||
this._activeLeases.clear()
|
||||
this._status = 'idle'
|
||||
++this._generation
|
||||
this._notify()
|
||||
}
|
||||
|
||||
/**
|
||||
* Record forward progress for the current query. Stale generation-scoped
|
||||
* events are ignored so a cancelled query cannot extend a newer turn. Call
|
||||
* this when API chunks, tool progress, or other observable work arrives.
|
||||
*/
|
||||
registerActivity(reason: string, generation?: number): void {
|
||||
void reason
|
||||
if (this._status !== 'running') return
|
||||
if (generation !== undefined && generation !== this._generation) return
|
||||
this._lastActivityAt = Date.now()
|
||||
this._scheduleTimeout()
|
||||
}
|
||||
|
||||
/**
|
||||
* Allow bounded active work to outlive the idle timeout without converting
|
||||
* QueryGuard into an inactivity-only watchdog. Pass the generation when the
|
||||
* lease is acquired from async callbacks so stale work cannot protect a newer
|
||||
* query. Without an explicit generation, the current generation is used.
|
||||
*/
|
||||
acquireLease(
|
||||
input: QueryGuardLeaseInput,
|
||||
generation = this._generation,
|
||||
): QueryGuardLease {
|
||||
if (this._status !== 'running' || generation !== this._generation) {
|
||||
return {
|
||||
id: '',
|
||||
release() {},
|
||||
}
|
||||
}
|
||||
|
||||
const now = Date.now()
|
||||
const leaseTimeoutMs =
|
||||
typeof input.timeoutMs === 'number' &&
|
||||
Number.isFinite(input.timeoutMs) &&
|
||||
input.timeoutMs > 0
|
||||
? input.timeoutMs
|
||||
: undefined
|
||||
const leaseHardCapMs =
|
||||
typeof input.hardCapMs === 'number' &&
|
||||
Number.isFinite(input.hardCapMs) &&
|
||||
input.hardCapMs > 0
|
||||
? input.hardCapMs
|
||||
: this._hardMaxQueryMs
|
||||
const queryHardDeadlineAt = this._queryStartedAt + this._hardMaxQueryMs
|
||||
const queryRemainingMs = Math.max(0, queryHardDeadlineAt - now)
|
||||
const effectiveHardCapMs = Math.min(leaseHardCapMs, queryRemainingMs)
|
||||
const leaseDeadlineAt =
|
||||
leaseTimeoutMs === undefined
|
||||
? now + effectiveHardCapMs
|
||||
: Math.min(
|
||||
now + leaseTimeoutMs + this._toolLeaseGraceMs,
|
||||
now + effectiveHardCapMs,
|
||||
)
|
||||
const leaseId = `${generation}:${input.owner}:${input.id}:${++this._leaseCounter}`
|
||||
this._activeLeases.set(leaseId, {
|
||||
leaseId,
|
||||
owner: input.owner,
|
||||
id: input.id,
|
||||
generation,
|
||||
startedAt: now,
|
||||
deadlineAt: leaseDeadlineAt,
|
||||
description: input.description,
|
||||
})
|
||||
this._lastActivityAt = now
|
||||
this._scheduleTimeout()
|
||||
|
||||
return {
|
||||
id: leaseId,
|
||||
release: () => this.releaseLease(leaseId, generation),
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Release a lease by id. Stale generation releases and repeated releases are
|
||||
* ignored, so old async cleanup cannot affect a newer query.
|
||||
*/
|
||||
releaseLease(leaseId: string, generation = this._generation): void {
|
||||
const lease = this._activeLeases.get(leaseId)
|
||||
if (!lease || lease.generation !== generation) return
|
||||
this._activeLeases.delete(leaseId)
|
||||
this._scheduleTimeout()
|
||||
}
|
||||
|
||||
/**
|
||||
* Is the guard active (dispatching or running)?
|
||||
* Always synchronous — not subject to React state batching delays.
|
||||
@@ -146,23 +323,93 @@ export class QueryGuard {
|
||||
}
|
||||
|
||||
/**
|
||||
* Start a watchdog timer. If the query doesn't complete within
|
||||
* QUERY_TIMEOUT_MS, automatically force-end to prevent infinite loops.
|
||||
* Start a watchdog timer. Stuck work aborts after the idle timeout, active
|
||||
* bounded work can continue while its lease is valid, and the hard maximum
|
||||
* aborts the query regardless of activity.
|
||||
*/
|
||||
private _startTimeout(): void {
|
||||
this._scheduleTimeout()
|
||||
}
|
||||
|
||||
private _scheduleTimeout(): void {
|
||||
this._clearTimeout()
|
||||
this._timeoutId = setTimeout(() => {
|
||||
if (this._status === 'running') {
|
||||
console.error(`[QueryGuard] Query timeout after ${QUERY_TIMEOUT_MS}ms — force-ending to prevent infinite spinner`)
|
||||
try {
|
||||
this._timeoutHandler?.(this._generation)
|
||||
} catch (error) {
|
||||
console.error('[QueryGuard] Timeout handler failed', error)
|
||||
} finally {
|
||||
this.forceEnd()
|
||||
}
|
||||
if (this._status !== 'running') return
|
||||
|
||||
const now = Date.now()
|
||||
const reason = this._getTimeoutReason(now)
|
||||
if (reason) {
|
||||
this._timeoutId = setTimeout(() => this._handleTimeout(), 0)
|
||||
return
|
||||
}
|
||||
|
||||
const nextDeadlineAt = this._getNextDeadlineAt(now)
|
||||
if (nextDeadlineAt === null) return
|
||||
this._timeoutId = setTimeout(
|
||||
() => this._handleTimeout(),
|
||||
Math.max(0, nextDeadlineAt - now),
|
||||
)
|
||||
}
|
||||
|
||||
private _handleTimeout(): void {
|
||||
this._timeoutId = null
|
||||
if (this._status !== 'running') return
|
||||
|
||||
const reason = this._getTimeoutReason(Date.now())
|
||||
if (!reason) {
|
||||
this._scheduleTimeout()
|
||||
return
|
||||
}
|
||||
|
||||
console.error(
|
||||
`[QueryGuard] Query ${reason} timeout — force-ending to prevent infinite spinner`,
|
||||
)
|
||||
try {
|
||||
this._timeoutHandler?.(this._generation, reason)
|
||||
} catch (error) {
|
||||
console.error('[QueryGuard] Timeout handler failed', error)
|
||||
} finally {
|
||||
this.forceEnd()
|
||||
}
|
||||
}
|
||||
|
||||
private _getTimeoutReason(now: number): QueryGuardTimeoutReason | null {
|
||||
if (now >= this._queryStartedAt + this._hardMaxQueryMs) {
|
||||
return 'hard_max'
|
||||
}
|
||||
|
||||
let hasValidLease = false
|
||||
for (const lease of this._activeLeases.values()) {
|
||||
if (lease.deadlineAt <= now) {
|
||||
return 'lease_expired'
|
||||
}
|
||||
}, QUERY_TIMEOUT_MS)
|
||||
hasValidLease = true
|
||||
}
|
||||
|
||||
if (
|
||||
!hasValidLease &&
|
||||
now >= this._lastActivityAt + this._idleTimeoutMs
|
||||
) {
|
||||
return 'idle'
|
||||
}
|
||||
|
||||
return null
|
||||
}
|
||||
|
||||
private _getNextDeadlineAt(now: number): number | null {
|
||||
if (this._status !== 'running') return null
|
||||
|
||||
const deadlines = [this._queryStartedAt + this._hardMaxQueryMs]
|
||||
const leaseDeadlines = [...this._activeLeases.values()]
|
||||
.map(lease => lease.deadlineAt)
|
||||
.filter(deadline => deadline > now)
|
||||
|
||||
if (leaseDeadlines.length > 0) {
|
||||
deadlines.push(Math.min(...leaseDeadlines))
|
||||
} else {
|
||||
deadlines.push(this._lastActivityAt + this._idleTimeoutMs)
|
||||
}
|
||||
|
||||
return Math.min(...deadlines)
|
||||
}
|
||||
|
||||
private _clearTimeout(): void {
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
import { describe, expect, test } from 'bun:test'
|
||||
import { DEFAULT_QUERY_HARD_MAX_MS } from './QueryGuard.js'
|
||||
import {
|
||||
getDefaultBashTimeoutMs,
|
||||
getEffectiveBashTimeoutMs,
|
||||
getMaxBashTimeoutMs,
|
||||
} from './timeouts.js'
|
||||
|
||||
describe('bash timeout helpers', () => {
|
||||
test('effective timeout clamps explicit values to the configured max', () => {
|
||||
const env = {
|
||||
BASH_DEFAULT_TIMEOUT_MS: '120000',
|
||||
BASH_MAX_TIMEOUT_MS: '300000',
|
||||
}
|
||||
|
||||
expect(getEffectiveBashTimeoutMs(900_000, env)).toBe(300_000)
|
||||
})
|
||||
|
||||
test('configured defaults and max values cannot exceed the query hard cap', () => {
|
||||
const env = {
|
||||
BASH_DEFAULT_TIMEOUT_MS: String(DEFAULT_QUERY_HARD_MAX_MS * 2),
|
||||
BASH_MAX_TIMEOUT_MS: String(DEFAULT_QUERY_HARD_MAX_MS * 3),
|
||||
}
|
||||
|
||||
expect(getDefaultBashTimeoutMs(env)).toBe(DEFAULT_QUERY_HARD_MAX_MS)
|
||||
expect(getMaxBashTimeoutMs(env)).toBe(DEFAULT_QUERY_HARD_MAX_MS)
|
||||
expect(getEffectiveBashTimeoutMs(DEFAULT_QUERY_HARD_MAX_MS * 4, env)).toBe(
|
||||
DEFAULT_QUERY_HARD_MAX_MS,
|
||||
)
|
||||
expect(getEffectiveBashTimeoutMs(undefined, env)).toBe(
|
||||
DEFAULT_QUERY_HARD_MAX_MS,
|
||||
)
|
||||
})
|
||||
|
||||
test('effective timeout uses the configured default for invalid explicit values', () => {
|
||||
const env = {
|
||||
BASH_DEFAULT_TIMEOUT_MS: '150000',
|
||||
BASH_MAX_TIMEOUT_MS: '600000',
|
||||
}
|
||||
|
||||
expect(getEffectiveBashTimeoutMs(300_000, env)).toBe(300_000)
|
||||
expect(getEffectiveBashTimeoutMs(0, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(-100, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(Number.NaN, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(Number.POSITIVE_INFINITY, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(Number.NEGATIVE_INFINITY, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(null, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs(undefined, env)).toBe(150_000)
|
||||
expect(getEffectiveBashTimeoutMs('60000', env)).toBe(150_000)
|
||||
})
|
||||
})
|
||||
+24
-4
@@ -1,9 +1,15 @@
|
||||
import { DEFAULT_QUERY_HARD_MAX_MS } from './QueryGuard.js'
|
||||
|
||||
// Constants for timeout values
|
||||
const DEFAULT_TIMEOUT_MS = 120_000 // 2 minutes
|
||||
const MAX_TIMEOUT_MS = 600_000 // 10 minutes
|
||||
|
||||
type EnvLike = Record<string, string | undefined>
|
||||
|
||||
function capToQueryHardMax(timeoutMs: number): number {
|
||||
return Math.min(timeoutMs, DEFAULT_QUERY_HARD_MAX_MS)
|
||||
}
|
||||
|
||||
/**
|
||||
* Get the default timeout for bash operations in milliseconds
|
||||
* Checks BASH_DEFAULT_TIMEOUT_MS environment variable or returns 2 minutes default
|
||||
@@ -14,10 +20,10 @@ export function getDefaultBashTimeoutMs(env: EnvLike = process.env): number {
|
||||
if (envValue) {
|
||||
const parsed = parseInt(envValue, 10)
|
||||
if (!isNaN(parsed) && parsed > 0) {
|
||||
return parsed
|
||||
return capToQueryHardMax(parsed)
|
||||
}
|
||||
}
|
||||
return DEFAULT_TIMEOUT_MS
|
||||
return capToQueryHardMax(DEFAULT_TIMEOUT_MS)
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -31,9 +37,23 @@ export function getMaxBashTimeoutMs(env: EnvLike = process.env): number {
|
||||
const parsed = parseInt(envValue, 10)
|
||||
if (!isNaN(parsed) && parsed > 0) {
|
||||
// Ensure max is at least as large as default
|
||||
return Math.max(parsed, getDefaultBashTimeoutMs(env))
|
||||
return capToQueryHardMax(Math.max(parsed, getDefaultBashTimeoutMs(env)))
|
||||
}
|
||||
}
|
||||
// Always ensure max is at least as large as default
|
||||
return Math.max(MAX_TIMEOUT_MS, getDefaultBashTimeoutMs(env))
|
||||
return capToQueryHardMax(Math.max(MAX_TIMEOUT_MS, getDefaultBashTimeoutMs(env)))
|
||||
}
|
||||
|
||||
export function getEffectiveBashTimeoutMs(
|
||||
requestedTimeout: unknown,
|
||||
env: EnvLike = process.env,
|
||||
): number {
|
||||
const timeoutMs =
|
||||
typeof requestedTimeout === 'number' &&
|
||||
Number.isFinite(requestedTimeout) &&
|
||||
requestedTimeout > 0
|
||||
? requestedTimeout
|
||||
: getDefaultBashTimeoutMs(env)
|
||||
|
||||
return Math.min(timeoutMs, getMaxBashTimeoutMs(env))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user