-
Notifications
You must be signed in to change notification settings - Fork 250
fix(task): skip saveClineMessages when history task aborts before messages load #1181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0c680cf
fix(task): skip saveClineMessages when history task aborts before mes…
edelauna c66ca51
test: strengthen resume-eviction-race assertions and type mock provider
edelauna c293703
test: add fallback mock for second getSavedClineMessages read in resu…
edelauna d6b6014
Merge branch 'main' into fix/resume-eviction-title-clobber
navedmerchant a80e0c5
fix(task): prevent saving unhydrated history messages during abort
edelauna File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| { | ||
| "fixtures": [ | ||
| { | ||
| "match": { | ||
| "userMessage": "RESUME_EVICTION_RACE_SMOKE" | ||
| }, | ||
| "response": { | ||
| "toolCalls": [ | ||
| { | ||
| "name": "attempt_completion", | ||
| "arguments": "{\"result\":\"Resume eviction smoke completed.\"}", | ||
| "id": "call_resume_eviction_001" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import * as assert from "assert" | ||
|
|
||
| import { setDefaultSuiteTimeout } from "./test-utils" | ||
| import { waitUntilCompleted, waitFor } from "./utils" | ||
|
|
||
| // Regression test for the "Work #1 (no message)" title-clobber bug reported | ||
| // against Zoo Code v3.76.0 (Discord, 2026-08-06). | ||
| // | ||
| // Root cause: Task#resumeTaskFromHistory() is started fire-and-forget by | ||
| // scheduleTask() after createTaskWithHistoryItem() adds the task to the | ||
| // registry, so `clineMessages` is [] until the first disk read resolves. | ||
| // ClineProvider#evictCurrentTask() (called by clearCurrentTask / the | ||
| // Back-to-parent / Go-to-subtask buttons) calls abortTask(), which calls | ||
| // saveClineMessages() → taskMetadata() while the array is still empty. | ||
| // taskMetadata() then persists the "no_messages" placeholder title, | ||
| // permanently clobbering the real title in the history store. | ||
| // | ||
| // The test exercises the race by: | ||
| // 1. Running a task to completion so a real title is persisted. | ||
| // 2. Starting resumeTask() (same path as showTaskWithId) without awaiting it. | ||
| // 3. Polling until the task appears on the stack, then immediately evicting — | ||
| // the task is on the stack but its message load is still in flight. | ||
| // 4. Asserting the stored title still matches the original. | ||
| // | ||
| // NOTE: Because the extension host reads task messages from disk in the same | ||
| // process as this test, the I/O window is very tight (< 1ms on local disk). | ||
| // The race is not reliably triggerable from the e2e layer; the canonical | ||
| // regression anchor is the unit test in | ||
| // src/core/task/__tests__/Task.resume-eviction-race.spec.ts, which controls | ||
| // the timing via a deferred promise. This e2e test serves as a smoke test that | ||
| // the resume-then-evict flow does not blow up and that the stored title is | ||
| // correct after a round-trip. | ||
| suite("Resume eviction race (title clobber regression)", function () { | ||
| setDefaultSuiteTimeout(this) | ||
|
|
||
| test("evicting a mid-resume task does not overwrite its stored title", async () => { | ||
| const api = globalThis.api | ||
|
|
||
| const ORIGINAL_TITLE = | ||
| "RESUME_EVICTION_RACE_SMOKE: complete immediately with 'Resume eviction smoke completed.'" | ||
|
|
||
| // Step 1 — run a task to completion so a real title is persisted. | ||
| const taskId = await waitUntilCompleted({ | ||
| api, | ||
| start: () => | ||
| api.startNewTask({ | ||
| configuration: { | ||
| mode: "ask", | ||
| autoApprovalEnabled: true, | ||
| enableCheckpoints: false, | ||
| }, | ||
| text: ORIGINAL_TITLE, | ||
| }), | ||
| }) | ||
|
|
||
| const beforeResume = await api.getTaskHistoryItem(taskId) | ||
| assert.ok(beforeResume, "Task should be in history after completion") | ||
| assert.ok( | ||
| beforeResume.task?.includes("RESUME_EVICTION_RACE_SMOKE"), | ||
| `Persisted title before resume should contain the prompt marker (got "${beforeResume.task}")`, | ||
| ) | ||
|
|
||
| // Drain the stack so we start clean. | ||
| while (api.getCurrentTaskStack().length > 0) { | ||
| await api.clearCurrentTask() | ||
| } | ||
|
|
||
| // Step 2 — fire resumeTask() without awaiting it. resumeTask() calls | ||
| // createTaskWithHistoryItem() which adds the task to the registry and | ||
| // calls scheduleTask() (fire-and-forget). The task's run() and | ||
| // resumeTaskFromHistory() start in the background. | ||
| const resumePromise = api.resumeTask(taskId) | ||
|
|
||
| // Step 3 — wait only until the task appears on the stack (i.e. | ||
| // createTaskWithHistoryItem has returned and addClineToStack has run), | ||
| // then immediately evict. This minimises the gap between the eviction | ||
| // and the in-flight message load, giving the best chance of hitting the | ||
| // race window before readTaskMessages() resolves. | ||
| await waitFor(() => api.getCurrentTaskStack().includes(taskId)) | ||
| await api.clearCurrentTask() | ||
|
|
||
| // Let the resume settle. | ||
| await resumePromise.catch(() => {}) | ||
|
|
||
| // Step 4 — the stored title must still be the real one. | ||
| const afterEviction = await api.getTaskHistoryItem(taskId) | ||
| assert.ok(afterEviction, "Task should still be in history after eviction") | ||
|
|
||
| // Before the fix this would be "Task #N (No messages)" / "工作 #N (無訊息)". | ||
| assert.strictEqual( | ||
| afterEviction.task, | ||
| beforeResume.task, | ||
| `Title must not change during resume eviction. Got: "${afterEviction.task}"`, | ||
| ) | ||
| }) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
223 changes: 223 additions & 0 deletions
223
src/core/task/__tests__/Task.resume-eviction-race.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,223 @@ | ||
| // cd src && npx vitest run core/task/__tests__/Task.resume-eviction-race.spec.ts | ||
| // | ||
| // Regression anchor for the "Work #1 (no message)" title-clobber bug | ||
| // (Zoo Code v3.76.0, Discord report 2026-08-06). | ||
| // | ||
| // Root cause: resumeTaskFromHistory() starts with an async disk read. Until | ||
| // that read resolves, clineMessages is []. evictCurrentTask() calls | ||
| // abortTask(), which called saveClineMessages() -> taskMetadata(). With an | ||
| // empty array, taskMetadata() writes the "no_messages" placeholder as the | ||
| // title, permanently clobbering the real one in the history store. | ||
| // | ||
| // Fix: abortTask() skips saveClineMessages() for history tasks whose message | ||
| // load has not completed. The on-disk data is already correct at that point. | ||
| import * as os from "os" | ||
| import * as path from "path" | ||
|
|
||
| import type { ClineMessage, GlobalState, HistoryItem, ProviderSettings } from "@roo-code/types" | ||
| import { TelemetryService } from "@roo-code/telemetry" | ||
|
|
||
| import { Task } from "../Task" | ||
| import { ClineProvider } from "../../webview/ClineProvider" | ||
|
|
||
| // ─── Hoisted mocks ─────────────────────────────────────────────────────────── | ||
|
|
||
| const { mockSaveApiMessages, mockSaveTaskMessages, mockReadApiMessages, mockReadTaskMessages, mockPWaitFor } = | ||
| vi.hoisted(() => ({ | ||
| mockSaveApiMessages: vi.fn().mockResolvedValue(undefined), | ||
| mockSaveTaskMessages: vi.fn().mockResolvedValue(undefined), | ||
| mockReadApiMessages: vi.fn().mockResolvedValue([]), | ||
| // Controlled per-test via a deferred promise so we can hold the "disk | ||
| // read" open while a rival navigation aborts the still-loading task. | ||
| mockReadTaskMessages: vi.fn<() => Promise<ClineMessage[]>>(), | ||
| mockPWaitFor: vi.fn().mockResolvedValue(undefined), | ||
| })) | ||
|
|
||
| // ─── Module mocks ──────────────────────────────────────────────────────────── | ||
| // vscode and fs/promises are globally aliased in vitest.config — no inline | ||
| // mock needed. | ||
|
|
||
| vi.mock("delay", () => ({ __esModule: true, default: vi.fn().mockResolvedValue(undefined) })) | ||
| vi.mock("execa", () => ({ execa: vi.fn() })) | ||
| vi.mock("p-wait-for", () => ({ default: mockPWaitFor })) | ||
|
|
||
| // taskMetadata is NOT mocked — the real implementation is under test. | ||
| vi.mock("../../task-persistence", async (importOriginal) => { | ||
| const mod = await importOriginal<typeof import("../../task-persistence")>() | ||
| return { | ||
| ...mod, | ||
| saveApiMessages: mockSaveApiMessages, | ||
| saveTaskMessages: mockSaveTaskMessages, | ||
| readApiMessages: mockReadApiMessages, | ||
| readTaskMessages: mockReadTaskMessages, | ||
| TaskHistoryStore: vi.fn().mockImplementation(function () { | ||
| return { | ||
| initialize: vi.fn().mockResolvedValue(undefined), | ||
| dispose: vi.fn(), | ||
| get: vi.fn(), | ||
| getAll: vi.fn().mockReturnValue([]), | ||
| upsert: vi.fn().mockResolvedValue([]), | ||
| delete: vi.fn().mockResolvedValue(undefined), | ||
| deleteMany: vi.fn().mockResolvedValue(undefined), | ||
| reconcile: vi.fn().mockResolvedValue(undefined), | ||
| initialized: Promise.resolve(), | ||
| } | ||
| }), | ||
| } | ||
| }) | ||
|
|
||
| vi.mock("../../mentions", () => ({ | ||
| parseMentions: vi | ||
| .fn() | ||
| .mockImplementation((text) => | ||
| Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] }), | ||
| ), | ||
| openMention: vi.fn(), | ||
| getLatestTerminalOutput: vi.fn(), | ||
| })) | ||
| vi.mock("../../../integrations/misc/extract-text", () => ({ | ||
| extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"), | ||
| })) | ||
| vi.mock("../../environment/getEnvironmentDetails", () => ({ | ||
| getEnvironmentDetails: vi.fn().mockResolvedValue(""), | ||
| })) | ||
| vi.mock("../../ignore/RooIgnoreController") | ||
| vi.mock("../../../utils/storage", () => ({ | ||
| getTaskDirectoryPath: vi | ||
| .fn() | ||
| .mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)), | ||
| getSettingsDirectoryPath: vi | ||
| .fn() | ||
| .mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)), | ||
| })) | ||
| vi.mock("../../../utils/fs", () => ({ fileExistsAtPath: vi.fn().mockReturnValue(false) })) | ||
|
|
||
| // ─── Helpers ───────────────────────────────────────────────────────────────── | ||
|
|
||
| function createDeferred<T>() { | ||
| let resolve!: (value: T) => void | ||
| const promise = new Promise<T>((res) => { | ||
| resolve = res | ||
| }) | ||
| return { promise, resolve } | ||
| } | ||
|
|
||
| /** | ||
| * Minimal slice of ClineProvider that Task reads during construction and abort. | ||
| * All types are derived from ClineProvider so TypeScript validates property | ||
| * names and signatures without requiring the full class to be satisfied. | ||
| */ | ||
| type MockProvider = Pick<ClineProvider, "log" | "updateTaskHistory"> & { | ||
| taskHistoryStore: Pick<ClineProvider["taskHistoryStore"], "get"> | ||
| context: { | ||
| globalStorageUri: Pick<ClineProvider["context"]["globalStorageUri"], "fsPath"> | ||
| globalState: Pick<ClineProvider["context"]["globalState"], "get" | "update" | "keys"> | ||
| workspaceState: Pick<ClineProvider["context"]["workspaceState"], "get" | "update" | "keys"> | ||
| secrets: Pick<ClineProvider["context"]["secrets"], "get" | "store" | "delete"> | ||
| extensionUri: Pick<ClineProvider["context"]["extensionUri"], "fsPath"> | ||
| extension: Pick<ClineProvider["context"]["extension"], "packageJSON"> | ||
| } | ||
| } | ||
|
|
||
| function makeMockProvider(updateTaskHistory: ReturnType<typeof vi.fn>): MockProvider { | ||
| return { | ||
| log: vi.fn(), | ||
| taskHistoryStore: { get: () => undefined }, | ||
| // vi.fn() is not directly assignable to the typed method signature. | ||
| updateTaskHistory: updateTaskHistory as unknown as ClineProvider["updateTaskHistory"], | ||
| context: { | ||
| globalStorageUri: { fsPath: path.join(os.tmpdir(), "test-storage") }, | ||
| globalState: { | ||
| get: vi.fn().mockImplementation((_key: keyof GlobalState) => undefined), | ||
| update: vi.fn().mockResolvedValue(undefined), | ||
| keys: vi.fn().mockReturnValue([]), | ||
| }, | ||
| workspaceState: { | ||
| get: vi.fn().mockImplementation(() => undefined), | ||
| update: vi.fn().mockResolvedValue(undefined), | ||
| keys: vi.fn().mockReturnValue([]), | ||
| }, | ||
| secrets: { | ||
| get: vi.fn().mockResolvedValue(undefined), | ||
| store: vi.fn().mockResolvedValue(undefined), | ||
| delete: vi.fn().mockResolvedValue(undefined), | ||
| }, | ||
| extensionUri: { fsPath: "/mock/extension/path" }, | ||
| extension: { packageJSON: { version: "1.0.0" } }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // ─── Tests ─────────────────────────────────────────────────────────────────── | ||
|
|
||
| describe("Task resume/eviction race (Work #1 (no message) regression)", () => { | ||
| let mockApiConfig: ProviderSettings | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
|
|
||
| if (!TelemetryService.hasInstance()) { | ||
| TelemetryService.createInstance([]) | ||
| } | ||
|
|
||
| mockApiConfig = { | ||
| apiProvider: "anthropic", | ||
| apiModelId: "claude-3-5-sonnet-20241022", | ||
| apiKey: "test-api-key", | ||
| } | ||
| }) | ||
|
|
||
| it("does not clobber the real task title when evicted mid-resume", async () => { | ||
| const REAL_TITLE = "Write a short paragraph about the benefits of regular code reviews" | ||
|
|
||
| const historyItem: HistoryItem = { | ||
| id: "parent-task-1", | ||
| number: 1, | ||
| task: REAL_TITLE, | ||
| ts: Date.now() - 60_000, | ||
| tokensIn: 500, | ||
| tokensOut: 300, | ||
| totalCost: 0.01, | ||
| workspace: path.join(os.tmpdir(), "mock-workspace"), | ||
| } | ||
|
|
||
| // Hold the disk read open so the task is aborted while clineMessages is | ||
| // still empty — the same window a user hits by navigating away quickly. | ||
| const readDeferred = createDeferred<ClineMessage[]>() | ||
| mockReadTaskMessages.mockReturnValueOnce(readDeferred.promise) | ||
|
|
||
| const updateTaskHistory = vi.fn().mockResolvedValue([]) | ||
| const mockProvider = makeMockProvider(updateTaskHistory) | ||
|
|
||
| const task = new Task({ | ||
| provider: mockProvider as unknown as ClineProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| historyItem, | ||
| taskNumber: historyItem.number, | ||
| startTask: false, | ||
| }) | ||
|
|
||
| // Fire task.run() without awaiting — mirrors the fire-and-forget pattern | ||
| // in ClineProvider#createTaskWithHistoryItem. For history tasks, run() | ||
| // calls resumeTaskFromHistory(), which starts with an async disk read. | ||
| const runPromise = task.run().catch(() => { | ||
| // After abort, downstream steps (e.g. ask()) throw — expected. | ||
| }) | ||
|
|
||
| // Abort while the disk read is still in flight, as evictCurrentTask() | ||
| // does when the user navigates away before messages load. | ||
| await task.abortTask(true) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| // The fix: saveClineMessages() must not be called for a history task | ||
| // with clineMessages still empty. Verify the call was skipped entirely, | ||
| // not just that the specific "no_messages" key was not written. | ||
| expect(updateTaskHistory).not.toHaveBeenCalled() | ||
|
|
||
| // Let the read resolve so the promise does not leak into the next test. | ||
| readDeferred.resolve([ | ||
| { ts: historyItem.ts, type: "say", say: "text", text: REAL_TITLE }, | ||
| { ts: historyItem.ts + 1, type: "say", say: "completion_result", text: "Done." }, | ||
| ]) | ||
| await runPromise | ||
| }) | ||
| }) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.