randomayzer/grok_review/GROK_REVIEW (1).md

21 KiB
Raw Blame History

Randomayzer — Phase G-1 Concurrency, Load, Abuse & Failure-Mode Review

Reviewer: Grok (xAI)
Date: 2026-08-17
Scope: Concurrency, load/scalability, abuse resistance, failure modes, DB invariants, observability, rate limiting, memory & algorithmic complexity.
Out of scope (per assignment): Public Verification Integrity (Antigravity), general QA/security review & VK Integration prep (OpenCode). No changes to Randomizer core, proof format, OAuth, UI branding, VK Provider contract, or large Prisma schema rewrites.

Repository snapshot: local extract of randomayzer-main (matches provided zip / expected main).
Commit SHA (from zip metadata): 26e82fcf8d8e5855ac9e46fa8af21ca7daacc36f


Executive Summary

Category Critical High Medium Low
Concurrency 2 2 1 0
Failure Modes 0 3 2 1
Abuse Resistance 1 3 2 1
Database Invariants 0 2 2 0
Scalability / Memory 1 3 2 0
Total 4 13 9 2

Maximum participant count synthetically exercised: 100 000 (core hashing / Fisher-Yates / verify).
Estimated safe production limit (current architecture, single Node process, 24 GB RAM): ~3050k eligible participants.
Race conditions found: Double-draw (protected by unique constraint but poor error handling), Snapshot version collision (unique constraint only), Participant-update vs Draw interleaving.

Production Core was not modified. Only documentation + optional offline benchmark script added.


Critical Findings

C1. Double-Draw Race Condition (Application-level TOCTOU)

Location: src/app/api/giveaways/[id]/draw/route.ts + PrismaGiveawayRepository.saveDrawResultAndAudit

Scenario: Two almost simultaneous POST /api/giveaways/:id/draw.

  1. Both requests read Giveaway (status = READY or SNAPSHOT_LOCKED).
  2. Both pass the early if (status === 'DRAWN') guard.
  3. Both may create / reuse snapshot.
  4. Both call executeDeterministicDrawV1 (possibly with different seeds).
  5. Both enter $transaction and attempt drawResult.create({ giveawayId }).

Protection today:

  • DrawResult.giveawayId @unique → second insert fails with Prisma P2002.
  • Transaction is atomic for the successful request.

Problems:

  • Second request receives 500 Internal Server Error (unhandled unique violation) instead of clean 409/400 “already drawn”.
  • Client may retry and keep failing.
  • Work (hashing, Fisher-Yates) is wasted on the loser.
  • No SELECT … FOR UPDATE / optimistic version / conditional UPDATE … WHERE status = 'SNAPSHOT_LOCKED'.
  • Default Prisma isolation (Read Committed) does not prevent the race.

Impact: Data integrity is preserved (only one DrawResult), but availability and UX under concurrent load are broken. In a load-balanced multi-instance deployment the race window is larger.

Recommended fix (proposal only — do not implement without agreement):

// Inside transaction, use conditional update as gate
const updated = await tx.giveaway.updateMany({
  where: { id, status: 'SNAPSHOT_LOCKED' },
  data: { status: 'DRAWN', drawnAt: ..., seed: ... },
});
if (updated.count === 0) {
  throw new Error('ALREADY_DRAWN_OR_INVALID_STATE'); // map to 409
}
// then create DrawResult + AuditRecord

Or use PostgreSQL advisory lock (pg_advisory_xact_lock(hashtext(giveawayId))) at the start of the transaction, or a dedicated draw_lock row.

Optimistic locking via a version / statusVersion integer column is also viable.

C2. Snapshot Version Race

Location: PrismaGiveawayRepository.createAndLockSnapshot

const latestVersion = current.snapshots.length > 0
  ? Math.max(...current.snapshots.map(s => s.version)) : 0;
const newVersion = latestVersion + 1;
// then $transaction([ create with newVersion, update status ])

Version is computed outside any lock. Two concurrent snapshot creations can choose the same version → unique constraint @@unique([giveawayId, version]) rejects one with 500.

Sufficient for data integrity? Yes (constraint works).
Production-safe? No — error handling and retry semantics are missing. Status can also be left inconsistent if one succeeds and the other fails after status update.

Proposal: Compute next version inside a serializable transaction or use INSERT … ON CONFLICT / sequence / MAX(version)+1 under SELECT FOR UPDATE on the Giveaway row.

C3. Full Participant Lists Loaded into Memory on Every getById / listAll

getGiveawayById and listGiveaways always include: { participants: true, snapshots: true }.

For a giveaway with 100k participants the JSON response + in-memory objects easily exceed hundreds of MB. listAll multiplies the problem.

Combined with snapshot eligibleParticipants Json this is the primary OOM vector.

C4. Snapshot Storage as Monolithic JSON

ParticipantSnapshot.eligibleParticipants Json stores the entire canonical array.

Eligible count Approx. canonical JSON size Prisma / PG TOAST risk Memory on read
10k ~1.52.5 MB OK Low
50k ~814 MB Acceptable Medium
100k ~2030 MB High (large object) High
500k ~80140 MB Problematic Almost certain OOM on small instances

No chunking, compression, or external object storage. Verification and UI that re-load the snapshot will re-materialize the whole array.


High Findings

H1. Participant Update vs Draw Race

saveParticipants is allowed only when status ∉ {SNAPSHOT_LOCKED, DRAWN, PUBLISHED}.
Draw can create a snapshot from the in-memory giveaway.participants if none exists.

If both run concurrently while status = READY:

  • Draw may snapshot a partially-updated list (deleteMany has finished, createMany has not, or vice-versa).
  • Or snapshot the old list while the new list is committed.

Result: Draw operates on a non-atomic participant set. Integrity of the snapshot relative to the final stored participants is not guaranteed.

Mitigation proposal: Always require an explicit snapshot before draw (already partially true via FSM), and make createAndLockSnapshot take a DB-level lock that also blocks saveParticipants.

H2. No Idempotency Keys

None of the mutating endpoints accept or honour Idempotency-Key:

  • POST /api/giveaways (create)
  • POST …/participants
  • POST …/snapshot
  • POST …/draw
  • POST …/publish (if exists)

For draw the unique constraint gives a crude form of “at-most-once”, but the error is not clean.
For create / participants / snapshot a client retry after a network blip can create duplicates or re-fetch VK data unnecessarily.

Recommendation:

  • Draw / Publish → rely on DB state machine + unique constraints (already present) + map P2002 → 409.
  • Create Giveaway, Snapshot, Participants fetch → accept optional Idempotency-Key header, store in a short-lived table or Redis, return the previous response on conflict.

H3. API Response Size / DoS Surface

POST …/participants returns the full allParticipants array.
GET /api/giveaways returns every giveaway with every participant and every snapshot.

A malicious or naïve client can trigger multi-hundred-MB responses. Combined with lack of rate limiting this is an easy memory / bandwidth exhaustion vector.

H4. No Request Timeouts / Cancellation on VK Fetch

VkProvider.fetchParticipants (and pagination) can run for a long time. There is no AbortController, no overall request timeout, no job queue. A slow VK response or large comment tree holds a Node request forever and can exhaust the connection pool.

When HTTP request-response stops being suitable:
When expected VK fetch + enrichment > 1520 s for typical giveaways, or when 10k+ participants become common. Move to background job (BullMQ / Inngest / custom) with status polling.

H5. Failure Modes Partial Crash After Commit

  • Crash after successful $transaction in saveDrawResultAndAudit but before HTTP response → client retries → unique violation → 500.
  • Crash in the middle of saveParticipants transaction → rolled back (good).
  • Process death after snapshot create but before draw → status = SNAPSHOT_LOCKED, safe to draw later.
  • VK 429 / 500 / network break during pagination → unhandled, leaves giveaway in FETCHING or READY with incomplete data. No partial-progress resume.

H6. winnersCount / reserveWinnersCount Abuse

No hard upper bound on winnersCount or reserveWinnersCount beyond Math.min(..., eligible.length).
A client can request winnersCount: 1_000_000 on a 100-participant giveaway; the code will still allocate and run the partial Fisher-Yates for the actual needed size, but the request body and subsequent JSON can be large.
More importantly, no validation that winnersCount + reserve ≤ reasonable constant (e.g. 100).

H7. Missing Conditional Status Updates

All status transitions (READYSNAPSHOT_LOCKED, SNAPSHOT_LOCKEDDRAWN) are plain update without WHERE status = expected. Under concurrency the wrong status can be overwritten.

