diff --git a/bench/bashloop/door.go b/bench/bashloop/door.go index 6a263ca9ab..f1c3320a77 100644 --- a/bench/bashloop/door.go +++ b/bench/bashloop/door.go @@ -38,6 +38,7 @@ import ( "strings" "time" + "github.com/Agent-Field/codeaf/internal/sqliteuri" _ "modernc.org/sqlite" // the store driver: graph.db is read read-only ) @@ -342,7 +343,12 @@ func doModelFlags(iv invocation) []string { // and no error — a run that ended before the store carried that table is an // honest no-reading, the same law collectReadings keeps. func applyStoreReadings(reading *readings, dbPath string) error { - db, err := sql.Open("sqlite", "file:"+dbPath+"?mode=ro") + uri, err := sqliteuri.FromPath(dbPath) + if err != nil { + return err + } + uri.RawQuery = "mode=ro" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return err } @@ -466,7 +472,12 @@ func storeCommand(tool, body string) string { // page teaches it). Zero bash calls with zero rejections and zero plan-CLI // words is the shape an unarmed arm leaves. func storeBeltFingerprints(dbPath string) (bashCalls, rejections, planWords int) { - db, err := sql.Open("sqlite", "file:"+dbPath+"?mode=ro") + uri, err := sqliteuri.FromPath(dbPath) + if err != nil { + return 0, 0, 0 + } + uri.RawQuery = "mode=ro" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return 0, 0, 0 } diff --git a/bench/bashloop/door_uri_test.go b/bench/bashloop/door_uri_test.go new file mode 100644 index 0000000000..03c24da5bb --- /dev/null +++ b/bench/bashloop/door_uri_test.go @@ -0,0 +1,42 @@ +package main + +import ( + "database/sql" + "path/filepath" + "runtime" + "testing" + + "github.com/Agent-Field/codeaf/internal/sqliteuri" +) + +func TestStoreReadersUseLiteralDatabasePath(t *testing.T) { + name := "graph%23 #&.db" + if runtime.GOOS != "windows" { + name = "graph%23 #&?.db" + } + path := filepath.Join(t.TempDir(), name) + uri := sqliteuri.File(path) + db, err := sql.Open("sqlite", uri.String()) + if err != nil { + t.Fatal(err) + } + _, err = db.Exec(`CREATE TABLE usage(cost REAL,prompt_tokens INT,completion_tokens INT); + INSERT INTO usage VALUES (0.25,11,7); + CREATE TABLE transcript(kind TEXT,tool TEXT,body TEXT); + INSERT INTO transcript VALUES('tool_call','bash','plandb show root');`) + if err != nil { + t.Fatal(err) + } + defer db.Close() + var got readings + if err = applyStoreReadings(&got, path); err != nil { + t.Fatal(err) + } + if got.CostUSD != .25 || got.InputTokens != 11 || got.OutputTokens != 7 || got.Calls != 1 { + t.Fatalf("usage read wrong store: %+v", got) + } + bash, _, plans := storeBeltFingerprints(path) + if bash != 1 || plans != 1 { + t.Fatalf("read wrong store: bash=%d plan=%d", bash, plans) + } +} diff --git a/docs/changes/unreleased/1910-plan-database-open.md b/docs/changes/unreleased/1910-plan-database-open.md new file mode 100644 index 0000000000..5427a8f5ab --- /dev/null +++ b/docs/changes/unreleased/1910-plan-database-open.md @@ -0,0 +1,13 @@ +--- +kind: fixed +title: Open plans beside writers and preserve literal database paths +pr: 1910 +surface: [engine] +invalidates: + - "Opening an existing plan previously acquired the writer lock, so a worker read could fail after five seconds behind another writer. Current-schema inspection and adoption now use a read-only snapshot." + - "Seven plan and ledger openers treated filename characters as SQLite URI syntax. They now encode the literal absolute or relative filesystem path." +--- + +Creation and migration retain the serialized writer path. Writes keep their +existing bounded retry budget; exhaustion names the contention and tells the +caller to retry after the other writer finishes, without claiming success. diff --git a/internal/e2e/plan_database_wire_e2e_test.go b/internal/e2e/plan_database_wire_e2e_test.go new file mode 100644 index 0000000000..086840b38e --- /dev/null +++ b/internal/e2e/plan_database_wire_e2e_test.go @@ -0,0 +1,222 @@ +//go:build e2e + +package e2e + +import ( + "context" + "database/sql" + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/sqliteuri" + _ "modernc.org/sqlite" +) + +func planCLI(t *testing.T, ctx context.Context, dir, path string, args ...string) (string, error) { + t.Helper() + argv := append([]string{"plandb"}, args...) + argv = append(argv, "--db", path, "--json") + home := t.TempDir() + env := append(os.Environ(), "CODEAF_HOME="+home, "CODEAF_PROFILE_DIR=", "PLANDB_DB=", "PLANDB_RUN=", "PLANDB_AGENT=") + cmd := guardedCommand(t, ctx, home, env, binary(t), argv...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + return string(out), err +} +func mustPlanCLI(t *testing.T, dir, path string, args ...string) string { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + out, err := planCLI(t, ctx, dir, path, args...) + if err != nil { + t.Fatalf("actual plan CLI %v failed: %v %s", args, err, out) + } + return out +} + +func TestPlanCLIReadsWhileWriterHeldAndFinishesAfterRelease(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "contended.db") + mustPlanCLI(t, dir, path, "init", "fictional-lock-plan", "--description", "Verify concurrent plan inspection") + claimPlanProof(t, dir, path, "fixture-writer") + uri := sqliteuri.File(path) + uri.RawQuery = "_txlock=immediate&_pragma=busy_timeout(1000)" + db, err := sql.Open("sqlite", uri.String()) + if err != nil { + t.Fatal(err) + } + defer db.Close() + tx, err := db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + // Hold beyond the old five-second busy timeout before opening a fresh CLI. + time.Sleep(6 * time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + began := time.Now() + out, err := planCLI(t, ctx, dir, path, "show", "t-proof") + cancel() + if err != nil || !strings.Contains(out, "fictional-proof-task") { + t.Fatalf("read CLI failed while writer remained held: %v %s", err, out) + } + readElapsed := time.Since(began) + type result struct { + out string + err error + } + finished := make(chan result, 1) + ctx, cancel = context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + go func() { + out, err := planCLI(t, ctx, dir, path, "done", "t-proof", "--agent", "fixture-writer", "--result", "LOCK_RELEASE_PROOF") + finished <- result{out, err} + }() + select { + case got := <-finished: + t.Fatalf("write ended before held transaction released: %v %s", got.err, got.out) + case <-time.After(time.Second): + } + if err := tx.Commit(); err != nil { + t.Fatal(err) + } + got := <-finished + if got.err != nil || !strings.Contains(got.out, "LOCK_RELEASE_PROOF") { + t.Fatalf("write did not recover after release: %v %s", got.err, got.out) + } + reopened := mustPlanCLI(t, dir, path, "show", "t-proof") + if !strings.Contains(reopened, "LOCK_RELEASE_PROOF") || !strings.Contains(reopened, `"done"`) { + t.Fatalf("reopen lost completion: %s", reopened) + } + t.Logf("PASS actual CLI: read returned in %s with writer still held after6s; done waited for release and reopened with exact completion proof.", readElapsed) +} + +func TestPlanCLIPreservesLiteralDatabasePaths(t *testing.T) { + for _, name := range []string{"plan%23.db", "plan#&%.db", "plan?mode=ro.db", "nested #%?&/ledger%23#&?.db"} { + t.Run(name, func(t *testing.T) { + for _, relative := range []bool{false, true} { + t.Run(fmt.Sprintf("relative=%t", relative), func(t *testing.T) { + dir := t.TempDir() + literal := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(literal), 0o700); err != nil { + t.Fatal(err) + } + path := literal + if relative { + path = name + } + mustPlanCLI(t, dir, path, "init", "literal-path-proof", "--description", "The filename is literal") + if info, err := os.Stat(literal); err != nil || info.Size() == 0 { + t.Fatalf("literal requested database missing: %v", err) + } + claimPlanProof(t, dir, path, "path-owner") + mustPlanCLI(t, dir, path, "done", "t-proof", "--agent", "path-owner", "--result", "LITERAL_PATH_OK") + out := mustPlanCLI(t, dir, path, "show", "t-proof") + if !strings.Contains(out, "LITERAL_PATH_OK") { + t.Fatalf("literal reopen lost update: %s", out) + } + // Inspect the entire isolated tree: a decoded/truncated sibling must not + // substitute for the exact filename or accompany a seemingly successful CLI. + _ = filepath.WalkDir(dir, func(actual string, d os.DirEntry, err error) error { + if err != nil { + t.Error(err) + return nil + } + if !d.IsDir() && actual != literal && actual != literal+"-wal" && actual != literal+"-shm" { + t.Errorf("unexpected decoded/truncated database sibling: %s", actual) + } + return nil + }) + t.Logf("PASS actual CLI literal path (relative=%t): create, claim, finish, reopen exact proof; no decoded sibling.", relative) + }) + } + }) + } +} + +func TestSpecialPathProjectLivePlanWorkflow(t *testing.T) { + requireTmuxAndKey(t) + const model = "deepseek/deepseek-v4.1-flash" + if e2eModel != model { + t.Fatalf("requires CODEAF_E2E_MODEL=%s", model) + } + home := recoveryShortHome(t, newHome(t, map[string]any{"memory.enabled": false})) + ws := workspaceAt(t, filepath.Join(t.TempDir(), "parcel #%23?& +"), false) + env := []string{config.APIKeyEnv + "=" + liveKey(t), "CODEAF_BASE_URL=https://openrouter.ai/api/v1", "CODEAF_PROFILE_DIR="} + stopRecoveryEngineOnCleanup(t, env, home, ws) + rig := startWithEnv(t, env, "special_path_plan", home, ws, 170, 48, "chat", "--model", model, "--one-model") + rig.skipSetup(t) + prompt := "We are preparing a fictional parcel counter. Inspect this repository and report the current files; do not edit yet." + rig.lit(prompt) + rig.keys("Enter") + if answer, _ := privateMemoryAwaitAnswer(home, prompt, 90*time.Second); answer == "" { + t.Fatal("initial inspection did not complete") + } + rig.lit("/task create count_parcels.py with a function count_lines(text) returning the number of nonempty lines, add test_count_parcels.py using the Python standard-library unittest module with at least three tests including empty input, run the tests, and finish only when they pass. Keep all work in your assigned project copy.") + rig.keys("Enter") + for deadline := time.Now().Add(4 * time.Minute); !freeConsentDoneNote(home); { + if time.Now().After(deadline) { + t.Fatalf("special-path actual task did not finish: %s", rig.capture()) + } + time.Sleep(pollEvery) + } + branch := runBranch(t, ws) + if branch == "" { + t.Fatal("actual task has no saved branch") + } + proofDir := t.TempDir() + for _, file := range []string{"count_parcels.py", "test_count_parcels.py"} { + raw, err := exec.Command("git", "-C", ws, "show", branch+":"+file).CombinedOutput() + if err != nil || len(raw) == 0 { + t.Fatalf("task artifact %s missing: %v %s", file, err, raw) + } + if err := os.WriteFile(filepath.Join(proofDir, file), raw, 0o600); err != nil { + t.Fatal(err) + } + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + check := exec.CommandContext(ctx, "python3", "-m", "unittest", "-v") + check.Dir = proofDir + output, checkErr := check.CombinedOutput() + cancel() + count := regexp.MustCompile(`Ran (\d+) tests?`).FindStringSubmatch(string(output)) + n := 0 + if len(count) > 1 { + n, _ = strconv.Atoi(count[1]) + } + if checkErr != nil || n < 3 { + t.Fatalf("actual task tests failed or omitted requested cases: %v %s", checkErr, output) + } + t.Logf("Actual branch artifacts independently checked with %d passing tests.", n) + models := callModels(t, home) + if len(models) == 0 { + t.Fatal("no live receipts") + } + for _, got := range models { + if got != model { + t.Fatalf("unexpected model %s", got) + } + } + t.Logf("PASS live special-path project: actual inspected repository, two-file tested plan completed, both artifacts on task branch; all %d receipts exact %s.\n%s", len(models), model, rig.capture()) +} + +func claimPlanProof(t *testing.T, dir, path, agent string) { + t.Helper() + mustPlanCLI(t, dir, path, "add", "fictional-proof-task", "--as", "proof", "--description", "Inspect and complete this fictional proof task") + out := mustPlanCLI(t, dir, path, "go", "--agent", agent) + var claimed struct { + ID string `json:"id"` + } + if err := json.Unmarshal([]byte(out), &claimed); err != nil || claimed.ID != "t-proof" { + t.Fatalf("actual CLI did not claim the proof child: %v %s", err, out) + } +} diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 250a282ae1..9d9017bc2c 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -803,6 +803,14 @@ no proof is not done: the worker proves it then, or reports it undone in its result. Reaching the end of the steps is not the same as having met every requirement in them, and speed is no permission to skip the walk. +An up-to-date plan remains readable while another worker is writing: `plandb show` +and the other read commands see the last committed state. Mutations wait for the +writer within a bounded retry budget. If that wait expires, the command reports +that no changes were made and asks you to retry after the other writer finishes. +Creating or upgrading an older database still requires a write lock. Paths passed +to `--db` name the literal file, including spaces, percent signs and punctuation; +a relative path remains relative to the command's working folder. + ## Costs and limits - **The cost cap.** A run may spend what is left of the conversation's own **spend rail** diff --git a/internal/plandb/open_contention_test.go b/internal/plandb/open_contention_test.go new file mode 100644 index 0000000000..1b8b14e860 --- /dev/null +++ b/internal/plandb/open_contention_test.go @@ -0,0 +1,131 @@ +package plandb + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +func TestOpenReadsCommittedPlanWhileAnotherWriterIsHeld(t *testing.T) { + path := filepath.Join(t.TempDir(), "plan.db") + first, err := Open(path, "mock", "root", "Mock task", "") + if err != nil { + t.Fatal(err) + } + defer first.Close() + tx, err := first.db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + if _, err = tx.Exec("UPDATE tasks SET title='not committed' WHERE id='root'"); err != nil { + t.Fatal(err) + } + began := time.Now() + second, err := Open(path, "", "", "", "") + if err != nil { + t.Fatalf("reading open waited behind writer (%s): %v", time.Since(began), err) + } + defer second.Close() + if time.Since(began) > time.Second { + t.Fatalf("reading open waited %s", time.Since(began)) + } + root, err := second.Show("root") + if err != nil || root.Title != "Mock task" { + t.Fatalf("committed snapshot: %+v %v", root, err) + } +} + +func TestOpenPreservesLiteralDatabasePath(t *testing.T) { + for _, name := range []string{"plan%23.db", "plan#question?.db", "plan & space ü.db"} { + t.Run(name, func(t *testing.T) { + if runtime.GOOS == "windows" && strings.Contains(name, "?") { + t.Skip("question mark is not a valid Windows filename") + } + path := filepath.Join(t.TempDir(), name) + store, err := Open(path, "mock", "root", "Mock task", "") + if err != nil { + t.Fatal(err) + } + if err = store.Close(); err != nil { + t.Fatal(err) + } + if _, err = os.Stat(path); err != nil { + t.Fatalf("literal database missing: %v", err) + } + reopened, err := Open(path, "", "", "", "") + if err != nil { + t.Fatal(err) + } + defer reopened.Close() + }) + } +} + +func TestOpenRelativeDatabasePathStaysInWorkingDirectory(t *testing.T) { + t.Chdir(t.TempDir()) + store, err := Open("plan%23.db", "mock", "root", "Mock task", "") + if err != nil { + t.Fatal(err) + } + if err := store.Close(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat("plan%23.db"); err != nil { + t.Fatal(err) + } + reopened, err := Open("plan%23.db", "", "", "", "") + if err != nil { + t.Fatal(err) + } + defer reopened.Close() +} + +func TestOpenBesideWriterStillRefusesContendedWritesTruthfully(t *testing.T) { + path := filepath.Join(t.TempDir(), "plan.db") + first, err := Open(path, "mock", "root", "Mock task", "") + if err != nil { + t.Fatal(err) + } + defer first.Close() + tx, err := first.db.Begin() + if err != nil { + t.Fatal(err) + } + defer tx.Rollback() + second, err := Open(path, "", "", "", "") + if err != nil { + t.Fatal(err) + } + defer second.Close() + // Shorten only this test's existing SQLite timeout; production retains5s. + if _, err = second.db.Exec("PRAGMA busy_timeout=25"); err != nil { + t.Fatal(err) + } + began := time.Now() + if tx, err := second.beginWriteUntil(time.Now().Add(10 * time.Millisecond)); err == nil { + tx.Rollback() + t.Fatal("contended write falsely reported success") + } else if !isBusy(err) || !strings.Contains(err.Error(), "retry this command") { + t.Fatalf("write exhaustion lost cause or recovery guidance: %v", err) + } + if time.Since(began) > time.Second { + t.Fatal("write ignored its configured busy bound") + } + root, err := second.Show("root") + if err != nil || root.Description != "" { + t.Fatalf("failed write changed plan: %+v %v", root, err) + } + if err = tx.Rollback(); err != nil { + t.Fatal(err) + } + if _, err = second.Amend("root", "lands after release"); err != nil { + t.Fatalf("write did not recover after lock release: %v", err) + } + if _, err = second.rdb.Exec("UPDATE tasks SET title='unexpected'"); err == nil { + t.Fatal("read handle accepted a write") + } +} diff --git a/internal/plandb/persist.go b/internal/plandb/persist.go index 569468ba4e..231ec0d6f8 100644 --- a/internal/plandb/persist.go +++ b/internal/plandb/persist.go @@ -10,6 +10,7 @@ import ( "strings" "time" + "github.com/Agent-Field/codeaf/internal/sqliteuri" "modernc.org/sqlite" ) @@ -42,7 +43,16 @@ const sqliteBusyCode = 5 // which a writer is free to retry. func isBusy(err error) bool { var refusal *sqlite.Error - return errors.As(err, &refusal) && refusal.Code() == sqliteBusyCode + return errors.As(err, &refusal) && refusal.Code()&255 == sqliteBusyCode +} + +// writeLockError preserves SQLite's typed cause and explains a safe retry. +// It is used before a write transaction begins, so no mutation was applied. +func writeLockError(err error) error { + if isBusy(err) { + return fmt.Errorf("plan store is busy with another writer; no changes were made; retry this command after the other writer finishes: %w", err) + } + return err } // errNoStore says the database has no meta row yet — a file that exists (or @@ -218,7 +228,12 @@ func openDatabase(path string) (*sql.DB, error) { return nil, fmt.Errorf("create plan store directory: %w", err) } } - dsn := "file:" + filepath.ToSlash(path) + "?_txlock=immediate&_pragma=busy_timeout(5000)" + uri, err := sqliteuri.FromPath(path) + if err != nil { + return nil, err + } + uri.RawQuery = "_txlock=immediate&_pragma=busy_timeout(5000)" + dsn := uri.String() db, err := sql.Open("sqlite", dsn) if err != nil { return nil, err @@ -232,9 +247,14 @@ func openDatabase(path string) (*sql.DB, error) { _ = db.Close() return nil, fmt.Errorf("open plan store database: %w", err) } - if err := ensureSchema(db); err != nil { - _ = db.Close() - return nil, err + // Current WAL stores need no writer lock merely to be opened. Checking + // their idempotent schema on a read-only, deferred handle preserves the + // same migration definitions; only a missing/older schema takes the writer. + if err := checkSchemaReadOnly(path); err != nil { + if err := ensureSchema(db); err != nil { + _ = db.Close() + return nil, err + } } return db, nil } @@ -247,7 +267,12 @@ func openDatabase(path string) (*sql.DB, error) { // for it. The write handle is always opened first, so the schema this one // reads already exists. func openReadDatabase(path string) (*sql.DB, error) { - db, err := sql.Open("sqlite", "file:"+filepath.ToSlash(path)+"?_pragma=busy_timeout(5000)") + uri, err := sqliteuri.FromPath(path) + if err != nil { + return nil, err + } + uri.RawQuery = "mode=ro&_pragma=busy_timeout(5000)" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return nil, err } @@ -271,7 +296,8 @@ func openReadDatabase(path string) (*sql.DB, error) { // EVERY STATEMENT IS IDEMPOTENT. Each table arrives IF NOT EXISTS and each // column is added only when the table does not already carry it, so a store // that is already current is rewritten to exactly the schema it holds and the -// commit writes no page — an open of an existing plan makes no write at all. +// commit writes no page. A current schema is checked first on a read-only +// DEFERRED handle, so an ordinary open does not acquire a writer lock. // AND NOTHING IS DROPPED on this road: the schema is created and never torn // down, so the table a reader reaches for is present from the first commit and // stays present after. @@ -283,10 +309,32 @@ func ensureSchema(db *sql.DB) error { if err := enterWAL(db); err != nil { return err } - tx, err := db.Begin() + return schemaTransaction(db) +} + +// checkSchemaReadOnly succeeds only when the existing schema and WAL mode +// already satisfy this build. mode=ro forbids any accidental migration here. +func checkSchemaReadOnly(path string) error { + db, err := openReadDatabase(path) if err != nil { return err } + defer db.Close() + var mode string + if err := db.QueryRow("PRAGMA journal_mode").Scan(&mode); err != nil { + return err + } + if !strings.EqualFold(mode, "wal") { + return errors.New("plan store needs WAL initialization") + } + return schemaTransaction(db) +} + +func schemaTransaction(db *sql.DB) error { + tx, err := db.Begin() + if err != nil { + return writeLockError(err) + } defer tx.Rollback() for _, statement := range schemaStatements { if _, err := tx.Exec(statement); err != nil { diff --git a/internal/plandb/store.go b/internal/plandb/store.go index d2c0acb733..fb66ce1900 100644 --- a/internal/plandb/store.go +++ b/internal/plandb/store.go @@ -102,43 +102,51 @@ func Open(path, project, rootID, rootTitle, rootDescription string, chat ...stri return nil, err } store.db = db - loaded, err := store.loadOrCreate(project, rootID, rootTitle, rootDescription, tag) + rdb, err := openReadDatabase(path) if err != nil { _ = db.Close() - store.db = nil return nil, err } - rdb, err := openReadDatabase(path) + store.rdb = rdb + loaded, err := store.loadOrCreate(project, rootID, rootTitle, rootDescription, tag) if err != nil { + _ = rdb.Close() _ = db.Close() - store.db = nil return nil, err } - store.rdb = rdb store.data = loaded return store, nil } -// loadOrCreate is the open's one transaction: it loads the plan if there is -// one and adopts it, or seeds a new one under the root. TWO PROCESSES CAN -// REACH THE SECOND ROAD AT ONCE — the runtime seeding, a worker's CLI init-ing -// — and BEGIN IMMEDIATE serializes them: the second re-reads and adopts the -// first one's store rather than writing its own over it. Adopting keeps the -// rule the load road states: a store that belongs to another run is a refusal, -// not a merge. +// loadOrCreate adopts a committed plan without taking the writer lock. New +// roots are seeded only under BEGIN IMMEDIATE: concurrent creators re-read +// and adopt the first committed root rather than overwriting it. A plan that +// belongs to a different run is always refused. func (s *Store) loadOrCreate(project, rootID, rootTitle, rootDescription, chat string) (state, error) { + // Adoption is a snapshot read, not a write. If there is no committed root + // yet, re-read under the writer lock below before seeding: another creator + // may have committed while this handle was waiting for that lock. + read, err := s.rdb.Begin() + if err != nil { + return state{}, err + } + loaded, err := loadState(read) + _ = read.Rollback() + if err == nil { + return adoptState(loaded, project, rootID) + } + if !errors.Is(err, errNoStore) { + return state{}, err + } tx, err := s.beginWrite() if err != nil { return state{}, err } defer tx.Rollback() - loaded, err := loadState(tx) + loaded, err = loadState(tx) switch { case err == nil: - if (rootID != "" && loaded.RootID != rootID) || (project != "" && loaded.Project != project) { - return state{}, errors.New("plan store belongs to a different run") - } - return loaded, nil + return adoptState(loaded, project, rootID) case !errors.Is(err, errNoStore): return state{}, err } @@ -167,6 +175,13 @@ func (s *Store) loadOrCreate(project, rootID, rootTitle, rootDescription, chat s return fresh, nil } +func adoptState(loaded state, project, rootID string) (state, error) { + if (rootID != "" && loaded.RootID != rootID) || (project != "" && loaded.Project != project) { + return state{}, errors.New("plan store belongs to a different run") + } + return loaded, nil +} + func (s *Store) Path() string { s.mu.Lock() defer s.mu.Unlock() @@ -2171,15 +2186,21 @@ func (s *Store) beginWrite() (*sql.Tx, error) { if err := s.errIfClosed(); err != nil { return nil, err } - deadline := time.Now().Add(writeLockWait) + return s.beginWriteUntil(time.Now().Add(writeLockWait)) +} + +func (s *Store) beginWriteUntil(deadline time.Time) (*sql.Tx, error) { for { tx, err := s.db.Begin() if err == nil { return tx, nil } - if !isBusy(err) || !time.Now().Before(deadline) { + if !isBusy(err) { return nil, err } + if !time.Now().Before(deadline) { + return nil, writeLockError(err) + } time.Sleep(time.Duration(rand.Intn(20)+1) * time.Millisecond) } } diff --git a/internal/session/plandb_tasks.go b/internal/session/plandb_tasks.go index 801f56d18d..1ed3db1f35 100644 --- a/internal/session/plandb_tasks.go +++ b/internal/session/plandb_tasks.go @@ -22,10 +22,10 @@ import ( "strings" "time" - _ "modernc.org/sqlite" - "github.com/Agent-Field/codeaf/internal/approval" "github.com/Agent-Field/codeaf/internal/plandb" + "github.com/Agent-Field/codeaf/internal/sqliteuri" + _ "modernc.org/sqlite" ) // PlanTaskRow is one row of the chat's plan. It carries what a surface draws @@ -615,7 +615,12 @@ func (a *Agent) PlanSpend(since time.Time) []PlanSpendLine { // digits vary, so a text comparison against a bound would misorder a whole // second against its own fraction. func planSpendBySeat(path, chat string, since time.Time) []PlanSpendLine { - db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + uri, err := sqliteuri.FromPath(path) + if err != nil { + return nil + } + uri.RawQuery = "mode=ro" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return nil } @@ -924,7 +929,12 @@ func planTrajectory(dir, id string) []PlanStep { // beside a writer, and a store that will not open as a reader answers no // figures rather than failing the read. func planSpendByTask(path string) map[string]float64 { - db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + uri, err := sqliteuri.FromPath(path) + if err != nil { + return nil + } + uri.RawQuery = "mode=ro" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return nil } @@ -958,7 +968,12 @@ type planTaskUsage struct { // its own read-only connection for the same reason. A row that names no model // names none, and a task whose rows carry no tokens has none known. func planUsageByTask(path string) map[string]planTaskUsage { - db, err := sql.Open("sqlite", "file:"+path+"?mode=ro") + uri, err := sqliteuri.FromPath(path) + if err != nil { + return nil + } + uri.RawQuery = "mode=ro" + db, err := sql.Open("sqlite", uri.String()) if err != nil { return nil } diff --git a/internal/session/plandb_uri_test.go b/internal/session/plandb_uri_test.go new file mode 100644 index 0000000000..41f2802e08 --- /dev/null +++ b/internal/session/plandb_uri_test.go @@ -0,0 +1,47 @@ +package session + +import ( + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/plandb" +) + +func TestPlanLedgerReadersUseLiteralDatabasePath(t *testing.T) { + name := "plan%23 #&.db" + if runtime.GOOS != "windows" { + name = "plan%23 #&?.db" + } + path := filepath.Join(t.TempDir(), name) + store, err := plandb.Open(path, "mock", "root", "Mock task", "", "mock-chat") + if err != nil { + t.Fatal(err) + } + defer store.Close() + if err = store.AddSpend("root", "mock/model", "work", .25, 11, 7); err != nil { + t.Fatal(err) + } + if got := planSpendByTask(path)["root"]; got != .25 { + t.Fatalf("task spend=%v", got) + } + if got := planUsageByTask(path)["root"]; got.model != "mock/model" || got.tokens != 18 { + t.Fatalf("task usage=%+v", got) + } + got := planSpendBySeat(path, "mock-chat", time.Time{}) + if len(got) != 1 { + t.Fatalf("seat spend=%+v", got) + } +} + +func TestPlanLedgerReadersDoNotCreateMissingDatabase(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing%23.db") + if len(planSpendByTask(path)) != 0 || len(planUsageByTask(path)) != 0 || len(planSpendBySeat(path, "mock-chat", time.Time{})) != 0 { + t.Fatal("missing database returned ledger rows") + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("read created a missing database: %v", err) + } +} diff --git a/internal/sqliteuri/path.go b/internal/sqliteuri/path.go index 3f42d2d3f6..5b34493aba 100644 --- a/internal/sqliteuri/path.go +++ b/internal/sqliteuri/path.go @@ -14,3 +14,13 @@ func File(path string) url.URL { } return url.URL{Scheme: "file", Path: path} } + +// FromPath resolves a filesystem path before encoding it as a SQLite URI. +// Unlike File, it also accepts relative paths without changing their meaning. +func FromPath(path string) (url.URL, error) { + absolute, err := filepath.Abs(path) + if err != nil { + return url.URL{}, err + } + return File(absolute), nil +}