fix(flocklease): reclaim expired lease when record still names caller - #1343
Open
jhrozek wants to merge 2 commits into
Open
fix(flocklease): reclaim expired lease when record still names caller#1343jhrozek wants to merge 2 commits into
jhrozek wants to merge 2 commits into
Conversation
flocklease.Renew treated any expired lease record as definitive loss (ErrLeaseHeld), even when the record still named the calling process as owner at its own fencing token. On a single-host deployment, a process suspended past the TTL (e.g. laptop sleep) would lose its lease on the next Renew after resume even though nothing else could have raced it. The safe invariant is not "the flock generation handle is still open" (Acquire can take over an expired record regardless), but that the durable record, read under the same stable transition lock Acquire/Release use, still shows the same owner at the same fencing token. On a single host that can only be true if nobody else raced an Acquire/takeover in the interim, so Renew now reclaims with a fresh expiry (token unchanged) instead of declaring loss. A record whose owner or token changed during the gap (a genuine takeover) still returns ErrLeaseHeld unconditionally. Clarifies port.SessionLease.Renew's doc comment (narrowing only, no contract change: implementations may still always treat expiry as loss if they can't prove otherwise) and flocklease.Renew's own comment. Rewrites the flocklease test that encoded the old behavior into two tests covering the reclaim-with-no-competitor and still-fails-after-genuine-takeover cases. Scope is flocklease only; k8slease and grpcdriver/sessionlease.go are untouched per the panel review in the issue. fixes #1333 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Panel review on 56b9bce flagged that the held == nil hard-fail branch in Renew -- the one that actually carries the cross-process safety argument -- wasn't isolated by either new test from that commit. Add TestRenewFailsWithoutLocallyTrackedGeneration: two independent flocklease.New instances over the same dir; the first Acquires, then Renew is called on the SECOND (unheld) instance with a copy of the first's returned port.Lease. The durable record still names that exact owner and token and has not expired, yet the second instance never populated its own `held` map for that generation, so Renew must still fail closed with ErrLeaseHeld. Also records the narrowed Renew/ErrLeaseHeld port contract in engine/CHANGELOG.md (doc-comment-only clarification, no exported signature changed, no task api:update needed) and notes the new uncontested-expiry-reclaim behavior in the flocklease adapter bullet of docs/adr/0027-cloud-native.md's resource inventory. Follow-up to #1333. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flocklease.Renewtreated any expired lease record as definitive loss(
port.ErrLeaseHeld), even when the record still named the calling process asowner at its own fencing token. On a single-host deployment, a process
suspended past the TTL (e.g. a laptop going to sleep) lost its lease on the
next
Renewafter resume, even though nothing else could possibly have racedit — a single host, and no competing process ever ran while this one was
suspended.
renewLoopthen treated that as immediate, ungraced loss andcancelled the run.
The safe invariant isn't "the flock generation handle is still open" (
Acquirecan take over an expired record regardless of whether the old holder's flock
is still technically held) — it's that the durable record, read under the same
stable transition lock
Acquire/Releasealready use, still shows the sameowner at the same fencing token. On a single host that can only be true if
nobody else raced an
Acquire/takeover in the interim, soRenewnow reclaimswith a fresh expiry (token unchanged) instead of declaring loss. A record whose
owner or token changed during the gap (a genuine takeover) still returns
ErrLeaseHeldunconditionally.Reviewed by a go-architect + kubernetes-operator-expert panel before
implementation (confirmed this does not generalize to
k8slease/grpcdriver— those backends have no equivalent local proof of exclusivity,and a genuine TTL blowout losing the lease there is correct behavior), and by
a full four-axis panel review (Spec/Standards/Test-adequacy/Domain) after
implementation — no blockers found; the one important finding (an untested
held == nilhard-fail branch) is addressed in the second commit.Development stage
Contract linkage
Renew/ErrLeaseHeldcontract intent; introduces no new durable architecture decision, no public API/persistence/trust-boundary change.internal/adapter/flocklease+ a narrowing doc-comment clarification onengine/port/lease.go(no exported signature changed,task api:checkunaffected). No gRPC/protobuf, CLI/config, or persistence-format change.Interface conformance
port.SessionLease.Renew's doc comment is clarified, not widened: bare expiry with the record still naming the caller's owner+token is no longer, by itself, defined as loss — only a holder/token change is. This narrows (never widens) when an implementation may returnErrLeaseHeld; every other backend (k8slease,grpcdriver,memlease) is untouched and still treats bare expiry as loss.Issue relationship
Fixes #1333
Type of change
Test plan
Baseline checks
golangci-lint run --config .golangci.yml ./internal/adapter/flocklease/...and./engine/port/...)go test ./internal/adapter/flocklease/... -race -count=1and./engine/port/... -race -count=1)engine/CHANGELOG.md+docs/adr/0027-cloud-native.mdtouched;matlatl check . --strictrun clean (372 documents, 3553 references, 0 broken)engine/port/lease.go, no exported signature moved/panel-review-equivalent four-axis panel run manually (see PR discussion) —PANEL: ship_blockers=0 important=1 advisory=6 reviewer_failures=0; the one important finding is 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 ./.../go build ./...sanity pass on both modules. CI will run the full gates.Changes
engine/port/lease.goRenew's doc comment: bare expiry with the record still naming the caller is not itself lossinternal/adapter/flocklease/flocklease.goRenewreclaims an expired-but-uncontested lease instead of returningErrLeaseHeldinternal/adapter/flocklease/flocklease_test.goheld == nilhard-failengine/CHANGELOG.mdRenew/ErrLeaseHeldcontractdocs/adr/0027-cloud-native.mdUser-facing change
A
mecated/mecak8sdeployment using the single-host--session-lease-dirbackend no longer loses a session's write access every time the host process is suspended (e.g. a laptop going to sleep) past the lease TTL, as long as nothing else actually took over the session in the meantime.Special notes for reviewers
Scope is deliberately narrow:
k8sleaseandgrpcdriver/sessionlease.goare untouched. A genuine TTL blowout on those backends should still lose the lease — there is no local proof of exclusivity there the way flock's stable transition lock provides. See #1333 and #1334 for the companion fix (a separate, orthogonal issue: once a loss is declared, for any reason, the resulting tombstone was permanent for non-StateRunningsessions).