ReviewOS

stacks/bench-review

db: schema-integrity epic — missing UNIQUE constraints, FKs off at runtime, no hot-path indexes, counter drift, mixed timestamps

#52
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Problem

Bench publishes ratings and "official" verified-judge responses about named public figures, so duplicate accounts, inflatable helpful-counts, stackable reviews, and duplicate verified identities are trust failures, not cosmetic bugs. Eight verified gaps share one theme: invariants the code already assumes are never enforced in the SQLite schema or at runtime.

Findings

  • users.email has no UNIQUE — only a non-unique (email, name) index exists (database/migrations/0000000113-create-users_email_name_index-index-in-users.sql); register() is a non-atomic check-then-insert whose own comments promise a transaction + unique index that don't exist (storage/framework/core/auth/src/register.ts:33,41). Concurrent signups create duplicates; login then resolves an arbitrary row.
  • judge_reviews_likes has no UNIQUE(judge_review_id, user_id) — the pivot has zero indexes (database/migrations/0000000038-create-judge_reviews_likes-table.sql), so the likeable trait's idempotency catch is dead code (storage/framework/core/orm/src/traits/likeable.ts:62-85) and the isLiked→like race (app/Actions/Reviews/LikeReviewAction.ts:73-80) makes helpful-counts scriptable upward.
  • FK enforcement is OFF at runtime — configureSqlitePragmas() exists for exactly this (storage/framework/core/database/src/drivers/sqlite.ts:57-66) but is never called. Delete paths orphan children: admin delete removes only the review row (app/Actions/Admin/Reviews/DeleteReviewAction.ts:36); self-delete cleans 2 of 6 child tables (app/Actions/Me/DeleteMyReviewAction.ts:47-49). Orphaned photos keep serving for moderation-deleted reviews.
  • No one-review-per-user-per-judge — SubmitReviewAction inserts with no existing-review lookup (app/Actions/Reviews/SubmitReviewAction.ts:102-140) and no UNIQUE(user_id, judge_id); ratings are averaged live (app/Actions/HomeHighlightsAction.ts:46), so one account can stack 60 reviews/hour under the throttle (routes/api.ts:156-160) and skew a judge's score.
  • Two verified judges can hold the same profile — uniqueness is checked only at claim time (app/Actions/Me/ClaimJudgeProfileAction.ts:48-57); the approve branch never re-checks claimed_judge_id collisions (app/Actions/Admin/Credentials/VerifyCredentialAction.ts:43-72) and no partial unique index exists. Both holders can post "official" responses.
  • judge_reviews has no index on judge_id/status/user_id — only title/anonymized indexes exist (app/Models/JudgeReview.ts:10-23); judge profile (app/Actions/Reviews/ReviewsByJudgeAction.ts:48-57), public feed (app/Actions/Reviews/LatestReviewsAction.ts:30-31), and my-reviews are full scans on the fastest-growing table.
  • comments counter drifts — SELECT-then-SET increment/decrement (app/Actions/Reviews/CommentSubmitAction.ts:66-103, app/Actions/Me/DeleteMyCommentAction.ts:55-60); DeleteAccountAction bulk-deletes comments without reconciling; the seeder writes comments: 5 with zero rows (database/seeders/ReviewSeeder.ts:210) — phantom engagement on cards.
  • Mixed created_at formats — DEFAULT CURRENT_TIMESTAMP ('YYYY-MM-DD HH:MM:SS') and toISOString() rows sort lexicographically in the same TEXT columns; within a day every ISO row outranks every default row. Live data already mixes (15 vs 1 in judge_reviews), in the exact column the feed orders by (app/Actions/Reviews/LatestReviewsAction.ts:30-31).

Root causes for the email UNIQUE, likes UNIQUE, and FK pragma are framework bugs filed separately against stacksjs/stacks — but bench needs its own .sql migrations and a boot-time pragma call regardless.