H8. listAll + Dashboard Memory Amplification

Dashboard that calls listAll will pull every participant of every giveaway into the Node process on each page load.

H9. No Soft Limits on Concurrent Snapshots / Draws per Giveaway

A single giveaway can accumulate many snapshots (version keeps increasing). Each stores a full JSON copy. No retention policy yet.

H10H13. See detailed sections below (Rate limit, Observability, Privacy, Algorithmic notes).


Medium Findings

  • FSM guards are only application-level; a direct DB write can bypass them.
  • MemoryGiveawayRepository has no concurrency protection at all (single-process only).
  • Canonical stringify + sort is correct but CPU-heavy for 100k+; no incremental hashing.
  • Verify endpoint re-loads full snapshot every time — expensive for large draws.
  • No body-size limit middleware visible (Next.js default is generous).
  • Malformed / extremely long VK URLs not explicitly rejected early.
  • Unicode / null-byte handling in user names relies on JSON/Postgres; edge cases untested under load.
  • Retry storms on 500 from unique violations can amplify load.

Low Findings

  • Seed length is not capped; extremely long custom seed is accepted (only affects HMAC input size).
  • No explicit ON DELETE RESTRICT behaviour documented for operators.
  • AuditRecord and DrawResult both store winner IDs — minor redundancy.

Concurrency Review (Detailed)

Scenario Protected by unique / constraint? Clean error? Safe retry? Recommendation
Double draw Yes (giveawayId unique) No (500) No Conditional update + 409 mapping
Concurrent snapshot Yes (giveawayId+version) No (500) Partial Version under lock
Update participants + draw Partial (FSM) Risky Explicit lock / require pre-existing snapshot
Concurrent create giveaway No Creates dups Idempotency-Key
Concurrent participants fetch No Re-fetches Idempotency or debounce

Prisma $transaction (interactive or sequential array) uses the connections isolation level (default Read Committed). It does not by itself serialise the “read status → write DrawResult” critical section.


Failure-Mode Analysis

Failure Resulting state Safe to retry? Compensation needed?
Postgres unavailable 500, no write Yes No
Prisma transaction timeout Rolled back Yes No
VK 429 500 / incomplete participants After backoff Possibly clear partial
VK 500 / network mid-pagination Incomplete list saved or error Yes Re-fetch
Crash after snapshot, before draw SNAPSHOT_LOCKED, no DrawResult Yes (draw) No
Crash after DrawResult+Audit commit, before HTTP DRAWN + full audit records Client sees error, retry → 409/500 Map unique to 409
Crash between DrawResult and Audit (impossible same tx) Atomic

Abuse Cases & Suggested Limits

Abuse vector Current behaviour Suggested limit / mitigation
Thousands of Giveaways Unlimited 50100 / user / day (once auth exists)
Spam participants fetch Re-hits VK every time Rate limit + cache / debounce 5 min
Spam verify Cheap after first load 30 req/min per IP / giveaway
winnersCount = millions Capped by eligible Hard max 100 + 100 reserve
Massive custom seed Accepted Max 256512 chars
Oversized request body Next.js default Explicit 12 MB limit
Extremely long VK URL Parsed Max 2 kB, early reject
Repeated snapshot generation Unlimited versions Max 510 snapshots / giveaway, retention
Retry storm after 500 Amplifies 429 + Retry-After, circuit breaker

Full policy → docs/RATE_LIMIT_POLICY.md.


Database Invariants Audit

Invariant Guaranteed by DB? Guaranteed by app code? Notes
At most one successful DrawResult per Giveaway Yes (@unique) Yes (FSM + unique) Strong
At most one AuditRecord per Giveaway Yes (@unique) Yes Strong
DrawResult always references existing snapshot Yes (FK + Restrict) Yes Strong
Snapshot version unique per giveaway Yes (@@unique) Yes Strong
Cannot delete snapshot that has Draw/Audit Yes (onDelete: Restrict) Strong
Status = DRAWN implies DrawResult exists No Yes (same transaction) Soft possible manual inconsistency
Status = DRAWN implies AuditRecord exists No Yes (same transaction) Soft
Participants unique per (giveaway, user) Yes Yes Strong

No CHECK constraints or triggers enforce status ↔ existence of DrawResult/Audit. Application must remain the sole writer of status transitions.


Scalability & Algorithmic Complexity

