fix: handle idle pool client errors in the PostgreSQL sink#1583
Draft
dcoric wants to merge 50 commits into
Draft
fix: handle idle pool client errors in the PostgreSQL sink#1583dcoric wants to merge 50 commits into
dcoric wants to merge 50 commits into
Conversation
Stub modules for the upcoming Postgres sink adapter. No behavior yet — each adapter file is a placeholder so subsequent commits can land each concern (helper, pushes, repos, users) in isolation. Refs finos#1497
Adds a third `oneOf` entry for the `database` definition in the JSON schema and regenerates the TypeScript config types. `connectionString` is optional at the schema level so an env-var fallback can supply it at runtime (added in a follow-up commit). Refs finos#1497
Adds `GIT_PROXY_POSTGRES_CONNECTION_STRING` to `serverConfig` and wires the postgres branch of `getDatabase()` to populate `connectionString` from it when the user config omits one. Mirrors the existing pattern used for `GIT_PROXY_MONGO_CONNECTION_STRING`. Refs finos#1497
Documents the new sink type in the shipped default config. Disabled by default so the `fs` backend continues to be selected unless an operator explicitly enables postgres. Refs finos#1497
Runtime deps for the new PostgreSQL sink adapter: - `pg` — node-postgres client + Pool, used by the adapter modules. - `connect-pg-simple` — express-session store backed by Postgres, used to persist UI sessions when the postgres sink is active. - `@types/pg`, `@types/connect-pg-simple` — TypeScript definitions. Refs finos#1497
Implements the foundation shared by the postgres adapter modules: - `connect()` lazily constructs a `pg.Pool` from the configured connection string and runs an idempotent `CREATE TABLE IF NOT EXISTS` bootstrap exactly once per process. All adapter modules acquire the pool through this function, so the schema is in place before any query is executed against `users` / `repos` / `pushes`. - `query()` is a thin convenience wrapper that awaits `connect()` and delegates to `pool.query`. - `resetConnection()` tears down the pool and bootstrap latch — used by the integration test harness between suites. - `getSessionStore()` returns a `connect-pg-simple` store bound to the same pool. Per issue finos#1497 it MUST NOT silently return undefined when postgres is the active sink (express-session would silently fall back to MemoryStore), so a missing connection string throws instead. The schema covers the three application tables plus the indexes used by `getPushes` (timestamp DESC) and `getRepo` (name lookup). The session table is left to `connect-pg-simple` via `createTableIfMissing: true`. Refs finos#1497
Implements the `Sink` push methods against the `pushes` table: - `getPushes`: filters by the same keys the mongo backend supports (error/blocked/allowPush/authorised/canceled/rejected/type) via a small allow-list mapping, then sorts `ORDER BY timestamp DESC` to preserve current backend ordering (issue finos#1497 must-fix). - `getPush` / `deletePush`: lookups by `id` PK. - `writeAudit`: upsert on `id` with the full Action serialized into the `data` JSONB column and the projection columns kept in sync. Throws `Invalid id` to match mongo behaviour. - `authorise` / `cancel` / `reject`: read-modify-write through `getPush` + `writeAudit`, identical to the mongo flow. `reject` assigns `action.rejection = rejection` so the persisted payload shape (reason / reviewer / timestamp) matches the existing backends. The Action class is reconstructed from the `data` JSONB via the existing `toClass` helper. Refs finos#1497
Implements the `Sink` user methods against the `users` table:
- `findUser` / `findUserByEmail` / `findUserByOIDC`: lower-case the
lookup keys to match the mongo and fs case-insensitivity behaviour.
- `getUsers`: optional username / email filters with the same
lower-casing; SELECT projects `password` away (matching mongo's
`.project({ password: 0 })`).
- `createUser`: insert with lower-cased username / email.
- `deleteUser`: delete by lower-cased username.
- `updateUser`: dynamic SET-builder that mirrors mongo's partial
upsert. Identity is by `_id` when supplied, otherwise by `username`;
if no matching row exists when keyed on username, a new row is
inserted so callers can patch-or-create without two round trips.
`_id` is exposed as an opaque string (UUID rendered as text) so the
HTTP/UI contract is unchanged versus the mongo backend (which renders
ObjectId via `.toString()`).
Refs finos#1497
Implements the `Sink` repo methods against the `repos` table. Permissions (`canPush` / `canAuthorise`) are stored as a single JSONB column matching the existing mongo/fs shape, with a TODO marker pointing at a future migration to a normalized `repo_users` join table (open question called out in issue finos#1497). Notable details: - `addUser*` use `jsonb_set` + a DISTINCT subquery so re-adding an existing user is a no-op, matching the fs adapter's `includes` guard. - `removeUser*` use `coalesce(..., '[]'::jsonb)` around the `array_agg` filter so that removing the last user leaves the array as `[]`, not `null` — issue finos#1497 explicitly requires this and the reader path additionally defaults `null` arrays to `[]` for belt-and-braces resilience against legacy rows. - `getRepos` accepts the same query keys as the mongo backend (name / project / url) with the same lower-casing on `name`. - `createRepo` returns the row with `_id` populated (UUID rendered as text), matching the mongo backend's contract. Refs finos#1497
Adds the `postgres` branch to the runtime `start()` selector so a sink config of `type: 'postgres'` resolves to the new adapter modules, and re-exports the full `Sink` surface from `src/db/postgres/index.ts`. The `getSessionStore` return type on the `Sink` interface and on the top-level `src/db/index.ts` re-export is widened from `MongoDBStore | undefined` to `MongoDBStore | Store | undefined`, where `Store` is the express-session base class — `connect-pg-simple` extends it. This keeps the existing mongo / fs callers type-compatible. Refs finos#1497
Per issue finos#1497 must-fix: when the active sink is one that promises a persistent session store (currently `mongo` or `postgres`), `db.getSessionStore()` returning undefined must NOT silently fall through to express-session's default `MemoryStore` — that store loses sessions on every restart and is unsafe in any multi-process deployment. `createApp` now resolves the store before registering the session middleware and throws if a persistent backend produced `undefined`. The `fs` backend is unaffected: it has always returned `undefined` deliberately, and falling back to MemoryStore there matches existing single-node-only fs semantics.
Mocks the `query` export from the postgres helper so the suite runs without a live database. Covers: - `getPushes` ordering — asserts the generated SQL contains `ORDER BY timestamp DESC` (issue finos#1497 must-fix). - `getPushes` column translation — `allowPush` filter maps to the `allow_push` snake_case column. - `getPushes` unknown filter keys are ignored (no spurious WHERE). - `getPush` returns null when the row is absent. - `writeAudit` throws `Invalid id` for non-string ids (mongo parity). - `writeAudit` upserts via `ON CONFLICT (id) DO UPDATE`. - `reject` writes a serialized Action into the `data` JSONB column with the `rejection` field populated — confirming the payload shape matches the existing backends. - `reject` throws when the push is missing.
Covers behaviour-critical paths:
- case insensitivity: findUser / findUserByEmail / createUser /
deleteUser all lower-case their lookup or stored values (parity
with the mongo and fs adapters).
- getUsers omits `password` from the projection (mirrors mongo's
`.project({ password: 0 })`).
- updateUser dispatches on `_id` vs `username`, and when the
username-keyed UPDATE matches nothing it falls back to INSERT —
this is the upsert semantics issue finos#1497 calls for.
- updateUser throws when given neither `_id` nor `username`.
Targets the parity invariants called out in issue finos#1497: - `getRepoById` defaults a NULL `users` JSONB to empty arrays — guards against legacy or partial rows and matches the fs/mongo contract. - `getRepo` lower-cases the lookup name. - `createRepo` serialises the default `{canPush:[],canAuthorise:[]}` into the JSONB column and stamps the generated `_id` back onto the returned object. - `addUserCanPush` lower-cases the user value before storing. - `removeUserCanPush` and `removeUserCanAuthorise` emit a SQL fragment that wraps the filtered array in `coalesce(..., '[]'::jsonb)`, so the array remains `[]` when the last user is removed — this is the explicit must-fix from the issue, and an end-to-end check sits in the integration suite.
Mocks the `pg` Pool and `connect-pg-simple` constructors so the suite can exercise the helper without a real database. Covers: - `connect()` is concurrency-safe: many parallel calls share one Pool and run the bootstrap SQL exactly once. - the bootstrap SQL creates `users`, `repos`, and `pushes` (assertion via regex against the inlined statement). - bootstrap failure does not permanently latch the helper: the next `connect()` retries instead of returning the rejected promise. - `query()` surfaces the helpful error message when the configured connection string is missing. - `getSessionStore()` throws (not returns undefined) when the connection string is missing — the explicit must-fix from issue finos#1497 to prevent a silent MemoryStore fallback. - `getSessionStore()` constructs the `connect-pg-simple` store with `createTableIfMissing: true` and shares the helper's pool. Full suite: 791 unit tests passing (+27 new), zero regressions.
Adds the scaffolding for postgres-backed integration tests: - `vitest.config.integration.postgres.ts`: separate vitest config that scopes `include` to `test/db/postgres/**/*.integration.test.ts`, sets `RUN_POSTGRES_TESTS=true`, points `CONFIG_FILE` at the dedicated postgres test config, and uses a single-fork pool so the lazy pg.Pool is shared across the suite. Mirrors the shape of `vitest.config.integration.ts` for mongo. - `test/setup-integration-postgres.ts`: connects a `pg.Client`, truncates the app tables (and the connect-pg-simple `session` table if it exists) between tests, drops them in `afterAll`, and calls `resetConnection()` + `invalidateCache()` so each test sees a fresh helper state. - `test-integration.postgres.proxy.config.json`: minimal config with a single enabled postgres sink and local auth, so `getDatabase()` resolves to postgres without the default fs entry winning first. - `package.json`: adds `npm run test:integration:postgres`. No suites yet — added in the next commit.
Parity with the mongo pushes integration suite, gated on RUN_POSTGRES_TESTS=true (set automatically by vitest.config.integration.postgres.ts). The suite is skipped in normal `npm test` runs and only executes against a real Postgres in the dedicated `npm run test:integration:postgres` task. The added test that goes beyond the mongo parity: - `getPushes` returns results in descending timestamp order across three rows with deliberately distinct timestamps — exercising the must-fix ordering requirement end-to-end against a real database. Otherwise the assertions mirror the existing mongo integration suite verbatim so backend parity is verifiable side-by-side.
Parity with the mongo users integration suite, gated on RUN_POSTGRES_TESTS=true. Mirrors the same case-insensitivity and filtering assertions, plus one additional test exercising the upsert-on-username path through `updateUser` end-to-end (the mongo adapter has this via its `upsert: true`, our postgres adapter implements it as an `UPDATE … WHERE username` fallback to `INSERT`). `getUsers` asserts `password` is `null` in list responses rather than `undefined`: the postgres SELECT projects `NULL::text AS password`, which round-trips as `null` rather than being elided from the JSON shape — semantically equivalent to mongo's omission for the API consumers.
Parity with the mongo repo integration suite, gated on RUN_POSTGRES_TESTS=true. The permission-JSONB block is the centrepiece — it exercises the explicit issue finos#1497 must-fix end-to-end against a real database: - starts with empty arrays in the JSONB column. - adding a user is deduplicated (re-adding does not double-insert). - removing the *last* user leaves the array as `[]`, not `null`. - the invariant applies symmetrically to `canAuthorise`. - removing one user from a multi-user list keeps the rest intact. Skipped without postgres available: full unit suite is 791 passing, 90 skipped (45 mongo + 18 postgres-pushes + 14 postgres-users + 13 postgres-repo), zero failures, zero regressions.
Adds a `postgres:16` service container to the `build-ubuntu` job and a new `PostgreSQL Integration Tests` step that runs `npm run test:integration:postgres` against it. The service uses the default `postgres` superuser with database `git_proxy_test`, matching the connection string our adapter and test harness default to. Per the issue's "Open Questions" section, a single Postgres version is sufficient for the initial lane; a broader matrix can follow once the backend has soaked in. Refs finos#1497
Documents the new `postgres` backend in the `sink` section of the architecture reference: - Lists `postgres` as a supported sink alongside `fs` and `mongo`. - Shows the minimal config block. - Documents the `GIT_PROXY_POSTGRES_CONNECTION_STRING` env-var fallback. - Calls out the v1 limitations explicitly (no migration tooling, no AWS RDS IAM auth, JSONB permissions, no split PG env vars, fail- loudly on missing connection string). Refs finos#1497
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
+ Coverage 85.51% 85.94% +0.42%
==========================================
Files 83 88 +5
Lines 7877 8367 +490
Branches 1312 1408 +96
==========================================
+ Hits 6736 7191 +455
- Misses 1114 1149 +35
Partials 27 27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add an on() method to the pg Pool test double so the helper can register event listeners on the pool without the mock throwing. No behaviour change yet; this just prepares the stub for the upcoming error-listener test.
node-postgres pools emit 'error' when an idle client fails (for example when the backend drops the connection). With no listener attached node treats it as an uncaught exception and crashes the process. Register a listener in ensurePool() that logs the error and lets the pool recycle the client instead. Resolves finos#1562
Cover that ensurePool() attaches an 'error' handler to the pg Pool, so a regression that drops the listener (and reintroduces the crash-on-idle risk) is caught.
Invoke the registered listener with a representative idle-client error and assert it logs via console.error and does not throw, confirming a backend hiccup is swallowed rather than propagated as an uncaught exception.
dcoric
force-pushed
the
feat/postgres-pool-error-handler
branch
from
33fd225 to
38d9f84
Compare
Mirrors the publicKeys array the mongo/fs backends persist on user documents. The idempotent ALTER covers databases bootstrapped before this column existed.
Upstream extended the User constructor with a publicKeys parameter ahead of _id, which silently shifted the _id argument in rowToUser — fix the call and surface public_keys on reads, inserts and the dynamic updateUser SET builder. JSONB params are stringified explicitly since node-postgres would otherwise serialise JS arrays as Postgres arrays.
Adds findUserBySSHKey, addPublicKey, removePublicKey and getPublicKeys so the postgres sink satisfies the extended Sink interface. Semantics match the mongo backend: a key registered to another user raises DuplicateSSHKeyError, re-adding a key or fingerprint the user already holds is rejected, and removals of unknown fingerprints are no-ops.
Covers add/find/remove round-trips, the cross-user and same-user duplicate guards, the empty-array invariant after the last key is removed, and seeding keys through createUser.
The vitest config and the integration proxy config both hardcoded postgres:postgres, so an exported GIT_PROXY_POSTGRES_CONNECTION_STRING was silently ignored. Keep the default as a fallback only and let the sink resolve the connection string from the environment.
An empty patch (e.g. only _id supplied) produced an empty SET list and a SQL syntax error. Throw a descriptive error instead so future handlers copying this builder don't inherit the failure mode.
The email column was NOT NULL UNIQUE, so the second user synced without an email (the AD mail attribute is optional) collided on the unique constraint. Relax the column to nullable with a best-effort partial unique index, matching the permissive mongo/fs backends, and collapse the updateUser fallback into a single atomic upsert so a concurrent insert of the same username can't drop the update. Only the supplied fields are merged on conflict, preserving the partial- update semantics.
Review feedback on finos#1532: the codebase is the source of truth, so comments should argue from the existing mongo/NeDB behavior rather than cite the originating issue. Drops the stale repo_users TODO (implemented on a follow-up branch) and the issue references in code comments, CI config and test names.
Review feedback on finos#1532: the cached git-source recovery isn't part of the postgres sink. The change and its tests now live on fix/configloader-detached-head unchanged.
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.
Description
Adds an idle-client error handler to the PostgreSQL connection pool.
node-postgrespools emit an'error'event when an idle client fails (for example when the database drops the connection). With no listener attached, Node treats this as an uncaught exception and crashes the process. This registers a listener inensurePool()that logs the error and lets the pool recycle the client.Related Issue
Resolves #1562
Checklist
General
Documentation
Configuration
config.schema.json) was modified: (not modified in this PR)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)