Repository navigation
Conversation
There was a problem hiding this comment.
ℹ️ No critical issues — this is a clean, well-documented feature with strong cross-dialect test coverage. One inline nit and a couple of minor observations below.
Reviewed changes
- Atomic owner bootstrap (
fix(auth)) —findOrCreateUsernow promotes the first user with a single conditionalUPDATEguarded bynot exists, closing the concurrent-first-login race that could leave an instance ownerless, andHeadplaneDb.dispose()releases the SQLite handle (or drains the pg pool) on shutdown. HeadplaneDbbundle (refactor(db)) —createDbClientreturns{ client, tables, dialect, dispose };auth.tsandhp-agent.tsunpack it once, so query bodies only changeusers→tables.users.server.databaseconfig (feat(config)) — new block for sqlitepath/ postgresurlorhost/name/user,ssl_mode,max_connections; omitting it preserves the historical<data_path>/hp_persist.db.- PostgreSQL backend (
feat(db)) —schema.postgres.ts, migrations underdrizzle/postgres/, and a partial unique index enforcing a single owner. - SQLite→Postgres copy helper (
feat(db)) —scripts/db-copy.tswith--dry-run/--allow-nonempty, plus contract and copy test suites running against real PostgreSQL 17.
ℹ️ Concurrent first login can surface a 500 on PostgreSQL
The single-owner guarantee is correct: the partial unique index on role = 'owner' makes two owners impossible, and the conditional UPDATE handles the SQLite case. The remaining rough edge is that under a genuinely simultaneous first pair of logins on PostgreSQL, the NOT EXISTS guard evaluates before the other racer has committed, so both UPDATEs attempt to promote — and the index makes the loser throw 23505, which findOrCreateUser does not catch. The result is a one-off failed login for one of the two users, not data corruption; they retry and become a member. This is a once-per-install-lifetime race, so leaving it as-is is defensible, but it is worth a conscious decision.
Technical details
# Losing owner-bootstrap racer errors on PostgreSQL
## Affected sites
- app/server/web/auth.ts — the conditional owner-promotion UPDATE in findOrCreateUser does not tolerate a unique_violation losing the race
## Required outcome
- Decide whether a concurrent-first-login should surface as a 500 (current) or resolve cleanly (e.g. catch 23505 and treat it as "someone else became owner")
## Open questions for the human
- Is failing the losing racer acceptable given the race requires two simultaneous first logins on a brand-new instance?
ℹ️ Nitpicks
scripts/db-copy.ts--dry-runstill runspgMigrateon the target and creates the tables before short-circuiting, so the "without writing anything" wording indocs/configuration/database.mdis slightly inaccurate — it writes schema, just no rows.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
c2139c7 to
f4de526
Compare
There was a problem hiding this comment.
Important
The pnpmDeps hash in nix/package.nix is now a placeholder, which will break nix build and the flake check.
Reviewed changes
nix/package.nixpnpmDeps hash (chore(nix)) — replaced the pre-fetched dependency hash with a placeholder (sha256-AAAA…) to be filled in from the value the build reports. The branch was also rebased onto a newermain(v0.7.1,72ea6aa), pulling in unrelated dependency and changelog updates.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
f4de526 to
94e4b4a
Compare
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the
pnpmDepsfix is clean.
Reviewed changes
nix/package.nixpnpmDepshash (chore(nix)) — replaced the placeholder hash with the real fixed-output derivation value from the build, resolving thenix build/ flake check breakage flagged in the prior review.
One earlier minor thread is still open and unchanged by this commit — AssertUserRowsMatch only pins users, so authSessions/hostInfo row shapes remain guarded only by the contract tests (existing thread on app/server/db/client.server.ts). That's a documentation nit or two extra Exact assertions, not a blocker; leaving it for the author to resolve, which is why I'm not approving yet.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
Review feedback on tale#611. The `HeadplaneTables` comment claimed the assertions pinned both schemas to identical row shapes, but only `users` was actually asserted. A drift in `auth_sessions` or `host_info` — a nullable mismatch, or a timestamp inferring as `string` — would have compiled clean and surfaced only when running against PostgreSQL. Adds the two missing assertions using the record types schema.ts already exports. Verified they bite: making `expires_at` nullable on the PostgreSQL side now fails typecheck rather than passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Row shapes pinned for every table (
refactor(db)) — addedAssertAuthSessionRowsMatchandAssertHostInfoRowsMatchalongside the existingAssertUserRowsMatch, so the SQLite/PostgreSQL schema drift is now caught at compile time for all three tables rather than onlyusers. TheHeadplaneTablesdoc comment was updated to match. This resolves the last open Pullfrog thread;typecheckandlintboth pass (the one lint warning is pre-existing inapp/utils/live-data.tsx).
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
|
Gently up ! |
There was a problem hiding this comment.
Important
The upstream v0.8.1 merge reverted the pg-aware pnpmDeps hash, so the nix job in build.yaml (nix flake check) fails on this branch.
Reviewed changes
- Merged upstream
main(775282de) —git diff 5f93d89..775282deequals the PR's net contribution (25 files, +2126/−85), so no upstream change was dropped; the postgres refactor was re-applied cleanly on top of upstream'sauth.ts/hp-agent.tsedits, andtypecheck,lint(one pre-existing warning) and 279 unit tests pass against the merged tree. - Inherited dependency bump —
drizzle-orm/drizzle-kitmoved from1.0.0-beta.21to1.0.0-rc.4;drizzle-kit checkand a no-opdrizzle-kit generateboth confirmschema.postgres.tsstill matches the committeddrizzle/postgresmigration. (Theintegration:dbsuite could not be run here — no docker — so the rc.4 bump was verified structurally rather than by executing those tests.)
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
nix/package.nixpnpmDepshash (chore(nix)) — replaced the non-pghash inherited from thev0.8.1merge (sha256-n+Uv…) withsha256-Hh3avME6MiGkJSoHgEJ6uIdBkDbmPmRA89mdyl8CHwY=. That is exactly the value the failed CI "Nix Flake Check" on the parent commit775282dereported (specified: n+Uv…/got: Hh3avME6…), so it is the correct fixed-output hash for the merged lockfile. This resolves the prior review's important finding.
The flake check on this head was still in_progress when this review was written, but the applied value is the CI-derivable one for this lockfile, and the change is a single hash line with no behavioral surface.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
…down Two independent fixes, neither dependent on the PostgreSQL work in tale#605. **Concurrent first logins leave the instance with no owner** `findOrCreateUser` inserted the new user, then counted rows in a separate statement, and promoted to owner only when the count was exactly 1. Two people signing in at the same moment interleave at the `await` boundaries, so both can observe a table that already holds two rows. Neither promotes itself, and the instance ends up with no owner and nobody able to administer it. The guard now lives inside the UPDATE, so the check and the write are one statement. This also removes the `count(*)` projection entirely. Note the condition tests for the absence of an owner rather than for a single user row. The two agree on a fresh install, and only the former is decidable in a single statement. **The database handle was never closed** Nothing disposed the database, so a graceful shutdown left the SQLite handle open with its journal un-finalized. `closeDbClient` is registered first in the disposer list so it runs last, after everything that might still query during shutdown. Adds a regression test that fails on the previous implementation (it produced zero owners, not two). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Groundwork for tale#605. No behaviour change, no new dependency, still SQLite only. `createDbClient` now returns a `HeadplaneDb` — the drizzle client, the tables it can execute against, its dialect, and a `dispose()`. Callers reach tables through the bundle instead of importing the schema module directly, so the schema travels with the connection that can execute it rather than being fixed at import time. That is the single thing standing between here and a second dialect. `auth.ts` and `hp-agent.ts` unpack it once (`const { client: db, tables } = opts.db`), which keeps every query body identical apart from `users` becoming `tables.users`. No new layer, no indirection at the call sites. `NodeSQLiteDatabase` no longer appears outside `db/client.server.ts`, so adding Postgres later touches the client module and the schema, not the query code. `wrapSqliteClient` is exported so the unit test helper builds in-memory databases through the same path the application uses, rather than a parallel one. The dispose logic moves inside the bundle, replacing the standalone `closeDbClient` added in the previous commit.
Groundwork for tale#605, and a small feature on its own: the database can now live somewhere other than `server.data_path`, which is useful when data_path sits on a volume chosen for cache-like data. Omitting the block reproduces the historical location exactly, so upgrading changes nothing and no existing database moves. `createDbClient` now takes a resolved `DatabaseConfig` discriminated union rather than a bare path, so adding a dialect adds a member instead of changing the signature. Scoped to `type: "sqlite"` deliberately. Accepting `"postgres"` here while the client cannot open one would ship config that validates and then fails at runtime; the union widens in the PR that can actually honour it. Note `server.database.path` is a plain `string`, not `string.lower` like the older path fields. That keyword rewrites the value it validates, so a path containing a capital letter silently resolves somewhere else on a case-sensitive filesystem. There is a test pinning this. The same issue affects `data_path` and the other `string.lower` paths today — happy to open a separate issue for that if you'd like it fixed. Adds docs/configuration/database.md, covering the move procedure and what is actually lost if the database is (the answer is roles and sessions, recoverable but tedious).
Closes the implementation half of tale#605. SQLite stays the default; omitting `server.database` changes nothing for an existing install. **A design concession I want to flag before you read the diff.** The plan was for the query layer to be typed against a union of the two dialects' clients, so both would be checked by the compiler. That does not work here. TypeScript cannot call a method on a union of generic signatures — every `.select()` and `.insert()` fails with "none of those signatures are compatible with each other". The only union that compiles is one containing `any`, which would erase type checking from every query in the codebase. So queries stay typed against the SQLite client and the PostgreSQL client is cast to it once, in `createPostgresClient`. The alternative was a repository port with a separate implementation per dialect, which is the large refactor you have said you do not want, for a three-table schema. What makes the cast safe is not the type system but `tests/integration/db`, which runs the same query code against both engines: SQLite in memory and a real PostgreSQL 17 under testcontainers. 25 tests, covering the divergences that types would not have caught anyway — timestamps surfacing as `Date` from both an integer epoch and a timestamptz, JSON round-tripping as an object from both text and jsonb, nullable columns, upserts, and the owner bootstrap. The API surface those queries use is identical on both dialects, and I checked that none of the SQLite-only methods (`.get()`, `.all()`, `.run()`) appear in shared query code. The cast is documented where it happens. **Owner bootstrap under READ COMMITTED.** The single-statement guard added earlier is atomic under SQLite's serialized writes but not under PostgreSQL: two concurrent first logins update different rows, take no conflicting row locks, both see no owner, and both commit. The PostgreSQL schema carries a partial unique index on `role = 'owner'` as the real guarantee, with a test asserting it bites. It is created unconditionally because PostgreSQL support is new — no existing database could already hold two owners. The SQLite schema deliberately does not get it: that migration would fail to apply on any install where the race has already fired. Also: the migrator differs by dialect (SQLite synchronous, PostgreSQL returning a promise), and a failed migration drains the pool before rethrowing so a failed start does not hang the process. Connection strings are redacted before they reach the log.
Finishes tale#605. Without this, switching `server.database.type` silently starts you over: every user is recreated on next sign-in with the default role, and whoever signs in first becomes the owner. The docs previously said as much, which is a poor answer for anyone with roles worth keeping. pnpm exec tsx scripts/db-copy.ts \ --from /var/lib/headplane/hp_persist.db \ --to postgres://headplane@10.0.0.5:5432/headplane Carries users, roles, Headscale links, live sessions and cached node info. Signed-in users stay signed in, since sessions come across and the cookie secret is unchanged. Expired sessions are dropped rather than copied. Three things it refuses to do quietly: - Copying into a target that already holds users, which would otherwise fail partway through on a primary key collision and leave it half-written. `--allow-nonempty` opts in. - Copying a source with more than one owner. PostgreSQL enforces a single owner via the partial unique index, and a constraint violation midway is a much worse message than naming the offending accounts up front. Reachable on a database hand-repaired around the owner-bootstrap race. - Printing the connection string with its password in it. `--dry-run` reports what would move without writing. Kept as a standalone script rather than a subcommand so it adds nothing to the server bundle or the startup path. It exports `copyDatabase` so the tests drive it directly; six integration tests run it against real PostgreSQL, and I also ran the documented command end to end to check the CLI wrapper, the refusal paths and their exit codes.
Review feedback on tale#611. The `HeadplaneTables` comment claimed the assertions pinned both schemas to identical row shapes, but only `users` was actually asserted. A drift in `auth_sessions` or `host_info` — a nullable mismatch, or a timestamp inferring as `string` — would have compiled clean and surfaced only when running against PostgreSQL. Adds the two missing assertions using the record types schema.ts already exports. Verified they bite: making `expires_at` nullable on the PostgreSQL side now fails typecheck rather than passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rebase over the UI and docs overhaul left hp-agent.ts with a broken conflict resolution (a duplicated prune block and a stray closing brace) and dropped several pieces of the PR: the pg dependencies in package.json while the lockfile kept them, the integration:db vitest project, the changelog entries, and the docs wiring. The react-router data import main needs in auth.ts was removed as well, and a stale VitePress config came back. hp-agent.ts is rebuilt from main's current file with the HeadplaneDb refactor applied on top, so main's batched host info upsert is kept. The agent integration harness wraps its SQLite handle the same way the unit test helper does. The database page is listed in the configuration sidebar and rewritten to the Fumadocs conventions. The pnpmDeps hash is regenerated for the merged lockfile. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
58582df to
4b7e85d
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Rebased the branch onto current
main(fc6f6ce) and added4b7e85d, which reintroduced everything the rebase had dropped or broken. - Rebuilt
app/server/hp-agent.tsfrommainwith theHeadplaneDbrefactor applied, restoring the upstream batched host-info upsert and the agent resiliency work that the broken conflict resolution had reverted. - Restored
pg/@types/pginpackage.jsonand stripped the committed conflict markers frompnpm-lock.yaml; regenerated thenix/package.nixpnpmDepshash. - Restored the
integration:dbvitest project, wrapped the agent harness's in-memory SQLite handle withwrapSqliteClient, re-added the changelog entries, the newdocs/configuration/database.mdand its sidebar entry, removed the stale VitePress config, and re-addedauth.ts'sreact-routerdataimport.
Verified at this head: pnpm install --frozen-lockfile, pnpm run typecheck, pnpm run lint (one pre-existing warning in app/utils/live-data.tsx) and pnpm run test:unit (282 passing) are all clean, and CI's Nix Flake Check and Build and Test both pass. The integration:db/integration:agent suites need Docker, which this environment lacks, so they were not run locally.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
Looking at this one now. |

