refactor(proxy): migrate from SQLite to Postgres via Kysely#235
refactor(proxy): migrate from SQLite to Postgres via Kysely#235maciej-konczal wants to merge 2 commits into
Conversation
Replaces better-sqlite3 with Kysely + pg, matching the project-wide "PostgreSQL via Kysely" rule in CLAUDE.md. Adds PROXY_DATABASE_URL with fallback to PLATFORM_DATABASE_URL, removes the SQLite file path default and the proxy-data Docker volume. Tests use kysely-pglite (in-memory PG) following tests/platform/platform-db-test-helper.ts. Behavior change: host-mode bun run dev now requires a Postgres available (Homebrew or Docker). README documents both paths. Docker stack unchanged for users -- PROXY_DATABASE_URL defaults to the existing postgres service. Also wires DB pool cleanup into the SIGTERM/SIGINT shutdown handler via resetProxyDb() so the pool doesn't outlive graceful shutdown. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@maciej-konczal is attempting to deploy a commit to the Finna Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR migrates the proxy service's persistence layer from
Confidence Score: 5/5Safe to merge; the SQLite-to-Postgres migration is complete and correct for the Docker deployment path, with good test coverage via kysely-pglite. The refactor is well-scoped, all async conversions are correct, and the new test suite validates every DB path. Two findings were surfaced — the migrate() function runs DDL without a wrapping transaction, and the POSTGRES_URL fallback can produce a malformed URL in PaaS environments — but neither affects the primary Docker deployment path where PLATFORM_DATABASE_URL is always injected by Compose. packages/proxy/src/db.ts — the migrate() transaction wrapping and POSTGRES_URL fallback construction. Important Files Changed
Sequence DiagramsequenceDiagram
participant Main as main.ts (startup)
participant DB as db.ts (createProxyDb)
participant PG as pg.Pool
participant Client as Hono Route Handler
Main->>DB: createProxyDb(url)
DB->>PG: "new pg.Pool({ connectionString })"
DB->>PG: migrate() [4 DDL statements]
PG-->>DB: ready promise resolves
DB-->>Main: ProxyDB instance
Main->>PG: getMetricsSeed() [seed Prometheus]
PG-->>Main: rows
Client->>DB: checkQuota(userId)
DB->>PG: SELECT user_quotas
DB->>PG: SUM(cost_usd) day
DB->>PG: SUM(cost_usd) month
PG-->>Client: QuotaCheck
Client->>DB: insertUsage(record)
DB->>PG: INSERT api_usage
PG-->>Client: void
Note over Main,PG: SIGTERM / SIGINT
Main->>DB: resetProxyDb()
DB->>PG: "kysely.destroy() -> pool.end()"
PG-->>DB: pool closed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/proxy/src/db.ts:232-259
The `migrate` function issues 4 separate DDL statements without wrapping them in a transaction. CLAUDE.md's atomicity rule is explicit: "2+ related DB writes MUST use a transaction. No exceptions." Without a transaction, a crash or pool error between statements leaves a partially-initialized schema. While `IF NOT EXISTS` guards make a retry recoverable, any concurrent second startup could observe the intermediate state and issue redundant DDL against a half-built schema.
### Issue 2 of 2
packages/proxy/src/db.ts:61-64
**`POSTGRES_URL` fallback constructs an invalid URL when the value already includes a database name.** Most PaaS environments (Heroku, Railway, Neon) set `POSTGRES_URL` as a full connection string that already includes the database component. Appending `/matrixos_platform` to such a value produces a path like `…/existingdb/matrixos_platform`, which the `pg` driver will reject with a connection error. Using `process.env.POSTGRES_URL` directly as the final fallback (without appending anything) avoids this.
Reviews (2): Last reviewed commit: "fix(proxy): single-query usage summary a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await kysely | ||
| .insertInto('user_quotas') | ||
| .values({ | ||
| user_id: userId, | ||
| daily_limit_usd: dailyLimitUsd, | ||
| monthly_limit_usd: monthlyLimitUsd, | ||
| enabled: true, | ||
| }) | ||
| .onConflict((oc) => | ||
| oc.column('user_id').doUpdateSet({ | ||
| daily_limit_usd: dailyLimitUsd, | ||
| monthly_limit_usd: monthlyLimitUsd, | ||
| }), | ||
| ) | ||
| .execute(); | ||
| }, |
There was a problem hiding this comment.
getUsageSummary
getUsageSummary first fetches all distinct user_id values, then fires getUserUsage for each one — which itself issues 3 parallel queries (day, month, all). For N users this is 1 + 3N round-trips to the database pool (which is capped at 10 connections). Under even modest user counts the pool will be saturated on every admin summary call. A single aggregation query grouping by user_id and window (date_trunc) would collapse this to 1–2 queries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/proxy/src/db.ts
Line: 132-147
Comment:
**N+1 queries in `getUsageSummary`**
`getUsageSummary` first fetches all distinct `user_id` values, then fires `getUserUsage` for each one — which itself issues 3 parallel queries (`day`, `month`, `all`). For N users this is 1 + 3N round-trips to the database pool (which is capped at 10 connections). Under even modest user counts the pool will be saturated on every admin summary call. A single aggregation query grouping by `user_id` and window (`date_trunc`) would collapse this to 1–2 queries.
How can I resolve this? If you propose a fix, please make it concise.| try { | ||
| await posthogErrorTracker.shutdown(); | ||
| await Promise.allSettled([ | ||
| posthogErrorTracker.shutdown(), | ||
| resetProxyDb(), | ||
| ]); | ||
| } catch (err: unknown) { | ||
| console.warn('[proxy] Failed during shutdown:', err instanceof Error ? err.message : String(err)); | ||
| } finally { |
There was a problem hiding this comment.
catch branch in shutdown
Promise.allSettled is explicitly designed to never reject — it resolves with an array of settled results regardless of individual promise outcomes. The catch block here will never execute, so any real error from resetProxyDb is silently swallowed into the settled result rather than being warned about. Inspecting the settled results would restore the intended behavior.
| try { | |
| await posthogErrorTracker.shutdown(); | |
| await Promise.allSettled([ | |
| posthogErrorTracker.shutdown(), | |
| resetProxyDb(), | |
| ]); | |
| } catch (err: unknown) { | |
| console.warn('[proxy] Failed during shutdown:', err instanceof Error ? err.message : String(err)); | |
| } finally { | |
| const results = await Promise.allSettled([ | |
| posthogErrorTracker.shutdown(), | |
| resetProxyDb(), | |
| ]); | |
| for (const result of results) { | |
| if (result.status === 'rejected') { | |
| console.warn('[proxy] Failed during shutdown:', result.reason instanceof Error ? result.reason.message : String(result.reason)); | |
| } | |
| } | |
| try { | |
| // intentionally empty — errors already logged above | |
| } catch { | |
| // unreachable | |
| } finally { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/proxy/src/main.ts
Line: 332-339
Comment:
**Unreachable `catch` branch in `shutdown`**
`Promise.allSettled` is explicitly designed to never reject — it resolves with an array of settled results regardless of individual promise outcomes. The `catch` block here will never execute, so any real error from `resetProxyDb` is silently swallowed into the settled result rather than being warned about. Inspecting the settled results would restore the intended behavior.
```suggestion
const results = await Promise.allSettled([
posthogErrorTracker.shutdown(),
resetProxyDb(),
]);
for (const result of results) {
if (result.status === 'rejected') {
console.warn('[proxy] Failed during shutdown:', result.reason instanceof Error ? result.reason.message : String(result.reason));
}
}
try {
// intentionally empty — errors already logged above
} catch {
// unreachable
} finally {
```
How can I resolve this? If you propose a fix, please make it concise.Addresses two review findings on the Postgres migration: - getUsageSummary previously fired 1 + 3N queries (one DISTINCT plus three per user). Collapsed to a single grouped query using FILTER aggregates -- O(1) round-trips regardless of user count, so the 10-connection pool no longer saturates on admin summary calls. - shutdown() wrapped Promise.allSettled in try/catch, but allSettled never rejects so the catch was dead code that silently swallowed resetProxyDb errors. Now iterates settled results and logs each rejected reason individually. New test in tests/proxy/db.test.ts locks in the per-user roll-up behavior so future query rewrites preserve it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
I would hold this PR until the following are addressed. Required before merge:
Related platform wiring check, possibly pre-existing but worth handling while touching platform compose: the platform orchestrator generates Current merge status I see: head |
2 participants
Replaces better-sqlite3 with Kysely + pg, matching the project-wide "PostgreSQL via Kysely" rule in CLAUDE.md. Adds PROXY_DATABASE_URL with fallback to PLATFORM_DATABASE_URL, removes the SQLite file path default and the proxy-data Docker volume. Tests use kysely-pglite (in-memory PG) following tests/platform/platform-db-test-helper.ts.
Behavior change: host-mode bun run dev now requires a Postgres available (Homebrew or Docker). README documents both paths. Docker stack unchanged for users -- PROXY_DATABASE_URL defaults to the existing postgres service.
Also wires DB pool cleanup into the SIGTERM/SIGINT shutdown handler via resetProxyDb() so the pool doesn't outlive graceful shutdown.