Repository navigation
🐛 fix: preserve write counters on read errors - #14532
hivecommons-hive[bot] merged 1 commit into
Conversation
Hive-Run: hivecommons#14531 Hive-Plan: preserve durable write counters Hive-Spec: write-counters#read-error-recovery Signed-off-by: Karibhou <157208370+Karibhou@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
There was a problem hiding this comment.
Read src/crux/proxy/write_counters_store.go at 397aa2e and its callers. Looks correct to me.
mergeIntoFile(lines 222-229) now only resets tonewDurableWriteCounts()whenerrors.Is(err, errInvalidWriteCounts); any other read error returns before the write, so the durable file is never replaced on a transient I/O failure.flush(lines 206-218) already re-mergespendingintoc.pendingon any error, and both callers (StartWriteCounterPersistence, lines 273-290) just log and retry on the next tick / final flush — so the returned read error turns into a retry, as the body says.MigrateWriteCounters(lines 162-176) inherits the change: an unreadable target file now leaves the legacy file in place instead of overwriting the target, which matches the function's documented intent.%w: %wis fine on this module's Go version;errors.Is(err, syscall.ELOOP)unwraps through*fs.PathError, and the self-referencing symlink gives a deterministic ELOOP even as root, so the new test is not flaky by construction.
CI is green; tide is the only pending context.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me. Read src/crux/proxy/write_counters_store.go at the v5 merge-base alongside the diff.
What I verified
flush(write_counters_store.go:204-218at base) already re-mergespendingintoc.pendingwhenevermergeIntoFilereturns an error, so returning the read error frommergeIntoFile(new:226-228) is sufficient to keep the pending counts for the next minute's retry — no change toflushis needed.LoadDurableWriteCountskeeps theos.ErrNotExist→ empty-result path untouched, so a first-ever flush (no file yet) still creates the file; only EACCES/EIO/ELOOP-class read failures now propagate.- The sentinel
errInvalidWriteCountsis only attached on thejson.Unmarshalbranch, so the "corrupt file → start over" recovery is preserved exactly as before; the updatedTestLoadDurableWriteCountsMissingAndCorruptpins that classification. MigrateWriteCounters(:155-170) already returned read errors from the legacy load; this change makes the target load symmetric, which matches its doc comment ("a legacy file that cannot be read is left in place").%w: %wdouble-wrapping is fine on this module (go 1.26).- Regression test: the self-referential symlink (
os.Symlink(filepath.Base(path), path)) gives a deterministicELOOPfromos.ReadFileeven as root; the assertedRecordedSinceof 2026-10-12 is the UTC Monday of the recorded week, consistent with the existingEarliestWeekfallback.
One non-blocking note for the maintainer: #14531 also names the crux mirror (hivecommons/crux proxy/write_counters_store.go). This PR fixes hive only, which is right for a Fixes #14531 scoped to this repo — just flagging that the mirror will need the same two-line change if it is not synced automatically.
CI was still running at review time; nothing had failed.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry. |
A transient error reading the durable write counters previously caused the next flush to replace the all-time totals with only pending counts. Preserve the file and return the read error so pending counts are retried; continue recovering from invalid JSON as before.
The regression exercises an unreadable path, verifies it is not replaced, and checks that a subsequent retry retains the original totals and recorded-since timestamp. The existing corrupt-file test also checks the parse-error classification.
Fixes #14531
Validation:
gofmt -landgit diff --checkpassed. Go tests are left to CI per AGENTS.md.— hive: backend=codex