Add facilitator-server Postgres e2e CI pipeline#34
Conversation
📝 WalkthroughWalkthroughAdds a CI job and Bun-based end-to-end tests for the facilitator-server; makes PostgreSQL optional at startup via TRACKING_ALLOW_IN_MEMORY_FALLBACK and updates tracking initialization to accept an onTrackingError callback. Adds key-normalization helpers and tests for E2E private keys. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Runner
participant Server as Facilitator Server
participant DB as PostgreSQL
participant HTTP as HTTP Endpoint
participant Track as Tracking System
CI->>Server: spawn server (env: DATABASE_URL, PORT, TRACKING_ALLOW_IN_MEMORY_FALLBACK, OTEL, EVM_*)
activate Server
Server->>DB: attempt connection & run migrations
activate DB
alt migrations succeed
DB-->>Server: ready
Server->>Track: init PostgreSQL-backed tracking
else migrations fail
DB-->>Server: error
Server->>Server: check TRACKING_ALLOW_IN_MEMORY_FALLBACK
alt fallback enabled
Server->>Track: init in-memory tracking
else fallback disabled
Server-->>CI: exit with error
end
end
deactivate DB
CI->>DB: poll DB readiness
CI->>HTTP: poll /supported until 200
CI->>HTTP: POST /verify with invalid payload
HTTP-->>Server: request received
Server->>Track: record request (path=/verify, status=400)
Track->>DB: insert record (if PG-backed)
DB-->>CI: query finds /verify row
CI->>Server: terminate process
deactivate Server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@examples/facilitator-server/src/index.ts`:
- Around line 71-79: The current log under the condition where pgClient is falsy
but DATABASE_URL is set emits a confusing message about setting
TRACKING_ALLOW_IN_MEMORY_FALLBACK=true; update that console.log to clearly state
that in-memory tracking is active because TRACKING_ALLOW_IN_MEMORY_FALLBACK is
enabled (i.e., the explicit fallback was allowed). Locate the block checking
pgClient and DATABASE_URL and change the message for the case where DATABASE_URL
is truthy and pgClient is falsy to something like "Resource tracking: In-memory
(DB init failed; TRACKING_ALLOW_IN_MEMORY_FALLBACK=true enabled fallback)" so it
accurately reflects that TRACKING_ALLOW_IN_MEMORY_FALLBACK is already true.
🧹 Nitpick comments (2)
examples/facilitator-server/tests/e2e-ci.ts (2)
80-97: Verifynodeavailability vs. usingbunto run the server.The server is spawned via
["node", "dist/index.js"]while the test itself runs under Bun. This works on GitHub Actions (ubuntu-latest ships with Node pre-installed), and aligns with the#!/usr/bin/env nodeshebang in the source. However, if anyone runs this test locally without Node installed (e.g., in a Bun-only setup), it will fail without a clear error message.Consider documenting this dependency or falling back to
bun run dist/index.jsif Node isn't available — though this is low-priority since the test is clearly CI-oriented.
55-73:waitForRecordquery will throw (not retry) on SQL errors.Unlike
waitForDatabaseandwaitForServer,waitForRecorddoes not wrappool.queryin a try/catch. If theresource_call_recordstable doesn't exist yet (e.g., a slow migration), the query will throw immediately instead of retrying. In practice this is unlikely sincewaitForServeralready gates on server readiness (which implies migrations completed), but the inconsistency in error handling across the threewaitFor*functions is worth noting.Proposed fix for consistency
while (Date.now() < deadline) { - const result = await pool.query<{ count: string }>( - `SELECT COUNT(*) AS count - FROM resource_call_records - WHERE path = '/verify' AND response_status = 400` - ); - - if (Number(result.rows[0]?.count ?? "0") > 0) { - return; + try { + const result = await pool.query<{ count: string }>( + `SELECT COUNT(*) AS count + FROM resource_call_records + WHERE path = '/verify' AND response_status = 400` + ); + + if (Number(result.rows[0]?.count ?? "0") > 0) { + return; + } + } catch { + // Table may not exist yet if migrations are still running. } await sleep(300);
| if (pgClient) { | ||
| console.log(`Resource tracking: PostgreSQL (Drizzle)`); | ||
| } else if (DATABASE_URL) { | ||
| console.log( | ||
| `Resource tracking: In-memory (DB init failed; set TRACKING_ALLOW_IN_MEMORY_FALLBACK=true to allow this explicitly)` | ||
| ); | ||
| } else { | ||
| console.log(`Resource tracking: In-memory (set DATABASE_URL for persistence)`); | ||
| } |
There was a problem hiding this comment.
Misleading log message when in-memory fallback is active.
When execution reaches Line 73, TRACKING_ALLOW_IN_MEMORY_FALLBACK is necessarily true (otherwise the process would have exited at Line 48). The message telling the user to "set TRACKING_ALLOW_IN_MEMORY_FALLBACK=true to allow this explicitly" is confusing because they already did.
Proposed fix
} else if (DATABASE_URL) {
console.log(
- `Resource tracking: In-memory (DB init failed; set TRACKING_ALLOW_IN_MEMORY_FALLBACK=true to allow this explicitly)`
+ `Resource tracking: In-memory (DB init failed; running with in-memory fallback)`
);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (pgClient) { | |
| console.log(`Resource tracking: PostgreSQL (Drizzle)`); | |
| } else if (DATABASE_URL) { | |
| console.log( | |
| `Resource tracking: In-memory (DB init failed; set TRACKING_ALLOW_IN_MEMORY_FALLBACK=true to allow this explicitly)` | |
| ); | |
| } else { | |
| console.log(`Resource tracking: In-memory (set DATABASE_URL for persistence)`); | |
| } | |
| if (pgClient) { | |
| console.log(`Resource tracking: PostgreSQL (Drizzle)`); | |
| } else if (DATABASE_URL) { | |
| console.log( | |
| `Resource tracking: In-memory (DB init failed; running with in-memory fallback)` | |
| ); | |
| } else { | |
| console.log(`Resource tracking: In-memory (set DATABASE_URL for persistence)`); | |
| } |
🤖 Prompt for AI Agents
In `@examples/facilitator-server/src/index.ts` around lines 71 - 79, The current
log under the condition where pgClient is falsy but DATABASE_URL is set emits a
confusing message about setting TRACKING_ALLOW_IN_MEMORY_FALLBACK=true; update
that console.log to clearly state that in-memory tracking is active because
TRACKING_ALLOW_IN_MEMORY_FALLBACK is enabled (i.e., the explicit fallback was
allowed). Locate the block checking pgClient and DATABASE_URL and change the
message for the case where DATABASE_URL is truthy and pgClient is falsy to
something like "Resource tracking: In-memory (DB init failed;
TRACKING_ALLOW_IN_MEMORY_FALLBACK=true enabled fallback)" so it accurately
reflects that TRACKING_ALLOW_IN_MEMORY_FALLBACK is already true.
1 participant
This adds a dedicated facilitator-server e2e job in CI with a Postgres service.\nIt builds core + facilitator-server, starts the real server, exercises /verify, and asserts tracking rows persist in resource_call_records.\nIt also makes DB fallback behavior explicit in server startup so migration failures are visible.
Summary by CodeRabbit
New Features
Improvements
Tests & CI