diff --git a/docs/adr/0027-cloud-native.md b/docs/adr/0027-cloud-native.md index b8c67f0b67..e14d403476 100644 --- a/docs/adr/0027-cloud-native.md +++ b/docs/adr/0027-cloud-native.md @@ -826,7 +826,7 @@ durable artifact survives and is reloaded), or **lost** (gone, possibly leaking) | 24 | `modelRouterBreaker` (per-run model-router circuit breaker, ADR 0031; ADR 0034 reuses the SAME per-run breaker for team members + Parallel branches — NO new breaker) | `agent.Run` | run | dies with the run | lost (run-scoped by design, mirroring `askReviewBreaker` row 22) | `engine/agent/modelrouter.go` (`modelRouterBreaker`); armed in `engine/agent/loop.go` (`startRun`) | | 25 | `gitWorktreeLister` (osfs-backed worktree discovery, issue #102) | `app.Build` | process lifetime | none (value type, no goroutine, no Close needed) | reconstructible (rebuilt from `cfg.Shell` at next Build; no state) | `internal/app/build.go` (`buildWorktreeLister`) | | 26 | routed team-member / Parallel-branch child engine (ADR 0034) | `buildMemberEngine` (member) / `buildParallelEngineFactory` (branch), minted in composition | per-AddMember (member; reused across rounds, torn down on member teardown) / per-call (branch; torn down with the branch fork) | dies with the member/branch (the SAME lifecycle as the non-routed member/branch engine it replaces — routing changes only the model, not the lifetime) | reconstructible (a new team/Parallel call re-classifies + re-mints); decision = derive (nothing persisted; the routed model is List-2 row 18) | `internal/app/build.go` (`buildMemberEngine`, `buildParallelEngineFactory`) | -| 27 | held session leases + per-session renewer goroutines (`Service.heldLeases`, Phase 4) + local retained generation-liveness flock fds + local mutation-capability/lost-owner maps (`SessionMutationCapability`, `Service.lostOwnership`, ADR 0294) | `server.Service` owns the lifecycle and shares the capability with its guarded SessionStore/EventLog/ToolCallRecorder projections; `flocklease.Lease` owns each local generation fd | session (one lease + renewer, retained generation fd, and capability state per leased session; one lightweight lost-owner entry until local teardown after definitive renewal loss) | acquire grants the capability. Close and joined graceful drain cancel renewal, invalidate locally, then release with a cancel-detached bounded context. Lease loss or drain timeout invalidates before cancelling the run; a definitive non-awaiting loss releases the exact generation, while awaiting loss retains the invalid hold without Release so takeover preserves the durable handoff point. Capability and held-lease tombstones are removed when stale lifecycle references settle, while `lostOwnership` prevents that stale Service from reacquiring until explicit `CloseSession` teardown — the one narrow exception being the stale-session reconcile sweep's `SettleIfStale`, whose caller is independently pre-verified (age-horizon + local liveness) as recovering a genuine crash orphan rather than a live handoff, and which clears `lostOwnership` itself on a successful re-Acquire (`acquireLeaseCore`'s `bypassTombstone` path, used only via `acquireMutationLeaseForStaleSettle`). Awaiting loss also retracts the local ask while preserving the durable snapshot. No mutation is admitted after invalidation starts, while an already-admitted backend call may still finish because this is local invalidation rather than token-bearing storage fencing. Local exact-token Release durably tombstones then closes/unlocks its generation fd; stale release cannot touch a successor, the stable per-session sentinel is operation-scoped, and failure paths close newly opened handles or retain failed-close references for retry. SIGKILL makes the kernel close every retained fd | **reconstructible/reset-by-design** (a restart starts with empty capability/lost-owner maps and re-acquires on the next admitted operation; record expiry preserves the generic TTL takeover contract while a crashed local holder's generation flock releases immediately so a survivor can take over before expiry and increment the token preserved in the durable record. No local validity/tombstone is persisted, and it must not survive process identity. The durable awaiting `PendingAsk` and session state remain in the snapshot; see List 2 row 42). Constructed when a lease backend is explicitly wired or store-provided, and automatically as the existing flock lease beneath every local JSONL StoreDir. Other no-lease shareable stores do not gain destructive-maintenance authority; automatic retention fails closed | `internal/adapter/server/service.go` (`heldLeases`, `lostOwnership`, `acquireLease`, `acquireLeaseCore`, `acquireMutationLeaseForStaleSettle`, `renewLoop`, `onLeaseLost`, `GracefulDrain`, `releaseLease`); `internal/adapter/server/mutation_capability.go` (`SessionMutationCapability`); `internal/adapter/flocklease/flocklease.go` (`Lease`, `heldLease`, `Acquire`, `Release`); wired at `internal/app/build.go` (`buildSessionLease`) | +| 27 | held session leases + per-session renewer goroutines (`Service.heldLeases`, Phase 4) + local retained generation-liveness flock fds + local mutation-capability/lost-owner maps (`SessionMutationCapability`, `Service.lostOwnership`, ADR 0294) | `server.Service` owns the lifecycle and shares the capability with its guarded SessionStore/EventLog/ToolCallRecorder projections; `flocklease.Lease` owns each local generation fd | session (one lease + renewer, retained generation fd, and capability state per leased session; one lightweight lost-owner entry until local teardown after definitive renewal loss) | acquire grants the capability. Close and joined graceful drain cancel renewal, invalidate locally, then release with a cancel-detached bounded context. Lease loss or drain timeout invalidates before cancelling the run; a definitive non-awaiting loss releases the exact generation, while awaiting loss retains the invalid hold without Release so takeover preserves the durable handoff point. Capability and held-lease tombstones are removed when stale lifecycle references settle, while `lostOwnership` prevents that stale Service from reacquiring until explicit `CloseSession` teardown — with TWO independent, narrowly-scoped exceptions, both root-authorized (`stale-session-reconcile`) and both re-verifying under `s.mu` before writing: (1) the stale-session reconcile sweep's `SettleIfStale`, whose caller is independently pre-verified (age-horizon + local liveness) as recovering a genuine crash orphan rather than a live handoff, and which clears `lostOwnership` itself on a successful re-Acquire (`acquireLeaseCore`'s `bypassTombstone` path, used only via `acquireMutationLeaseForStaleSettle`) — this covers ONLY a `StateRunning` candidate; and (2) `ReconcileLeaseLossTombstone` (issue #1334), the awaiting/cancelled counterpart, for the sessions `onLeaseLost` itself drives OUT of `StateRunning` while handling the very loss that set the tombstone (to `awaiting` via the `preserveAwaiting` branch, or eventually `cancelled`) — a population `SettleIfStale`'s `StateRunning`-only candidacy can never rediscover. It performs a bounded TRIAL Acquire+immediate-Release (the shared `leaseTrial` helper, also used by `SessionStale`'s own refinement) against exactly the ids `Service.lostOwnership` already names (an in-memory read, never a store-wide scan — that population is the steady state for huge numbers of ordinary finished sessions), clears the tombstone plus any stale invalid `heldLeases` entry `onLeaseLost`'s `preserveAwaiting` branch left behind, and never holds a lease or repairs session state itself: the next genuine run-entry's existing `loadAndReopen`/`resumeFromAwaiting` still does that. Wired into the SAME composition-level sweep pass as `SettleIfStale` (`internal/app/session_reconcile.go`'s `reconcileLeaseLossTombstones`), no new goroutine. Awaiting loss also retracts the local ask while preserving the durable snapshot. No mutation is admitted after invalidation starts, while an already-admitted backend call may still finish because this is local invalidation rather than token-bearing storage fencing. Local exact-token Release durably tombstones then closes/unlocks its generation fd; stale release cannot touch a successor, the stable per-session sentinel is operation-scoped, and failure paths close newly opened handles or retain failed-close references for retry. SIGKILL makes the kernel close every retained fd | **reconstructible/reset-by-design** (a restart starts with empty capability/lost-owner maps and re-acquires on the next admitted operation; record expiry preserves the generic TTL takeover contract while a crashed local holder's generation flock releases immediately so a survivor can take over before expiry and increment the token preserved in the durable record. No local validity/tombstone is persisted, and it must not survive process identity. The durable awaiting `PendingAsk` and session state remain in the snapshot; see List 2 row 42). Constructed when a lease backend is explicitly wired or store-provided, and automatically as the existing flock lease beneath every local JSONL StoreDir. Other no-lease shareable stores do not gain destructive-maintenance authority; automatic retention fails closed | `internal/adapter/server/service.go` (`heldLeases`, `lostOwnership`, `acquireLease`, `acquireLeaseCore`, `acquireMutationLeaseForStaleSettle`, `leaseTrial`, `SessionStale`, `ReconcileLeaseLossTombstone`, `LostOwnershipCandidates`, `renewLoop`, `onLeaseLost`, `GracefulDrain`, `releaseLease`); `internal/app/session_reconcile.go` (`reconcileLeaseLossTombstones`); `internal/adapter/server/mutation_capability.go` (`SessionMutationCapability`); `internal/adapter/flocklease/flocklease.go` (`Lease`, `heldLease`, `Acquire`, `Release`); wired at `internal/app/build.go` (`buildSessionLease`) | | 28 | MCP standalone-SSE listener goroutine (`handleSSE`) per connected server | `mcp.Server` | per connected server (rides the SDK session, opened after `initialize` when `DisableStandaloneSSE: false`) | `Server.Close()` → `session.Close()` → `conn.Close()` cancels `connCtx` → `handleSSE` returns (async; the `mcp` package's `goleak` gate has a targeted ignore list for the SDK + stdlib goroutines that unwind asynchronously after close) | none (the SDK reconnects the stream itself on a transient drop; #177/ADR 0056 reconnects the whole session when the SSE reconnect exhausts → `ErrSessionMissing`) | `internal/adapter/mcp/mcp.go` (`dial`); ADR 0057 | | 29 | guardrail session waiver (`WaiverHolder`, ADR 0062) | `app.Build` constructs; the engine arms it via the `modelhook.Runner`'s `port.HookApprovalLearner` on a human `VerdictAllowAlways`, `modelhook.Runner.check` consults it | process | self-clearing; dies with the process (no `Close` — a nil `*WaiverHolder` is the byte-identical OFF posture) | **lost** (in-memory; a waiver never silently survives restart — fail-safe: the call re-blocks/re-asks until a human re-approves it, ADR 0062) | `internal/adapter/modelhook/waiver.go` (`WaiverHolder`); armed via `internal/adapter/modelhook/modelhook.go` (`LearnHookApproval`); constructed in `internal/app/build.go` | | 30 | scheduler tick goroutine (Phase 5, ADR 0059) | `internal/adapter/scheduler` (`Scheduler`), held by `server.Service.scheduler` | process | `Scheduler.Stop` cancels the tick loop + joins in-flight fires (with a grace) + releases the leader lease; `Service.Close` stops it FIRST so fires drain while the service is alive | **reconstructible** (a restarted process re-acquires the leader lease or ticks standalone, and re-polls `ScheduleStore.Due` — the store is ground truth, the lookahead is derived); only constructed when `--scheduler` is set (byte-identical default when unwired) | `internal/adapter/scheduler/scheduler.go` (`tickLoop`, `Start`, `Stop`); wired at `internal/app/build.go` (`startScheduler`) | diff --git a/docs/design/IMPLEMENTATION-NOTES.md b/docs/design/IMPLEMENTATION-NOTES.md index 7a0a601a65..5f38cce5b8 100644 --- a/docs/design/IMPLEMENTATION-NOTES.md +++ b/docs/design/IMPLEMENTATION-NOTES.md @@ -881,8 +881,17 @@ signals the run. The Service retracts local ask delivery and prevents later rela persistence, but leaves the durable `PendingAsk` unresolved and byte-identical for TTL takeover. Settled stale run references remove heavyweight held-lease/capability tombstones; the lightweight -`lostOwnership` denial remains until explicit local session teardown so that stale -Service cannot reacquire. +`lostOwnership` denial otherwise fails every ordinary caller fast so that a stale +Service cannot reacquire. It is cleared by explicit local session teardown +(`CloseSession`), or automatically by the composition-level stale-session sweep's +`ReconcileLeaseLossTombstone` (issue #1334): a bounded trial Acquire+immediate-Release +against the real backend proves the lease is genuinely free before the tombstone is +dropped, letting the next real run-entry repair the session (Interrupt for cancelled, +the awaiting-resume machinery for awaiting) without waiting for teardown or a process +restart. That trial is serialized against `onLeaseLost`'s own Release for the same id +via a dedicated per-id lock (`leaseLossMu`), since `engine/port.SessionLease`'s +same-id calls are caller-serialized and a conforming backend need not make an +overlapping Acquire/Release safe on its own. The gRPC in-stream approval path also enters a Service-owned live-run gate: holding the Service mutex orders the verdict against lease invalidation before it reaches the parent diff --git a/internal/adapter/server/classification.go b/internal/adapter/server/classification.go index ceb7e4d6b9..4a00f79331 100644 --- a/internal/adapter/server/classification.go +++ b/internal/adapter/server/classification.go @@ -410,6 +410,8 @@ var serviceAccessTable = map[string]ClassificationEntry{ "LeaseSweepDisabled": {KindExempt, "reads the process-wide sticky sweep-disabled flag SessionStale sets, consumed only by the composition-owned sweep"}, "StaleRunningCandidates": {KindSharedInfrastructure, "root-authorized metadata-only enumeration of owned running, non-scheduled sessions; returns no transcript content"}, "SettleIfStale": {KindSharedInfrastructure, "root-authorized authoritative reload and settlement of a stale running candidate; ownerless records are rejected under ownership enforcement"}, + "LostOwnershipCandidates": {KindSharedInfrastructure, "root-authorized in-memory enumeration of this process's own lease-loss tombstone ids (issue #1334); no store scan, no transcript content"}, + "ReconcileLeaseLossTombstone": {KindSharedInfrastructure, "root-authorized trial-Acquire clearing this process's own lease-loss tombstone once the backend proves it free (issue #1334); no session-state mutation"}, "DeleteSessionForRetention": {KindExempt, "legacy composition retention callback; revalidates durable taxonomy/state and acquires the session mutation lease before deletion"}, "DeleteSessionForRetentionCandidate": {KindExempt, "composition-owned retention callback over planner metadata; holds run-entry, lease, and backend family exclusions through conditional deletion"}, } @@ -472,7 +474,7 @@ var systemAccessTable = map[syscaller.Root]ClassificationEntry{ }, syscaller.RootStaleSessionReconcile: { KindSharedInfrastructure, - "stale-session repair: may enumerate metadata and settle only owned, running, non-scheduled crash orphans through the root-authorized narrow server seam; cannot read transcripts or use caller memory", + "stale-session repair: may enumerate metadata and settle only owned, running, non-scheduled crash orphans through the root-authorized narrow server seam, AND (issue #1334) clear this process's own lease-loss tombstones for awaiting/cancelled sessions via a bounded trial-Acquire; cannot read transcripts or use caller memory", }, } diff --git a/internal/adapter/server/lease_test.go b/internal/adapter/server/lease_test.go index 6964344730..79ad87cca0 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -2,8 +2,10 @@ package server_test import ( "context" + "encoding/json" "errors" "iter" + "strings" "sync" "sync/atomic" "testing" @@ -30,6 +32,14 @@ type fakeLease struct { acquireErr error acquires int acquireExpiry time.Time // expiry the next Acquire grants (zero = time.Now()+1h) + // acquireHook, when set, runs synchronously inside Acquire (after the count + // bump, before the grant is returned), passed the requested owner string so + // a test can distinguish a trial Acquire (owner suffixed with + // staleTrialLeaseSuffix) from an ordinary run-entry Acquire — used by + // concurrency tests that need to observe or assert on exactly when an + // Acquire call happened relative to some other in-flight call (e.g. a + // same-id Release). + acquireHook func(id session.SessionID, owner string) renewHook func(port.Lease) (port.Lease, error) releaseHook func(port.Lease) error @@ -40,8 +50,14 @@ type fakeLease struct { func (f *fakeLease) Acquire(_ context.Context, id session.SessionID, owner string) (port.Lease, error) { f.mu.Lock() - defer f.mu.Unlock() f.acquires++ + hook := f.acquireHook + f.mu.Unlock() + if hook != nil { + hook(id, owner) + } + f.mu.Lock() + defer f.mu.Unlock() if f.acquireErr != nil { return port.Lease{}, f.acquireErr } @@ -770,3 +786,797 @@ func TestLeaseTransientRenewBlipKeepsRun(t *testing.T) { svc.CloseSession(sess.ID) <-cancelled // the explicit cancel now ends it cleanly. } + +// TestReconcileLeaseLossTombstoneRecoversCancelledSession is issue #1334's core +// repro: onLeaseLost drives a session with no parked ask OUT of StateRunning +// via run.Cancel() (to StateCancelled), so it can never again match the +// StateRunning-only stale-session sweep (SessionStale/SettleIfStale) and the +// lostOwnership tombstone would otherwise never clear short of CloseSession or +// a process restart. ReconcileLeaseLossTombstone must clear it once a genuine +// trial-Acquire proves the lease free, unblocking the next ordinary run-entry +// (which itself, unchanged, repairs the session state via loadAndReopen's +// existing Interrupt seam). +func TestReconcileLeaseLossTombstoneRecoversCancelledSession(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingProvider{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + // Fail the next renew: no parked ask, so onLeaseLost's preserveAwaiting + // branch does not apply — it cancels the live run, which blockingProvider + // ends with StopCancelled once ctx is done. + var lost atomic.Bool + lease.mu.Lock() + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + + var sawCancel bool + for ev := range run.Events() { + if ev.Type == session.EvResult && ev.Result != nil && ev.Result.Stop == session.StopCancelled { + sawCancel = true + } + } + svc.FinishRun(sess.ID, run) + if !lost.Load() || !sawCancel { + t.Fatal("precondition: the renewer never lost the lease and cancelled the run") + } + + // The engine here has no Store wired (mirrors newAskingService's + // engineSaves=false), so the durable snapshot is whatever this test writes + // directly — the crash-orphan-test idiom. Persist the StateCancelled shape + // onLeaseLost's real cancel path would leave behind. + cancelled := session.New(sess.ID, session.ModeDefault, sess.EnvironmentRef, session.Limits{}, time.Unix(0, 0)) + if err := cancelled.RecordUserPrompt("go", nil); err != nil { + t.Fatalf("RecordUserPrompt: %v", err) + } + if err := cancelled.BeginTurn(); err != nil { + t.Fatalf("BeginTurn: %v", err) + } + if err := cancelled.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if err := store.Save(context.Background(), cancelled); err != nil { + t.Fatalf("overwrite Save: %v", err) + } + + // Precondition: the tombstone still fails every ordinary caller fast, even + // though the session itself is an ordinary Interrupt-recoverable cancelled + // snapshot. + if _, err := svc.StartRun(context.Background(), sess.ID, "again"); !errors.Is(err, server.ErrSessionLeasedElsewhere) { + t.Fatalf("StartRun before reconcile = %v, want ErrSessionLeasedElsewhere", err) + } + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + ids, err := svc.LostOwnershipCandidates(staleCtx) + if err != nil { + t.Fatalf("LostOwnershipCandidates: %v", err) + } + if len(ids) != 1 || ids[0] != sess.ID { + t.Fatalf("LostOwnershipCandidates = %v, want [%q]", ids, sess.ID) + } + cleared, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID) + if err != nil { + t.Fatalf("ReconcileLeaseLossTombstone: %v", err) + } + if !cleared { + t.Fatal("ReconcileLeaseLossTombstone = false, want true (the lease is genuinely free)") + } + + // The tombstone is gone, not merely bypassed once: a normal re-entry now + // succeeds, and loadAndReopen's existing Interrupt seam repairs the state. + run2, err := svc.StartRun(context.Background(), sess.ID, "again") + if err != nil { + t.Fatalf("StartRun after ReconcileLeaseLossTombstone = %v, want success (tombstone should be cleared)", err) + } + run2.Cancel() + for range run2.Events() { + } + svc.FinishRun(sess.ID, run2) +} + +// TestOnLeaseLostSerializesAgainstReconcileTrial is the panel finding's +// regression test (issue #1334 follow-up): onLeaseLost's real Release for a +// lost session id must never overlap ReconcileLeaseLossTombstone's trial +// Acquire for the SAME id (engine/port/lease.go's "callers serialize same-id +// calls" contract). It blocks the Release call mid-flight, asserts a +// concurrently-invoked ReconcileLeaseLossTombstone has NOT yet started its +// trial Acquire, then unblocks Release and confirms the Acquire only happens +// once Release has returned — proving leaseLossMu, not luck, orders them. +func TestOnLeaseLostSerializesAgainstReconcileTrial(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingProvider{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + releaseStarted := make(chan struct{}) + releaseProceed := make(chan struct{}) + var unblockOnce sync.Once + unblockRelease := func() { unblockOnce.Do(func() { close(releaseProceed) }) } + // Register the unblock as cleanup FIRST, before anything can Fatal: an + // earlier failure (e.g. the very overlap this test guards against) must + // still release the blocked Release/reconcile goroutines rather than + // leaking them (JAORMX's non-blocking test-robustness follow-up). + t.Cleanup(unblockRelease) + var released atomic.Bool + var blockedOnce sync.Once + lease.mu.Lock() + lease.releaseHook = func(port.Lease) error { + // Only onLeaseLost's OWN Release (the first one) is under test; a + // later trial's self-Release (leaseTrial's immediate Acquire+Release) + // must run normally or it would deadlock on releaseProceed too. + blockedOnce.Do(func() { + close(releaseStarted) + <-releaseProceed + released.Store(true) + }) + return nil + } + var lost atomic.Bool + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + + var overlap atomic.Bool + var trialAcquires atomic.Int32 + lease.mu.Lock() + lease.acquireHook = func(_ session.SessionID, owner string) { + if !strings.HasSuffix(owner, "-stale-trial") { + return // the initial real run-entry Acquire, not the reconcile's trial. + } + trialAcquires.Add(1) + if !released.Load() { + overlap.Store(true) + } + } + lease.mu.Unlock() + + // Drain to StopCancelled: onLeaseLost's run.Cancel() runs BEFORE its Release + // call, so the run can finish while our Release hook is still blocked. + for ev := range run.Events() { + if ev.Type == session.EvResult && ev.Result != nil && ev.Result.Stop == session.StopCancelled { + break + } + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never lost the lease") + } + + select { + case <-releaseStarted: + case <-time.After(2 * time.Second): + t.Fatal("onLeaseLost never called Release") + } + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + reconcileDone := make(chan struct{}) + go func() { + defer close(reconcileDone) + if _, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID); err != nil { + t.Errorf("ReconcileLeaseLossTombstone: %v", err) + } + }() + + // Give the reconcile goroutine ample time to run ahead if leaseLossMu did + // NOT serialize it: it must still be blocked on the per-id lock, so no + // trial Acquire can have happened yet. + time.Sleep(100 * time.Millisecond) + if n := trialAcquires.Load(); n != 0 { + t.Fatalf("trial Acquire count = %d before Release returned, want 0 (Acquire started while Release was still in flight)", n) + } + select { + case <-reconcileDone: + t.Fatal("ReconcileLeaseLossTombstone returned before Release completed; leaseLossMu did not serialize it") + default: + } + + unblockRelease() + + select { + case <-reconcileDone: + case <-time.After(2 * time.Second): + t.Fatal("ReconcileLeaseLossTombstone never completed after Release was unblocked") + } + if overlap.Load() { + t.Fatal("trial Acquire ran while Release was still in flight — same-id overlap") + } + if trialAcquires.Load() != 1 { + t.Fatalf("trial Acquire count = %d, want 1", trialAcquires.Load()) + } +} + +// TestCloseSessionSerializesAgainstReconcileTrial is JAORMX's second-round +// panel follow-up: closeSessionLocal's unconditional tombstone clear is a +// SECOND writer that could race ReconcileLeaseLossTombstone's trial the same +// way onLeaseLost's Release could — CloseSession could observe the tombstone, +// clear it (without ever calling the real backend itself), and let a brand +// new StartRun perform a REAL Acquire while the trial's own self-Release +// (leaseTrial's immediate Acquire-then-Release) was still in flight. +// +// A third-round follow-up review found this version's assertion vacuous: it +// only called the reopening StartRun AFTER both CloseSession and Reconcile +// had already been confirmed done, by which point the trial Release had +// necessarily already completed — so overlap could never be observed either +// way. Fixed by starting a POLLING reopen goroutine concurrently with +// CloseSession, immediately once the trial Release is confirmed blocked +// (before it is ever unblocked): while the tombstone is genuinely still set +// (the fixed behaviour), each poll is refused locally with +// ErrSessionLeasedElsewhere and never reaches the backend at all, so the +// reopening real Acquire can only happen once the tombstone is actually +// cleared — exactly the moment this test needs to observe. The same review +// also flagged that draining to StopCancelled only proves onLeaseLost called +// run.Cancel(), which happens BEFORE its own Release in the function body — +// not that the Release itself had returned — so swapping in the +// trial-blocking releaseHook right after was not provably safe from +// intercepting that first Release instead of the trial's. Fixed by an +// explicit "first Release completed" signal, installed before triggering the +// loss and waited on before installing the trial-blocking hook. +func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingProvider{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + // Installed BEFORE the loss so onLeaseLost's own Release (the FIRST + // Release call, made from inside its function body AFTER run.Cancel()) + // is provably observed complete before this test ever installs the + // trial-blocking hook below. + firstReleaseDone := make(chan struct{}) + var firstReleaseOnce sync.Once + lease.mu.Lock() + lease.releaseHook = func(port.Lease) error { + firstReleaseOnce.Do(func() { close(firstReleaseDone) }) + return nil + } + var lost atomic.Bool + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + + // Drive the loss: draining to StopCancelled only proves onLeaseLost + // called run.Cancel(), which precedes its own Release call in the + // function body — the explicit wait below is what actually proves that + // Release returned. + for ev := range run.Events() { + if ev.Type == session.EvResult && ev.Result != nil && ev.Result.Stop == session.StopCancelled { + break + } + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never lost the lease") + } + select { + case <-firstReleaseDone: + case <-time.After(2 * time.Second): + t.Fatal("onLeaseLost never completed its own Release") + } + + // Overwrite with the StateCancelled shape onLeaseLost's real cancel path + // leaves behind (this Config has no Store wired into the engine, so the + // durable snapshot is whatever this test writes directly). + cancelled := session.New(sess.ID, session.ModeDefault, sess.EnvironmentRef, session.Limits{}, time.Unix(0, 0)) + if err := cancelled.RecordUserPrompt("go", nil); err != nil { + t.Fatalf("RecordUserPrompt: %v", err) + } + if err := cancelled.BeginTurn(); err != nil { + t.Fatalf("BeginTurn: %v", err) + } + if err := cancelled.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if err := store.Save(context.Background(), cancelled); err != nil { + t.Fatalf("overwrite Save: %v", err) + } + + // Now block the TRIAL's own self-Release (leaseTrial's immediate + // Acquire-then-Release). onLeaseLost's own Release is already provably + // complete (firstReleaseDone above), so this hook's first invocation IS + // the trial's self-Release. + trialReleaseStarted := make(chan struct{}) + trialReleaseProceed := make(chan struct{}) + var unblockOnce sync.Once + unblockTrialRelease := func() { unblockOnce.Do(func() { close(trialReleaseProceed) }) } + t.Cleanup(unblockTrialRelease) + var trialReleaseDone atomic.Bool + var blockedOnce sync.Once + lease.mu.Lock() + lease.releaseHook = func(port.Lease) error { + blockedOnce.Do(func() { + close(trialReleaseStarted) + <-trialReleaseProceed + trialReleaseDone.Store(true) + }) + return nil + } + lease.mu.Unlock() + + var overlap atomic.Bool + lease.mu.Lock() + lease.acquireHook = func(_ session.SessionID, owner string) { + if strings.HasSuffix(owner, "-stale-trial") { + return // the reconcile's own trial Acquire, not the reopening real one. + } + if !trialReleaseDone.Load() { + overlap.Store(true) + } + } + lease.mu.Unlock() + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + reconcileDone := make(chan struct{}) + go func() { + defer close(reconcileDone) + if _, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID); err != nil { + t.Errorf("ReconcileLeaseLossTombstone: %v", err) + } + }() + + select { + case <-trialReleaseStarted: + case <-time.After(2 * time.Second): + t.Fatal("ReconcileLeaseLossTombstone never reached its trial Release") + } + + closeDone := make(chan struct{}) + go func() { + defer close(closeDone) + svc.CloseSession(sess.ID) + }() + + // CloseSession must still be blocked on leaseLossMu: it must not have + // cleared the tombstone (or returned at all) while the trial Release is + // still in flight. + time.Sleep(100 * time.Millisecond) + select { + case <-closeDone: + t.Fatal("CloseSession returned before the trial Release completed; leaseLossMu did not serialize closeSessionLocal") + default: + } + + // Start the REOPENING StartRun NOW, concurrently with the still-blocked + // trial Release — not after everything settles. While the tombstone is + // genuinely still set, each attempt is refused LOCALLY with + // ErrSessionLeasedElsewhere and never reaches the backend, so this loop + // only performs its real Acquire once the tombstone is actually cleared: + // the exact moment that must not precede the trial Release completing. + var run2 *agent.Run + reopenDone := make(chan struct{}) + go func() { + defer close(reopenDone) + for { + r, err := svc.StartRun(context.Background(), sess.ID, "again") + if err == nil { + run2 = r + return + } + if errors.Is(err, server.ErrSessionLeasedElsewhere) { + time.Sleep(2 * time.Millisecond) + continue + } + t.Errorf("StartRun reopen: %v", err) + return + } + }() + + // The reopen must still be spinning on the tombstone: it must not have + // succeeded (a real Acquire) while the trial Release is still blocked. + time.Sleep(100 * time.Millisecond) + select { + case <-reopenDone: + t.Fatal("reopening StartRun succeeded before the trial Release completed; the tombstone was cleared too early") + default: + } + + unblockTrialRelease() + + select { + case <-reconcileDone: + case <-time.After(2 * time.Second): + t.Fatal("ReconcileLeaseLossTombstone never completed after its trial Release was unblocked") + } + select { + case <-closeDone: + case <-time.After(2 * time.Second): + t.Fatal("CloseSession never completed after the trial Release was unblocked") + } + select { + case <-reopenDone: + case <-time.After(2 * time.Second): + t.Fatal("reopening StartRun never completed after the trial Release was unblocked") + } + if run2 == nil { + t.Fatal("reopening StartRun failed for a reason other than ErrSessionLeasedElsewhere") + } + run2.Cancel() + for range run2.Events() { + } + svc.FinishRun(sess.ID, run2) + + if overlap.Load() { + t.Fatal("a real Acquire ran while the trial Release was still in flight — same-id overlap") + } +} + +// TestReconcileLeaseLossTombstoneRecoversAwaitingSession is the awaiting +// counterpart: onLeaseLost's preserveAwaiting branch drives the session to +// StateAwaiting (parked on a permission ask) rather than cancelling it, and +// deliberately leaves heldLeases[id] in place (marked invalid) instead of +// deleting it. ReconcileLeaseLossTombstone must clear BOTH the tombstone and +// that stale invalid heldLeases entry, or the very next real Acquire attempt +// would still hard-refuse via acquireLeaseCore's separate held-but-invalid +// check even with the tombstone gone. +func TestReconcileLeaseLossTombstoneRecoversAwaitingSession(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + var ran atomic.Int64 + cat := tool.NewCatalog() + cat.MustRegister(&writeAskTool{ran: &ran}) + // Engine has no Store wired (mirrors newAskingService's engineSaves=false): + // the run's internal cancel-on-loss must not overwrite the durable awaiting + // snapshot svc.Persist writes below. Two scripted turns: the initial ask, + // then the post-resume completion. + engine := agent.NewEngine(agent.Deps{ + LLM: mockllm.New( + mockllm.ToolCallTurn(session.NewToolCall("w1", "Write", json.RawMessage(`{"path":"a.go"}`))), + mockllm.TextTurn("done after approval"), + ), + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + var askID string + for ev := range run.Events() { + if ev.Type == session.EvPermissionAsk && ev.Ask != nil { + askID = ev.Ask.AskID + svc.Persist(context.Background(), sess.ID) // durable awaiting snapshot; marks runState.awaiting. + break + } + } + if askID == "" { + t.Fatal("the run never raised a permission ask") + } + + // The run stays genuinely parked (nothing resolves the ask); fail the next + // renew so onLeaseLost's preserveAwaiting branch fires. + var lost atomic.Bool + lease.mu.Lock() + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + for range run.Events() { // drain the retract + cancel-driven terminal. + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never attempted a Renew") + } + if ran.Load() != 0 { + t.Fatalf("Write executed %d time(s) pre-approval, want 0", ran.Load()) + } + + // Precondition: the durable snapshot is still the awaiting one svc.Persist + // wrote (the parked run's cancel-driven terminal must not have overwritten + // it — the engine has no Store), yet every ordinary caller fails fast on + // the tombstone. + reloaded, err := store.Load(context.Background(), sess.ID) + if err != nil { + t.Fatalf("reload: %v", err) + } + if reloaded.State != session.StateAwaiting { + t.Fatalf("precondition: durable state = %q, want awaiting", reloaded.State) + } + if err := svc.Approve(context.Background(), sess.ID, askID, session.VerdictAllowOnce); !errors.Is(err, server.ErrSessionLeasedElsewhere) { + t.Fatalf("Approve before reconcile = %v, want ErrSessionLeasedElsewhere", err) + } + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + cleared, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID) + if err != nil { + t.Fatalf("ReconcileLeaseLossTombstone: %v", err) + } + if !cleared { + t.Fatal("ReconcileLeaseLossTombstone = false, want true (the lease is genuinely free)") + } + + // The tombstone (and the stale invalid heldLeases bookkeeping) is gone: the + // pending ask resumes normally through the existing awaiting-resume seam. + resumed, err := svc.ApproveRun(context.Background(), sess.ID, askID, session.VerdictAllowOnce, "") + if err != nil { + t.Fatalf("ApproveRun after ReconcileLeaseLossTombstone = %v, want success", err) + } + var stop session.StopReason + for ev := range resumed.Events() { + if ev.Type == session.EvResult && ev.Result != nil { + stop = ev.Result.Stop + } + } + svc.FinishRun(sess.ID, resumed) + if ran.Load() != 1 { + t.Fatalf("pending Write executed %d time(s) on resume, want exactly 1", ran.Load()) + } + if stop != session.StopEndTurn { + t.Fatalf("resumed run stop = %q, want %q", stop, session.StopEndTurn) + } +} + +// TestReconcileLeaseLossTombstoneRefusesWhenGenuinelyHeldElsewhere is the +// safety-critical negative counterpart: a genuine trial-Acquire failure (a +// real competitor, or a peer's not-yet-expired hold) must leave the tombstone +// in place — ReconcileLeaseLossTombstone must never clear it unconditionally. +func TestReconcileLeaseLossTombstoneRefusesWhenGenuinelyHeldElsewhere(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingProvider{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + var lost atomic.Bool + lease.mu.Lock() + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + for range run.Events() { + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never lost the lease") + } + + cancelled := session.New(sess.ID, session.ModeDefault, sess.EnvironmentRef, session.Limits{}, time.Unix(0, 0)) + if err := cancelled.RecordUserPrompt("go", nil); err != nil { + t.Fatalf("RecordUserPrompt: %v", err) + } + if err := cancelled.BeginTurn(); err != nil { + t.Fatalf("BeginTurn: %v", err) + } + if err := cancelled.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if err := store.Save(context.Background(), cancelled); err != nil { + t.Fatalf("overwrite Save: %v", err) + } + + // Unlike the recovery test, a REAL competitor now genuinely holds the + // lease — the trial-Acquire must see that, not merely "expired". + lease.mu.Lock() + lease.acquireErr = port.ErrLeaseHeld + lease.mu.Unlock() + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + cleared, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID) + if err != nil { + t.Fatalf("ReconcileLeaseLossTombstone while genuinely held elsewhere: %v", err) + } + if cleared { + t.Fatal("ReconcileLeaseLossTombstone while genuinely held elsewhere = true, want false") + } + + if _, err := svc.StartRun(context.Background(), sess.ID, "again"); !errors.Is(err, server.ErrSessionLeasedElsewhere) { + t.Fatalf("StartRun after a refused reconcile = %v, want still ErrSessionLeasedElsewhere", err) + } +} + +// TestReconcileLeaseLossTombstoneFailSafeOnGenericError is the fail-safe +// negative case for a bare, non-sentinel trial-Acquire error — anything other +// than port.ErrLeaseHeld/port.ErrLeaseUnsupported. leaseTrial must treat +// ambiguity as "not free," never as license to clear the tombstone. +func TestReconcileLeaseLossTombstoneFailSafeOnGenericError(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + ps := permstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingProvider{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, ps), + Model: "test-model", + }) + svc, err := newPlacementTestService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + var lost atomic.Bool + lease.mu.Lock() + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + for range run.Events() { + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never lost the lease") + } + + cancelled := session.New(sess.ID, session.ModeDefault, sess.EnvironmentRef, session.Limits{}, time.Unix(0, 0)) + if err := cancelled.RecordUserPrompt("go", nil); err != nil { + t.Fatalf("RecordUserPrompt: %v", err) + } + if err := cancelled.BeginTurn(); err != nil { + t.Fatalf("BeginTurn: %v", err) + } + if err := cancelled.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if err := store.Save(context.Background(), cancelled); err != nil { + t.Fatalf("overwrite Save: %v", err) + } + + // A bare backend error — not one of the two lease sentinels — must be + // treated as ambiguous, never as proof the lease is free. + lease.mu.Lock() + lease.acquireErr = errors.New("boom") + lease.mu.Unlock() + + staleCtx := syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile) + cleared, err := svc.ReconcileLeaseLossTombstone(staleCtx, sess.ID) + if err != nil { + t.Fatalf("ReconcileLeaseLossTombstone on a generic trial error: %v", err) + } + if cleared { + t.Fatal("ReconcileLeaseLossTombstone on a generic trial error = true, want false (fail-safe)") + } + + if _, err := svc.StartRun(context.Background(), sess.ID, "again"); !errors.Is(err, server.ErrSessionLeasedElsewhere) { + t.Fatalf("StartRun after a fail-safe-refused reconcile = %v, want still ErrSessionLeasedElsewhere", err) + } +} diff --git a/internal/adapter/server/sdk_typescript_release_test.go b/internal/adapter/server/sdk_typescript_release_test.go index c8b4ab156b..105712c965 100644 --- a/internal/adapter/server/sdk_typescript_release_test.go +++ b/internal/adapter/server/sdk_typescript_release_test.go @@ -226,6 +226,7 @@ func TestSDKTypescriptRelease_Scenario1_PublicServiceProjectionParity(t *testing "LoadSession", "LoadSessionWithMCP", "LookupRun", + "LostOwnershipCandidates", "MaintenanceMutationAvailable", "ManualDreamCapabilities", "MaybeAutoApprovePlan", @@ -236,6 +237,7 @@ func TestSDKTypescriptRelease_Scenario1_PublicServiceProjectionParity(t *testing "PublishSessionEvent", "ReattachPlacement", "ReattachPlacementInScope", + "ReconcileLeaseLossTombstone", "RecoverNotice", "ResolvedModel", "RetryFailedRun", diff --git a/internal/adapter/server/service.go b/internal/adapter/server/service.go index 4bb847e30b..2711d94eb0 100644 --- a/internal/adapter/server/service.go +++ b/internal/adapter/server/service.go @@ -1030,14 +1030,46 @@ type Service struct { heldLeases map[session.SessionID]*heldLease // lostOwnership remembers that this Service definitively lost an id even after // the heavyweight heldLease/capability tombstones are removed at lifecycle - // settlement. Cleared by explicit CloseSession teardown, and — the ONE other - // caller, narrowly scoped — by acquireLeaseCore's bypassTombstone=true path - // (used only via acquireMutationLeaseForStaleSettle, i.e. SettleIfStale/the - // stale-session reconcile sweep) on a genuine re-Acquire success. Every other + // settlement. Cleared by explicit CloseSession teardown, by acquireLeaseCore's + // bypassTombstone=true path (used only via acquireMutationLeaseForStaleSettle, + // i.e. SettleIfStale/the StateRunning stale-session reconcile sweep) on a + // genuine re-Acquire success, and by ReconcileLeaseLossTombstone (issue #1334) + // on a genuine trial-Acquire success — the awaiting/cancelled counterpart: + // onLeaseLost drives the session OUT of StateRunning as part of handling the + // loss, so it can never become a StateRunning candidate again for the sweep + // above to rediscover, and without ReconcileLeaseLossTombstone the tombstone + // would stay permanent short of CloseSession or a process restart. Every other // caller (acquireLease/reaffirmLease with bypassTombstone=false) still fails // fast on it forever. lostOwnership map[session.SessionID]struct{} + // leaseLossMu serializes the THREE writers that touch a session id's + // lostOwnership tombstone and the real backend calls around it (issue + // #1334 panel review, two rounds): onLeaseLost's real Release, Reconcile + // LeaseLossTombstone's trial Acquire+Release, and closeSessionLocal's + // unconditional tombstone clear. onLeaseLost sets lostOwnership[id] and + // unlocks s.mu well before it calls SessionLease.Release, and either the + // composition-level sweep (a trial Acquire) or a concurrent CloseSession/ + // DeleteSession (an unconditional tombstone clear that unblocks the NEXT + // real Acquire) can observe the tombstone and act while that Release is + // still in flight — a same-id Acquire/Release overlap engine/port/lease.go's + // CONCURRENCY contract explicitly leaves to the CALLER to prevent ("Calls + // for the SAME id from one process are serialised by the caller"). A + // conforming backend is not required to make that overlap safe. This is a + // DEDICATED lock, never runEntryMu: onLeaseLost's own doc forbids taking + // runEntryMu (lease loss cancels operations that may be holding it, so + // waiting on it here could deadlock their cancellation), and this lock is + // never held by any cancellation path, so acquiring it here carries no + // such risk. closeSessionLocal is the one exception that already holds + // runEntryMu (every caller does) before also taking leaseLossMu — a FIXED + // order (runEntryMu → leaseLossMu) that introduces no cycle, since neither + // onLeaseLost nor ReconcileLeaseLossTombstone ever takes runEntryMu. Held + // for each function's ENTIRE body (a keyedMutex, freed once no caller + // holds the key) so all three are strictly ordered relative to one + // another for the same id: exactly one of loss-handling, trial-reconcile, + // or close-teardown runs at a time, never interleaved mid-flight. + leaseLossMu keyedMutex + // leaseDisabled is set (once) when Config.SessionLease reports // ErrLeaseUnsupported: the seam never works on this backend, so the run-entry // gate stickily stops consulting it and degrades to the no-lease path (the @@ -1052,7 +1084,9 @@ type Service struct { // cross-replica unsoundness the lease check exists to prevent. Kept // separate from leaseDisabled because it gates a DIFFERENT seam (the // staleness sweep, not run-entry acquisition) with its own diagnostic. - // Guarded by s.mu. + // Shared with ReconcileLeaseLossTombstone (issue #1334), which runs the + // SAME kind of trial-Acquire probe against the SAME backend. Guarded by + // s.mu. leaseSweepDisabled bool // draining is the cloud-native drain gate (ADR 0048, mecak8s): once armed by @@ -2771,8 +2805,22 @@ func (s *Service) closeSessionAuthorized(id session.SessionID) { // closeSessionLocal releases only process-local ownership. The caller must hold // brokerMu for id so no engine can borrow and install the attachment while it is -// being closed. +// being closed. It also takes leaseLossMu for id (JAORMX's follow-up on the +// #1334 panel fix): this is the ONE place that unconditionally clears +// lostOwnership[id] outside onLeaseLost/ReconcileLeaseLossTombstone, and every +// caller (CloseSession/EndSession via closeSessionAuthorized, DeleteSession, +// DeleteSessionForRetentionCandidate, DeleteSessionForRetention) already holds +// runEntryMu for id — never leaseLossMu — so taking it here in the FIXED order +// runEntryMu → leaseLossMu introduces no new cycle (onLeaseLost/ +// ReconcileLeaseLossTombstone never take runEntryMu, per onLeaseLost's own doc +// comment). Without this, a close racing a trial reconcile could clear the +// tombstone and let a new real Acquire begin while the trial's own Release was +// still in flight — the same same-id Acquire/Release overlap leaseLossMu +// exists to prevent, just reached via a second writer of the tombstone. func (s *Service) closeSessionLocal(id session.SessionID) { + unlockLeaseLoss := s.leaseLossMu.lock(id) + defer unlockLeaseLoss() + // Release composition-owned session-scoped state first (e.g. the per-session // learned permission rules) so it never outlives the session, even if the // per-session engine teardown below is a no-op for this id. @@ -6727,8 +6775,13 @@ func (s *Service) renewLoop(renewCtx context.Context, id session.SessionID, expe // timer and local broker transaction are also stopped/invalidated here; the // durable snapshot itself is never mutated. Do not acquire runEntryMu here: lease // loss cancels operations that may be holding it, so waiting for that lock would -// deadlock their cancellation. +// deadlock their cancellation. leaseLossMu IS taken, for the whole body, to keep +// the tombstone-set + Release sequence atomic against ReconcileLeaseLossTombstone's +// trial Acquire for the same id (see leaseLossMu's own doc comment). func (s *Service) onLeaseLost(ctx context.Context, id session.SessionID, expected *heldLease, cause error) { + unlock := s.leaseLossMu.lock(id) + defer unlock() + s.stopAuthorizationExpiry(id) s.invalidateLocalAuthorization(context.WithoutCancel(ctx), id) var run *agent.Run @@ -6912,44 +6965,79 @@ func (s *Service) SessionStale(ctx context.Context, meta port.SessionMeta) bool if disabled { return false } - trialCtx, cancel := context.WithTimeout(ctx, leaseAcquireTimeout) - lease, err := s.cfg.SessionLease.Acquire(trialCtx, meta.ID, s.cfg.LeaseOwner+staleTrialLeaseSuffix) - cancel() - switch { - case errors.Is(err, port.ErrLeaseHeld): - s.mu.Lock() - _, selfHeld := s.heldLeases[meta.ID] - s.mu.Unlock() + free, held := s.leaseTrial(ctx, meta.ID, "session staleness sweep") + if held != nil { // The self-held-lease correction: ErrLeaseHeld against our OWN trial // call (a different owner string than the real hold, so the backend // sees a genuine conflict) is NOT evidence of a live peer when this // process itself is the one holding the real lease — it is evidence // this process's own prior run died without releasing it. + s.mu.Lock() + _, selfHeld := s.heldLeases[meta.ID] + s.mu.Unlock() return selfHeld + } + return free +} + +// leaseTrial performs a bounded TRIAL Acquire+immediate-Release against the +// real lease backend, proving whether id's lease is genuinely free right now +// — the refinement SHARED by SessionStale (StateRunning crash-orphan +// detection) and ReconcileLeaseLossTombstone (issue #1334's awaiting/ +// cancelled tombstone clearing). Both trial owners use the SAME suffixed +// owner string (staleTrialLeaseSuffix, never a new unrelated string) so a +// trial is self-attributable in lease-backend diagnostics, and neither ever +// holds the trial lease across the caller's later decision — a successful +// trial releases immediately, so there is nothing to hold across a write. +// +// held is non-nil ONLY for a genuine port.ErrLeaseHeld: the caller decides +// what that means for ITS OWN bookkeeping — SessionStale's self-held-lease +// correction consults s.heldLeases to distinguish "this process's own prior +// run died without releasing it" (stale) from "a genuinely different live +// owner holds it" (not stale); ReconcileLeaseLossTombstone simply leaves its +// tombstone in place either way, since by definition it already knows this +// process lost the lease. leaseTrial itself makes no ownership judgement on +// ErrLeaseHeld. Every OTHER outcome is folded into (false, nil) so both +// callers stay one switch shorter: ErrLeaseUnsupported stickily disables the +// sweep for the process lifetime (logged once via logCtx), and any other +// error/timeout is a fail-safe WARN (never treat ambiguity as free). Only +// free==true (nobody held it) is safe to act on. +func (s *Service) leaseTrial(ctx context.Context, id session.SessionID, logCtx string) (free bool, held error) { + trialCtx, cancel := context.WithTimeout(ctx, leaseAcquireTimeout) + lease, err := s.cfg.SessionLease.Acquire(trialCtx, id, s.cfg.LeaseOwner+staleTrialLeaseSuffix) + cancel() + switch { + case errors.Is(err, port.ErrLeaseHeld): + return false, err case errors.Is(err, port.ErrLeaseUnsupported): s.mu.Lock() firstTime := !s.leaseSweepDisabled s.leaseSweepDisabled = true s.mu.Unlock() if firstTime { - s.cfg.Diagnostics.Log(ctx, port.LevelInfo, "session staleness sweep: lease backend does not support leasing; disabling the sweep", + s.cfg.Diagnostics.Log(ctx, port.LevelInfo, logCtx+": lease backend does not support leasing; disabling the sweep", "owner", s.cfg.LeaseOwner) } - return false + return false, nil case err != nil: - // Infra error or timeout — fail-safe: never mass-abandon on a flaky - // lease backend. - s.cfg.Diagnostics.Log(ctx, port.LevelWarn, "session staleness sweep: trial lease acquire failed; treating as not stale (fail-safe)", - "session", string(meta.ID), "err", err.Error()) - return false + // Infra error or timeout — fail-safe: never treat ambiguity as free. + s.cfg.Diagnostics.Log(ctx, port.LevelWarn, logCtx+": trial lease acquire failed; treating as not free (fail-safe)", + "session", string(id), "err", err.Error()) + return false, nil } // Success: nobody held it. Release the trial immediately — this function - // only decides staleness, it performs no write, so there is nothing to - // hold the lease across. + // only decides, it performs no write, so there is nothing to hold across + // the caller's later action. relCtx, relCancel := context.WithTimeout(context.WithoutCancel(ctx), leaseAcquireTimeout) - _ = s.cfg.SessionLease.Release(relCtx, lease) + if relErr := s.cfg.SessionLease.Release(relCtx, lease); relErr != nil { + // A leaked trial silently pins the lease until its own TTL expiry with + // no diagnostic explaining the delay — worth one WARN even though the + // caller's decision (free==true) already went through. + s.cfg.Diagnostics.Log(ctx, port.LevelWarn, logCtx+": trial lease release failed; it will pin until TTL expiry", + "session", string(id), "err", relErr.Error()) + } relCancel() - return true + return true, nil } // LeaseSweepDisabled reports whether SessionStale has stickily disabled the @@ -7027,6 +7115,143 @@ func (s *Service) SettleIfStale(ctx context.Context, id session.SessionID) (bool return true, nil } +// LostOwnershipCandidates returns the ids this process currently holds a +// lease-loss tombstone for (lostOwnership, set by onLeaseLost). It is an +// in-memory snapshot of Service's OWN bookkeeping, NOT a store-wide scan: +// lostOwnership is already scoped to exactly the ids this process itself +// definitively lost, so there is no risk of the unbounded Acquire fan-out a +// store-wide scan of every awaiting/cancelled session would cause — those are +// the STEADY STATE for huge numbers of ordinary finished sessions (issue +// #1334). Exported for internal/app's composition-level sweep, mirroring +// StaleRunningCandidates. +func (s *Service) LostOwnershipCandidates(ctx context.Context) ([]session.SessionID, error) { + if !staleReconcileAuthorized(ctx) { + return nil, ErrManagementUnauthorized + } + s.mu.Lock() + defer s.mu.Unlock() + if len(s.lostOwnership) == 0 { + return nil, nil + } + ids := make([]session.SessionID, 0, len(s.lostOwnership)) + for id := range s.lostOwnership { + ids = append(ids, id) + } + return ids, nil +} + +// ReconcileLeaseLossTombstone is StaleRunningCandidates/SettleIfStale's +// counterpart for the OTHER shape issue #1334 fixes: onLeaseLost drives the +// session OUT of StateRunning as part of handling the loss — to awaiting via +// the preserveAwaiting branch, or eventually to cancelled via run.Cancel() — +// so the StateRunning-only staleness sweep above can never rediscover it, and +// the lostOwnership tombstone (by design a PERMANENT fail-fast for every +// ordinary caller, see lostOwnership's own doc comment) would otherwise clear +// only via CloseSession or a process restart, regardless of whether the +// original loss was a genuine takeover or a false positive. +// +// Unlike SettleIfStale/acquireMutationLeaseForStaleSettle this performs NO +// session-state repair and installs NO renewer: an awaiting/cancelled session +// has no run currently driving it (HOLD-FOR-SESSION-LIFE is tied to an active +// run), so re-acquiring and holding the lease here would leak it. Instead this +// is a bounded TRIAL Acquire immediately released — proof the lease is +// genuinely free, nothing ever held across the call — mirroring SessionStale's +// own trial-lease refinement (same owner suffix, same fail-safe-on-any- +// ambiguity posture: ErrLeaseHeld, a real Acquire error, or an unsupported +// backend all leave the tombstone untouched; only a successful trial proves +// the backend record is free, whether because a peer genuinely released it or +// because it simply expired once this process's renewer stopped ticking after +// the loss — Renew's ErrLeaseHeld cannot distinguish the two, so real time +// passing is what makes a re-Acquire safe, exactly as +// acquireMutationLeaseForStaleSettle's doc explains for the StateRunning +// case). Clearing the tombstone only unblocks the NEXT genuine run-entry +// (StartRunContent / ApproveRun's resumeFromAwaiting); that entry's own +// loadAndReopen/resumeFromAwaiting still performs the actual session-state +// repair (Interrupt for cancelled; the awaiting resume machinery for +// awaiting) exactly as it always has — this function never touches session +// state, only the local lease bookkeeping that was blocking it. +// +// Reports whether it actually cleared the tombstone (false, nil is the honest +// no-op for "still held" / "already cleared by a concurrent caller"/"no +// tombstone for this id"). Exported for internal/app's composition-level +// sweep, mirroring SessionStale/SettleIfStale/StaleRunningCandidates. +// +// CloseSession can no longer interleave WITHIN this call: closeSessionLocal +// (its one tombstone-clearing chokepoint) now also takes leaseLossMu for id, +// so a concurrent close either completes entirely before this call starts or +// blocks until this call's whole trial (Acquire + Release + tombstone-clear) +// has finished — never mid-trial (issue #1334 panel follow-up; closeSessionLocal's +// own doc comment has the lock-order rationale). The pre-trial and post-trial +// re-checks of s.lostOwnership below are kept anyway as defense in depth +// (e.g. a concurrent caller that legitimately re-acquired for real between the +// checks), each still re-reading under s.mu, the same "last write wins, +// re-verified under the lock" posture SettleIfStale documents for its own +// TOCTOU window. +// +// The trial Acquire below is additionally serialized against onLeaseLost's +// real Release for the same id via leaseLossMu (its own doc comment has the +// full rationale): held for this function's ENTIRE body, so the trial can +// never overlap a same-id Release still in flight. +func (s *Service) ReconcileLeaseLossTombstone(ctx context.Context, id session.SessionID) (bool, error) { + if !staleReconcileAuthorized(ctx) { + return false, ErrManagementUnauthorized + } + if s.cfg.SessionLease == nil { + return false, nil + } + + unlock := s.leaseLossMu.lock(id) + defer unlock() + + s.mu.Lock() + if s.leaseDisabled { + s.mu.Unlock() + return false, nil + } + if _, lost := s.lostOwnership[id]; !lost { + s.mu.Unlock() + return false, nil + } + if h, held := s.heldLeases[id]; held && h.valid { + // A concurrent caller already re-acquired for real; nothing to do. + s.mu.Unlock() + return false, nil + } + disabled := s.leaseSweepDisabled + s.mu.Unlock() + if disabled { + return false, nil + } + + free, held := s.leaseTrial(ctx, id, "lease-loss tombstone reconcile") + if held != nil || !free { + // Still genuinely held (a live peer, or this process's own + // not-yet-expired record from before the loss was declared), or the + // trial declined ambiguously (leaseTrial already logged the + // unsupported/error case) — leave the tombstone; the next sweep pass + // re-checks. + return false, nil + } + + s.mu.Lock() + defer s.mu.Unlock() + if _, lost := s.lostOwnership[id]; !lost { + return false, nil // a concurrent caller (CloseSession, another pass) already cleared it. + } + if h, held := s.heldLeases[id]; held && h.valid { + return false, nil // a concurrent real acquire won the race while our trial ran. + } + delete(s.lostOwnership, id) + if h, held := s.heldLeases[id]; held && !h.valid { + // Stale local bookkeeping left behind by onLeaseLost's preserveAwaiting + // branch — clear it too, or acquireLeaseCore's separate + // heldLeases-held-but-invalid check would still hard-refuse the very + // next real Acquire attempt even with the tombstone gone. + delete(s.heldLeases, id) + } + return true, nil +} + func (s *Service) cleanupRunAdmission(id session.SessionID, st *runState, promoted *bool) { if *promoted { return diff --git a/internal/app/session_reconcile.go b/internal/app/session_reconcile.go index 0ecdedf81d..44d0e9a17f 100644 --- a/internal/app/session_reconcile.go +++ b/internal/app/session_reconcile.go @@ -101,6 +101,7 @@ func sweepStaleSessions(ctx context.Context, svc *server.Service, diag port.Diag // silent here rather than double-logging every tick. return } + reconcileLeaseLossTombstones(ctx, svc, diag) rows, err := svc.StaleRunningCandidates(ctx) if err != nil { diag.Log(ctx, port.LevelWarn, "stale-session sweep: list failed; skipping sweep", "err", err.Error()) @@ -136,3 +137,46 @@ func sweepStaleSessions(ctx context.Context, svc *server.Service, diag port.Diag "settled", settled) } } + +// reconcileLeaseLossTombstones is issue #1334's fix for the shape the sweep +// above cannot reach: onLeaseLost drives a session OUT of StateRunning as part +// of handling a declared lease loss (to awaiting via the preserveAwaiting +// branch, or eventually to cancelled), so it can never become a StateRunning +// candidate again for sweepStaleSessions to rediscover — and the +// lostOwnership tombstone (a permanent fail-fast by design for every ordinary +// caller) would otherwise never clear short of CloseSession or a process +// restart. Scoped to exactly the ids THIS process tombstoned +// (svc.LostOwnershipCandidates is an in-memory read of Service's own +// bookkeeping, not a store-wide scan), so this never fans out trial-Acquire +// calls against the — potentially huge — steady-state population of ordinary +// finished awaiting/cancelled sessions in the store. +func reconcileLeaseLossTombstones(ctx context.Context, svc *server.Service, diag port.Diagnostics) { + ids, err := svc.LostOwnershipCandidates(ctx) + if err != nil { + diag.Log(ctx, port.LevelWarn, "stale-session sweep: lease-loss tombstone candidate list failed", "err", err.Error()) + return + } + var cleared, failed int + var firstErr string + for _, id := range ids { + ok, recErr := svc.ReconcileLeaseLossTombstone(ctx, id) + if recErr != nil { + failed++ + if firstErr == "" { + firstErr = recErr.Error() + } + continue + } + if ok { + cleared++ + } + } + if failed > 0 { + diag.Log(ctx, port.LevelWarn, "stale-session sweep: some lease-loss tombstone reconciles failed", + "failed", failed, "first_err", firstErr) + } + if cleared > 0 { + diag.Log(ctx, port.LevelInfo, "stale-session sweep: cleared stranded lease-loss tombstones", + "cleared", cleared) + } +} diff --git a/internal/app/session_reconcile_test.go b/internal/app/session_reconcile_test.go index 137495f463..636a3c67f7 100644 --- a/internal/app/session_reconcile_test.go +++ b/internal/app/session_reconcile_test.go @@ -2,7 +2,10 @@ package app import ( "context" + "errors" + "iter" "sync" + "sync/atomic" "testing" "time" @@ -298,6 +301,148 @@ func TestSweepStaleSessionsSkipsWhenLeaseSweepDisabled(t *testing.T) { } } +// fakeLease is a minimal programmable port.SessionLease for the sweep's +// lease-loss-tombstone wiring test — a package-local counterpart to +// internal/adapter/server's own (unexported, so not importable here) fakeLease. +type fakeLease struct { + mu sync.Mutex + acquires int + renewHook func(port.Lease) (port.Lease, error) + releases int +} + +func (f *fakeLease) Acquire(_ context.Context, id session.SessionID, owner string) (port.Lease, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.acquires++ + return port.Lease{SessionID: id, Owner: owner, Token: 1, Expiry: time.Now().Add(time.Hour)}, nil +} + +func (f *fakeLease) Renew(_ context.Context, l port.Lease) (port.Lease, error) { + f.mu.Lock() + hook := f.renewHook + f.mu.Unlock() + if hook != nil { + return hook(l) + } + return l, nil +} + +func (f *fakeLease) Release(context.Context, port.Lease) error { + f.mu.Lock() + f.releases++ + f.mu.Unlock() + return nil +} + +var _ port.SessionLease = (*fakeLease)(nil) + +// blockingLLM streams nothing until ctx is cancelled, then ends the stream — +// so a run stays live (StateRunning) until something cancels it, giving the +// renewer's lease-loss Cancel a live run to hit (mirrors +// internal/adapter/server/lease_test.go's own blockingProvider). +type blockingLLM struct{} + +func (blockingLLM) Stream(ctx context.Context, _ port.LLMRequest) (iter.Seq2[port.Chunk, error], error) { + return func(yield func(port.Chunk, error) bool) { + <-ctx.Done() + yield(port.Chunk{Kind: port.ChunkDone, Stop: session.StopCancelled}, nil) + }, nil +} + +func (blockingLLM) Capabilities() port.ProviderCapabilities { return port.ProviderCapabilities{} } + +var _ port.LLMProvider = blockingLLM{} + +// TestSweepStaleSessionsClearsLeaseLossTombstone is issue #1334's composition- +// wiring regression guard: sweepStaleSessions must itself call +// reconcileLeaseLossTombstones on every pass — a regression dropping that one +// call would pass every other sweep test in this file, since none of them +// ever set up a lease-loss tombstone. It drives a REAL lease loss (a live run +// cancelled by the renewer's definitive ErrLeaseHeld, exactly as +// internal/adapter/server/lease_test.go's own Service-level tests do), then +// proves the SWEEP itself (never a direct ReconcileLeaseLossTombstone call) +// clears the tombstone: StartRun is refused before the sweep runs and +// succeeds only after it. +func TestSweepStaleSessionsClearsLeaseLossTombstone(t *testing.T) { + lease := &fakeLease{} + store := memstore.New() + cat := tool.NewCatalog() + engine := agent.NewEngine(agent.Deps{ + LLM: blockingLLM{}, + Catalog: cat, + Policy: permpolicy.NewPolicy(nil, permstore.New()), + Model: "test-model", + }) + svc, err := newTestServerService(server.Config{ + Engine: engine, + Store: store, + SessionLease: lease, + LeaseOwner: "owner-test", + LeaseTTL: 90 * time.Millisecond, + LeaseRenewInterval: 15 * time.Millisecond, + }) + if err != nil { + t.Fatalf("new service: %v", err) + } + t.Cleanup(svc.Close) + + sess, err := svc.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + run, err := svc.StartRun(context.Background(), sess.ID, "go") + if err != nil { + t.Fatalf("StartRun: %v", err) + } + + var lost atomic.Bool + lease.mu.Lock() + lease.renewHook = func(port.Lease) (port.Lease, error) { + lost.Store(true) + return port.Lease{}, port.ErrLeaseHeld + } + lease.mu.Unlock() + for range run.Events() { + } + svc.FinishRun(sess.ID, run) + if !lost.Load() { + t.Fatal("precondition: the renewer never lost the lease") + } + + // The engine has no Store wired, so the durable snapshot is whatever this + // test writes directly (the crash-orphan-test idiom): the cancelled shape + // the real onLeaseLost cancel path leaves behind. + cancelled := session.New(sess.ID, session.ModeDefault, sess.EnvironmentRef, session.Limits{}, time.Unix(0, 0)) + if err := cancelled.RecordUserPrompt("go", nil); err != nil { + t.Fatalf("RecordUserPrompt: %v", err) + } + if err := cancelled.BeginTurn(); err != nil { + t.Fatalf("BeginTurn: %v", err) + } + if err := cancelled.Cancel(); err != nil { + t.Fatalf("Cancel: %v", err) + } + if err := store.Save(context.Background(), cancelled); err != nil { + t.Fatalf("overwrite Save: %v", err) + } + + if _, err := svc.StartRun(context.Background(), sess.ID, "again"); !errors.Is(err, server.ErrSessionLeasedElsewhere) { + t.Fatalf("StartRun before sweep = %v, want ErrSessionLeasedElsewhere", err) + } + + sweepStaleSessions(syscaller.Context(context.Background(), syscaller.RootStaleSessionReconcile), svc, port.NopDiagnostics{}) + + run2, err := svc.StartRun(context.Background(), sess.ID, "again") + if err != nil { + t.Fatalf("StartRun after sweep = %v, want success (the sweep must have cleared the lease-loss tombstone)", err) + } + run2.Cancel() + for range run2.Events() { + } + svc.FinishRun(sess.ID, run2) +} + // TestStartStaleSessionReconcileExitsOnCancel pins the goroutine-exit // contract: the sweeper goroutine started by startStaleSessionReconcile // returns when its returned close func is called. The package's goleak diff --git a/internal/syscaller/syscaller.go b/internal/syscaller/syscaller.go index 066d7a5347..7e5dad0626 100644 --- a/internal/syscaller/syscaller.go +++ b/internal/syscaller/syscaller.go @@ -44,7 +44,9 @@ const ( // not cover request-driven stale-model refreshes, which retain their caller. RootModelCatalogRefresh Root = "model-catalog-refresh" // RootStaleSessionReconcile enumerates and settles only crash-orphaned - // running-session metadata through the narrow server maintenance seam. + // running-session metadata through the narrow server maintenance seam, + // AND (issue #1334) clears this process's own lease-loss tombstones for + // awaiting/cancelled sessions once a trial-Acquire proves the lease free. RootStaleSessionReconcile Root = "stale-session-reconcile" // RootJWKSRefresh is the token validator's background JWKS refresh, which // owns the server-root context handed to the validator constructor. diff --git a/user-docs/features/session-continuity.md b/user-docs/features/session-continuity.md index d2e847789d..68852a8b30 100644 --- a/user-docs/features/session-continuity.md +++ b/user-docs/features/session-continuity.md @@ -144,6 +144,22 @@ prevents unsafe release assumptions. Without a suitable lease backend, destructive maintenance fails closed rather than relying on process-local liveness. +A lease loss is not always a genuine takeover: a missed renewal from a +transient network blip looks the same, at first, as losing the session to a +real competing owner. Either way, the owning process immediately stops acting +as owner and marks the session locally off-limits, refusing every further +prompt or resume attempt for it on its own. A background sweep, running every +five minutes, then checks with the real lease backend whether the lease has +actually become free; if it has, the sweep lifts the local mark automatically. +A follow-up prompt or approval can then repair the session's terminal state +through the ordinary run-entry recovery path, with no operator action and no +process restart required. Recovery depends on the lease backend itself being +reachable and able to confirm the lease is free: a failed or unavailable check +leaves the session refused for that pass, and the sweep simply retries five +minutes later, rather than assuming the lease is free on an inconclusive +answer. If the lease backend does not support leasing at all, the sweep never +runs, and the session stays refused until an explicit close. + ## Restart and deployment limitations - A durable snapshot does not preserve an in-flight Go goroutine. A process that