Acceptance criteria

  • Dedupe existing duplicate users.email rows, then a .sql migration adds CREATE UNIQUE INDEX users_email_unique ON users(email); register's 409 path also maps the constraint error.
  • Dedupe likes pairs (keep MIN(id)), then CREATE UNIQUE INDEX judge_reviews_likes_user_review_unique ON judge_reviews_likes(judge_review_id, user_id).
  • configureSqlitePragmas() is called on API server boot (foreign_keys=ON, WAL, busy_timeout).
  • A shared deleteReviewCascade(reviewId) covers review_comments, review_photos, review_flags, judge_responses, judge_reviews_likes, user_notifications and is used by DeleteReviewAction, DeleteMyReviewAction, and DeleteAccountAction.
  • SubmitReviewAction returns 409 for an existing non-rejected review by the same user for the same judge, backed by a partial unique index ON judge_reviews(user_id, judge_id) WHERE status != 'rejected' AND user_id IS NOT NULL.
  • VerifyCredentialAction's approve branch re-checks for an existing verified holder of the same claimed_judge_id (409 on conflict), backed by CREATE UNIQUE INDEX users_verified_judge_claim_unique ON users(claimed_judge_id) WHERE credential_verified_at IS NOT NULL AND credential_type = 'judge'.
  • Index migrations for judge_reviews: (judge_id, status, created_at), (status, created_at), and (user_id).
  • Comment counts are derived at read time (likes pattern, app/Helpers/reviewLikes.ts) or every mutation sets the column via subquery COUNT(*); the seeder no longer writes phantom counts.
  • One-time migration normalizes existing created_at/updated_at values to ISO-8601; inserts set created_at explicitly (or new defaults use strftime('%Y-%m-%dT%H:%M:%fZ','now')).

From the 2026-06-10 production-readiness audit.

Upstream (framework root causes):

9 days ago

Partial progress, and the lead finding in this issue is now stale.

"users.email has no UNIQUE — only a non-unique (email, name) index exists" — no longer true. The live schema has it:

CREATE UNIQUE INDEX "users_email_unique" ON "users" ("email")

alongside users_verified_judge_claim_unique (partial, on claimed_judge_id where the credential is a verified judge). Worth re-reading the rest of the description against the current schema before working it — at least one other bullet may have aged out the same way.

Missing UNIQUE constraints generally — 30 added in 1791307627118-add-missing-unique-indexes.sql; index count 27 → 57; ./buddy doctor now reports "22 declared unique constraints all indexed, 6 tables skipped". Note doctor undercounts: it reported 14 missing of 22, while auditing every model's unique: true field plus every useUuid table against the live schema found 30.

Checked before applying, since doctor warns that duplicates make the migration hard-fail: 0 of 30 columns held duplicate values, all 30 column names were resolved against the live schema (models name fields camelCase, the database snake_case — subscriptions.providerId is provider_id), and all 30 target tables have a tracked create-table migration, which matters because CREATE UNIQUE INDEX IF NOT EXISTS still fails if the table is absent.

FKs off at runtime — split out to #86 with current numbers (25 of 33 declared FKs absent from the live schema) and a hazard this description does not cover: a table rebuild applies every current model constraint, not just the FK, so each one can fail on data the model disagrees with. That already happened once — court_houses rebuild carried "image" TEXT not null against 202 NULL rows.

Not addressed here: hot-path indexes, counter drift, mixed timestamps. Those remain as described.

9 days ago

Triage — one finding in this issue is factually wrong, and two others have landed. Keeping it open for the rest.

"FKs off at runtime" is not true. I reported the same thing myself earlier and was wrong: that reading came from the sqlite3 CLI, which defaults foreign_keys to OFF per connection. The app does not go through the CLI — bun-query-builder's DEFAULT_SQLITE_PRAGMAS applies PRAGMA foreign_keys = ON to every connection it opens. Enforcement is on in the running app. Worth striking from the issue so nobody re-derives a fix for it.

Landed since filing:

  • Hot-path indexes. 1791479015000-index-foreign-key-columns.sql adds 21 CREATE INDEX IF NOT EXISTS statements covering every foreign-key child column — SQLite never auto-indexes those, so each one was a full scan on join or cascade.
  • Missing unique indexes partially — 1791307627118-add-missing-unique-indexes.sql, plus the framework's unique-index skip bug (stacksjs/stacks#1952) synced so the runner stops dropping all-CREATE-UNIQUE-INDEX files.

Still open, and #47 carries the two concrete ones:

  • review_drafts_user_unique is a plain index, not UNIQUE
  • No partial UNIQUE on review_flags(judge_review_id, user_id)
  • Counter drift (the denormalised judge_reviews.comments is still best-effort on both increment and delete)
  • Mixed timestamp types
9 days ago

All nine criteria are met. The audit found five already done and four genuinely open — two of which turned out to be live bugs rather than hardening.

Already in place, never ticked: users_email_unique, judge_reviews_likes_user_review_unique, the partial unique on judge_reviews(user_id, judge_id) with SubmitReviewAction's 409, users_verified_judge_claim_unique with VerifyCredentialAction's re-check, and all three judge_reviews hot-path indexes.

Pragmas — asked for a configureSqlitePragmas() at boot. Measured: all three are already set. bun-query-builder applies foreign_keys and busy_timeout per connection, and journal_mode=WAL is a property of the file, not the session. An app-owned call would duplicate the db layer, so I added a test instead. Each of the three is invisible when it regresses — with foreign_keys off, every FK the repair pass added becomes decorative.

Delete cascade (882e9839) — two real bugs. Both review-delete paths omitted judge_responses, and since every child holds an FK with ON DELETE NO ACTION and FKs are enforced, that wasn't an orphan-row problem but a hard block: a review a judge had replied to could not be deleted, and the moderator got a constraint 500 on the review most likely to need taking down. Worse, judge_reviews was the first entry of USER_OWNED_TABLES, deleted by user_id inside a per-table .catch(() => {}) — so any review with a comment, photo, flag or response failed its constraint, the throw was swallowed, and the review survived an erasure request while the rows around it were removed. It failed silently by construction.

Comment counter (5193224f) — two of three writers did read-modify-write. The Math.max(0, n - 1) clamp in the delete path was the tell: it only exists if the value is already expected to be wrong. All three now recount, which makes every mutation self-healing. Fifteen seeder fixtures claimed 18, 12, 9… comments while creating no review_comments rows, so the first real comment on a seeded review appeared to delete seventeen.

Timestamps (ea45243f) — SELECT '2026-10-13 12:00:00' < '2026-10-13T11:00:00.000Z' returns 1. A row created an hour later sorts first. All 2,740 imported judges are space-format (the ingest relied on the column default) and the admin create path from #50 writes ISO, so the first judge added through that UI would have sorted wrong. Migration normalizes history; the ingest now writes ISO so it is not reintroduced.

Deliberately not done: deriving comment counts at read time, as the likes helper does. Stronger — there would be no number to be wrong — but the column is selected directly by the feed, article page, admin queue and seeder. Recounting removes the drift without that surgery; dropping the column is separate work.

Ready to close unless you want that last piece folded in.

9 days ago

Closing — all nine acceptance criteria met and verified.

  • users.email UNIQUE with a 409 on duplicate signup instead of a 500
  • likes deduped behind judge_reviews_likes_user_review_unique
  • SQLite pragmas (foreign_keys, busy_timeout, WAL) confirmed set on the app's connection and pinned by a test
  • shared deleteReviewCascade / deleteReviewsByUser, used by the admin delete, the author delete and account deletion
  • one live review per user per judge, behind a partial unique index with a 409
  • one verified claimant per judge, behind a partial unique index with a 409
  • hot-path indexes on judge_reviews: (judge_id, status, created_at), (status, created_at), (user_id)
  • comment counts recounted on every mutation; the seeder's phantom counts removed
  • timestamps normalized to ISO-8601, and the ingest script no longer relies on the column default

Left open deliberately, as its own piece of work: deriving comment counts at read time the way app/Helpers/reviewLikes.ts does. That would remove the denormalised column entirely, but it is selected directly by the feed, the article page, the admin queue and the seeder fixtures. Recounting removes the drift without that surgery.

Sign in to comment on this issue.