fix(spurctld): retry accounting writes and reconcile drift from job store

[yansun1996]

What

Fixes SPUR-29: the accounting notifier fired Postgres writes fire-and-forget with no retry, so sacct and squeue would permanently diverge on any transient DB blip, with no visibility that it happened.

Approach

• Bounded retry (3 attempts, exponential backoff, per-attempt timeout) on notify_job_start/notify_job_end's DB writes, escalating to error! only after retries are exhausted.
• A periodic (120s) reconciliation pass that diffs in-memory job state against the accounting DB and re-issues writes for anything missing or stale, gated on a consensus-backed leadership check (ensure_linearizable, not just a locally-cached leader flag).
• The reconciliation diff logic accounts for: bare rows left by a record_job_end that ran without a preceding record_job_start, transient read failures during reconciliation (treated as "unknown, skip" rather than "confirmed missing," to avoid clobbering a correct record), and non-terminal states accounting doesn't model.
• Found via review, empirically verified against a real Postgres instance: the backfill-then-terminal-write sequence in resync_job was non-atomic — record_job_start's upsert unconditionally resets a row to state='RUNNING', end_time=NULL even when only backfilling missing metadata on an already-terminal record, and a failure partway through left that corrupted state in place for a full reconciliation cycle. Fixed by wrapping the backfill-then-terminal-write sequence in a single database transaction, so any failure rolls back cleanly instead of leaving a partially-applied, incorrect state.
• Added a timeout around the reconciliation transaction (mirroring the same pattern already used for notifier retries), so a single wedged (not fully down) Postgres connection can't silently stall the entire reconciliation loop forever.
• Fixed a Preempted-job misclassification in reconciliation's state comparison (was using is_terminal(), which excludes Preempted; switched to is_finalized(), matching the convention used elsewhere in the codebase) — closes a narrow but real gap where a durably-preempted job's correct accounting record could get clobbered every cycle.

Known limitations, documented not fixed

• Leadership can still change mid-reconciliation-pass for very large job backlogs (the consensus check only guards the start of a pass, not each individual write within it) — a real, disclosed residual risk that would need batch-size limits and periodic re-checking to close properly; out of scope for this pass.
• Two #[ignore]-gated DB tests in reconcile.rs share a test username derived only from process ID rather than a per-test discriminator (unlike the rest of the file's test-naming convention) — confirmed this causes no actual collision today (different job IDs, only one test touches the shared usage table) and these tests aren't part of the default CI run, so deferred rather than fixed in this already-substantial pass.

Testing

• New DB-backed tests (#[ignore]-gated, matching the existing convention) validated against a real Postgres instance, including a test that specifically reproduces the transaction-rollback fix: forces a failure mid-transaction and confirms the original terminal record is left untouched rather than corrupted.
• Pure-logic unit tests for retry/backoff, diff functions, and the is_finalized() state-comparison fix, with no DB dependency.
• Live-verified on the real cluster across 2 rounds, including actual Postgres outage fault injection confirming the job's accounting record is correctly and fully backfilled after recovery — not left showing RUNNING with no end_time.

Review

3 rounds of independent adversarial review (including GitHub Copilot's automated review, cross-validated — one round empirically verified the core bug against a live Postgres instance) + cross-validation passes, which found and required fixing real correctness bugs before this was mergeable — see commit history for details.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 25.82322% with 428 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   70.85%   70.32%   -0.53%     
==========================================
  Files         140      141       +1     
  Lines       41849    42382     +533     
==========================================
+ Hits        29650    29801     +151     
- Misses      12199    12581     +382     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Improves Spur controller accounting robustness by adding bounded retries to notifier writes and a periodic reconciliation loop that re-syncs accounting DB state from the in-memory job store, guarded by a consensus-backed leadership check.

Changes:

• Add per-notification bounded retry with exponential backoff + per-attempt timeout for accounting DB writes.
• Introduce a periodic reconciliation loop to detect and repair missing/stale accounting rows using job-store state, gated by ensure_linearizable()-backed leadership.
• Add DB batch query helper (job_accounting_states) plus unit/integration tests for reconciliation and retry behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/spurctld/src/raft.rs	Adds ensure_leader() (consensus-backed) alongside cached is_leader().
crates/spurctld/src/main.rs	Wires up the periodic accounting reconciliation loop on successful DB init.
crates/spurctld/src/accounting/reconcile.rs	New reconciliation loop + diff/backfill logic + tests.
crates/spurctld/src/accounting/notifier.rs	Adds retry/backoff + per-attempt timeout for DB write notifications + unit tests.
crates/spurctld/src/accounting/mod.rs	Exposes the reconciliation loop entrypoint from the accounting module.
crates/spurctld/src/accounting/db.rs	Adds AccountingRowState + job_accounting_states() batch query for reconciliation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[yansun1996]

Live testing performed

Verified on a real 2-node cluster (not simulated/mocked), across two rounds:

• Ran a normal job start-to-completion lifecycle and confirmed accounting output matched the live job state throughout.
• Fault-injected a database outage spanning a job's start/end transition; confirmed the write retries as expected, and after the next periodic reconciliation pass, the job's accounting record was fully and correctly backfilled — not left showing an inconsistent state.
• After a review round found (and a reviewer independently reproduced against a real database) a data-corruption path in the reconciliation backfill logic, re-verified with the same fault-injection scenario that the fix resolves it: the job's final accounting record is correct, not stuck showing an incorrect intermediate state.

[shiv-tyagi]

Found a few small nits (hardcoded 120s interval vs a config knob, a full job-store clone per cycle, an avoidable String alloc in accounting_expected_state), ignoring for now. LGTM.