diff --git a/docs/CLI.md b/docs/CLI.md index b01e0b8922..e9b960b59c 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -421,6 +421,10 @@ Task `finished_at` is empty while a task is running, and artifact exists. A `running` record without a live session lease is reported as `interrupted`; opening that session also repairs the persisted lifecycle state. +When the lease probe cannot decide whether a runtime holds a session (an +unreadable lease lock or sidecar), `task list` reports a `running` record as +`unknown` and `session list` reports the state as `unknown`. + Schema compatibility rules for version 1: - consumers must ignore unknown fields; diff --git a/docs/CLI.zh-CN.md b/docs/CLI.zh-CN.md index 527642a236..e1ac07d22a 100644 --- a/docs/CLI.zh-CN.md +++ b/docs/CLI.zh-CN.md @@ -367,6 +367,9 @@ ID，需要一并保留该私有身份密钥。任务仍在运行时 `artifact_complete=true`。没有 live session lease 的 `running` 记录会显示为 `interrupted`；再次打开该 session 时也会自动修复持久化生命周期状态。 +lease 探针无法判定是否有 runtime 持有该 session 时（lease lock 或 sidecar 不可读）， +`task list` 把 `running` 记录显示为 `unknown`，`session list` 把状态显示为 `unknown`。 + Schema version 1 的兼容规则： - 消费端必须忽略未知字段； diff --git a/internal/frontend/cli/doctor.go b/internal/frontend/cli/doctor.go index 89fd4c616f..ec473c7eda 100644 --- a/internal/frontend/cli/doctor.go +++ b/internal/frontend/cli/doctor.go @@ -238,7 +238,18 @@ func doctorRedactSessionsCommand(args []string) int { } fmt.Fprintf(os.Stdout, "session secret cleanup %s %d/%d files", action, res.FilesChanged, res.FilesScanned) if res.FilesSkipped > 0 { - fmt.Fprintf(os.Stdout, " (%d skipped: active lease held)", res.FilesSkipped) + // Say which state kept each file from being rewritten. Only a proven + // holder may be reported as an active lease; a probe that could not + // decide is its own reason, never folded into that claim. + switch { + case res.FilesSkippedUnprovable == 0: + fmt.Fprintf(os.Stdout, " (%d skipped: active lease held)", res.FilesSkipped) + case res.FilesSkippedHeld == 0: + fmt.Fprintf(os.Stdout, " (%d skipped: could not prove the lease)", res.FilesSkipped) + default: + fmt.Fprintf(os.Stdout, " (%d skipped: %d active lease held, %d could not prove the lease)", + res.FilesSkipped, res.FilesSkippedHeld, res.FilesSkippedUnprovable) + } } fmt.Fprintln(os.Stdout) } diff --git a/internal/frontend/cli/doctor_test.go b/internal/frontend/cli/doctor_test.go index 9fc63eb9a4..3666a581e1 100644 --- a/internal/frontend/cli/doctor_test.go +++ b/internal/frontend/cli/doctor_test.go @@ -10,6 +10,8 @@ import ( "reasonix/internal/base/testenv" "reasonix/internal/contract/config" + "reasonix/internal/state/sessionstore" + "reasonix/internal/state/store" ) func TestDoctorCommandPrintsJSON(t *testing.T) { @@ -172,3 +174,50 @@ func TestDoctorRedactSessionsCommand(t *testing.T) { t.Fatalf("doctor redact-sessions leaked secret:\n%s", data) } } + +// The summary line is a report about evidence. When the lease probe cannot +// decide, it must not repeat the held claim; refusing makes the incomplete +// scrub visible and exits non-zero. +func TestDoctorRedactSessionsNamesAnUnprovableLease(t *testing.T) { + dir := testenv.TempDir(t) + const secret = "sensitive-value-that-must-not-leak" + path := filepath.Join(dir, "abc.jsonl") + body := `{"role":"tool","content":"DEEPSEEK_API_KEY=` + secret + `"}` + "\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + if err := sessionstore.SaveSessionLeaseInfo(path, sessionstore.SessionLeaseInfo{ + SessionPath: path, + WriterID: "other-host-1234-deadbeef", + PID: os.Getpid() + 1, + }); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(store.SessionLeaseLock(sessionstore.CanonicalSessionPath(path)), 0o755); err != nil { + t.Fatal(err) + } + + var rc int + stdout, stderr := captureCLIOutput(t, func() { + rc = doctorCommand([]string{"redact-sessions", "--dir", dir}, "test-version") + }) + if rc != 1 { + t.Fatalf("rc = %d, want 1 (an incomplete scrub is not success)", rc) + } + if !strings.Contains(stdout, "(1 skipped: could not prove the lease)") { + t.Fatalf("summary does not name the unprovable lease:\n%s", stdout) + } + if strings.Contains(stdout, "active lease held") { + t.Fatalf("summary claimed an active lease the probe never proved:\n%s", stdout) + } + if !strings.Contains(stderr, "could not be proven") { + t.Fatalf("refusal reason missing from stderr:\n%s", stderr) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != body { + t.Fatalf("redaction rewrote a session it could not prove:\n%s", data) + } +} diff --git a/internal/frontend/cli/session_machine.go b/internal/frontend/cli/session_machine.go index 20bded19a5..f00d65e448 100644 --- a/internal/frontend/cli/session_machine.go +++ b/internal/frontend/cli/session_machine.go @@ -29,6 +29,12 @@ const ( machineSchemaVersion = 1 machineIdentityKeyBytes = 32 machineIdentityKeyFile = "machine-id.key" + + // machineStateUnknown is what the machine surfaces report when the lease + // probe could not prove ownership. It is neither active nor idle: the + // evidence supports no claim either way, and the schema keeps state a + // string so consumers that ignore unknown values keep working. + machineStateUnknown = "unknown" ) type machineSession struct { @@ -313,12 +319,20 @@ func machineSessions(dir string, identityKey []byte) ([]machineSession, error) { } meta, ok, _ := sessionstore.LoadBranchMeta(info.Path) state := "idle" - if sessionstore.SessionLeaseHeld(info.Path) { + switch leaseErr := sessionstore.SessionLeaseOwnership(info.Path); { + case leaseErr == nil: + if ok && meta.InFlightTurn != nil { + state = "interrupted" + } else if info.Recovered { + state = "recovered" + } + case errors.Is(leaseErr, sessionstore.ErrSessionLeaseHeld): state = "active" - } else if ok && meta.InFlightTurn != nil { - state = "interrupted" - } else if info.Recovered { - state = "recovered" + default: + // The probe could not prove the lease either way. Reporting active + // would claim a holder that was never observed; falling back to + // idle/interrupted would claim the opposite. Say unknown. + state = machineStateUnknown } scope := info.Scope if scope == "" { diff --git a/internal/frontend/cli/session_machine_test.go b/internal/frontend/cli/session_machine_test.go index 822922b099..dc3f5ed011 100644 --- a/internal/frontend/cli/session_machine_test.go +++ b/internal/frontend/cli/session_machine_test.go @@ -13,6 +13,7 @@ import ( "reasonix/internal/base/testenv" "reasonix/internal/contract/config" "reasonix/internal/contract/provider" + "reasonix/internal/state/store" ) func TestSessionMachineListIsStableAndRedacted(t *testing.T) { @@ -193,3 +194,54 @@ func saveMachineTestSession(t *testing.T, dir, id string, updatedAt time.Time) { t.Fatalf("saved meta says updated %v, the fixture asked for %v: ordering would follow the write clock", got.UpdatedAt, updatedAt) } } + +// installUnprovableMachineLease leaves a readable foreign sidecar and a lease +// lock the probe cannot open, so ownership is neither provably held nor free. +func installUnprovableMachineLease(t *testing.T, sessionPath string) { + t.Helper() + if err := sessionstore.SaveSessionLeaseInfo(sessionPath, sessionstore.SessionLeaseInfo{ + SessionPath: sessionPath, + WriterID: "other-host-1234-deadbeef", + PID: os.Getpid() + 1, + AcquiredAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("save foreign lease info: %v", err) + } + if err := os.Mkdir(store.SessionLeaseLock(sessionstore.CanonicalSessionPath(sessionPath)), 0o755); err != nil { + t.Fatalf("make unopenable lease lock: %v", err) + } +} + +// An unprovable lease supports no state claim: neither active (a holder was +// never observed) nor idle/interrupted/recovered (nobody proved it free). +func TestSessionMachineSaysUnknownWhenLeaseOwnershipIsUnprovable(t *testing.T) { + identityKey := installMachineTestIdentity(t) + dir := testenv.TempDir(t) + saveMachineTestSession(t, dir, "unknown", time.Date(2026, 7, 23, 12, 0, 0, 0, time.UTC)) + installUnprovableMachineLease(t, filepath.Join(dir, "unknown.jsonl")) + + var listOut bytes.Buffer + if code := runSessionCommand([]string{"list", "--json", "--dir", dir}, &listOut); code != 0 { + t.Fatalf("list exit code = %d, output = %s", code, listOut.String()) + } + var list machineSessionList + if err := json.Unmarshal(listOut.Bytes(), &list); err != nil { + t.Fatalf("decode list: %v", err) + } + if len(list.Sessions) != 1 || list.Sessions[0].State != machineStateUnknown { + t.Fatalf("sessions = %+v, want one session in state %q", list.Sessions, machineStateUnknown) + } + + var showOut bytes.Buffer + args := []string{"status", "--json", machineSessionIDWithKey("unknown", identityKey), "--dir", dir} + if code := runSessionCommand(args, &showOut); code != 0 { + t.Fatalf("status exit code = %d, output = %s", code, showOut.String()) + } + var show machineSessionShow + if err := json.Unmarshal(showOut.Bytes(), &show); err != nil { + t.Fatalf("decode status: %v", err) + } + if show.Session.State != machineStateUnknown { + t.Fatalf("status = %+v, want state %q", show.Session, machineStateUnknown) + } +} diff --git a/internal/frontend/cli/task_machine.go b/internal/frontend/cli/task_machine.go index 48cd74c909..b727fda84a 100644 --- a/internal/frontend/cli/task_machine.go +++ b/internal/frontend/cli/task_machine.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "io" "os" "reasonix/internal/state/sessionstore" @@ -147,7 +148,9 @@ func machineTasks(dir, sessionFilter string, identityKey []byte) ([]machineTask, if sessionFilter != "" && sessionID != sessionFilter { continue } - sessionActive := sessionstore.SessionLeaseHeld(session.Path) + leaseErr := sessionstore.SessionLeaseOwnership(session.Path) + sessionActive := errors.Is(leaseErr, sessionstore.ErrSessionLeaseHeld) + sessionUnknown := leaseErr != nil && !sessionActive views, err := jobs.ListArtifactViews(session.Path) if err != nil { return nil, err @@ -159,10 +162,19 @@ func machineTasks(dir, sessionFilter string, identityKey []byte) ([]machineTask, status := view.Status finishedAt := machineUnixMillis(view.FinishedAt) artifactComplete := view.ArtifactComplete - if status == jobs.Running && !sessionActive { - status = jobs.Interrupted - finishedAt = "" - artifactComplete = false + if status == jobs.Running { + switch { + case sessionUnknown: + // The probe did not prove a holder, but it also did not prove + // none: neither running nor interrupted is supported. + status = jobs.Status(machineStateUnknown) + finishedAt = "" + artifactComplete = false + case !sessionActive: + status = jobs.Interrupted + finishedAt = "" + artifactComplete = false + } } out = append(out, machineTask{ ID: view.ID, @@ -186,7 +198,10 @@ func machineTasks(dir, sessionFilter string, identityKey []byte) ([]machineTask, finishedAt := "" artifactComplete := false if status == sessionstore.SubagentRunning { - if !sessionActive { + switch { + case sessionUnknown: + status = sessionstore.SubagentStatus(machineStateUnknown) + case !sessionActive: status = sessionstore.SubagentInterrupted } } else { diff --git a/internal/frontend/cli/task_machine_test.go b/internal/frontend/cli/task_machine_test.go index f59ddf1e14..0353f56a5a 100644 --- a/internal/frontend/cli/task_machine_test.go +++ b/internal/frontend/cli/task_machine_test.go @@ -224,3 +224,76 @@ func TestTaskMachineRejectsADirThatIsNotThere(t *testing.T) { } } } + +// A persisted running record is only as good as the lease evidence beside it: +// a proven holder keeps it running, a proven-free session marks it interrupted, +// and an unprovable probe must say unknown rather than pick either claim. +func TestTaskMachineReportsUnknownOnlyWhenLeaseCannotBeProven(t *testing.T) { + identityKey := installMachineTestIdentity(t) + cases := []struct { + name string + setup func(t *testing.T, sessionPath string) + want string + }{ + {name: "free", setup: func(*testing.T, string) {}, want: string(jobs.Interrupted)}, + {name: "held", setup: func(t *testing.T, sessionPath string) { + lease, err := sessionstore.TryAcquireSessionLease(sessionPath) + if err != nil { + t.Fatalf("TryAcquireSessionLease: %v", err) + } + t.Cleanup(lease.Release) + }, want: string(jobs.Running)}, + {name: "unprovable", setup: installUnprovableMachineLease, want: machineStateUnknown}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := testenv.TempDir(t) + saveMachineTestSession(t, dir, "session", time.Now()) + sessionPath := filepath.Join(dir, "session.jsonl") + tc.setup(t, sessionPath) + + artifactDir := jobs.ArtifactDir(sessionPath) + if err := os.MkdirAll(artifactDir, 0o700); err != nil { + t.Fatal(err) + } + jobMeta := `{"id":"task-1","kind":"task","status":"running","startedAt":1700000000000}` + if err := os.WriteFile(filepath.Join(artifactDir, "task-1.json"), []byte(jobMeta), 0o600); err != nil { + t.Fatal(err) + } + + subDir := filepath.Join(dir, "subagents") + if err := os.MkdirAll(subDir, 0o700); err != nil { + t.Fatal(err) + } + now := time.Now().UTC() + meta, err := json.Marshal(sessionstore.SubagentMeta{ + Ref: "sa_running", CreatedAt: now, UpdatedAt: now, + Status: sessionstore.SubagentRunning, Kind: "task", ParentSession: "session", + }) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(subDir, "sa_running.meta.json"), meta, 0o600); err != nil { + t.Fatal(err) + } + + tasks, err := machineTasks(dir, machineSessionIDWithKey("session", identityKey), identityKey) + if err != nil { + t.Fatalf("machineTasks: %v", err) + } + byID := make(map[string]machineTask, len(tasks)) + for _, task := range tasks { + byID[task.ID] = task + } + for _, id := range []string{"task-1", "sa_running"} { + got, ok := byID[id] + if !ok { + t.Fatalf("task %s missing from projection: %+v", id, tasks) + } + if got.Status != tc.want { + t.Fatalf("%s status = %q, want %q", id, got.Status, tc.want) + } + } + }) + } +} diff --git a/internal/frontend/doctor/session_redact.go b/internal/frontend/doctor/session_redact.go index 6db46546f9..0d40fc0ff8 100644 --- a/internal/frontend/doctor/session_redact.go +++ b/internal/frontend/doctor/session_redact.go @@ -26,14 +26,19 @@ type RedactSessionsOptions struct { } // RedactSessionsResult summarizes a historical session-log redaction run. +// FilesSkipped is the total refused because ownership was not free; +// FilesSkippedHeld and FilesSkippedUnprovable split it by the state the probe +// actually proved. Only the held split is a lease claim the evidence supports. type RedactSessionsResult struct { - Dirs []string `json:"dirs"` - FilesScanned int64 `json:"files_scanned"` - FilesChanged int64 `json:"files_changed"` - FilesSkipped int64 `json:"files_skipped"` - BytesRewritten int64 `json:"bytes_rewritten"` - DryRun bool `json:"dry_run"` - Errors []string `json:"errors,omitempty"` + Dirs []string `json:"dirs"` + FilesScanned int64 `json:"files_scanned"` + FilesChanged int64 `json:"files_changed"` + FilesSkipped int64 `json:"files_skipped"` + FilesSkippedHeld int64 `json:"files_skipped_held"` + FilesSkippedUnprovable int64 `json:"files_skipped_unprovable"` + BytesRewritten int64 `json:"bytes_rewritten"` + DryRun bool `json:"dry_run"` + Errors []string `json:"errors,omitempty"` } // RedactSessions masks credential-shaped values already persisted in Reasonix @@ -79,9 +84,20 @@ func RedactSessions(opts RedactSessionsOptions) RedactSessionsResult { }) for _, path := range candidates { res.FilesScanned++ - if sessionPath := redactionSessionPath(path); sessionPath != "" && sessionRedactionLeaseHeld(sessionPath) { - res.FilesSkipped++ - continue + if sessionPath := redactionSessionPath(path); sessionPath != "" { + if state := sessionRedactionLeaseState(sessionPath); state != nil { + res.FilesSkipped++ + if errors.Is(state, sessionstore.ErrSessionLeaseHeld) { + res.FilesSkippedHeld++ + } else { + // Not proof of a holder, so refuse and say so: rewriting a + // session whose ownership is unproven risks a live write, + // and "active lease held" would claim what was never seen. + res.FilesSkippedUnprovable++ + res.Errors = append(res.Errors, fmt.Sprintf("%s: skipped: %v", path, state)) + } + continue + } } changed, rewritten, err := redactSessionArtifact(path, opts.DryRun) if err != nil { @@ -178,15 +194,28 @@ func redactionSessionPath(path string) string { } } -func sessionRedactionLeaseHeld(sessionPath string) bool { - if sessionstore.SessionLeaseHeld(sessionPath) { - return true +// sessionRedactionLeaseState reports the ownership state of the session an +// artifact belongs to: nil when it is safe to rewrite, ErrSessionLeaseHeld for +// a live holder, ErrSessionLeaseUnprovable when the probe cannot decide. A +// guardian log belongs to its parent transcript; a proven holder wins over an +// unprovable probe because it must never be rewritten. +func sessionRedactionLeaseState(sessionPath string) error { + state := sessionstore.SessionLeaseOwnership(sessionPath) + before, guardian := strings.CutSuffix(sessionPath, ".guardian.jsonl") + if !guardian { + return state } - if before, ok := strings.CutSuffix(sessionPath, ".guardian.jsonl"); ok { - parent := before + ".jsonl" - return sessionstore.SessionLeaseHeld(parent) + parent := sessionstore.SessionLeaseOwnership(before + ".jsonl") + switch { + case errors.Is(state, sessionstore.ErrSessionLeaseHeld): + return state + case errors.Is(parent, sessionstore.ErrSessionLeaseHeld): + return parent + case state != nil: + return state + default: + return parent } - return false } // redactSessionArtifact dispatches one candidate file to a format-aware diff --git a/internal/frontend/doctor/session_redact_test.go b/internal/frontend/doctor/session_redact_test.go index a9b68eef57..e5cc7e3da4 100644 --- a/internal/frontend/doctor/session_redact_test.go +++ b/internal/frontend/doctor/session_redact_test.go @@ -7,12 +7,31 @@ import ( "reasonix/internal/state/sessionstore" "strings" "testing" + "time" "reasonix/internal/base/testenv" "reasonix/internal/contract/provider" "reasonix/internal/state/store" ) +// installUnopenableLease leaves a readable foreign sidecar and a lease lock the +// probe cannot open. Ownership is then neither provably held nor free — the +// state redaction used to fold into "active lease held". +func installUnopenableLease(t *testing.T, sessionPath string) { + t.Helper() + if err := sessionstore.SaveSessionLeaseInfo(sessionPath, sessionstore.SessionLeaseInfo{ + SessionPath: sessionPath, + WriterID: "other-host-1234-deadbeef", + PID: os.Getpid() + 1, + AcquiredAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("SaveSessionLeaseInfo: %v", err) + } + if err := os.Mkdir(store.SessionLeaseLock(sessionstore.CanonicalSessionPath(sessionPath)), 0o755); err != nil { + t.Fatalf("make unopenable lease lock: %v", err) + } +} + func TestRedactSessionsScrubsHistoricalSessionArtifacts(t *testing.T) { dir := testenv.TempDir(t) const secret = "sk-real-secret-value-123456" @@ -181,8 +200,12 @@ func TestRedactSessionsSkipsLeasedSession(t *testing.T) { defer lease.Release() res := RedactSessions(RedactSessionsOptions{Dirs: []string{dir}}) - if res.FilesSkipped != 1 { - t.Fatalf("FilesSkipped = %d, want 1", res.FilesSkipped) + if res.FilesSkipped != 1 || res.FilesSkippedHeld != 1 || res.FilesSkippedUnprovable != 0 { + t.Fatalf("skip counts = total %d, held %d, unprovable %d; want 1/1/0", + res.FilesSkipped, res.FilesSkippedHeld, res.FilesSkippedUnprovable) + } + if len(res.Errors) != 0 { + t.Fatalf("a proven holder is a skip, not an error: %v", res.Errors) } data, err := os.ReadFile(path) if err != nil { @@ -193,6 +216,71 @@ func TestRedactSessionsSkipsLeasedSession(t *testing.T) { } } +// TestRedactSessionsRefusesAnUnprovableLease pins the third state's consequence: +// a probe that cannot prove ownership must not authorize a rewrite, and must not +// be reported as an active lease held by somebody. +func TestRedactSessionsRefusesAnUnprovableLease(t *testing.T) { + dir := testenv.TempDir(t) + const secret = "sk-real-secret-value-123456" + path := filepath.Join(dir, "abc.jsonl") + body := `{"role":"tool","content":"DEEPSEEK_API_KEY=` + secret + `"}` + "\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + installUnopenableLease(t, path) + + res := RedactSessions(RedactSessionsOptions{Dirs: []string{dir}}) + if res.FilesSkipped != 1 || res.FilesSkippedUnprovable != 1 || res.FilesSkippedHeld != 0 { + t.Fatalf("skip counts = total %d, held %d, unprovable %d; want 1/0/1", + res.FilesSkipped, res.FilesSkippedHeld, res.FilesSkippedUnprovable) + } + if len(res.Errors) != 1 || !strings.Contains(res.Errors[0], "could not be proven") { + t.Fatalf("refusal must say the lease could not be proven: %v", res.Errors) + } + if strings.Contains(strings.Join(res.Errors, "\n"), "held") { + t.Fatalf("unprovable lease was reported as held: %v", res.Errors) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != body || !strings.Contains(string(data), secret) { + t.Fatalf("redaction rewrote a session whose ownership it could not prove:\n%s", data) + } +} + +// TestRedactSessionsStillRewritesACrashedLeftover is the free control: a foreign +// sidecar whose lock is free is a leftover, not an unprovable lease, and must +// still be scrubbed. +func TestRedactSessionsStillRewritesACrashedLeftover(t *testing.T) { + dir := testenv.TempDir(t) + const secret = "sk-real-secret-value-123456" + path := filepath.Join(dir, "abc.jsonl") + if err := os.WriteFile(path, []byte(`{"role":"tool","content":"DEEPSEEK_API_KEY=`+secret+`"}`+"\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := sessionstore.SaveSessionLeaseInfo(path, sessionstore.SessionLeaseInfo{ + SessionPath: path, + WriterID: "other-host-1234-deadbeef", + PID: os.Getpid() + 1, + AcquiredAt: time.Now().UTC(), + }); err != nil { + t.Fatal(err) + } + + res := RedactSessions(RedactSessionsOptions{Dirs: []string{dir}}) + if len(res.Errors) != 0 || res.FilesChanged != 1 || res.FilesSkipped != 0 { + t.Fatalf("crashed leftover = %+v, want one rewrite and no skip", res) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(data), secret) { + t.Fatalf("crashed leftover's secret survived:\n%s", data) + } +} + // TestRedactSessionsRemovesDamagedSalvageSidecar pins the salvage-sidecar // privacy gap (#6613 review): the .events.jsonl.damaged file preserves raw // bytes tail repair truncated away, which can include secrets. The bytes are diff --git a/internal/state/sessionstore/recovery_gc.go b/internal/state/sessionstore/recovery_gc.go index 7abd60b1ed..99640b1f85 100644 --- a/internal/state/sessionstore/recovery_gc.go +++ b/internal/state/sessionstore/recovery_gc.go @@ -57,17 +57,15 @@ type recoveryTrashPendingMeta struct { } // SessionLeaseHeld reports whether ANY live runtime — this process included — -// holds the session's write lease. SessionLeaseHeldByOtherRuntime deliberately -// answers false for the current process; GC needs the stricter question, since -// a branch open in one of our own tabs is just as much in use. +// may hold the session's write lease; GC needs the stricter question because a +// branch open in one of our own tabs is just as much in use. An unprovable +// probe counts as held here: these callers only skip work, and the three-state +// question belongs to SessionLeaseOwnership. func SessionLeaseHeld(path string) bool { if strings.TrimSpace(path) == "" { return false } - if _, ok := sessionLeaseOwners.Load(canonicalSessionSavePath(path)); ok { - return true - } - return SessionLeaseHeldByOtherRuntime(path) + return SessionLeaseOwnership(path) != nil } // RecoveryBranchCoveredByParent reports whether a conflict-recovery branch diff --git a/internal/state/sessionstore/session_lease.go b/internal/state/sessionstore/session_lease.go index ed8a2eed49..8d1299ce0b 100644 --- a/internal/state/sessionstore/session_lease.go +++ b/internal/state/sessionstore/session_lease.go @@ -20,6 +20,12 @@ import ( var ErrSessionLeaseHeld = errors.New("session lease held by another runtime") +// ErrSessionLeaseUnprovable means the ownership probe could not decide whether +// a runtime owns the session. It is neither held nor free: an unopenable lock +// or unreadable metadata is not evidence of a writer, so fail-closed callers +// refuse on it and state surfaces report unknown. +var ErrSessionLeaseUnprovable = errors.New("session lease ownership could not be proven") + // sessionLeaseOwners reserves a canonical session path for one in-process // acquisition attempt or live lease. sessionLeaseActiveOwners contains only // generations that have acquired the cross-process lock and written their @@ -169,22 +175,32 @@ func TryReclaimCurrentProcessSessionLease(path string) (*SessionLease, error) { return lease, nil } -// SessionLeaseHeldByOtherRuntime reports whether path's session lease is held -// by a live runtime other than the calling process. Callers use it to keep -// destructive operations away from sessions another process may be writing; -// leases held by this process report false because callers tear their own -// runtimes down before acting. The lock decides whenever a lock file exists, -// including when the sidecar is missing: a holder keeps the lock for its whole -// lifetime and the sidecar can be deleted underneath it. With no lock file the -// common uncontended case touches nothing, and a probe cannot steal a lease. -func SessionLeaseHeldByOtherRuntime(path string) bool { +// SessionLeaseOwnership reports who owns path's session lease: nil when free, +// ErrSessionLeaseHeld when a live runtime (this process included) holds it, and +// ErrSessionLeaseUnprovable when the probe cannot obtain evidence either way. +// The third answer is not a flavour of held: callers that cannot prove a +// session safe refuse it, and listings report unknown. +func SessionLeaseOwnership(path string) error { if strings.TrimSpace(path) == "" { - return false + return nil } path = canonicalSessionSavePath(path) + if _, ok := sessionLeaseOwners.Load(path); ok { + // A reservation or live generation in this process owns the path. A + // pending acquisition reports held too: the caller must not act on a + // session this process is already acquiring. + return ErrSessionLeaseHeld + } + return probeSessionLeaseOwnership(path) +} + +// probeSessionLeaseOwnership asks the OS lock. A held lock proves a holder even +// without a sidecar; a free lock with no sidecar touches nothing; any other +// probe failure leaves ownership unproven. +func probeSessionLeaseOwnership(path string) error { if _, ok := sessionLeaseActiveOwners.Load(path); ok { // Held by this process; no need to touch the lock file. - return false + return nil } info, err := LoadSessionLeaseInfo(path) if err != nil { @@ -193,14 +209,14 @@ func SessionLeaseHeldByOtherRuntime(path string) bool { // a live holder keeps. Ask it only when a lock file exists, using the // package's existing existence probe (a lock has no sidecars). if !sessionArtifactExists(store.SessionLeaseLock(path)) { - return false + return nil } unlock, lockErr := tryLockSessionLeaseFile(path) if lockErr == nil { unlock() - return false + return nil } - return true + return sessionLeaseLockVerdict(path, nil, lockErr) } unlock, lockErr := tryLockSessionLeaseFile(path) if lockErr == nil { @@ -208,23 +224,49 @@ func SessionLeaseHeldByOtherRuntime(path string) bool { // bad metadata so future probes do not keep reporting a ghost owner. _ = os.Remove(sessionLeaseInfoPath(path)) unlock() - return false + return nil } - // An unreadable info file with a live lock still hides the holder's - // identity, so err on the side of treating the session as busy. - return true + // Unreadable metadata hides the holder's identity, so only the lock can + // answer. A lock that is not provably held leaves ownership unproven. + return sessionLeaseLockVerdict(path, nil, lockErr) } if info != nil && info.PID == os.Getpid() && info.WriterID == SessionWriterID() { - return false + return nil } - unlock, err := tryLockSessionLeaseFile(path) - if err == nil { + unlock, lockErr := tryLockSessionLeaseFile(path) + if lockErr == nil { // Foreign info but a free lock: leftover from a crashed process. _ = os.Remove(sessionLeaseInfoPath(path)) unlock() + return nil + } + return sessionLeaseLockVerdict(path, info, lockErr) +} + +// sessionLeaseLockVerdict names the state one failed non-blocking lease-lock +// attempt proves. ErrSessionLeaseHeld is the only failure that proves a live +// holder; anything else is unprovable, with the underlying cause attached so +// the caller can say why. +func sessionLeaseLockVerdict(path string, info *SessionLeaseInfo, lockErr error) error { + if errors.Is(lockErr, ErrSessionLeaseHeld) { + return &SessionLeaseError{Path: path, Info: info} + } + if lockErr == nil { + return ErrSessionLeaseUnprovable + } + return fmt.Errorf("%w: %w", ErrSessionLeaseUnprovable, lockErr) +} + +// SessionLeaseHeldByOtherRuntime reports whether path's session lease may be +// held by a live runtime other than this one, keeping destructive operations +// away from sessions another process may be writing. Unprovable probes answer +// true: a caller that only skips work wants "may be busy", not a claim about +// who is writing. Callers that must not guess use SessionLeaseOwnership. +func SessionLeaseHeldByOtherRuntime(path string) bool { + if strings.TrimSpace(path) == "" { return false } - return true + return probeSessionLeaseOwnership(canonicalSessionSavePath(path)) != nil } // SessionLeaseHeldByCurrentRuntime reports whether this process has completed diff --git a/internal/state/sessionstore/session_lease_test.go b/internal/state/sessionstore/session_lease_test.go index c05a341e06..38e3482b5f 100644 --- a/internal/state/sessionstore/session_lease_test.go +++ b/internal/state/sessionstore/session_lease_test.go @@ -480,3 +480,89 @@ func TestAProbeDoesNotCreateTheLeaseLock(t *testing.T) { t.Fatalf("a probe created the lease lock (stat err = %v); the common uncontended case must touch nothing", err) } } + +// The three ownership answers are different facts, and the probe must not fold +// "I could not tell" into either of the other two. A lock it cannot open is +// not a holder the evidence names: destructive callers get their own state to +// refuse on, while the bool convenience keeps saying "may be busy". +func TestSessionLeaseOwnershipKeepsTheThreeAnswersApart(t *testing.T) { + foreignInfo := func(t *testing.T, key string) { + t.Helper() + if err := SaveSessionLeaseInfo(key, SessionLeaseInfo{ + SessionPath: key, + WriterID: "other-host-1234-deadbeef", + PID: os.Getpid() + 1, + AcquiredAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("SaveSessionLeaseInfo: %v", err) + } + } + + t.Run("free without sidecar or lock", func(t *testing.T) { + userPath, _ := leaseTestPath(t) + if err := SessionLeaseOwnership(userPath); err != nil { + t.Fatalf("SessionLeaseOwnership = %v, want free", err) + } + }) + + t.Run("held by a foreign sidecar and a live lock", func(t *testing.T) { + userPath, key := leaseTestPath(t) + unlock, err := tryLockSessionLeaseFile(key) + if err != nil { + t.Fatalf("tryLockSessionLeaseFile: %v", err) + } + defer unlock() + foreignInfo(t, key) + t.Cleanup(func() { _ = os.Remove(sessionLeaseInfoPath(key)) }) + if err := SessionLeaseOwnership(userPath); !errors.Is(err, ErrSessionLeaseHeld) { + t.Fatalf("SessionLeaseOwnership = %v, want ErrSessionLeaseHeld", err) + } + }) + + t.Run("held lock without a sidecar", func(t *testing.T) { + // The merged #14416 case: deleting the metadata does not free a lock a + // live holder keeps, and the three-state probe must agree with the bool. + userPath, key := leaseTestPath(t) + unlock, err := tryLockSessionLeaseFile(key) + if err != nil { + t.Fatalf("tryLockSessionLeaseFile: %v", err) + } + defer unlock() + if err := SessionLeaseOwnership(userPath); !errors.Is(err, ErrSessionLeaseHeld) { + t.Fatalf("SessionLeaseOwnership = %v, want ErrSessionLeaseHeld", err) + } + }) + + t.Run("crashed foreign leftover is free", func(t *testing.T) { + userPath, key := leaseTestPath(t) + foreignInfo(t, key) + t.Cleanup(func() { _ = os.Remove(sessionLeaseInfoPath(key)) }) + if err := SessionLeaseOwnership(userPath); err != nil { + t.Fatalf("SessionLeaseOwnership = %v, want free (the lock is free)", err) + } + if _, err := os.Stat(sessionLeaseInfoPath(key)); !os.IsNotExist(err) { + t.Fatalf("stale sidecar should still be retired, stat err = %v", err) + } + }) + + t.Run("unopenable lock is unprovable", func(t *testing.T) { + userPath, key := leaseTestPath(t) + foreignInfo(t, key) + t.Cleanup(func() { _ = os.Remove(sessionLeaseInfoPath(key)) }) + // A directory where the lock file belongs: the probe cannot open it, and + // the failure is not ErrSessionLeaseHeld. + if err := os.Mkdir(store.SessionLeaseLock(key), 0o755); err != nil { + t.Fatalf("mkdir lease lock: %v", err) + } + err := SessionLeaseOwnership(userPath) + if !errors.Is(err, ErrSessionLeaseUnprovable) { + t.Fatalf("SessionLeaseOwnership = %v, want ErrSessionLeaseUnprovable", err) + } + if errors.Is(err, ErrSessionLeaseHeld) { + t.Fatal("the unprovable answer also matched ErrSessionLeaseHeld") + } + if !SessionLeaseHeldByOtherRuntime(userPath) { + t.Fatal("the bool convenience stopped saying may-be-busy for an unprovable lease") + } + }) +}