mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
/new (alias of /clear) silently did nothing when typed right after cancelling a request, for two stacked reasons: - hasBlockingBackgroundWork() gated on the registry's hasUnfinalizedTasks(), which counts cancelled-but-not-yet-finalized entries. That clause exists for the headless holdback loop (every task_started must pair with a task_notification), but /clear and session resume abort-and-reset the registry right after the gate — suppressing that very notification — so blocking on it only made the switch fail in the window between cancel and finalizeCancelled(). The gate now keys off a new BackgroundTaskRegistry.hasRunningTasks(), which counts only entries still actually executing; the headless holdback keeps using hasUnfinalizedTasks() unchanged. - When genuinely blocked, interactive mode showed only a transient debug line while non-interactive returned a proper error. Interactive now returns the same visible error message, so a blocked /clear no longer looks like a no-op. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
082b3bb3d9
commit
5f41b166e6
7 changed files with 153 additions and 29 deletions
|
|
@ -65,7 +65,7 @@ describe('clearCommand', () => {
|
|||
resetChat: mockResetChat,
|
||||
}) as unknown as GeminiClient,
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: mockResetBackgroundTasks,
|
||||
abortAll: mockAbortBackgroundTasks,
|
||||
}),
|
||||
|
|
@ -324,7 +324,7 @@ describe('clearCommand', () => {
|
|||
config: {
|
||||
getHookSystem: mockGetHookSystem,
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: mockResetBackgroundTasks,
|
||||
abortAll: mockAbortBackgroundTasks,
|
||||
}),
|
||||
|
|
@ -404,7 +404,65 @@ describe('clearCommand', () => {
|
|||
services: {
|
||||
config: {
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(true),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(true),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: vi.fn().mockReturnValue({
|
||||
getAll: vi.fn().mockReturnValue([]),
|
||||
hasRunningEntries: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getMonitorRegistry: vi.fn().mockReturnValue({
|
||||
getRunning: vi.fn().mockReturnValue([]),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getWorkflowRunRegistry: vi.fn().mockReturnValue({
|
||||
hasRunningEntries: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
abortAll: vi.fn(),
|
||||
}),
|
||||
getHookSystem: mockGetHookSystem,
|
||||
startNewSession: mockStartNewSession,
|
||||
getGeminiClient: vi.fn().mockReturnValue({
|
||||
resetChat: mockResetChat,
|
||||
} as unknown as GeminiClient),
|
||||
getModel: vi.fn().mockReturnValue('test-model'),
|
||||
getApprovalMode: vi.fn().mockReturnValue('default'),
|
||||
getToolRegistry: vi.fn().mockReturnValue({
|
||||
getAllTools: vi.fn().mockReturnValue([]),
|
||||
}),
|
||||
getDebugLogger: vi.fn().mockReturnValue({ warn: vi.fn() }),
|
||||
},
|
||||
},
|
||||
session: {
|
||||
startNewSession: vi.fn(),
|
||||
},
|
||||
});
|
||||
|
||||
const result = await clearCommand.action(blockedContext, '');
|
||||
|
||||
expect(result).toEqual({
|
||||
type: 'message',
|
||||
messageType: 'error',
|
||||
content:
|
||||
"Stop the current session's running background tasks before starting a new session.",
|
||||
});
|
||||
expect(mockStartNewSession).not.toHaveBeenCalled();
|
||||
expect(mockResetChat).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('returns a visible error when blocked in interactive mode (#5949)', async () => {
|
||||
// Interactive mode used to bail with only a transient debug line,
|
||||
// so a blocked /new looked like the command silently did nothing.
|
||||
if (!clearCommand.action)
|
||||
throw new Error('clearCommand must have an action.');
|
||||
|
||||
const blockedContext = createMockCommandContext({
|
||||
executionMode: 'interactive',
|
||||
services: {
|
||||
config: {
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasRunningTasks: vi.fn().mockReturnValue(true),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: vi.fn().mockReturnValue({
|
||||
|
|
@ -460,7 +518,7 @@ describe('clearCommand', () => {
|
|||
services: {
|
||||
config: {
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: vi.fn().mockReturnValue({
|
||||
|
|
@ -516,7 +574,7 @@ describe('clearCommand', () => {
|
|||
services: {
|
||||
config: {
|
||||
getBackgroundTaskRegistry: vi.fn().mockReturnValue({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: vi.fn().mockReturnValue({
|
||||
|
|
|
|||
|
|
@ -47,14 +47,14 @@ export const clearCommand: SlashCommand = {
|
|||
const content =
|
||||
"Stop the current session's running background tasks before starting a new session.";
|
||||
context.ui.setDebugMessage(content);
|
||||
if (context.executionMode !== 'interactive') {
|
||||
return {
|
||||
type: 'message' as const,
|
||||
messageType: 'error' as const,
|
||||
content,
|
||||
};
|
||||
}
|
||||
return;
|
||||
// Return the error in every mode. Interactive mode used to bail
|
||||
// with only the transient debug line above, so a blocked /clear
|
||||
// looked like the command silently did nothing (issue #5949).
|
||||
return {
|
||||
type: 'message' as const,
|
||||
messageType: 'error' as const,
|
||||
content,
|
||||
};
|
||||
}
|
||||
|
||||
// Fire SessionEnd event (non-blocking to avoid UI lag)
|
||||
|
|
|
|||
|
|
@ -235,7 +235,7 @@ describe('useResumeCommand', () => {
|
|||
getGeminiClient: () => geminiClient,
|
||||
startNewSession: vi.fn(),
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
@ -331,7 +331,7 @@ describe('useResumeCommand', () => {
|
|||
getGeminiClient: () => geminiClient,
|
||||
startNewSession: vi.fn(),
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
@ -424,7 +424,7 @@ describe('useResumeCommand', () => {
|
|||
getGeminiClient: () => geminiClient,
|
||||
startNewSession: vi.fn(),
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
@ -489,7 +489,7 @@ describe('useResumeCommand', () => {
|
|||
|
||||
const config = {
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(true),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(true),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
@ -554,7 +554,7 @@ describe('useResumeCommand', () => {
|
|||
|
||||
const config = {
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
@ -629,7 +629,7 @@ describe('useResumeCommand', () => {
|
|||
getGeminiClient: () => geminiClient,
|
||||
startNewSession: vi.fn(),
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: vi.fn().mockReturnValue(false),
|
||||
hasRunningTasks: vi.fn().mockReturnValue(false),
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getBackgroundShellRegistry: () => ({
|
||||
|
|
|
|||
|
|
@ -12,14 +12,14 @@ import {
|
|||
} from './backgroundWorkUtils.js';
|
||||
|
||||
function createMockConfig(overrides?: {
|
||||
hasUnfinalizedTasks?: boolean;
|
||||
hasRunningTasks?: boolean;
|
||||
runningMonitors?: unknown[];
|
||||
hasRunningEntries?: boolean;
|
||||
hasRunningWorkflows?: boolean;
|
||||
}): Config {
|
||||
return {
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: () => overrides?.hasUnfinalizedTasks ?? false,
|
||||
hasRunningTasks: () => overrides?.hasRunningTasks ?? false,
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getMonitorRegistry: () => ({
|
||||
|
|
@ -42,11 +42,13 @@ describe('hasBlockingBackgroundWork', () => {
|
|||
expect(hasBlockingBackgroundWork(createMockConfig())).toBe(false);
|
||||
});
|
||||
|
||||
it('returns true when background tasks are unfinalized', () => {
|
||||
// #5949: the gate keys off hasRunningTasks(), NOT hasUnfinalizedTasks()
|
||||
// — a cancelled task whose finalize callback hasn't fired yet must not
|
||||
// block /clear or session resume (both abort-and-reset right after the
|
||||
// gate, suppressing the pending notification anyway).
|
||||
it('returns true when background tasks are still running', () => {
|
||||
expect(
|
||||
hasBlockingBackgroundWork(
|
||||
createMockConfig({ hasUnfinalizedTasks: true }),
|
||||
),
|
||||
hasBlockingBackgroundWork(createMockConfig({ hasRunningTasks: true })),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
|
|
@ -75,10 +77,10 @@ describe('hasBlockingBackgroundWork', () => {
|
|||
).toBe(true);
|
||||
});
|
||||
|
||||
it('short-circuits: does not check monitors or shells when tasks are unfinalized', () => {
|
||||
it('short-circuits: does not check monitors or shells when tasks are running', () => {
|
||||
const config = {
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: () => true,
|
||||
hasRunningTasks: () => true,
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getMonitorRegistry: () => {
|
||||
|
|
@ -95,7 +97,7 @@ describe('hasBlockingBackgroundWork', () => {
|
|||
it('short-circuits: does not check shells when monitors are running', () => {
|
||||
const config = {
|
||||
getBackgroundTaskRegistry: () => ({
|
||||
hasUnfinalizedTasks: () => false,
|
||||
hasRunningTasks: () => false,
|
||||
reset: vi.fn(),
|
||||
}),
|
||||
getMonitorRegistry: () => ({
|
||||
|
|
|
|||
|
|
@ -8,7 +8,13 @@ import type { Config } from '@qwen-code/qwen-code-core';
|
|||
|
||||
export function hasBlockingBackgroundWork(config: Config): boolean {
|
||||
return (
|
||||
config.getBackgroundTaskRegistry().hasUnfinalizedTasks() ||
|
||||
// hasRunningTasks, not hasUnfinalizedTasks: a cancelled task whose
|
||||
// finalize callback hasn't fired yet must not block /clear or
|
||||
// session resume — both abort-and-reset the registry right after
|
||||
// this gate, suppressing the pending notification anyway. Gating on
|
||||
// the unfinalized set made /new silently no-op when typed in the
|
||||
// window between cancel and finalize (issue #5949).
|
||||
config.getBackgroundTaskRegistry().hasRunningTasks() ||
|
||||
config.getMonitorRegistry().getRunning().length > 0 ||
|
||||
config.getBackgroundShellRegistry().hasRunningEntries() ||
|
||||
// R7 (wenshao): the WorkflowRunRegistry is a 4th sibling that the
|
||||
|
|
|
|||
|
|
@ -873,6 +873,44 @@ describe('BackgroundTaskRegistry', () => {
|
|||
expect(registry.hasUnfinalizedTasks()).toBe(false);
|
||||
});
|
||||
|
||||
it('hasRunningTasks ignores cancelled-but-not-notified entries (#5949)', () => {
|
||||
// Session-switch gates (/clear, /resume) key off hasRunningTasks so a
|
||||
// task the user just cancelled — aborted, only its terminal
|
||||
// notification outstanding — cannot make the switch silently no-op.
|
||||
// hasUnfinalizedTasks must still report it for the headless holdback.
|
||||
registry.register({
|
||||
agentId: 'test-1',
|
||||
description: 'test agent',
|
||||
status: 'running',
|
||||
startTime: Date.now(),
|
||||
abortController: new AbortController(),
|
||||
isBackgrounded: true,
|
||||
outputFile: '/tmp/test.jsonl',
|
||||
});
|
||||
expect(registry.hasRunningTasks()).toBe(true);
|
||||
|
||||
registry.cancel('test-1');
|
||||
expect(registry.get('test-1')!.status).toBe('cancelled');
|
||||
expect(registry.hasRunningTasks()).toBe(false);
|
||||
expect(registry.hasUnfinalizedTasks()).toBe(true);
|
||||
|
||||
registry.finalizeCancelled('test-1', '');
|
||||
expect(registry.hasRunningTasks()).toBe(false);
|
||||
});
|
||||
|
||||
it('hasRunningTasks ignores foreground entries', () => {
|
||||
registry.register({
|
||||
agentId: 'fg-1',
|
||||
description: 'foreground agent',
|
||||
status: 'running',
|
||||
startTime: Date.now(),
|
||||
abortController: new AbortController(),
|
||||
isBackgrounded: false,
|
||||
outputFile: '/tmp/test.jsonl',
|
||||
});
|
||||
expect(registry.hasRunningTasks()).toBe(false);
|
||||
});
|
||||
|
||||
it('hasUnfinalizedTasks clears once every entry has been notified', () => {
|
||||
registry.register({
|
||||
agentId: 'a',
|
||||
|
|
|
|||
|
|
@ -1008,6 +1008,26 @@ export class BackgroundTaskRegistry {
|
|||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* True while any background entry is still actually executing. Unlike
|
||||
* `hasUnfinalizedTasks()`, a `cancelled`-but-not-yet-finalized entry
|
||||
* does NOT count: its work has already been aborted and only the
|
||||
* terminal task-notification is outstanding. Session-switch gates
|
||||
* (/clear, /resume) key off this instead — they abort-and-reset the
|
||||
* registry right after passing the gate, which suppresses that very
|
||||
* notification, so blocking on it made the command silently no-op
|
||||
* when the user cleared immediately after cancelling (issue #5949).
|
||||
* Headless holdback loops must keep using `hasUnfinalizedTasks()` so
|
||||
* every task_started still pairs with a task_notification.
|
||||
*/
|
||||
hasRunningTasks(): boolean {
|
||||
for (const entry of this.agents.values()) {
|
||||
if (!entry.isBackgrounded) continue;
|
||||
if (entry.status === 'running') return true;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Drops every in-memory entry without touching sidecar state.
|
||||
*
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue