refactor(refid)!: rename SequenceStore to Store, replace gorm postgres backend with raw SQL

[sthanikan2000]

Summary

• Renames the storage interface from SequenceStore to Store — a generic name for what has always been a backend-agnostic interface. Next(ctx, scopeKey, max) (int64, error) is unchanged: one atomic call, no reservation/commit split.
• Replaces the gorm-based postgres_store.go with github.com/OpenNSW/core/refid/postgres, using database/sql + the pgx stdlib driver directly (no ORM). Next is still a single atomic INSERT ... ON CONFLICT DO UPDATE ... RETURNING counter statement — the same shape the gorm version already used, ported as-is, not redesigned.
• Fixes an independent bug: NewPostgresStore used to panic on an invalid table name while AutoMigrate returned an error for the same input. New now returns an error (never panics) for both, matching every other constructor in this repo.
• Adds refid/internal/sqlident, a shared table-name validator.
• go.mod drops gorm.io/gorm/gorm.io/driver/postgres entirely and promotes github.com/jackc/pgx/v5 (previously indirect, via gorm) to direct.

Why, and why this PR looks different from its earlier version

A reviewer flagged that two concurrent Generate calls could mint duplicate IDs. The fix I first pushed here grew into a full deferred-commit redesign (Reserve/Reservation/Commit/Rollback, pessimistic row-locking, a shared state machine, deadlock hardening across both backends) to let a caller defer consuming a counter until its own downstream save succeeded.

That solved a problem that isn't actually refid's to solve. refid's job is to atomically hand back the next counter value with no race conditions — persisting it correctly is the downstream service's problem. Once the design goes back to one atomic call (this PR), the duplicate-ID bug is gone structurally, not mitigated: there's no window between reading a value and persisting it for two callers to act on the same one. The other two bugs the reviewer found (Commit marking itself resolved before its write succeeded; Generate leaking a reservation when a later segment failed) don't need fixing either, because there's no Reservation object left to have them.

Confirmed there are zero consumers of this package anywhere else in the monorepo, so the breaking rename is safe.

Test plan

• go build ./..., go vet ./..., golangci-lint run ./... all clean in refid/
• go test -race ./... passes in refid/
• Ran the postgres integration test (TestStore_Integration) against a real postgres:16-alpine container
• TestNew_InvalidTableName covers both New and Migrate returning errors (not panicking) for the same invalid names

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: 51159ae7-76b1-452a-81b3-d16306acf15a

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ginaxu1]

PTAL these issues:

1. Optimistic CAS plus "save the record, then Commit" breaks uniqueness
   Reserve does an unlocked SELECT, so two concurrent Generate calls for the same scope key receive the same candidate. The documented pattern then lets both callers persist that ID before either Commit runs. One Commit wins, the other returns ErrReservationConflict, but the duplicate row is already written. If the caller's ID column is unique, the loser retries Generate, still reads the uncommitted counter, and can loop on the same ID until the winner happens to Commit. A crash or error after save and before Commit is worse: the next Generate reissues an ID that already exists.

Suggested fix: pick one of these, not a hybrid.

• Pessimistic reservation (preserves both uniqueness and no-gaps): Reserve opens a transaction, SELECT ... FOR UPDATE (or INSERT the scope row then lock it), and holds that transaction on the Reservation. Commit writes the counter and commits the tx. Rollback rolls the tx back. Concurrent Generate for the same scope key waits instead of minting a duplicate. Document that the DB connection is held until the reservation is resolved, and keep that window small.
• Consume at issuance (preserves uniqueness, accepts gaps): atomically increment in Reserve the way Next used to (INSERT ... ON CONFLICT DO UPDATE SET counter = counter + 1 ... RETURNING), and make Reservation a no-op or a handle that cannot un-consume. Downstream save failure may gap a value; that is the correct tradeoff for a unique reference ID.
  Do not keep unlocked read + CAS after the caller has already stored the ID. ErrReservationConflict is not a valid recovery path once the ID has been persisted.

2. reservation.Commit marks resolved before the write succeeds (refid/postgres/store.go)
   resolved is set to true, then Exec runs. A transient DB error returns failure but leaves the reservation unretryable: a later Commit is a silent no-op, and a deferred Rollback does nothing. Combined with (1), the caller has a saved record and an unconsumed counter.

Suggested fix: set resolved only after a successful write. On ErrReservationConflict, mark it resolved (this reservation is dead; retry Generate). On any other error, leave it unresolved so Commit can be retried. Commit after Rollback must return an error, not nil. Rollback after a successful Commit stays a no-op. Guard resolved with a mutex so Commit and Rollback cannot race.

3. Generate leaks reservations when a later segment fails (refid/registry.go)
   The render loop returns immediately on the first render error and drops any Reservations already taken. For a format with two sequence segments, a failure on the second (overflow, cancelled context, DB error) never calls Rollback on the first. That is a lock/connection leak the moment a Store holds resources across Reserve (including the pessimistic design in (1), and today's memStore).

Suggested fix: accumulate reservations as they are returned; on render error, Rollback each one (errors.Join is fine), then return the original error. Add a test with two sequence segments where the second Reserve fails and assert the first counter is unconsumed.

4. Schema rename will break any existing refid_sequences table
   Migrate is CREATE TABLE IF NOT EXISTS, so an existing table keeps updated_at. The new upsert interpolates created_at, which is not on the old table, so Commit fails at runtime. There is no ALTER/migration, and the column rename is not needed for the CAS write.

Suggested fix: keep updated_at, set it on INSERT and on the CAS UPDATE, and do not introduce created_at. If you truly need a new column, ship a real migration, not CREATE TABLE IF NOT EXISTS

[sthanikan2000]

@ginaxu1 Thank you for your comment. I discussed the counter reservation functionality with @lokewate, and we felt that introducing it would add unnecessary complexity to the RefID package.

Also, the possibility of a stale counter value should be a very rare edge case, since the time window between calling Generate and persisting the data is expected to be very short. If this does occur downstream, we believe it should be handled by the downstream service.

One possible approach is to call Generate immediately before persisting the data in the downstream service. Alternatively, the downstream service could persist the data without a RefID initially and attempt to persist the RefID afterward with a retry mechanism if the first attempt fails.

---

Note: the scope of this PR has changed. I've reverted the Reserve/Reservation/Commit/Rollback design entirely and gone back to a single atomic Next(ctx, scopeKey, max) (int64, error) call — the same contract SequenceStore already had, just renamed to Store. This also resolves your other two points (the resolved state being set before the write is confirmed, and Generate leaking a reservation when a later segment fails) as a side effect, since there's no Reservation object left to have those bugs. Point 4 is moot as well: we kept updated_at rather than renaming it to created_at, since there's no compare-and-swap predicate anymore that would need a column to be left untouched.

The PR now covers:

• Renaming SequenceStore to Store (same single-method contract, just a more generic name)
• Porting the Postgres backend from gorm to raw SQL (database/sql + pgx) — the same atomic upsert-and-increment statement as before, just without the ORM
• An independent fix: NewPostgresStore used to panic on an invalid table name while AutoMigrate returned an error for the same input; New now returns an error for both, matching every other constructor in this repo

[ginaxu1]

Lgtm