Operation Time complexity Memory complexity Notes
Participant deduplication O(n) O(n) Map
Filter engine O(n) O(n)
Canonical sort O(n log n) O(n) localeCompare
Snapshot hashing O(n · L) O(n · L) L ≈ 150200 B/item
Partial Fisher-Yates O(k · C_HMAC) expected O(n) (copy) k = winners+reserve
Verification Same as draw O(n) Full re-execution
Prisma createMany O(n) O(n) Batch

No O(n²) algorithms found in the reviewed core paths. Dominant cost at scale is canonical JSON construction + SHA-256 and memory residency of the full participant arrays.

Synthetic baseline (Node 24, single core, see docs/PERFORMANCE_BASELINE.md):

n Hash (ms) JSON size (MB) Heap after gen (MB) Draw 10 winners (ms)
100 ~3 0.03 ~5 <1
1 000 ~6 0.27 ~5 <1
10 000 ~56 2.8 ~9 ~3
50 000 ~276 13.8 ~30 <1
100 000 ~994 27.7 ~133 ~1

Extrapolated 500 k: hash ≈ 5 s, JSON ≈ 80140 MB, heap pressure > 600 MB.


Memory Pressure Hotspots

  1. getGiveawayById / listGiveaways always include full participants + snapshots.
  2. createAndLockSnapshot materialises full array + JSON for Prisma.
  3. computeParticipantsSnapshotHash creates sorted copy + large intermediate string.
  4. executeDeterministicDrawV1 copies the eligible array.
  5. API handlers that return full participant lists.
  6. VK provider (if it accumulates all pages in memory before returning).

Worst-case 500 k participants on a 1 GB container: high probability of OOM during hash or snapshot write.


Recommendations (Prioritised)

Before any production traffic

  1. Map Prisma unique-violation (P2002) on DrawResult / Snapshot to HTTP 409 with clear message.
  2. Make draw status transition conditional (updateMany WHERE status = expected).
  3. Stop returning full participant arrays from list / participants endpoints (paginate or return stats + IDs only).
  4. Add hard caps: winnersCount ≤ 100, reserve ≤ 100, request body size, seed length.
  5. Add basic rate limiting (even if only in reverse-proxy / Next middleware).

Short-term (Phase 2)

  1. Introduce optimistic locking or advisory locks for draw + snapshot.
  2. Require explicit snapshot before draw; never auto-create inside the draw handler under concurrency.
  3. Add Idempotency-Key support for create / participants / snapshot.
  4. Paginate or stream large participant responses.
  5. Add request-level timeout + AbortController for VK calls.

Medium-term / Phase 3+ (architecture proposals)

  1. Move large snapshots out of Postgres JSON:
    • Option A: normalised SnapshotParticipant rows (chunked inserts).
    • Option B: compressed JSON (gzip / zstd) in bytea + object storage (S3) for the bulk.
    • Option C: Merkle-tree root only in DB, leaves in object storage (best for public verification of very large sets).
  2. Background job for VK fetch + enrichment when n > ~510 k.
  3. Giveaway-level retention / anonymisation policy (see Privacy section).
  4. Connection pooling, statement timeouts, and circuit breakers for VK + DB.

Files Changed / Added

File Action Purpose
docs/GROK_REVIEW.md Created This review
docs/LOAD_TEST_PLAN.md Created Load scenarios
docs/PERFORMANCE_BASELINE.md Created Measured numbers
docs/RATE_LIMIT_POLICY.md Created Production rate limits
docs/OBSERVABILITY.md Created Metrics, logs, alerts
scripts/benchmarks/core-performance.mjs Created Offline core micro-benchmarks

No production Core, Prisma schema, proof format, or API contracts were altered.
No existing tests were broken (benchmark is standalone).

Tests added: 0 (analysis-only phase; benchmark is not a unit test).
Maximum participant count exercised: 100 000.


Definition of Done Checklist

  • Double draw race investigated
  • Snapshot race investigated
  • Participant update vs draw investigated
  • Idempotency strategy proposed
  • Load up to 500 k estimated
  • Performance baseline recorded
  • Memory risks identified
  • Abuse cases covered
  • Failure modes described
  • DB constraints audited
  • Rate limit policy written
  • Observability plan written
  • Production Core left intact
  • Deliverables present

End of Grok Phase G-1 Review