fix(server): clear lease-loss tombstones for awaiting/cancelled sessions - #1344
Conversation
JAORMX
left a comment
There was a problem hiding this comment.
Second-pass review of eae5a18: the normal awaiting/cancelled recovery path is correct, but the new reconciliation path can overlap backend Acquire with the loss handler Release for the same session. This violates the explicit SessionLease caller-serialization contract and needs fixing before approval; details inline. Please also update the canonical session-continuity documentation for next-sweep automatic recovery and correct the teardown-only lostOwnership description in docs/design/IMPLEMENTATION-NOTES.md. Non-blocking follow-ups: make the pre/post-trial bookkeeping robust against close/reacquire/second-loss, directly test authorization on both new maintenance methods, and exercise a real stateful competing holder rather than only a scripted ErrLeaseHeld. Trial Release failure is a temporary TTL-bound delay, not evidence of concurrent writers. Existing head CI is green; the branch currently conflicts with main. I ran no local tests or linters.
eae5a18 to
e5be498
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-reviewed fedd04b. leaseLossMu fixes the originally reported onLeaseLost Release versus reconcile Acquire overlap, and the living implementation notes are updated. One same-ID backend-call path remains outside that coordination: CloseSession can clear the tombstone while reconciliation is still releasing its trial lease, allowing a new StartRun to Acquire concurrently with that Release. This is the same SessionLease serialization requirement, not a new ownership policy; concrete interleaving inline. Please also complete the previously requested canonical user-docs/features/session-continuity.md update explaining automatic next-sweep recovery once the backend is available; the implementation notes do not replace user documentation. Non-blocking: the new regression test needs cleanup that unblocks releaseProceed on assertion failures. Current head CI is green and the branch is mergeable; no local tests or linters were run.
…ons (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 <noreply@anthropic.com>
Follow-up to 1ccb837, 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 <noreply@anthropic.com>
…l Acquire JAORMX's second-pass review of eae5a18 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 <noreply@anthropic.com>
…le trial JAORMX's second-pass review of fedd04b 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 <noreply@anthropic.com>
fedd04b to
5ed8d57
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Follow-up review of 5ed8d57: the production lock coordination and cleanup fix address the prior findings, but the new regression test does not exercise the claimed close/reopen-versus-trial-Release ordering, and the required user-facing cadence/backend-availability details are still absent. Please address these two blockers; CI is separate from this review.
… 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 <noreply@anthropic.com>
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed the exact head 4eaa054. The close/reconcile/reopen regression now starts the reopening admission while the trial Release is blocked and detects any real Acquire before release completion; the prior vacuous ordering is covered. The canonical session-continuity documentation now states the five-minute cadence and backend-free-confirmation condition. The remaining panel suggestions are non-blocking test determinism/editorial polish. No local tests or linters were run; CI is tracked separately.
Summary
When
Service.onLeaseLostdeclares a cross-process session-lease loss, it setsa permanent
lostOwnership[id]tombstone. Every normal caller ofacquireLease/reaffirmLease(StartRunContent,resumeFromAwaiting/ApproveRun,RenameSession,DeleteSession, etc.) fails fast on thattombstone forever, by design — the only two things that cleared it were
CloseSession(not something a normal client resuming a session calls) andthe
StateRunning-only stale-session reconcile sweep (#1302). ButonLeaseLostitself drives the session out ofStateRunningwhilehandling the loss (to
awaitingor eventuallycancelled), so the sweepnever rediscovers it and the tombstone stayed permanent — regardless of
whether the original loss was a genuine takeover or a false positive (see the
companion fix in #1333).
Adds
Service.LostOwnershipCandidates(an in-memory read of this process'sown lease-loss tombstones — deliberately not a store-wide scan, to avoid an
unbounded fan-out of trial-
Acquirecalls against every ordinaryawaiting/cancelled session) and
Service.ReconcileLeaseLossTombstone(abounded trial-
Acquire+immediate-release against the real backend, mirroringSessionStale's own refinement, that clears the tombstone — and any staleinvalid
heldLeasesbookkeeping — only on proof the lease is genuinely free,never unconditionally). Both are wired into the existing composition-level
stale-session sweep ticker, so a stranded tombstone self-heals on the next
pass instead of needing a manual
CloseSessionor process restart.Reviewed by a go-architect + kubernetes-operator-expert panel before
implementation, and by a full four-axis panel review after — no blockers
found; four important findings (a stale ADR line, two test-coverage gaps, and
a ~20-line duplicated trial-Acquire block versus
SessionStale) are alladdressed in the second commit, which also extracts the shared
leaseTrialhelper both call sites now use.
Development stage
Contract linkage
Service-level additions consumed only by the existing composition-level sweep (no new goroutine, no new gRPC/HTTP surface — confirmed by the security review below); gated by the samestaleReconcileAuthorizedsystem-root authorization every sibling stale-reconcile method already uses.Interface conformance
Servicemethods (LostOwnershipCandidates,ReconcileLeaseLossTombstone), both gated by the existingstaleReconcileAuthorizedsystem-root check — not reachable from any gRPC/HTTP-authenticated caller (verified: no new handler registers them; the only caller is the composition-level sweep). Registered inclassification.goand the SDK-parity test alongside their siblings.Issue relationship
Fixes #1334
Type of change
Test plan
Baseline checks
golangci-lint run --config .golangci.yml ./internal/adapter/server/... ./internal/app/...)go test ./internal/adapter/server/... ./internal/app/... -race -count=1)internal/, outside the engine module/panel-review-equivalent four-axis panel run manually —PANEL: ship_blockers=0 important=4 advisory=4 reviewer_failures=0; all four important findings fixed in the second commitFull-repo
task lint/task testintentionally not run in this environment (disk space constrained during development); scoped checks above cover every touched package plus a fullgo vet ./...sanity pass on the root module. CI will run the full gates.Changes
internal/adapter/server/service.goLostOwnershipCandidates,ReconcileLeaseLossTombstone, extracted sharedleaseTrialhelperinternal/adapter/server/classification.gointernal/adapter/server/sdk_typescript_release_test.gointernal/adapter/server/lease_test.gointernal/app/session_reconcile.gointernal/app/session_reconcile_test.goServicemethod) clears a tombstonedocs/adr/0027-cloud-native.mdinternal/syscaller/syscaller.goUser-facing change
A session whose cross-process lease was lost while parked awaiting a
permission approval, or after being cancelled, is no longer permanently stuck
read-only. It self-heals on the next stale-session sweep pass once the
underlying lease is verifiably free — no manual intervention or process
restart required.
Special notes for reviewers
The safety-critical property here is that the tombstone is never
cleared unconditionally — only after a real trial-
Acquireproves the leasefree, with a fail-safe error path that leaves the tombstone in place. This is
covered by
TestReconcileLeaseLossTombstoneRefusesWhenGenuinelyHeldElsewhereand the new
TestReconcileLeaseLossTombstoneFailSafeOnGenericError. Asecure-code-reviewer pass specifically traced the TOCTOU/split-brain question
and confirmed no double-holder scenario is reachable. See #1333 for the
companion fix (preventing a false-positive loss in the first place on the
single-host flock backend).