[architect] refactor: crash-safe persist for pkg/convergence/mutation Ledger (unique temp + fsync) - #5625
Conversation
Ledger.persistLocked staged through the guessable fixed name path+".tmp" with no fsync of file or directory — the exact pattern pkg/beads removed in #4742 and pkg/turn's FileStore never had. For a store whose contract is that an epoch is never handed out that a restart could forget, the missing fsync makes the contract unkeepable, and the fixed temp name lets any second writer on the path destroy an in-flight commit. Adopt the shared idiom: unique CreateTemp name, chmod to ledgerFileMode, fsync temp, rename, fsync directory. Behavior-preserving; pinned by TestPersistUsesUniqueTempNameAndLeavesNoResidue. Fixes #5623 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sec-check <sec-check@hive.kubestellar.io>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
Changelog: this PR changes code but does not touch If it is user-visible — a feature, a fix an operator would notice, a This is a reminder, not a gate; it never blocks a merge. |
kubestellar-prow
Bot
added
the
size/L
clubanderson
left a comment
clubanderson
left a comment
There was a problem hiding this comment.
Verdict: approve. Reviewed against #5623, the surrounding pkg/convergence/mutation code, and the sibling persist paths (turn.FileStore.Persist, beads post-#4742).
Crash-safe persist is correct. The new persistLocked is structurally identical to turn.FileStore.Persist (store.go): unique os.CreateTemp name, keep-flag cleanup on every error path, chmod, write, fsync temp, close, rename, fsync directory. That closes both gaps named in #5623 — the guessable fixed path+".tmp" (the pre-#4742 beads failure) and the missing file/dir fsync that made the Acquire contract ("an epoch is never handed out that a restart could forget") unkeepable. Error ordering is right: keep flips only after a successful rename, so no path leaks a temp file or removes a committed ledger.
No mutex re-entrancy. persistLocked is pure I/O — it never takes l.mu and calls no locked Ledger methods; its only call sites (Acquire, transition) hold the lock exactly as before.
Tests assert the real invariant. The new test pins the unique-name property the hard way — a hostile pre-planted path+".tmp" must survive persist untouched — plus no-residue, reopen-survival, and exact mode. Torn-file behavior at the final path is impossible by construction (atomic rename), and the corrupt-file REFUSAL contract plus restart reconstruction are already pinned by TestLedger_CorruptFileRefuses / TestLedger_RestartReconstructs. The fsync calls themselves aren't unit-assertable without fault injection; matching the already-accepted turn/beads idiom line-for-line is the right substitute.
Two behavior deltas beyond durability, both acceptable:
- File mode is now exactly 0660 via
Chmod(umask-independent), whereos.WriteFile(0660)was umask-masked (typically 0640 in practice). This is the deliberate beads "widen explicitly" idiom and what #5623 recommends — flagging it so nobody mistakes a mode diff on disk for drift. - A new narrow window: if the directory open/sync fails after the rename, the entry is durable on disk while
Acquirerolls back memory and returns an error. Disk-ahead-of-memory is the fail-safe direction for a fencing ledger (a phantom hold self-fences via TTL reconciliation on reopen; a grant is never lost), andturn.FileStorehas the identical window.
Re #5555 / RFC #4002 convergence: this makes the future Phase-1 journal/store convergence easier, not harder — persistLocked is now a structural clone of turn.FileStore.Persist, ready to be extracted into one shared atomic-persist helper. The journal-side finding is correctly left to #5624; no overlap here.
Scope matches the claim exactly (one function + one test, package still inert), CI fully green.
|
LGTM label has been added. DetailsGit tree hash: d2581ab7dfdbca888e9763d7e18301c890f412a0 |
clubanderson
added
approved
|
[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 |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
…, dedupe, persistence The Lifecycle Timeline panel rendered only re-stamped enumeration noise: 4 of 6 stages had no producer, the 500-raw-event ring flooded every eval cycle (~14min of history under a claimed 6h window), and restarts wiped it. Finish the feature instead of removing it (#5656): Store (pkg/timeline): events are now keyed by repo#number+kind into JOURNEYS — one entry per work item with per-stage FirstAt/LastAt/Count/ agent/attrs and a derived Current stage (merged sticky-terminal; blocked holds until later progress). Re-enumeration refreshes a timestamp instead of appending. Ring is sized for journeys (500 journeys, LRU by LastAt) and persists to /data/lifecycle-timeline.json with the atomic-persist idiom (unique temp + fsync + rename, per the post-#5625 mutation ledger), throttled for non-terminal floods and immediate for pr_opened/merged/blocked; corrupt files are moved aside, never fatal. FleetHealth now reports CoveredMs — how much history actually backs the counts — alongside the requested window. Producers, all riding existing paths (no new polling, no new loops): - classified: the scheduler's classifier pass (BuildKickMessages / BuildAgentMessageFromLastActionable) records lane/tier/model at the moment lane routing decides them (Scheduler.SetLifecycleRecorder). - pr_opened: the pr-request watcher's typed PROpenedHook plus the attribution audit stream (agent_pr_created) — deduped by design. - merged: every pr_merged attribution audit entry — both automerge sweep paths and MergePR (dashboard queue + merge watcher) — via the audit sink bridge in cmd/hive/lifecyclewire.go. - blocked: the fix-loop escalation (needs-human) records the real blockage with fix attempts and failing checks, not just hook annotations. Refs are normalized to the short repo#number form the enumeration producer already uses, so all stages of one item share one journey. Dashboard: /api/lifecycle-timeline serves {journeys, fleet}; the ACMM filter now gates whole journeys. Panel B renders one row per item with stage chips (enum → class → kick → pr → merged/blocked), current stage colored by the existing kind→color mapping, and an honest coverage label from fleet.coveredMs. EnableLifecyclePersistence mirrors the session-persistence wiring. pkg/retro keeps working: applyTimeline reads the journey's kicked-stage Count (kick cardinality preserved) and stage attrs for PR refs/state; its existing Reconstruct expectations pass unchanged. Tests: store dedupe/transitions/eviction/persistence round-trip/honest coverage/race, producer wires (classified, pr_opened, merged, blocked via a real runEscalationSweep pass), journey API contract, and markup-pin tests for the new panel per the #5479 style. Fixes #5656 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
hivecommons#5625 idiom) The lease registry write used the older fixed-name tmp + rename idiom. This upgrades it to the hivecommons#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 hivecommons#5681 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
…c7d6bc) (#5737) The #5625-idiom port onto saveLeasesLocked (8c7d6bc, refs #5681) shipped with no test delta, leaving the function at 60.8%: the happy path was pinned by contribute_lease_restart_test.go but none of the new invariants were. Pins: - the registry lands 0600 (owner-only C4 authorization record) - a successful save leaves no CreateTemp leftovers beside the registry - a failed rename removes its unique temp file and leaves prior state alone - expired / zero-expiry leases are skipped at write time - nil hub and persistence-off are silent no-ops - save -> loadLeases round trip restores the lease and advances taskGen (#2568) Test-only change: src/pkg/dashboard/contribute_lease_persist_test.go. Signed-off-by: hive-quality <quality@hive.local> Co-authored-by: hive-quality <quality@hive.local> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Merge origin/v4 into v5. Conflict resolution policy: keep v5's structure (split files, seams, edge channel), adopt v4's new functionality. Highlights: - workflows: v4's owner-derived dual-publish image names + cross-org mirror, with v5's RELEASE_BRANCH=v5 / CHANNELS=edge - pkg/config: ported v4's pause ownership, kick-only channels + ValidateChannels, cadence owners, budget floor warnings, curator opt-in into v5's split config files - pkg/agent: kept v5 terminal seam (removed v4 terminal.go); ported v4 thrash breaker, credential probe recovery, coverage preamble - pkg/dashboard: ported v4 branding (hivecommons#5752) into webstatic (InjectBranding + branded CSP index); kept v5 seams (SchedulerControl, deps.Watsonx, openrouter alias); v4 crash-safe lease persist (hivecommons#5625); union of capability consts - pkg/hub: ported v4's pollLatestSHAsTick extraction into saas_sha_poller.go - CHANGELOG: per-subsection union of Unreleased + v4 release sections Validation: go build ./..., go vet ./..., go test on all touched packages, contributor-relay JS tests 305/305. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Andy Anderson <andy@clubanderson.com>
1 participant
Refactor
Claims exactly:
src/pkg/convergence/mutation/ledger.go(persistLockedonly) + newledger_persist_test.go. No other files, no behavior change, package remains unwired/inert.Ledger.persistLockedstaged through the guessable fixed namepath+".tmp"viaos.WriteFileand renamed with no fsync of file or directory — the patternpkg/beadsremoved in #4742 and thatturn.FileStore.Persistnever had. The ledger's contract ("an epoch is never handed out that a restart could forget", ledger.go) is unkeepable without the fsync, and the fixed temp name lets a second writer on the same path destroy an in-flight commit. This is the §2.3 durability gap recorded insrc/docs/design/agent-turn-handoff.md(RFC #4002 step 3).Change: unique
os.CreateTempname, chmod toledgerFileMode(0660, preserving the outcome/proof/beads idiom), fsync temp, rename, fsync directory. Pinned byTestPersistUsesUniqueTempNameAndLeavesNoResidue: a hostile pre-plantedpath+".tmp"is untouched, no temp residue remains, the acquisition survives reopen, and mode is 0660.Validation:
go test ./pkg/convergence/mutation/ -count=1ok; gofmt/vet clean.Disjoint from open PRs #4032 (proxy auth) and #5559 (UPGRADE.md) — no file overlap. The companion journal-convergence finding is tracked separately in #5624 and is NOT attempted here.
Fixes #5623
Filed by architect agent (ACMM L5 — hold-gated mode). Hold-gated: human review required.
— hive: agent=architect backend=copilot model=claude-opus-4-6