From cd169947b5e79d382d786161844485be80940e1b Mon Sep 17 00:00:00 2001 From: iiwish Date: Wed, 26 Aug 2026 16:34:10 +0800 Subject: [PATCH] MUL-6686 feat(daemon): use readable workdir paths (#7316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Create new task environments under sanitized, bounded readable segments: -/-/workdir. The readable name is the visible part; the substance is moving task identity off the directory name. A human-readable segment is mutable — workspace slugs and issue prefixes change — so workspace/task identity now lives in .task_owner, .gc_meta.json and a stable ID-keyed index under .task_roots that freezes one physical root per task for its lifetime. GC and disk-usage read that authoritative identity instead of inferring a workspace from a path string, which also fixes active and preparation-crashed roots being attributed to a nonexistent workspace. Existing workdirs are not renamed or migrated, legacy roots are adopted into the index, and both layouts render privacy-safe relative paths. Labels are lowercased ASCII with separators and traversal collapsed, segments are capped at 24 characters so the two together stay within the previous 49-character Windows path budget, and stable ID suffixes drawn from the UUIDv7 random tail keep roots distinct across renames and case-insensitive filesystems. Closes #6063 Co-authored-by: iiwish Co-authored-by: J --- server/internal/daemon/daemon.go | 60 +- server/internal/daemon/diskusage.go | 31 +- server/internal/daemon/diskusage_test.go | 95 ++++ server/internal/daemon/execenv/execenv.go | 232 ++++++-- .../internal/daemon/execenv/execenv_test.go | 518 +++++++++++++++++- .../internal/daemon/execenv/isolation_test.go | 20 +- .../daemon/execenv/isolation_unix_test.go | 4 +- .../daemon/execenv/isolation_windows_test.go | 15 +- .../internal/daemon/execenv/root_identity.go | 365 ++++++++++++ .../daemon/execenv/root_identity_test.go | 276 ++++++++++ server/internal/daemon/gc.go | 63 ++- server/internal/daemon/gc_test.go | 280 ++++++++++ .../daemon/leader_workdir_reuse_test.go | 6 + .../daemon/repocache/cache_windows_test.go | 41 +- server/internal/daemon/types.go | 2 + server/internal/daemon/workdir_race_test.go | 8 +- server/internal/handler/agent.go | 53 +- .../internal/handler/agent_work_dir_test.go | 52 +- server/internal/handler/daemon.go | 6 + server/internal/handler/daemon_test.go | 12 + 20 files changed, 1997 insertions(+), 142 deletions(-) create mode 100644 server/internal/daemon/execenv/root_identity.go create mode 100644 server/internal/daemon/execenv/root_identity_test.go diff --git a/server/internal/daemon/daemon.go b/server/internal/daemon/daemon.go index fbb6b4480..0eabe4147 100644 --- a/server/internal/daemon/daemon.go +++ b/server/internal/daemon/daemon.go @@ -5071,13 +5071,16 @@ func (d *Daemon) handleTask(ctx context.Context, task Task, slot int) { // reportTaskResult and execenv.WriteGCMeta below. markActiveEnvRoot // is reference-counted, so the duplicate marks runTask installs are // correctly nested within these. - predictedEnvRoot := execenv.PredictRootDir(d.cfg.WorkspacesRoot, task.WorkspaceID, task.ID) - if predictedEnvRoot != "" { - d.markActiveEnvRoot(predictedEnvRoot) - defer d.unmarkActiveEnvRoot(predictedEnvRoot) + resolvedEnvRoot, resolveRootErr := execenv.ResolveRootDir(taskRootDirParams(d.cfg.WorkspacesRoot, task)) + if resolveRootErr != nil { + taskLog.Error("resolve stable task env root", "error", resolveRootErr) + } + if resolvedEnvRoot != "" { + d.markActiveEnvRoot(resolvedEnvRoot) + defer d.unmarkActiveEnvRoot(resolvedEnvRoot) } if task.PriorWorkDir != "" { - if priorRoot := filepath.Dir(task.PriorWorkDir); priorRoot != "" && priorRoot != predictedEnvRoot { + if priorRoot := filepath.Dir(task.PriorWorkDir); priorRoot != "" && priorRoot != resolvedEnvRoot { d.markActiveEnvRoot(priorRoot) defer d.unmarkActiveEnvRoot(priorRoot) } @@ -5602,7 +5605,7 @@ func (d *Daemon) reportTerminalTask(parentCtx context.Context, report terminalTa // internal task with no IDs at all). The caller skips writing a meta file // in that case so the directory falls back to mtime-based orphan cleanup. func gcMetaForTask(task Task) (execenv.GCMeta, bool) { - meta := execenv.GCMeta{WorkspaceID: task.WorkspaceID} + meta := execenv.GCMeta{WorkspaceID: task.WorkspaceID, TaskID: task.ID} switch { case task.ChatSessionID != "": meta.Kind = execenv.GCKindChat @@ -5626,6 +5629,16 @@ func gcMetaForTask(task Task) (execenv.GCMeta, bool) { return meta, true } +func taskRootDirParams(workspacesRoot string, task Task) execenv.RootDirParams { + return execenv.RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: task.WorkspaceID, + WorkspaceSlug: task.WorkspaceSlug, + TaskID: task.ID, + IssueIdentifier: task.IssueIdentifier, + } +} + // runtimeDisplayNameOverrides maps a provider key to the human-facing runtime // name when simple title-casing would read awkwardly. Providers not listed // here fall back to capitalizing the key (claude → "Claude", codex → "Codex"). @@ -5792,7 +5805,7 @@ func sessionHomeReachable(provider string, env *execenv.Environment, envReused b // shouldReusePriorWorkdir keeps the local_directory lock and cross-agent // isolation invariants without forcing managed follow-ups onto a fresh // provider session. Every managed issue or chat task may reuse only directories -// that resolve to the {workspace}/{task}/workdir shape, carry Prepare-time +// that resolve to the two-segment managed root shape, carry Prepare-time // managed-env provenance for the same workspace/scope/agent, and carry a // matching daemon task-context marker. Other task kinds have no durable scope // with which to prove ownership and therefore start fresh. @@ -5827,7 +5840,7 @@ func shouldReusePriorWorkdir(task Task, localAssignment *localDirectoryAssignmen return "", false } parts := strings.Split(rel, string(filepath.Separator)) - if len(parts) != 3 || parts[0] != task.WorkspaceID || parts[1] == "" || parts[2] != "workdir" { + if len(parts) != 3 || parts[0] == "" || parts[1] == "" || parts[2] != "workdir" { return "", false } if task.AgentID == "" || (task.IssueID == "" && task.ChatSessionID == "") { @@ -6720,14 +6733,17 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i // Mark candidate env roots as active before any env work so the GC loop // can't reclaim artifacts inside them mid-execution. We mark both the - // predicted root for a fresh Prepare and the prior root for Reuse — they + // stable root for a fresh Prepare and the prior root for Reuse — they // usually differ (Reuse keeps the original task's directory). - predictedRoot := execenv.PredictRootDir(d.cfg.WorkspacesRoot, task.WorkspaceID, task.ID) - d.markActiveEnvRoot(predictedRoot) - defer d.unmarkActiveEnvRoot(predictedRoot) + resolvedRoot, err := execenv.ResolveRootDir(taskRootDirParams(d.cfg.WorkspacesRoot, task)) + if err != nil { + return TaskResult{}, fmt.Errorf("resolve stable task env root: %w", err) + } + d.markActiveEnvRoot(resolvedRoot) + defer d.unmarkActiveEnvRoot(resolvedRoot) if task.PriorWorkDir != "" { priorRoot := filepath.Dir(task.PriorWorkDir) - if priorRoot != predictedRoot { + if priorRoot != resolvedRoot { d.markActiveEnvRoot(priorRoot) defer d.unmarkActiveEnvRoot(priorRoot) } @@ -6742,7 +6758,7 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i // *os.File cannot cross its JSON response back to us. Claiming there would // leave the agent running with no protection at all — which is exactly the // re-dispatch window this guards. - envClaim, err := execenv.ClaimEnvRoot(d.cfg.WorkspacesRoot, task.WorkspaceID, task.ID) + envClaim, err := execenv.ClaimEnvRoot(taskRootDirParams(d.cfg.WorkspacesRoot, task)) if err != nil { return TaskResult{}, fmt.Errorf("claim execution environment: %w", err) } @@ -7025,11 +7041,13 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i if env == nil { var err error prepParams := execenv.PrepareParams{ - WorkspacesRoot: d.cfg.WorkspacesRoot, - Profile: d.cfg.Profile, - WorkspaceID: task.WorkspaceID, - TaskID: task.ID, - AgentName: agentName, + WorkspacesRoot: d.cfg.WorkspacesRoot, + Profile: d.cfg.Profile, + WorkspaceID: task.WorkspaceID, + WorkspaceSlug: task.WorkspaceSlug, + TaskID: task.ID, + IssueIdentifier: task.IssueIdentifier, + AgentName: agentName, // This run already holds the claim (envClaim above) and the reset // it implies; preparation must not try to take it again. EnvRootPreclaimed: true, @@ -7126,8 +7144,8 @@ func (d *Daemon) runTask(ctx context.Context, task Task, provider string, slot i } } // Belt-and-suspenders: also mark whatever root we ended up with, in case - // future changes diverge from PredictRootDir. - if env.RootDir != predictedRoot && env.RootDir != "" { + // future changes diverge from ResolveRootDir. + if env.RootDir != resolvedRoot && env.RootDir != "" { d.markActiveEnvRoot(env.RootDir) defer d.unmarkActiveEnvRoot(env.RootDir) } diff --git a/server/internal/daemon/diskusage.go b/server/internal/daemon/diskusage.go index e71d5b112..23481ef24 100644 --- a/server/internal/daemon/diskusage.go +++ b/server/internal/daemon/diskusage.go @@ -190,8 +190,8 @@ func ScanDiskUsage(workspacesRoot string, artifactPatterns []string) (DiskUsageR if strings.HasPrefix(wsEntry.Name(), ".") { continue } - wsID := wsEntry.Name() - wsDir := filepath.Join(workspacesRoot, wsID) + physicalWorkspace := wsEntry.Name() + wsDir := filepath.Join(workspacesRoot, physicalWorkspace) taskEntries, err := os.ReadDir(wsDir) if err != nil { continue @@ -201,19 +201,20 @@ func ScanDiskUsage(workspacesRoot string, artifactPatterns []string) (DiskUsageR continue } taskDir := filepath.Join(wsDir, t.Name()) - usage := buildTaskUsage(taskDir, wsID, t.Name(), matcher) + usage := buildTaskUsage(taskDir, physicalWorkspace, t.Name(), matcher) report.Tasks = append(report.Tasks, usage) report.TotalSizeBytes += usage.SizeBytes report.TotalArtifactSizeBytes += usage.ArtifactSizeBytes - ws, ok := wsAgg[wsID] + workspaceID := usage.WorkspaceID + ws, ok := wsAgg[workspaceID] if !ok { ws = &WorkspaceDiskUsage{ - WorkspaceID: wsID, - WorkspaceShort: ShortID(wsID), + WorkspaceID: workspaceID, + WorkspaceShort: usage.WorkspaceShort, } - wsAgg[wsID] = ws + wsAgg[workspaceID] = ws } ws.TaskCount++ ws.SizeBytes += usage.SizeBytes @@ -313,8 +314,24 @@ func buildTaskUsage(taskDir, wsID, taskShort string, matcher artifactMatcher) Ta } metaPresent := false + if provenance, err := execenv.ReadManagedEnvProvenance(taskDir); err == nil && provenance != nil { + if workspaceID := strings.TrimSpace(provenance.WorkspaceID); workspaceID != "" { + usage.WorkspaceID = workspaceID + usage.WorkspaceShort = ShortID(workspaceID) + } + } + if owner, err := execenv.ReadEnvRootOwner(taskDir); err == nil && owner != nil { + if workspaceID := strings.TrimSpace(owner.WorkspaceID); workspaceID != "" { + usage.WorkspaceID = workspaceID + usage.WorkspaceShort = ShortID(workspaceID) + } + } if meta, err := execenv.ReadGCMeta(taskDir); err == nil && meta != nil { metaPresent = true + if workspaceID := strings.TrimSpace(meta.WorkspaceID); workspaceID != "" { + usage.WorkspaceID = workspaceID + usage.WorkspaceShort = ShortID(workspaceID) + } usage.Kind = string(meta.Kind) usage.ParentID = parentIDForMeta(meta) if !meta.CompletedAt.IsZero() { diff --git a/server/internal/daemon/diskusage_test.go b/server/internal/daemon/diskusage_test.go index f2d3a33bc..335026149 100644 --- a/server/internal/daemon/diskusage_test.go +++ b/server/internal/daemon/diskusage_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "os" "path/filepath" "runtime" @@ -207,6 +208,100 @@ func TestScanDiskUsage_AggregatesAndCategorizes(t *testing.T) { } } +func TestScanDiskUsage_MixedLayoutsUseMetadataIdentity(t *testing.T) { + t.Parallel() + + root := t.TempDir() + workspaceID := "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + readableTaskID := "22222222-ee7a-4603-a72d-b659c34a1dc3" + legacyTaskSegment := "a548b2390cb2" + readableTaskSegment := "mul-6063-b659c34a1dc3" + + legacyDir := filepath.Join(root, workspaceID, legacyTaskSegment) + writeFile(t, filepath.Join(legacyDir, "workdir", "legacy.txt"), 10) + mustWriteMeta(t, legacyDir, execenv.GCMeta{ + Kind: execenv.GCKindIssue, + IssueID: "issue-legacy", + WorkspaceID: workspaceID, + CompletedAt: time.Now().Add(-time.Hour), + }) + + readableDir := filepath.Join(root, "asset-feed-a548b2390cb2", readableTaskSegment) + writeFile(t, filepath.Join(readableDir, "workdir", "readable.txt"), 20) + mustWriteMeta(t, readableDir, execenv.GCMeta{ + Kind: execenv.GCKindIssue, + IssueID: "issue-readable", + TaskID: readableTaskID, + WorkspaceID: workspaceID, + CompletedAt: time.Now().Add(-time.Hour), + }) + + report, err := ScanDiskUsage(root, nil) + if err != nil { + t.Fatalf("ScanDiskUsage: %v", err) + } + if report.TotalTaskCount != 2 || report.TotalWorkspaceCount != 1 { + t.Fatalf("totals = tasks:%d workspaces:%d, want 2/1", report.TotalTaskCount, report.TotalWorkspaceCount) + } + for _, task := range report.Tasks { + if task.WorkspaceID != workspaceID { + t.Errorf("task %q workspace_id = %q, want metadata id %q", task.Path, task.WorkspaceID, workspaceID) + } + } + byShort := map[string]TaskDiskUsage{} + for _, task := range report.Tasks { + byShort[task.TaskShort] = task + } + if _, ok := byShort[legacyTaskSegment]; !ok { + t.Errorf("legacy task was not reported under physical directory segment %q", legacyTaskSegment) + } + if _, ok := byShort[readableTaskSegment]; !ok { + t.Errorf("readable task was not reported under physical directory segment %q", readableTaskSegment) + } +} + +func TestScanDiskUsage_ReadableActiveRootUsesOwnerIdentityWithoutGCMeta(t *testing.T) { + t.Parallel() + + root := t.TempDir() + workspaceID := "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + taskID := "22222222-ee7a-4603-a72d-b659c34a1dc3" + env, err := execenv.Prepare(execenv.PrepareParams{ + WorkspacesRoot: root, + WorkspaceID: workspaceID, + WorkspaceSlug: "Asset Feed", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + AgentName: "Active Task", + }, slog.Default()) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + defer env.Cleanup(true) + if _, err := os.Stat(filepath.Join(env.RootDir, ".gc_meta.json")); !os.IsNotExist(err) { + t.Fatalf("fixture must represent an active root without completion metadata; stat err = %v", err) + } + + report, err := ScanDiskUsage(root, nil) + if err != nil { + t.Fatalf("ScanDiskUsage: %v", err) + } + if len(report.Tasks) != 1 { + t.Fatalf("tasks = %d, want 1", len(report.Tasks)) + } + usage := report.Tasks[0] + if usage.WorkspaceID != workspaceID { + t.Fatalf("workspace_id = %q, want owner identity %q", usage.WorkspaceID, workspaceID) + } + wantTaskSegment := filepath.Base(env.RootDir) + if usage.TaskShort != wantTaskSegment { + t.Fatalf("task_short = %q, want physical directory segment %q", usage.TaskShort, wantTaskSegment) + } + if len(report.Workspaces) != 1 || report.Workspaces[0].WorkspaceID != workspaceID { + t.Fatalf("workspace aggregate = %+v, want authoritative workspace %q", report.Workspaces, workspaceID) + } +} + func TestScanDiskUsage_ManagedCodexSandboxIsExactAndDeduplicated(t *testing.T) { t.Parallel() diff --git a/server/internal/daemon/execenv/execenv.go b/server/internal/daemon/execenv/execenv.go index 8bb6a7a2c..5e20b779f 100644 --- a/server/internal/daemon/execenv/execenv.go +++ b/server/internal/daemon/execenv/execenv.go @@ -37,10 +37,12 @@ type ProjectResourceForEnv struct { // PrepareParams holds all inputs needed to set up an execution environment. type PrepareParams struct { - WorkspacesRoot string // base path for all envs (e.g., ~/multica_workspaces) - WorkspaceID string // workspace UUID — tasks are grouped under this - TaskID string // task UUID — used for directory name - AgentName string // for git branch naming only + WorkspacesRoot string // base path for all envs (e.g., ~/multica_workspaces) + WorkspaceID string // workspace UUID — stable identity and path suffix + WorkspaceSlug string // human-readable workspace path prefix + TaskID string // task UUID — stable identity and path suffix + IssueIdentifier string // human-readable issue key (e.g. MUL-6063); empty for non-issue tasks + AgentName string // for git branch naming only // EnvRootPreclaimed says the CALLER already holds this env root's claim // (see ClaimEnvRoot) and has already reset it. Prepare then skips claiming. // @@ -350,14 +352,59 @@ type Environment struct { lockFile *os.File } -// PredictRootDir returns the env root path that Prepare would create for the -// given task, without performing any I/O. Callers use this to claim ownership -// of the directory (e.g. against the GC loop) before Prepare/Reuse runs. -func PredictRootDir(workspacesRoot, workspaceID, taskID string) string { - if workspacesRoot == "" || workspaceID == "" || taskID == "" { +// RootDirParams is the identity and display data used to derive a new task's +// environment root. IDs provide stable collision-safe suffixes; user-controlled +// labels are only readable prefixes and never serve as identity. +type RootDirParams struct { + WorkspacesRoot string + WorkspaceID string + WorkspaceSlug string + TaskID string + IssueIdentifier string +} + +// Keep each readable segment within the same aggregate budget as main's +// /<12-char task key> layout. The env root prefixes both the +// checkout and git ref paths, so every extra character is costly on Windows. +const readablePathSegmentMax = 24 + +// PredictRootDir returns the readable path proposed for a task without doing +// I/O. ResolveRootDir freezes the first proposal and must be used by callers +// that need the task's authoritative physical root. +func PredictRootDir(params RootDirParams) string { + if params.WorkspacesRoot == "" || params.WorkspaceID == "" || params.TaskID == "" { return "" } - return filepath.Join(workspacesRoot, workspaceID, taskKey(taskID)) + return filepath.Join( + params.WorkspacesRoot, + readablePathSegment(params.WorkspaceSlug, "workspace", params.WorkspaceID), + readablePathSegment(params.IssueIdentifier, "task", params.TaskID), + ) +} + +// readablePathSegment converts a user-controlled label into a bounded, +// lowercase ASCII prefix and appends the stable ID suffix. The suffix keeps +// paths distinct when labels differ only by case, sanitize to the same value, +// or change later. +func readablePathSegment(label, fallback, id string) string { + prefix := strings.ToLower(strings.TrimSpace(label)) + prefix = nonAlphanumeric.ReplaceAllString(prefix, "-") + prefix = strings.Trim(prefix, "-") + if prefix == "" { + prefix = fallback + } + + // UUIDv7's leading bits are a timestamp shared by burst-created tasks. + // taskKey takes the random tail; it is equally suitable for workspace UUIDs. + suffix := strings.ToLower(taskKey(id)) + maxPrefix := readablePathSegmentMax - len(suffix) - 1 + if len(prefix) > maxPrefix { + prefix = strings.TrimRight(prefix[:maxPrefix], "-") + } + if prefix == "" { + prefix = fallback + } + return prefix + "-" + suffix } // Prepare creates an isolated execution environment for a task. @@ -374,7 +421,16 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { return nil, fmt.Errorf("execenv: task ID is required") } - envRoot := PredictRootDir(params.WorkspacesRoot, params.WorkspaceID, params.TaskID) + envRoot, err := ResolveRootDir(RootDirParams{ + WorkspacesRoot: params.WorkspacesRoot, + WorkspaceID: params.WorkspaceID, + WorkspaceSlug: params.WorkspaceSlug, + TaskID: params.TaskID, + IssueIdentifier: params.IssueIdentifier, + }) + if err != nil { + return nil, err + } // Self-heal the root-level daemon marker on every task start so a marker // removed while the daemon runs is restored before the agent spawns. The @@ -409,7 +465,7 @@ func Prepare(params PrepareParams, logger *slog.Logger) (*Environment, error) { return nil, fmt.Errorf("execenv: create env root %s: %w", envRoot, err) } } else { - lock, reset, err := claimEnvRoot(envRoot, params.TaskID) + lock, reset, err := claimEnvRoot(envRoot, params.WorkspaceID, params.TaskID) if err != nil { return nil, fmt.Errorf("execenv: %w", err) } @@ -1013,7 +1069,10 @@ const ( // GCMeta is persisted to .gc_meta.json inside the env root so the GC loop // can decide whether the directory is reclaimable. It is a discriminated -// union keyed on Kind: only the ID field matching Kind is meaningful. +// union keyed on Kind: only the parent ID field matching Kind is meaningful. +// TaskID is also persisted for every new task so local reports never need to +// recover task identity from the directory name; for quick-create it doubles +// as the parent ID used by the GC status endpoint. // // Older meta files (pre-v2) lack the Kind field; readers must default empty // Kind to GCKindIssue for backward compatibility — only IssueID was written @@ -1222,12 +1281,15 @@ func (c *EnvRootClaim) Release() { // Callers that pass the claim to Prepare must also set // PrepareParams.EnvRootPreclaimed, or Prepare will try to take a lock this // claim already holds and fail. -func ClaimEnvRoot(workspacesRoot, workspaceID, taskID string) (*EnvRootClaim, error) { - envRoot := PredictRootDir(workspacesRoot, workspaceID, taskID) +func ClaimEnvRoot(params RootDirParams) (*EnvRootClaim, error) { + envRoot, err := ResolveRootDir(params) + if err != nil { + return nil, fmt.Errorf("execenv: resolve env root: %w", err) + } if envRoot == "" { return nil, fmt.Errorf("execenv: claim env root: workspaces root, workspace ID and task ID are all required") } - lock, reset, err := claimEnvRoot(envRoot, taskID) + lock, reset, err := claimEnvRoot(envRoot, params.WorkspaceID, params.TaskID) if err != nil { return nil, fmt.Errorf("execenv: %w", err) } @@ -1313,9 +1375,16 @@ func LockEnvRootForReuse(wsRoot *os.Root, rel, envRoot string) (*EnvRootClaim, o return &EnvRootClaim{rootDir: envRoot, lock: lock}, info, nil } -// envRootOwnerFile records which task an env root belongs to: WHO owns it. +// envRootOwnerFile records which workspace and task an env root belongs to: +// WHO owns it. The execution lock below separately answers whether that owner +// is still running. const envRootOwnerFile = ".task_owner" +const ( + envRootOwnerTempPrefix = ".task_owner-" + envRootOwnerTempSuffix = ".tmp" +) + // envRootLockFile carries the env root's exclusive execution lock: whether the // owner is STILL RUNNING. The two answer different questions and both are // needed — a dead task's directory still holds its work, and a live task's @@ -1344,7 +1413,7 @@ const envRootLockFile = ".task_lock" // // Holding the lock also serialises everything below it, which is what makes // repairing a torn marker safe: no other execution can be mid-claim. -func claimEnvRoot(envRoot, taskID string) (lockFile *os.File, reset bool, err error) { +func claimEnvRoot(envRoot, workspaceID, taskID string) (lockFile *os.File, reset bool, err error) { if err := os.MkdirAll(envRoot, 0o755); err != nil { return nil, false, fmt.Errorf("create env root %s: %w", envRoot, err) } @@ -1370,18 +1439,28 @@ func claimEnvRoot(envRoot, taskID string) (lockFile *os.File, reset bool, err er lockFile = nil } }() + if err := removeStaleEnvRootOwnerTemps(envRoot); err != nil { + return nil, false, fmt.Errorf("remove stale env root owner temp files for %s: %w", envRoot, err) + } - owner, err := readEnvRootOwner(envRoot) + owner, err := ReadEnvRootOwner(envRoot) if err != nil { return nil, false, fmt.Errorf("read env root owner for %s: %w", envRoot, err) } switch { - case owner == taskID: + case owner.TaskID == taskID && (owner.WorkspaceID == "" || owner.WorkspaceID == workspaceID): + // Upgrade legacy task-only markers while the lock makes the rewrite + // exclusive. Disk usage can then attribute active roots by workspace. + if owner.WorkspaceID == "" { + if err := writeEnvRootOwner(envRoot, workspaceID, taskID); err != nil { + return nil, false, err + } + } // Ours, and the execution that left it is provably gone — we hold the // lock it would still be holding. return lockFile, true, nil - case owner != "": - return nil, false, fmt.Errorf("env root %s belongs to task %s; refusing to reset it for task %s", envRoot, owner, taskID) + case owner.TaskID != "": + return nil, false, fmt.Errorf("env root %s belongs to task %s in workspace %s; refusing to reset it for task %s in workspace %s", envRoot, owner.TaskID, owner.WorkspaceID, taskID, workspaceID) } // No owner recorded. Either the directory is new, or a crash tore the @@ -1394,7 +1473,7 @@ func claimEnvRoot(envRoot, taskID string) (lockFile *os.File, reset bool, err er if hasWork { return nil, false, fmt.Errorf("env root %s already holds files but names no owning task; refusing to delete it", envRoot) } - if err := writeEnvRootOwner(envRoot, taskID); err != nil { + if err := writeEnvRootOwner(envRoot, workspaceID, taskID); err != nil { return nil, false, err } return lockFile, false, nil @@ -1423,13 +1502,61 @@ func releaseLockFile(f *os.File) { _ = f.Close() } -// writeEnvRootOwner records taskID as the owner. Callers must hold the env -// root lock, which is what lets this overwrite a marker torn by an earlier -// crash without racing another execution mid-claim. -func writeEnvRootOwner(envRoot, taskID string) error { +// writeEnvRootOwner records authoritative workspace/task identity. Callers +// must hold the env root lock, which makes overwriting a torn or legacy marker +// safe from another execution racing mid-claim. The same-directory temp file +// and rename keep lock-free readers from observing a truncated JSON marker. +func writeEnvRootOwner(envRoot, workspaceID, taskID string) error { path := filepath.Join(envRoot, envRootOwnerFile) - if err := os.WriteFile(path, []byte(taskID), 0o644); err != nil { - return fmt.Errorf("record env root owner for %s: %w", envRoot, err) + data, err := json.Marshal(EnvRootOwner{WorkspaceID: workspaceID, TaskID: taskID}) + if err != nil { + return fmt.Errorf("encode env root owner for %s: %w", envRoot, err) + } + + tmp, err := os.CreateTemp(envRoot, envRootOwnerTempPrefix+"*"+envRootOwnerTempSuffix) + if err != nil { + return fmt.Errorf("create temp env root owner for %s: %w", envRoot, err) + } + tmpPath := tmp.Name() + defer os.Remove(tmpPath) + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write temp env root owner for %s: %w", envRoot, err) + } + if err := tmp.Chmod(0o644); err != nil { + tmp.Close() + return fmt.Errorf("chmod temp env root owner for %s: %w", envRoot, err) + } + if err := tmp.Sync(); err != nil { + tmp.Close() + return fmt.Errorf("sync temp env root owner for %s: %w", envRoot, err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp env root owner for %s: %w", envRoot, err) + } + if err := os.Rename(tmpPath, path); err != nil { + return fmt.Errorf("replace env root owner for %s: %w", envRoot, err) + } + return nil +} + +// removeStaleEnvRootOwnerTemps clears unpublished files left by a process that +// exited before the atomic rename. Callers hold the env root lock, so no live +// owner write can be using one of these files. +func removeStaleEnvRootOwnerTemps(envRoot string) error { + entries, err := os.ReadDir(envRoot) + if err != nil { + return err + } + for _, entry := range entries { + name := entry.Name() + if !strings.HasPrefix(name, envRootOwnerTempPrefix) || !strings.HasSuffix(name, envRootOwnerTempSuffix) { + continue + } + if err := os.RemoveAll(filepath.Join(envRoot, name)); err != nil { + return err + } } return nil } @@ -1439,14 +1566,40 @@ func writeEnvRootOwner(envRoot, taskID string) error { // error, not an empty owner: treating it as unowned would hand the caller a // licence to delete the very directory it could not identify. func readEnvRootOwner(envRoot string) (string, error) { - b, err := os.ReadFile(filepath.Join(envRoot, envRootOwnerFile)) - if errors.Is(err, os.ErrNotExist) { - return "", nil - } + owner, err := ReadEnvRootOwner(envRoot) if err != nil { return "", err } - return strings.TrimSpace(string(b)), nil + return owner.TaskID, nil +} + +// EnvRootOwner is written before any task content so active and partially +// prepared roots retain authoritative identity without completion metadata. +type EnvRootOwner struct { + WorkspaceID string `json:"workspace_id,omitempty"` + TaskID string `json:"task_id"` +} + +// ReadEnvRootOwner reads both current JSON markers and legacy plain task IDs. +func ReadEnvRootOwner(envRoot string) (*EnvRootOwner, error) { + b, err := os.ReadFile(filepath.Join(envRoot, envRootOwnerFile)) + if errors.Is(err, os.ErrNotExist) { + return &EnvRootOwner{}, nil + } + if err != nil { + return nil, err + } + trimmed := strings.TrimSpace(string(b)) + if !strings.HasPrefix(trimmed, "{") { + return &EnvRootOwner{TaskID: trimmed}, nil + } + var owner EnvRootOwner + if err := json.Unmarshal(b, &owner); err != nil { + return nil, err + } + owner.TaskID = strings.TrimSpace(owner.TaskID) + owner.WorkspaceID = strings.TrimSpace(owner.WorkspaceID) + return &owner, nil } // resetEnvRootContents empties an env root the caller already owns and holds @@ -1485,5 +1638,14 @@ func envRootHoldsWork(envRoot string) (bool, error) { } func isEnvRootBookkeeping(name string) bool { - return name == envRootOwnerFile || name == envRootLockFile + if name == envRootOwnerFile || name == envRootLockFile { + return true + } + // An unpublished owner temp is a crash leftover from writeEnvRootOwner, not + // task content. claimEnvRoot clears these before it looks, but + // findOwnedTaskRoot inspects candidate roots WITHOUT the lock — and reading + // one as work makes adoption refuse a root that holds nothing a task could + // lose, wedging that task permanently. + return strings.HasPrefix(name, envRootOwnerTempPrefix) && + strings.HasSuffix(name, envRootOwnerTempSuffix) } diff --git a/server/internal/daemon/execenv/execenv_test.go b/server/internal/daemon/execenv/execenv_test.go index 4bc2b3d30..03915c341 100644 --- a/server/internal/daemon/execenv/execenv_test.go +++ b/server/internal/daemon/execenv/execenv_test.go @@ -12,6 +12,7 @@ import ( "strings" "sync" "testing" + "time" ) func testLogger() *slog.Logger { @@ -41,22 +42,395 @@ func TestShortID(t *testing.T) { func TestPredictRootDir(t *testing.T) { t.Parallel() - got := PredictRootDir("/root", "ws-uuid", "a1b2c3d4-e5f6-7890-abcd-ef1234567890") - want := filepath.Join("/root", "ws-uuid", "ef1234567890") + got := PredictRootDir(RootDirParams{ + WorkspacesRoot: "/root", + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + IssueIdentifier: "MUL-6063", + }) + want := filepath.Join("/root", "asset-feed-a548b2390cb2", "mul-6063-b659c34a1dc3") if got != want { t.Errorf("PredictRootDir = %q, want %q", got, want) } - if got := PredictRootDir("", "ws", "task"); got != "" { + if got := PredictRootDir(RootDirParams{WorkspaceID: "ws", TaskID: "task"}); got != "" { t.Errorf("expected empty when workspaces root missing, got %q", got) } - if got := PredictRootDir("/r", "", "task"); got != "" { + if got := PredictRootDir(RootDirParams{WorkspacesRoot: "/r", TaskID: "task"}); got != "" { t.Errorf("expected empty when workspace ID missing, got %q", got) } - if got := PredictRootDir("/r", "ws", ""); got != "" { + if got := PredictRootDir(RootDirParams{WorkspacesRoot: "/r", WorkspaceID: "ws"}); got != "" { t.Errorf("expected empty when task ID missing, got %q", got) } } +func TestResolveRootDirFreezesReadableNamesBeforePrepare(t *testing.T) { + t.Parallel() + + root := t.TempDir() + base := RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + } + + first, err := ResolveRootDir(base) + if err != nil { + t.Fatalf("resolve fallback root: %v", err) + } + if _, err := os.Stat(first); !os.IsNotExist(err) { + t.Fatalf("ResolveRootDir should freeze identity before creating the env root; stat err = %v", err) + } + + enriched := base + enriched.WorkspaceSlug = "Asset Feed" + enriched.IssueIdentifier = "MUL-6063" + second, err := ResolveRootDir(enriched) + if err != nil { + t.Fatalf("resolve enriched root: %v", err) + } + if second != first { + t.Fatalf("same task moved from %q to %q when readable fields arrived", first, second) + } + + renamed := enriched + renamed.WorkspaceSlug = "Renamed Workspace" + renamed.IssueIdentifier = "NEW-6063" + third, err := ResolveRootDir(renamed) + if err != nil { + t.Fatalf("resolve renamed root: %v", err) + } + if third != first { + t.Fatalf("same task moved from %q to %q after workspace/issue rename", first, third) + } + + env, err := Prepare(PrepareParams{ + WorkspacesRoot: renamed.WorkspacesRoot, + WorkspaceID: renamed.WorkspaceID, + WorkspaceSlug: renamed.WorkspaceSlug, + TaskID: renamed.TaskID, + IssueIdentifier: renamed.IssueIdentifier, + AgentName: "Stable Root", + Task: TaskContextForEnv{IssueID: "issue-stable-root"}, + }, testLogger()) + if err != nil { + t.Fatalf("Prepare after pre-StartTask reclaim: %v", err) + } + defer env.Cleanup(true) + if env.RootDir != first { + t.Fatalf("Prepare root = %q, want frozen root %q", env.RootDir, first) + } + + liveRename := renamed + liveRename.WorkspaceSlug = "Renamed Again" + liveRename.IssueIdentifier = "NEXT-6063" + afterPrepare, err := ResolveRootDir(liveRename) + if err != nil { + t.Fatalf("resolve after live rename: %v", err) + } + if afterPrepare != first { + t.Fatalf("live task moved from %q to %q after issue prefix changed", first, afterPrepare) + } +} + +func TestResolveRootDirAdoptsExistingOwnedRootBeforeIndex(t *testing.T) { + t.Parallel() + + root := t.TempDir() + const ( + workspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + taskID = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" + ) + original := PredictRootDir(RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: workspaceID, + WorkspaceSlug: "Asset Feed", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + }) + if err := os.MkdirAll(original, 0o755); err != nil { + t.Fatalf("seed existing root: %v", err) + } + if err := writeEnvRootOwner(original, workspaceID, taskID); err != nil { + t.Fatalf("seed existing owner: %v", err) + } + + resolved, err := ResolveRootDir(RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: workspaceID, + WorkspaceSlug: "Renamed Workspace", + TaskID: taskID, + IssueIdentifier: "NEW-6063", + }) + if err != nil { + t.Fatalf("resolve renamed existing root: %v", err) + } + if resolved != original { + t.Fatalf("existing owned root %q was orphaned in favor of %q", original, resolved) + } +} + +func TestResolveRootDirAdoptsRootWithInterruptedOwnerTemp(t *testing.T) { + t.Parallel() + + root := t.TempDir() + const ( + workspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + taskID = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" + ) + original := PredictRootDir(RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: workspaceID, + WorkspaceSlug: "Asset Feed", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + }) + if err := os.MkdirAll(original, 0o755); err != nil { + t.Fatalf("seed existing root: %v", err) + } + tempOwner := filepath.Join(original, envRootOwnerTempPrefix+"crashed"+envRootOwnerTempSuffix) + if err := os.WriteFile(tempOwner, []byte(`{"workspace_id":"`+workspaceID+`"`), 0o600); err != nil { + t.Fatalf("seed interrupted owner temp: %v", err) + } + + resolved, err := ResolveRootDir(RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: workspaceID, + WorkspaceSlug: "Renamed Workspace", + TaskID: taskID, + IssueIdentifier: "NEW-6063", + }) + if err != nil { + t.Fatalf("resolve root with interrupted owner temp: %v", err) + } + if resolved != original { + t.Fatalf("recoverable root %q was orphaned in favor of %q", original, resolved) + } + if _, err := os.Stat(tempOwner); err != nil { + t.Fatalf("resolution unexpectedly mutated the candidate root: %v", err) + } +} + +func TestResolveRootDirConcurrentFirstClaimsChooseOnePhysicalRoot(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := []RootDirParams{ + { + WorkspacesRoot: root, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + }, + { + WorkspacesRoot: root, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + IssueIdentifier: "MUL-6063", + }, + } + + start := make(chan struct{}) + paths := make([]string, len(params)) + errs := make([]error, len(params)) + var wg sync.WaitGroup + for i := range params { + wg.Add(1) + go func() { + defer wg.Done() + <-start + paths[i], errs[i] = ResolveRootDir(params[i]) + }() + } + close(start) + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Fatalf("resolve %d: %v", i, err) + } + } + if paths[0] != paths[1] { + t.Fatalf("concurrent claims chose two roots: %q and %q", paths[0], paths[1]) + } +} + +func TestRemoveRootDirRecordKeepsSharedIndexParent(t *testing.T) { + t.Parallel() + + params := RootDirParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + } + envRoot, err := ResolveRootDir(params) + if err != nil { + t.Fatalf("ResolveRootDir: %v", err) + } + if err := RemoveRootDirRecord(params.WorkspacesRoot, envRoot, EnvRootOwner{ + WorkspaceID: params.WorkspaceID, + TaskID: params.TaskID, + }); err != nil { + t.Fatalf("RemoveRootDirRecord: %v", err) + } + indexInfo, err := os.Stat(filepath.Join(params.WorkspacesRoot, taskRootIndexDir)) + if err != nil { + t.Fatalf("shared task root index was removed: %v", err) + } + if !indexInfo.IsDir() { + t.Fatal("shared task root index is not a directory") + } +} + +func TestPruneTaskRootIndexBoundsAbandonedEntries(t *testing.T) { + t.Parallel() + + root := t.TempDir() + terminal := RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + } + running := terminal + running.TaskID = "6d68c76c-ff8b-5704-b83e-c76ad45b2ed4" + materialized := terminal + materialized.TaskID = "7e79d87d-008c-6805-c94f-d87be56c3fe5" + recent := terminal + recent.TaskID = "8f8ae98e-119d-7906-da50-e98cf67d40f6" + var materializedRoot string + for _, params := range []RootDirParams{terminal, running, materialized, recent} { + resolved, err := ResolveRootDir(params) + if err != nil { + t.Fatalf("ResolveRootDir(%s): %v", params.TaskID, err) + } + if params.TaskID == materialized.TaskID { + materializedRoot = resolved + } + } + if err := os.MkdirAll(materializedRoot, 0o755); err != nil { + t.Fatalf("materialize protected env root: %v", err) + } + + indexDir := filepath.Join(root, taskRootIndexDir) + stalePending := filepath.Join(indexDir, taskRootPendingPrefix+"stale") + recentPending := filepath.Join(indexDir, taskRootPendingPrefix+"recent") + for _, dir := range []string{stalePending, recentPending} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("seed pending entry: %v", err) + } + } + now := time.Now() + old := now.Add(-2 * taskRootIndexMinPruneAge) + for _, params := range []RootDirParams{terminal, running, materialized} { + recordPath := filepath.Join(taskRootRecordDir(params), taskRootRecordFile) + if err := os.Chtimes(recordPath, old, old); err != nil { + t.Fatalf("age task root record %s: %v", recordPath, err) + } + } + if err := os.Chtimes(stalePending, old, old); err != nil { + t.Fatalf("age pending entry %s: %v", stalePending, err) + } + + removed, err := PruneTaskRootIndex(root, 0, now, func(_, taskID string) bool { + return taskID == terminal.TaskID || taskID == materialized.TaskID || taskID == recent.TaskID + }) + if err != nil { + t.Fatalf("PruneTaskRootIndex: %v", err) + } + if removed != 2 { + t.Fatalf("removed = %d, want terminal record plus stale pending entry", removed) + } + for _, removedPath := range []string{taskRootRecordDir(terminal), stalePending} { + if _, err := os.Stat(removedPath); !os.IsNotExist(err) { + t.Fatalf("stale entry %s still exists: %v", removedPath, err) + } + } + for _, keptPath := range []string{taskRootRecordDir(running), taskRootRecordDir(materialized), taskRootRecordDir(recent), recentPending} { + if _, err := os.Stat(keptPath); err != nil { + t.Fatalf("protected entry %s was removed: %v", keptPath, err) + } + } +} + +func TestResolveRootDirRejectsRecordOutsideStableIdentity(t *testing.T) { + t.Parallel() + + params := RootDirParams{ + WorkspacesRoot: t.TempDir(), + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + } + if err := installTaskRootRecord(taskRootRecordDir(params), taskRootRecord{ + WorkspaceID: params.WorkspaceID, + TaskID: params.TaskID, + RelativePath: filepath.Join("unrelated-workspace", "unrelated-task"), + }); err != nil { + t.Fatalf("seed corrupt record: %v", err) + } + if _, err := ResolveRootDir(params); err == nil { + t.Fatal("ResolveRootDir accepted a record outside the task's stable identity") + } else if !strings.Contains(err.Error(), "does not match its stable identity") { + t.Fatalf("error = %v, want stable identity rejection", err) + } else if !strings.Contains(err.Error(), taskRootRecordDir(params)) { + t.Fatalf("error = %v, want actionable record directory", err) + } +} + +func TestReadablePathSegmentSanitizesUserControlledLabels(t *testing.T) { + t.Parallel() + tests := []struct { + name string + label string + fallback string + id string + want string + }{ + {name: "separators and traversal", label: `../Asset\\Feed/Team`, fallback: "workspace", id: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", want: "asset-feed-a548b2390cb2"}, + {name: "non ascii removed", label: "日本語 Product", fallback: "workspace", id: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", want: "product-a548b2390cb2"}, + {name: "case normalized", label: "MUL-6063", fallback: "task", id: "5c57b65b-ee7a-4603-a72d-a548b2390cb2", want: "mul-6063-a548b2390cb2"}, + {name: "empty label falls back", label: "...", fallback: "task", id: "5c57b65b-ee7a-4603-a72d-a548b2390cb2", want: "task-a548b2390cb2"}, + {name: "label is bounded", label: strings.Repeat("a", 100), fallback: "task", id: "5c57b65b-ee7a-4603-a72d-a548b2390cb2", want: strings.Repeat("a", 11) + "-a548b2390cb2"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := readablePathSegment(tt.label, tt.fallback, tt.id); got != tt.want { + t.Fatalf("readablePathSegment(%q) = %q, want %q", tt.label, got, tt.want) + } + }) + } +} + +func TestPredictRootDirWorstCaseLabelsStayWithinWindowsBudget(t *testing.T) { + t.Parallel() + + root := PredictRootDir(RootDirParams{ + WorkspacesRoot: "/root", + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: strings.Repeat("workspace", 20), + TaskID: "01a01ec0-e69d-7000-8000-0123456789ab", + IssueIdentifier: strings.Repeat("issue", 20), + }) + rel, err := filepath.Rel("/root", root) + if err != nil { + t.Fatalf("relative env root: %v", err) + } + parts := strings.Split(rel, string(filepath.Separator)) + if len(parts) != 2 { + t.Fatalf("relative env root = %q, want exactly two segments", rel) + } + for _, part := range parts { + if len(part) > readablePathSegmentMax { + t.Fatalf("path segment %q has length %d, want <= %d", part, len(part), readablePathSegmentMax) + } + } + // main's opaque layout spent 36 + 1 + 12 characters below WorkspacesRoot. + // The readable layout must not consume a larger Windows path budget. + if got, max := len(rel), 36+1+taskKeyLen; got > max { + t.Fatalf("relative env root %q has length %d, want <= %d", rel, got, max) + } +} + func TestSanitizeName(t *testing.T) { t.Parallel() tests := []struct { @@ -6171,7 +6545,13 @@ func TestPredictRootDirDistinctForSharedUUIDv7Prefix(t *testing.T) { } seen := make(map[string]string, len(ids)) for _, id := range ids { - root := PredictRootDir("/root", "ws-uuid", id) + root := PredictRootDir(RootDirParams{ + WorkspacesRoot: "/root", + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: id, + IssueIdentifier: "MUL-6063", + }) if prev, dup := seen[root]; dup { t.Fatalf("tasks %s and %s share env root %q — a truncated task id is back", prev, id, root) } @@ -6208,11 +6588,13 @@ func TestPrepareDoesNotDeleteConcurrentTaskEnv(t *testing.T) { ) envA, err := Prepare(PrepareParams{ - WorkspacesRoot: workspacesRoot, - WorkspaceID: "ws-collision", - TaskID: taskA, - AgentName: "Agent A", - Task: TaskContextForEnv{IssueID: taskA}, + WorkspacesRoot: workspacesRoot, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: taskA, + IssueIdentifier: "MUL-6063", + AgentName: "Agent A", + Task: TaskContextForEnv{IssueID: taskA}, }, testLogger()) if err != nil { t.Fatalf("Prepare task A: %v", err) @@ -6226,11 +6608,13 @@ func TestPrepareDoesNotDeleteConcurrentTaskEnv(t *testing.T) { } envB, err := Prepare(PrepareParams{ - WorkspacesRoot: workspacesRoot, - WorkspaceID: "ws-collision", - TaskID: taskB, - AgentName: "Agent B", - Task: TaskContextForEnv{IssueID: taskB}, + WorkspacesRoot: workspacesRoot, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: taskB, + IssueIdentifier: "MUL-6063", + AgentName: "Agent B", + Task: TaskContextForEnv{IssueID: taskB}, }, testLogger()) if err != nil { t.Fatalf("Prepare task B: %v", err) @@ -6280,11 +6664,17 @@ func TestPrepareRefusesEnvRootOwnedByAnotherTask(t *testing.T) { workspacesRoot := t.TempDir() const taskID = "01a01ec0-e69d-7000-8000-0123456789ab" - envRoot := PredictRootDir(workspacesRoot, "ws-owned", taskID) + envRoot := PredictRootDir(RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: "ws-owned", + WorkspaceSlug: "Owned Workspace", + TaskID: taskID, + IssueIdentifier: "MUL-1", + }) if err := os.MkdirAll(filepath.Join(envRoot, "workdir"), 0o755); err != nil { t.Fatalf("seed env root: %v", err) } - if err := writeEnvRootOwner(envRoot, "11111111-2222-3333-4444-555555555555"); err != nil { + if err := writeEnvRootOwner(envRoot, "ws-owned", "11111111-2222-3333-4444-555555555555"); err != nil { t.Fatalf("seed owner: %v", err) } survivor := filepath.Join(envRoot, "workdir", "other-task-work.txt") @@ -6293,11 +6683,13 @@ func TestPrepareRefusesEnvRootOwnedByAnotherTask(t *testing.T) { } _, err := Prepare(PrepareParams{ - WorkspacesRoot: workspacesRoot, - WorkspaceID: "ws-owned", - TaskID: taskID, - AgentName: "Intruder", - Task: TaskContextForEnv{IssueID: taskID}, + WorkspacesRoot: workspacesRoot, + WorkspaceID: "ws-owned", + WorkspaceSlug: "Owned Workspace", + TaskID: taskID, + IssueIdentifier: "MUL-1", + AgentName: "Intruder", + Task: TaskContextForEnv{IssueID: taskID}, }, testLogger()) if err == nil { t.Fatal("Prepare accepted an env root owned by another task") @@ -6439,7 +6831,7 @@ func TestClaimEnvRootRefusesUnownedDirectoryWithContent(t *testing.T) { t.Fatalf("seed: %v", err) } - if _, _, err := claimEnvRoot(envRoot, "aaaaaaaa-1111-2222-3333-0123456789ab"); err == nil { + if _, _, err := claimEnvRoot(envRoot, "ws", "aaaaaaaa-1111-2222-3333-0123456789ab"); err == nil { t.Fatal("claimEnvRoot took a directory holding files with no owner") } else if !strings.Contains(err.Error(), "names no owning task") { t.Fatalf("error = %v, want it to explain the missing owner", err) @@ -6459,7 +6851,7 @@ func TestClaimEnvRootAdoptsEmptyDirectory(t *testing.T) { t.Fatalf("seed: %v", err) } const id = "aaaaaaaa-1111-2222-3333-0123456789ab" - lock, reset, err := claimEnvRoot(envRoot, id) + lock, reset, err := claimEnvRoot(envRoot, "ws", id) if err != nil { t.Fatalf("claimEnvRoot on an empty directory: %v", err) } @@ -6578,7 +6970,7 @@ func TestClaimEnvRootRepairsTornOwnerMarker(t *testing.T) { } const id = "aaaaaaaa-1111-2222-3333-0123456789ab" - lock, _, err := claimEnvRoot(envRoot, id) + lock, _, err := claimEnvRoot(envRoot, "ws", id) if err != nil { t.Fatalf("claimEnvRoot wedged on a torn marker: %v", err) } @@ -6588,6 +6980,80 @@ func TestClaimEnvRootRepairsTornOwnerMarker(t *testing.T) { } } +// TestWriteEnvRootOwnerAtomicallyReplacesMarker pins the write protocol used +// when a legacy task-only marker is upgraded. Rewriting the file in place can +// expose empty or partial JSON to disk-usage readers and permanently wedge the +// root after a crash. Renaming a complete same-directory temp file changes the +// file identity while leaving only a fully parseable marker at the public path. +func TestWriteEnvRootOwnerAtomicallyReplacesMarker(t *testing.T) { + t.Parallel() + envRoot := t.TempDir() + ownerPath := filepath.Join(envRoot, envRootOwnerFile) + const taskID = "aaaaaaaa-1111-2222-3333-0123456789ab" + if err := os.WriteFile(ownerPath, []byte(taskID), 0o644); err != nil { + t.Fatalf("seed legacy owner: %v", err) + } + before, err := os.Stat(ownerPath) + if err != nil { + t.Fatalf("stat legacy owner: %v", err) + } + + if err := writeEnvRootOwner(envRoot, "ws-authoritative", taskID); err != nil { + t.Fatalf("upgrade owner: %v", err) + } + after, err := os.Stat(ownerPath) + if err != nil { + t.Fatalf("stat upgraded owner: %v", err) + } + if os.SameFile(before, after) { + t.Fatal("owner marker was rewritten in place instead of atomically replaced") + } + + owner, err := ReadEnvRootOwner(envRoot) + if err != nil { + t.Fatalf("read upgraded owner: %v", err) + } + if owner.WorkspaceID != "ws-authoritative" || owner.TaskID != taskID { + t.Fatalf("owner = %#v, want authoritative workspace and task identity", owner) + } + entries, err := os.ReadDir(envRoot) + if err != nil { + t.Fatalf("read env root: %v", err) + } + if len(entries) != 1 || entries[0].Name() != envRootOwnerFile { + t.Fatalf("env root entries = %v, want only %s", entries, envRootOwnerFile) + } +} + +func TestClaimEnvRootRecoversOwnerTempLeftBeforeRename(t *testing.T) { + t.Parallel() + envRoot := t.TempDir() + staleTemp := filepath.Join(envRoot, envRootOwnerTempPrefix+"crashed"+envRootOwnerTempSuffix) + if err := os.WriteFile(staleTemp, []byte(`{"workspace_id":"ws"`), 0o600); err != nil { + t.Fatalf("seed unpublished owner temp: %v", err) + } + + const taskID = "aaaaaaaa-1111-2222-3333-0123456789ab" + lock, reset, err := claimEnvRoot(envRoot, "ws", taskID) + if err != nil { + t.Fatalf("claim after interrupted owner write: %v", err) + } + defer releaseLockFile(lock) + if reset { + t.Fatal("recovering an unpublished owner write should not reset the env root") + } + if _, err := os.Stat(staleTemp); !os.IsNotExist(err) { + t.Fatalf("stale owner temp still exists: %v", err) + } + owner, err := ReadEnvRootOwner(envRoot) + if err != nil { + t.Fatalf("read recovered owner: %v", err) + } + if owner.WorkspaceID != "ws" || owner.TaskID != taskID { + t.Fatalf("owner = %#v, want recovered workspace and task identity", owner) + } +} + // TestReleaseLockFreesEnvRootForALaterDispatch pins the lifetime rule that // Windows CI surfaced: the lock belongs to the task EXECUTION, and nothing in // production calls Environment.Cleanup — the GC reclaims env roots on its own diff --git a/server/internal/daemon/execenv/isolation_test.go b/server/internal/daemon/execenv/isolation_test.go index 17d3beb46..19997cc03 100644 --- a/server/internal/daemon/execenv/isolation_test.go +++ b/server/internal/daemon/execenv/isolation_test.go @@ -277,7 +277,14 @@ func TestPrepareIsolatedKeepsTheClaimWithTheParent(t *testing.T) { taskID = "01a01ec0-e69d-7000-8000-0123456789ab" ) - claim, err := ClaimEnvRoot(workspacesRoot, workspaceID, taskID) + rootParams := RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: workspaceID, + WorkspaceSlug: "Readable Workspace", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + } + claim, err := ClaimEnvRoot(rootParams) if err != nil { t.Fatalf("parent claim: %v", err) } @@ -286,7 +293,9 @@ func TestPrepareIsolatedKeepsTheClaimWithTheParent(t *testing.T) { env, err := PrepareIsolated(context.Background(), preparationHelperTestCommand(), PrepareParams{ WorkspacesRoot: workspacesRoot, WorkspaceID: workspaceID, + WorkspaceSlug: "Readable Workspace", TaskID: taskID, + IssueIdentifier: "MUL-6063", AgentName: "Isolated", EnvRootPreclaimed: true, Task: TaskContextForEnv{IssueID: taskID}, @@ -297,17 +306,20 @@ func TestPrepareIsolatedKeepsTheClaimWithTheParent(t *testing.T) { if env == nil || env.WorkDir == "" { t.Fatal("PrepareIsolated returned no environment") } + if env.RootDir != claim.RootDir() { + t.Fatalf("helper prepared %q while parent claimed %q", env.RootDir, claim.RootDir()) + } // The helper has exited. If the claim had been taken inside it, the lock // would be gone and this second claim would succeed. - if second, err := ClaimEnvRoot(workspacesRoot, workspaceID, taskID); err == nil { + if second, err := ClaimEnvRoot(rootParams); err == nil { second.Release() t.Fatal("production PrepareIsolated returned without retaining the execution lock") } // And releasing it must hand the env root back for a later dispatch. claim.Release() - next, err := ClaimEnvRoot(workspacesRoot, workspaceID, taskID) + next, err := ClaimEnvRoot(rootParams) if err != nil { t.Fatalf("env root stayed locked after release: %v", err) } @@ -380,7 +392,7 @@ func TestPrepareIsolatedFailsLoudlyWhenPreclaimIsNotDeclared(t *testing.T) { taskID = "01a01ec0-e69d-7000-8000-0123456789ab" ) - claim, err := ClaimEnvRoot(workspacesRoot, workspaceID, taskID) + claim, err := ClaimEnvRoot(RootDirParams{WorkspacesRoot: workspacesRoot, WorkspaceID: workspaceID, TaskID: taskID}) if err != nil { t.Fatalf("parent claim: %v", err) } diff --git a/server/internal/daemon/execenv/isolation_unix_test.go b/server/internal/daemon/execenv/isolation_unix_test.go index e4fb04af8..7a95ea9e8 100644 --- a/server/internal/daemon/execenv/isolation_unix_test.go +++ b/server/internal/daemon/execenv/isolation_unix_test.go @@ -49,7 +49,7 @@ func TestPrepareIsolated_PermanentFIFOBlockThenImmediateRetry(t *testing.T) { if elapsed := time.Since(startedAt); elapsed > 3*time.Second { t.Fatalf("PrepareIsolated took %s, want a bounded process termination", elapsed) } - if _, err := os.Stat(PredictRootDir(params.WorkspacesRoot, params.WorkspaceID, params.TaskID)); err != nil { + if _, err := os.Stat(PredictRootDir(RootDirParams{WorkspacesRoot: params.WorkspacesRoot, WorkspaceID: params.WorkspaceID, WorkspaceSlug: params.WorkspaceSlug, TaskID: params.TaskID, IssueIdentifier: params.IssueIdentifier})); err != nil { t.Fatalf("first helper did not reach environment preparation before timeout: %v", err) } @@ -79,7 +79,7 @@ func TestPrepareIsolated_PermanentFIFOBlockThenImmediateRetry(t *testing.T) { if err != nil { t.Fatalf("immediate retry PrepareIsolated: %v", err) } - if env == nil || env.RootDir != PredictRootDir(params.WorkspacesRoot, params.WorkspaceID, params.TaskID) { + if env == nil || env.RootDir != PredictRootDir(RootDirParams{WorkspacesRoot: params.WorkspacesRoot, WorkspaceID: params.WorkspaceID, WorkspaceSlug: params.WorkspaceSlug, TaskID: params.TaskID, IssueIdentifier: params.IssueIdentifier}) { t.Fatalf("retry environment = %#v, want the predicted root", env) } } diff --git a/server/internal/daemon/execenv/isolation_windows_test.go b/server/internal/daemon/execenv/isolation_windows_test.go index 56f5b4192..cf047dd0d 100644 --- a/server/internal/daemon/execenv/isolation_windows_test.go +++ b/server/internal/daemon/execenv/isolation_windows_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "time" ) @@ -92,11 +93,13 @@ func TestPrepareIsolated_WindowsKillsDescendantBeforeRetry(t *testing.T) { ready, } params := PrepareParams{ - WorkspacesRoot: filepath.Join(dir, "workspaces"), - WorkspaceID: "ws-windows-job", - TaskID: "12345678-1111-2222-3333-444444444444", - Provider: "claude", - Task: TaskContextForEnv{IssueID: "issue-windows-job"}, + WorkspacesRoot: filepath.Join(dir, "workspaces"), + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: strings.Repeat("workspace", 20), + TaskID: "12345678-1111-7222-8333-444444444444", + IssueIdentifier: strings.Repeat("issue", 20), + Provider: "claude", + Task: TaskContextForEnv{IssueID: "issue-windows-job"}, } ctx, cancel := context.WithCancel(context.Background()) @@ -136,7 +139,7 @@ func TestPrepareIsolated_WindowsKillsDescendantBeforeRetry(t *testing.T) { if err != nil { t.Fatalf("immediate retry PrepareIsolated: %v", err) } - if env == nil || env.RootDir != PredictRootDir(params.WorkspacesRoot, params.WorkspaceID, params.TaskID) { + if env == nil || env.RootDir != PredictRootDir(RootDirParams{WorkspacesRoot: params.WorkspacesRoot, WorkspaceID: params.WorkspaceID, WorkspaceSlug: params.WorkspaceSlug, TaskID: params.TaskID, IssueIdentifier: params.IssueIdentifier}) { t.Fatalf("retry environment = %#v, want the predicted root", env) } } diff --git a/server/internal/daemon/execenv/root_identity.go b/server/internal/daemon/execenv/root_identity.go new file mode 100644 index 000000000..6e71cf490 --- /dev/null +++ b/server/internal/daemon/execenv/root_identity.go @@ -0,0 +1,365 @@ +package execenv + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +const ( + taskRootIndexDir = ".task_roots" + taskRootRecordFile = "root.json" + // taskRootPendingPrefix names the staging directory installTaskRootRecord + // renames into place. A directory still carrying it never became + // authoritative for any task. + taskRootPendingPrefix = ".pending-" + taskRootIndexMinPruneAge = time.Hour +) + +type taskRootRecord struct { + WorkspaceID string `json:"workspace_id"` + TaskID string `json:"task_id"` + RelativePath string `json:"relative_path"` +} + +// ResolveRootDir returns the one physical env root assigned to a task. The +// first caller freezes the readable path in an index keyed only by stable IDs; +// later claims keep that path even when display labels are added or renamed. +func ResolveRootDir(params RootDirParams) (string, error) { + proposed := PredictRootDir(params) + if proposed == "" { + return "", nil + } + + recordDir := taskRootRecordDir(params) + record, err := readTaskRootRecord(recordDir) + if err == nil { + return validateTaskRootRecord(params, recordDir, record) + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + + candidate, err := findOwnedTaskRoot(params) + if err != nil { + return "", err + } + if candidate == "" { + candidate = proposed + } + relative, err := filepath.Rel(params.WorkspacesRoot, candidate) + if err != nil { + return "", fmt.Errorf("execenv: make task root relative: %w", err) + } + record = taskRootRecord{ + WorkspaceID: params.WorkspaceID, + TaskID: params.TaskID, + RelativePath: relative, + } + if err := installTaskRootRecord(recordDir, record); err != nil { + return "", err + } + + // Another claimant may have won the atomic install with different readable + // labels. Always re-read the authoritative record instead of returning our + // proposal. + record, err = readTaskRootRecord(recordDir) + if err != nil { + return "", err + } + return validateTaskRootRecord(params, recordDir, record) +} + +func taskRootRecordDir(params RootDirParams) string { + return filepath.Join( + params.WorkspacesRoot, + taskRootIndexDir, + stableIdentityKey(params.WorkspaceID+"\x00"+params.TaskID), + ) +} + +func stableIdentityKey(value string) string { + sum := sha256.Sum256([]byte(value)) + return hex.EncodeToString(sum[:16]) +} + +func readTaskRootRecord(recordDir string) (taskRootRecord, error) { + recordPath := filepath.Join(recordDir, taskRootRecordFile) + data, err := os.ReadFile(recordPath) + if err != nil { + return taskRootRecord{}, err + } + var record taskRootRecord + if err := json.Unmarshal(data, &record); err != nil { + return taskRootRecord{}, fmt.Errorf("execenv: decode task root record %s: %w", recordPath, err) + } + return record, nil +} + +func installTaskRootRecord(recordDir string, record taskRootRecord) error { + parent := filepath.Dir(recordDir) + if err := os.MkdirAll(parent, 0o755); err != nil { + return fmt.Errorf("execenv: create task root index: %w", err) + } + tmpDir, err := os.MkdirTemp(parent, taskRootPendingPrefix) + if err != nil { + return fmt.Errorf("execenv: create task root record: %w", err) + } + defer os.RemoveAll(tmpDir) + data, err := json.Marshal(record) + if err != nil { + return fmt.Errorf("execenv: encode task root record: %w", err) + } + if err := os.WriteFile(filepath.Join(tmpDir, taskRootRecordFile), data, 0o644); err != nil { + return fmt.Errorf("execenv: write task root record: %w", err) + } + if err := os.Rename(tmpDir, recordDir); err != nil { + // A complete non-empty directory is installed atomically. If it exists, + // another claimant won and its record is authoritative. + if _, readErr := readTaskRootRecord(recordDir); readErr == nil { + return nil + } + return fmt.Errorf("execenv: install task root record: %w", err) + } + return nil +} + +// validateTaskRootRecord fails closed on anything it cannot vouch for: a task +// that cannot prove which root is its own must not fall back to proposing a +// fresh one, because the original may still hold live work. That refusal is +// permanent until an operator intervenes, so every error names recordDir — +// the one path they need to inspect or remove. +func validateTaskRootRecord(params RootDirParams, recordDir string, record taskRootRecord) (string, error) { + if record.WorkspaceID != params.WorkspaceID || record.TaskID != params.TaskID { + return "", fmt.Errorf("execenv: task root record %s belongs to workspace %s task %s, not workspace %s task %s", + recordDir, record.WorkspaceID, record.TaskID, params.WorkspaceID, params.TaskID) + } + relative := filepath.Clean(record.RelativePath) + if relative == "." || filepath.IsAbs(relative) { + return "", fmt.Errorf("execenv: task root record %s holds invalid relative path %q", recordDir, record.RelativePath) + } + parts := strings.Split(relative, string(filepath.Separator)) + if len(parts) != 2 || parts[0] == "" || parts[1] == "" || parts[0] == ".." || parts[1] == ".." { + return "", fmt.Errorf("execenv: task root record %s holds invalid relative path %q", recordDir, record.RelativePath) + } + if !validTaskRootSegment(parts[0], params.WorkspaceID, true) || !validTaskRootSegment(parts[1], params.TaskID, false) { + return "", fmt.Errorf("execenv: task root record %s points at %q, which does not match its stable identity", recordDir, record.RelativePath) + } + return filepath.Join(params.WorkspacesRoot, relative), nil +} + +func validTaskRootSegment(segment, id string, workspace bool) bool { + segment = strings.ToLower(segment) + id = strings.ToLower(id) + key := strings.ToLower(taskKey(id)) + if workspace && segment == id { + return true + } + if !workspace && segment == key { + return true + } + return strings.HasSuffix(segment, "-"+key) +} + +// RemoveRootDirRecord removes the stable index after GC has reclaimed a +// terminal task root. It verifies that the record still points at envRoot so a +// stale cleanup can never remove another task's identity. +func RemoveRootDirRecord(workspacesRoot, envRoot string, owner EnvRootOwner) error { + if workspacesRoot == "" || envRoot == "" || owner.WorkspaceID == "" || owner.TaskID == "" { + return nil + } + params := RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: owner.WorkspaceID, + TaskID: owner.TaskID, + } + recordDir := taskRootRecordDir(params) + record, err := readTaskRootRecord(recordDir) + if errors.Is(err, os.ErrNotExist) { + return nil + } + if err != nil { + return err + } + resolved, err := validateTaskRootRecord(params, recordDir, record) + if err != nil { + return err + } + if filepath.Clean(resolved) != filepath.Clean(envRoot) { + return fmt.Errorf("execenv: task root record points to %s, not reclaimed root %s", resolved, envRoot) + } + if err := os.RemoveAll(recordDir); err != nil { + return fmt.Errorf("execenv: remove task root record: %w", err) + } + // The index directory itself is deliberately left in place. Removing it + // when the last record goes would race installTaskRootRecord, which does + // MkdirAll(parent) and then MkdirTemp(parent, ...): a removal landing + // between those two calls hands the claim an ENOENT and fails the task, + // which is a poor trade for one empty directory. + return nil +} + +// PruneTaskRootIndex removes abandoned unpublished directories and stable +// records whose physical env root never materialized. Complete records are +// removed only when eligible confirms the task is terminal or inaccessible; +// a live/non-terminal task keeps its frozen path even when the root is absent. +// +// A minimum grace period protects the tiny publication window even when the +// operator configures the general orphan TTL as zero. +func PruneTaskRootIndex(workspacesRoot string, retention time.Duration, now time.Time, eligible func(workspaceID, taskID string) bool) (removed int, err error) { + indexDir := filepath.Join(workspacesRoot, taskRootIndexDir) + entries, readErr := os.ReadDir(indexDir) + if errors.Is(readErr, os.ErrNotExist) { + return 0, nil + } + if readErr != nil { + return 0, fmt.Errorf("execenv: read task root index: %w", readErr) + } + + pruneAge := retention + if pruneAge < taskRootIndexMinPruneAge { + pruneAge = taskRootIndexMinPruneAge + } + var errs []error + for _, entry := range entries { + if !entry.IsDir() { + continue + } + entryPath := filepath.Join(indexDir, entry.Name()) + age, ok := taskRootRecordAge(entryPath, now) + if !ok || age <= pruneAge { + continue + } + + if strings.HasPrefix(entry.Name(), taskRootPendingPrefix) { + if removeErr := os.RemoveAll(entryPath); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("remove stale task root pending entry %s: %w", entryPath, removeErr)) + continue + } + removed++ + continue + } + + record, recordErr := readTaskRootRecord(entryPath) + if errors.Is(recordErr, os.ErrNotExist) { + if removeErr := os.RemoveAll(entryPath); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("remove stale empty task root record %s: %w", entryPath, removeErr)) + continue + } + removed++ + continue + } + if recordErr != nil { + errs = append(errs, fmt.Errorf("read stale task root record %s: %w", entryPath, recordErr)) + continue + } + params := RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: record.WorkspaceID, + TaskID: record.TaskID, + } + resolved, validateErr := validateTaskRootRecord(params, entryPath, record) + if validateErr != nil { + errs = append(errs, fmt.Errorf("validate stale task root record %s: %w", entryPath, validateErr)) + continue + } + if _, statErr := os.Stat(resolved); statErr == nil { + continue + } else if !errors.Is(statErr, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("inspect env root for record %s: %w", entryPath, statErr)) + continue + } + if eligible == nil || !eligible(record.WorkspaceID, record.TaskID) { + continue + } + if removeErr := os.RemoveAll(entryPath); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("remove stale task root record %s: %w", entryPath, removeErr)) + continue + } + removed++ + } + return removed, errors.Join(errs...) +} + +// findOwnedTaskRoot adopts roots created before the stable index existed. The +// owner marker is authoritative; readable suffixes only narrow the scan. +func findOwnedTaskRoot(params RootDirParams) (string, error) { + workspaceEntries, err := os.ReadDir(params.WorkspacesRoot) + if errors.Is(err, os.ErrNotExist) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("execenv: scan existing task roots: %w", err) + } + workspaceSuffix := strings.ToLower(taskKey(params.WorkspaceID)) + var found string + for _, workspaceEntry := range workspaceEntries { + if !workspaceEntry.IsDir() || strings.HasPrefix(workspaceEntry.Name(), ".") { + continue + } + name := workspaceEntry.Name() + if name != params.WorkspaceID && !strings.HasSuffix(strings.ToLower(name), "-"+workspaceSuffix) { + continue + } + workspaceDir := filepath.Join(params.WorkspacesRoot, name) + taskEntries, readErr := os.ReadDir(workspaceDir) + if readErr != nil { + return "", fmt.Errorf("execenv: read candidate workspace root %s: %w", workspaceDir, readErr) + } + for _, taskEntry := range taskEntries { + if !taskEntry.IsDir() { + continue + } + taskName := strings.ToLower(taskEntry.Name()) + taskSuffix := strings.ToLower(taskKey(params.TaskID)) + if taskName != taskSuffix && !strings.HasSuffix(taskName, "-"+taskSuffix) { + continue + } + candidate := filepath.Join(workspaceDir, taskEntry.Name()) + owner, readErr := ReadEnvRootOwner(candidate) + if readErr != nil { + return "", fmt.Errorf("execenv: read candidate env root owner for %s: %w", candidate, readErr) + } + if owner.TaskID == "" { + hasWork, inspectErr := envRootHoldsWork(candidate) + if inspectErr != nil { + return "", fmt.Errorf("execenv: inspect candidate env root %s: %w", candidate, inspectErr) + } + if hasWork { + return "", fmt.Errorf("execenv: candidate env root %s holds files but has no owner", candidate) + } + } else if owner.TaskID != params.TaskID { + continue + } + if owner.WorkspaceID != "" && owner.WorkspaceID != params.WorkspaceID { + continue + } + if found != "" && found != candidate { + return "", fmt.Errorf("execenv: task %s owns multiple env roots", params.TaskID) + } + found = candidate + } + } + return found, nil +} + +// taskRootRecordAge dates a record by its file, which is written once inside a +// staging directory and never rewritten, so its mtime is the install time. A +// staging directory that died before the write has no file; fall back to the +// directory itself so those are still reclaimable. +func taskRootRecordAge(recordDir string, now time.Time) (time.Duration, bool) { + if info, err := os.Stat(filepath.Join(recordDir, taskRootRecordFile)); err == nil { + return now.Sub(info.ModTime()), true + } + info, err := os.Stat(recordDir) + if err != nil { + return 0, false + } + return now.Sub(info.ModTime()), true +} diff --git a/server/internal/daemon/execenv/root_identity_test.go b/server/internal/daemon/execenv/root_identity_test.go new file mode 100644 index 000000000..9e32e6130 --- /dev/null +++ b/server/internal/daemon/execenv/root_identity_test.go @@ -0,0 +1,276 @@ +package execenv + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +const ( + pruneWorkspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + pruneTTL = 72 * time.Hour +) + +func pruneParams(root, taskID string) RootDirParams { + return RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: pruneWorkspaceID, + WorkspaceSlug: "Asset Feed", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + } +} + +func pruneTaskRootIndexForTest(t *testing.T, root string, now time.Time, eligible func(string, string) bool) int { + t.Helper() + removed, err := PruneTaskRootIndex(root, pruneTTL, now, eligible) + if err != nil { + t.Fatalf("PruneTaskRootIndex: %v", err) + } + return removed +} + +// TestPruneTaskRootIndexKeepsTheFreezeWindow is the guard that matters most +// here. ResolveRootDir installs a record BEFORE Prepare creates the directory, +// so "record with no env root" is the correct state for the window in between. +// A sweep that treated absence alone as garbage would delete a live task's +// frozen identity and hand the same task a second physical root — the exact +// orphaning the index exists to prevent. +func TestPruneTaskRootIndexKeepsTheFreezeWindow(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := pruneParams(root, "5c57b65b-ee7a-4603-a72d-b659c34a1dc3") + frozen, err := ResolveRootDir(params) + if err != nil { + t.Fatalf("ResolveRootDir: %v", err) + } + if _, err := os.Stat(frozen); !os.IsNotExist(err) { + t.Fatalf("fixture must represent the pre-Prepare window; stat err = %v", err) + } + + if removed := pruneTaskRootIndexForTest(t, root, time.Now(), func(string, string) bool { return true }); removed != 0 { + t.Fatalf("removed %d records inside the freeze window, want 0", removed) + } + again, err := ResolveRootDir(params) + if err != nil { + t.Fatalf("ResolveRootDir after prune: %v", err) + } + if again != frozen { + t.Fatalf("task moved from %q to %q after a prune cycle", frozen, again) + } +} + +func TestPruneTaskRootIndexReclaimsOnlyStrandedRecords(t *testing.T) { + t.Parallel() + + root := t.TempDir() + liveParams := pruneParams(root, "5c57b65b-ee7a-4603-a72d-b659c34a1dc3") + strandedParams := pruneParams(root, "5c57b65b-ee7a-4603-a72d-c760d45b2ed4") + + livePath, err := ResolveRootDir(liveParams) + if err != nil { + t.Fatalf("resolve live root: %v", err) + } + if err := os.MkdirAll(livePath, 0o755); err != nil { + t.Fatalf("create live root: %v", err) + } + // The stranded task got a record but never reached ClaimEnvRoot, so its + // root never materialised and cleanTaskDir will never see it. + if _, err := ResolveRootDir(strandedParams); err != nil { + t.Fatalf("resolve stranded root: %v", err) + } + + past := time.Now().Add(pruneTTL + time.Hour) + if removed := pruneTaskRootIndexForTest(t, root, past, func(string, string) bool { return true }); removed != 1 { + t.Fatalf("removed %d records, want exactly the stranded one", removed) + } + if _, err := os.Stat(taskRootRecordDir(strandedParams)); !os.IsNotExist(err) { + t.Fatalf("stranded record survived the sweep; stat err = %v", err) + } + resolved, err := ResolveRootDir(liveParams) + if err != nil { + t.Fatalf("resolve live root after sweep: %v", err) + } + if resolved != livePath { + t.Fatalf("live task moved from %q to %q after the sweep", livePath, resolved) + } +} + +// A record that cannot be parsed is what keeps its task failing closed. The +// sweep must not quietly convert that into "propose a fresh root", because the +// original root may still hold work. +func TestPruneTaskRootIndexKeepsUnreadableRecords(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := pruneParams(root, "5c57b65b-ee7a-4603-a72d-b659c34a1dc3") + recordDir := taskRootRecordDir(params) + if err := os.MkdirAll(recordDir, 0o755); err != nil { + t.Fatalf("seed record dir: %v", err) + } + if err := os.WriteFile(filepath.Join(recordDir, taskRootRecordFile), []byte("{not json"), 0o644); err != nil { + t.Fatalf("seed corrupt record: %v", err) + } + + past := time.Now().Add(pruneTTL + time.Hour) + removed, err := PruneTaskRootIndex(root, pruneTTL, past, func(string, string) bool { return true }) + if err == nil { + t.Fatal("PruneTaskRootIndex did not report the unreadable record") + } + if removed != 0 { + t.Fatalf("removed %d unreadable records, want 0", removed) + } + if _, err := os.Stat(filepath.Join(recordDir, taskRootRecordFile)); err != nil { + t.Fatalf("unreadable record was reclaimed: %v", err) + } +} + +// installTaskRootRecord stages into .pending-* and renames. A staging directory +// that outlives the gate is an install that died before the rename, so it was +// never authoritative for any task. +func TestPruneTaskRootIndexReclaimsAbandonedStagingDirs(t *testing.T) { + t.Parallel() + + root := t.TempDir() + indexDir := filepath.Join(root, taskRootIndexDir) + if err := os.MkdirAll(indexDir, 0o755); err != nil { + t.Fatalf("seed index: %v", err) + } + pending, err := os.MkdirTemp(indexDir, taskRootPendingPrefix) + if err != nil { + t.Fatalf("seed staging dir: %v", err) + } + if err := os.WriteFile(filepath.Join(pending, taskRootRecordFile), []byte(`{"workspace_id":"w"}`), 0o644); err != nil { + t.Fatalf("seed staging record: %v", err) + } + + if removed := pruneTaskRootIndexForTest(t, root, time.Now(), nil); removed != 0 { + t.Fatalf("removed %d staging dirs inside the gate, want 0", removed) + } + past := time.Now().Add(pruneTTL + time.Hour) + if removed := pruneTaskRootIndexForTest(t, root, past, nil); removed != 1 { + t.Fatalf("removed %d staging dirs past the gate, want 1", removed) + } + if _, err := os.Stat(pending); !os.IsNotExist(err) { + t.Fatalf("abandoned staging dir survived; stat err = %v", err) + } +} + +func TestPruneTaskRootIndexReclaimsEmptyFinalRecordDir(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := pruneParams(root, "5c57b65b-ee7a-4603-a72d-b659c34a1dc3") + recordDir := taskRootRecordDir(params) + if err := os.MkdirAll(recordDir, 0o755); err != nil { + t.Fatalf("seed empty final record dir: %v", err) + } + + past := time.Now().Add(pruneTTL + time.Hour) + if removed := pruneTaskRootIndexForTest(t, root, past, nil); removed != 1 { + t.Fatalf("removed %d empty final record dirs, want 1", removed) + } + if _, err := os.Stat(recordDir); !os.IsNotExist(err) { + t.Fatalf("empty final record dir survived; stat err = %v", err) + } +} + +// writeEnvRootOwner stages into a temp file and renames. A crash in between +// leaves a root holding nothing but that temp — no task content, and no owner +// marker either. findOwnedTaskRoot inspects candidates without the env root +// lock, so it must read that root as adoptable rather than refusing it as +// "holds files but has no owner", which would wedge the task permanently. +func TestResolveRootDirAdoptsRootHoldingOnlyAnUnpublishedOwnerTemp(t *testing.T) { + t.Parallel() + + root := t.TempDir() + const taskID = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" + params := pruneParams(root, taskID) + original := PredictRootDir(params) + if err := os.MkdirAll(original, 0o755); err != nil { + t.Fatalf("seed env root: %v", err) + } + tmp, err := os.CreateTemp(original, envRootOwnerTempPrefix+"*"+envRootOwnerTempSuffix) + if err != nil { + t.Fatalf("seed unpublished owner temp: %v", err) + } + if _, err := tmp.WriteString(`{"workspace_id":"` + pruneWorkspaceID + `","task_id":"` + taskID + `"}`); err != nil { + t.Fatalf("write unpublished owner temp: %v", err) + } + tmp.Close() + if _, err := os.Stat(filepath.Join(original, envRootOwnerFile)); !os.IsNotExist(err) { + t.Fatalf("fixture must have no published owner marker; stat err = %v", err) + } + + resolved, err := ResolveRootDir(params) + if err != nil { + t.Fatalf("ResolveRootDir refused a root holding only an unpublished owner temp: %v", err) + } + if resolved != original { + t.Fatalf("resolved %q, want the existing root %q", resolved, original) + } + + claim, err := ClaimEnvRoot(params) + if err != nil { + t.Fatalf("ClaimEnvRoot: %v", err) + } + defer claim.Release() + owner, err := ReadEnvRootOwner(original) + if err != nil { + t.Fatalf("ReadEnvRootOwner: %v", err) + } + if owner.TaskID != taskID || owner.WorkspaceID != pruneWorkspaceID { + t.Fatalf("owner = %+v, want the claiming task's identity", owner) + } + entries, err := os.ReadDir(original) + if err != nil { + t.Fatalf("read env root: %v", err) + } + for _, e := range entries { + if strings.HasPrefix(e.Name(), envRootOwnerTempPrefix) { + t.Fatalf("claim left an unpublished owner temp behind: %s", e.Name()) + } + } +} + +// Every fail-closed error must name the record directory: the refusal is +// permanent until an operator removes it, and they need the path to do that. +func TestValidateTaskRootRecordErrorsNameTheRecordDir(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := pruneParams(root, "5c57b65b-ee7a-4603-a72d-b659c34a1dc3") + recordDir := taskRootRecordDir(params) + + cases := []struct { + name string + record taskRootRecord + }{ + { + name: "identity mismatch", + record: taskRootRecord{WorkspaceID: "other", TaskID: params.TaskID, RelativePath: filepath.Join("a", "b")}, + }, + { + name: "invalid relative path", + record: taskRootRecord{WorkspaceID: params.WorkspaceID, TaskID: params.TaskID, RelativePath: "."}, + }, + { + name: "outside stable identity", + record: taskRootRecord{WorkspaceID: params.WorkspaceID, TaskID: params.TaskID, RelativePath: filepath.Join("unrelated-workspace", "unrelated-task")}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := validateTaskRootRecord(params, recordDir, tc.record) + if err == nil { + t.Fatal("validateTaskRootRecord accepted an invalid record") + } + if !strings.Contains(err.Error(), recordDir) { + t.Fatalf("error %q does not name the record dir %q", err, recordDir) + } + }) + } +} diff --git a/server/internal/daemon/gc.go b/server/internal/daemon/gc.go index 48d35294d..5f19a6160 100644 --- a/server/internal/daemon/gc.go +++ b/server/internal/daemon/gc.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "sort" "strings" "time" @@ -69,12 +70,13 @@ type gcStats struct { // hermesMemoryStoresReclaimed is counted separately from storesReclaimed: // the two stores hold different things on different TTLs, so folding them // into one number would make either figure unreadable for an operator. - hermesMemoryStoresReclaimed int // per-agent Hermes memory stores reclaimed past their TTL - hermesSessionStoresReclaimed int // per-conversation Hermes session stores reclaimed past their TTL - repoCachesReclaimed int // bare repo caches under .repos evicted past their TTL - taskTempDirsReclaimed int // per-task temp dirs under the temp base reclaimed after their owning execution ended - bytesReclaimed int64 // total bytes freed in this cycle - byPattern map[string]int // configured basename or managed path label -> reclaim count + hermesMemoryStoresReclaimed int // per-agent Hermes memory stores reclaimed past their TTL + hermesSessionStoresReclaimed int // per-conversation Hermes session stores reclaimed past their TTL + repoCachesReclaimed int // bare repo caches under .repos evicted past their TTL + taskTempDirsReclaimed int // per-task temp dirs under the temp base reclaimed after their owning execution ended + taskRootIndexEntriesReclaimed int // abandoned stable-root records and unpublished entries reclaimed past the orphan TTL + bytesReclaimed int64 // total bytes freed in this cycle + byPattern map[string]int // configured basename or managed path label -> reclaim count } // runGC performs a single GC scan across all workspace directories. @@ -105,6 +107,26 @@ func (d *Daemon) runGC(ctx context.Context) { d.gcWorkspace(ctx, wsDir, stats) } + // Stable-root records are published before the physical env root so a + // re-dispatch cannot choose a different readable path. If preparation never + // reaches ClaimEnvRoot, no task directory exists for the normal GC walk to + // reclaim. Bound those records separately, but only after the orphan grace + // period and an authoritative terminal/not-found task check. + rootRecordsRemoved, rootRecordsErr := execenv.PruneTaskRootIndex(root, d.cfg.GCOrphanTTL, time.Now(), func(_ string, taskID string) bool { + if ctx.Err() != nil || d.client == nil { + return false + } + status, statusErr := d.client.GetTaskGCCheck(ctx, taskID) + if statusErr != nil { + return isAccessNotFound(statusErr) + } + return isAgentTaskTerminal(status.Status) + }) + if rootRecordsErr != nil { + d.logger.Warn("gc: prune task root index failed", "error", rootRecordsErr) + } + stats.taskRootIndexEntriesReclaimed += rootRecordsRemoved + // Prune stale worktree references from all bare repo caches, then evict the // caches nothing needs anymore. These live outside any workspace directory // and are never reclaimed by the task walk above. @@ -150,7 +172,7 @@ func (d *Daemon) runGC(ctx context.Context) { } } - if stats.cleaned > 0 || stats.orphaned > 0 || stats.artifactDirs > 0 || stats.storesReclaimed > 0 || stats.hermesMemoryStoresReclaimed > 0 || stats.hermesSessionStoresReclaimed > 0 || stats.repoCachesReclaimed > 0 || stats.taskTempDirsReclaimed > 0 { + if stats.cleaned > 0 || stats.orphaned > 0 || stats.artifactDirs > 0 || stats.storesReclaimed > 0 || stats.hermesMemoryStoresReclaimed > 0 || stats.hermesSessionStoresReclaimed > 0 || stats.repoCachesReclaimed > 0 || stats.taskTempDirsReclaimed > 0 || stats.taskRootIndexEntriesReclaimed > 0 { d.logger.Info("gc: cycle complete", "cleaned", stats.cleaned, "orphaned", stats.orphaned, @@ -162,6 +184,7 @@ func (d *Daemon) runGC(ctx context.Context) { "hermes_session_stores_reclaimed", stats.hermesSessionStoresReclaimed, "repo_caches_reclaimed", stats.repoCachesReclaimed, "task_temp_dirs_reclaimed", stats.taskTempDirsReclaimed, + "task_root_index_entries_reclaimed", stats.taskRootIndexEntriesReclaimed, "bytes_reclaimed", stats.bytesReclaimed, "by_pattern", stats.byPattern, ) @@ -177,7 +200,7 @@ func (d *Daemon) gcWorkspace(ctx context.Context, wsDir string, stats *gcStats) } cleanedHere := 0 - issueCandidates := make([]issueGCCandidate, 0, len(taskEntries)) + issueCandidatesByWorkspace := make(map[string][]issueGCCandidate) for _, entry := range taskEntries { if ctx.Err() != nil { return @@ -192,13 +215,25 @@ func (d *Daemon) gcWorkspace(ctx context.Context, wsDir string, stats *gcStats) } meta, metaErr := execenv.ReadGCMeta(taskDir) if metaErr == nil && meta.Kind == execenv.GCKindIssue && strings.TrimSpace(meta.IssueID) != "" { - issueCandidates = append(issueCandidates, issueGCCandidate{taskDir: taskDir, meta: meta}) - continue + if workspaceID := strings.TrimSpace(meta.WorkspaceID); workspaceID != "" { + issueCandidatesByWorkspace[workspaceID] = append( + issueCandidatesByWorkspace[workspaceID], + issueGCCandidate{taskDir: taskDir, meta: meta}, + ) + continue + } } action := d.shouldCleanTaskDir(ctx, taskDir) cleanedHere += d.applyGCAction(taskDir, action, stats) } - cleanedHere += d.gcWorkspaceIssues(ctx, filepath.Base(wsDir), issueCandidates, stats) + workspaceIDs := make([]string, 0, len(issueCandidatesByWorkspace)) + for workspaceID := range issueCandidatesByWorkspace { + workspaceIDs = append(workspaceIDs, workspaceID) + } + sort.Strings(workspaceIDs) + for _, workspaceID := range workspaceIDs { + cleanedHere += d.gcWorkspaceIssues(ctx, workspaceID, issueCandidatesByWorkspace[workspaceID], stats) + } // Remove the workspace directory itself if it's now empty. if cleanedHere > 0 { @@ -783,12 +818,18 @@ func isAgentTaskTerminal(status string) bool { // that count for the cycle summary. A failed removal reports zero reclaimed. func (d *Daemon) cleanTaskDir(taskDir string) int64 { bytes := dirSize(taskDir) + owner, ownerErr := execenv.ReadEnvRootOwner(taskDir) if err := os.RemoveAll(taskDir); err != nil { d.logger.Warn("gc: remove task dir failed", "dir", taskDir, "error", err) return 0 } else { d.logger.Info("gc: removed", "dir", taskDir, "bytes_reclaimed", bytes) } + if ownerErr == nil && owner != nil { + if err := execenv.RemoveRootDirRecord(d.cfg.WorkspacesRoot, taskDir, *owner); err != nil { + d.logger.Warn("gc: remove stable task root record failed", "dir", taskDir, "error", err) + } + } return bytes } diff --git a/server/internal/daemon/gc_test.go b/server/internal/daemon/gc_test.go index 0390ee8ce..df0ecf565 100644 --- a/server/internal/daemon/gc_test.go +++ b/server/internal/daemon/gc_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io/fs" "log/slog" "net/http" "net/http/httptest" @@ -57,6 +58,245 @@ func createTaskDir(t *testing.T, root, wsID, dirName string, meta *execenv.GCMet return taskDir } +// TestRunGC_StrandedTaskRootRecordsFollowTaskStatus guards the wiring AND the +// invariant that makes the sweep safe. +// +// A record whose env root never materialised has no other remover: cleanTaskDir +// only fires on a root it reclaims, and nothing else walks .task_roots. But +// absence of the root cannot authorise removal on its own — ResolveRootDir +// publishes the record BEFORE Prepare creates the directory, so a record that +// an aged-out re-dispatch is in the middle of reusing looks identical to a +// leftover. Deleting there hands the same task a second physical root, which is +// the orphaning the index exists to prevent. +// +// The age gate alone does not separate those two: an old record IS the state a +// stalled re-dispatch reads. Only the task's own status does, so the sweep asks +// the server and keeps anything non-terminal. +func TestRunGC_StrandedTaskRootRecordsFollowTaskStatus(t *testing.T) { + root := t.TempDir() + const workspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + params := func(taskID string) execenv.RootDirParams { + return execenv.RootDirParams{ + WorkspacesRoot: root, WorkspaceID: workspaceID, + WorkspaceSlug: "Asset Feed", TaskID: taskID, IssueIdentifier: "MUL-6063", + } + } + const ( + runningTask = "5c57b65b-ee7a-4603-a72d-c760d45b2ed4" + terminalTask = "5c57b65b-ee7a-4603-a72d-d871e56c3fe5" + liveRootTask = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" + ) + statuses := map[string]string{ + runningTask: "running", + terminalTask: "completed", + // liveRootTask is terminal too: only its surviving root protects it. + liveRootTask: "completed", + } + + mux := http.NewServeMux() + mux.HandleFunc("/api/daemon/tasks/", func(w http.ResponseWriter, r *http.Request) { + taskID := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/api/daemon/tasks/"), "/gc-check") + status, ok := statuses[taskID] + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + json.NewEncoder(w).Encode(map[string]any{"status": status}) + }) + + d := newGCTestDaemon(t, mux) + d.cfg.WorkspacesRoot = root + + frozen := map[string]string{} + for _, taskID := range []string{runningTask, terminalTask, liveRootTask} { + resolved, err := execenv.ResolveRootDir(params(taskID)) + if err != nil { + t.Fatalf("resolve root for %s: %v", taskID, err) + } + frozen[taskID] = resolved + } + // Only this one ever reached ClaimEnvRoot. + if err := os.MkdirAll(frozen[liveRootTask], 0o755); err != nil { + t.Fatalf("create live root: %v", err) + } + + past := time.Now().Add(-31 * 24 * time.Hour) + if err := filepath.WalkDir(filepath.Join(root, ".task_roots"), func(path string, _ fs.DirEntry, err error) error { + if err != nil { + return err + } + return os.Chtimes(path, past, past) + }); err != nil { + t.Fatalf("backdate task root index: %v", err) + } + + d.runGC(context.Background()) + + // Renaming is how we observe survival: a frozen record ignores the new + // labels, a reclaimed one re-proposes from them. + for _, tc := range []struct { + taskID string + want bool + why string + }{ + {runningTask, true, "a non-terminal task's frozen identity was swept; a re-dispatch would now get a second root"}, + {liveRootTask, true, "a record was swept while its env root still exists"}, + {terminalTask, false, "a terminal task's stranded record survived the sweep"}, + } { + renamed := params(tc.taskID) + renamed.WorkspaceSlug = "Renamed Workspace" + renamed.IssueIdentifier = "NEW-6063" + resolved, err := execenv.ResolveRootDir(renamed) + if err != nil { + t.Fatalf("resolve %s after GC: %v", tc.taskID, err) + } + if kept := resolved == frozen[tc.taskID]; kept != tc.want { + t.Errorf("task %s: record kept = %v, want %v — %s", tc.taskID, kept, tc.want, tc.why) + } + } +} + +func TestRunGC_MixedLayoutsUseMetadataWorkspaceIdentity(t *testing.T) { + workspaceID := "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + legacyIssueID := "11111111-1111-1111-1111-111111111111" + readableIssueID := "22222222-2222-2222-2222-222222222222" + var batchRequests int + + mux := http.NewServeMux() + mux.HandleFunc("/api/daemon/workspaces/"+workspaceID+"/issues/gc-check", func(w http.ResponseWriter, r *http.Request) { + batchRequests++ + var body struct { + IssueIDs []string `json:"issue_ids"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Fatalf("decode batch request: %v", err) + } + issues := make([]map[string]any, 0, len(body.IssueIDs)) + for _, issueID := range body.IssueIDs { + issues = append(issues, map[string]any{ + "id": issueID, "found": true, "status": "done", + "updated_at": time.Now().Add(-10 * 24 * time.Hour), + }) + } + json.NewEncoder(w).Encode(map[string]any{"issues": issues}) + }) + + d := newGCTestDaemon(t, mux) + legacyDir := createTaskDir(t, d.cfg.WorkspacesRoot, workspaceID, "11111111", &execenv.GCMeta{ + Kind: execenv.GCKindIssue, IssueID: legacyIssueID, WorkspaceID: workspaceID, + CompletedAt: time.Now().Add(-10 * 24 * time.Hour), + }) + readableDir := createTaskDir(t, d.cfg.WorkspacesRoot, "asset-feed-a548b2390cb2", "mul-6063-b659c34a1dc3", &execenv.GCMeta{ + Kind: execenv.GCKindIssue, IssueID: readableIssueID, WorkspaceID: workspaceID, + TaskID: "22222222-ee7a-4603-a72d-b659c34a1dc3", CompletedAt: time.Now().Add(-10 * 24 * time.Hour), + }) + + d.runGC(context.Background()) + + if batchRequests != 2 { + t.Fatalf("batch requests = %d, want one per physical layout", batchRequests) + } + for _, taskDir := range []string{legacyDir, readableDir} { + if _, err := os.Stat(taskDir); !os.IsNotExist(err) { + t.Fatalf("task dir %s was not reclaimed", taskDir) + } + } +} + +func TestRunGC_PrunesOnlyTerminalAbandonedTaskRootRecords(t *testing.T) { + t.Parallel() + + const ( + workspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" + terminalTask = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" + runningTask = "6d68c76c-ff8b-5704-b83e-c76ad45b2ed4" + unavailableTask = "7e79d87d-008c-6805-c94f-d87be56c3fe5" + ) + mux := http.NewServeMux() + mux.HandleFunc("/api/daemon/tasks/", func(w http.ResponseWriter, r *http.Request) { + status := "running" + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + if len(parts) >= 2 && parts[len(parts)-2] == unavailableTask { + http.Error(w, "temporarily unavailable", http.StatusServiceUnavailable) + return + } + if len(parts) >= 2 && parts[len(parts)-2] == terminalTask { + status = "failed" + } + json.NewEncoder(w).Encode(map[string]any{"status": status}) + }) + + d := newGCTestDaemon(t, mux) + d.cfg.GCOrphanTTL = time.Hour + terminalParams := execenv.RootDirParams{ + WorkspacesRoot: d.cfg.WorkspacesRoot, + WorkspaceID: workspaceID, + TaskID: terminalTask, + } + runningParams := terminalParams + runningParams.TaskID = runningTask + unavailableParams := terminalParams + unavailableParams.TaskID = unavailableTask + terminalRoot, err := execenv.ResolveRootDir(terminalParams) + if err != nil { + t.Fatalf("resolve terminal root: %v", err) + } + runningRoot, err := execenv.ResolveRootDir(runningParams) + if err != nil { + t.Fatalf("resolve running root: %v", err) + } + unavailableRoot, err := execenv.ResolveRootDir(unavailableParams) + if err != nil { + t.Fatalf("resolve unavailable root: %v", err) + } + + indexDir := filepath.Join(d.cfg.WorkspacesRoot, ".task_roots") + entries, err := os.ReadDir(indexDir) + if err != nil { + t.Fatalf("read task root index: %v", err) + } + old := time.Now().Add(-2 * time.Hour) + for _, entry := range entries { + recordPath := filepath.Join(indexDir, entry.Name(), "root.json") + if err := os.Chtimes(recordPath, old, old); err != nil { + t.Fatalf("age task root record: %v", err) + } + } + + d.runGC(t.Context()) + + terminalRenamed := terminalParams + terminalRenamed.WorkspaceSlug = "Renamed Workspace" + terminalRenamed.IssueIdentifier = "NEW-6063" + terminalAfterGC, err := execenv.ResolveRootDir(terminalRenamed) + if err != nil { + t.Fatalf("resolve terminal root after GC: %v", err) + } + if terminalAfterGC == terminalRoot { + t.Fatalf("terminal abandoned record still froze root %q", terminalAfterGC) + } + runningRenamed := runningParams + runningRenamed.WorkspaceSlug = "Renamed Workspace" + runningRenamed.IssueIdentifier = "NEW-6064" + runningAfterGC, err := execenv.ResolveRootDir(runningRenamed) + if err != nil { + t.Fatalf("resolve running root after GC: %v", err) + } + if runningAfterGC != runningRoot { + t.Fatalf("GC changed non-terminal task root from %q to %q", runningRoot, runningAfterGC) + } + unavailableRenamed := unavailableParams + unavailableRenamed.WorkspaceSlug = "Renamed Workspace" + unavailableRenamed.IssueIdentifier = "NEW-6065" + unavailableAfterGC, err := execenv.ResolveRootDir(unavailableRenamed) + if err != nil { + t.Fatalf("resolve unavailable root after GC: %v", err) + } + if unavailableAfterGC != unavailableRoot { + t.Fatalf("GC changed task root after an inconclusive status check from %q to %q", unavailableRoot, unavailableAfterGC) + } +} + func TestShouldCleanTaskDir_DoneIssueOverTTL(t *testing.T) { t.Parallel() issueID := "11111111-1111-1111-1111-111111111111" @@ -281,6 +521,43 @@ func TestCleanTaskDir_RemovesDirectory(t *testing.T) { } } +func TestCleanTaskDir_RemovesStableRootRecord(t *testing.T) { + t.Parallel() + + root := t.TempDir() + params := execenv.PrepareParams{ + WorkspacesRoot: root, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: "Asset Feed", + TaskID: "5c57b65b-ee7a-4603-a72d-b659c34a1dc3", + IssueIdentifier: "MUL-6063", + AgentName: "GC Identity", + } + env, err := execenv.Prepare(params, slog.Default()) + if err != nil { + t.Fatalf("Prepare: %v", err) + } + original := env.RootDir + d := &Daemon{cfg: Config{WorkspacesRoot: root}, logger: slog.Default()} + if bytes := d.cleanTaskDir(original); bytes <= 0 { + t.Fatalf("reclaimed bytes = %d, want owner metadata bytes", bytes) + } + + resolved, err := execenv.ResolveRootDir(execenv.RootDirParams{ + WorkspacesRoot: root, + WorkspaceID: params.WorkspaceID, + WorkspaceSlug: "Renamed Workspace", + TaskID: params.TaskID, + IssueIdentifier: "NEW-6063", + }) + if err != nil { + t.Fatalf("ResolveRootDir after terminal GC: %v", err) + } + if resolved == original { + t.Fatalf("terminal GC left stale root record %q", resolved) + } +} + func TestGcWorkspace_CleansEmptyWorkspaceDir(t *testing.T) { t.Parallel() issueID := "77777777-7777-7777-7777-777777777777" @@ -2059,6 +2336,9 @@ func TestGCMetaForTask(t *testing.T) { if meta.WorkspaceID != "ws" { t.Fatalf("workspace_id: want %q, got %q", "ws", meta.WorkspaceID) } + if meta.TaskID != tc.task.ID { + t.Fatalf("task_id: want %q, got %q", tc.task.ID, meta.TaskID) + } }) } diff --git a/server/internal/daemon/leader_workdir_reuse_test.go b/server/internal/daemon/leader_workdir_reuse_test.go index c695f224d..88010f2cd 100644 --- a/server/internal/daemon/leader_workdir_reuse_test.go +++ b/server/internal/daemon/leader_workdir_reuse_test.go @@ -32,6 +32,8 @@ func TestRunTaskSquadLeaderReusesWorkdirBeforeGCMetaWritten(t *testing.T) { defer cleanup() first := leaderReuseTestTask("task-first") + first.WorkspaceSlug = "original-workspace" + first.IssueIdentifier = "MUL-6063" firstResult, err := d.runTask(context.Background(), first, "claude", 0, d.logger) if err != nil { t.Fatalf("first runTask: %v", err) @@ -47,6 +49,10 @@ func TestRunTaskSquadLeaderReusesWorkdirBeforeGCMetaWritten(t *testing.T) { } second := leaderReuseTestTask("task-second") + // Labels are mutable display data. A renamed workspace or issue must keep + // reusing the recorded path rather than deriving a new root from the names. + second.WorkspaceSlug = "renamed-workspace" + second.IssueIdentifier = "NEW-6063" second.PriorSessionID = firstResult.SessionID second.PriorWorkDir = firstResult.WorkDir secondResult, err := d.runTask(context.Background(), second, "claude", 0, d.logger) diff --git a/server/internal/daemon/repocache/cache_windows_test.go b/server/internal/daemon/repocache/cache_windows_test.go index c9b2023ef..5316b7a1a 100644 --- a/server/internal/daemon/repocache/cache_windows_test.go +++ b/server/internal/daemon/repocache/cache_windows_test.go @@ -3,10 +3,14 @@ package repocache import ( + "crypto/sha256" + "encoding/hex" "os" "path/filepath" "strings" "testing" + + "github.com/multica-ai/multica/server/internal/daemon/execenv" ) // The isolated checkout exists so a Codex task can commit inside its own @@ -33,12 +37,41 @@ func TestIsolatedCheckoutIsCommittableOnWindows(t *testing.T) { baseRef := getRemoteDefaultBranch(barePath) baseCommit := gitRefCommit(t, barePath, baseRef) + // Exercise the production checkout under the longest readable env-root + // segments. This needs a real Windows runner: Git creates the task branch as + // a nested file below .git/refs/heads, where MAX_PATH applies to the complete + // path and a too-generous readable prefix fails with "Filename too long". + // Keep the base representative of the default user-level workspace root. + // t.TempDir embeds the full test name; nesting the production layout under + // it would spend that synthetic length twice and test the fixture, not the + // readable-path budget. Keep the short base, but derive the task suffix from + // this test's unique source path so concurrent go test processes cannot wipe + // each other's checkout while resetting the fixture. + workspacesRoot := filepath.Join(os.TempDir(), "mw") + taskSuffixHash := sha256.Sum256([]byte(sourceRepo)) + taskID := "a1b2c3d4-e5f6-7890-abcd-" + hex.EncodeToString(taskSuffixHash[:6]) + envRoot := execenv.PredictRootDir(execenv.RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: "a05b0e10-ee7a-4603-a72d-a548b2390cb2", + WorkspaceSlug: strings.Repeat("workspace", 20), + TaskID: taskID, + IssueIdentifier: strings.Repeat("issue", 20), + }) + if err := os.RemoveAll(envRoot); err != nil { + t.Fatalf("clear worst-case readable env root: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(envRoot) }) + workDir := filepath.Join(envRoot, "workdir") + if err := os.MkdirAll(workDir, 0o755); err != nil { + t.Fatalf("create worst-case readable workdir: %v", err) + } + result, err := cache.CreateWorktree(WorktreeParams{ WorkspaceID: "ws-1", RepoURL: sourceRepo, - WorkDir: t.TempDir(), + WorkDir: workDir, AgentName: "Windows Codex", - TaskID: "a1b2c3d4-e5f6-7890-abcd-ef1234567890", + TaskID: taskID, IsolatedGitMetadata: true, }) if err != nil { @@ -51,6 +84,10 @@ func TestIsolatedCheckoutIsCommittableOnWindows(t *testing.T) { if !isUnder(gitDir, result.Path) { t.Fatalf("git dir %s is outside the task checkout %s", gitDir, result.Path) } + taskRef := filepath.Join(gitDir, "refs", "heads", filepath.FromSlash(result.BranchName)) + if _, err := os.Stat(taskRef); err != nil { + t.Fatalf("task branch ref was not created at %s: %v", taskRef, err) + } // 2. Branch, stage, commit — the sequence that fails on a linked worktree. runGitAuthored(t, result.Path, "checkout", "-b", "feature/windows-commit") diff --git a/server/internal/daemon/types.go b/server/internal/daemon/types.go index 731652326..23c9a7078 100644 --- a/server/internal/daemon/types.go +++ b/server/internal/daemon/types.go @@ -86,6 +86,8 @@ type Task struct { RuntimeID string `json:"runtime_id"` IssueID string `json:"issue_id"` WorkspaceID string `json:"workspace_id"` + WorkspaceSlug string `json:"workspace_slug,omitempty"` + IssueIdentifier string `json:"issue_identifier,omitempty"` RemoteMCPConnections []remotemcp.Connection `json:"remote_mcp_connections,omitempty"` // RemoteMCPDaemonToken stays inside the daemon and authenticates the local // broker's credential-resolution calls. It must never enter agent env/config. diff --git a/server/internal/daemon/workdir_race_test.go b/server/internal/daemon/workdir_race_test.go index 0700d28e9..31ab640e3 100644 --- a/server/internal/daemon/workdir_race_test.go +++ b/server/internal/daemon/workdir_race_test.go @@ -94,7 +94,7 @@ func TestRunTask_StartTaskCalledAfterWorkdirOnDisk(t *testing.T) { workspacesRoot := t.TempDir() workspaceID := "ws-runtask" taskID := "task-runtask-after-mkdir" - expectedEnvRoot := execenv.PredictRootDir(workspacesRoot, workspaceID, taskID) + expectedEnvRoot := execenv.PredictRootDir(execenv.RootDirParams{WorkspacesRoot: workspacesRoot, WorkspaceID: workspaceID, TaskID: taskID}) expectedWorkDir := filepath.Join(expectedEnvRoot, "workdir") var ( @@ -168,7 +168,7 @@ func TestRunTask_InjectsPrivateTaskTempDir(t *testing.T) { workspacesRoot := filepath.Join(t.TempDir(), strings.Repeat("long-workspaces-root-", 3)) workspaceID := "ws-private-temp" taskID := "task-private-temp-with-long-id-that-would-overflow-socket-paths" - envRoot := execenv.PredictRootDir(workspacesRoot, workspaceID, taskID) + envRoot := execenv.PredictRootDir(execenv.RootDirParams{WorkspacesRoot: workspacesRoot, WorkspaceID: workspaceID, TaskID: taskID}) captureFile := filepath.Join(t.TempDir(), "agent-env.txt") fakeBin := filepath.Join(t.TempDir(), "claude") @@ -788,7 +788,7 @@ func TestHandleTask_KeepsEnvRootActiveAcrossCompletion(t *testing.T) { workspacesRoot := t.TempDir() workspaceID := "ws-active-during-complete" taskID := "task-active-during-complete" - expectedEnvRoot := execenv.PredictRootDir(workspacesRoot, workspaceID, taskID) + expectedEnvRoot := execenv.PredictRootDir(execenv.RootDirParams{WorkspacesRoot: workspacesRoot, WorkspaceID: workspaceID, TaskID: taskID}) var ( completeCalled atomic.Bool @@ -824,7 +824,7 @@ func TestHandleTask_KeepsEnvRootActiveAcrossCompletion(t *testing.T) { // the outer guard added in handleTask, the deferred unmark would bring // isActiveEnvRoot back to false before reportTaskResult fires. d.runner = taskRunnerFunc(func(_ context.Context, tk Task, _ string, _ int, _ *slog.Logger) (TaskResult, error) { - predicted := execenv.PredictRootDir(d.cfg.WorkspacesRoot, tk.WorkspaceID, tk.ID) + predicted := execenv.PredictRootDir(taskRootDirParams(d.cfg.WorkspacesRoot, tk)) d.markActiveEnvRoot(predicted) defer d.unmarkActiveEnvRoot(predicted) return TaskResult{ diff --git a/server/internal/handler/agent.go b/server/internal/handler/agent.go index 07c3a16e3..f09f425b6 100644 --- a/server/internal/handler/agent.go +++ b/server/internal/handler/agent.go @@ -359,6 +359,8 @@ type AgentTaskResponse struct { RuntimeID string `json:"runtime_id"` IssueID string `json:"issue_id"` WorkspaceID string `json:"workspace_id"` + WorkspaceSlug string `json:"workspace_slug,omitempty"` + IssueIdentifier string `json:"issue_identifier,omitempty"` RemoteMCPConnections []remotemcp.Connection `json:"remote_mcp_connections,omitempty"` // PluginHookTools are the workspace's agent-trigger plugin hooks, which the // daemon renders as MCP tools for this task. Resolved at claim time so @@ -421,8 +423,8 @@ type AgentTaskResponse struct { PriorSessionResumeUnavailable bool `json:"prior_session_resume_unavailable,omitempty"` WorkDir string `json:"work_dir,omitempty"` // local working directory pinned for this task; populated once the daemon reports it // RelativeWorkDir is a privacy-safe display form of WorkDir intended for - // the UI. For standard tasks it strips the daemon's workspaces root so - // the user sees `//workdir`; for local_directory + // the UI. For standard tasks it strips the daemon's workspaces root while + // preserving either the legacy or readable workspace/task segments; for local_directory // tasks the absolute path lives outside the envRoot layout, so we strip // recognised home-directory prefixes (`/Users//`, `/home//`, // `:/Users//`) and otherwise fall back to the basename so @@ -821,9 +823,11 @@ func taskToResponse(t db.AgentTaskQueue, workspaceID string) AgentTaskResponse { // rendered in transcripts that frequently end up in screen shares, // screenshots, and recordings, so this function is the only guard. // -// - For standard tasks (work_dir laid out as `// -// /workdir` by execenv.Prepare), it strips everything up to and -// including the workspaces root, returning `//workdir`. +// - For standard tasks, it validates the adjacent workspace/task segments +// by their stable ID suffixes, then strips everything before them. This +// accepts both legacy `/` roots and readable +// `-/-` roots without treating +// the labels as identity. // - For local_directory tasks the absolute path lives outside the envRoot // layout. We try to recognise common home-directory prefixes // (`/Users//`, `/home//`, `:/Users//`) and strip @@ -836,8 +840,8 @@ func taskToResponse(t db.AgentTaskQueue, workspaceID string) AgentTaskResponse { // (i.e. work_dir was exactly the user's home — rendering nothing is // preferable to a chip that says ``). taskDirSegment() must stay in // lock-step with server/internal/daemon/execenv/git.go:taskKey — both -// consume the same task UUID; if that helper changes, this one must too -// or the envRoot match silently degrades to the local_directory fallback. +// consume the same task UUID. legacyTaskDirSegment keeps privacy-safe display +// working for pre-#7347 roots and roots created by an earlier build of this PR. func relativeWorkDir(workDir, workspaceID, taskID string) string { if workDir == "" { return "" @@ -847,9 +851,12 @@ func relativeWorkDir(workDir, workspaceID, taskID string) string { normalized := strings.ReplaceAll(workDir, "\\", "/") if workspaceID != "" && taskID != "" { - envRootSuffix := workspaceID + "/" + taskDirSegment(taskID) - if idx := strings.Index(normalized, envRootSuffix); idx >= 0 { - return normalized[idx:] + parts := strings.Split(normalized, "/") + for i := 0; i+1 < len(parts); i++ { + if matchesWorkspacePathSegment(parts[i], workspaceID) && + matchesTaskPathSegment(parts[i+1], taskID) { + return strings.Join(parts[i:], "/") + } } } @@ -860,6 +867,22 @@ func relativeWorkDir(workDir, workspaceID, taskID string) string { return basename(normalized) } +func matchesWorkspacePathSegment(segment, workspaceID string) bool { + lower := strings.ToLower(segment) + legacy := legacyTaskDirSegment(workspaceID) + current := strings.ToLower(taskDirSegment(workspaceID)) + return strings.EqualFold(segment, workspaceID) || + strings.HasSuffix(lower, "-"+legacy) || strings.HasSuffix(lower, "-"+current) +} + +func matchesTaskPathSegment(segment, taskID string) bool { + lower := strings.ToLower(segment) + legacy := legacyTaskDirSegment(taskID) + current := strings.ToLower(taskDirSegment(taskID)) + return strings.EqualFold(segment, legacy) || strings.EqualFold(segment, current) || + strings.HasSuffix(lower, "-"+legacy) || strings.HasSuffix(lower, "-"+current) +} + // taskDirSegmentLen and taskDirSegment mirror execenv.taskKeyLen / // execenv.taskKey — the LAST 12 hex chars of the task id. Kept inline here so // the agent handler has zero imports from the daemon package (which would @@ -879,6 +902,16 @@ func taskDirSegment(uuid string) string { return s } +// legacyTaskDirSegment mirrors the historical shortID layout: the first eight +// dash-free characters. It is display compatibility only, never new identity. +func legacyTaskDirSegment(uuid string) string { + s := strings.ReplaceAll(uuid, "-", "") + if len(s) > 8 { + return strings.ToLower(s[:8]) + } + return strings.ToLower(s) +} + // homeDirPattern matches the well-known per-user home layouts on macOS, // Linux, and Windows after backslash normalization: // diff --git a/server/internal/handler/agent_work_dir_test.go b/server/internal/handler/agent_work_dir_test.go index 3775ed3aa..f69ef63ea 100644 --- a/server/internal/handler/agent_work_dir_test.go +++ b/server/internal/handler/agent_work_dir_test.go @@ -24,11 +24,12 @@ import ( func TestRelativeWorkDir(t *testing.T) { const ( wsID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" - taskID = "5c57b65b-ee7a-4603-a72d-a548b2390cb2" + taskID = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" // The env-root segment is the TAIL of the task id, not a leading // prefix: UUIDv7 puts a timestamp in front, so a leading slice is // shared by every task created in the same ~65.5s window (#7326). - taskSeg = "a548b2390cb2" + wsSeg = "a548b2390cb2" + taskSeg = "b659c34a1dc3" ) tests := []struct { @@ -59,6 +60,27 @@ func TestRelativeWorkDir(t *testing.T) { taskID: taskID, expected: wsID + "/" + taskSeg, }, + { + name: "readable envRoot path strips workspaces root", + workDir: "/Users/alice/multica_workspaces/asset-feed-" + wsSeg + "/mul-6063-" + taskSeg + "/workdir", + wsID: wsID, + taskID: taskID, + expected: "asset-feed-" + wsSeg + "/mul-6063-" + taskSeg + "/workdir", + }, + { + name: "legacy readable envRoot keeps privacy-safe display", + workDir: "/Users/alice/multica_workspaces/asset-feed-a05b0e10/mul-6063-5c57b65b/workdir", + wsID: wsID, + taskID: taskID, + expected: "asset-feed-a05b0e10/mul-6063-5c57b65b/workdir", + }, + { + name: "legacy opaque envRoot keeps privacy-safe display", + workDir: "/Users/alice/multica_workspaces/" + wsID + "/5c57b65b/workdir", + wsID: wsID, + taskID: taskID, + expected: wsID + "/5c57b65b/workdir", + }, { name: "local_directory path under /Users home is stripped", workDir: "/Users/df007df/repos/foo", @@ -207,22 +229,24 @@ func TestTaskToResponseDerivesPrivateDurableWorkDir(t *testing.T) { } } -// TestTaskDirSegmentMatchesDaemon pins taskDirSegment() to -// execenv.PredictRootDir's path layout. Both helpers consume the same task -// UUID; if the daemon's taskKey logic drifts, this test trips loudly instead -// of letting the UI silently fall back to the "tail two segments" branch. -// Without this guard, a daemon-side change back to a truncated prefix would -// not break a build — it would just quietly degrade every standard-task -// work_dir chip into the local_directory fallback. -func TestTaskDirSegmentMatchesDaemon(t *testing.T) { +// TestStableIDSuffixMatchesDaemon pins the handler's path validation to the +// current readable suffixes used by execenv.PredictRootDir. The table above +// separately pins compatibility with both historical layouts. +func TestStableIDSuffixMatchesDaemon(t *testing.T) { const ( workspacesRoot = "/tmp/workspaces" workspaceID = "a05b0e10-ee7a-4603-a72d-a548b2390cb2" - taskID = "5c57b65b-ee7a-4603-a72d-a548b2390cb2" + taskID = "5c57b65b-ee7a-4603-a72d-b659c34a1dc3" ) - daemonRoot := execenv.PredictRootDir(workspacesRoot, workspaceID, taskID) - expected := workspacesRoot + "/" + workspaceID + "/" + taskDirSegment(taskID) + daemonRoot := execenv.PredictRootDir(execenv.RootDirParams{ + WorkspacesRoot: workspacesRoot, + WorkspaceID: workspaceID, + WorkspaceSlug: "asset-feed", + TaskID: taskID, + IssueIdentifier: "MUL-6063", + }) + expected := workspacesRoot + "/asset-feed-" + taskDirSegment(workspaceID) + "/mul-6063-" + taskDirSegment(taskID) if daemonRoot != expected { - t.Fatalf("daemon PredictRootDir = %q, handler-side reconstruction = %q — taskDirSegment is out of sync with execenv.taskKey", daemonRoot, expected) + t.Fatalf("daemon PredictRootDir = %q, handler-side suffix expectation = %q", daemonRoot, expected) } } diff --git a/server/internal/handler/daemon.go b/server/internal/handler/daemon.go index 44f65a207..8d5a2fbbb 100644 --- a/server/internal/handler/daemon.go +++ b/server/internal/handler/daemon.go @@ -1979,6 +1979,7 @@ func claimResponseAgentIdentityMatches(resp AgentTaskResponse) bool { func (h *Handler) buildClaimedTaskResponse(r *http.Request, task *db.AgentTaskQueue, runtime db.AgentRuntime, runtimeID, runtimeWorkspaceID string) (resp AgentTaskResponse, deliveredCommentIDs []pgtype.UUID, agentSkillCount, builtinSkillCount int, failure *claimBuildFailure) { // Build response with fresh agent data (name + skills + custom_env + custom_args). resp = taskToResponse(*task, runtimeWorkspaceID) + var issueNumber int32 if task.IssueID.Valid { if policy, enabled := h.issueWindowPolicy(r.Context(), runtime.WorkspaceID); enabled { visible, visibilityErr := h.issueIDsWithinWindow(r.Context(), runtime.WorkspaceID, policy, []pgtype.UUID{task.IssueID}) @@ -2270,6 +2271,7 @@ func (h *Handler) buildClaimedTaskResponse(r *http.Request, task *db.AgentTaskQu return resp, deliveredCommentIDs, agentSkillCount, builtinSkillCount, failure } resp.ThreadName = issue.Title + issueNumber = issue.Number // Squad-leader briefing injection: keyed off the task being a // leader-task (is_leader_task) carrying a squad_id — NOT off the @@ -3081,6 +3083,10 @@ func (h *Handler) buildClaimedTaskResponse(r *http.Request, task *db.AgentTaskQu // shared context. Empty string when the owner hasn't set one; the daemon // skips rendering the heading in that case. if ws, err := h.Queries.GetWorkspace(r.Context(), parseUUID(resp.WorkspaceID)); err == nil { + resp.WorkspaceSlug = ws.Slug + if issueNumber > 0 { + resp.IssueIdentifier = service.IssueIdentifier(ws.IssuePrefix, issueNumber) + } if ws.Context.Valid { resp.WorkspaceContext = ws.Context.String } diff --git a/server/internal/handler/daemon_test.go b/server/internal/handler/daemon_test.go index f9a5667fb..0bf8559a7 100644 --- a/server/internal/handler/daemon_test.go +++ b/server/internal/handler/daemon_test.go @@ -653,6 +653,10 @@ func TestClaimTaskByRuntime_PopulatesWorkspaceContext(t *testing.T) { runtimeID := createClaimReclaimRuntime(t, ctx, "Workspace context claim runtime") agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, "Workspace context claim agent") taskID := createDispatchedClaimFixtureTask(t, ctx, agentID, runtimeID, issueID, "120 seconds", false) + var workspaceSlug, issuePrefix string + var issueNumber int32 + dbfx.QueryRow(t, `SELECT slug, issue_prefix FROM workspace WHERE id = $1`, testWorkspaceID).Scan(&workspaceSlug, &issuePrefix) + dbfx.QueryRow(t, `SELECT number FROM issue WHERE id = $1`, issueID).Scan(&issueNumber) req := newDaemonTokenRequest("POST", "/api/daemon/runtimes/"+runtimeID+"/tasks/claim", nil, testWorkspaceID, "workspace-context-claim") @@ -663,6 +667,8 @@ func TestClaimTaskByRuntime_PopulatesWorkspaceContext(t *testing.T) { Task *struct { ID string `json:"id"` WorkspaceContext string `json:"workspace_context"` + WorkspaceSlug string `json:"workspace_slug"` + IssueIdentifier string `json:"issue_identifier"` } `json:"task"` } w.JSON(&resp) @@ -675,6 +681,12 @@ func TestClaimTaskByRuntime_PopulatesWorkspaceContext(t *testing.T) { if resp.Task.WorkspaceContext != wsContext { t.Errorf("workspace_context = %q, want %q", resp.Task.WorkspaceContext, wsContext) } + if resp.Task.WorkspaceSlug != workspaceSlug { + t.Errorf("workspace_slug = %q, want %q", resp.Task.WorkspaceSlug, workspaceSlug) + } + if want := service.IssueIdentifier(issuePrefix, issueNumber); resp.Task.IssueIdentifier != want { + t.Errorf("issue_identifier = %q, want %q", resp.Task.IssueIdentifier, want) + } } // TestClaimTaskByRuntime_WorkspaceContextEmptyWhenUnset verifies the field