From 174576a66182754965f30fbafe665d2f89a90e9e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 10 Sep 2026 13:40:16 +0200 Subject: [PATCH 1/5] fix(server): clear lease-loss tombstones for awaiting/cancelled sessions (fixes #1334) onLeaseLost drives a session OUT of StateRunning while handling a declared lease loss (to awaiting via preserveAwaiting, or eventually cancelled), so the StateRunning-only stale-session sweep (SessionStale/SettleIfStale) could never rediscover it and the lostOwnership tombstone - a permanent fail-fast by design for every ordinary caller - stayed wedged short of CloseSession or a process restart. Add Service.LostOwnershipCandidates (an in-memory read of this process's own lease-loss tombstones, not a store-wide scan) and Service.ReconcileLeaseLossTombstone (a bounded trial-Acquire+immediate-release against the real backend, mirroring SessionStale's own refinement, that clears the tombstone plus any stale invalid heldLeases bookkeeping once the lease is proven genuinely free - never unconditionally). Wire both into the existing composition-level stale-session sweep so a stranded tombstone self-heals on the next pass instead of needing a manual CloseSession or restart. Co-Authored-By: Claude Sonnet 5 --- internal/adapter/server/classification.go | 2 + internal/adapter/server/lease_test.go | 328 ++++++++++++++++++ .../server/sdk_typescript_release_test.go | 2 + internal/adapter/server/service.go | 153 +++++++- internal/app/session_reconcile.go | 44 +++ 5 files changed, 524 insertions(+), 5 deletions(-) diff --git a/internal/adapter/server/classification.go b/internal/adapter/server/classification.go index ceb7e4d6b9..1aae9b5f70 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"}, } diff --git a/internal/adapter/server/lease_test.go b/internal/adapter/server/lease_test.go index 6964344730..00ac7d1703 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -2,6 +2,7 @@ package server_test import ( "context" + "encoding/json" "errors" "iter" "sync" @@ -770,3 +771,330 @@ 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) +} + +// 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) + } +} 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..b0fe8d95c5 100644 --- a/internal/adapter/server/service.go +++ b/internal/adapter/server/service.go @@ -1030,10 +1030,15 @@ 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{} @@ -1052,7 +1057,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 @@ -7027,6 +7034,142 @@ 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. +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 + } + 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 + } + + 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): + // Still genuinely held — a live peer, or this process's own + // not-yet-expired record from before the loss was declared. Leave the + // tombstone; the next sweep pass re-checks. + return false, nil + 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, "lease-loss tombstone reconcile: lease backend does not support leasing; disabling the sweep", + "owner", s.cfg.LeaseOwner) + } + return false, nil + case err != nil: + s.cfg.Diagnostics.Log(ctx, port.LevelWarn, "lease-loss tombstone reconcile: trial lease acquire failed; leaving tombstone in place (fail-safe)", + "session", string(id), "err", err.Error()) + return false, nil + } + // Success: nobody holds it. Release immediately — nothing is held across + // this call, mirroring SessionStale's own trial. + relCtx, relCancel := context.WithTimeout(context.WithoutCancel(ctx), leaseAcquireTimeout) + _ = s.cfg.SessionLease.Release(relCtx, lease) + relCancel() + + 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) + } +} From 824b64354a209de1bf3ff80866b6976f56fab142 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 10 Sep 2026 15:27:30 +0200 Subject: [PATCH 2/5] fix(server): address panel findings on the #1334 lease-tombstone fix Follow-up to 1ccb837a2, addressing a panel review of that commit: - Extract the ~30-line trial-Acquire -> switch -> Release block SessionStale and ReconcileLeaseLossTombstone each duplicated into one shared Service.leaseTrial helper. held is non-nil only for a genuine ErrLeaseHeld; each caller keeps its own distinct ownership judgement (SessionStale's self-held-lease correction; ReconcileLeaseLossTombstone's leave-the-tombstone-in-place). - Add internal/app/session_reconcile_test.go's TestSweepStaleSessionsClearsLeaseLossTombstone: drives a real lease loss through a live run, then proves sweepStaleSessions itself (not a direct ReconcileLeaseLossTombstone call) clears the tombstone via reconcileLeaseLossTombstones - closing the composition-wiring coverage gap a regression dropping that one call would have slipped through. - Add TestReconcileLeaseLossTombstoneFailSafeOnGenericError: a bare non-sentinel Acquire error must leave the tombstone in place, never clear it on ambiguity. - Log a WARN when the trial's own Release fails (a leaked trial otherwise silently pins the lease until TTL with no diagnostic trail). - Update docs/adr/0027-cloud-native.md row 27, internal/syscaller's RootStaleSessionReconcile doc comment, and classification.go's rationale for that root to describe BOTH tombstone-clearing exceptions (SettleIfStale for StateRunning, ReconcileLeaseLossTombstone for awaiting/cancelled) instead of the now-stale "one narrow exception" text. - Note the bounded CloseSession/ReconcileLeaseLossTombstone interleave in the latter's doc comment. Co-Authored-By: Claude Sonnet 5 --- docs/adr/0027-cloud-native.md | 2 +- internal/adapter/server/classification.go | 2 +- internal/adapter/server/lease_test.go | 85 +++++++++++++ internal/adapter/server/service.go | 115 ++++++++++------- internal/app/session_reconcile_test.go | 145 ++++++++++++++++++++++ internal/syscaller/syscaller.go | 4 +- 6 files changed, 304 insertions(+), 49 deletions(-) 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/internal/adapter/server/classification.go b/internal/adapter/server/classification.go index 1aae9b5f70..4a00f79331 100644 --- a/internal/adapter/server/classification.go +++ b/internal/adapter/server/classification.go @@ -474,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 00ac7d1703..c15af22bcd 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -1098,3 +1098,88 @@ func TestReconcileLeaseLossTombstoneRefusesWhenGenuinelyHeldElsewhere(t *testing 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/service.go b/internal/adapter/server/service.go index b0fe8d95c5..ee07de3968 100644 --- a/internal/adapter/server/service.go +++ b/internal/adapter/server/service.go @@ -6919,44 +6919,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 @@ -7094,6 +7129,14 @@ func (s *Service) LostOwnershipCandidates(ctx context.Context) ([]session.Sessio // 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. +// +// RESIDUAL INTERLEAVE (bounded, safe): CloseSession can race this call and +// clear s.lostOwnership[id] itself (unconditionally, via closeSessionLocal) +// between the pre-trial check and the post-trial re-check below. Both +// re-checks re-read s.lostOwnership under s.mu, so a CloseSession that wins +// the race simply makes this call an honest no-op (false, nil) rather than a +// double-clear or a stale write — the same "last write wins, re-verified +// under the lock" posture SettleIfStale documents for its own TOCTOU window. func (s *Service) ReconcileLeaseLossTombstone(ctx context.Context, id session.SessionID) (bool, error) { if !staleReconcileAuthorized(ctx) { return false, ErrManagementUnauthorized @@ -7121,35 +7164,15 @@ func (s *Service) ReconcileLeaseLossTombstone(ctx context.Context, id session.Se return false, nil } - 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): - // Still genuinely held — a live peer, or this process's own - // not-yet-expired record from before the loss was declared. Leave the - // tombstone; the next sweep pass re-checks. - return false, nil - 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, "lease-loss tombstone reconcile: lease backend does not support leasing; disabling the sweep", - "owner", s.cfg.LeaseOwner) - } - return false, nil - case err != nil: - s.cfg.Diagnostics.Log(ctx, port.LevelWarn, "lease-loss tombstone reconcile: trial lease acquire failed; leaving tombstone in place (fail-safe)", - "session", string(id), "err", err.Error()) + 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 } - // Success: nobody holds it. Release immediately — nothing is held across - // this call, mirroring SessionStale's own trial. - relCtx, relCancel := context.WithTimeout(context.WithoutCancel(ctx), leaseAcquireTimeout) - _ = s.cfg.SessionLease.Release(relCtx, lease) - relCancel() s.mu.Lock() defer s.mu.Unlock() 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. From 1ed0a3d7d5e1d559b40b026183bb7f88934c4bf6 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 14 Sep 2026 12:25:52 +0200 Subject: [PATCH 3/5] fix(server): serialize onLeaseLost's Release against reconcile's trial Acquire JAORMX's second-pass review of eae5a18f flagged that ReconcileLeaseLossTombstone's trial Acquire could overlap onLeaseLost's real Release for the same session id, violating engine/port.SessionLease's caller-serialization contract for same-id calls. Add a dedicated per-id lock (leaseLossMu, separate from runEntryMu to avoid the documented cancellation-deadlock hazard) held across each function's entire body, and a regression test that blocks Release mid-flight to prove the trial Acquire cannot start until it returns. Also corrects IMPLEMENTATION-NOTES.md's now-stale "teardown-only" description of lostOwnership clearing to describe the automatic sweep path. Co-Authored-By: Claude Sonnet 5 --- docs/design/IMPLEMENTATION-NOTES.md | 13 ++- internal/adapter/server/lease_test.go | 150 +++++++++++++++++++++++++- internal/adapter/server/service.go | 36 ++++++- 3 files changed, 195 insertions(+), 4 deletions(-) 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/lease_test.go b/internal/adapter/server/lease_test.go index c15af22bcd..b22390c90c 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "iter" + "strings" "sync" "sync/atomic" "testing" @@ -31,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 @@ -41,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 } @@ -889,6 +904,139 @@ func TestReconcileLeaseLossTombstoneRecoversCancelledSession(t *testing.T) { 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 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: + } + + close(releaseProceed) + + 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()) + } +} + // TestReconcileLeaseLossTombstoneRecoversAwaitingSession is the awaiting // counterpart: onLeaseLost's preserveAwaiting branch drives the session to // StateAwaiting (parked on a permission ask) rather than cancelling it, and diff --git a/internal/adapter/server/service.go b/internal/adapter/server/service.go index ee07de3968..ba452c8e38 100644 --- a/internal/adapter/server/service.go +++ b/internal/adapter/server/service.go @@ -1043,6 +1043,26 @@ type Service struct { // fast on it forever. lostOwnership map[session.SessionID]struct{} + // leaseLossMu serializes onLeaseLost's real backend Release against + // ReconcileLeaseLossTombstone's trial Acquire for the SAME session id + // (issue #1334 panel finding): onLeaseLost sets lostOwnership[id] and + // unlocks s.mu well before it calls SessionLease.Release, and the + // composition-level sweep can observe the tombstone and start a trial + // Acquire 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. Held for + // each function's ENTIRE body (a keyedMutex, freed once no caller holds + // the key) so the two are strictly ordered: either the whole loss + // handling (tombstone + Release) completes before a trial starts, or the + // whole trial (+ tombstone-clear) completes before a loss is handled. + 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 @@ -6734,8 +6754,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 @@ -7137,6 +7162,11 @@ func (s *Service) LostOwnershipCandidates(ctx context.Context) ([]session.Sessio // the race simply makes this call an honest no-op (false, nil) rather than a // double-clear or a stale write — 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 @@ -7144,6 +7174,10 @@ func (s *Service) ReconcileLeaseLossTombstone(ctx context.Context, id session.Se if s.cfg.SessionLease == nil { return false, nil } + + unlock := s.leaseLossMu.lock(id) + defer unlock() + s.mu.Lock() if s.leaseDisabled { s.mu.Unlock() From 5ed8d57d59ddadf641f9df30a2d36abad0ec535c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 14 Sep 2026 13:20:26 +0200 Subject: [PATCH 4/5] fix(server): serialize CloseSession's tombstone clear against reconcile trial JAORMX's second-pass review of fedd04b0 found leaseLossMu had one more gap: closeSessionLocal unconditionally clears the lostOwnership tombstone without taking leaseLossMu, so CloseSession could still let a new real Acquire begin while ReconcileLeaseLossTombstone's own trial Release was in flight for the same session id - the same same-id overlap the earlier fix closed for onLeaseLost, just reached through a second writer. closeSessionLocal now takes leaseLossMu too, in the fixed order runEntryMu -> leaseLossMu (every caller already holds runEntryMu; onLeaseLost and ReconcileLeaseLossTombstone never do, so no new cycle). Also completes the previously-requested user-docs/features/session-continuity.md update describing the automatic next-sweep recovery, and fixes the earlier regression test's cleanup so a failing assertion can't leave the release-hook goroutine blocked. Co-Authored-By: Claude Sonnet 5 --- internal/adapter/server/lease_test.go | 190 ++++++++++++++++++++++- internal/adapter/server/service.go | 77 +++++---- user-docs/features/session-continuity.md | 12 ++ 3 files changed, 252 insertions(+), 27 deletions(-) diff --git a/internal/adapter/server/lease_test.go b/internal/adapter/server/lease_test.go index b22390c90c..302890af25 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -947,6 +947,13 @@ func TestOnLeaseLostSerializesAgainstReconcileTrial(t *testing.T) { 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() @@ -1022,7 +1029,7 @@ func TestOnLeaseLostSerializesAgainstReconcileTrial(t *testing.T) { default: } - close(releaseProceed) + unblockRelease() select { case <-reconcileDone: @@ -1037,6 +1044,187 @@ func TestOnLeaseLostSerializesAgainstReconcileTrial(t *testing.T) { } } +// 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. This +// blocks that trial Release, races a concurrent CloseSession against it, +// confirms CloseSession cannot complete until the trial Release returns, and +// then confirms the REOPENING StartRun's real Acquire only happens after — +// proving closeSessionLocal's own leaseLossMu acquisition orders them, not +// timing. +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) + } + + 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() + + // Drive the loss to completion FIRST (onLeaseLost's own Release runs + // unblocked): the tombstone is set and the lease's real Release has + // already happened, mirroring the sequential setup in + // TestReconcileLeaseLossTombstoneRecoversCancelledSession. + 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") + } + + // 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 already ran above, + // unblocked, under the default nil releaseHook (f.releases already + // counts it) — this NEW hook only ever observes the trial's call, so its + // own 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: + } + + 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") + } + + // The tombstone is gone (whichever of Reconcile/Close cleared it last), + // so a normal re-entry now succeeds — and, critically, its real Acquire + // must only have happened (if at all so far) after the trial Release + // returned. + run2, err := svc.StartRun(context.Background(), sess.ID, "again") + if err != nil { + t.Fatalf("StartRun after close+reconcile = %v, want success (tombstone should be cleared)", err) + } + 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 diff --git a/internal/adapter/server/service.go b/internal/adapter/server/service.go index ba452c8e38..2711d94eb0 100644 --- a/internal/adapter/server/service.go +++ b/internal/adapter/server/service.go @@ -1043,24 +1043,31 @@ type Service struct { // fast on it forever. lostOwnership map[session.SessionID]struct{} - // leaseLossMu serializes onLeaseLost's real backend Release against - // ReconcileLeaseLossTombstone's trial Acquire for the SAME session id - // (issue #1334 panel finding): onLeaseLost sets lostOwnership[id] and - // unlocks s.mu well before it calls SessionLease.Release, and the - // composition-level sweep can observe the tombstone and start a trial - // Acquire 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. Held for - // each function's ENTIRE body (a keyedMutex, freed once no caller holds - // the key) so the two are strictly ordered: either the whole loss - // handling (tombstone + Release) completes before a trial starts, or the - // whole trial (+ tombstone-clear) completes before a loss is handled. + // 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 @@ -2798,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. @@ -7155,13 +7176,17 @@ func (s *Service) LostOwnershipCandidates(ctx context.Context) ([]session.Sessio // tombstone for this id"). Exported for internal/app's composition-level // sweep, mirroring SessionStale/SettleIfStale/StaleRunningCandidates. // -// RESIDUAL INTERLEAVE (bounded, safe): CloseSession can race this call and -// clear s.lostOwnership[id] itself (unconditionally, via closeSessionLocal) -// between the pre-trial check and the post-trial re-check below. Both -// re-checks re-read s.lostOwnership under s.mu, so a CloseSession that wins -// the race simply makes this call an honest no-op (false, nil) rather than a -// double-clear or a stale write — the same "last write wins, re-verified -// under the lock" posture SettleIfStale documents for its own TOCTOU window. +// 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 diff --git a/user-docs/features/session-continuity.md b/user-docs/features/session-continuity.md index d2e847789d..2a193981e5 100644 --- a/user-docs/features/session-continuity.md +++ b/user-docs/features/session-continuity.md @@ -144,6 +144,18 @@ 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 then checks with +the real lease backend, on a bounded interval, 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. Until the sweep confirms this, the session stays +refused, even if the original loss turns out to have been a false alarm. + ## Restart and deployment limitations - A durable snapshot does not preserve an in-flight Go goroutine. A process that From 4eaa054b6df6ba551531bd155866c442d341f168 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 14 Sep 2026 13:56:25 +0200 Subject: [PATCH 5/5] fix(server): make the close/reconcile race test actually exercise the race JAORMX's third-pass review found the prior regression test's overlap assertion was vacuous: the reopening StartRun ran only after both CloseSession and Reconcile were already confirmed done, by which point the trial Release had necessarily completed. Fixed by starting a polling reopen goroutine concurrently with CloseSession, right after the trial Release is confirmed blocked (and before it is ever unblocked) - while the tombstone is genuinely still set, each poll is refused locally and never reaches the backend, so the real Acquire can only happen once the tombstone actually clears. Also fixed a related gap: draining to StopCancelled only proves onLeaseLost called run.Cancel(), which precedes its own Release in the function body, not that the Release itself returned - added an explicit "first Release completed" signal before installing the trial-blocking hook. Also states the actual five-minute sweep cadence and the lease-backend- availability caveat in user-docs/features/session-continuity.md, per the same review. Co-Authored-By: Claude Sonnet 5 --- internal/adapter/server/lease_test.go | 107 ++++++++++++++++++----- user-docs/features/session-continuity.md | 16 ++-- 2 files changed, 94 insertions(+), 29 deletions(-) diff --git a/internal/adapter/server/lease_test.go b/internal/adapter/server/lease_test.go index 302890af25..79ad87cca0 100644 --- a/internal/adapter/server/lease_test.go +++ b/internal/adapter/server/lease_test.go @@ -1050,12 +1050,26 @@ func TestOnLeaseLostSerializesAgainstReconcileTrial(t *testing.T) { // 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. This -// blocks that trial Release, races a concurrent CloseSession against it, -// confirms CloseSession cannot complete until the trial Release returns, and -// then confirms the REOPENING StartRun's real Acquire only happens after — -// proving closeSessionLocal's own leaseLossMu acquisition orders them, not -// timing. +// (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() @@ -1089,18 +1103,28 @@ func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { t.Fatalf("StartRun: %v", err) } - var lost atomic.Bool + // 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 to completion FIRST (onLeaseLost's own Release runs - // unblocked): the tombstone is set and the lease's real Release has - // already happened, mirroring the sequential setup in - // TestReconcileLeaseLossTombstoneRecoversCancelledSession. + // 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 @@ -1110,6 +1134,11 @@ func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { 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 @@ -1129,10 +1158,9 @@ func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { } // Now block the TRIAL's own self-Release (leaseTrial's immediate - // Acquire-then-Release). onLeaseLost's own Release already ran above, - // unblocked, under the default nil releaseHook (f.releases already - // counts it) — this NEW hook only ever observes the trial's call, so its - // own first invocation IS the trial's self-Release. + // 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 @@ -1194,6 +1222,40 @@ func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { 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 { @@ -1206,14 +1268,13 @@ func TestCloseSessionSerializesAgainstReconcileTrial(t *testing.T) { case <-time.After(2 * time.Second): t.Fatal("CloseSession never completed after the trial Release was unblocked") } - - // The tombstone is gone (whichever of Reconcile/Close cleared it last), - // so a normal re-entry now succeeds — and, critically, its real Acquire - // must only have happened (if at all so far) after the trial Release - // returned. - run2, err := svc.StartRun(context.Background(), sess.ID, "again") - if err != nil { - t.Fatalf("StartRun after close+reconcile = %v, want success (tombstone should be cleared)", err) + 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() { diff --git a/user-docs/features/session-continuity.md b/user-docs/features/session-continuity.md index 2a193981e5..68852a8b30 100644 --- a/user-docs/features/session-continuity.md +++ b/user-docs/features/session-continuity.md @@ -148,13 +148,17 @@ 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 then checks with -the real lease backend, on a bounded interval, 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 +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. Until the sweep confirms this, the session stays -refused, even if the original loss turns out to have been a false alarm. +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