-
Notifications
You must be signed in to change notification settings - Fork 0
fix(snapshot): harden track staging and tree writes #76
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: dev
Are you sure you want to change the base?
Changes from all commits
69e3eae
f09f0b4
d7d809e
f7ee4f5
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 |
|---|---|---|
|
|
@@ -8,11 +8,14 @@ import z from "zod" | |
| import { Config } from "../config/config" | ||
| import { Instance } from "../project/instance" | ||
| import { Scheduler } from "../scheduler" | ||
| import { git as runGit } from "../util/git" | ||
|
|
||
| export namespace Snapshot { | ||
| const log = Log.create({ service: "snapshot" }) | ||
| const hour = 60 * 60 * 1000 | ||
| const prune = "7.days" | ||
| const treehash = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/ | ||
| const retry = [0, 25, 100] | ||
| type Gate = { | ||
| held: boolean | ||
| wait: (() => void)[] | ||
|
|
@@ -134,14 +137,12 @@ export namespace Snapshot { | |
| await $`git --git-dir ${git} config core.fsmonitor false`.quiet().nothrow() | ||
| log.info("initialized") | ||
| } | ||
| await add(git) | ||
| const hash = await $`git --git-dir ${git} --work-tree ${Instance.worktree} write-tree` | ||
| .quiet() | ||
| .cwd(Instance.directory) | ||
| .nothrow() | ||
| .text() | ||
| log.info("tracking", { hash, cwd: Instance.directory, git }) | ||
| return hash.trim() | ||
| const staged = await add(git) | ||
| if (!staged) return | ||
| const tree = await write(git) | ||
| if (!tree) return | ||
| log.info("tracking", { hash: tree, cwd: Instance.directory, git }) | ||
| return tree | ||
| } | ||
|
|
||
| export const Patch = z.object({ | ||
|
|
@@ -153,7 +154,8 @@ export namespace Snapshot { | |
| export async function patch(hash: string): Promise<Patch> { | ||
| const git = gitdir() | ||
| using _ = await lock(git) | ||
| await add(git) | ||
| const staged = await add(git) | ||
| if (!staged) return { hash, files: [] } | ||
| const result = | ||
| await $`git -c core.autocrlf=false -c core.longpaths=true -c core.symlinks=true -c core.quotepath=false --git-dir ${git} --work-tree ${Instance.worktree} diff --no-ext-diff --name-only ${hash} -- .` | ||
| .quiet() | ||
|
|
@@ -235,7 +237,8 @@ export namespace Snapshot { | |
| export async function diff(hash: string) { | ||
| const git = gitdir() | ||
| using _ = await lock(git) | ||
| await add(git) | ||
| const staged = await add(git) | ||
| if (!staged) return "" | ||
| const result = | ||
| await $`git -c core.autocrlf=false -c core.longpaths=true -c core.symlinks=true -c core.quotepath=false --git-dir ${git} --work-tree ${Instance.worktree} diff --no-ext-diff ${hash} -- .` | ||
| .quiet() | ||
|
|
@@ -330,10 +333,101 @@ export namespace Snapshot { | |
|
|
||
| async function add(git: string) { | ||
| await syncExclude(git) | ||
| await $`git -c core.autocrlf=false -c core.longpaths=true -c core.symlinks=true --git-dir ${git} --work-tree ${Instance.worktree} add .` | ||
| .quiet() | ||
| .cwd(Instance.directory) | ||
| .nothrow() | ||
| const result = await command({ | ||
| args: [ | ||
| "-c", | ||
| "core.autocrlf=false", | ||
| "-c", | ||
| "core.longpaths=true", | ||
| "-c", | ||
| "core.symlinks=true", | ||
| "--git-dir", | ||
| git, | ||
| "--work-tree", | ||
| Instance.worktree, | ||
| "add", | ||
| ".", | ||
| ], | ||
| cwd: Instance.directory, | ||
| retryLock: true, | ||
| }) | ||
| if (result.exitCode === 0) return true | ||
| log.warn("git add failed", { | ||
| git, | ||
| cwd: Instance.directory, | ||
| exitCode: result.exitCode, | ||
| stderr: result.stderr, | ||
| stdout: result.stdout, | ||
| }) | ||
| return false | ||
| } | ||
|
|
||
| async function write(git: string) { | ||
| const result = await command({ | ||
| args: ["--git-dir", git, "--work-tree", Instance.worktree, "write-tree"], | ||
| cwd: Instance.directory, | ||
| retryLock: true, | ||
| }) | ||
| if (result.exitCode !== 0) { | ||
| log.warn("write-tree failed", { | ||
| git, | ||
| cwd: Instance.directory, | ||
| exitCode: result.exitCode, | ||
| stderr: result.stderr, | ||
| stdout: result.stdout, | ||
| }) | ||
| return | ||
| } | ||
| if (!treehash.test(result.text)) { | ||
| log.warn("write-tree returned invalid hash", { | ||
| git, | ||
| cwd: Instance.directory, | ||
| hash: result.text, | ||
| }) | ||
| return | ||
| } | ||
| return result.text | ||
| } | ||
|
|
||
| function output(input: Buffer | ReadableStream<Uint8Array>) { | ||
| if (Buffer.isBuffer(input)) return input.toString() | ||
| return "" | ||
| } | ||
|
|
||
| function conflict(input: string) { | ||
| const text = input.toLowerCase() | ||
| if (text.includes("index.lock")) return true | ||
| if (text.includes("another git process seems to be running")) return true | ||
| return false | ||
| } | ||
riatzukiza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| async function command(input: { | ||
| args: string[] | ||
| cwd: string | ||
| retryLock?: boolean | ||
| }) { | ||
| let last = { | ||
| exitCode: 1, | ||
| stderr: "", | ||
| stdout: "", | ||
| text: "", | ||
| } | ||
| for (const ms of retry) { | ||
| if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms)) | ||
| const result = await runGit(input.args, { cwd: input.cwd }) | ||
| const stdout = (await result.text()).trim() | ||
| const stderr = output(result.stderr) | ||
| last = { | ||
| exitCode: result.exitCode, | ||
| stderr, | ||
| stdout, | ||
| text: stdout, | ||
| } | ||
| if (last.exitCode === 0) return last | ||
| if (!input.retryLock) return last | ||
| if (!conflict(`${stderr}\n${stdout}`)) return last | ||
| } | ||
| return last | ||
| } | ||
|
Comment on lines
+404
to
431
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. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chain🏁 Script executed: fd -t f "git\.ts" | head -20Repository: riatzukiza/opencode Length of output: 96 🏁 Script executed: find . -path "*/util/git.ts" -o -path "*/git.ts" | grep -E "(util|src)" | head -10Repository: riatzukiza/opencode Length of output: 98 🏁 Script executed: git ls-files | grep -E "git\.ts" | head -20Repository: riatzukiza/opencode Length of output: 96 🏁 Script executed: cat -n packages/opencode/src/util/git.tsRepository: riatzukiza/opencode Length of output: 2507 🏁 Script executed: cat -n packages/opencode/src/snapshot/index.ts | sed -n '404,431p'Repository: riatzukiza/opencode Length of output: 993 🏁 Script executed: # Check the full snapshot function context and callers
cat -n packages/opencode/src/snapshot/index.ts | sed -n '390,450p'Repository: riatzukiza/opencode Length of output: 2133 🏁 Script executed: # Find where the command() function is called to verify if both stdout and text fields are used
rg -A 3 -B 1 "command\(" packages/opencode/src/snapshot/index.ts | grep -E "(add|write|stdout|text)" | head -40Repository: riatzukiza/opencode Length of output: 180 🏁 Script executed: # Search for calls to the command function to see usage patterns
rg "await command\(" packages/opencode/src/snapshot/index.ts -A 5 -B 2Repository: riatzukiza/opencode Length of output: 515 🏁 Script executed: # Find the full add() and write() functions to see which result fields are used
rg -A 20 "async function add\(git: string\)" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode Length of output: 533 🏁 Script executed: # Find the full write() function
rg -A 15 "async function write\(git: string\)" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode Length of output: 518 🏁 Script executed: # Check if result.text is used anywhere after calling command()
rg "result\.text" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode Length of output: 265 🏁 Script executed: # Let's also verify the retry variable to understand the for loop better
rg -B 30 "async function command" packages/opencode/src/snapshot/index.ts | grep -E "retry|function snapshot"Repository: riatzukiza/opencode Length of output: 45 🏁 Script executed: # Find where retry is defined
rg "retry" packages/opencode/src/snapshot/index.ts -B 5 -A 2 | head -60Repository: riatzukiza/opencode Length of output: 1316 🏁 Script executed: # Check the full context around the command function to see retry definition and usage
cat -n packages/opencode/src/snapshot/index.ts | sed -n '350,410p'Repository: riatzukiza/opencode Length of output: 1972 🏁 Script executed: # Search for where result.text is actually used in the file
rg -n "result\.text" packages/opencode/src/snapshot/index.tsRepository: riatzukiza/opencode Length of output: 289
Three issues in this function:
Additionally, the returned object includes both ♻️ Proposed refactor addressing all three points- async function command(input: {
- args: string[]
- cwd: string
- retryLock?: boolean
- }) {
- let last = {
- exitCode: 1,
- stderr: "",
- stdout: "",
- text: "",
- }
- for (const ms of retry) {
- if (ms > 0) await new Promise((resolve) => setTimeout(resolve, ms))
- const result = await runGit(input.args, { cwd: input.cwd })
- const stdout = (await result.text()).trim()
- const stderr = output(result.stderr)
- last = {
- exitCode: result.exitCode,
- stderr,
- stdout,
- text: stdout,
- }
- if (last.exitCode === 0) return last
- if (!input.retryLock) return last
- if (!conflict(`${stderr}\n${stdout}`)) return last
- }
- return last
- }
+ async function command(input: { args: string[]; cwd: string; retryLock?: boolean }) {
+ const run = async (ms: number) => {
+ if (ms > 0) await new Promise<void>((resolve) => setTimeout(resolve, ms))
+ const result = await runGit(input.args, { cwd: input.cwd })
+ const stdout = result.text().trim()
+ const stderr = output(result.stderr)
+ return { exitCode: result.exitCode, stderr, stdout }
+ }
+ return retry.reduce(
+ async (prev, ms) => {
+ const last = await prev
+ if (last.exitCode === 0) return last
+ if (!input.retryLock) return last
+ if (!conflict(`${last.stderr}\n${last.stdout}`)) return last
+ return run(ms)
+ },
+ run(0),
+ )
+ }🤖 Prompt for AI Agents |
||
|
|
||
| async function syncExclude(git: string) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output()silently discardsReadableStreamstderr — narrow the type to match actual callers.The
ReadableStream<Uint8Array>branch is unreachable:util/git.tsreturnsBufferforstderrin both the ACP and non-ACP paths. The overly broad union type creates a false safety guarantee — ifstderrwere ever actually a stream,conflict()would receive""and suppress the lock-contention retry silently.♻️ Proposed fix
🤖 Prompt for AI Agents
🧩 Analysis chain
🏁 Script executed:
Repository: riatzukiza/opencode
Length of output: 200
🏁 Script executed:
Repository: riatzukiza/opencode
Length of output: 177
🏁 Script executed:
Repository: riatzukiza/opencode
Length of output: 2507
Narrow
output()parameter type toBuffer— theReadableStreambranch is dead code.The function accepts
Buffer | ReadableStream<Uint8Array>but only handles theBuffercase, silently returning""for streams. Both branches inutil/git.tsalways returnBufferforstderr: the ACP branch explicitly creates buffers viaBuffer.from(), and the Bun shell branch buffers output by default without stream configuration. The single caller at line 419 passesresult.stderr, which is alwaysBuffer. TheReadableStreambranch is unreachable and should be removed.♻️ Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents