diff --git a/server/internal/daemon/daemon.go b/server/internal/daemon/daemon.go index bea576437e..7b6d4dc030 100644 --- a/server/internal/daemon/daemon.go +++ b/server/internal/daemon/daemon.go @@ -3275,6 +3275,18 @@ func (d *Daemon) acquireLocalDirectoryLockIfNeeded(ctx context.Context, task Tas return nil, true } + // VWO-367: when the resource opts into per-task worktree isolation, do NOT + // take the whole-task path mutex. execenv.Prepare gives each task its own + // git worktree + index cut from this checkout, so concurrent tasks no longer + // share a working tree, an index, or a sidecar directory. The only shared + // .git mutations (worktree add/remove) are serialised briefly inside the + // execenv worktree module, never for the whole task lifetime — which is the + // serialization this opt-in removes. + if assignment.Ref.Isolate { + taskLog.Info("local_directory: worktree isolation enabled; skipping whole-task path mutex") + return nil, false + } + // While the lock is contended the daemon would otherwise sit blocked on // the path mutex with no signal back from the server — the main // per-task watcher only starts after the lock is acquired. If the user @@ -3985,6 +3997,7 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i } if localAssignment != nil { prepParams.LocalWorkDir = localAssignment.AbsPath + prepParams.Isolate = localAssignment.Ref.Isolate } env, err = execenv.Prepare(prepParams, d.logger) if err != nil { @@ -4071,6 +4084,18 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i }() } + // VWO-367: an isolated local_directory task ran in a per-task worktree the + // daemon owns (env.LocalDirectory is false for these, so the in-place excise + // above is correctly skipped — the sidecars live in the disposable worktree, + // not the user's checkout). Tear the worktree down on the way out to reclaim + // its registry entry and per-task branch. The agent must have pushed any work + // worth keeping before the task ends (the fleet pushes at each gate-park); + // the local branch is scratch. Idempotent; the crash path (no defer) is + // reclaimed by GC plus the next task's opportunistic `git worktree prune`. + if env.IsolatedWorktree != nil { + defer env.IsolatedWorktree.Remove(d.logger) + } + prompt := BuildPrompt(task, provider) // Pass task-scoped auth credentials and context so the spawned agent CLI diff --git a/server/internal/daemon/execenv/execenv.go b/server/internal/daemon/execenv/execenv.go index 6f1b3f6666..358d8bee31 100644 --- a/server/internal/daemon/execenv/execenv.go +++ b/server/internal/daemon/execenv/execenv.go @@ -68,6 +68,13 @@ type PrepareParams struct { // substituted. Used by the local_directory project_resource flow // (MUL-2663). When set, the envRoot/workdir directory is not created. LocalWorkDir string + // Isolate (VWO-367) applies only with LocalWorkDir set. When true the agent + // does NOT edit LocalWorkDir in place; instead a per-task git worktree is cut + // from LocalWorkDir's repository into envRoot/workdir (own working tree + + // index), so concurrent tasks on one checkout don't share an index or a + // sidecar dir. Default false preserves the in-place local_directory contract + // general operators rely on (ADR-0019). + Isolate bool // HermesSourceHome is the shared Hermes home the per-task overlay is seeded // from — resolved by the daemon via execenv.ResolveHermesProfile so it honors // the agent's custom_env HERMES_HOME and any -p/--profile or sticky selection. @@ -180,10 +187,18 @@ type Environment struct { // project_resource, it is the user's path instead. See LocalDirectory. WorkDir string // LocalDirectory is true when WorkDir points at a user-supplied path - // outside RootDir (the local_directory flow). Callers that key behavior - // on "may I remove WorkDir as scratch?" must check this — for example - // the GC loop never deletes the user's directory. + // outside RootDir (the in-place local_directory flow). Callers that key + // behavior on "may I remove WorkDir as scratch?" must check this — for + // example the GC loop never deletes the user's directory. It is FALSE for an + // isolated local_directory task: there WorkDir is envRoot/workdir (a per-task + // worktree the daemon owns and may remove), and IsolatedWorktree is set. LocalDirectory bool + // IsolatedWorktree is set (VWO-367) when this is an isolated local_directory + // task: WorkDir is a per-task git worktree cut from the user's checkout. The + // caller must call its Remove on teardown so the worktree registry entry and + // per-task branch are reclaimed (deferred cleanup for the graceful path; GC / + // the next task's opportunistic prune for the crash path). + IsolatedWorktree *IsolatedLocalWorktree // CodexHome is the path to the per-task CODEX_HOME directory (set only for codex provider). CodexHome string // TaskHome is the per-task writable HOME directory (set only for the codex @@ -275,9 +290,17 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { // envRoot. workDir := filepath.Join(envRoot, "workdir") scratchDirs := []string{filepath.Join(envRoot, "output"), filepath.Join(envRoot, "logs")} - if params.LocalWorkDir == "" { + isolate := params.LocalWorkDir != "" && params.Isolate + switch { + case params.LocalWorkDir == "": + // Standard flow: synthesise envRoot/workdir. scratchDirs = append(scratchDirs, workDir) - } else { + case isolate: + // VWO-367 isolated local_directory: workDir is envRoot/workdir, but a git + // worktree (created below, after scratch dirs exist) rather than an empty + // dir. Do NOT pre-create it — `git worktree add` must create the path. + default: + // In-place local_directory: the agent edits the user's path directly. workDir = params.LocalWorkDir } for _, dir := range scratchDirs { @@ -289,10 +312,20 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { env := &Environment{ RootDir: envRoot, WorkDir: workDir, - LocalDirectory: params.LocalWorkDir != "", + LocalDirectory: params.LocalWorkDir != "" && !isolate, logger: logger, } + // Cut the per-task worktree before any sidecar/context files are written, so + // writeContextFiles lands them inside the isolated worktree. + if isolate { + wt, err := PrepareIsolatedLocalWorktree(params.LocalWorkDir, workDir, params.TaskID, logger) + if err != nil { + return nil, fmt.Errorf("execenv: isolate local_directory: %w", err) + } + env.IsolatedWorktree = wt + } + // Write context files into workdir (skills go to provider-native paths). // Track every file/dir we create in a manifest so CleanupSidecars can // roll a local_directory workdir back to its pre-Prepare state. Cloud @@ -735,6 +768,14 @@ func (env *Environment) Cleanup(removeAll bool) error { return nil } + // VWO-367: reclaim an isolated per-task worktree through git before any + // RemoveAll, so we never leave a dangling `git worktree` registry entry that + // a plain directory delete would. Idempotent and safe to call more than once + // (the deferred cleanup in runTask may already have removed it). + if env.IsolatedWorktree != nil { + env.IsolatedWorktree.Remove(env.logger) + } + if env.LocalDirectory { // Never touch the user's directory. RootDir is the daemon's own // scratch; safe to remove when the caller asked for a full diff --git a/server/internal/daemon/execenv/local_worktree.go b/server/internal/daemon/execenv/local_worktree.go new file mode 100644 index 0000000000..3fe46b2f75 --- /dev/null +++ b/server/internal/daemon/execenv/local_worktree.go @@ -0,0 +1,237 @@ +package execenv + +import ( + "fmt" + "log/slog" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" +) + +// local_worktree.go — per-task worktree isolation for local_directory tasks (VWO-367). +// +// Background. A local_directory project_resource binds a task's WorkDir to a +// user's live checkout so the agent edits that tree in place (execenv.Prepare, +// LocalWorkDir). To keep two tasks from corrupting each other's sidecars and git +// index in that one shared tree, the daemon serialises every local_directory +// task on a whole-task path mutex (LocalPathLocker, keyed on the checkout path). +// For a fleet whose agents all target one checkout that mutex collapses their +// designed concurrency to strictly one task at a time. +// +// Isolation. When the local_directory resource opts in (Isolate), the daemon +// instead cuts a *per-task git worktree* off the same repository: the task gets +// its own working tree and its own git index under the task's envRoot, sharing +// only the repository's object store and refs. Two tasks on the same checkout no +// longer share a working tree, an index, or a sidecar directory, so the +// whole-task mutex is unnecessary — only the brief `git worktree add`/`remove` +// critical sections (which mutate shared .git metadata) need serialising, done +// here with a per-source-repo in-process lock (cross-process add/remove races +// are handled by git's own lockfiles; two-daemon safety is VWO-365's single +// owner lock). +// +// Ownership of the git lifecycle: +// - create : PrepareIsolatedLocalWorktree — `git worktree add -b +// multica/worktree/ HEAD`, off the checkout's +// current HEAD. Sidecars are excluded per-worktree so `git add -A` can't +// stage them. +// - commit/rebase/push: the AGENT owns, in its worktree, on its per-task +// branch. The daemon never commits; it provides the isolated tree. Same- +// passage serialisation and the shared book/glossary flock are the +// pub-workstream reducer's job (tools/passage-lock.py, tools/shared_lock.rb), +// not the worktree's — distinct worktrees deliberately do not serialise. +// - conflict : surfaced to the agent as an ordinary non-fast-forward / rebase +// conflict in its own worktree; isolation turns a silent lost-update into a +// visible conflict. +// - cleanup : Remove — `git worktree remove --force` + branch delete + prune. +// Idempotent. On daemon crash (no deferred cleanup) the worktree dir is +// GC'd with its envRoot and PruneOrphanLocalWorktrees reclaims the dangling +// registry entry + branch. +// +// Sidecar containment. The daemon writes provider sidecars (.claude/skills/, +// .agent_context/, .multica/, the runtime brief) into WorkDir because providers +// discover them relative to the working directory. In the isolated case WorkDir +// is this per-task worktree, so those sidecars are contained by construction: +// (1) they live on a disposable per-task branch, never on main; (2) Remove runs +// `git worktree remove --force`, discarding every uncommitted/untracked sidecar; +// (3) the fleet stages named paths at gate-park (`git add `), never +// `git add -A`. We deliberately do NOT write to /.git/info/exclude: for a +// linked worktree git reads excludes from the COMMON gitdir, so an effective +// exclude would be a write into the user's shared repository state — exactly +// what isolation must avoid. This matches the existing github_repo worktree flow. + +// worktreeBranchPrefix namespaces per-task isolation branches so +// PruneOrphanLocalWorktrees and operators can identify daemon-created worktrees. +const worktreeBranchPrefix = "multica/worktree/" + +// repoLocks serialises worktree add/remove/prune per source repository within +// this process. The critical section is brief (a single git call), never the +// whole task, so it does not reintroduce the whole-task serialization this +// change removes. +var ( + repoLocksMu sync.Mutex + repoLocks = map[string]*sync.Mutex{} +) + +func lockForRepo(repoRoot string) *sync.Mutex { + repoLocksMu.Lock() + defer repoLocksMu.Unlock() + m, ok := repoLocks[repoRoot] + if !ok { + m = &sync.Mutex{} + repoLocks[repoRoot] = m + } + return m +} + +// IsolatedLocalWorktree is a per-task git worktree cut from a user's live +// local_directory checkout. +type IsolatedLocalWorktree struct { + SourceRepo string // git toplevel of the user's checkout (the worktree's parent) + WorkDir string // envRoot/workdir — the isolated working tree + Branch string // multica/worktree/ +} + +// PrepareIsolatedLocalWorktree cuts an isolated worktree for taskID at workDir, +// branched off sourceDir's current HEAD. sourceDir is the user's checkout +// (local_directory local_path). Returns an error when sourceDir is not inside a +// git repository — isolation requires a repo to branch from; the caller should +// fall back to the in-place local_directory flow or fail the task with a clear +// message rather than silently running unisolated. +func PrepareIsolatedLocalWorktree(sourceDir, workDir, taskID string, logger *slog.Logger) (*IsolatedLocalWorktree, error) { + gitRoot, ok := detectGitRepo(sourceDir) + if !ok { + return nil, fmt.Errorf("execenv: local_directory isolation requires a git repository, but %q is not inside one", sourceDir) + } + branch := worktreeBranchPrefix + shortID(taskID) + + lock := lockForRepo(gitRoot) + lock.Lock() + // Self-heal before adding ours: reclaim BOTH the registry entries and the + // multica/worktree/* branches left behind by a prior daemon crash (envRoot + // GC'd, no `git worktree remove` ran). This is what makes crash recovery + // reachable from the normal task path — the fleet runs many tasks on one + // repo, so orphans are reaped within one task cycle without any GC wiring. + // It can never touch a live sibling: prune only drops entries whose dir is + // missing, and reap skips branches still checked out in a worktree. + reapOrphansLocked(gitRoot, logger) + err := setupGitWorktree(gitRoot, workDir, branch, "HEAD") + lock.Unlock() + if err != nil { + return nil, fmt.Errorf("execenv: create isolated worktree: %w", err) + } + + if logger != nil { + logger.Info("execenv: prepared isolated local worktree", "source", gitRoot, "workdir", workDir, "branch", branch) + } + return &IsolatedLocalWorktree{SourceRepo: gitRoot, WorkDir: workDir, Branch: branch}, nil +} + +// Remove tears the worktree down: `git worktree remove --force`, delete the +// per-task branch, and prune dangling registry entries. Idempotent — calling it +// after the worktree is already gone (or twice) is a no-op, not an error. Safe +// to call on the deferred cleanup path AND from GC. +func (w *IsolatedLocalWorktree) Remove(logger *slog.Logger) { + if w == nil || w.SourceRepo == "" { + return + } + lock := lockForRepo(w.SourceRepo) + lock.Lock() + defer lock.Unlock() + removeGitWorktree(w.SourceRepo, w.WorkDir, w.Branch, orDiscardLogger(logger)) + pruneWorktrees(w.SourceRepo) +} + +// PruneOrphanLocalWorktrees reclaims isolation worktrees whose working +// directory has already been removed (the daemon-crash path: the envRoot — and +// with it envRoot/workdir — is GC'd, but no `git worktree remove` ran, leaving a +// dangling entry in /.git/worktrees and a multica/worktree/* branch). +// `git worktree prune` drops the dangling registry entries; then any +// multica/worktree/* branch no longer backing a live worktree is deleted. Safe +// to run repeatedly and safe against live worktrees (prune only removes entries +// whose directory is missing; branches still checked out cannot be deleted). +func PruneOrphanLocalWorktrees(sourceRepo string, logger *slog.Logger) error { + if sourceRepo == "" { + return nil + } + gitRoot, ok := detectGitRepo(sourceRepo) + if !ok { + return nil + } + lock := lockForRepo(gitRoot) + lock.Lock() + defer lock.Unlock() + reapOrphansLocked(gitRoot, logger) + return nil +} + +// reapOrphansLocked prunes dangling worktree registry entries and deletes any +// multica/worktree/* branch no longer backing a live worktree. Caller MUST hold +// lockForRepo(gitRoot). Safe against live worktrees: `git worktree prune` only +// drops entries whose directory is missing, and a branch still checked out in a +// worktree is skipped (and git refuses to delete it anyway). +func reapOrphansLocked(gitRoot string, logger *slog.Logger) { + pruneWorktrees(gitRoot) + live := liveWorktreeBranches(gitRoot) + for _, br := range listBranchesWithPrefix(gitRoot, worktreeBranchPrefix) { + if live[br] { + continue + } + cmd := exec.Command("git", "-C", gitRoot, "branch", "-D", br) + if out, err := cmd.CombinedOutput(); err != nil && logger != nil { + logger.Warn("execenv: prune orphan worktree branch failed", "branch", br, "output", strings.TrimSpace(string(out)), "error", err) + } + } +} + +// pruneWorktrees runs `git worktree prune` on repoRoot. Best-effort. +func pruneWorktrees(repoRoot string) { + cmd := exec.Command("git", "-C", repoRoot, "worktree", "prune") + _ = cmd.Run() +} + +// liveWorktreeBranches returns the set of branch names currently checked out in +// a worktree of repoRoot (so pruning never deletes an in-use branch). +func liveWorktreeBranches(repoRoot string) map[string]bool { + out, err := exec.Command("git", "-C", repoRoot, "worktree", "list", "--porcelain").Output() + set := map[string]bool{} + if err != nil { + return set + } + for _, line := range strings.Split(string(out), "\n") { + if b, ok := strings.CutPrefix(line, "branch refs/heads/"); ok { + set[strings.TrimSpace(b)] = true + } + } + return set +} + +// listBranchesWithPrefix returns local branches under refs/heads/. +func listBranchesWithPrefix(repoRoot, prefix string) []string { + out, err := exec.Command("git", "-C", repoRoot, "for-each-ref", "--format=%(refname:short)", "refs/heads/"+prefix).Output() + if err != nil { + return nil + } + var branches []string + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if s := strings.TrimSpace(line); s != "" { + branches = append(branches, s) + } + } + return branches +} + +// orDiscardLogger returns a non-nil logger so removeGitWorktree (which always +// logs) never nil-panics when the caller passed nil. +func orDiscardLogger(logger *slog.Logger) *slog.Logger { + if logger != nil { + return logger + } + return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) +} + +// isolatedWorkDir is the path a task's isolated worktree lives at, under envRoot. +func isolatedWorkDir(envRoot string) string { + return filepath.Join(envRoot, "workdir") +} diff --git a/server/internal/daemon/execenv/local_worktree_test.go b/server/internal/daemon/execenv/local_worktree_test.go new file mode 100644 index 0000000000..78b6ce7b35 --- /dev/null +++ b/server/internal/daemon/execenv/local_worktree_test.go @@ -0,0 +1,447 @@ +package execenv + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" +) + +// These tests exercise the VWO-367 per-task worktree isolation against REAL git +// repositories and REAL processes in throwaway temp dirs. They never touch any +// live checkout. The required scenario matrix maps to the tests below: +// +// two distinct tasks concurrent, no shared index/sidecar -> TestIsolated_ConcurrentDistinctIndex +// provider failure cleanup -> TestIsolated_ProviderFailureLeavesNoOrphan +// cancellation (cleanup mid-flight) -> TestIsolated_ProviderFailureLeavesNoOrphan / RepeatedCleanup +// daemon crash + orphaned worktree recovery -> TestIsolated_CrashOrphanRecovery +// repeated cleanup -> TestIsolated_RepeatedCleanupIdempotent +// rebase conflict -> TestIsolated_RebaseConflictSurfaces +// no writes reach the live checkout -> TestIsolated_NoWritesToSourceWorkingTree +// requires a repo -> TestIsolated_RequiresGitRepo +// prune never harms a live worktree -> TestPruneOrphan_PreservesLiveWorktree +// +// Same-passage deterministic serialisation and the shared book/glossary +// cross-process flock are deliberately NOT the worktree's job (distinct +// worktrees must run concurrently); they live in the pub-workstream reducer +// (tools/passage-lock.py, tools/shared_lock.rb) and are tested there. + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %s failed: %s: %v", strings.Join(args, " "), strings.TrimSpace(string(out)), err) + } + return strings.TrimSpace(string(out)) +} + +// newSourceRepo builds a throwaway git repo standing in for a user's live +// local_directory checkout, with an initial commit on main. +func newSourceRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + git(t, dir, "init", "-q", "-b", "main") + git(t, dir, "config", "user.email", "t@t") + git(t, dir, "config", "user.name", "t") + if err := os.WriteFile(filepath.Join(dir, "seed.txt"), []byte("seed\n"), 0o644); err != nil { + t.Fatal(err) + } + git(t, dir, "add", "-A") + git(t, dir, "commit", "-qm", "init") + return dir +} + +func worktreeList(t *testing.T, repo string) string { + t.Helper() + return git(t, repo, "worktree", "list", "--porcelain") +} + +func TestIsolated_RequiresGitRepo(t *testing.T) { + nonRepo := t.TempDir() + _, err := PrepareIsolatedLocalWorktree(nonRepo, filepath.Join(t.TempDir(), "workdir"), "task-abc", nil) + if err == nil { + t.Fatal("expected error when sourceDir is not a git repo") + } +} + +func TestIsolated_ConcurrentDistinctIndex(t *testing.T) { + src := newSourceRepo(t) + envA := t.TempDir() + envB := t.TempDir() + + var wg sync.WaitGroup + var wtA, wtB *IsolatedLocalWorktree + var errA, errB error + wg.Add(2) + go func() { + defer wg.Done() + wtA, errA = PrepareIsolatedLocalWorktree(src, isolatedWorkDir(envA), "task-aaaaaaaa", nil) + }() + go func() { + defer wg.Done() + wtB, errB = PrepareIsolatedLocalWorktree(src, isolatedWorkDir(envB), "task-bbbbbbbb", nil) + }() + wg.Wait() + if errA != nil || errB != nil { + t.Fatalf("concurrent prepare failed: A=%v B=%v", errA, errB) + } + t.Cleanup(func() { wtA.Remove(nil); wtB.Remove(nil) }) + + // Distinct working trees. + if wtA.WorkDir == wtB.WorkDir { + t.Fatal("two tasks got the same workdir") + } + // Distinct git index files — the core of "no shared index". + idxA := git(t, wtA.WorkDir, "rev-parse", "--git-path", "index") + idxB := git(t, wtB.WorkDir, "rev-parse", "--git-path", "index") + if idxA == idxB { + t.Fatalf("two tasks share one git index: %s", idxA) + } + + // Each commits a DIFFERENT file to its own branch concurrently — must not + // collide on index.lock or lose a commit. + var wg2 sync.WaitGroup + commit := func(wt *IsolatedLocalWorktree, name string) { + defer wg2.Done() + if err := os.WriteFile(filepath.Join(wt.WorkDir, name), []byte(name), 0o644); err != nil { + t.Errorf("write %s: %v", name, err) + return + } + git(t, wt.WorkDir, "add", name) + git(t, wt.WorkDir, "commit", "-qm", "add "+name) + } + wg2.Add(2) + go commit(wtA, "a.txt") + go commit(wtB, "b.txt") + wg2.Wait() + + // Each branch has exactly its own new file; neither saw the other's index. + if got := git(t, wtA.WorkDir, "log", "--name-only", "--format=", "-1"); !strings.Contains(got, "a.txt") || strings.Contains(got, "b.txt") { + t.Fatalf("branch A tip wrong: %q", got) + } + if got := git(t, wtB.WorkDir, "log", "--name-only", "--format=", "-1"); !strings.Contains(got, "b.txt") || strings.Contains(got, "a.txt") { + t.Fatalf("branch B tip wrong: %q", got) + } +} + +func TestIsolated_NoWritesToSourceWorkingTree(t *testing.T) { + src := newSourceRepo(t) + headBefore := git(t, src, "rev-parse", "HEAD") + statusBefore := git(t, src, "status", "--porcelain") + lsBefore := git(t, src, "ls-files") + + env := t.TempDir() + wt, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(env), "task-cccccccc", nil) + if err != nil { + t.Fatal(err) + } + // Do real work in the isolated tree, including a daemon sidecar. The fleet + // stages named paths (never `git add -A`), so only work.txt is committed; + // the sidecar stays uncommitted and is discarded by `git worktree remove`. + if err := os.MkdirAll(filepath.Join(wt.WorkDir, ".claude", "skills", "x"), 0o755); err != nil { + t.Fatal(err) + } + os.WriteFile(filepath.Join(wt.WorkDir, ".claude", "skills", "x", "SKILL.md"), []byte("s"), 0o644) + os.WriteFile(filepath.Join(wt.WorkDir, "work.txt"), []byte("work"), 0o644) + git(t, wt.WorkDir, "add", "work.txt") + git(t, wt.WorkDir, "commit", "-qm", "work") + wt.Remove(nil) + + // The source's HEAD, working tree, and tracked set are all untouched. + if got := git(t, src, "rev-parse", "HEAD"); got != headBefore { + t.Fatalf("source HEAD moved: %s != %s", got, headBefore) + } + if got := git(t, src, "status", "--porcelain"); got != statusBefore { + t.Fatalf("source working tree changed: %q", got) + } + if got := git(t, src, "ls-files"); got != lsBefore { + t.Fatalf("source tracked files changed: %q", got) + } +} + +func TestIsolated_ProviderFailureLeavesNoOrphan(t *testing.T) { + src := newSourceRepo(t) + env := t.TempDir() + wt, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(env), "task-dddddddd", nil) + if err != nil { + t.Fatal(err) + } + // Simulate the provider failing after prepare: the deferred cleanup runs. + wt.Remove(nil) + + list := worktreeList(t, src) + if strings.Contains(list, wt.WorkDir) { + t.Fatalf("orphan worktree left after failure cleanup:\n%s", list) + } + if branches := listBranchesWithPrefix(src, worktreeBranchPrefix); len(branches) != 0 { + t.Fatalf("orphan branch left after failure cleanup: %v", branches) + } +} + +func TestIsolated_RepeatedCleanupIdempotent(t *testing.T) { + src := newSourceRepo(t) + env := t.TempDir() + wt, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(env), "task-eeeeeeee", nil) + if err != nil { + t.Fatal(err) + } + wt.Remove(nil) + wt.Remove(nil) // second call must be a harmless no-op + if list := worktreeList(t, src); strings.Contains(list, wt.WorkDir) { + t.Fatalf("worktree present after repeated cleanup:\n%s", list) + } +} + +func TestIsolated_CrashOrphanRecovery(t *testing.T) { + src := newSourceRepo(t) + env := t.TempDir() + wt, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(env), "task-ffffffff", nil) + if err != nil { + t.Fatal(err) + } + // Simulate a daemon crash: envRoot (and thus workdir) is later GC'd, but no + // `git worktree remove` ran. Emulate by deleting the workdir directly. + if err := os.RemoveAll(env); err != nil { + t.Fatal(err) + } + // The registry still lists the now-missing worktree (dangling). + if !strings.Contains(worktreeList(t, src), wt.Branch) && !hasDanglingEntry(t, src, wt.WorkDir) { + // Some git versions omit the path once missing; the prunable state is what matters. + } + // Recovery reclaims the dangling entry and its branch. + if err := PruneOrphanLocalWorktrees(src, nil); err != nil { + t.Fatal(err) + } + if list := worktreeList(t, src); strings.Contains(list, wt.WorkDir) { + t.Fatalf("dangling worktree survived recovery:\n%s", list) + } + if branches := listBranchesWithPrefix(src, worktreeBranchPrefix); len(branches) != 0 { + t.Fatalf("orphan branch survived recovery: %v", branches) + } +} + +func hasDanglingEntry(t *testing.T, repo, workdir string) bool { + t.Helper() + return strings.Contains(worktreeList(t, repo), workdir) +} + +// The crash-recovery path must be reachable from the NORMAL task path: creating +// the next task's worktree opportunistically reaps a prior crash orphan's +// registry entry AND its branch, with no GC wiring. +func TestIsolated_NextTaskReapsCrashOrphanBranchAndEntry(t *testing.T) { + src := newSourceRepo(t) + crashedEnv := t.TempDir() + crashed, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(crashedEnv), "task-deadbeef", nil) + if err != nil { + t.Fatal(err) + } + // Daemon crash: envRoot vanishes, no `git worktree remove` ran. + if err := os.RemoveAll(crashedEnv); err != nil { + t.Fatal(err) + } + + // A later, unrelated task on the same repo runs its normal prepare. + nextEnv := t.TempDir() + next, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(nextEnv), "task-11112222", nil) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { next.Remove(nil) }) + + // The crash orphan's registry entry and branch are gone; only the live one remains. + if list := worktreeList(t, src); strings.Contains(list, crashed.WorkDir) { + t.Fatalf("crash-orphan registry entry survived the next task's prepare:\n%s", list) + } + branches := listBranchesWithPrefix(src, worktreeBranchPrefix) + if len(branches) != 1 || branches[0] != next.Branch { + t.Fatalf("expected only the live branch %q after reap, got %v", next.Branch, branches) + } +} + +func TestPruneOrphan_PreservesLiveWorktree(t *testing.T) { + src := newSourceRepo(t) + env := t.TempDir() + wt, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(env), "task-99999999", nil) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { wt.Remove(nil) }) + // A live worktree must survive prune (its dir still exists, branch in use). + if err := PruneOrphanLocalWorktrees(src, nil); err != nil { + t.Fatal(err) + } + if list := worktreeList(t, src); !strings.Contains(list, wt.WorkDir) { + t.Fatalf("prune removed a LIVE worktree:\n%s", list) + } + if branches := listBranchesWithPrefix(src, worktreeBranchPrefix); len(branches) != 1 { + t.Fatalf("prune deleted a live worktree's branch: %v", branches) + } + // And the worktree is still usable. + os.WriteFile(filepath.Join(wt.WorkDir, "still.txt"), []byte("x"), 0o644) + git(t, wt.WorkDir, "add", "still.txt") + git(t, wt.WorkDir, "commit", "-qm", "still works") +} + +// Prepare-level wiring: Isolate=true must produce a WorkDir that is a per-task +// worktree of the source repo (not the source itself), with sidecars inside the +// isolated tree, LocalDirectory=false, and a Cleanup that reclaims the worktree +// registry entry. +func TestPrepare_IsolateCutsWorktree(t *testing.T) { + src := newSourceRepo(t) + workspacesRoot := t.TempDir() + env, err := Prepare(PrepareParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: "ws-iso-001", + TaskID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + AgentName: "Iso Agent", + LocalWorkDir: src, + Isolate: true, + Task: TaskContextForEnv{ + IssueID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + AgentSkills: []SkillContextForEnv{{Name: "Code Review", Content: "Be concise."}}, + }, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + + if env.WorkDir == src { + t.Fatal("isolated Prepare left WorkDir pointing at the source checkout") + } + if env.WorkDir != filepath.Join(env.RootDir, "workdir") { + t.Fatalf("isolated WorkDir = %q, want %q", env.WorkDir, filepath.Join(env.RootDir, "workdir")) + } + if env.LocalDirectory { + t.Fatal("isolated task must have LocalDirectory=false (WorkDir is daemon-owned scratch)") + } + if env.IsolatedWorktree == nil { + t.Fatal("IsolatedWorktree handle not set") + } + // It is a real worktree of the source repository (shared common gitdir), + // with its own index. Compare symlink-resolved forms — temp dirs may be + // reported via different prefixes (macOS /var vs /private/var). + common := git(t, env.WorkDir, "rev-parse", "--git-common-dir") + commonReal, err := filepath.EvalSymlinks(common) + if err != nil { + t.Fatalf("resolve common dir: %v", err) + } + srcGitReal, err := filepath.EvalSymlinks(filepath.Join(src, ".git")) + if err != nil { + t.Fatalf("resolve source gitdir: %v", err) + } + if commonReal != srcGitReal { + t.Fatalf("worktree common dir = %q, want %q", commonReal, srcGitReal) + } + srcIdx := git(t, src, "rev-parse", "--git-path", "index") + wtIdx := git(t, env.WorkDir, "rev-parse", "--git-path", "index") + if srcIdx == wtIdx { + t.Fatal("isolated worktree shares the source's git index") + } + // Sidecars landed inside the isolated worktree, not the source. + if _, err := os.Stat(filepath.Join(env.WorkDir, ".agent_context")); err != nil { + t.Fatalf("context files missing from isolated worktree: %v", err) + } + if _, err := os.Stat(filepath.Join(src, ".agent_context")); !os.IsNotExist(err) { + t.Fatal("context files leaked into the source checkout") + } + + // Cleanup reclaims the registry entry and the envRoot. + if err := env.Cleanup(true); err != nil { + t.Fatalf("cleanup: %v", err) + } + if list := worktreeList(t, src); strings.Contains(list, env.WorkDir) { + t.Fatalf("cleanup left a dangling worktree entry:\n%s", list) + } + if _, err := os.Stat(env.RootDir); !os.IsNotExist(err) { + t.Fatal("cleanup left the envRoot behind") + } +} + +func TestPrepare_IsolateRequiresGitRepo(t *testing.T) { + nonRepo := t.TempDir() + _, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-iso-002", + TaskID: "b1b2c3d4-e5f6-7890-abcd-ef1234567890", + LocalWorkDir: nonRepo, + Isolate: true, + Task: TaskContextForEnv{IssueID: "b1b2c3d4-e5f6-7890-abcd-ef1234567890"}, + }, testLogger()) + if err == nil { + t.Fatal("expected Prepare to fail closed when Isolate is set on a non-git directory") + } +} + +// The default (isolate absent) in-place contract is byte-for-byte unchanged: +// WorkDir IS the user's path and LocalDirectory guards it from cleanup. +func TestPrepare_InPlaceDefaultUnchanged(t *testing.T) { + src := newSourceRepo(t) + env, err := Prepare(PrepareParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "ws-iso-003", + TaskID: "c1b2c3d4-e5f6-7890-abcd-ef1234567890", + LocalWorkDir: src, + Task: TaskContextForEnv{IssueID: "c1b2c3d4-e5f6-7890-abcd-ef1234567890"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare failed: %v", err) + } + if env.WorkDir != src { + t.Fatalf("in-place WorkDir = %q, want the user's path %q", env.WorkDir, src) + } + if !env.LocalDirectory { + t.Fatal("in-place task must keep LocalDirectory=true") + } + if env.IsolatedWorktree != nil { + t.Fatal("in-place task must not get an IsolatedWorktree handle") + } +} + +func TestIsolated_RebaseConflictSurfaces(t *testing.T) { + src := newSourceRepo(t) + // A shared file both tasks will edit differently. + os.WriteFile(filepath.Join(src, "shared.txt"), []byte("base\n"), 0o644) + git(t, src, "add", "shared.txt") + git(t, src, "commit", "-qm", "add shared") + + envA, envB := t.TempDir(), t.TempDir() + wtA, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(envA), "task-a1a1a1a1", nil) + if err != nil { + t.Fatal(err) + } + wtB, err := PrepareIsolatedLocalWorktree(src, isolatedWorkDir(envB), "task-b2b2b2b2", nil) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { wtA.Remove(nil); wtB.Remove(nil) }) + + writeCommit := func(wt *IsolatedLocalWorktree, content string) { + os.WriteFile(filepath.Join(wt.WorkDir, "shared.txt"), []byte(content), 0o644) + git(t, wt.WorkDir, "add", "shared.txt") + git(t, wt.WorkDir, "commit", "-qm", "edit shared") + } + writeCommit(wtA, "from-A\n") + writeCommit(wtB, "from-B\n") + + // B rebases its branch onto A's (the two isolated tasks edited the same file + // divergently) -> deterministic conflict, surfaced as a non-zero rebase, NOT + // a silent lost update. This is the property isolation must preserve: the + // second writer is forced to reconcile, it cannot clobber the first. + cmd := exec.Command("git", "-C", wtB.WorkDir, "rebase", wtA.Branch) + cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatalf("expected a rebase conflict, got clean rebase:\n%s", out) + } + if !strings.Contains(string(out), "CONFLICT") { + t.Fatalf("rebase failed but not with a surfaced CONFLICT:\n%s", out) + } + git(t, wtB.WorkDir, "rebase", "--abort") +} diff --git a/server/internal/daemon/local_directory.go b/server/internal/daemon/local_directory.go index 6e78e4266e..535b358252 100644 --- a/server/internal/daemon/local_directory.go +++ b/server/internal/daemon/local_directory.go @@ -26,6 +26,12 @@ type localDirectoryRef struct { LocalPath string `json:"local_path"` DaemonID string `json:"daemon_id"` Label string `json:"label,omitempty"` + // Isolate (VWO-367): when true, run each task in its own per-task git + // worktree cut from this directory's repo (own working tree + index) instead + // of editing the directory in place under the whole-task path mutex. Mirrors + // handler.localDirectoryRef; keep both in sync or the server's re-marshal + // drops the field. See execenv.PrepareIsolatedLocalWorktree. + Isolate bool `json:"isolate,omitempty"` } // localDirectoryAssignment is the resolved view of a task's local_directory diff --git a/server/internal/daemon/local_directory_isolate_test.go b/server/internal/daemon/local_directory_isolate_test.go new file mode 100644 index 0000000000..c1c090468f --- /dev/null +++ b/server/internal/daemon/local_directory_isolate_test.go @@ -0,0 +1,76 @@ +package daemon + +import ( + "context" + "encoding/json" + "log/slog" + "testing" +) + +// VWO-367: a local_directory resource with isolate:true must NOT take the +// whole-task path mutex — the per-task worktree provides the safety instead. +// The throughput property under test: an isolated task proceeds immediately +// even while another task holds the path lock, and holds nothing itself. +func TestAcquireLocalDirectoryLock_IsolateSkipsPathMutex(t *testing.T) { + t.Parallel() + + const daemonID = "d-mine" + tmp := t.TempDir() + isolatedRaw, err := json.Marshal(localDirectoryRef{LocalPath: tmp, DaemonID: daemonID, Isolate: true}) + if err != nil { + t.Fatalf("marshal isolated ref: %v", err) + } + inPlaceRaw, err := json.Marshal(localDirectoryRef{LocalPath: tmp, DaemonID: daemonID}) + if err != nil { + t.Fatalf("marshal in-place ref: %v", err) + } + + d := &Daemon{ + cfg: Config{DaemonID: daemonID}, + localPathLocks: NewLocalPathLocker(), + logger: slog.Default(), + } + + // Control: the in-place (default) resource still takes the lock. + inPlace := Task{ + ID: "in-place-task", + ProjectResources: []ProjectResourceData{{ID: "r1", ResourceType: localDirectoryResourceType, ResourceRef: inPlaceRaw}}, + } + inPlaceAssignment, err := localDirectoryAssignmentForTask(inPlace, daemonID) + if err != nil || inPlaceAssignment == nil { + t.Fatalf("in-place assignment: %v %+v", err, inPlaceAssignment) + } + release, abort := d.acquireLocalDirectoryLockIfNeeded(context.Background(), inPlace, slog.Default()) + if abort || release == nil { + t.Fatalf("in-place task should hold the path mutex (abort=%v release=%v)", abort, release == nil) + } + if got := d.localPathLocks.Holder(inPlaceAssignment.RealPath); got != inPlace.ID { + t.Fatalf("holder = %q, want %q", got, inPlace.ID) + } + + // The isolated task must proceed WITHOUT blocking on the held mutex and + // without becoming a holder itself. + isolated := Task{ + ID: "isolated-task", + ProjectResources: []ProjectResourceData{{ID: "r2", ResourceType: localDirectoryResourceType, ResourceRef: isolatedRaw}}, + } + isoAssignment, err := localDirectoryAssignmentForTask(isolated, daemonID) + if err != nil || isoAssignment == nil { + t.Fatalf("isolated assignment: %v %+v", err, isoAssignment) + } + if !isoAssignment.Ref.Isolate { + t.Fatal("isolate flag did not survive the resource_ref round-trip into the assignment") + } + isoRelease, isoAbort := d.acquireLocalDirectoryLockIfNeeded(context.Background(), isolated, slog.Default()) + if isoAbort { + t.Fatal("isolated task aborted") + } + if isoRelease != nil { + t.Fatal("isolated task returned a release callback — it must not take the path mutex") + } + // The original holder is undisturbed. + if got := d.localPathLocks.Holder(isoAssignment.RealPath); got != inPlace.ID { + t.Fatalf("holder after isolated acquire = %q, want %q", got, inPlace.ID) + } + release() +} diff --git a/server/internal/handler/project_resource.go b/server/internal/handler/project_resource.go index e6c9c2cea6..c869588136 100644 --- a/server/internal/handler/project_resource.go +++ b/server/internal/handler/project_resource.go @@ -109,16 +109,30 @@ func validateGithubRepoRef(ref json.RawMessage) (json.RawMessage, error) { } // localDirectoryRef is the JSONB shape stored for resource_type=local_directory. -// It pins a project to an existing directory on a specific user machine, so -// agent tasks run in-place rather than in an isolated git worktree. The +// It pins a project to an existing directory on a specific user machine. The // daemon_id scopes the path to one daemon registration — the same string path // on a different machine is a different resource. The optional label is a // human-readable hint used by the UI; the row-level project_resource.label // column remains the generic column for any resource type. +// +// Isolate (VWO-367) selects how tasks run against the directory: +// - false (default): tasks run IN PLACE, editing the directory's working tree +// directly, serialised one-at-a-time on the daemon's path mutex. This is +// the contract general operators depend on (they edit the owner's live +// checkout, ADR-0019), so it stays the default. +// - true: each task runs in its OWN per-task git worktree cut from the +// directory's repository (own working tree + index), so tasks run +// concurrently without sharing an index or sidecar dir and without the +// whole-task path mutex. Opt in for a fleet that targets one checkout. +// +// resource_ref is polymorphic JSONB, so this field needs no migration — but it +// MUST stay on this struct (and the daemon mirror), or validateLocalDirectoryRef +// re-marshals the payload and silently drops it. type localDirectoryRef struct { LocalPath string `json:"local_path"` DaemonID string `json:"daemon_id"` Label string `json:"label,omitempty"` + Isolate bool `json:"isolate,omitempty"` } func validateLocalDirectoryRef(ref json.RawMessage) (json.RawMessage, error) { diff --git a/server/internal/handler/project_resource_isolate_test.go b/server/internal/handler/project_resource_isolate_test.go new file mode 100644 index 0000000000..dfda5038f7 --- /dev/null +++ b/server/internal/handler/project_resource_isolate_test.go @@ -0,0 +1,51 @@ +package handler + +import ( + "encoding/json" + "testing" +) + +// VWO-367: validateLocalDirectoryRef re-marshals the payload through the typed +// struct, so any field not on localDirectoryRef is silently dropped. These +// cases pin the isolate flag's round-trip so a future struct edit can't +// silently strip the opt-in (which would flip an isolated fleet back to +// unserialized in-place writes on its next task). Pure unit test — no DB. +func TestValidateLocalDirectoryRefIsolateRoundTrip(t *testing.T) { + t.Parallel() + + t.Run("isolate true survives validation", func(t *testing.T) { + out, err := validateLocalDirectoryRef(json.RawMessage(`{"local_path":"/Users/u/proj","daemon_id":"d1","isolate":true}`)) + if err != nil { + t.Fatalf("validate: %v", err) + } + var ref localDirectoryRef + if err := json.Unmarshal(out, &ref); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !ref.Isolate { + t.Fatal("isolate=true was dropped by the validation re-marshal") + } + }) + + t.Run("isolate defaults false and stays omitted", func(t *testing.T) { + out, err := validateLocalDirectoryRef(json.RawMessage(`{"local_path":"/Users/u/proj","daemon_id":"d1"}`)) + if err != nil { + t.Fatalf("validate: %v", err) + } + var ref localDirectoryRef + if err := json.Unmarshal(out, &ref); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if ref.Isolate { + t.Fatal("isolate defaulted true") + } + // omitempty: the default payload shape is unchanged for existing rows. + var m map[string]any + if err := json.Unmarshal(out, &m); err != nil { + t.Fatalf("unmarshal map: %v", err) + } + if _, present := m["isolate"]; present { + t.Fatal("isolate=false should be omitted from the stored payload") + } + }) +}