fix(lifecycle): address PR #5 review
- 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) <noreply@anthropic.com>
This commit is contained in:
parent
1420bb9493
commit
3945b10f20
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
Loading…
Reference in New Issue