From 2714b1c7477369bc50fe11dbdd64f5fa418c5dfa Mon Sep 17 00:00:00 2001 From: Doug Baggett Date: Wed, 2 Sep 2026 11:26:43 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20fix(contribute):=20make=20th?= =?UTF-8?q?e=20task=20lease=20outlive=20the=20hub=20process?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/pkg/dashboard/api_contribute_test.go | 3 + .../contribute_lease_restart_test.go | 422 ++++++++++++++++++ src/pkg/dashboard/contribute_ws.go | 335 +++++++++++++- 3 files changed, 753 insertions(+), 7 deletions(-) create mode 100644 src/pkg/dashboard/contribute_lease_restart_test.go diff --git a/src/pkg/dashboard/api_contribute_test.go b/src/pkg/dashboard/api_contribute_test.go index 8b5801894..019fa643b 100644 --- a/src/pkg/dashboard/api_contribute_test.go +++ b/src/pkg/dashboard/api_contribute_test.go @@ -31,16 +31,19 @@ func setupContributeEnv(t *testing.T) { func redirectContributeWSDisk(t *testing.T, dir string) { t.Helper() oldActivity, oldCompleted, oldFailed, oldNoPR := activityFilePath, completedTasksFile, failedTasksFile, noPRStreaksFile + oldLeases := taskLeasesFile oldAsyncActivitySave := asyncActivitySave oldActivityPersistenceEnabled := activityPersistenceEnabled activityFilePath = filepath.Join(dir, "activity.json") completedTasksFile = filepath.Join(dir, "completed-tasks.json") failedTasksFile = filepath.Join(dir, "failed-tasks.json") noPRStreaksFile = filepath.Join(dir, "no-pr-streaks.json") + taskLeasesFile = filepath.Join(dir, "task-leases.json") asyncActivitySave = false activityPersistenceEnabled = false t.Cleanup(func() { activityFilePath, completedTasksFile, failedTasksFile, noPRStreaksFile = oldActivity, oldCompleted, oldFailed, oldNoPR + taskLeasesFile = oldLeases asyncActivitySave = oldAsyncActivitySave activityPersistenceEnabled = oldActivityPersistenceEnabled }) diff --git a/src/pkg/dashboard/contribute_lease_restart_test.go b/src/pkg/dashboard/contribute_lease_restart_test.go new file mode 100644 index 000000000..7f0863f31 --- /dev/null +++ b/src/pkg/dashboard/contribute_lease_restart_test.go @@ -0,0 +1,422 @@ +package dashboard + +import ( + "encoding/json" + "log/slog" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "github.com/gorilla/websocket" +) + +// contribute_lease_restart_test.go covers kubestellar/hive#5681: a hub restart made +// every in-flight contributor task unresumable. +// +// Leases lived only in process memory. A restart — which self-upgrade rolls (#5391) +// make routine — emptied the registry while the relays carried on working, so every +// reconnecting relay failed lookupLease, was answered "no active lease for this +// task", had its agent interrupted mid-turn, and was handed the identical issue back +// seconds later. Observed 2026-09-02: 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. +// +// #4260's contribute_reconnect_resume_test.go pins the resume contract across a +// SOCKET drop and passes throughout, because that reconnect is to a live hub. These +// tests exercise the same contract across a PROCESS boundary, which had no coverage. + +// restartedHub returns a second hub built over the same on-disk state as the first, +// which is what a hub restart is: a new process, an empty connection table, and +// whatever the previous process persisted. covK2Hub reuses HIVE_CONTRIBUTORS_DIR once +// it is set, so the second call boots against the first call's files. +func restartedHub(t *testing.T) *ContributeWSHub { + t.Helper() + hub, _ := covK2Hub(t) + return hub +} + +// --- 1. The headline contract, end to end -------------------------------------- + +// TestLeaseRestart_ResumeSurvivesHubRestart is the incident itself, driven through +// the real protocol: assign a task, replace the hub with a freshly constructed one +// over the same /data (a restart), and let the relay reconnect and re-assert the task +// it never stopped working. It must be resumed — not revoked, and above all not +// handed the same issue back as a brand-new assignment, which is the frame that types +// a fresh prompt into a pane whose CLI is still mid-turn. +func TestLeaseRestart_ResumeSurvivesHubRestart(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("HIVE_CONTRIBUTORS_DIR", filepath.Join(tmpDir, "contributors")) + t.Setenv("HIVE_FEDERATION_REGISTRY_PATH", filepath.Join(tmpDir, "federation", "registry.json")) + // Unlike setupWSTest, ledger persistence stays ON: the whole point is that the + // lease reaches disk before the process ends. + redirectContributeWSDisk(t, filepath.Join(tmpDir, "ws-state")) + + s1 := NewServer(0, slog.Default()) + s1.registerContributeRoutes() + ts1 := httptest.NewServer(s1.mux) + defer ts1.Close() + s1.deps = &Dependencies{GHAppAuth: newSucceedingAppAuth(t, "ghs_restart_5681")} + s1.contributeHub.server = s1 + seedOneIssue(s1, 5617, "[v5] reviewer lane follow-ups") + + conn, reg := registerAndAuth(t, s1, ts1, "restart-resume-user") + conn.WriteJSON(WSMessage{Type: "ready", Seq: 1}) + assign := readMsg(t, conn) + if assign.Type != "task_assign" || assign.Number != 5617 { + t.Fatalf("expected task_assign for #5617, got type=%s number=%d", assign.Type, assign.Number) + } + if assign.TaskGen == 0 { + t.Fatalf("task_assign carried no task_gen — the relay would have nothing to echo") + } + onlyLeaseIdentity(t, s1.contributeHub) // the assignment reached the registry + + // The hub restarts for an upgrade. The relay's socket closes (code 1012 since + // #5390) and a brand-new process comes up over the same /data: new hub, empty + // connection table, leases read back from disk. + conn.Close() + ts1.Close() + + s2 := NewServer(0, slog.Default()) + s2.registerContributeRoutes() + ts2 := httptest.NewServer(s2.mux) + defer ts2.Close() + s2.deps = &Dependencies{GHAppAuth: newSucceedingAppAuth(t, "ghs_restart_5681")} + s2.contributeHub.server = s2 + seedOneIssue(s2, 5617, "[v5] reviewer lane follow-ups") + + // Errorf, not Fatalf: when this regresses, the protocol frames below are the + // evidence worth seeing — the revoke, and the re-offer of the same issue. + if len(s2.contributeHub.leases) == 0 { + t.Errorf("#5681: the restarted hub restored no leases — every in-flight task " + + "is unresumable and its agent will be interrupted mid-turn") + } + + // The relay reconnects one backoff later and re-asserts the task it is still + // working, carrying the task id and generation the PREVIOUS process issued. + conn2, _, err := websocket.DefaultDialer.Dial(wsURL(ts2), nil) + if err != nil { + t.Fatalf("reconnect dial: %v", err) + } + defer conn2.Close() + readMsg(t, conn2) // auth_challenge + conn2.WriteJSON(WSMessage{Type: "auth_response", RegistrationToken: reg["registration_token"], CLIBackend: "claude"}) + readMsg(t, conn2) // auth_ok + + conn2.WriteJSON(WSMessage{ + Type: "task_progress", Seq: 2, TaskID: assign.TaskID, TaskGen: assign.TaskGen, + Repo: assign.Repo, Number: assign.Number, Kind: "issue", Title: assign.Title, + Status: "working", + }) + + deadline := time.Now().Add(1500 * time.Millisecond) + for { + remaining := time.Until(deadline) + if remaining <= 0 { + break + } + conn2.SetReadDeadline(time.Now().Add(remaining)) + _, raw, rerr := conn2.ReadMessage() + if rerr != nil { + break + } + var m WSMessage + if json.Unmarshal(raw, &m) != nil { + continue + } + switch m.Type { + case "task_revoke": + t.Fatalf("#5681: the relay reconnecting after a hub restart was told %q — "+ + "this is the revoke that interrupts a working agent", m.Reason) + case "task_assign": + t.Fatalf("#5681: the hub re-offered %s#%d to the very relay it just took it "+ + "from — ownership was never in question, only the record of it", + m.Repo, m.Number) + } + } + + if !hubHoldsTask(s2.contributeHub, assign.TaskID) { + t.Fatalf("#5681: the restarted hub neither revoked nor holds the task — the " + + "relay is working an item the hub has no record of") + } +} + +// --- 2. What survives, and what deliberately does not --------------------------- + +// TestLeaseRestart_UnexpiredLeaseIsReAdoptable is the unit-level core: a lease +// recorded by one process is matched by lookupLease in the next, on the exact +// {identity, task_id, repo, number, generation} tuple C4 requires. +func TestLeaseRestart_UnexpiredLeaseIsReAdoptable(t *testing.T) { + hub1, _ := covK2Hub(t) + const identity = "c-restart-me" + hub1.recordLease(identity, "ct-live", "myorg/repo1", 5617, "contributor", 42, time.Now()) + + hub2 := restartedHub(t) + l := hub2.lookupLease(identity, "ct-live", "myorg/repo1", 5617, 42, time.Now()) + if l == nil { + t.Fatalf("#5681: a lease recorded before the restart was not re-adoptable after it") + } + if l.repo != "myorg/repo1" || l.number != 5617 || l.gen != 42 { + t.Fatalf("restored lease lost its match tuple: %+v", l) + } + + // C4 is untouched: the restored record is still matched EXACTLY, so a claim that + // differs in any field is still refused. + for name, got := range map[string]*taskLease{ + "wrong task": hub2.lookupLease(identity, "ct-other", "myorg/repo1", 5617, 42, time.Now()), + "wrong generation": hub2.lookupLease(identity, "ct-live", "myorg/repo1", 5617, 43, time.Now()), + "wrong repo": hub2.lookupLease(identity, "ct-live", "myorg/other", 5617, 42, time.Now()), + "wrong number": hub2.lookupLease(identity, "ct-live", "myorg/repo1", 9999, 42, time.Now()), + "other identity": hub2.lookupLease("c-someone-else", "ct-live", "myorg/repo1", 5617, 42, time.Now()), + "unversioned": hub2.lookupLease(identity, "ct-live", "myorg/repo1", 5617, 0, time.Now()), + } { + if got != nil { + t.Errorf("restart must not relax the C4 exact-match contract (%s was accepted)", name) + } + } +} + +// TestLeaseRestart_ExpiredLeaseIsNotRestored: persistence must not resurrect a task +// that was already past its re-adoption window. A stale file is not authority. +func TestLeaseRestart_ExpiredLeaseIsNotRestored(t *testing.T) { + hub1, _ := covK2Hub(t) + const identity = "c-stale" + hub1.recordLease(identity, "ct-stale", "myorg/repo1", 11, "contributor", 7, time.Now()) + // Rewind past the window and force a rewrite so the file itself holds the + // expired record (saveLeasesLocked skips expired leases, so write it directly). + hub1.leaseMu.Lock() + hub1.leases[identity].expiresAt = time.Now().Add(-time.Minute) + hub1.leaseMu.Unlock() + writeRawLeaseFile(t, []persistedLease{{ + Identity: identity, TaskID: "ct-stale", Repo: "myorg/repo1", Number: 11, + Tier: "contributor", Gen: 7, ExpiresAt: time.Now().Add(-time.Minute), + }}) + + hub2 := restartedHub(t) + if hub2.lookupLease(identity, "ct-stale", "myorg/repo1", 11, 7, time.Now()) != nil { + t.Fatalf("#5681: an already-expired lease was restored — a stale file must not " + + "resurrect a task that is no longer re-adoptable") + } +} + +// TestLeaseRestart_RevokedLeaseStaysRevoked: every release path revokes the lease, and +// that revoke has to reach disk. A revoke lost to the next restart would resurrect a +// task the hub had already released. +func TestLeaseRestart_RevokedLeaseStaysRevoked(t *testing.T) { + hub1, _ := covK2Hub(t) + const identity = "c-released" + hub1.recordLease(identity, "ct-done", "myorg/repo1", 21, "contributor", 8, time.Now()) + hub1.revokeLease(identity, "ct-done") + + hub2 := restartedHub(t) + if hub2.lookupLease(identity, "ct-done", "myorg/repo1", 21, 8, time.Now()) != nil { + t.Fatalf("#5681: a revoked lease came back after the restart — a released task " + + "must never be re-adoptable") + } +} + +// TestLeaseRestart_RenewedWindowSurvives is the #4260 half across a process boundary. +// A task that has been progressing for longer than leaseTTL has a window anchored on +// its LAST report; persisting only the assignment-time window would bring it back +// already expired — exactly the bug #4260 fixed in memory. +func TestLeaseRestart_RenewedWindowSurvives(t *testing.T) { + hub1, _ := covK2Hub(t) + const identity = "c-longrunner" + assigned := time.Now().Add(-(2 * leaseTTL)) + hub1.recordLease(identity, "ct-long", "myorg/repo1", 33, "contributor", 9, assigned) + // Still working: a progress report a moment ago carried the window forward. + hub1.renewLease(identity, "ct-long", time.Now()) + + hub2 := restartedHub(t) + if hub2.lookupLease(identity, "ct-long", "myorg/repo1", 33, 9, time.Now()) == nil { + t.Fatalf("#5681/#4260: the restart restored the ASSIGNMENT window rather than the "+ + "renewed one, so a task progressing for %v came back unresumable", 2*leaseTTL) + } +} + +// --- 3. The restored lease must not be double-assigned -------------------------- + +// TestLeaseRestart_RestoredLeaseHoldsItsIssue closes the other half of the contract. +// The double-assignment guard is built from LIVE connections, which is empty right +// after a restart — so without this, the item whose holder is about to resume could +// be handed to somebody else in the meantime, turning "lose the task" into two relays +// on one issue. +func TestLeaseRestart_RestoredLeaseHoldsItsIssue(t *testing.T) { + hub1, _ := covK2Hub(t) + hub1.recordLease("c-holder", "ct-held", "myorg/repo1", 10, "contributor", 12, time.Now()) + + hub2, s2 := covK2Hub(t) + seedTwoIssues(s2, 10, 20) + + other := &ContributorConnection{ + profile: &ContributorProfile{GitHubUsername: "newcomer", ContributorID: "c-newcomer", TrustTier: "contributor"}, + lastPong: time.Now(), + } + msg := hub2.selectTask(other) + if msg == nil || msg.Type != "task_assign" { + t.Fatalf("expected the newcomer to be assigned the free issue, got %+v", msg) + } + if msg.Number == 10 { + t.Fatalf("#5681: issue #10 was handed to a second contributor while its restored " + + "lease holder was still reconnecting — a real double assignment") + } + + // The HOLDER is never blocked by its own lease: asking for work is itself the + // statement that it is not holding that task any more. + holder := &ContributorConnection{ + profile: &ContributorProfile{GitHubUsername: "holder", ContributorID: "c-holder", TrustTier: "contributor"}, + lastPong: time.Now(), + } + if own := hub2.selectTask(holder); own == nil || own.Type != "task_assign" { + t.Fatalf("a contributor's own restored lease must not lock it out of work: %+v", own) + } +} + +// TestLeaseRestart_HoldLapsesAfterGrace pins that this is a restart measure, NOT a +// change to what a lease means in steady state. A dropped socket keeps its lease so +// the relay can resume (#4260) while its item merely cools down (#2356); honouring +// leases as holds for the full TTL would silently replace that hedge with a +// 30-minute park on every disconnect. +func TestLeaseRestart_HoldLapsesAfterGrace(t *testing.T) { + hub, _ := covK2Hub(t) + hub.recordLease("c-holder", "ct-held", "myorg/repo1", 10, "contributor", 12, time.Now()) + hub.leaseMu.Lock() + hub.leases["c-holder"].restored = true // as if loaded at boot + hub.leaseMu.Unlock() + + if len(hub.leasedIssueKeys("c-other", time.Now())) != 1 { + t.Fatalf("a restored lease must hold its item during the post-restart grace window") + } + past := hub.startedAt.Add(leaseHoldGraceAfterStart + time.Second) + if got := hub.leasedIssueKeys("c-other", past); len(got) != 0 { + t.Fatalf("after the grace window the live-connection guard is back in sole "+ + "charge; leases must contribute no holds, got %v", got) + } + + // A lease minted by THIS process is never a hold: its holder has a live + // connection, which the existing guard already covers. + hub.recordLease("c-fresh", "ct-fresh", "myorg/repo1", 77, "contributor", 13, time.Now()) + for key := range hub.leasedIssueKeys("c-other", time.Now()) { + if key == "myorg/repo1#77" { + t.Fatalf("a lease minted in this process must not act as a restart hold") + } + } +} + +// --- 4. The generation fence must not alias across the restart ------------------ + +// TestLeaseRestart_GenerationAdvancesPastRestoredLeases: taskGen is an in-memory +// counter that restarts at zero. Without raising it past every restored generation, a +// post-restart assignment would mint numbers that ALIAS the restored ones, and the +// #2568 Gate would accept a pre-restart straggler against a brand-new task. +func TestLeaseRestart_GenerationAdvancesPastRestoredLeases(t *testing.T) { + hub1, _ := covK2Hub(t) + hub1.recordLease("c-a", "ct-a", "myorg/repo1", 1, "contributor", 75, time.Now()) + + hub2 := restartedHub(t) + if got := hub2.nextTaskGen(); got <= 75 { + t.Fatalf("#5681/#2568: the first generation minted after the restart was %d, which "+ + "aliases the restored lease's generation 75 — a pre-restart straggler would "+ + "be accepted against a new task", got) + } +} + +// --- 5. Persistence hygiene ------------------------------------------------------ + +// TestLeaseRestart_FileIsOwnerOnly: the registry is the C4 authorization record a +// resume is matched against, so it is owner-only on both sides — unlike the sibling +// contributor ledgers, which are reports. +func TestLeaseRestart_FileIsOwnerOnly(t *testing.T) { + hub, _ := covK2Hub(t) + hub.recordLease("c-perm", "ct-perm", "myorg/repo1", 3, "contributor", 4, time.Now()) + + info, err := os.Stat(hub.taskLeasesPath()) + if err != nil { + t.Fatalf("lease registry was not written: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("lease registry mode = %04o, want 0600", perm) + } +} + +// TestLeaseRestart_MalformedFileStartsEmpty: an unreadable registry must degrade to +// "no leases" — the pre-fix behavior — rather than panic or block startup. +func TestLeaseRestart_MalformedFileStartsEmpty(t *testing.T) { + hub1, _ := covK2Hub(t) + hub1.recordLease("c-x", "ct-x", "myorg/repo1", 1, "contributor", 2, time.Now()) + if err := os.WriteFile(hub1.taskLeasesPath(), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } + + hub2 := restartedHub(t) + if n := len(hub2.leases); n != 0 { + t.Fatalf("a malformed registry must start empty, got %d leases", n) + } + if hub2.lookupLease("c-x", "ct-x", "myorg/repo1", 1, 2, time.Now()) != nil { + t.Fatalf("a malformed registry must grant nothing") + } +} + +// TestLeaseRestart_ExpiredLeasesArePruned: a relay that never comes back leaves a +// lease nothing ever looks up. cleanupLoop's prune keeps it from holding its issue — +// and from sitting in the registry — until the process ends. +func TestLeaseRestart_ExpiredLeasesArePruned(t *testing.T) { + hub, _ := covK2Hub(t) + hub.recordLease("c-gone", "ct-gone", "myorg/repo1", 4, "contributor", 5, time.Now()) + hub.leaseMu.Lock() + hub.leases["c-gone"].expiresAt = time.Now().Add(-time.Second) + hub.leaseMu.Unlock() + + if n := hub.pruneExpiredLeases(time.Now()); n != 1 { + t.Fatalf("pruned %d expired leases, want 1", n) + } + if n := len(hub.leases); n != 0 { + t.Fatalf("registry still holds %d leases after the prune", n) + } + // Idempotent: nothing left to drop. + if n := hub.pruneExpiredLeases(time.Now()); n != 0 { + t.Fatalf("prune dropped %d on a clean registry, want 0", n) + } +} + +// --- helpers ------------------------------------------------------------------- + +// writeRawLeaseFile puts an arbitrary record set on disk, so a test can present the +// next boot with a file saveLeasesLocked would never have produced. +func writeRawLeaseFile(t *testing.T, records []persistedLease) { + t.Helper() + data, err := json.Marshal(records) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(taskLeasesFile), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(taskLeasesFile, data, 0o600); err != nil { + t.Fatal(err) + } +} + +// seedTwoIssues puts two admissible issues in the status payload so a test can prove +// which one selectTask picked. +func seedTwoIssues(s *Server, a, b int) { + s.statusMu.Lock() + s.status = &StatusPayload{ + Repos: []FrontendRepo{{ + Name: "repo1", + Full: "myorg/repo1", + ActionableIssues: []any{ + map[string]any{ + "number": float64(a), "title": "Issue A", + "url": "https://github.com/myorg/repo1/issues/" + itoa(a), "author": "someone", + }, + map[string]any{ + "number": float64(b), "title": "Issue B", + "url": "https://github.com/myorg/repo1/issues/" + itoa(b), "author": "someone", + }, + }, + }}, + } + s.statusMu.Unlock() +} diff --git a/src/pkg/dashboard/contribute_ws.go b/src/pkg/dashboard/contribute_ws.go index ec8d65da6..4d4954c7a 100644 --- a/src/pkg/dashboard/contribute_ws.go +++ b/src/pkg/dashboard/contribute_ws.go @@ -504,6 +504,15 @@ type ContributeWSHub struct { completedTasksFile string failedTasksFile string noPRStreaksFile string + // taskLeasesFile is where the server-issued lease registry is persisted so it + // survives a hub restart (#5681). Overridable per hub for tests, like the + // sibling ledgers. + taskLeasesFile string + // startedAt is when this hub process came up. It bounds the window in which a + // lease restored from the previous process is honoured as a hold on its work + // item (#5681, leaseHoldGraceAfterStart). Written once at construction and only + // read afterwards, so it needs no lock. + startedAt time.Time noWorkVerdictsFile string asyncActivitySave bool persistActivity bool @@ -570,12 +579,27 @@ type ContributeWSHub struct { // by recordLease at assignment and cleared by revokeLease on every release path; a // resume that does not match an unexpired lease here is rejected outright. type taskLease struct { - identity string - taskID string - repo string - number int - tier string - gen uint64 + identity string + taskID string + repo string + number int + // key is the canonical, source-aware work-item identity (worksource.Ref.Key — + // the same spelling WSTaskAssign.identityKey produces). It is carried so the + // double-assignment guard in selectTask can tell which ITEM a lease holds + // without re-deriving it from repo/number, which is wrong for external work: + // Linear and Jira items deliberately carry Number == 0 and put their identity + // in Key (#4245), so every zero-numbered item in a repo would collide as + // "repo#0" (#5120). + key string + tier string + gen uint64 + // restored marks a lease loadLeases read from disk at startup rather than one + // recordLease minted in this process (#5681). It is deliberately NOT persisted: + // it means "issued by the PREVIOUS process, whose holder has not reconnected + // here yet", which is only ever true for the current boot. It is what lets the + // double-assignment guard hold an item for a relay the hub has not seen yet + // WITHOUT changing what a lease means in steady state. + restored bool expiresAt time.Time } @@ -608,9 +632,20 @@ const leaseTTL = wsTaskTimeout // reconstructed from client-supplied fields. Called from selectTask under the new // assignment's generation. func (h *ContributeWSHub) recordLease(identity, taskID, repo string, number int, tier string, gen uint64, now time.Time) { + h.recordLeaseForKey(identity, taskID, repo, number, "", tier, gen, now) +} + +// recordLeaseForKey is recordLease plus the assignment's canonical work-item key. +// selectTask calls this form with chosen.ref.Key() so an EXTERNAL item's lease +// carries its real identity; an empty key falls back to the repo#number spelling, +// which is exact for GitHub work and is what the plain recordLease form records. +func (h *ContributeWSHub) recordLeaseForKey(identity, taskID, repo string, number int, key, tier string, gen uint64, now time.Time) { if identity == "" || taskID == "" { return } + if key == "" { + key = worksource.Ref{Repo: repo, Number: number}.Key() + } h.leaseMu.Lock() if h.leases == nil { h.leases = make(map[string]*taskLease) @@ -620,10 +655,13 @@ func (h *ContributeWSHub) recordLease(identity, taskID, repo string, number int, taskID: taskID, repo: repo, number: number, + key: key, tier: tier, gen: gen, expiresAt: now.Add(leaseTTL), } + // #5681: a lease the hub issued must outlive the process that issued it. + h.saveLeasesLocked() h.leaseMu.Unlock() } @@ -648,6 +686,11 @@ func (h *ContributeWSHub) renewLease(identity, taskID string, now time.Time) { h.leaseMu.Lock() if l, ok := h.leases[identity]; ok && l.taskID == taskID { l.expiresAt = now.Add(leaseTTL) + // #5681: persist the EXTENDED window. Without this a restart would restore + // the window as it stood at assignment, so a task that had been progressing + // for longer than leaseTTL — the exact case #4260 fixed in memory — would + // come back already expired and could not be resumed. + h.saveLeasesLocked() } h.leaseMu.Unlock() } @@ -666,6 +709,10 @@ func (h *ContributeWSHub) revokeLease(identity, taskID string) { h.leaseMu.Lock() if l, ok := h.leases[identity]; ok && (taskID == "" || l.taskID == taskID) { delete(h.leases, identity) + // #5681: a revoke that did not reach disk would be undone by the next + // restart, resurrecting a released task. Persist it with the same urgency + // as the in-memory delete. + h.saveLeasesLocked() } h.leaseMu.Unlock() } @@ -695,6 +742,7 @@ func (h *ContributeWSHub) lookupLease(identity, taskID, repo string, number int, if now.After(l.expiresAt) { // Expired: drop it so it can never be re-adopted, and treat as no lease. delete(h.leases, identity) + h.saveLeasesLocked() return nil } if l.taskID != taskID || l.gen != clientGen { @@ -709,6 +757,248 @@ func (h *ContributeWSHub) lookupLease(identity, taskID, repo string, number int, return l } +// persistedLease is the on-disk form of a taskLease (#5681). +// +// It carries the lease and nothing else. There is no credential in it: the scoped +// GitHub token is minted per assignment and delivered separately (#2537), never +// stored here. Restoring a lease therefore grants exactly one thing — the ability +// to RE-ADOPT a task the hub already issued to that identity — and never the +// ability to obtain a fresh credential without passing selectTask's gates. +type persistedLease struct { + Identity string `json:"identity"` + TaskID string `json:"task_id"` + Repo string `json:"repo"` + Number int `json:"number"` + Key string `json:"key,omitempty"` + Tier string `json:"tier"` + Gen uint64 `json:"gen"` + ExpiresAt time.Time `json:"expires_at"` +} + +func (h *ContributeWSHub) taskLeasesPath() string { + if h != nil && h.taskLeasesFile != "" { + return h.taskLeasesFile + } + return taskLeasesFile +} + +// saveLeasesLocked writes the server-issued lease registry to disk (#5681). +// +// THE CALLER MUST HOLD leaseMu. The snapshot and the write happen under the same +// lock deliberately: if the snapshot were taken under the lock and the rename done +// outside it, two concurrent mutations could land their renames in the opposite +// order and leave the file describing an OLDER registry than the one in memory — +// and the whole point of the file is that it is what the next process boots from. +// The cost is negligible: the file holds one record per contributor identity +// (bounded by maxWSConnections) and every mutation site is low-frequency — +// assignment, release, and one task_progress per relay per PROGRESS_REPORT_INTERVAL_MS. +// +// Leases already past their expiry are skipped rather than written: a lease that +// can no longer be re-adopted must not be able to come back from disk. +func (h *ContributeWSHub) saveLeasesLocked() { + if h == nil || !h.persistTaskLedgers { + return + } + now := time.Now() + records := make([]persistedLease, 0, len(h.leases)) + for _, l := range h.leases { + if l == nil || l.expiresAt.IsZero() || now.After(l.expiresAt) { + continue + } + records = append(records, persistedLease{ + Identity: l.identity, + TaskID: l.taskID, + Repo: l.repo, + Number: l.number, + Key: l.key, + Tier: l.tier, + Gen: l.gen, + ExpiresAt: l.expiresAt, + }) + } + data, err := json.Marshal(records) + if err != nil { + h.logger.Warn("[contribute-ws] task leases marshal failed", "error", err) + return + } + path := h.taskLeasesPath() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + h.logger.Warn("[contribute-ws] task leases directory creation failed", "error", err) + return + } + tmpPath := path + ".tmp" + // 0600, unlike the sibling ledgers: this file is the C4 authorization record + // that lookupLease matches a resume against, so it is owner-only on both sides + // — nothing else on the host has any business reading which contributor holds + // which work item, and nothing else has any business writing it. + if err := os.WriteFile(tmpPath, data, 0o600); err != nil { + h.logger.Warn("[contribute-ws] task leases write failed", "error", err) + return + } + if err := os.Rename(tmpPath, path); err != nil { + h.logger.Warn("[contribute-ws] task leases rename failed", "error", err) + } +} + +// loadLeases restores the server-issued lease registry at hub startup (#5681). +// +// Leases lived only in process memory. A hub restart — which self-upgrade rolls +// (#5391) make routine rather than rare — erased every record of what the hub had +// assigned, while the relays carried on working: they hold one task at a time and +// re-assert it on reconnect (#4260). With the registry empty, EVERY in-flight +// resume failed lookupLease, was answered "no active lease for this task", and had +// its agent interrupted mid-turn — then was handed the identical issue back seconds +// later. Ownership was never in question; only the record of it. +// +// This does not weaken C4. The restored record is still one the SERVER issued and +// wrote itself; 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, and a lease whose +// expiry has passed is dropped rather than loaded — so a stale file cannot +// resurrect a task that is no longer re-adoptable. +func (h *ContributeWSHub) loadLeases() { + if h == nil || !h.persistTaskLedgers { + return + } + data, err := os.ReadFile(h.taskLeasesPath()) + if err != nil { + return + } + var records []persistedLease + if json.Unmarshal(data, &records) != nil { + h.logger.Warn("[contribute-ws] task leases file unreadable; starting with an empty registry") + return + } + now := time.Now() + var maxGen uint64 + restored := 0 + + h.leaseMu.Lock() + if h.leases == nil { + h.leases = make(map[string]*taskLease) + } + for _, rec := range records { + // gen == 0 could never be matched by lookupLease (it refuses clientGen 0), + // so such a record is unusable; drop it rather than hold an issue hostage. + if rec.Identity == "" || rec.TaskID == "" || rec.Gen == 0 { + continue + } + if rec.ExpiresAt.IsZero() || now.After(rec.ExpiresAt) { + continue + } + key := rec.Key + if key == "" { + key = worksource.Ref{Repo: rec.Repo, Number: rec.Number}.Key() + } + h.leases[rec.Identity] = &taskLease{ + identity: rec.Identity, + taskID: rec.TaskID, + repo: rec.Repo, + number: rec.Number, + key: key, + tier: rec.Tier, + gen: rec.Gen, + restored: true, + expiresAt: rec.ExpiresAt, + } + if rec.Gen > maxGen { + maxGen = rec.Gen + } + restored++ + } + h.leaseMu.Unlock() + + // #2568: taskGen is an in-memory counter that restarts at zero, so without this + // a post-restart assignment would mint generations that ALIAS the ones just + // restored — and the Gate (generationAccepted) would then accept a pre-restart + // straggler against a brand-new task that happened to draw the same number. + // Advancing the counter past every restored generation keeps what the hub + // issues strictly ahead of what it has already issued. + for { + cur := h.taskGen.Load() + if cur >= maxGen || h.taskGen.CompareAndSwap(cur, maxGen) { + break + } + } + + if restored > 0 { + h.logger.Info("[contribute-ws] restored task leases across restart", + "count", restored, "max_gen", maxGen) + } +} + +// pruneExpiredLeases drops leases that have aged out of their re-adoption window and +// rewrites the file when anything changed (#5681). lookupLease already drops an +// expired lease it happens to read, but a lease whose relay never comes back is +// never looked up: without this it would sit in the registry — and in the +// double-assignment guard below — until the process ended. Called from cleanupLoop +// alongside the other stale-state reaping. Returns how many were dropped. +func (h *ContributeWSHub) pruneExpiredLeases(now time.Time) int { + dropped := 0 + h.leaseMu.Lock() + for identity, l := range h.leases { + if l == nil || l.expiresAt.IsZero() || now.After(l.expiresAt) { + delete(h.leases, identity) + dropped++ + } + } + if dropped > 0 { + h.saveLeasesLocked() + } + h.leaseMu.Unlock() + return dropped +} + +// leaseHoldGraceAfterStart is how long after startup the hub treats a RESTORED +// lease as an active hold on its work item (#5681). +// +// It exists to cover exactly one window: the hub has just booted, it has restored +// the leases the previous process issued, but the relays holding them have not +// reconnected yet, so h.connections — the only thing the double-assignment guard +// used to consult — is empty. Since those relays WILL resume (that is the whole +// point of persisting the lease), handing the same item to somebody else during +// those seconds would convert the old "lose the task" bug into a real double +// assignment. +// +// It is bounded well under leaseTTL on purpose. A lease is NOT a hold in steady +// state: a relay whose socket drops keeps its lease so it can resume (#4260), while +// its item is left merely cooling down (#2356's speculative release hedge) rather +// than blocked. Honouring leases as holds for the full TTL would silently replace +// that hedge with a 30-minute park for every disconnect. The relay reconnects on a +// one-second backoff and re-asserts its task immediately, so two minutes is many +// times the window that actually needs covering, and after it the ordinary +// live-connection guard is back in sole charge. +const leaseHoldGraceAfterStart = 2 * time.Minute + +// leasedIssueKeys returns the canonical work-item keys that a RESTORED, unexpired +// lease is holding for some identity OTHER than exceptIdentity, during the brief +// post-restart grace window (#5681). Outside that window, or with nothing restored, +// it returns nothing and the guard behaves exactly as it did before. +// +// A lease belonging to the REQUESTER is deliberately never an exclusion: asking for +// work is itself the statement that it is not holding that task any more, and the +// assignment replaces its lease. +func (h *ContributeWSHub) leasedIssueKeys(exceptIdentity string, now time.Time) map[string]bool { + keys := make(map[string]bool) + if h == nil || h.startedAt.IsZero() || now.Sub(h.startedAt) > leaseHoldGraceAfterStart { + return keys + } + h.leaseMu.Lock() + defer h.leaseMu.Unlock() + for identity, l := range h.leases { + if l == nil || !l.restored || identity == exceptIdentity { + continue + } + if l.expiresAt.IsZero() || now.After(l.expiresAt) { + continue + } + if l.key != "" { + keys[l.key] = true + } + } + return keys +} + // rateLimitHourWindow and rateLimitDayWindow are the trailing (rolling) windows // over which tier_limits.max_per_hour and max_per_day are counted (#2566). They // are sliding windows anchored on "now", not calendar buckets: a contributor's @@ -839,6 +1129,8 @@ func NewContributeWSHub(logger *slog.Logger, server *Server) *ContributeWSHub { completedTasksFile: completedTasksFile, failedTasksFile: failedTasksFile, noPRStreaksFile: noPRStreaksFile, + taskLeasesFile: taskLeasesFile, + startedAt: time.Now(), noWorkVerdictsFile: noWorkVerdictsPath(), asyncActivitySave: asyncActivitySave, persistActivity: activityPersistenceEnabled, @@ -855,6 +1147,10 @@ func NewContributeWSHub(logger *slog.Logger, server *Server) *ContributeWSHub { hub.loadNoPRStreaks() hub.loadNoWorkVerdicts() hub.loadActivity() + // #5681: restore the leases the PREVIOUS process issued before any relay can + // reconnect, so an in-flight task survives the restart instead of being revoked + // out from under a working agent. + hub.loadLeases() go hub.cleanupLoop() return hub } @@ -1006,6 +1302,11 @@ var failedTasksFile = "/data/contributors/failed-tasks.json" var noPRStreaksFile = "/data/contributors/no-pr-streaks.json" +// taskLeasesFile is the durable home of the server-issued task-lease registry +// (#5681). It sits beside the other contributor ledgers, but is written 0600: it is +// the C4 authorization record a resume is matched against, not a report. +var taskLeasesFile = "/data/contributors/task-leases.json" + // noPRStreakRecord is the in-memory and on-disk shape of one no-PR completion // streak (#3980). LastAt is the most recent no-PR completion; the streak is // discarded once it is older than noPRStreakResetAfter — enforced lazily on @@ -4228,6 +4529,12 @@ func (h *ContributeWSHub) cleanupLoop() { // through the SAME cooldown+generation-bump path a manual requeue uses. h.reclaimExpiredLeases(time.Now()) + // #5681: drop leases that aged out without ever being looked up — a relay + // that never came back after a restart leaves one behind, and it would + // otherwise keep its issue out of the assignment pool until the process + // ended. + h.pruneExpiredLeases(time.Now()) + // Deregister under the lock; CLOSE outside it. // // closeWithReason writes a Close frame with a deadline, so it can block for @@ -4899,6 +5206,16 @@ func (h *ContributeWSHub) selectTask(c *ContributorConnection) *WSMessage { } h.mu.RUnlock() + // #5681: for the first leaseHoldGraceAfterStart of this process, also exclude + // items held by a lease RESTORED from the previous one. Those relays have not + // reconnected yet, so the live-connection scan above cannot see them — but they + // will resume, so offering their work to somebody else now would convert the + // old "lose the task" bug into a real double assignment. Outside that window + // this contributes nothing. + for key := range h.leasedIssueKeys(identityOf(c), time.Now()) { + activeIssues[key] = true + } + // #2436 finding 3 / #2566: enforce tier_limits per identity. The config ships // populated MaxConcurrent/MaxPerHour/MaxPerDay defaults, so an operator // reasonably believes concurrency AND rate are capped — and since #2562 the @@ -5404,7 +5721,11 @@ func (h *ContributeWSHub) selectTask(c *ContributorConnection) *WSMessage { // reconnect can be validated against what the hub actually issued — the exact // {task, repo, generation, tier} bound here — instead of reconstructing ownership // from client-supplied task_progress fields. Revoked on every release path. - h.recordLease(identityOf(c), taskID, chosen.repoFull, chosen.number, c.profile.TrustTier, gen, time.Now()) + // #5681: record the item's canonical key too, so the double-assignment guard can + // recognise the lease after a restart — including for external work, whose + // identity is Key rather than repo#number (#4245). + h.recordLeaseForKey(identityOf(c), taskID, chosen.repoFull, chosen.number, + chosen.ref.Key(), c.profile.TrustTier, gen, time.Now()) // #2566: record this assignment against the identity's rolling hourly/daily // windows so the next selectTask enforces tier_limits.max_per_hour / From bb3812a128512e35a53f1cfc9e481787beebe983 Mon Sep 17 00:00:00 2001 From: Doug Baggett Date: Wed, 2 Sep 2026 11:26:53 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=93=9D=20docs:=20record=20that=20cont?= =?UTF-8?q?ributor=20task=20leases=20now=20survive=20a=20hub=20restart?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. - agent-state-inventory.md: rows 31 (task leases) and 32 (taskGen) move from volatile to fixed. Residual 2 warned that persisting one without the other would silently break the generation fence — that warning was load-bearing and is now marked resolved, with the coupling stated in loadLeases and pinned by TestLeaseRestart_GenerationAdvancesPastRestoredLeases rather than carried only by that document. - CHANGELOG.md: user-visible — in-flight contributor work now survives an upgrade roll. Refs #5681 Signed-off-by: Doug Baggett --- CHANGELOG.md | 1 + src/docs/contributor-relay.md | 4 ++++ src/docs/design/agent-state-inventory.md | 17 ++++++++++------- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e397c003c..36817703e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Hive did not historically maintain a complete changelog. This file starts a prag ### Fixed - The reviewer lane's escalated-PR work list is now ordered by each PR's real creation time instead of a PR-number proxy ([#5617](https://github.com/kubestellar/hive/issues/5617)). The list is capped at three PRs per kick and documented "oldest first", but its rows carried no age signal at all — `ci-failing.json` recorded no creation time, so ordering fell back to (repo name, PR number). PR numbers are monotonic only *within* a repo, so that proxy sorted by repo **name** first, and against the per-kick cap the result was starvation rather than mere cosmetic disorder: on a multi-repo hive, a month-old escalated PR in a late-alphabet repo sat behind newer ones from an early-alphabet repo on every kick, indefinitely. The creation time was already fetched during PR enumeration and simply never threaded into the work-list artifact; it now is, and each row renders an `opened:` line so the reviewer can confirm the ordering without running `gh pr list` (which the kick's invariants forbid). Rows from an older hub that recorded no creation time keep the previous (repo, number) proxy among themselves and sort after every row whose age is known, so a stale artifact degrades to the old behavior rather than being reshuffled. +- A hub restart no longer throws away every in-flight contributor task ([#5681](https://github.com/kubestellar/hive/issues/5681)). A contributor 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**, which is deliberate — a client must never be able to assert ownership of work the server did not assign. But leases lived only in the hub's memory, so a restart emptied the registry and *no* in-flight resume could match: the relay was told `no active lease for this task`, its agent was interrupted mid-turn, and the identical issue was handed straight back as a fresh assignment seconds later. Observed 2026-09-02: revoked at 14:24:40, reassigned to the same relay at 14:24:44, discarding two and a half minutes of a turn that was progressing normally. Self-upgrade rolls made this routine rather than rare, and it hit every contributor holding a task at the moment of any restart. The lease registry is now persisted to `/data/contributors/task-leases.json` (owner-only, beside the existing contributor ledgers) on every assignment, renewal and release, and restored at startup. Nothing about who may claim what is loosened: the restored record is one the hub itself wrote, a resume is still matched exactly on identity, task id, repo, number and generation, and a lease already past its window is dropped at load rather than restored. Two couplings move with it — the renewed window is what gets persisted (so a task that has been progressing for longer than the 30-minute lease TTL does not come back already expired), and the in-memory assignment-generation counter is advanced past every restored lease at boot, without which a post-restart assignment could mint a fencing token that aliased a pre-restart one. For the first two minutes after a restart a restored lease also holds its work item, so an issue whose relay is still reconnecting is not offered to a second contributor in the meantime. - Hold-gated PRs can no longer silently carry unreviewed commits into a merge ([#5589](https://github.com/kubestellar/hive/issues/5589)). A PR sitting under a hold label could accumulate commits from other authors or agents (typically a worktree cut from a contaminated base), and once the hold lifted the merge lanes treated it like any other approved PR — the diff a human saw under the hold was not the diff that merged. The governor now snapshots each hold-gated PR's head SHA plus commit/author sets into a PVC ledger (`/data/metrics/hold-guard.json`) and compares at lift time: an unchanged head clears silently, while any drift keeps the PR out of `merge-eligible.json` and `ci-failing.json`, posts a one-time evidence comment naming the unreviewed commits and authors (plain text, never @-mentions), re-applies the `hold` label, and re-pins the snapshot to the drifted head so removing the re-applied hold after reading the evidence is the fresh approval. Both auto-merge sweeps additionally now respect hold and `do-not-merge` labels directly — the self-authored sweep previously listed PRs independently of the enumeration hold gate and would squash a hold-labelled App PR on green. from [#5480](https://github.com/kubestellar/hive/issues/5480): an App-bot comment review submitted through `hive-review` and attributed as `agent_pr_reviewed`, plus an advisory bead included in the advisory digest. `hive-review` now prints its asynchronous result path, and the kick requires a confirmed `ok` result plus the bead before removing `needs-human`, applying a terminal reviewer label, or closing; if either write fails, the PR remains in the human queue for a later retry instead of becoming silently adjudicated. - Version flips no longer trap hosted spokes with large, long-lived `/data` PVCs in a startup-probe death loop ([#5525](https://github.com/kubestellar/hive/issues/5525)). The v5 entrypoint synchronously ran recursive per-agent `chown` and shared-home `chmod` passes before the Go server could bind `:3002`; on an NFS/RWX volume with months of worktrees the walk exceeded the probe budget, kubelet killed the container with exit 137, and the next boot restarted the same walk from the beginning. Size-dependent permission repair now runs in a root background worker, so the dashboard and health endpoint start independently of PVC size. Protected completion markers bind each finished pass to the ownership-schema revision and target UID, and the agent manager waits for both the shared-home and per-agent markers before touching that agent's tree, preserving UID isolation without putting the server behind the migration. Completed steady-state boots skip the recursive pass, while a changed UID invalidates its marker and repairs only the affected agent before launch. diff --git a/src/docs/contributor-relay.md b/src/docs/contributor-relay.md index 078fb67e2..55c78d22f 100644 --- a/src/docs/contributor-relay.md +++ b/src/docs/contributor-relay.md @@ -408,6 +408,10 @@ Task prompt sent to CLI That last line types a fresh prompt into a pane whose CLI is still mid-turn, interrupting it. Renewing the lease on every progress report keeps the two clocks together: a task the hub still considers alive is a task the relay can still resume. +**The lease has to outlive the hub process.** Leases used to live only in the hub's memory, so a restart — which self-upgrade rolls ([#5391](https://github.com/kubestellar/hive/issues/5391)) make routine rather than rare — emptied the registry while every relay carried on working. After the roll *no* in-flight resume could match, and each one produced the same four lines above, this time for a reason the relay could do nothing about ([#5681](https://github.com/kubestellar/hive/issues/5681)). Observed 2026-09-02: 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. The registry is now written to `/data/contributors/task-leases.json` (owner-only) on every assignment, renewal and release, and read back at startup, so a restart is just a longer-than-usual disconnect. + +This does not loosen who may claim what. The restored record is one the *hub itself* wrote, and a resume is matched against it exactly as before; a lease whose window has already passed is dropped at startup rather than restored. For the first two minutes after a restart the hub also treats a restored lease as a hold on its work item, so an issue whose relay is still reconnecting is not handed to somebody else in the meantime. + A resume that is genuinely refused — an operator yanked the task, or the relay stopped reporting for longer than the lease window — still ends in `task_revoke`, and that is correct. The relay clears its task and asks for new work. **A dropped socket is not a failed issue.** The disconnect books a short cooldown on the issue so a second session cannot pick it up during the reconnect window and file a duplicate PR ([#2356](https://github.com/kubestellar/hive/issues/2356)). That cooldown no longer counts toward the consecutive-failure quarantine: three drops on a flaky connection used to park a perfectly workable issue for six hours with nothing having actually failed. Real failures — `task_failed`, the relay's own progress watchdog giving up, the wedged-task backstop — still count, and still quarantine. diff --git a/src/docs/design/agent-state-inventory.md b/src/docs/design/agent-state-inventory.md index 35b19143e..3303a1866 100644 --- a/src/docs/design/agent-state-inventory.md +++ b/src/docs/design/agent-state-inventory.md @@ -184,8 +184,8 @@ key generations (`/data/saas/hub-generations.json`). | 28 | Failure cooldowns + quarantine counters | `failedTasks` / `consecutiveFailures` (`:373`, `:381`), persisted to `/data/contributors/failed-tasks.json` | Survives (#2435 livelock fix, made durable with the ledger) | **fixed** | | 29 | No-PR completion streaks | `noPRStreaks` (`:394`), `/data/contributors/no-pr-streaks.json` | Survives (#3980; geometric backoff must outlive the completion entry it escalates) | **fixed** | | 30 | `no_work_needed` verdicts | `noWorkVerdicts` (`:416`), `/data/contributors/no-work-verdicts.json` | Survives (#3987/#3997) — "persisted in the same PVC-backed ledger dir as the cooldowns so a pod restart does not forget the verdict" | **fixed** | -| 31 | Task leases — the server-authoritative record of what was issued to whom | `leases` (`:467`), in-memory only | **All active leases void.** A reconnecting relay's `task_progress` can only re-adopt against an exact unexpired lease (the C4 fix), so after a roll no in-flight contributor task can resume — the relay is asked to re-`ready` and the task is re-dispatched from scratch. Deliberate security posture (never rebuild ownership from client-supplied fields); the durability cost is accepted, not accidental | **volatile** (by security choice) | -| 32 | Assignment generation counter (`taskGen`) — fencing tokens | `:341`, `atomic.Uint64`, in-memory | Restarts at zero. Safe *only because* row 31 also dies: re-adoption requires a matching lease, so a stale pre-roll generation can never coincide with a live post-roll lease. The fencing guarantee is load-bearing across two volatile structures | **volatile** (correct today, fragile coupling) | +| 31 | Task leases — the server-authoritative record of what was issued to whom | `leases`, persisted to `/data/contributors/task-leases.json` (0600) | Survives ([#5681](https://github.com/kubestellar/hive/issues/5681)). Previously **all active leases went void**: a reconnecting relay could only re-adopt against an exact unexpired lease (the C4 fix), so after a roll no in-flight contributor task could resume — the agent was interrupted mid-turn and handed the identical issue back seconds later. The durability cost was accepted as a security posture, but the posture never required volatility: the restored record is one the *server* wrote, matched exactly as before, so nothing is rebuilt from client-supplied fields. Expired records are dropped at load rather than restored | **fixed** | +| 32 | Assignment generation counter (`taskGen`) — fencing tokens | `:341`, `atomic.Uint64`, in-memory; high-water mark re-derived from row 31 at boot | Restarts at zero, then `loadLeases` advances it past every restored lease's generation ([#5681](https://github.com/kubestellar/hive/issues/5681)). This is what residual 2 below warned about: making row 31 durable without this would let a post-roll assignment mint a generation that ALIASES a restored one, and the #2568 Gate would accept a pre-roll straggler against a brand-new task. The fence no longer depends on two structures dying together | **fixed** (re-derived, not persisted) | | 33 | Per-tier rate-limit ledger (`assignmentTimes`, #2436/#2566) | `:435`, in-memory only | **Every contributor's rolling per-hour/per-day assignment count resets to zero.** The tier limits an operator set are silently un-enforced for up to a day's window after each roll — same "admin-visible number is inert" class that #2566 fixed, reintroduced at roll frequency | **volatile** | | 34 | Live connections + `currentTask` + pending-auth counter | `connections` / `ContributorConnection.currentTask` (`:73`), `pendingConns` | Connection-scoped by nature; relays reconnect with backoff | **benign** (given row 31) | | 35 | Activity feed (50 entries) + SSE fan-out registry | `activity` (`:351`), `contribute_sse.go` | Operations view starts empty | **benign** | @@ -301,11 +301,14 @@ Honest gaps in the current state of things, and in this inventory: cycle's worth of pause/override/LastKick mutations. Nobody has been burned hard enough to make it write-through; the RFC's journal, if it lands, should not inherit this cadence. -2. **Row 32's coupling is undocumented elsewhere.** The generation-fencing - guarantee holds because `taskGen` and `leases` share a process lifetime. - Any change that persists one without the other (e.g. "let's make leases - survive restarts") silently breaks the fence. This document is currently - the only place that states it. +2. ~~**Row 32's coupling is undocumented elsewhere.**~~ **Resolved by + [#5681](https://github.com/kubestellar/hive/issues/5681).** The warning was + accurate and it was load-bearing: leases were made to survive restarts, and + the fence would have broken silently had `taskGen` not been re-derived from + the restored leases in the same change. The coupling is now stated in + `loadLeases` itself and pinned by a test + (`TestLeaseRestart_GenerationAdvancesPastRestoredLeases`), so it is no longer + carried only by this document. 3. **Rate limits reset on every roll** (row 33). Known now; not yet an incident; cheap to fix inside the existing ledger dir. 4. **Webhook loss** (row 3) has no re-derivation path. It predates this