Closes #605.
Adds PostgreSQL as an alternative to the embedded SQLite database. SQLite stays
the default and the zero-config path. Omitting
server.databasechangesnothing for an existing install.
Six commits, each reviewable on its own:
fix(auth): two bugs found while scoping this, unrelated to PostgreSQL.Concurrent first logins could leave the instance with no owner (the count
ran as a statement separate from the insert, so both racers saw two rows and
neither promoted itself), and nothing ever closed the database handle. The
regression test fails on the old code.
refactor(db):createDbClientreturns the connection bundled with itsschema and dialect.
auth.tsandhp-agent.tsunpack it once, so every querybody is unchanged apart from
usersbecomingtables.users.docs(changelog)feat(config):server.database, which also lets the SQLite file livesomewhere other than
data_path.feat(db): the PostgreSQL backend, migrations, and the contract tests.feat(db): a SQLite -> PostgreSQL copy helper.One thing to look at first
The plan was for the query layer to be typed against a union of both dialects'
clients. That does not compile: TypeScript cannot call a method on a union of
generic signatures, and every
.select()fails with "none of those signaturesare compatible with each other". The only union that compiles contains
any,which would erase type checking from every query in the codebase.
So queries stay typed against the SQLite client and the PostgreSQL client is
cast to it once, in
createPostgresClient, with the reasoning written at thedefinition. The alternative was a repository port with one implementation per
dialect. The large refactor you have said you do not want, for a three-table
schema.
What makes the cast safe is
tests/integration/db, which runs the same querycode against both engines: SQLite in memory, and real PostgreSQL 17 under
testcontainers (already a devDependency, already used by the docker and dex
suites). 31 tests, covering the divergences types would not have caught anyway.
Timestamps surfacing as
Datefrom both an integer epoch and atimestamptz,JSON round-tripping as an object from both
textandjsonb, nullable columns,upserts, ownership transfer.
I also checked that no SQLite-only method (
.get(),.all(),.run()) appearsin shared query code, and said so in a comment where the cast lives.
If you would rather have compiler-proven safety than tested safety, say so. But
that means the repository port, and I did not want to spring that on you.
Owner bootstrap under READ COMMITTED
The single-statement guard from commit 1 is atomic under SQLite's serialized
writes. It is not under PostgreSQL: two concurrent first logins update
different rows, take no conflicting row locks, both see no owner, and both
commit. The PostgreSQL schema carries a partial unique index on
role = 'owner'as the real guarantee, with a test asserting it actually rejects a second owner.
It is created unconditionally because PostgreSQL support is new. No existing
database could already hold two owners. SQLite deliberately does not get it:
that migration would fail to apply on any install where the race already fired.
Notes
drizzle/*migrations are untouched; PostgreSQL lives indrizzle/postgres/.promise). A failed migration drains the pool before rethrowing so a failed
start does not hang the process.
server.database.password_pathfollows the existing_pathsecret convention.pg.docs/configuration/database.mdis new. The database was undocumented.Testing
pnpm run typecheck,pnpm run lintcleanpnpm test:unit-> 210 passingpnpm vitest run --project integration:db-> 31 passing against PostgreSQL 17Not included
docs/NixOS-options.mdneeds regenerating fromnix/options.nix(I added theserver.databasesubmodule there). It is built bynixosOptionsDocand I do nothave nix available, so I left the generated file alone rather than hand-editing
build output. Happy for you to regenerate, or I can if you tell me the command
you use.