From 9d11d15ecc0ceca9ad60e1608d023614f3cf3eb2 Mon Sep 17 00:00:00 2001 From: mark wang Date: Thu, 29 Oct 2026 12:11:34 -0400 Subject: [PATCH 1/2] fix(processgroup): one undecidable group no longer hides the rest A registered group whose boot identity Core cannot decide (an older Darwin agent's clock-derived boot id) is rightly kept without signaling. But the preview aborted on the first such group, so `codefly clear --dry-run` listed nothing and stopped before counting sockets (cli#1233). The preview now names the group and its recovery (`codefly recover process-group PGID --preview`) and keeps listing; the stop path names it the same way; clear's dry run prints what it could list. Co-Authored-By: Claude Opus 5.5 --- cmd/clear.go | 10 +++-- docs/orchestration.md | 5 ++- pkg/processgroup/registered.go | 28 +++++++++--- pkg/processgroup/registered_test.go | 70 +++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 10 deletions(-) diff --git a/cmd/clear.go b/cmd/clear.go index ea792815..dd8a1b52 100644 --- a/cmd/clear.go +++ b/cmd/clear.go @@ -188,19 +188,23 @@ func clearCommand(ctx context.Context, args []string, options clearOptions) (ret // files under ~/.codefly/runs — or counts it on a dry run. func sweepStaleState(ctx context.Context, w *wool.Wool, options clearOptions) []error { if options.dryRun { + var failures []error if !options.keepProcesses { + // A group the preview cannot decide is named in err and kept; the + // rest are still listed, and the dry run goes on to count sockets. groups, err := processgroup.PreviewManagedProcessGroups(ctx, options.scope) + w.Info("would stop registered process groups", wool.Field("count", len(groups)), wool.Field("pgids", groups)) if err != nil { - return []error{err} + w.Warn("would keep process groups whose ownership cannot be decided", wool.ErrField(err)) + failures = append(failures, err) } - w.Info("would stop registered process groups", wool.Field("count", len(groups)), wool.Field("pgids", groups)) } if n := manager.CountStaleAgentSockets(); n > 0 { w.Info("would remove stale agent sockets", wool.Field("count", n)) } else { w.Info("no stale agent sockets") } - return nil + return failures } if n := manager.SweepStaleAgentSockets(); n > 0 { w.Info("removed stale agent sockets", wool.Field("count", n)) diff --git a/docs/orchestration.md b/docs/orchestration.md index ac30f70c..c3e357da 100644 --- a/docs/orchestration.md +++ b/docs/orchestration.md @@ -1150,7 +1150,10 @@ recover its groups; upgrade the cleanup consumer along with the producer. error when a live leader has the recorded start identity but a different boot identity. Older Darwin agents wrote a boot timestamp where current agents write a boot-session UUID. This mismatch does not prove PID reuse: cleanup neither -signals the group nor discards its recovery evidence. Recover the owning run +signals the group nor discards its recovery evidence. Such a group is kept and +named, with `codefly recover process-group PGID --preview`, in the error; it +does not stop a preview (`codefly clear --dry-run`) from listing every other +group, nor a stop from handling them. Recover the owning run and update its agent before restarting. A successful stop must not hide a process it could not authenticate. If registry cleanup fails, stop also skips the signature-based orphan fallback so it cannot bypass that refusal. diff --git a/pkg/processgroup/registered.go b/pkg/processgroup/registered.go index 12c90958..51a72c79 100644 --- a/pkg/processgroup/registered.go +++ b/pkg/processgroup/registered.go @@ -9,29 +9,45 @@ import ( ) // PreviewManagedProcessGroups lists live, authenticated, in-scope registrations -// without signaling or removing records. Refusals match managed stop admission. +// without signaling or removing records. Refusals match managed stop admission: +// a group whose ownership cannot be decided is left out and named in the error, +// with how to recover it, and every other group is still listed. func PreviewManagedProcessGroups(ctx context.Context, scope Scope) ([]int, error) { groups, err := runners.ListProcessGroups(ctx) if err != nil { return nil, err } var selected []int + var kept []error for _, group := range groups { if !group.Alive() { continue } ownership, err := group.InspectOwnership(ctx) if err != nil { - return nil, fmt.Errorf("inspect registered group %d: %w", group.PGID(), err) + kept = append(kept, undecided(group.PGID(), err)) + continue } if !ownership.Authenticated { - return nil, fmt.Errorf("group %d ownership is unproven", group.PGID()) + kept = append(kept, undecided(group.PGID(), errUnproven)) + continue } if registeredGroupInScope(group, ownership, scope) { selected = append(selected, group.PGID()) } } - return selected, nil + return selected, errors.Join(kept...) +} + +// errUnproven: the group's members do not prove they belong to its record. +var errUnproven = errors.New("live group ownership is unproven") + +// undecided is a live registered group Core cannot decide is the one its +// record describes (an unproven owner, or a boot identity an older writer +// recorded differently). It is kept without signaling; the error names it and +// the supported way to recover it, so one such group never hides the others. +func undecided(pgid int, err error) error { + return fmt.Errorf("inspect registered group %d: %w; kept without signaling, recover it with `codefly recover process-group %d --preview`", pgid, err, pgid) } // Core owns supported registry discovery, authentication and identity-bound @@ -70,10 +86,10 @@ func reconcileRegisteredGroup(ctx context.Context, group *runners.TrackedProcess } ownership, err := group.InspectOwnership(ctx) if err != nil { - return CleanupFailed, false, err + return CleanupFailed, false, undecided(group.PGID(), err) } if !ownership.Authenticated { - return CleanupFailed, false, fmt.Errorf("live group ownership is unproven; registration retained without signaling") + return CleanupFailed, false, undecided(group.PGID(), errUnproven) } if !registeredGroupInScope(group, ownership, policy.scope) { return CleanupClean, true, nil diff --git a/pkg/processgroup/registered_test.go b/pkg/processgroup/registered_test.go index e5606ff4..dd9360ed 100644 --- a/pkg/processgroup/registered_test.go +++ b/pkg/processgroup/registered_test.go @@ -2,11 +2,13 @@ package processgroup import ( "context" + "encoding/json" "io" "os" "os/exec" "path/filepath" "strconv" + "strings" "syscall" "testing" "time" @@ -92,3 +94,71 @@ func TestRegisteredStopUsesPersistedWorkspaceAfterOwnerAndEnvironmentDisappear(t t.Fatalf("target missing from recovery evidence: %+v", evidence) } } + +// legacyBootID rewrites a group's registry record as an older Darwin writer +// left it: the boot id differs while the leader's pid and start are unchanged, +// which Core refuses to decide (checkBootIdentity) and keeps the record for. +func legacyBootID(t *testing.T, pgid int) { + t.Helper() + home, err := os.UserHomeDir() + if err != nil { + t.Fatal(err) + } + paths, err := filepath.Glob(filepath.Join(home, ".codefly", "*", "authenticated-v2", strconv.Itoa(pgid)+".pgid")) + if err != nil || len(paths) != 1 { + t.Fatalf("record for %d: %v %v", pgid, paths, err) + } + raw, err := os.ReadFile(paths[0]) + if err != nil { + t.Fatal(err) + } + var record map[string]any + if err := json.Unmarshal(raw, &record); err != nil { + t.Fatal(err) + } + // An older writer recorded one boot id for the leader and the owner alike. + for _, field := range []string{"leader", "owner"} { + identity, ok := record[field].(map[string]any) + if !ok { + t.Fatalf("record has no %s: %s", field, raw) + } + identity["boot_id"] = "1793000000:123456" + } + out, err := json.Marshal(record) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(paths[0], out, 0o600); err != nil { + t.Fatal(err) + } +} + +// One group whose boot identity Core cannot decide is kept and named, with +// the command that recovers it, but it no longer stops the preview or the +// stop from handling every other group in the workspace (cli#1233). +func TestAnUndecidableGroupIsKeptWithoutStoppingTheRest(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + workspace := newWorkspace(t, "first") + undecidable := startUnobservableRun(t, workspace, workspace) + healthy := startUnobservableRun(t, workspace, workspace) + legacyBootID(t, undecidable) + + preview, err := PreviewManagedProcessGroups(context.Background(), InWorkspace(workspace)) + if len(preview) != 1 || preview[0] != healthy { + t.Fatalf("preview=%v err=%v, want only the healthy group %d", preview, err, healthy) + } + if err == nil || !strings.Contains(err.Error(), strconv.Itoa(undecidable)) || !strings.Contains(err.Error(), "codefly recover process-group "+strconv.Itoa(undecidable)) { + t.Fatalf("preview error %v does not name the undecidable group and its recovery", err) + } + + _, err = StopManagedProcessGroups(context.Background(), InWorkspace(workspace)) + if groupAlive(healthy) { + t.Fatal("the healthy group survived the stop") + } + if !groupAlive(undecidable) { + t.Fatal("the undecidable group was signaled") + } + if err == nil || !strings.Contains(err.Error(), "codefly recover process-group "+strconv.Itoa(undecidable)) { + t.Fatalf("stop error %v does not name the undecidable group's recovery", err) + } +} From 82691533424918f20f22edf39462f06b1f717538 Mon Sep 17 00:00:00 2001 From: mark wang Date: Thu, 29 Oct 2026 12:27:41 -0400 Subject: [PATCH 2/2] test(clear): the dry run reports past an undecidable group Review on #1236: a regression test for clear's dry-run output. With one group recorded by an older agent, the dry run logs the healthy group it would stop, warns about the kept group with its recovery command, counts the stale agent socket, returns the refusal, and signals nothing. It fails with either half of the fix reverted. Co-Authored-By: Claude Opus 5.5 --- cmd/clear_dryrun_test.go | 136 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) create mode 100644 cmd/clear_dryrun_test.go diff --git a/cmd/clear_dryrun_test.go b/cmd/clear_dryrun_test.go new file mode 100644 index 00000000..5563f244 --- /dev/null +++ b/cmd/clear_dryrun_test.go @@ -0,0 +1,136 @@ +package cmd + +import ( + "context" + "encoding/json" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "sync" + "syscall" + "testing" + + runners "github.com/codefly-dev/core/runners/base" + "github.com/codefly-dev/core/wool" + + "github.com/codefly-dev/cli/pkg/processgroup" +) + +// capturedLogs keeps every line a command writes, with its fields. +type capturedLogs struct { + mu sync.Mutex + logs []*wool.Log +} + +func (c *capturedLogs) Process(msg *wool.Log) { + c.mu.Lock() + defer c.mu.Unlock() + c.logs = append(c.logs, msg) +} + +// line is the captured log whose message is msg, with its fields as text. +func (c *capturedLogs) line(msg string) (string, bool) { + c.mu.Lock() + defer c.mu.Unlock() + for _, l := range c.logs { + if l.Message != msg { + continue + } + var fields []string + for _, f := range l.Fields { + fields = append(fields, fmt.Sprintf("%s=%v", f.Key, f.Value)) + } + return strings.Join(fields, " "), true + } + return "", false +} + +// trackedSleep starts a real tracked process group, killed with the test. +func trackedSleep(t *testing.T) int { + t.Helper() + group, err := runners.StartTrackedProcessGroup(exec.Command("sleep", "60")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = syscall.Kill(-group.PGID(), syscall.SIGKILL) }) + return group.PGID() +} + +// recordedByAnOlderAgent rewrites a group's record as an older Darwin writer +// left it: one clock-derived boot id for leader and owner, which Core refuses +// to decide while the leader's pid and start are unchanged. +func recordedByAnOlderAgent(t *testing.T, home string, pgid int) { + t.Helper() + paths, err := filepath.Glob(filepath.Join(home, ".codefly", "*", "authenticated-v2", strconv.Itoa(pgid)+".pgid")) + if err != nil || len(paths) != 1 { + t.Fatalf("record for %d: %v %v", pgid, paths, err) + } + raw, err := os.ReadFile(paths[0]) + if err != nil { + t.Fatal(err) + } + var record map[string]any + if err := json.Unmarshal(raw, &record); err != nil { + t.Fatal(err) + } + for _, field := range []string{"leader", "owner"} { + identity, ok := record[field].(map[string]any) + if !ok { + t.Fatalf("record has no %s: %s", field, raw) + } + identity["boot_id"] = "1793000000:123456" + } + out, err := json.Marshal(record) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(paths[0], out, 0o600); err != nil { + t.Fatal(err) + } +} + +// A dry run with one group Core cannot decide still says everything it can: +// the healthy group it would stop, a warning naming the kept group and its +// recovery, and the stale sockets it would remove; and it returns the +// refusal rather than hiding it (cli#1233). +func TestClearDryRunReportsPastAnUndecidableGroup(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("TMPDIR", t.TempDir()) + healthy, undecidable := trackedSleep(t), trackedSleep(t) + recordedByAnOlderAgent(t, home, undecidable) + + // One stale agent socket: its owner's pid is dead. + gone := exec.Command("true") + if err := gone.Run(); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(os.TempDir(), fmt.Sprintf("codefly-uds-%d-x", gone.Process.Pid)), 0o700); err != nil { + t.Fatal(err) + } + + logs := &capturedLogs{} + w := wool.Get(context.Background()).WithLogger(logs) + failures := sweepStaleState(context.Background(), w, clearOptions{dryRun: true, scope: processgroup.AllWorkspaces()}) + + groups, ok := logs.line("would stop registered process groups") + if !ok || !strings.Contains(groups, strconv.Itoa(healthy)) || strings.Contains(groups, strconv.Itoa(undecidable)) { + t.Fatalf("would stop %q (logged %v), want only the healthy group %d", groups, ok, healthy) + } + warning, ok := logs.line("would keep process groups whose ownership cannot be decided") + if !ok || !strings.Contains(warning, "codefly recover process-group "+strconv.Itoa(undecidable)) { + t.Fatalf("ownership warning %q (logged %v) does not name group %d's recovery", warning, ok, undecidable) + } + if sockets, ok := logs.line("would remove stale agent sockets"); !ok || !strings.Contains(sockets, "count=1") { + t.Fatalf("stale sockets %q (logged %v), want count 1", sockets, ok) + } + if len(failures) != 1 || !strings.Contains(failures[0].Error(), "inspect registered group "+strconv.Itoa(undecidable)) { + t.Fatalf("returned %v, want the undecidable group's refusal", failures) + } + if syscall.Kill(-healthy, 0) != nil || syscall.Kill(-undecidable, 0) != nil { + t.Fatal("the dry run signaled a group") + } +}