🐛 fix(contribute): persist task leases across hub restarts (v4 port of #5688) - #5721
Merged
Merged
Conversation
A hub restart threw away every in-flight contributor task — and then handed the identical issue back seconds later. A relay holds one task at a time and keeps working through a brief disconnect, re-asserting the task when it reconnects. The hub honours that re-assertion only against a server-issued lease (lookupLease), which is deliberate: C4 established that a client must never be able to assert ownership of work the server did not assign. But leases lived only in process memory. A restart emptied the registry, so after an upgrade NO in-flight resume could match: the relay was told "no active lease for this task", the revoke interrupted the agent mid-turn, and the same issue was re-assigned as fresh work. Observed 2026-09-02 (#5681): revoked at 14:24:40, the same issue reassigned to the same relay at 14:24:44, discarding two and a half minutes of a turn that was progressing normally. Thirteen shell commands and a fork/clone in, then killed. Ownership was never in question — only the record of it. Self-upgrade rolls (#5391) make this routine rather than rare, and it hits every contributor holding a task at the moment of any restart. The registry is now persisted to /data/contributors/task-leases.json on every assignment, renewal and release, and restored at startup. This does not weaken C4. The restored record is one the SERVER wrote; a resume still has to match it exactly on {identity, task_id, repo, number, generation} and still has to be inside the window. Nothing is reconstructed from client-supplied fields, the file carries no credential (the scoped token is minted per assignment and delivered separately, #2537), it is written 0600 rather than the sibling ledgers' 0644 because it is an authorization record rather than a report, and a lease already past its expiry is dropped at load rather than restored. Three couplings move with it: - The RENEWED window is what gets persisted. Persisting only the assignment-time window would bring a task that had been progressing for longer than leaseTTL back already expired — precisely the defect #4260 fixed in memory. - taskGen is advanced past every restored generation at boot. It is an in-memory counter that restarts at zero, so persisting leases without this would let a post-restart assignment mint a fencing token that ALIASES a restored one, and the #2568 Gate would accept a pre-restart straggler against a brand-new task. src/docs/design/agent-state-inventory.md residual 2 called this exact hazard out in advance ("any change that persists one without the other silently breaks the fence"). - For leaseHoldGraceAfterStart (2 min) after boot, a RESTORED lease also holds its work item in the double-assignment guard, which is otherwise built purely from live connections and is empty right after a restart. Without it, making the resume work would convert "lose the task" into two relays on one issue. Deliberately scoped: a lease is not a hold in steady state — a dropped socket keeps its lease so the relay can resume (#4260) while its item merely cools down (#2356), and honouring leases as holds for the full 30-minute TTL would silently replace that hedge with a long park on every disconnect. Leases minted by this process never act as holds; their holders have live connections, which the existing guard already covers. The lease also now carries the item's canonical worksource key, so the guard recognises external work (Linear/Jira items carry Number == 0 and put their identity in Key, #4245) instead of colliding every such item as "repo#0" (#5120). #4260's contribute_reconnect_resume_test.go pins the resume contract across a SOCKET drop and passed throughout — that reconnect is to a live hub. The new contribute_lease_restart_test.go exercises the same contract across a PROCESS boundary, which had no coverage: it drives the real protocol through a hub replaced by a freshly constructed one over the same /data, and fails on the incident's own string ("no active lease for this task") when the restore is removed. Refs #5681 Signed-off-by: Doug Baggett <doug.baggett@gmail.com> (cherry picked from commit 2714b1c) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
- contributor-relay.md: the resume section already documented the disconnect case and the exact four-line revoke/reassign symptom. Adds the restart case beside it, including that the restored record is still hub-written and still matched exactly, and the two-minute post-restart hold. - changelog.d/fixed-5681-task-lease-restart.md: user-visible — in-flight contributor work now survives an upgrade roll. v4 adaptations of the v5 original: the changelog entry lands as a changelog.d fragment rather than a CHANGELOG.md edit (#5675), and the agent-state-inventory.md rows are dropped because that design document does not exist on v4 — its residual-2 warning about persisting leases without the generation counter is carried in loadLeases' comments and pinned by TestLeaseRestart_GenerationAdvancesPastRestoredLeases instead. Refs #5681 Signed-off-by: Doug Baggett <doug.baggett@gmail.com> (cherry picked from commit bb3812a) Signed-off-by: Andrew Anderson <andy@clubanderson.com>
#5625 idiom) The lease registry write used the older fixed-name tmp + rename idiom. This upgrades it to the #5625 standard the mutation claim ledger set: a UNIQUE os.CreateTemp name (a fixed name lets a non-cooperating process clobber a commit in flight), an explicit chmod pinning the 0600 owner-only invariant rather than inheriting it from CreateTemp, an fsync of the bytes before the rename — the whole point of this file is that the next process boots from it, so the record must be durable, not merely renamed — and an fsync of the directory so the rename itself survives a crash. A failed attempt removes its unique temp file instead of leaving a stale fixed-name .tmp beside the registry. The sibling contributor ledgers (0644 reports, not authorization records) keep their existing idiom; upgrading them is out of scope for this port. Refs #5681 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
Bot
added
dco-signoff: yes
clubanderson
added
lgtm
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
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
2 participants
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.
What
v4 port of #5688 (authored by Danathar — the fix and its analysis are his work; this PR carries his commits with authorship preserved). The live hub runs v4 and loses every in-flight contributor task lease on each restart, which today's
:stableroll churn makes a recurring interruption rather than a rare one.A relay holds one task at a time and keeps working through a brief disconnect, re-asserting the task on reconnect. The hub honours that re-assertion only against a server-issued lease — but leases lived only in process memory, so every restart emptied the registry, every in-flight resume was answered
no active lease for this task, the agent was interrupted mid-turn, and the identical issue was handed back seconds later. The registry is now persisted to/data/contributors/task-leases.jsonon every assignment, renewal and release, and restored at startup.Fixes #5681
Correctness properties preserved from the v5 review
All verified on v4 by the ported test suite (10 cases, all passing):
taskGenis re-derived past every restored lease at boot, so a post-restart assignment can never mint a fencing token that aliases a pre-restart one and slip a straggler past thegenerationAcceptedgate — pinned byTestLeaseRestart_GenerationAdvancesPastRestoredLeases.TestLeaseRestart_RestoredLeaseHoldsItsIssue), and the hold lapses after the grace window (TestLeaseRestart_HoldLapsesAfterGrace) — a lease is deliberately not a hold in steady state.TestLeaseRestart_FileIsOwnerOnly).TestLeaseRestart_ExpiredLeaseIsNotRestored,TestLeaseRestart_RevokedLeaseStaysRevoked); the renewed window is what gets persisted (TestLeaseRestart_RenewedWindowSurvives).TestLeaseRestart_ResumeSurvivesHubRestartdrives the real protocol through a hub replaced by a freshly constructed one over the same/data, and fails on the incident's own string when the restore is removed.v4 adaptations
The fix commit cherry-picked cleanly — v4's
contribute_ws.gocarries the same generation fence and lease registry despite v5's dashboard decomposition. The docs commit needed three adaptations:agent-state-inventory.mdrows dropped — that design document does not exist on v4; its load-bearing residual-2 warning (persisting leases without the generation counter silently breaks the fence) is carried inloadLeases' comments and pinned by test instead.tmp+ rename idiom as a follow-up; since this file is what the next process boots from, the port lands the crash-safe form directly: uniqueos.CreateTempname, explicit chmod pinning 0600, fsync of the bytes before the rename, rename, then fsync of the directory. Failed attempts remove their unique temp file. The 0644 sibling report ledgers keep their existing idiom.Note for v5: item 3 is a delta this port has that v5
HEADdoes not — v5'ssaveLeasesLockedstill uses the fixed-nametmp+ rename idiom and should get the same #5625 upgrade as a follow-up.Coverage
pkg/dashboardwithgo test -short -race -count=1 -coverprofile: 92.0% of statements — above the package's documented 84% floor and the 90% default.🤖 Generated with Claude Code