Repository navigation
fix(api): order same-instant audit events by a durable monotonic key, not a random Guid #508
Description
Split out of the #494 review (PR #503, codex round 3). Accepted there as a documented residual because the fix needs a migration, which #494 was specified not to do.
The defect
AuditEventRepository.GetProvenanceChunkAsync picks the last change with:
ORDER BY e."EntityId", e."OccurredAtUtc" DESC, e."Id" DESCAuditWriter mints Guid.NewGuid() — a random v4 — so "Id" carries no chronology. When two reportable changes for one entity share an OccurredAtUtc, the tiebreak resolves arbitrarily and the wrong actor can be named as the last changer. The timestamp displayed is correct either way; only the name is at risk.
ListAsync uses the same tiebreak. There it is merely arbitrary rather than wrong — paging only needs a deterministic order — but the Audit page can still display a same-instant pair in the wrong order.
Why it is reachable, not theoretical
Most concurrent changes to one record cannot collide: the aggregate Version concurrency token serialises them, and the loser gets a 409 having written nothing (audit shares the transaction).
RecordBirdMovementHandler escapes that argument. It writes an audit event keyed to the flocks id while only inserting a BirdMovement row — it never mutates Flock, so no Version bump, so it does not serialise against Flock.Update. Two such requests can land in the same microsecond against the same flock.
What is needed
A durable monotonic ordering value on AuditEvents — a bigint GENERATED ALWAYS AS IDENTITY or a sequence-backed column — used as the tiebreak in both queries.
Note the alternatives that do not work, so they are not re-proposed:
- UUIDv7 for new ids — only orders to millisecond granularity and keeps a random tail, so it does not resolve a microsecond collision; and existing rows are v4, so a mixed table orders meaninglessly.
ctid— physical location, not durable acrossVACUUM FULL.
Cost
A migration (#407: its own dotnet ef migrations add, never folded into InitialCreate), a docs/schema/ regeneration committed in the same PR (#417), and a sim-harness pass (#370).
Deliberately not done in #503
No test pins the current arbitrary outcome — pinning a known loss turns "did not fix" into "spec". The behaviour is recorded in a comment at the query and here.
Activity
docs: record the same-instant last-changer residual at the query that…
feat(api,web): show who created and last changed a record, inline on …
mforce commentedon Aug 11, 2026
Findings from reading the code after #503 merged (
4ffa7f17)Four things the issue body should carry before anyone picks this up.
1. Raising the timestamp's precision is a non-fix — add it to the list of rejected alternatives
OccurredAtUtcis alreadytimestamptz, which Postgres stores at microsecond precision, andAuditWriterreadsclock.UtcNowper call rather than once per unit of work. So the collision is not "the column is too coarse". Two writers can legitimately produce the same microsecond, and no number of extra digits creates an ordering between them — only a shared monotonic counter does. Belongs beside UUIDv7 andctidin What is needed, because "just store a finer timestamp" is the first thing anyone will reach for.2. There are FIVE tiebreak sites, not two
The body says "both queries".
AuditEventRepositoryactually has five, and a fix that updates fewer leaves the defect live in whatever it skips::33ListAsync(LINQ, the admin/auditviewer):96created.Createper entity:223creatorCTE:241:265promotedOnly two can actually resolve wrongly today, but all five should move to the new column: the three that cannot collide are not guaranteed not to, they are merely unreachable given today's actions, and leaving a random-Guid tiebreak behind re-creates the defect the moment that changes.
3. Prefer
GENERATED BY DEFAULT AS IDENTITYoverGENERATED ALWAYSThe body specifies
ALWAYS.BY DEFAULTis the better choice here:ALWAYSrejects an explicit value outright, which blocks any future backfill or data migration that needs to preserve an order it computed. Nothing in the app writes the column either way — EF marks itValueGeneratedOnAddand never sends it — soALWAYSbuys no real protection while costing that escape hatch.4. The migration rewrites the table — which argues for doing it now, not later
ALTER TABLE … ADD COLUMN … AS IDENTITYhas a sequence-backed default, so Postgres cannot use the fast constant-default path: it rewrites the whole table and takes anACCESS EXCLUSIVElock for the duration. Two consequences:AuditEventsis small and pre-1.0, and it only gets more expensive. If it does wait until the table is large, the shape becomes: add a nullablebigintwith a sequence default, backfill in batches ordered by(OccurredAtUtc, Id), thenSET NOT NULL— more steps, no rewrite lock.One caveat to state in the fix, not discover later
A sequence gives insert order, not commit order. Two concurrent transactions can take sequence values in one order and commit in the other. That is still strictly better than a random v4 and correct for every case the History column cares about — but the fix should not claim to establish a true total order for auditing purposes, or the next person will rely on something it does not provide.
Confirmed non-impacts
AuditEventsis not one of the exported datasets, so no export contract changes (per Daily entry: require grading to reconcile sellable eggs before submit #394, checked by reading the callers rather than by running).AuditVocabularyCoverageTests— keys on actions and entity types, untouched by a new column.docs(plans): correct the #508 blast-radius list
docs(plans): correct the #508 INV-1 and INV-3 statements
docs(plans): correct the #508 contract invariants to match the shippe…
mforce commentedon Sep 6, 2026
Fixed and merged in #700 — squash commit
8fcf084fb3889b78e9bee950f6e324d7e0f238dconmain.What shipped.
AuditEventsgained abigint GENERATED ALWAYS AS IDENTITYcolumn,Sequence, mapped as an EF shadow property so it reaches no API response, export or screen. It replacesIdas the tiebreak in five order clauses —ListAsync, andcreated,creator,latest,promotedinGetProvenanceChunkAsync. The issue only named two; a pre-dispatch review refuted the reasoning that the other three were harmless (their cardinality holds by the aggregate state machine, never by anything the audit layer enforces), so all five moved.The migration is hand-written, not the EF scaffold. The scaffolded one-liner backfills existing rows in physical order. For this append-only heap that is insert order today, but it is not a contract — a
VACUUM FULL,CLUSTERorpg_repackrewrite may reorder it. The shippedUp()adds the column nullable, backfills withrow_number() OVER (ORDER BY "OccurredAtUtc", "Id"), setsNOT NULL, attaches the identity, then advances the sequence. Verified against a real Postgres, including the empty-table case every Testcontainers run hits. It rewrites the table underACCESS EXCLUSIVE— so does the one-liner, since an identity column assigns per-row values either way — andmigrateruns as a pre-deploy job.Two regression tests, one per surface, both pinning their Guids so the old behaviour fails deterministically rather than about half the time. Reverting the fix with the tests kept reproduces the original symptom exactly:
Expected: "second@farm.test" / Actual: "first@farm.test".The alternatives this issue ruled out stayed ruled out — no UUIDv7, no
ctid, no change toAuditWriter's random id.Acceptance criteria the fix deliberately does NOT meet, so nobody assumes otherwise:
row_number()over a random Guid — deterministic and stable, but arbitrary, exactly as before. The guarantee holds for rows inserted after the migration.OccurredAtUtcis still the primary sort. A backwards clock step can give a later insert an earlier timestamp, andSequenceis then never consulted. Closing that means redefining "latest" as insert order for every pair, which is a larger behaviour change than this issue scoped. Recorded, not filed — the owner chose to leave it documented.Both are written up in
docs/plans/508-audit-monotonic-order/01-diagnosis.md, which ships with the change, along with the 38 siblingThenBy(x => x.Id)sites (none a wrong value today) and the export's missing ordering key (input for the unshipped legacy-import work).docs: record the #508 audit ordering key and the tracked-file guard l…
docs(plans): commit the #722 and #745 design records (#754)
chore(main): release 0.1.0 (#542)
docs(auth): record the OAuth 2.1 decision for MCP authentication (#801)
docs(plans): reword two lines the #508 image-pin guard read as a Dock…
perf(ci): start the serialized integration collection first (#861)
docs(plans): reword two lines the #508 image-pin guard read as a Dock…
docs(designs): MUI revamp design doc, component map, layout system, IA (