From 3945b10f2057743919d33799dd9144c4a4de8324 Mon Sep 17 00:00:00 2001 From: harshitsinghbhandari <24b4506@iitb.ac.in> Date: Wed, 27 May 2026 01:37:14 +0530 Subject: [PATCH] fix(lifecycle): address PR #5 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - shouldWriteSessionRuntime: never resurrect a terminal session; an observation may refresh the runtime axis but must touch neither the session axis nor the detecting memory (gated in ApplyRuntimeObservation). - OnSpawnCompleted: error on an unseeded session instead of fabricating a partial record (SM must seed first — a missing seed is a contract violation). - OnKillRequested: no-op on an unknown/already-gone session (benign race) instead of fabricating a terminal record. - keyedMutex: reference-count entries and evict on last release so the lock map stays bounded in a long-running daemon. - runtimeSubstateFromFacts: map RuntimeProbeIndeterminate to RuntimeUnknown with a neutral reason, distinct from the probe_error of a failed probe. Adds tests for terminal non-resurrection, unseeded spawn-completed error, and unknown-session kill no-op. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/internal/lifecycle/decide_bridge.go | 16 ++++-- backend/internal/lifecycle/manager.go | 59 +++++++++++++++++---- backend/internal/lifecycle/manager_test.go | 40 ++++++++++++++ 3 files changed, 101 insertions(+), 14 deletions(-) diff --git a/backend/internal/lifecycle/decide_bridge.go b/backend/internal/lifecycle/decide_bridge.go index 059ac2eb6..d1ac7f65f 100644 --- a/backend/internal/lifecycle/decide_bridge.go +++ b/backend/internal/lifecycle/decide_bridge.go @@ -71,8 +71,12 @@ func runtimeSubstateFromFacts(f ports.RuntimeFacts) domain.RuntimeSubstate { return domain.RuntimeSubstate{State: domain.RuntimeExited, Reason: domain.RuntimeReasonTmuxMissing} case ports.RuntimeProbeFailed: return domain.RuntimeSubstate{State: domain.RuntimeProbeFailed, Reason: domain.RuntimeReasonProbeError} - default: - return domain.RuntimeSubstate{State: domain.RuntimeUnknown, Reason: domain.RuntimeReasonProbeError} + case ports.RuntimeProbeIndeterminate: + // Probe ran but couldn't tell — distinct from a probe error, so no + // probe_error reason; the ambiguity is carried by RuntimeUnknown alone. + return domain.RuntimeSubstate{State: domain.RuntimeUnknown} + default: // unset + return domain.RuntimeSubstate{State: domain.RuntimeUnknown} } } @@ -158,8 +162,14 @@ func isLivenessOwned(s domain.SessionSubstate) bool { // (e.g. detecting -> working); it must NOT clobber an activity-owned // needs_input/blocked/idle the activity axis is responsible for. func shouldWriteSessionRuntime(d decide.LifecycleDecision, cur domain.CanonicalSessionLifecycle) bool { + if isTerminal(cur.Session.State) { + // A terminal session is only reopened by an explicit Restore — never by + // an observation. Even a death-axis verdict (e.g. detecting) must not + // resurrect it; the runtime axis is still patched separately. + return false + } if d.SessionState == domain.SessionWorking { - return !isTerminal(cur.Session.State) && isLivenessOwned(cur.Session) + return isLivenessOwned(cur.Session) } return true } diff --git a/backend/internal/lifecycle/manager.go b/backend/internal/lifecycle/manager.go index 55a594bb4..b7f9d0aaa 100644 --- a/backend/internal/lifecycle/manager.go +++ b/backend/internal/lifecycle/manager.go @@ -11,6 +11,7 @@ package lifecycle import ( "context" + "fmt" "sync" "time" @@ -54,25 +55,43 @@ func New(store ports.LifecycleStore, notifier ports.Notifier, messenger ports.Ag // keyedMutex hands out one lock per session id so the load->decide->persist // read-modify-write is serial within a session but parallel across sessions. +// +// Entries are reference-counted and evicted when the last holder releases, so +// the map stays bounded to sessions with in-flight operations rather than +// growing unbounded over the lifetime of a long-running daemon. type keyedMutex struct { mu sync.Mutex - locks map[domain.SessionID]*sync.Mutex + locks map[domain.SessionID]*lockEntry +} + +type lockEntry struct { + mu sync.Mutex + refs int } func (k *keyedMutex) lock(id domain.SessionID) func() { k.mu.Lock() if k.locks == nil { - k.locks = make(map[domain.SessionID]*sync.Mutex) + k.locks = make(map[domain.SessionID]*lockEntry) } - m, ok := k.locks[id] + e, ok := k.locks[id] if !ok { - m = &sync.Mutex{} - k.locks[id] = m + e = &lockEntry{} + k.locks[id] = e } + e.refs++ k.mu.Unlock() - m.Lock() - return m.Unlock + e.mu.Lock() + return func() { + e.mu.Unlock() + k.mu.Lock() + e.refs-- + if e.refs == 0 { + delete(k.locks, id) + } + k.mu.Unlock() + } } func (m *Manager) withLock(id domain.SessionID, fn func() error) error { @@ -127,10 +146,15 @@ func (m *Manager) ApplyRuntimeObservation(ctx context.Context, id domain.Session patch.Runtime = &rt changed = true } - if shouldWriteSessionRuntime(d, cur) { - changed = setSessionIfChanged(&patch, cur, d.SessionState, d.SessionReason) || changed + // A terminal session is reopened only by an explicit Restore: an + // observation may refresh the runtime axis above but must touch neither + // the session axis nor the detecting memory. + if !isTerminal(cur.Session.State) { + if shouldWriteSessionRuntime(d, cur) { + changed = setSessionIfChanged(&patch, cur, d.SessionState, d.SessionReason) || changed + } + changed = setDetecting(&patch, cur, d.Detecting) || changed } - changed = setDetecting(&patch, cur, d.Detecting) || changed return patch, changed, nil }) @@ -203,10 +227,16 @@ func (m *Manager) ApplyActivitySignal(ctx context.Context, id domain.SessionID, // (display: spawning) — the agent "acknowledges" via the first activity signal. func (m *Manager) OnSpawnCompleted(ctx context.Context, id domain.SessionID, o ports.SpawnOutcome) error { return m.withLock(id, func() error { - cur, _, err := m.store.Load(ctx, id) + cur, exists, err := m.store.Load(ctx, id) if err != nil { return err } + if !exists { + // The SM seeds the initial lifecycle before spawning; a completion + // for an unseeded session is a contract violation, not a stray + // observation, so surface it rather than fabricating a record. + return fmt.Errorf("lifecycle: OnSpawnCompleted for unseeded session %q", id) + } rt := domain.RuntimeSubstate{State: domain.RuntimeAlive, Reason: domain.RuntimeReasonProcessRunning} if cur.Runtime != rt { if err := m.store.PatchLifecycle(ctx, id, ports.LifecyclePatch{Runtime: &rt}); err != nil { @@ -228,6 +258,13 @@ func (m *Manager) OnSpawnCompleted(ctx context.Context, id domain.SessionID, o p // in-flight detecting memory. func (m *Manager) OnKillRequested(ctx context.Context, id domain.SessionID, r ports.KillReason) error { return m.mutate(ctx, id, func(cur domain.CanonicalSessionLifecycle, exists bool) (ports.LifecyclePatch, bool, error) { + if !exists { + // Killing an unknown/already-gone session is a benign race; no-op + // rather than fabricating a terminal record for a session we never + // knew about. + return ports.LifecyclePatch{}, false, nil + } + var patch ports.LifecyclePatch changed := false diff --git a/backend/internal/lifecycle/manager_test.go b/backend/internal/lifecycle/manager_test.go index 6f9f7f717..dae266b87 100644 --- a/backend/internal/lifecycle/manager_test.go +++ b/backend/internal/lifecycle/manager_test.go @@ -123,6 +123,25 @@ func TestApplyRuntimeObservation_NoRecordIsNoOp(t *testing.T) { } } +func TestApplyRuntimeObservation_DoesNotResurrectTerminal(t *testing.T) { + mgr, store := newManager() + store.seed(sid, lc(domain.SessionTerminated, domain.ReasonManuallyKilled, domain.RuntimeExited)) + + // A failed probe would normally route to detecting, but a terminal session + // must not be reopened by an observation (only an explicit Restore does). + if err := mgr.ApplyRuntimeObservation(context.Background(), sid, ports.RuntimeFacts{RuntimeState: ports.RuntimeProbeFailed, ProcessState: ports.ProcessProbeAlive, ObservedAt: t0}); err != nil { + t.Fatalf("apply: %v", err) + } + + l := mustLoad(t, store) + if l.Session.State != domain.SessionTerminated || l.Session.Reason != domain.ReasonManuallyKilled { + t.Errorf("session = %v/%v, want terminated/manually_killed (no resurrection)", l.Session.State, l.Session.Reason) + } + if l.Detecting != nil { + t.Errorf("terminal session must not gain detecting memory, got %+v", l.Detecting) + } +} + // ---- ApplyActivitySignal ---- func TestApplyActivitySignal(t *testing.T) { @@ -314,6 +333,27 @@ func TestOnKillRequested(t *testing.T) { } } +func TestOnSpawnCompleted_UnseededErrors(t *testing.T) { + mgr, store := newManager() + err := mgr.OnSpawnCompleted(context.Background(), sid, ports.SpawnOutcome{Branch: "x"}) + if err == nil { + t.Error("OnSpawnCompleted for an unseeded session must error, not fabricate a record") + } + if _, ok, _ := store.Load(context.Background(), sid); ok { + t.Error("no record should have been created") + } +} + +func TestOnKillRequested_UnseededIsNoOp(t *testing.T) { + mgr, store := newManager() + if err := mgr.OnKillRequested(context.Background(), sid, ports.KillReason{Kind: ports.KillManual}); err != nil { + t.Fatalf("kill of unknown session should be a benign no-op, got %v", err) + } + if _, ok, _ := store.Load(context.Background(), sid); ok { + t.Error("killing an unknown session must not fabricate a terminal record") + } +} + func TestTickEscalationsIsNoOp(t *testing.T) { mgr, store := newManager() store.seed(sid, lc(domain.SessionWorking, domain.ReasonTaskInProgress, domain.RuntimeAlive))