fix(webapp): auto-recover replication services after stream errors#3613
fix(webapp): auto-recover replication services after stream errors#3613ericallam wants to merge 2 commits into
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughThis PR adds configurable error recovery for the runs and sessions replication services. When a logical replication stream fails (e.g., during a database failover), the system can reconnect with exponential backoff, exit to let an external supervisor restart the host, or remain stopped with logging. Environment variables control per-service strategy selection and tuning. The implementation integrates into both services' lifecycle (on error, stream start, and shutdown) and is validated through containerized integration tests that force replication stream failures. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
When the underlying logical-replication client errored (e.g. after a Postgres failover), the runs and sessions replication services logged the error and left the stream stopped. The host process kept running, the WAL backed up, and ClickHouse silently fell behind. Both services now run a configurable recovery strategy on stream errors, defaulting to in-process reconnect with exponential backoff so a fresh self-hosted setup heals on its own: - "reconnect" (default) re-subscribes via the existing subscribe(lastLsn) path with exponential backoff (1s -> 60s cap, unlimited attempts), which re-validates the publication, re-acquires the leader lock, and resumes from the last acknowledged LSN. - "exit" calls process.exit after a short flush window so a host's supervisor (Docker restart=always, systemd, k8s, etc.) can replace the process. - "log" preserves the historical behaviour. Per-service strategy + exit knobs are env-driven via RUN_REPLICATION_ERROR_STRATEGY / SESSION_REPLICATION_ERROR_STRATEGY plus matching *_EXIT_DELAY_MS / *_EXIT_CODE. Reconnect tuning is shared across both services via REPLICATION_RECONNECT_INITIAL_DELAY_MS / _MAX_DELAY_MS / _MAX_ATTEMPTS (0 = unlimited).
Addresses PR review feedback:
- LogicalReplicationClient.subscribe() can throw before its internal
"error" listener is wired up (notably when pg client.connect() fails
mid-failover). The reconnect strategy's catch block only logged, so
recovery silently stopped. Now also calls scheduleReconnect(err) — the
pendingReconnect guard makes it idempotent if an error event was also
emitted.
- Reject negative values for the new replication-recovery env vars and
cap exit codes at 255.
- Convert the new ReplicationErrorRecovery{Deps,} interfaces to type
aliases to match the repo's TypeScript style.
- Tighten the reconnect dep comment to drop a stale "lastAcknowledgedLsn"
reference (the wrapper-tracked resume LSN is what callers actually pass).
- Restore process.exit after service.shutdown() in the exit-strategy
test so a delayed exit timer can't terminate the test worker.
ericallam
force-pushed
the
fix/replication-auto-recover-on-stream-error
branch
from
6f8cc24 to
5ba46ff
Compare
| pendingReconnect = setTimeout(async () => { | ||
| pendingReconnect = null; | ||
| if (isShuttingDown()) return; | ||
|
|
||
| try { | ||
| await reconnect(); | ||
| // Success path is handled by notifyStreamStarted, which fires from | ||
| // the replication client's "start" event after the stream is live. | ||
| } catch (err) { | ||
| // subscribe() can throw without first emitting an "error" event — | ||
| // notably when the initial pg client.connect() fails because Postgres | ||
| // is still unreachable mid-failover. Schedule the next attempt | ||
| // ourselves so recovery doesn't silently stop. If subscribe() did | ||
| // also emit an "error" event, handle() will call scheduleReconnect() | ||
| // first; the guard on pendingReconnect makes this idempotent. | ||
| logger.error("Replication reconnect attempt failed", { | ||
| attempt, | ||
| error: err, | ||
| }); | ||
| scheduleReconnect(err); | ||
| } | ||
| }, delay); |
There was a problem hiding this comment.
🚩 Reconnect silently stalls if subscribe() fails to acquire the leader lock
When the reconnect callback calls this._replicationClient.subscribe(...) (replicationErrorRecovery.server.ts:90), the subscribe() method in client.ts:240 may fail to acquire the leader lock (line 254-258). In that case, it emits leaderElection(false), calls this.stop(), and returns — without throwing and without emitting an error event. From the recovery module's perspective, the reconnect() promise resolved successfully, so it does not schedule another attempt. But notifyStreamStarted() will never fire either (no start event is emitted), so the attempt counter is never reset. The stream is now permanently dead with no further recovery.
In practice this would only occur in multi-replica deployments where another instance wins the leader lock during the reconnect window. Since the other instance is handling replication, this may be acceptable — but the current instance has no path to ever resume if the other instance later dies. This is a design limitation rather than a clear bug, but worth understanding.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
When the logical-replication stream errored (most commonly after a Postgres failover), the runs and sessions replication services logged the error and left the underlying client stopped. The host process kept running, the WAL backed up, and ClickHouse silently fell behind.
Fix
Both services now run a configurable recovery strategy on stream errors, defaulting to in-process reconnect with exponential backoff so a fresh self-hosted setup heals on its own.
reconnect(default) — re-subscribe with exponential backoff (1s → 60s cap, unlimited attempts).LogicalReplicationClient.subscribe(lastLsn)re-validates the publication, re-acquires the leader lock, and resumes from the last acknowledged LSN.exit—process.exit(1)after a short flush window so a host supervisor (Dockerrestart=always, systemd, k8s) can replace the process.log— preserves the old behaviour.Per-service strategy + exit knobs are env-driven (
RUN_REPLICATION_ERROR_STRATEGY/SESSION_REPLICATION_ERROR_STRATEGY+*_EXIT_DELAY_MS,*_EXIT_CODE). Reconnect tuning is shared across both services (REPLICATION_RECONNECT_INITIAL_DELAY_MS,_MAX_DELAY_MS,_MAX_ATTEMPTS;MAX_ATTEMPTS=0means unlimited).Test plan
Integration tests cover all three strategies by simulating a failover with
pg_terminate_backendagainst the WAL sender:reconnect— kill the backend, insert a new row, assert it lands in ClickHouseexit— kill the backend, assertprocess.exit(1)is calledlog— kill the backend, insert a new row, assert it does not land in ClickHousepnpm --filter webapp test --run runsReplicationService.errorRecovery