Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,67 @@ describe("ArchiveService integration", () => {
expect(archived.branchName).toBeNull();
expect(ctx.archiveRepo.findAll()).toHaveLength(1);
}));

it("archive succeeds when worktree has an in-progress merge conflict", () =>
withTestContext({}, async (ctx) => {
const { worktreePath } = await ctx.setupWorktree("detached");

const wt = (cmd: string) =>
execSync(`git ${cmd}`, {
cwd: worktreePath,
encoding: "utf8",
stdio: "pipe",
});

await fs.writeFile(path.join(worktreePath, "c.txt"), "base\n");
wt("add c.txt");
wt("commit -m base");
wt("tag base_commit");

await fs.writeFile(path.join(worktreePath, "c.txt"), "AAA\n");
wt("add c.txt");
wt("commit -m a");
wt("tag commit_a");

wt("reset --hard base_commit");
await fs.writeFile(path.join(worktreePath, "c.txt"), "BBB\n");
wt("add c.txt");
wt("commit -m b");

wt("merge commit_a || true");

const archived = await ctx.service.archiveTask(ctx.archiveInput());

expect(archived.checkpointId).toBeNull();
expect(await pathExists(worktreePath)).toBe(false);
expect(ctx.archiveRepo.findAll()).toHaveLength(1);
}));

it("archive succeeds and drops the checkpoint when worktree removal fails", () =>
withTestContext({}, async (ctx) => {
await ctx.setupWorktree("detached");

// Simulate git refusing to remove the worktree (e.g. a stale lock).
// Capture succeeded, so the checkpoint ref exists — but since the
// worktree stays registered, keeping the restore point would make a
// later unarchive fail to re-add it. The archive must still be recorded
// and its checkpoint dropped.
const deleteSpy = vi
.spyOn(WorktreeManager.prototype, "deleteWorktree")
.mockRejectedValue(new Error("worktree is locked"));

try {
const archived = await ctx.service.archiveTask(ctx.archiveInput());

expect(deleteSpy).toHaveBeenCalled();
expect(archived.checkpointId).toBeNull();
const records = ctx.archiveRepo.findAll();
expect(records).toHaveLength(1);
expect(records[0].checkpointId).toBeNull();
} finally {
deleteSpy.mockRestore();
}
}));
});

describe("local/cloud mode", () => {
Expand Down
80 changes: 56 additions & 24 deletions packages/workspace-server/src/services/archive/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -246,24 +246,30 @@ export class ArchiveService {
archivedTask.branchName = actualBranch;
}

await step(
async () => {
if (!archivedTask.checkpointId) {
throw new Error("checkpointId must be set for worktree mode");
}
await this.captureWorktreeCheckpoint(
folderPath,
worktreePath,
archivedTask.checkpointId,
);
},
async () => {
if (archivedTask.checkpointId) {
const checkpointId = archivedTask.checkpointId;
try {
if (!checkpointId) {
throw new Error("checkpointId must be set for worktree mode");
}
await step(
() =>
this.captureWorktreeCheckpoint(
folderPath,
worktreePath,
checkpointId,
),
async () => {
const git = createGitClient(folderPath);
await deleteCheckpoint(git, archivedTask.checkpointId);
}
},
);
await deleteCheckpoint(git, checkpointId);
},
);
} catch (error) {
this.log.warn(
`Failed to capture checkpoint for ${worktreePath}; archiving without a restore point`,
{ error },
);
archivedTask.checkpointId = null;
}
}

await step(
Expand All @@ -277,13 +283,39 @@ export class ArchiveService {

await step(
async () => {
const manager = new WorktreeManager({
mainRepoPath: folderPath,
worktreeBasePath: this.workspaceSettings.getWorktreeLocation(),
});
await manager.deleteWorktree(worktreePath);
const parentDir = path.dirname(worktreePath);
await forceRemove(parentDir);
try {
const manager = new WorktreeManager({
mainRepoPath: folderPath,
worktreeBasePath: this.workspaceSettings.getWorktreeLocation(),
});
await manager.deleteWorktree(worktreePath);
const parentDir = path.dirname(worktreePath);
await forceRemove(parentDir);
} catch (error) {
this.log.warn(
`Failed to remove worktree at ${worktreePath}; archiving anyway (on-disk worktree may need manual cleanup)`,
{ error },
);
// The worktree is still registered under its original name, so a
// later unarchive can't re-add it from the checkpoint (git rejects
// the duplicate name/path), leaving the task un-restorable. Drop
// the restore point — and its now-orphaned checkpoint ref — so the
// archive record stays internally consistent, matching how a
// failed capture above already sets checkpointId to null.
const orphanedCheckpointId = archivedTask.checkpointId;
if (orphanedCheckpointId) {
archivedTask.checkpointId = null;
try {
const git = createGitClient(folderPath);
await deleteCheckpoint(git, orphanedCheckpointId);
} catch (cleanupError) {
this.log.warn(
`Failed to delete orphaned checkpoint ${orphanedCheckpointId}`,
{ error: cleanupError },
);
}
}
}
},
async () => {},
);
Comment on lines 284 to 321

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Silent deletion failure leaves unarchivable state when checkpoint succeeded

When deleteWorktree throws (e.g. a git lock file or a still-registered worktree), the inner try/catch swallows the error and step() considers the deletion a success. If checkpoint capture already succeeded at this point, the archive record is written with a non-null checkpointId, while the git worktree is still registered under its original name in the main repo. A subsequent unarchiveTask call hits shouldRestoreWorktree = true, then restoreWorktreeFromCheckpoint asks the WorktreeManager to add a worktree with preferredName = worktree.name — which git will reject because that name/path is already registered, leaving the task permanently un-restorable.

The fix in the checkpoint path (nulling checkpointId on failure) avoids this because shouldRestoreWorktree becomes false. The same treatment would apply here: if deletion fails and a checkpoint was captured, set archivedTask.checkpointId = null (and clean up the now-orphaned checkpoint ref) so the archive record is internally consistent.

Expand Down
Loading