fix(accounting): preserve unset fields on sacctmgr modify

[biluriuday]

Summary

sacctmgr modify <entity> set <field>=<val> reset every field the command
didn't mention back to its default instead of leaving it untouched — so
changing one attribute silently wiped the rest (SPUR-96). This makes
modify a true partial patch and hardens the accounting write path around it.

Approach

Partial-patch semantics, end to end:

• CLI (sacctmgr) sends only the fields the user restated, via proto3
  field presence; add still sets everything.
• Proto marks settable fields optional — unset means "leave unchanged",
  set means "write" (per-field clear-vs-store semantics documented inline).
• Controller maps that presence onto a dynamic INSERT ... ON CONFLICT DO UPDATE that touches only the provided columns and preserves the rest with
  COALESCE/CASE.

Also in this PR

• One default account per user: enforced with a partial unique index plus a
  dedup + column-drop backfill in migrate(), serialized by a
  transaction-scoped advisory lock. add_user demotes the user's other rows
  before the upsert. Drops the redundant, write-only associations.is_default
  so users.default_account is the single source of truth.
• QOS allow-list validation collapsed from N+1 lookups to one batch query.
• SQL-injection hardening: table and conflict targets are closed enums, so
  dynamically built SQL can only ever splice known literals.
• Fail-loud parsing: bad numeric fields error instead of silently
  defaulting; fairshare rejects fractional/out-of-range values instead of
  truncating; errors name the exact alias the user typed.
• Dead code: removed the unused spur-core::Association type.
• CI: a new test-db job runs the (normally #[ignore]d) accounting DB
  tests against an ephemeral, pinned, secret-free Postgres service.

Design notes

• Kept users.default_account denormalized (dropped the duplicate flag rather
  than normalizing into its own table) — removes drift with a minimal change.
• The migration rewrites data and rebuilds the index in a single transaction
  under a fixed advisory lock, so HA controllers can't apply it concurrently.

Testing

• cargo clippy (no warnings), cargo fmt --check, and cargo test.
• Accounting DB integration tests against a real Postgres: partial-patch
  preservation, default-account demotion, the migration/dedup upgrade path,
  concurrent migrate() serialization, and batch QOS validation.
• e2e accounting tests assert modify leaves unrelated fields intact.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 24.63394% with 875 lines in your changes missing coverage. Please review.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
- Coverage   73.66%   73.36%   -0.30%     
==========================================
  Files         164      164              
  Lines       56954    57984    +1030     
==========================================
+ Hits        41953    42535     +582     
- Misses      15001    15449     +448     

🚀 New features to boost your workflow:

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

[Copilot]

Pull request overview

This PR fixes SPUR-96 by making sacctmgr modify behave like a true partial patch end-to-end (CLI → proto presence → controller/db writes), so unspecified fields are preserved instead of being reset to defaults. It also tightens accounting write-path behavior (default-account uniqueness, batched QOS validation) and adds CI coverage for DB integration tests.

Changes:

• Switch account/user/QOS upserts to proto3 field presence + patch structs so only restated fields are written and others are preserved.
• Enforce “one default account per user” via schema migration + partial unique index + per-user serialization in add_user.
• Add DB integration tests to CI (ephemeral Postgres) and extend e2e coverage for partial-patch behavior.

Reviewed changes

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

Show a summary per file

File	Description
tests/native_host/e2e/test_accounting.py	Adds e2e assertions that modify preserves unstated fields and clears explicitly empty ones.
proto/slurm.proto	Marks sacctmgr-settable fields as optional to enable proto3 presence semantics for partial patches.
crates/spurctld/src/accounting/grpc.rs	Maps proto presence to nullable patch updates; improves validation (fairshare, limits) and batches QOS allow-list checks.
crates/spurctld/src/accounting/db.rs	Implements dynamic “update only provided columns” upserts; adds migration serialization + default-account uniqueness enforcement.
crates/spur-proto/build.rs	Ensures Cargo rebuilds generated proto code when .proto files change.
crates/spur-core/src/job.rs	Comment tweak referencing associations after Association type removal.
crates/spur-core/src/accounting.rs	Removes unused/dead Association type.
crates/spur-core/src/account_limits.rs	Comment tweak aligning wording with association semantics.
crates/spur-cli/src/sacctmgr.rs	Sends only restated fields for modify; improves parsing/alias error reporting for fail-loud behavior.
.github/workflows/ci.yml	Adds test-db job to run ignored accounting DB integration tests against a pinned Postgres service.

---

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

[yansun1996]

Nice fix — the partial-patch semantics read cleanly end to end, and the tri-state (unset preserves / empty-0 clears / value sets) is consistent across the CLI, gRPC, and DB layers. The migration dedup + partial unique index + advisory-lock serialization look correct, and the injection hardening via the closed UpsertTable enum is a good call.

Just a few small comments inline — all optional, nothing blocking.

[yansun1996]

Thanks for the thorough follow-up — all the earlier review comments look addressed: the conflicting account/defaultaccount case is now rejected before it can clear a default, add user no longer demotes an existing default when defaultaccount= is omitted, and the comments now state the invariants rather than the fix history. The partial-patch upsert, the one-default-per-user migration (transaction-scoped advisory lock, idempotent), and the dynamic-SQL hardening all read cleanly.

One small thing surfaced while reviewing: remove_user's two DELETEs aren't wrapped in a transaction, so they aren't atomic. That's already being handled in #451, so no action needed here — just flagging that both PRs rewrite remove_user, so whichever lands second will need a quick reconcile.

LGTM.