Repository navigation
feat(checkpoints): per-write checkpoints, task-start baseline, and perWriteCheckpoints setting (B1, #1375) #1404
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
base: main
Are you sure you want to change the base?
Changes from all commits
abfbe7f
b98f159
abc10b4
0ea153d
cb657cb
92ceb90
23e9451
b01a334
c07bea3
93e3640
faf3f9d
fb57670
cae09fb
c6a23c5
484d5cf
0310c99
a83a105
d21a437
dd4802d
6666de5
3391528
6f00e59
e6e69fc
20af1c7
a0d292e
8045ab0
8865c4a
911ae24
377c082
da46574
b6d8698
4b44f10
5a32d91
2e66db5
c585b1f
c6bcc62
e2417cb
fc11d6d
7f9ebb4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest" | ||
|
|
||
| import type { Task } from "../../task/Task" | ||
| import { RepoPerTaskCheckpointService } from "../../../services/checkpoints" | ||
| import { checkGitInstalled } from "../../../utils/git" | ||
| import { getCheckpointService } from "../index" | ||
|
|
||
| vi.mock("../../../services/checkpoints", () => ({ | ||
| RepoPerTaskCheckpointService: { create: vi.fn() }, | ||
| })) | ||
|
|
||
| vi.mock("../../../utils/git", () => ({ | ||
| checkGitInstalled: vi.fn(), | ||
| })) | ||
|
|
||
| vi.mock("../../../utils/path", () => ({ | ||
| getWorkspacePath: vi.fn(() => "/workspace"), | ||
| })) | ||
|
|
||
| vi.mock("../../../i18n", () => ({ | ||
| t: (key: string) => key, | ||
| })) | ||
|
|
||
| /** | ||
| * Structural service double: getCheckpointService only registers listeners, | ||
| * awaits initShadowGit, and reads isInitialized. The double assertion at the | ||
| * create() call sites is confined to this file (AGENTS.md last-resort cast): | ||
| * the real class carries a Git handle and event surface the test never | ||
| * exercises. | ||
| */ | ||
| function serviceDouble(initShadowGitImpl: () => Promise<void>) { | ||
| const listeners = new Map<string, (...args: unknown[]) => void>() | ||
| let initialized = false | ||
| const svc = { | ||
| on: vi.fn((event: string, cb: (...args: unknown[]) => void) => { | ||
| listeners.set(event, cb) | ||
| }), | ||
| initShadowGit: vi.fn(() => initShadowGitImpl()), | ||
| get isInitialized() { | ||
| return initialized | ||
| }, | ||
| completeInit: () => { | ||
| initialized = true | ||
| listeners.get("initialize")?.() | ||
| }, | ||
| } | ||
| return svc | ||
| } | ||
|
|
||
| function makeTask(): Task { | ||
| return { | ||
| taskId: "task-init", | ||
| cwd: "/workspace", | ||
| enableCheckpoints: true, | ||
| checkpointService: undefined, | ||
| checkpointServiceInitializing: false, | ||
| checkpointTimeout: 1, | ||
| providerRef: { | ||
| deref: vi.fn().mockReturnValue({ | ||
| context: { globalStorageUri: { fsPath: "/tmp/b3-init-storage" } }, | ||
| postMessageToWebview: vi.fn(), | ||
| log: vi.fn(), | ||
| }), | ||
| }, | ||
| say: vi.fn().mockResolvedValue(undefined), | ||
| } as unknown as Task | ||
| } | ||
|
|
||
| describe("getCheckpointService initialization failures", () => { | ||
| beforeEach(() => { | ||
| vi.mocked(checkGitInstalled).mockReset() | ||
| vi.mocked(RepoPerTaskCheckpointService.create).mockReset() | ||
| }) | ||
|
|
||
| it("does not store a service whose shadow-git initialization failed", async () => { | ||
| vi.mocked(checkGitInstalled).mockResolvedValue(true) | ||
| const failing = serviceDouble(async () => { | ||
| throw new Error("shadow git init exploded") | ||
| }) | ||
| vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue( | ||
| failing as unknown as RepoPerTaskCheckpointService, | ||
| ) | ||
|
|
||
| const task = makeTask() | ||
| const service = await getCheckpointService(task) | ||
|
|
||
| // The failed service must not be stored: a stored uninitialized handle | ||
| // would be returned by the fast path to every later save, and the | ||
| // initializing flag must not strand later lookups in a wait for an | ||
| // initialization that already died. | ||
| expect(service).toBeUndefined() | ||
| expect(task.checkpointService).toBeUndefined() | ||
| expect(task.checkpointServiceInitializing).toBe(false) | ||
| expect(task.enableCheckpoints).toBe(false) | ||
| }) | ||
|
|
||
| it("does not store a service when Git is not installed", async () => { | ||
| vi.mocked(checkGitInstalled).mockResolvedValue(false) | ||
| const svc = serviceDouble(async () => {}) | ||
| vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) | ||
|
|
||
| const task = makeTask() | ||
| const service = await getCheckpointService(task) | ||
|
|
||
| expect(service).toBeUndefined() | ||
| expect(task.checkpointService).toBeUndefined() | ||
| expect(task.checkpointServiceInitializing).toBe(false) | ||
| expect(task.enableCheckpoints).toBe(false) | ||
| }) | ||
|
|
||
| it("stores an initialized service on the success path", async () => { | ||
| vi.mocked(checkGitInstalled).mockResolvedValue(true) | ||
| const svc = serviceDouble(async () => { | ||
| svc.completeInit() | ||
| }) | ||
| vi.mocked(RepoPerTaskCheckpointService.create).mockReturnValue(svc as unknown as RepoPerTaskCheckpointService) | ||
|
|
||
| const task = makeTask() | ||
| const service = await getCheckpointService(task) | ||
|
|
||
| expect(service).toBe(svc) | ||
| expect(task.checkpointService).toBe(svc) | ||
| expect(task.checkpointServiceInitializing).toBe(false) | ||
| expect(task.enableCheckpoints).toBe(true) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,6 +115,15 @@ | |
| const service = RepoPerTaskCheckpointService.create(options) | ||
| task.checkpointServiceInitializing = true | ||
| await checkGitInstallation(task, service, log, provider) | ||
| if (!service.isInitialized) { | ||
| // checkGitInstallation disables checkpoints when Git is unavailable and | ||
| // logs initShadowGit failures without throwing: storing such a service | ||
| // would hand later saves (and the fast path above) an uninitialized | ||
| // handle whose isInitialized can never flip, so a failed | ||
| // initialization must resolve to no service at all. | ||
| task.checkpointServiceInitializing = false | ||
| return undefined | ||
| } | ||
| task.checkpointService = service | ||
| if (task.enableCheckpoints) { | ||
| sendCheckpointInitWarn(task) | ||
|
|
@@ -202,6 +211,10 @@ | |
| } catch (err) { | ||
| log(`[Task#getCheckpointService] initShadowGit -> ${err.message}`) | ||
| task.enableCheckpoints = false | ||
| // The "initialize" event never fires for a failed initShadowGit, so | ||
| // clear the flag here or later lookups wait on an initialization | ||
| // that already died. | ||
| task.checkpointServiceInitializing = false | ||
|
Check warning on line 217 in src/core/checkpoints/index.ts
|
||
| } | ||
| } catch (err) { | ||
| log(`[Task#getCheckpointService] Unexpected error during Git check: ${err.message}`) | ||
|
|
@@ -211,6 +224,29 @@ | |
| } | ||
| } | ||
|
|
||
| // Serialize checkpoint saves per task. saveCheckpoint() runs stageAll + commit, so two | ||
| // concurrent saves for the same task (for example the task-start baseline racing the first | ||
| // per-write save after the baseline timeout) can stage each other's writes or interleave | ||
| // commits. A per-task promise queue makes every save wait for the previous one, including | ||
| // a baseline that is still running. | ||
| const checkpointSaveQueues = new Map<string, Promise<unknown>>() | ||
|
|
||
| function enqueueCheckpointSave<T>(taskId: string, operation: () => Promise<T>): Promise<T> { | ||
| const previous = checkpointSaveQueues.get(taskId) ?? Promise.resolve() | ||
| const next = previous.then(operation, operation) | ||
|
Comment on lines
+234
to
+236
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const tail = next.then( | ||
| () => undefined, | ||
| () => undefined, | ||
| ) | ||
| checkpointSaveQueues.set(taskId, tail) | ||
| void tail.finally(() => { | ||
|
Check warning on line 242 in src/core/checkpoints/index.ts
|
||
| if (checkpointSaveQueues.get(taskId) === tail) { | ||
|
Check warning on line 243 in src/core/checkpoints/index.ts
|
||
| checkpointSaveQueues.delete(taskId) | ||
|
Check warning on line 244 in src/core/checkpoints/index.ts
|
||
| } | ||
| }) | ||
| return next | ||
| } | ||
|
|
||
| export async function checkpointSave(task: Task, force = false, suppressMessage = false) { | ||
| const service = await getCheckpointService(task) | ||
|
|
||
|
|
@@ -220,13 +256,14 @@ | |
|
|
||
| TelemetryService.instance.captureCheckpointCreated(task.taskId) | ||
|
|
||
| // Start the checkpoint process in the background. | ||
| return service | ||
| .saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }) | ||
| .catch((err) => { | ||
| console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) | ||
| task.enableCheckpoints = false | ||
| }) | ||
| // Start the checkpoint process in the background, queued behind any save already | ||
| // running for this task so stageAll + commit never overlap. | ||
| return enqueueCheckpointSave(task.taskId, () => | ||
| service.saveCheckpoint(`Task: ${task.taskId}, Time: ${Date.now()}`, { allowEmpty: force, suppressMessage }), | ||
| ).catch((err) => { | ||
|
Comment on lines
+261
to
+263
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) | ||
|
Check warning on line 264 in src/core/checkpoints/index.ts
|
||
| task.enableCheckpoints = false | ||
| }) | ||
| } | ||
|
|
||
| export type CheckpointRestoreOptions = { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.