v1.7: Postgres backend with verified SQLite migration - #6
Merged
Conversation
Spec for moving persistence from SQLite to Postgres (v1.7) and adopting SuperTokens without breaking existing Discord/GitHub logins (v1.8). Decisions settled during brainstorming: dual backend with Postgres as the default, tests against real Postgres in Docker, auto-migrate on boot behind guards, and the identities auth split done up front so v1.8 stays small. Two findings drove the design. SuperTokens supports external user ID mapping, so users.id can stay 'provider:providerId' and no foreign key or SUPER_ADMIN_IDS handling has to change. And SuperTokens' /auth/callback/github is not a subdirectory of the currently registered /auth/github/callback, which would fail GitHub's redirect_uri rule - the registered callback gets widened to /auth so both paths work at once and passport keeps running throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight tasks, each ending in an independently testable deliverable: 1. Make the db interface async (SQLite unchanged underneath) - 292 call sites 2. Split db.js into facade + SQLite driver + schema module 3. Postgres test harness: Testcontainers, per-file databases, CI matrix 4. Postgres schema and driver, gated by a cross-dialect parity test 5. The identities auth split, with in-place upgrades for both backends 6. The SQLite to Postgres migrator, verified before COMMIT 7. Auto-migration on boot behind guards 8. Deployment config, Unraid runbook, smoke suites against Postgres Task 1 is deliberately alone: mixing the async refactor with the driver split would make the diff unreviewable. Two landmines found while writing this that the spec had not caught, both silent-corruption class rather than crash class: - pg returns BIGINT as a string by default. Every epoch-ms column here is BIGINT and every consumer does arithmetic on it, so the int8 type parser has to be registered before the first query runs. - Postgres folds unquoted identifiers to lowercase, so listLeaderboard's 'AS userId' would return 'userid' and every rungsClaimed would read undefined on a leaderboard that still rendered. Both are pinned by tests/db.parity.test.js. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every server/db.js export returns a Promise; all call sites across server/ and tests/ now await. better-sqlite3 remains the implementation, so behaviour is unchanged - this is purely the shape change that lets a pg driver slot in behind the same interface. shared/ is deliberately untouched and stays synchronous: the client bundles it, and an async reducer would be a far larger change than this migration needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pure refactor - no SQL changes. server/db.js is now a re-export shim so every existing import path keeps working, while server/db/ holds the seam the Postgres driver plugs into next. Adds a schema_migrations table: the schema has outgrown what CREATE TABLE IF NOT EXISTS plus guarded ALTERs can honestly express, and the identities split needs a real version marker. Also extracts server/db/shared.js: findAvailableUsername, parseEventRow, and the putEvent/upsertParticipation row normalizers are dialect-free logic the Postgres driver will need identically, so they live once instead of being duplicated (and drifting) per driver. findAvailableUsername is now async and awaits its isTaken predicate - harmless for SQLite's synchronous predicate, required for Postgres's async one - so one implementation serves both.
Static `import { driver } from '../server/db/index.js'` is hoisted by the
ESM spec above the `process.env.DB_PATH = ':memory:'` assignment earlier in
the same file, so it stood up the driver (and its data/rackstack.db file)
against the real default path instead of an in-memory DB - polluting a
real on-disk file across test runs and producing state leakage between
tests. Pull `driver` off the existing dynamic `dbMod` import instead, same
as every other export these files already consume that way.
Caught by rerunning tests/db.test.js in isolation during self-review: it
failed deterministically (config/roles state carried over from a prior
run) even though the full `npm test` run had passed, because npm test's
earlier passing run happened to be the one that created the stray file.
Code review finding: the function has been async (it awaits the shared, async findAvailableUsername) since this refactor split it out of db.js, so the inherited "Sync" suffix was false - misleading for Task 4's Postgres driver author, who needs this exact seam. Renamed in schema.sqlite.js and updated its one call site in applySchema. driver.sqlite.js imports it aliased (dedupeUsernamesSchema) so it doesn't shadow the driver's own dedupeUsernames interface method, which stays a thin, now explicitly awaited, delegation to it (was a bare `return dedupeUsernamesSync(db)`, inconsistent with the explicit awaits elsewhere in the same file).
Testcontainers locally, a service container in CI, and a per-test-file database so the 19 db-touching suites cannot see each other's rows. The matrix exists because we ship two backends: a SQLite driver that CI never exercises would drift from the Postgres one, and dialect drift is the bug class that loses data. Locally, the only container runtime available is rootless Podman (no docker binary), so pg-global.js points DOCKER_HOST at the Podman socket and disables Ryuk when the developer hasn't already set either - Testcontainers then works unmodified on Docker or Podman. Ryuk's absence makes teardown() the only thing that stops/removes the container; StartedTestContainer#stop() removes volumes too, so no reaper is required as long as teardown runs. No Postgres driver exists yet (that's Task 4), so tests/harness.test.js is the only suite that actually exercises TEST_DATABASE_URL right now - the other 452 tests hardcode DB_PATH=':memory:' and keep running against SQLite regardless of TEST_BACKEND.
Both backends now satisfy the same interface and the full suite runs against each. tests/db.parity.test.js pins the behaviours that differ between the dialects and would otherwise fail silently in production: - pg returns BIGINT as a string by default; every epoch-ms column in this schema is BIGINT, so the int8 type parser is registered before any query. - Postgres folds unquoted identifiers to lowercase, so listLeaderboard's camelCase aliases are double-quoted - unquoted, the client receives 'userid' and every rungsClaimed reads undefined. - config_history ordered by rowid on SQLite; Postgres has none, so it gains a BIGSERIAL key and the admin rollback UI keeps its newest-first order. - COLLATE NOCASE becomes a unique functional index on lower(username). - SQLITE_CONSTRAINT_UNIQUE becomes SQLSTATE 23505 - that retry path is what keeps a display-name collision from locking a player out on login. Also points all 13 db-touching test files at provisionDatabase() instead of hardcoding DB_PATH=':memory:', so npm test genuinely exercises Postgres instead of silently falling back to SQLite regardless of TEST_BACKEND. tests/db.test.js and tests/db.events.test.js gain Postgres-branch schema introspection (pg_tables/information_schema vs. sqlite_master/PRAGMA); the two guarded-ALTER tests are SQLite-only (schema.pg.js has no ALTER history to replay) and skip on pg via it.runIf. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Critical: an ambient DATABASE_URL was never cleared on the sqlite test path, and server/db/index.js checks it before DB_PATH - so a developer with DATABASE_URL exported (plausible mid-migration, and the var Task 8 documents for production) would have every "sqlite" test file silently connect to that real database instead. Centralized all env-var writing for both backends inside provisionDatabase() itself (the pg branch is now the only writer of DATABASE_URL in the codebase; the sqlite branch clears it), removing the duplicated per-file if/else from all 14 test files. Covered by a new regression test in tests/harness.test.js that sets DATABASE_URL to an RFC-2606-reserved unreachable host and proves the facade still resolves to sqlite end-to-end, plus a deliberate full `npm run test:sqlite` run with that bogus DATABASE_URL exported (464/464 green in under 2s - no connection ever attempted). Important: extracted dedupeUsernames' suffixing walk (identical in both drivers apart from the SQL) into shared.js's dedupeUsernameRows, so the -2/-3 convention can't drift between drivers again. Replaced the false "exercised elsewhere in this suite" claim on db.test.js's guarded-ALTER skip with a real cross-backend test that calls applySchema() a second time against an already-populated database - the actual production restart path - on both backends. Minor: fixed db.parity.test.js's beforeAll/afterAll split (a RED-path import failure left `db` undefined, so afterAll's TypeError masked the real error and skipped cleanup) by switching to a top-level import like every other rewired file; added ORDER BY tie-breakers to driver.pg.js's listEvents/getAllUsersWithSaves so ties sort deterministically instead of arbitrarily on Postgres. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
users.id stays 'provider:providerId', so all three foreign keys and SUPER_ADMIN_IDS keep working untouched - the split is additive from the application's point of view. upsertUser now resolves through identities and records last_login_at. Existing databases are migrated in place on boot. SQLite needs the full table-rebuild dance because DROP COLUMN is refused while the column participates in a table-level UNIQUE constraint, and the rebuild is followed by a foreign_key_check - a violation there would mean orphaned saves. Both schemas leave their original users CREATE TABLE untouched and migrate the columns away as a guarded step that runs on every boot, so even a fresh database exercises the real migration path, not just the dedicated upgrade-path test. getAllUsersWithSaves() keeps exposing provider via the user's primary (earliest-created) identity - identical to the old column's output for every single-identity user today. New listIdentities(userId) export is unused until v1.8. supertokens_user_id ships nullable and unused; v1.8 fills it in.
Written now rather than with Task 8 because the backup half is valid today and should happen before anything else touches production data. Leads with a status table making clear the migrator does not exist yet and Task 5 has open defects, so nobody follows Part B against live data by mistake. The rollback section names the one-way door explicitly: reverting to SQLite always works mechanically, because the migration never modifies or deletes that file, but it is frozen at cutover. Rolling back ten minutes later is free; rolling back three days later silently discards three days of everyone's progress with no error shown to anyone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NeverEndingCode
changed the title
…k, atomic upsertUser writes Four Important findings from review, all in the migration paths: 1. identities is now created unconditionally on every boot, before the sqlite guard (schema.sqlite.js) - it was previously created only as a side effect of the users rebuild, so any database reaching "users has no provider column" without identities existing would boot cleanly and then throw on first login. Postgres already created it unconditionally. 2. schema.sqlite.js's foreign_key_check and its throw now run INSIDE the rebuild's db.transaction(), before it returns, so a violation aborts the rebuild instead of being discovered after COMMIT - previously the process died post-commit, but the next boot saw no provider column, skipped the rebuild, and never checked again: log-and-continue spread across two boots. The orphan test now also asserts users still has its provider column afterward, pinning the rollback rather than only the rejection. 3. upsertUser's two writes (users, identities) are now atomic on both drivers - db.transaction() on sqlite, a checked-out client with BEGIN/COMMIT/ROLLBACK on pg. Previously a failure on the second write left a users row with no identity, which the identity lookup can never find again - the next login attempt would retry INSERT INTO users on the same primary key and raise a constraint code neither retry guard recognizes, a permanent lockout. Covered by a new test that forces the identities write to fail and asserts no orphaned users row survives. 4. Both base `users` DDLs now ship in their final, post-split shape (no provider/provider_id) instead of declaring columns migrateIdentities immediately removes on every fresh boot. This closes the concrete risk that a future guardedAddColumn addition would exist on users but be absent from the rebuild's hardcoded users_new column list, silently dropping that column's data for any database still upgrading through the pre-split shape - now the rebuild only ever runs against a real upgrade target. users_new carries a comment warning it must stay in sync with the base DDL. Plus the minors: foreign_keys restored in a finally; the pg guard pinned to current_schema(); last_login_at set at insert (not just on return visits); the upgrade-path test asserts save.data byte-for-byte; a case pinning getAllUsersWithSaves returning provider: null for a user with no identity row; migrateIdentities private on both backends; provider_id added to the primary-identity tie-break. npm run test:all: sqlite 472/472, pg 469/472 + 3 pre-existing skips.
The test added in 7cc2446 renamed `identities` away entirely to force upsertUser's second write to fail. That also breaks the identity SELECT at the top of upsertUser, so the miss branch's INSERT INTO users never even ran - the test passed against both the fixed driver and the old, non-atomic one, for the wrong reason (nothing to roll back either way). Replaced with a trigger (SQLite: CREATE TRIGGER ... BEFORE INSERT; pg: a BEFORE INSERT trigger function) that only fires for one specific provider_id, so the identity lookup still misses normally, the users insert still runs, and only the identities insert fails - the actual failure mode the fix addresses. Verified directly: reverted driver.sqlite.js and driver.pg.js to their pre-fix (2f3bfe0) content in turn and reran this test - it now fails on both, with the orphaned users row as the visible symptom, exactly as described in the review. Restored the fixed drivers and reran - green on both. Also reverted schema.sqlite.js's FK-check-inside-the-transaction fix locally and confirmed the upgrade test's rollback-pinning assertion (`expect(cols).toContain('provider')`) catches that regression too, before restoring it. npm run test:all: sqlite 472/472, pg 469/472 + 3 pre-existing skips.
…the vacuous last_login_at assertion
Two items from re-review:
1. Important (new, introduced by the prior fix round): moving schema.pg.js's
base users DDL to the post-split shape (last round's Important 4) was
correct, but it meant migrateIdentities's backfill-and-DROP-COLUMN branch
became unreachable by any test that boots a driver normally - no pg test
run ever exercised it anymore, unlike sqlite's dedicated old-shape test
(which bypasses applySchema's DDL via a raw better-sqlite3 handle and so
stayed unaffected). Reinstated as a permanent test: builds a genuinely
pre-split users table directly against a fresh Postgres database (not via
applySchema), runs applySchema over it, and asserts the columns are
dropped, the identity backfilled, users.id unchanged, and the save
preserved byte-for-byte - plus idempotency across two more calls. Gated
on driver.__backend === 'pg' since no postgres container is even started
when running the sqlite suite.
2. Minor 7 (still open): the returning-login test's last_login_at assertion
was `>= ` against its own insert-time baseline, which is trivially true
even with the UPDATE deleted entirely - the same vacuous-test shape
caught in the atomicity test last round. Fixed by faking only `Date`
(vi.useFakeTimers({ toFake: ['Date'] })), advancing the clock a real 50s
between the two upsertUser calls, and asserting the exact resulting
value - only true if the UPDATE actually ran and actually wrote it.
Both verified to fail against the un-fixed code before being finalized:
- Reverted schema.pg.js's backfill (kept the DROP COLUMN, removed the
INSERT) -> new pg upgrade test fails with the identity row missing.
- Removed the `UPDATE identities SET last_login_at` statement from both
drivers in turn -> the fake-timer assertion fails with the stale
insert-time value on both backends.
All reverts restored before this commit; git diff against HEAD was empty
for every production file both times.
npm run test:all: sqlite 472/473 (1 skipped - pg-only test), pg 470/473
(3 skipped - pre-existing sqlite-only tests).
Copies every table inside one transaction and verifies before COMMIT: row counts plus a SHA-256 content fingerprint computed identically on both sides, with rows sorted by primary key and columns by name so neither side's natural ordering can make identical data look different. Any mismatch rolls back. verifyMigration is exported as its own function rather than gated by a test-only injection hook, so the rollback test can prove it actually rejects a deliberately corrupted target - not merely that it agrees with itself on clean data - and so it's independently testable. That separation caught a real bug during TDD: verification was fingerprinting the *original* usernames straight off the SQLite file, so a legitimate in-memory dedupe rename always looked like corruption. Fixed by sharing one dedupedUsers() helper between the insert step and the verifier, the same pattern already used for identity synthesis, so neither can independently drift from what was actually written. Checkpoints the WAL before reading, refusing outright if the checkpoint fails - recent commits live in rackstack.db-wal, so a migrator that read only the main file would quietly move stale data. The SQLite file is never modified beyond that checkpoint and never deleted - it is the rollback path. Tolerates old schemas: a v1.1-era source with only users and saves (still carrying provider/provider_id on users, no identities table) migrates cleanly and gets its identities synthesised. Every table's insert filters source columns against the target's actual information_schema.columns first, since a legacy users row's provider/provider_id - and config_history's absent id - have no 1:1 home in the post-split Postgres schema. Dedupes case-variant duplicate usernames in memory before inserting (the same -2/-3 suffixing convention as dedupeUsernames), since applySchema's unique index on lower(username) exists before this migrator ever runs. Every rename is logged so the operator can see which players were affected. Extracts the BIGINT->Number pg type parser out of driver.pg.js into pgTypes.js so migrate.js's own pg.Pool gets it too - a first test run caught last_save round-tripping as a string because migrate.js never went through driver.pg.js. tests/fixtures/v11-sqlite.db is the committed v1.1-era two-table fixture. tests/migrate.test.js covers the brief's four cases plus eight more: idempotency, an actual username collision, the WAL checkpoint, verifyMigration in isolation (clean-pass, content-mismatch, and count-mismatch), a fully-modern source exercising every table (identities already split, config_history, minigame_sessions, live_events, event_participation), and a byte-exact untouched-source-file assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
…efuse-not-drop policy
Three Important fixes from code review:
1. The WAL checkpoint guard couldn't detect its own failure. wal_checkpoint(TRUNCATE)
reports a blocked checkpoint as a returned row ({busy: 1}), not an exception - the
result was discarded, so a genuinely blocked checkpoint proceeded silently. Also,
new Database(sqlitePath) was outside the try, so an unreadable file or a directory
path threw a raw SqliteError instead of the operator-facing message and leaked the
pg pool. Fixed both, and corrected the comment's claim that this checkpoint is a
correctness requirement - it isn't: a SQLite reader always merges committed WAL
frames regardless of whether anyone checkpoints. The real WAL trap is copying the
*files* for a backup while -wal is open (what the runbook actually warns about),
which this function, reading through a live connection, never does.
2. The emptiness guard checked only `users`, but server/index.js seeds live_events on
every boot - a target the app has booted against even once has an empty `users` but
a populated `live_events`, and would either collide with a raw SQLSTATE 23505 or,
for a v1.1 source with no live_events table, silently COMMIT while leaving those
rows unaccounted for. Now checks every table in TABLES and names the offending one.
3. A source column or table with no home in the target schema was silently dropped -
and, for columns, excluded from verifyMigration's fingerprint too, making the loss
unverifiable. Refuse-and-explain is now the default: ALLOWED_DROPPED_COLUMNS
allowlists the one intentional case (legacy users.provider/provider_id), and
assertKnownTables refuses before BEGIN if the source has a table TABLES doesn't
know about.
Four minors also fixed: pool leaks on early applySchema/emptiness-check failures now
call pool.end() before rethrowing; a logger without .error falls back to .log instead
of masking the real error with a TypeError; a failing ROLLBACK no longer replaces the
original error that caused it; config_history's source read is now ORDER BY rowid so
insertion order survives into the target's BIGSERIAL id, which the admin rollback UI
depends on. Also unified two independently-written sort comparators and added direct
test coverage for username-rename logging.
tests/migrate.test.js: 12 -> 21 tests. Each Important's fix was verified red/green by
swapping in the pre-fix migrate.js and confirming the corresponding new test fails;
one test (the WAL-truncation-size check) intentionally also passes against the old
code, since its job is narrower (proving the checkpoint call isn't a no-op, which was
already true) than the two tests that specifically target busy-detection and
open-failure-wrapping. Full findings, including two honestly-flagged coverage gaps
(no test for the ROLLBACK-preserves-original-error fix; the config_history-order test
doesn't discriminate old vs new code because SQLite's implicit scan order already
matched insertion order in the tested scenario), are in
.superpowers/sdd/2026-08-01-v1.7-postgres/task-6-report.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
Migrates only when DATABASE_URL is set, a SQLite file exists, and Postgres is empty. A populated target is left alone, so a restart can never re-import over live data. A migration failure is fatal by design. Serving an empty game is worse than being down - a stopped container gets investigated, an empty leaderboard might not be noticed until saves have been overwritten on top of it. server/index.js imports the db modules dynamically because the facade resolves its driver at module-evaluation time; a static import would open the pool before the migration had a chance to run.
server/index.js:99 logged e.message verbatim, which is empty for Node's own
AggregateError - exactly the shape a connection-refused Postgres produces.
That line is the operator's only signal for why the server refused to boot
over live save data, so printing nothing after the colon defeated the whole
point of making the failure fatal.
describeFatalMigrationError (server/db/migrate.js) falls back to the joined
.errors messages, then String(e), so the line can never come out blank.
Verified against a real ECONNREFUSED Postgres target: now logs "connect
ECONNREFUSED ::1:1; connect ECONNREFUSED 127.0.0.1:1" instead of nothing.
Per code review on task 7: the brief's sample code logged e.message directly;
following it verbatim here would have shipped a worse outcome than the
requirement ("the operator learns why the server refused to start") intends.
Adds a postgres service to compose, DATABASE_URL to the Unraid template and .env.example, and a migration runbook link to the README. The runbook leads with copying all three rackstack.db* files: recent progress lives in the -wal, and copying only the .db is the most likely way to lose data during this migration. Also fixes the six tests/e2e/smoke-v1*.mjs suites, which had not been run since Task 2 removed server/db.js's `db` export: they destructured `db` and called `db.pragma(...)` unconditionally, throwing a TypeError on startup every time. Fixed to use `driver.__raw` guarded on `driver.__backend === 'sqlite'` (busy_timeout has no Postgres meaning), and verified all six pass against both SQLite and a scratch Postgres database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
…rgence
Whole-branch review of v1.7 turned up one Critical and five Important
findings. All are fixed here.
CRITICAL - lost updates on concurrent requests. Before this branch every db
call was a synchronous better-sqlite3 call, so `getSave -> evaluate ->
putSave` completed inside one event-loop turn and nothing could interleave
with it. Node's single thread was the lock. Making the interface async
(Postgres cannot be synchronous) silently removed that guarantee: two
concurrent requests for one user both awaited getSave, both saw the same
state, and the second putSave discarded the first - with both requests
returning 200. Two open tabs is the normal case for an idle game, and
client/src/game/api.js only serializes flushes within a single tab.
server/userLock.js adds a per-user promise chain; loadAndEvaluate,
applyActions and the minigame-finish sequence hold it across the whole
read-modify-write. Different users never block each other. This was a
regression against pre-v1.7 behaviour on BOTH backends, not a Postgres gap.
IMPORTANT - DB_PATH defaults disagreed between the migrator and the driver
facade ('/app/data/rackstack.db' vs a repo-relative path). With DATABASE_URL
set and DB_PATH unset - anything not started from the Docker image, since
only the Dockerfile supplies it - the migrator probed a path that did not
exist, reported 'fresh Postgres install', skipped the migration, and the
server came up serving an empty Postgres while the real saves sat untouched
and invisible at the other path. That is the "serve an empty game" outcome
the fatal boot guard exists to prevent, reached through the one branch that
is deliberately not fatal. resolveSqlitePath in db/shared.js is now the
single authority; db.sqlitePath.test.js guards against the defaults drifting
apart again.
IMPORTANT - .env.example shipped an uncommented DB_PATH=/app/data/..., which
the README tells local developers to copy verbatim; the SQLite driver
mkdir's the parent, so `npm run dev` died with EACCES on /app. Commented out.
IMPORTANT - three documents claimed `postgres://` is "rejected outright".
Nothing rejects it: pg-connection-string parses it identically to
`postgresql://`, verified directly. An operator debugging a genuine
connection problem would have chased the scheme first. Claim removed from
.env.example, README and the runbook.
IMPORTANT - docker-compose.yml hardcoded DATABASE_URL under `environment:`,
which takes precedence over `env_file:`, so the documented rollback ("blank
out DATABASE_URL and restart") was a no-op on compose. Now
${DATABASE_URL-default} - without the colon, so an explicitly empty value in
.env stays empty instead of falling back to the default. The README gains a
table of where that variable actually lives per deployment style.
IMPORTANT - three "no pool leak" assertions in migrate.test.js proved
nothing: each made a second migrateSqliteToPostgres call, but every call
builds its own pool, so a pool leaked by the first cannot affect the second.
All three passed with the pool-ending try/catch deleted. They now spy on
pg.Pool.prototype.end. Verified all three fail against the pre-fix code.
Minors also addressed: sqlite/pg ordering divergence in listEvents and
getAllUsersWithSaves (seedSeasonalEvents stamps every seasonal event with
the same created_at, so ties are guaranteed, not hypothetical);
verifyMigration's fingerprint now sorts both sides with a total order, so
two config_history rows sharing (version, updated_at) cannot fail
verification and refuse boot over identical data; insertUserAndIdentity
destroys rather than pools a client whose ROLLBACK failed, and no longer
lets a rollback error replace the SQLSTATE 23505 its caller's retry logic
depends on; a Postgres failure that surfaces after the auto-migrate guard
now prints the operator-facing FATAL line instead of a raw
ERR_UNHANDLED_REJECTION, and the fatal path flushes stderr before exiting
(writes to a pipe - what Docker gives the container - are async, so
process.exit could truncate the only diagnostic there is).
Reviewed and deliberately NOT added: a cross-table "every migrated user has
an identity" check. Per-table verification already proves target users and
target identities each match their source in count and content, and two sets
that each equal their source cannot disagree about which users have
identities - the check would be unreachable. Rationale recorded in
verifyMigration so it isn't re-added.
Verified: sqlite 492 passed/26 skipped, postgres 515 passed/3 skipped, all
six e2e smoke suites pass. Booted against a real Postgres container (fresh
install -> skip -> listening) and against an unreachable one (FATAL line,
exit 1). Not machine-verified: the compose interpolation change, since no
docker/podman compose CLI is installed here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
NeverEndingCode
changed the title
NeverEndingCode
marked this pull request as ready for review
August 6, 2026 01:26
CI has been failing on every commit since 5bd119a, on BOTH matrix jobs, with "No test files found, exiting with code 1" preceded by `TypeError: webidl.util.markAsUncloneable is not a function`. The dual-backend matrix was only ever verified locally; the green result it was believed to produce never happened. Cause: tests/setup/pg-global.js imported @testcontainers/postgresql at module scope. Testcontainers pulls in undici@8.9.0, which declares `engines: { node: '>=22.19.0' }` and throws on require under Node 20. CI runs Node 20 to match the Dockerfile's node:20-bookworm-slim, so the import blew up during globalSetup before a single test file was collected. It took out the sqlite job too - an ES import is hoisted, so `if (TEST_BACKEND === 'sqlite') return` never got a chance to run. It passed locally only because this machine is on Node 22. Fix: import it dynamically inside setup(), after both early returns. CI supplies TEST_DATABASE_URL from a Postgres service container and has no use for Testcontainers at all, so this keeps CI on the same Node major as production instead of bumping CI to 22 and testing a runtime we don't ship. Verified by making the dynamic import target unresolvable - reproducing CI's constraint that testcontainers cannot load - and running both matrix configurations: TEST_BACKEND=sqlite passed 492/26 skipped, and CI-style TEST_BACKEND=pg with TEST_DATABASE_URL pointed at a service container passed 515/3 skipped. Restored the real import and confirmed the local Testcontainers path still provisions and passes 515/3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves persistence from SQLite-only to a dual-backend design where
DATABASE_URLselects Postgres and SQLite remains fully supported and still the default. Includes an automatic, verified SQLite to Postgres migration that runs on boot behind guards, and the operator runbook for cutting over an existing Unraid install without losing data.All eight planned tasks are complete, each individually reviewed with fix rounds, followed by a whole-branch review whose findings are fixed in
a227b65.Test status
npm run test:sqlitenpm test(Postgres)npm run smoke(6 e2e suites, real Chromium)Also booted end to end against a real Postgres container: fresh install to
[migrate] skippedtolistening on :3000, and against an unreachable Postgres to confirm the fatal path prints a usable reason and exits 1.What ships
server/db.jsfronts one async repository interface (server/db/index.js) implemented bydriver.pg.jsanddriver.sqlite.js. Every caller (routes, services, minigames) is backend-agnostic. Dialect-free logic lives once inserver/db/shared.js.server/db/migrate.js, alsonpm run migrate:pg). One transaction, per-table SHA-256 content fingerprints verified before COMMIT, refuses rather than acting when the target is non-empty or the source has a table it does not know about. The SQLite file is never modified beyond a WAL checkpoint and never deleted, so it stays the rollback artifact.identitiestable split out ofusers, withusers.iddeliberately unchanged soSUPER_ADMIN_IDSand every foreign key keep working.supertokens_user_idships unused, for v1.8.npm run test:allruns everything against both; CI does the same via a Postgres service container, Testcontainers locally.docker-compose.yml,unraid-template.xml,.env.example, anddocs/postgres-migration-runbook.md(backup, cutover, rollback).Whole-branch review findings, all fixed in a227b65
Critical — lost updates on concurrent requests. Before this branch every db call was synchronous, so
getSave -> evaluate -> putSavecompleted in one event-loop turn and could not interleave. Making the interface async removed that guarantee silently: two concurrent requests for one user both read the same state and the second write discarded the first, with both returning 200. Two open tabs is the normal case for an idle game. Fixed with a per-user promise chain (server/userLock.js) held across the whole read-modify-write inloadAndEvaluate,applyActionsand the minigame-finish path. This was a regression against pre-v1.7 behaviour on both backends, not a Postgres-only gap.Important — the two
DB_PATHdefaults disagreed. The migrator fell back to/app/data/rackstack.db, the driver facade to a repo-relative path. WithDATABASE_URLset andDB_PATHunset (anything not started from the Docker image, since only the Dockerfile supplies it) the migrator probed a nonexistent path, reported "fresh Postgres install", skipped the migration, and the server served an empty Postgres while the real saves sat invisible at the other path. That is the exact outcome the fatal boot guard exists to prevent, reached through the one branch that is deliberately not fatal.resolveSqlitePathis now the single authority, with a test guarding against the defaults drifting apart again.Important — operator config.
.env.exampleshipped an uncommented container-onlyDB_PATHthat broke the README's own local-dev flow withEACCESon/app. Three documents claimedpostgres://is "rejected outright" when nothing rejects it (verified:pg-connection-stringparses both schemes identically).docker-compose.ymlhardcodedDATABASE_URLunderenvironment:, which outranksenv_file:, making the documented rollback a no-op on compose — now${DATABASE_URL-default}, without the colon so an explicitly empty value stays empty.Important — three vacuous pool-leak assertions. Each proved "no leak" by making a second migrator call, but every call builds its own pool, so a leaked pool cannot affect the next one. All three passed with the pool-ending
try/catchdeleted. They now spy onpg.Pool.prototype.end, and all three were confirmed to fail against the pre-fix code.Minors also fixed: sqlite/pg ordering divergence in
listEventsandgetAllUsersWithSaves; a total sort order inverifyMigrationso tiedconfig_historyrows cannot fail verification over identical data;insertUserAndIdentitynow destroys rather than pools a client whose ROLLBACK failed, and no longer lets a rollback error replace the SQLSTATE 23505 its retry logic depends on; a Postgres failure surfacing after the auto-migrate guard now prints the operator-facing FATAL line rather than a rawERR_UNHANDLED_REJECTION; the fatal path flushes stderr before exiting, since writes to a pipe are async andprocess.exitcould truncate the only diagnostic there is.CI was red the whole time, and nobody knew
Worth calling out separately, because it invalidated a claim this branch had been carrying: CI had failed on every single commit since
5bd119a, on both matrix jobs, withNo test files found, exiting with code 1. The dual-backend matrix was only ever verified on a developer machine; the green CI result it was believed to produce never once happened.tests/setup/pg-global.jsimported@testcontainers/postgresqlat module scope. Testcontainers pulls inundici@8.9.0, which declaresengines: { node: '>=22.19.0' }and throwswebidl.util.markAsUncloneable is not a functionon Node 20. CI runs Node 20 deliberately, to match the Dockerfile'snode:20-bookworm-slim. The import blew up duringglobalSetupbefore a single test file was collected — and it took out the sqlite job too, which has nothing to do with Postgres, because an ES import is hoisted and runs regardless of theif (TEST_BACKEND === 'sqlite') returnon the first line ofsetup(). It passed locally only because this machine is on Node 22.Fixed by importing it dynamically, after both early returns. CI supplies
TEST_DATABASE_URLfrom a Postgres service container and has no use for Testcontainers at all, so this keeps CI on the same Node major as production rather than bumping CI to 22 and testing a runtime we do not ship. Verified by making the import target unresolvable — reproducing CI's constraint exactly — and running both matrix configurations green, then restoring it and confirming the local Testcontainers path still provisions.Both jobs now pass in CI.
Landmines caught during implementation
Not anticipated by the plan, found while building:
await x().propparses asawait (x().prop)— reads a property off a Promise and yieldsundefinedrather than throwing. ~25 instances during the sync-to-async refactor.DATABASE_URLredirected the entire SQLite test run at a live database, writing to it and runningDROP INDEXagainst it while reporting green.tests/helpers/backend.jsis now the only writer of that variable.process.env.DB_PATH, booting the driver against the on-disk database mid-test.pgreturnsBIGINTas a string by default, and every epoch-ms column here isBIGINT.listLeaderboard'sAS userIdwould have returneduseridand everyrungsClaimedwould have readundefinedon a leaderboard that still rendered.npm testand were only ever validated withnode --check(syntax only, never resolves imports).Not verified here
Running a real migration against the owner's production Unraid export. That cannot be done from the repo alone —
docs/postgres-migration-runbook.mdPart A covers taking that backup, and it is the one step that needs the live data.The compose interpolation change is reasoned from documented behaviour, not machine-checked: no
docker composeorpodman-composeCLI is installed on this machine.Release
Version metadata is at 1.7.0 (
package.json, the Dockerfile'sorg.opencontainers.image.versionlabel,CHANGELOG.md). Thev1.7.0tag is deliberately not created on this branch:.github/workflows/docker-publish.ymlfires on any pushedv*.*.*tag and publishes to GHCR as both1.7.0andlatest, and every previous tag in this repo sits on the merge commit onmain. Tag after merging.🤖 Generated with Claude Code
https://claude.ai/code/session_011JYNx69xztWN2qDLe5m1aS