From ac38b59322c4c2bf64748ec247f166a522a0bb10 Mon Sep 17 00:00:00 2001 From: kami Date: Mon, 27 Jul 2026 18:18:50 +0400 Subject: [PATCH] fix(orchestrator): resolve adapters by herdr instance id, not harness kind (B2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdapterFactory.Herdrs is keyed by herdr instance id (e.g. "homesrv-claude"), but Reconcile, expire, and rotate all looked adapters up by session.Harness (the harness kind, e.g. "claude"). In production this key never resolves, so every one of those call sites silently no-ops via a bare `continue`: orphaned panes are never killed on restart, expired leases never kill their pane, and rotation exits before it begins. Add Coordinator.adapterFor(taskID, session), matching the fallback already used correctly by refreshSessionHealth (HerdrID, then the lease's HarnessID, then Harness as a last resort), and route all four call sites through it. Regression test TestAdapterResolvedByHerdrIDNotHarnessKind registers an adapter under "homesrv-claude" and leases with Session.Harness == "claude" (reproducing the real key mismatch) and asserts rotation still fires — the existing rotation tests used a keyed-by-nothing fake adapter that matched any lookup string and so masked this bug entirely. AUDIT.md B2. --- internal/orchestrator/orchestrator.go | 35 +++++++++---- internal/orchestrator/rotation_test.go | 70 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 11 deletions(-) diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 12b95b2..c76d451 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -175,6 +175,25 @@ type SessionHealth struct { LastError string `json:"last_error,omitempty"` } +// adapterFor resolves the herdr adapter for a session. Session.HerdrID (the +// registered herdr instance id, e.g. "homesrv-claude") is authoritative; +// Session.Harness (the harness kind, e.g. "claude") is only a fallback for +// sessions persisted before HerdrID was tracked. Adapters are keyed by +// instance id, so falling back to the lease's harness id (recorded on the +// task) rather than the kind keeps this resolvable even then. +func (c *Coordinator) adapterFor(taskID string, session herdr.Session) (herdr.Adapter, error) { + id := session.HerdrID + if id == "" { + if task, ok := c.Store.Task(taskID); ok && task.Lease != nil { + id = task.Lease.HarnessID + } + } + if id == "" { + id = session.Harness + } + return c.Adapters.Adapter(id) +} + func (c *Coordinator) MonitorHealth() MonitorHealth { c.healthMu.RLock() defer c.healthMu.RUnlock() @@ -218,13 +237,7 @@ func (c *Coordinator) refreshSessionHealth(ctx context.Context) { } c.healthMu.Unlock() for taskID, session := range sessions { - adapterID := session.HerdrID - if adapterID == "" { - if task, ok := c.Store.Task(taskID); ok && task.Lease != nil { - adapterID = task.Lease.HarnessID - } - } - a, err := c.Adapters.Adapter(adapterID) + a, err := c.adapterFor(taskID, session) if err != nil { continue } @@ -288,7 +301,7 @@ func (c *Coordinator) Reconcile(ctx context.Context) error { if ok && t.State == domain.StateLeased { continue } - if a, err := c.Adapters.Adapter(session.Harness); err == nil { + if a, err := c.adapterFor(taskID, session); err == nil { _ = a.Kill(ctx, session) } delete(c.sessions, taskID) @@ -373,7 +386,7 @@ func (c *Coordinator) expire(ctx context.Context) ([]domain.Event, error) { c.mu.Lock() for taskID, s := range c.sessions { if t, ok := c.Store.Task(taskID); ok && t.State == domain.StateLeased { - if a, ae := c.Adapters.Adapter(s.Harness); ae == nil { + if a, ae := c.adapterFor(taskID, s); ae == nil { if p, ok := a.(herdr.PaneExit); ok { if exited, ee := p.PaneExited(ctx, s); ee == nil && exited { b, _ := json.Marshal(map[string]string{"reason": "pane_exited", "harness_id": s.Harness}) @@ -394,7 +407,7 @@ func (c *Coordinator) expire(ctx context.Context) ([]domain.Event, error) { s, ok := c.sessions[e.TaskID] delete(c.sessions, e.TaskID) if ok { - if a, ae := c.Adapters.Adapter(s.Harness); ae == nil { + if a, ae := c.adapterFor(e.TaskID, s); ae == nil { _ = a.Kill(ctx, s) } } @@ -416,7 +429,7 @@ func (c *Coordinator) rotate(ctx context.Context, hard float64) { if !ok || task.State != domain.StateLeased { continue } - a, err := c.Adapters.Adapter(session.Harness) + a, err := c.adapterFor(taskID, session) if err != nil { continue } diff --git a/internal/orchestrator/rotation_test.go b/internal/orchestrator/rotation_test.go index b2fc42f..a2b8737 100644 --- a/internal/orchestrator/rotation_test.go +++ b/internal/orchestrator/rotation_test.go @@ -136,6 +136,76 @@ func TestRotationEmitsValidReleaseWithAnchorSHA(t *testing.T) { func mustJSON(v any) []byte { b, _ := json.Marshal(v); return b } +// keyedHarnessAdapter reports Session.Harness as the harness kind ("claude"), +// distinct from the herdr instance id ("homesrv-claude") under which it is +// registered in AdapterFactory.Herdrs — reproducing production's real key +// mismatch (adapters are keyed by herdr instance id; CLIAdapter.Lease sets +// Session.Harness to the harness kind). +type keyedHarnessAdapter struct{ fakeAdapter } + +func (a *keyedHarnessAdapter) Lease(_ context.Context, _ string, worktree string) (herdr.Session, error) { + return herdr.Session{Harness: "claude", PaneID: "pane-1", Worktree: worktree}, nil +} + +// TestAdapterResolvedByHerdrIDNotHarnessKind guards B2: AdapterFactory.Herdrs +// is keyed by herdr instance id (e.g. "homesrv-claude"), never by the +// harness kind Session.Harness holds (e.g. "claude"). Reconcile, expire, and +// rotate must all resolve the adapter via Session.HerdrID (set at lease +// time), not Session.Harness, or every one of them silently no-ops via a +// bare Adapter-not-registered continue. +func TestAdapterResolvedByHerdrIDNotHarnessKind(t *testing.T) { + repo := t.TempDir() + run(t, repo, "init") + run(t, repo, "config", "user.email", "t@t") + run(t, repo, "config", "user.name", "t") + run(t, repo, "commit", "--allow-empty", "-m", "init") + + s, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if err := s.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: "t1", Surface: string(authz.System), Payload: mustJSON(map[string]any{ + "source": "jsonl", "external_id": "1", "project": "p", + })}); err != nil { + t.Fatal(err) + } + task := s.Tasks()[0] + ref, err := s.PutArtifact([]byte("handoff")) + if err != nil { + t.Fatal(err) + } + a := &keyedHarnessAdapter{fakeAdapter{occupancy: .95, boundary: true, ref: ref}} + factory := orchestrator.AdapterFactory{Herdrs: map[string]herdr.Adapter{"homesrv-claude": a}} + c := &orchestrator.Coordinator{Store: s, Worktrees: worktrees{path: repo}, Adapters: factory, StatePath: t.TempDir() + "/sessions.json"} + + leaseEvt, err := s.Lease(task.ID, "homesrv-claude", time.Minute) + if err != nil { + t.Fatal(err) + } + if err := c.Start(context.Background(), leaseEvt); err != nil { + t.Fatal(err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go c.Monitor(ctx, .8, time.Millisecond) + + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + if got, ok := s.Task(task.ID); ok && got.State == domain.StateQueued { + break + } + time.Sleep(time.Millisecond) + } + got, ok := s.Task(task.ID) + if !ok || got.State != domain.StateQueued { + t.Fatalf("rotation did not complete via herdr-id-keyed adapter: state=%v ok=%v", got.State, ok) + } + if a.releases == 0 { + t.Fatalf("adapter Release was never invoked — adapter lookup used Session.Harness instead of Session.HerdrID") + } +} + // erroringBoundaryAdapter supports Face B but its probe always fails — this // must block release (never silently treat an unanswerable boundary check // as safe to interrupt), unlike an adapter that doesn't implement the