Bug: Multiple issues in PostgresExternalService change detection setup
Version: @skip-adapter/postgres@0.0.19
1. Race condition in setupPgNotify
open_instances.add(instance) is called before the CREATE FUNCTION / LISTEN / CREATE TRIGGER queries execute:
// Current code (simplified from src/index.ts):
const setupPgNotify = async () => {
if (!this.open_instances.has(instance)) {
this.open_instances.add(instance); // ← Marked as set up HERE
await this.client.query(/* CREATE FUNCTION ... */); // ← But these can fail
await this.client.query(/* LISTEN ... */);
await this.client.query(/* CREATE TRIGGER ... */);
}
};
If any query fails (e.g., permission error, connection timeout), the instance is permanently marked as set up in open_instances, but the trigger/listener was never created. All subsequent calls to setupPgNotify for the same instance skip setup entirely (the has() check passes), leaving a dead channel — no change notifications are ever received for that table.
Fix: Move open_instances.add(instance) to after all three queries succeed, and wrap the block in try/catch.
2. No error handler on pg.Client
The constructor creates a pg.Client and calls .connect(), but never registers an error event handler:
constructor(db_config) {
this.client = new pg.Client(db_config);
// No this.client.on("error", ...) handler
this.client.connect().then(() => { /* ... */ });
}
When the PostgreSQL connection drops (network blip, server restart, idle timeout), the error event fires. Without a handler:
- Node.js may throw an unhandled error (crashes the process)
- Stale entries remain in
open_instances — the instance thinks channels are active, but the underlying connection is dead
- On reconnect,
setupPgNotify skips re-creation because open_instances still contains the old entries
Fix: Add this.client.on("error", ...) that clears open_instances, allowing channels to be re-created on the next call.
3. No accessors for client or open_instances
Both client and open_instances are private with no getters. This makes it impossible to:
- Monitor connection health (
client._connected, client.connection)
- Check which instances are actively set up
- Build diagnostics/health-check endpoints
- Detect the stale-channel scenario described above
Fix: Expose read-only accessors:
getClient(): pg.Client {
return this.client;
}
getOpenInstances(): Set<string> {
return new Set(this.open_instances); // defensive copy
}
Impact
In production, a single PostgreSQL connection blip silently breaks all reactive change detection. The system appears to be running normally (no errors, no crashes), but no table changes propagate through the reactive graph. The only recovery is a full process restart. Without accessors, there is no way to detect this state programmatically.
Current workaround
We maintain a patch file for @skip-adapter/postgres@0.0.19 that:
- Wraps
setupPgNotify queries in try/catch, moves open_instances.add() to after success
- Adds
client.on("error") handler that clears open_instances
- Exposes
getClient() and getOpenInstances() accessors
Expected behavior
open_instances.add() should only be called after all setup queries succeed
pg.Client should have an error event handler that invalidates stale channel state
- Read-only accessors should be available for monitoring and diagnostics
Bug: Multiple issues in
PostgresExternalServicechange detection setupVersion:
@skip-adapter/postgres@0.0.191. Race condition in
setupPgNotifyopen_instances.add(instance)is called before theCREATE FUNCTION/LISTEN/CREATE TRIGGERqueries execute:If any query fails (e.g., permission error, connection timeout), the instance is permanently marked as set up in
open_instances, but the trigger/listener was never created. All subsequent calls tosetupPgNotifyfor the same instance skip setup entirely (thehas()check passes), leaving a dead channel — no change notifications are ever received for that table.Fix: Move
open_instances.add(instance)to after all three queries succeed, and wrap the block in try/catch.2. No error handler on
pg.ClientThe constructor creates a
pg.Clientand calls.connect(), but never registers anerrorevent handler:When the PostgreSQL connection drops (network blip, server restart, idle timeout), the
errorevent fires. Without a handler:open_instances— the instance thinks channels are active, but the underlying connection is deadsetupPgNotifyskips re-creation becauseopen_instancesstill contains the old entriesFix: Add
this.client.on("error", ...)that clearsopen_instances, allowing channels to be re-created on the next call.3. No accessors for
clientoropen_instancesBoth
clientandopen_instancesareprivatewith no getters. This makes it impossible to:client._connected,client.connection)Fix: Expose read-only accessors:
Impact
In production, a single PostgreSQL connection blip silently breaks all reactive change detection. The system appears to be running normally (no errors, no crashes), but no table changes propagate through the reactive graph. The only recovery is a full process restart. Without accessors, there is no way to detect this state programmatically.
Current workaround
We maintain a patch file for
@skip-adapter/postgres@0.0.19that:setupPgNotifyqueries in try/catch, movesopen_instances.add()to after successclient.on("error")handler that clearsopen_instancesgetClient()andgetOpenInstances()accessorsExpected behavior
open_instances.add()should only be called after all setup queries succeedpg.Clientshould have anerrorevent handler that invalidates stale channel state