Skip to content

Checkpoints logging tweaks + defensive catches #945

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.

Sign up for GitHub

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
cte merged 7 commits into from
Feb 11, 2025
Merged

Checkpoints logging tweaks + defensive catches #945

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
68ea2e8
Checkpoints logging tweaks
cte Feb 11, 2025
6b35187
Add duration logging
cte Feb 11, 2025
37c2a17
Check for empty stash commits
cte Feb 11, 2025
d896078
Store initial checkpoint
cte Feb 11, 2025
be2e0b7
Merge pull request #946 from RooVetGit/cte/store-initial-checkpoint
cte Feb 11, 2025
d9ac8ef
Complete the comment
cte Feb 11, 2025
94cb09f
Log restoreCheckpoint duration
cte Feb 11, 2025
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
21 changes: 3 additions & 18 deletions src/core/Cline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3288,12 +3288,7 @@ export class Cline {
]),
)
} catch (err) {
this.providerRef
.deref()
?.log(
`[checkpointDiff] Encountered unexpected error: $${err instanceof Error ? err.message : String(err)}`,
)

this.providerRef.deref()?.log("[checkpointDiff] disabling checkpoints for this task")
this.checkpointsEnabled = false
}
}
Expand All @@ -3315,12 +3310,7 @@ export class Cline {
await this.say("checkpoint_saved", commit.commit)
}
} catch (err) {
this.providerRef
.deref()
?.log(
`[checkpointSave] Encountered unexpected error: $${err instanceof Error ? err.message : String(err)}`,
)

this.providerRef.deref()?.log("[checkpointSave] disabling checkpoints for this task")
this.checkpointsEnabled = false
}
}
Expand Down Expand Up @@ -3390,12 +3380,7 @@ export class Cline {
// Cline instance.
this.providerRef.deref()?.cancelTask()
} catch (err) {
this.providerRef
.deref()
?.log(
`[restoreCheckpoint] Encountered unexpected error: $${err instanceof Error ? err.message : String(err)}`,
)

this.providerRef.deref()?.log("[checkpointRestore] disabling checkpoints for this task")
this.checkpointsEnabled = false
}
}
Expand Down
115 changes: 82 additions & 33 deletions src/services/checkpoints/CheckpointService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,22 +132,64 @@ export class CheckpointService {
stashSha: string
force?: boolean
}) {
if (force) {
await this.git.checkout(["-f", this.mainBranch])
} else {
await this.git.checkout(this.mainBranch)
let currentBranch = await this.git.revparse(["--abbrev-ref", "HEAD"])

if (currentBranch !== this.mainBranch) {
if (force) {
try {
await this.git.checkout(["-f", this.mainBranch])
} catch (err) {
this.log(
`[restoreMain] failed to force checkout ${this.mainBranch}: ${err instanceof Error ? err.message : String(err)}`,
)
}
} else {
try {
await this.git.checkout(this.mainBranch)
} catch (err) {
this.log(
`[restoreMain] failed to checkout ${this.mainBranch}: ${err instanceof Error ? err.message : String(err)}`,
)

// Escalate to a forced checkout if we can't checkout the
// main branch under nor
Copy link
Collaborator

@mrubens mrubens Feb 11, 2025

Choose a reason for hiding this comment

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

Did this get cut off?

Copy link
Collaborator Author

@cte cte Feb 11, 2025

Choose a reason for hiding this comment

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

Oops, I think I lost my train of thought while writing that 😂

currentBranch = await this.git.revparse(["--abbrev-ref", "HEAD"])

if (currentBranch !== this.mainBranch) {
await this.git.checkout(["-f", this.mainBranch]).catch(() => {})
}
}
}
}

currentBranch = await this.git.revparse(["--abbrev-ref", "HEAD"])

if (currentBranch !== this.mainBranch) {
throw new Error(`Unable to restore ${this.mainBranch}`)
}

if (stashSha) {
this.log(`[restoreMain] applying stash ${stashSha}`)
await this.git.raw(["stash", "apply", "--index", stashSha])

try {
await this.git.raw(["stash", "apply", "--index", stashSha])
} catch (err) {
this.log(`[restoreMain] Failed to apply stash: ${err instanceof Error ? err.message : String(err)}`)
}
}

this.log(`[restoreMain] restoring from ${branch}`)
await this.git.raw(["restore", "--source", branch, "--worktree", "--", "."])
this.log(`[restoreMain] restoring from ${branch} branch`)

try {
await this.git.raw(["restore", "--source", branch, "--worktree", "--", "."])
} catch (err) {
this.log(`[restoreMain] Failed to restore branch: ${err instanceof Error ? err.message : String(err)}`)
}
}

public async saveCheckpoint(message: string) {
const startTime = Date.now()

await this.ensureBranch(this.mainBranch)

const stashSha = (await this.git.raw(["stash", "create"])).trim()
Expand All @@ -172,15 +214,13 @@ export class CheckpointService {
*/
try {
await this.git.add(["-A"])
const status = await this.git.status()
this.log(`[saveCheckpoint] status: ${JSON.stringify(status)}`)
Copy link
Collaborator Author

@cte cte Feb 11, 2025

Choose a reason for hiding this comment

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

This logging was too verbose.

} catch (err) {
await this.git.checkout(["-f", this.mainBranch])
await this.git.branch(["-D", stashBranch]).catch(() => {})

throw new Error(
`[saveCheckpoint] Failed in stage stash phase: ${err instanceof Error ? err.message : String(err)}`,
this.log(
`[saveCheckpoint] failed in stage stash phase: ${err instanceof Error ? err.message : String(err)}`,
)
await this.restoreMain({ branch: stashBranch, stashSha, force: true })
await this.git.branch(["-D", stashBranch]).catch(() => {})
throw err
}

/**
Expand All @@ -192,17 +232,25 @@ export class CheckpointService {
* - UNDO: Create branch
* - UNDO: Change branch
*/
let stashCommit

try {
// TODO: Add a test to see if empty commits break this.
const tempCommit = await this.git.commit(message, undefined, { "--no-verify": null })
this.log(`[saveCheckpoint] tempCommit: ${message} -> ${JSON.stringify(tempCommit)}`)
stashCommit = await this.git.commit(message, undefined, { "--no-verify": null })
this.log(`[saveCheckpoint] stashCommit: ${message} -> ${JSON.stringify(stashCommit)}`)
} catch (err) {
await this.git.checkout(["-f", this.mainBranch])
this.log(
`[saveCheckpoint] failed in stash commit phase: ${err instanceof Error ? err.message : String(err)}`,
)
await this.restoreMain({ branch: stashBranch, stashSha, force: true })
await this.git.branch(["-D", stashBranch]).catch(() => {})
throw err
}

throw new Error(
`[saveCheckpoint] Failed in stash commit phase: ${err instanceof Error ? err.message : String(err)}`,
)
if (!stashCommit) {
Copy link
Collaborator Author

@cte cte Feb 11, 2025

Choose a reason for hiding this comment

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

If we don't generate a stash commit then there are no changes to store in a checkpoint. This has handled by the diff step below, but we can short circuit that.

this.log("[saveCheckpoint] no stash commit")
await this.restoreMain({ branch: stashBranch, stashSha })
await this.git.branch(["-D", stashBranch])
return undefined
}

/**
Expand All @@ -219,12 +267,10 @@ export class CheckpointService {
try {
diff = await this.git.diff([latestSha, stashBranch])
} catch (err) {
this.log(`[saveCheckpoint] failed in diff phase: ${err instanceof Error ? err.message : String(err)}`)
await this.restoreMain({ branch: stashBranch, stashSha, force: true })
await this.git.branch(["-D", stashBranch]).catch(() => {})

throw new Error(
`[saveCheckpoint] Failed in diff phase: ${err instanceof Error ? err.message : String(err)}`,
)
throw err
}

if (!diff) {
Expand All @@ -249,12 +295,10 @@ export class CheckpointService {
await this.git.reset(["--hard", this.mainBranch])
this.log(`[saveCheckpoint] reset ${this.hiddenBranch}`)
} catch (err) {
this.log(`[saveCheckpoint] failed in reset phase: ${err instanceof Error ? err.message : String(err)}`)
await this.restoreMain({ branch: stashBranch, stashSha, force: true })
await this.git.branch(["-D", stashBranch]).catch(() => {})

throw new Error(
`[saveCheckpoint] Failed in reset phase: ${err instanceof Error ? err.message : String(err)}`,
)
throw err
}

/**
Expand Down Expand Up @@ -289,18 +333,23 @@ export class CheckpointService {
this.currentCheckpoint = commit
this.log(`[saveCheckpoint] cherry-pick commit = ${commit}`)
} catch (err) {
this.log(
`[saveCheckpoint] failed in cherry pick phase: ${err instanceof Error ? err.message : String(err)}`,
)
await this.git.reset(["--hard", latestSha]).catch(() => {})
await this.restoreMain({ branch: stashBranch, stashSha, force: true })
await this.git.branch(["-D", stashBranch]).catch(() => {})

throw new Error(
`[saveCheckpoint] Failed in cherry pick phase: ${err instanceof Error ? err.message : String(err)}`,
)
throw err
}

await this.restoreMain({ branch: stashBranch, stashSha })
await this.git.branch(["-D", stashBranch])

// We've gotten reports that checkpoints can be slow in some cases, so
// we'll log the duration of the checkpoint save.
const duration = Date.now() - startTime
this.log(`[saveCheckpoint] saved checkpoint ${commit} in ${duration}ms`)

return { commit }
}

Expand Down
7 changes: 5 additions & 2 deletions src/services/checkpoints/__tests__/CheckpointService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,12 @@ describe("CheckpointService", () => {
})

it("does not create a checkpoint if there are no pending changes", async () => {
const commit0 = await service.saveCheckpoint("Zeroth checkpoint")
expect(commit0?.commit).toBeFalsy()

await fs.writeFile(testFile, "Ahoy, world!")
const commit = await service.saveCheckpoint("First checkpoint")
expect(commit?.commit).toBeTruthy()
const commit1 = await service.saveCheckpoint("First checkpoint")
expect(commit1?.commit).toBeTruthy()

const commit2 = await service.saveCheckpoint("Second checkpoint")
expect(commit2?.commit).toBeFalsy()
Expand Down
Loading