diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index 6ddbabca7..8145fe514 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -483,6 +483,14 @@ func TestTUISandboxSetupCommandGatedToWindowsNativeBackend(t *testing.T) { } func TestRunSandboxPolicyJSONGoldenIncludesManagerBaselineFields(t *testing.T) { + // Point HOME at an empty directory so the default credential-store + // deny-read entries (which depend on what exists in the real home, e.g. + // ~/.aws on the macOS CI image) cannot leak host paths into the golden + // comparison. + emptyHome := t.TempDir() + t.Setenv("HOME", emptyHome) + t.Setenv("USERPROFILE", emptyHome) + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", "") store := newSandboxTestStore(t) workspace := t.TempDir() deps := appDeps{ diff --git a/internal/contextreport/contextreport_test.go b/internal/contextreport/contextreport_test.go index 85be3a6ef..3a2d07bdf 100644 --- a/internal/contextreport/contextreport_test.go +++ b/internal/contextreport/contextreport_test.go @@ -28,7 +28,7 @@ func TestBuildCountsProjectGuidelinesAndFreeBudget(t *testing.T) { Model: "gpt-4.1", }, Registry: registry, - ContextWindow: 10_000, + ContextWindow: 20_000, }) if err != nil { t.Fatalf("Build returned error: %v", err) @@ -46,8 +46,8 @@ func TestBuildCountsProjectGuidelinesAndFreeBudget(t *testing.T) { if report.ProviderName != "openai" || report.ModelID != "gpt-4.1" || report.APIModel != "gpt-4.1" { t.Fatalf("provider metadata = %#v", report) } - if report.ContextWindow != 10_000 { - t.Fatalf("ContextWindow = %d, want 10000", report.ContextWindow) + if report.ContextWindow != 20_000 { + t.Fatalf("ContextWindow = %d, want 20000", report.ContextWindow) } if report.ProjectGuidelineFile != "AGENTS.md" { t.Fatalf("ProjectGuidelineFile = %q, want AGENTS.md", report.ProjectGuidelineFile) @@ -59,7 +59,7 @@ func TestBuildCountsProjectGuidelinesAndFreeBudget(t *testing.T) { t.Fatalf("UsedTokens = %d, want > 0", report.UsedTokens) } if report.FreeTokens <= 0 { - t.Fatalf("FreeTokens = %d, want > 0", report.FreeTokens) + t.Fatalf("FreeTokens = %d, UsedTokens = %d, ContextWindow = %d, want > 0", report.FreeTokens, report.UsedTokens, report.ContextWindow) } if report.FreeTokens+report.UsedTokens != report.ContextWindow { t.Fatalf("free + used = %d, want %d", report.FreeTokens+report.UsedTokens, report.ContextWindow) diff --git a/internal/cron/store.go b/internal/cron/store.go index cee2fa0f2..47c743238 100644 --- a/internal/cron/store.go +++ b/internal/cron/store.go @@ -9,6 +9,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/Gitlawb/zero/internal/fsutil" ) const ( @@ -144,7 +146,11 @@ func (s *Store) writeJob(job Job) error { if err := os.WriteFile(tmp, data, 0o600); err != nil { return err } - return os.Rename(tmp, filepath.Join(dir, "metadata.json")) + if err := fsutil.RenameWithRetry(tmp, filepath.Join(dir, "metadata.json"), nil); err != nil { + _ = os.Remove(tmp) + return err + } + return nil } func (s *Store) Get(id string) (Job, error) { diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go new file mode 100644 index 000000000..ace33502a --- /dev/null +++ b/internal/fsutil/rename.go @@ -0,0 +1,45 @@ +// Package fsutil provides small filesystem helpers shared across packages. +package fsutil + +import ( + "errors" + "os" + "runtime" + "syscall" + "time" +) + +// RenameWithRetry renames src to dst, retrying briefly on Windows when the +// destination is transiently locked (antivirus scanners, search indexers, or +// a concurrent reader holding the file open). rename overrides os.Rename so +// tests can exercise the retry path; pass nil to use os.Rename. +func RenameWithRetry(src, dst string, rename func(src, dst string) error) error { + if rename == nil { + rename = os.Rename + } + var err error + for i := 0; i < 10; i++ { + err = rename(src, dst) + if err == nil { + return nil + } + if runtime.GOOS == "windows" { + if os.IsPermission(err) || isWindowsSharingOrLockViolation(err) { + time.Sleep(10 * time.Millisecond) + continue + } + } + break + } + return err +} + +func isWindowsSharingOrLockViolation(err error) bool { + var errno syscall.Errno + if errors.As(err, &errno) { + const ERROR_SHARING_VIOLATION syscall.Errno = 32 + const ERROR_LOCK_VIOLATION syscall.Errno = 33 + return errno == ERROR_SHARING_VIOLATION || errno == ERROR_LOCK_VIOLATION + } + return false +} diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go new file mode 100644 index 000000000..8a64852d2 --- /dev/null +++ b/internal/fsutil/rename_test.go @@ -0,0 +1,47 @@ +package fsutil + +import ( + "errors" + "runtime" + "syscall" + "testing" +) + +func TestRenameWithRetryRetriesOnWindows(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("RenameWithRetry only retries on Windows") + } + var attempts int + err := RenameWithRetry("src", "dst", func(src, dst string) error { + attempts++ + switch attempts { + case 1: + return syscall.Errno(32) // ERROR_SHARING_VIOLATION + case 2: + return syscall.Errno(33) // ERROR_LOCK_VIOLATION + default: + return nil + } + }) + if err != nil { + t.Fatalf("expected success after retries, got: %v", err) + } + if attempts != 3 { + t.Errorf("expected 3 attempts, got %d", attempts) + } +} + +func TestRenameWithRetryNonRetryableError(t *testing.T) { + sentinel := errors.New("disk on fire") + var attempts int + err := RenameWithRetry("src", "dst", func(src, dst string) error { + attempts++ + return sentinel + }) + if !errors.Is(err, sentinel) { + t.Fatalf("expected sentinel error, got: %v", err) + } + if attempts != 1 { + t.Errorf("expected only 1 attempt for non-retryable error, got %d", attempts) + } +} diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index d535d0d9e..a2b8550f2 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -51,8 +51,11 @@ func TestPermissionProfileFromPolicyBuildsWorkspaceWriteProfile(t *testing.T) { t.Fatalf("read-only subpaths = %#v, want git metadata carveout %q", profile.FileSystem.WriteRoots[0].ReadOnlySubpaths, want) } } - if len(profile.FileSystem.DenyRead) != 1 || len(profile.FileSystem.DenyWrite) != 1 { - t.Fatalf("deny paths = %#v / %#v, want one each", profile.FileSystem.DenyRead, profile.FileSystem.DenyWrite) + // DenyRead may also carry default credential-store entries when the host + // has them, so assert containment rather than an exact count. Compare the + // normalized (symlink-resolved) form the profile stores. + if !stringSliceContains(profile.FileSystem.DenyRead, normalizeProfilePaths([]string{denyRead})[0]) || len(profile.FileSystem.DenyWrite) != 1 { + t.Fatalf("deny paths = %#v / %#v, want configured entries present", profile.FileSystem.DenyRead, profile.FileSystem.DenyWrite) } if profile.Network.Mode != NetworkDeny { t.Fatalf("network profile = %#v, want deny", profile.Network) @@ -370,3 +373,48 @@ func mkdirAll(paths ...string) error { } return nil } + +func TestCredentialDenyReadPathsIn(t *testing.T) { + home := t.TempDir() + awsDir := filepath.Join(home, ".aws") + gcloudDir := filepath.Join(home, ".config", "gcloud") + if err := mkdirAll(awsDir, gcloudDir); err != nil { + t.Fatal(err) + } + keyFile := filepath.Join(home, "sa-key.json") + if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + + paths := credentialDenyReadPathsIn(home, keyFile, nil) + for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile}) { + if !stringSliceContains(paths, want) { + t.Errorf("credential deny paths = %#v, want %q included", paths, want) + } + } + + // A path the host does not have is dropped, not emitted blind. + if stringSliceContains(paths, filepath.Join(home, ".azure")) { + t.Errorf("credential deny paths = %#v, must not include the absent ~/.azure", paths) + } + + // An explicit AllowRead entry covering a store is an opt-out. + optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) + } + if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { + t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) + } + + if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + t.Errorf("credential deny paths for blank home = %#v, want none", got) + } + + // The GOOGLE_APPLICATION_CREDENTIALS target stays protected even when no + // home directory is resolvable. + homeless := credentialDenyReadPathsIn("", keyFile, nil) + if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { + t.Errorf("credential deny paths without home = %#v, want key file included", homeless) + } +} diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index a565edba2..194736667 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -3,6 +3,7 @@ package sandbox import ( "os" "path/filepath" + "runtime" "strings" ) @@ -100,7 +101,7 @@ func PermissionProfileFromPolicy(workspaceRoot string, policy Policy, scope *Sco Kind: FileSystemRestricted, ReadRoots: readRoots, WriteRoots: writeRoots, - DenyRead: normalizeProfilePaths(policy.DenyRead), + DenyRead: dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy)...)), DenyWrite: normalizeProfilePaths(policy.DenyWrite), IncludePlatformRoots: true, AllowTemp: true, @@ -146,6 +147,66 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop return dedupeStrings(readRoots) } +// credentialDenyReadPaths returns default deny-read entries for well-known +// credential stores (~/.aws, ~/.config/gcloud, ~/.azure, and the file +// GOOGLE_APPLICATION_CREDENTIALS points to) so sandboxed commands cannot read +// cloud secrets under the read-all workspace posture. Two deliberate limits: +// +// - Windows is skipped: a non-empty profile DenyRead switches the Windows +// runner onto the capability-SID/ACL deny path and away from the +// WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit +// once the Windows deny-read model is settled. +// - A candidate nested under a user-configured AllowRead entry is dropped, +// so `allowRead: ["~/.aws"]` remains an explicit opt-out. +// +// These are profile-level rules only; they are intentionally NOT merged into +// Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and +// must keep reflecting user configuration alone. +func credentialDenyReadPaths(policy Policy) []string { + if runtime.GOOS == "windows" { + return nil + } + // A failed home lookup only drops the home-based candidates; the + // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + home, _ := os.UserHomeDir() + return credentialDenyReadPathsIn(home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), policy.AllowRead) +} + +// credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths, +// separated so tests can exercise it against a synthetic home directory. +func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead []string) []string { + var candidates []string + if home = strings.TrimSpace(home); home != "" { + candidates = append(candidates, + filepath.Join(home, ".aws"), + filepath.Join(home, ".config", "gcloud"), + filepath.Join(home, ".azure"), + ) + } + if target := strings.TrimSpace(googleCredentials); target != "" { + candidates = append(candidates, target) + } + allowRoots := normalizeProfilePaths(allowRead) + out := make([]string, 0, len(candidates)) + for _, path := range normalizeProfilePaths(candidates) { + // Only stores that actually exist on this host need a deny rule. + if _, err := os.Stat(path); err != nil { + continue + } + reincluded := false + for _, allow := range allowRoots { + if pathWithinRoot(allow, path) { + reincluded = true + break + } + } + if !reincluded { + out = append(out, path) + } + } + return out +} + // userGitConfigReadPaths returns the user's global git config FILES so a // sandboxed git can read identity and config (user.name/email, aliases) instead // of failing with "unable to access ~/.gitconfig". It is deliberately the config diff --git a/internal/sandbox/reentrancy_test.go b/internal/sandbox/reentrancy_test.go index 4958b4a03..9e0b0bcb6 100644 --- a/internal/sandbox/reentrancy_test.go +++ b/internal/sandbox/reentrancy_test.go @@ -61,6 +61,7 @@ func TestSandboxEnvironmentPreservesCallerEnv(t *testing.T) { }, DefaultPolicy(), BackendMacOSSeatbelt, + "", ) for _, want := range []string{ diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 1f5f66c63..4bcf83808 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -10,6 +10,8 @@ import ( "runtime" "strings" "sync/atomic" + + "github.com/Gitlawb/zero/internal/providercatalog" ) var errNativeSandboxUnavailable = errors.New("native sandbox backend is unavailable") @@ -205,7 +207,7 @@ func linuxSandboxHelperCommandPlan(execRequest SandboxExecutionRequest, policy P if err != nil { return CommandPlan{}, err } - env := sandboxEnvironmentForCommand(spec.Env, policy, BackendLinuxBwrap) + env := sandboxEnvironmentForCommand(spec.Env, policy, BackendLinuxBwrap, "") planDir := spec.Dir if helper.Dir != "" { planDir = helper.Dir @@ -316,7 +318,7 @@ func seatbeltCommandPlanWithProfile(spec CommandSpec, workspaceRoot string, prof if envBackend == "" { envBackend = BackendMacOSSeatbelt } - env := sandboxEnvironmentForCommand(spec.Env, policy, envBackend) + env := sandboxEnvironmentForCommand(spec.Env, policy, envBackend, "") plan := CommandPlan{ Backend: backend, TargetBackend: backend.TargetBackend(), @@ -351,7 +353,7 @@ func seatbeltCompatibilityPermissionProfile(writeRoots []string, policy Policy) fs.WriteRoots = append(fs.WriteRoots, WritableRoot{Root: root}) } } - fs.DenyRead = normalizeProfilePaths(policy.DenyRead) + fs.DenyRead = dedupeStrings(append(normalizeProfilePaths(policy.DenyRead), credentialDenyReadPaths(policy)...)) fs.DenyWrite = normalizeProfilePaths(policy.DenyWrite) return PermissionProfile{ FileSystem: fs, @@ -370,11 +372,11 @@ func existingBubblewrapMounts() []string { return mounts } -func sandboxEnvironment(policy Policy, backend BackendName, _ string) []string { - return sandboxEnvironmentForCommand(nil, policy, backend) +func sandboxEnvironment(policy Policy, backend BackendName, workspaceRoot string) []string { + return sandboxEnvironmentForCommand(nil, policy, backend, workspaceRoot) } -func sandboxEnvironmentForCommand(specEnv []string, policy Policy, backend BackendName) []string { +func sandboxEnvironmentForCommand(specEnv []string, policy Policy, backend BackendName, workspaceRoot string) []string { env := cloneStrings(specEnv) if specEnv == nil { // Preserve the caller environment for sandboxed commands. The sandbox @@ -382,6 +384,7 @@ func sandboxEnvironmentForCommand(specEnv []string, policy Policy, backend Backe // command env values still replace inherited values below. env = os.Environ() } + env = scrubSensitiveEnv(env) pathValue := envListValue(env, "PATH", defaultPath()) if runtime.GOOS == "darwin" { // Preserve standard user tool locations so a bare `python3`/`node` @@ -396,8 +399,15 @@ func sandboxEnvironmentForCommand(specEnv []string, policy Policy, backend Backe "ZERO_SANDBOX_NETWORK=" + string(policy.Network), EnvSandboxed + "=1", } - if runtime.GOOS == "windows" { - overrides = append(overrides, "COMSPEC="+envListValue(env, "COMSPEC", "cmd.exe")) + if workspaceRoot != "" { + overrides = append(overrides, "HOME="+workspaceRoot) + } + if backend == BackendWindowsRestrictedToken || backend == BackendWindowsElevated { + overrides = append(overrides, + "COMSPEC="+envListValue(env, "COMSPEC", "cmd.exe"), + "SystemRoot="+envListValue(env, "SystemRoot", `C:\Windows`), + "WINDIR="+envListValue(env, "WINDIR", `C:\Windows`), + ) } return upsertEnvList(env, overrides...) } @@ -409,13 +419,6 @@ func cloneStrings(values []string) []string { return append([]string{}, values...) } -func firstEnv(key string, fallback string) string { - if value := strings.TrimSpace(os.Getenv(key)); value != "" { - return value - } - return fallback -} - func envListValue(env []string, key string, fallback string) string { for _, entry := range env { existingKey, value, ok := strings.Cut(entry, "=") @@ -963,3 +966,55 @@ func regexpQuoteMeta(value string) string { ) return replacer.Replace(value) } + +func scrubSensitiveEnv(env []string) []string { + // Secrets not covered by the provider catalog: cloud/VCS credentials and + // providers Zero talks to through generic OpenAI-compatible endpoints. + sensitiveKeys := []string{ + "COHERE_API_KEY", + "PERPLEXITY_API_KEY", + "ANYSCALE_API_KEY", + "AZURE_OPENAI_API_KEY", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "GITLAB_TOKEN", + "GH_TOKEN", + "ZERO_WEBSEARCH_API_KEY", + "ZERO_DAEMON_REMOTE_TOKEN", + } + for _, descriptor := range providercatalog.All() { + for _, key := range descriptor.AuthEnvVars { + // AWS_PROFILE is a profile name, not a secret, and scrubbing it + // protects nothing: the SDKs fall back to the default profile in + // ~/.aws/credentials, which credentialDenyReadPaths blocks at the + // filesystem level instead. GOOGLE_APPLICATION_CREDENTIALS IS + // scrubbed: it points at a service-account key file at an + // arbitrary path, so hiding the pointer complements the deny-read + // rule on the file itself. + if key == "AWS_PROFILE" { + continue + } + sensitiveKeys = append(sensitiveKeys, key) + } + } + + out := make([]string, 0, len(env)) + for _, kv := range env { + key, _, ok := strings.Cut(kv, "=") + if !ok { + out = append(out, kv) + continue + } + drop := false + for _, sensitive := range sensitiveKeys { + if strings.EqualFold(key, sensitive) { + drop = true + break + } + } + if !drop { + out = append(out, kv) + } + } + return out +} diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 94c1709b6..5e7a5b256 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "reflect" "runtime" "strings" "testing" @@ -637,3 +638,39 @@ func TestLinuxHelperPlanPreservesRealExtraRootCwd(t *testing.T) { } assertArgsContainSequence(t, bwrapArgs, "--chdir", resolvedExtra) } + +func TestScrubSensitiveEnv(t *testing.T) { + inputEnv := []string{ + "PATH=/usr/bin", + "OPENAI_API_KEY=sk-proj-12345", + "ANTHROPIC_API_KEY=sk-ant-12345", + "GEMINI_API_KEY=AIzaSy12345", + "DEEPSEEK_API_KEY=ds-12345", + "GITHUB_TOKEN=ghp_12345", + "AWS_ACCESS_KEY_ID=AKIA12345", + "AWS_SECRET_ACCESS_KEY=secret12345", + "GOOGLE_API_KEY=AIzaSy67890", + "XAI_API_KEY=xai-12345", + "HUGGINGFACE_API_KEY=hf_12345", + "GOOGLE_APPLICATION_CREDENTIALS=/home/user/sa-key.json", + "AWS_PROFILE=staging", + "SAFE_VAR=hello", + } + scrubbed := scrubSensitiveEnv(inputEnv) + expected := map[string]string{ + "PATH": "/usr/bin", + "SAFE_VAR": "hello", + "AWS_PROFILE": "staging", + } + actual := make(map[string]string, len(scrubbed)) + for _, entry := range scrubbed { + key, value, _ := strings.Cut(entry, "=") + if _, dup := actual[key]; dup { + t.Errorf("duplicate key %q in scrubbed env: %v", key, scrubbed) + } + actual[key] = value + } + if !reflect.DeepEqual(actual, expected) { + t.Errorf("scrubSensitiveEnv() = %v, want %v", actual, expected) + } +} diff --git a/internal/sandbox/windows_runner.go b/internal/sandbox/windows_runner.go index b783910ce..2b271f96d 100644 --- a/internal/sandbox/windows_runner.go +++ b/internal/sandbox/windows_runner.go @@ -326,7 +326,7 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli if err != nil { return CommandPlan{}, err } - childEnv := windowsSandboxChildEnv(spec.Env, policy, execRequest.WorkspaceRoot) + childEnv := sandboxEnvironmentForCommand(spec.Env, policy, BackendWindowsRestrictedToken, execRequest.WorkspaceRoot) // The unelevated enforcement tier maps to the runner's unelevated level: same // restricted token, but the runner applies the workspace ACLs itself instead // of requiring the elevated setup marker. @@ -367,27 +367,6 @@ func windowsRestrictedTokenCommandPlan(execRequest SandboxExecutionRequest, poli }, execRequest), nil } -func windowsSandboxChildEnv(specEnv []string, policy Policy, workspaceRoot string) []string { - var env []string - if specEnv != nil { - env = cloneStrings(specEnv) - } else { - env = append(env, os.Environ()...) - } - env = upsertEnvList(env, - "HOME="+workspaceRoot, - "PATH="+firstEnv("PATH", defaultPath()), - "TERM="+firstEnv("TERM", "dumb"), - EnvSandboxBackend+"="+string(BackendWindowsRestrictedToken), - "ZERO_SANDBOX_NETWORK="+string(policy.Network), - EnvSandboxed+"=1", - "COMSPEC="+firstEnv("COMSPEC", "cmd.exe"), - "SystemRoot="+firstEnv("SystemRoot", `C:\Windows`), - "WINDIR="+firstEnv("WINDIR", `C:\Windows`), - ) - return env -} - func upsertEnvList(env []string, values ...string) []string { out := cloneStrings(env) for _, value := range values { diff --git a/internal/sandbox/windows_unelevated.go b/internal/sandbox/windows_unelevated.go index 6934071de..980664d6a 100644 --- a/internal/sandbox/windows_unelevated.go +++ b/internal/sandbox/windows_unelevated.go @@ -7,6 +7,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/Gitlawb/zero/internal/fsutil" ) const windowsUnelevatedSetupMarkerSchemaVersion = 1 @@ -138,7 +140,7 @@ func recordWindowsUnelevatedAppliedPlan(sandboxHome string, applied WindowsUnele _ = os.Remove(tmpPath) return fmt.Errorf("close windows unelevated setup marker temp file: %w", err) } - if err := os.Rename(tmpPath, path); err != nil { + if err := fsutil.RenameWithRetry(tmpPath, path, nil); err != nil { _ = os.Remove(tmpPath) return fmt.Errorf("replace windows unelevated setup marker: %w", err) } diff --git a/internal/sessions/store.go b/internal/sessions/store.go index b0da699aa..3eb98f14e 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -14,6 +14,8 @@ import ( "sync" "sync/atomic" "time" + + "github.com/Gitlawb/zero/internal/fsutil" ) const ( @@ -841,7 +843,7 @@ func (store *Store) writeMetadata(session Metadata) error { if err := writeFileSync(tmp, append(data, '\n'), 0o600); err != nil { return fmt.Errorf("write zero session metadata: %w", err) } - if err := os.Rename(tmp, path); err != nil { + if err := fsutil.RenameWithRetry(tmp, path, nil); err != nil { _ = os.Remove(tmp) return fmt.Errorf("replace zero session metadata: %w", err) } @@ -904,7 +906,7 @@ func (store *Store) writeFileAtomicSync(path string, content []byte, perm os.Fil if err := writeFileSync(tmp, content, perm); err != nil { return err } - if err := os.Rename(tmp, path); err != nil { + if err := fsutil.RenameWithRetry(tmp, path, nil); err != nil { _ = os.Remove(tmp) return err } diff --git a/internal/swarm/mailbox.go b/internal/swarm/mailbox.go index d9a9a4f0b..904c0abcb 100644 --- a/internal/swarm/mailbox.go +++ b/internal/swarm/mailbox.go @@ -11,6 +11,7 @@ import ( "sync/atomic" "time" + "github.com/Gitlawb/zero/internal/fsutil" "github.com/Gitlawb/zero/internal/lockutil" ) @@ -34,6 +35,9 @@ type Mailbox struct { MaxMessages int // LockTimeout bounds how long Send/MarkRead wait for the inbox lock. LockTimeout time.Duration + + // rename is used to override the rename operation in tests. + rename func(src, dst string) error } const ( @@ -216,7 +220,7 @@ func (m *Mailbox) Send(team, recipient string, msg Message) error { return fmt.Errorf("%w: %d messages", ErrMailboxFull, len(messages)) } messages = append(messages, msg) - return atomicWriteJSON(path, messages) + return m.atomicWriteJSON(path, messages) } // ReadAndConsume reads the recipient's inbox and marks every previously-unread @@ -258,7 +262,7 @@ func (m *Mailbox) ReadAndConsume(team, recipient string) ([]Message, error) { } } if changed { - if err := atomicWriteJSON(path, messages); err != nil { + if err := m.atomicWriteJSON(path, messages); err != nil { return nil, err } } @@ -301,7 +305,7 @@ func (m *Mailbox) readLocked(path string) ([]Message, error) { // atomicWriteJSON writes data as pretty JSON to a sibling temp file (0600) then // renames it over path, so a reader never observes a partial write. -func atomicWriteJSON(path string, data any) error { +func (m *Mailbox) atomicWriteJSON(path string, data any) error { encoded, err := json.MarshalIndent(data, "", " ") if err != nil { return fmt.Errorf("swarm: encode inbox: %w", err) @@ -324,7 +328,7 @@ func atomicWriteJSON(path string, data any) error { if err := tmp.Close(); err != nil { return fmt.Errorf("swarm: close temp inbox: %w", err) } - if err := os.Rename(tmpName, path); err != nil { + if err := m.renameWithRetry(tmpName, path); err != nil { return fmt.Errorf("swarm: commit inbox: %w", err) } return nil @@ -395,3 +399,7 @@ func acquireLock(lockPath string, timeout time.Duration) (func(), error) { time.Sleep(2 * time.Millisecond) } } + +func (m *Mailbox) renameWithRetry(src, dst string) error { + return fsutil.RenameWithRetry(src, dst, m.rename) +} diff --git a/internal/swarm/mailbox_test.go b/internal/swarm/mailbox_test.go index 2164eb911..2e59e4a54 100644 --- a/internal/swarm/mailbox_test.go +++ b/internal/swarm/mailbox_test.go @@ -8,6 +8,7 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "testing" "time" ) @@ -295,6 +296,7 @@ func TestMailboxConcurrentSends(t *testing.T) { defer wg.Done() if err := mb.Send("team", "bob", Message{From: "a", Body: "concurrent"}); err != nil { failures.Add(1) + t.Logf("Send error: %v", err) } }() } @@ -310,3 +312,51 @@ func TestMailboxConcurrentSends(t *testing.T) { t.Fatalf("concurrent sends lost messages: got %d, want %d", len(msgs), n) } } + +func TestMailboxRenameRetry(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("renameWithRetry only retries on Windows") + } + mb := newTestMailbox(t) + + var attempts int + mb.rename = func(src, dst string) error { + attempts++ + if attempts < 3 { + // Return a retryable Windows sharing violation error + return syscall.Errno(32) + } + // Then succeed + return os.Rename(src, dst) + } + + err := mb.Send("team", "alice", Message{Body: "retry-test"}) + if err != nil { + t.Fatalf("expected Send to succeed after retries, got: %v", err) + } + if attempts < 3 { + t.Errorf("expected at least 3 attempts, got %d", attempts) + } +} + +func TestMailboxRenameNonRetryableError(t *testing.T) { + mb := newTestMailbox(t) + + var attempts int + expectedErr := errors.New("non-retryable error") + mb.rename = func(src, dst string) error { + attempts++ + return expectedErr + } + + err := mb.Send("team", "alice", Message{Body: "retry-test"}) + if err == nil { + t.Fatal("expected Send to fail immediately") + } + if !errors.Is(err, expectedErr) && !strings.Contains(err.Error(), expectedErr.Error()) { + t.Errorf("expected error %v, got %v", expectedErr, err) + } + if attempts != 1 { + t.Errorf("expected only 1 attempt for non-retryable error, got %d", attempts) + } +}