Move cross-instance NOTIFY behind DBAdapter.notify#4672
Merged
Conversation
Realm.#notifyFileChange was issuing raw `SELECT pg_notify(...)` SQL against this.#dbAdapter. In the host/browser context the adapter is SQLite, which has no pg_notify function — every write logged `SQLITE_ERROR: no such function: pg_notify`. The host runs a single in-process realm with no peers to notify, so issuing the SQL was also semantically a no-op even when it didn't error. Push the cross-instance broadcast capability down into the existing DBAdapter abstraction: - Add `notify(channel, payload): Promise<void>` to the DBAdapter interface, documented as best-effort cache-coherency. - PgAdapter implements via `SELECT pg_notify($1, $2)`. - SQLiteAdapter implements as a no-op (no pub/sub primitive, no peers). - Realm.#notifyFileChange calls `this.#dbAdapter.notify(...)` and no longer embeds pg-specific SQL. - Update the realm-server listener test to drive notifications via the new method (also gives PgAdapter.notify direct CI coverage). - Add `notify` to the inline DBAdapter mocks in bot-runner / realm-server / runtime-common tests. Surfaced as flaky test-log noise after PR #4660 (CS-10892: realm_file_changes NOTIFY channel) widened the call site without gating SQLite. Original diagnosis and the kind === 'sqlite' guard fix from Hassan Abdel-Rahman on cs-11036-indexer-resume-progress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lukemelia
force-pushed
the
cs-pg-notify-browser-guard
branch
from
f9dcc47 to
8b212d8
Compare
lukemelia
changed the title
…guard # Conflicts: # packages/postgres/pg-adapter.ts # packages/realm-server/tests/realm-file-changes-listener-test.ts
4 tasks
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves cross-instance invalidation notifications behind the DBAdapter abstraction so the host/browser (SQLite) environment no longer attempts to call pg_notify, eliminating SQLITE_ERROR: no such function: pg_notify noise while preserving Postgres behavior.
Changes:
- Added
notify(channel, payload): Promise<void>to theDBAdapterinterface as a best-effort cross-instance broadcast primitive. - Implemented
notifyinPgAdapter(viaSELECT pg_notify($1, $2)) and inSQLiteAdapteras an intentional no-op. - Updated
Realm.#notifyFileChangeand tests/mocks to usedbAdapter.notify(...)instead of embedding raw pg-specific SQL.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime-common/db.ts | Extends DBAdapter with a documented best-effort notify API. |
| packages/runtime-common/realm.ts | Switches file-change broadcasting from raw pg_notify SQL to dbAdapter.notify. |
| packages/postgres/pg-adapter.ts | Implements DBAdapter.notify using parameterized pg_notify. |
| packages/host/app/lib/sqlite-adapter.ts | Implements DBAdapter.notify as a no-op for SQLite/host. |
| packages/realm-server/tests/realm-file-changes-listener-test.ts | Drives notifications through dbAdapter.notify to cover PgAdapter.notify. |
| packages/realm-server/tests/screenshot-card-test.ts | Updates DBAdapter test mock shape to include notify. |
| packages/realm-server/tests/prerender-proxy-test.ts | Updates DBAdapter test mock shape to include notify. |
| packages/runtime-common/tests/run-command-task-shared-tests.ts | Updates DBAdapter test mock shape to include notify. |
| packages/bot-runner/tests/command-runner-test.ts | Updates DBAdapter test mock shape to include notify. |
| packages/bot-runner/tests/bot-runner-test.ts | Updates DBAdapter test mock shape to include notify. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
This is unrelated to your PR, I’ll look more at how it escaped the colour-ignoring changes: 
|
backspace
approved these changes
May 6, 2026
habdelra
added a commit
that referenced
this pull request
May 6, 2026
PR #4672 added a `notify` method to the DBAdapter interface but indexing-event-sink-test.ts (introduced in CS-10930) still constructs mock adapters that don't implement it. Lint flagged TS2741 on the merge of main into this branch. Both mock adapters in this file are recording stubs for `execute`; add a no-op `notify` so the type matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
habdelra
added a commit
that referenced
this pull request
May 6, 2026
The DBAdapter interface gained a notify() method (PR #4672, cs-pg-notify- browser-guard) the same week tests/indexing-event-sink-test.ts was added (CS-10930). Each PR was rebased on a main that didn't see the other, so the merge of both leaves two DBAdapter literals in this test missing the new required method, breaking lint:types in CI for any branch that merges current main. Add async notify() {} to both mock literals (line 18 and line 362). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
habdelra
added a commit
that referenced
this pull request
May 6, 2026
PR #4672 added a `notify` method to the DBAdapter interface but indexing-event-sink-test.ts (introduced in CS-10930) still constructs mock adapters that don't implement it. Lint flagged TS2741 on the merge of main into this branch. Both mock adapters in this file are recording stubs for `execute`; add a no-op `notify` so the type matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
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
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.
Summary
Realm.#notifyFileChangewas issuing rawSELECT pg_notify(...)SQL againstthis.#dbAdapter. In the host/browser context the adapter is SQLite, which has nopg_notifyfunction — every write loggedSQLITE_ERROR: no such function: pg_notify. The host runs a single in-process realm with no peers to notify, so issuing the SQL was also semantically a no-op even when it didn't error.This pushes the cross-instance broadcast capability down into the existing
DBAdapterabstraction:notify(channel, payload): Promise<void>to theDBAdapterinterface, documented as best-effort cache-coherency.PgAdapterimplements viaSELECT pg_notify($1, $2).SQLiteAdapterimplements as a no-op (no pub/sub primitive, no peers).Realm.#notifyFileChangecallsthis.#dbAdapter.notify(...)and no longer embeds pg-specific SQL.PgAdapter.notifydirect CI coverage).notifyto the inlineDBAdaptermocks in bot-runner / realm-server / runtime-common tests.Surfaced as flaky test-log noise after PR #4660 (CS-10892: realm_file_changes NOTIFY channel) widened the call site without gating SQLite. Original diagnosis and the simpler
kind === 'sqlite'guard fix were Hassan's, oncs-11036-indexer-resume-progress; this PR replaces that guard with the adapter-level abstraction.Test plan
no such function: pg_notifylines in the test log.realm-file-changes-listener-test.tsstill passes end-to-end against real Postgres (now exercisingPgAdapter.notify).🤖 Generated with Claude Code