fix(opal-server): freeze publishes during broadcaster backbone gaps to keep the fleet consistent#933
Conversation
…is down During a broadcaster-backbone outage, a publish reaching one worker was still delivered to that worker's own clients (local in-process notifier) while peers' clients stayed stale — a transient fleet split lasting the whole outage (observed 3/9 PDPs diverging in a 3-server/9-PDP staging test). Gate the whole publish on live backbone connectivity: add is_backbone_connected() to ReconnectingBroadcaster (True only while actively subscribed — unlike is_reader_healthy(), which deliberately stays healthy across a transient reconnect) and a FreezablePubSubEndpoint that skips publish entirely while disconnected. Clients stay connected but frozen on the last consistent state; the write still lands in its datasource and the existing reconnect resync makes every client refetch it, so the fleet converges together. Skipping the whole publish also keeps the replay buffer out of the recovery path (no partial replayed state racing the resync refetch). Configurable via OPAL_BROADCAST_FREEZE_ON_DISCONNECT (default true). Scope: designed for source-based updates (entry carries a URL clients refetch). Inline updates issued during an outage are dropped rather than deferred — accepted, inline is legacy. Validated on a 3-server/9-PDP staging stack against a real RDS stop/start: during-outage divergence 3/9 -> 0/9, zero client disconnects and zero worker restarts across ~20-minute outages, and a source-based write made during the gap converged 9/9 via the resync refetch alone (its notification provably frozen, nothing replayed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…exemptions, alias bypass Review findings on the freeze-on-disconnect gate, all fixed here: - Gate on a real backbone GAP (had a session, lost it, reader retrying) via is_in_backbone_gap(), not on mere "not subscribed". Freezing while the reader was never started (healthy idle worker: the global listening context is only pinned when statistics are enabled, and upstream cancels the reader when the last listener leaves) or never yet connected (boot) silently dropped publishes fleet-wide with no resync ever firing to reconcile them. - Exempt internal coordination topics from the freeze: "__"-prefixed channels (statistics protocol, broadcaster keepalive) and the git-webhook trigger topic. These target server-side subscribers, not clients — dropping them broke statistics state (ghost clients, workers that never sync) and lost repo-pull triggers that no resync re-issues (webhooks are the only pull trigger with the default POLICY_REPO_POLLING_INTERVAL=0). Exempt topics keep the pre-freeze deliver-locally + buffer-for-replay behavior. - Re-bind the library's class-level `notify = publish` alias, which bound the BASE publish and let endpoint.notify(...) bypass the gate entirely. - Refuse BROADCAST_FREEZE_ON_DISCONNECT with BROADCAST_RESYNC_ON_RECONNECT disabled (warn + disable freeze): the resync is the freeze's only recovery path, so that combination silently lost every update published during gaps. - Rate-limit the freeze log: WARNING once per gap episode, DEBUG afterwards, and a suppressed-count summary on recovery (a long outage previously emitted an unbounded WARNING per frozen stats keepalive). - Document recovery scope honestly (configured-source refetch; one-off URL / inline updates are dropped by a freeze) and the residual windows that keep pre-freeze behavior (outbound-failure while subscribed, connect flap, RPC client-originated publishes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
✅ Deploy Preview for opal-docs canceled.
|
CI fixes: - pre-commit / docformatter: new docstrings were not wrapped per the pinned docformatter 1.7.5; re-wrapped in place (no code changes), verified idempotent alongside black/isort and the test suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a “fleet-consistency freeze” for OPAL Server publishes during broadcaster backbone gaps so that writes received by a single worker during an outage are not delivered locally (and thus can’t split the fleet) until recovery via reconnect resync.
Changes:
- Add backbone subscription/gap tracking to
ReconnectingBroadcasterand a newFreezablePubSubEndpointthat suppressespublish()during gaps (with exemptions). - Wire the new endpoint into server pubsub initialization with a safety guardrail when resync is disabled.
- Add config flag
BROADCAST_FREEZE_ON_DISCONNECT(defaulttrue) and new unit tests covering gap lifecycle + freeze behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/opal-server/opal_server/tests/reconnecting_broadcaster_test.py | Adds tests for is_backbone_connected / is_in_backbone_gap lifecycle and freeze gating behavior. |
| packages/opal-server/opal_server/pubsub.py | Switches server endpoint to FreezablePubSubEndpoint and applies freeze + exemptions + resync guardrail. |
| packages/opal-server/opal_server/pubsub_resilience.py | Adds backbone connectivity state, gap detection API, and implements FreezablePubSubEndpoint. |
| packages/opal-server/opal_server/config.py | Introduces BROADCAST_FREEZE_ON_DISCONNECT configuration option and documentation text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… over An exempt (internal-topic) publish arriving mid-gap reaches the delivery path and was resetting the freeze-episode counter and logging "Backbone recovered" while the backbone was still down. Emit the summary only when the gap has actually ended; exempt deliveries mid-gap no longer end the episode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…antics CI fixes: - E2E Tests / App Tests: the cross-instance consistency test asserted the pre-freeze behavior (a one-off update published during a backbone outage reaches clients via buffer+replay). With BROADCAST_FREEZE_ON_DISCONNECT (default on) that publish is frozen so the fleet never splits. The test now asserts the new semantics: the publish is frozen at the gate, exempt internal topics still ride buffer+replay, recovery resyncs clients, the frozen update reached NO client (new check_clients_not_logged helper), and a post-recovery re-publish converges both clients together and logs the freeze-episode summary. Also raise the gitea startup wait budget (120s -> 300s): slow bind-mount environments need minutes to bring the web listener up; fast environments exit the poll early and pay nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Account freeze episodes per backbone gap: the reader now bumps a monotonic gap generation on every connected->disconnected edge, and publish() flushes the previous gap's pending summary when a frozen publish arrives from a new generation — back-to-back gaps no longer merge into one episode (which suppressed gap B's WARNING and mis-attributed the summary). publish() also routes through _should_freeze, so the matrix-tested predicate is the production one. Exemptions: an empty topic list no longer slips past the gate (all([]) is True), and the server-to-server coordination channels are exempted by their CONFIGURED names instead of relying on the "__" naming convention, which only covers the defaults. Guardrails: the resync-disabled warning now fires only when the reconnecting broadcaster (the gap signal) was actually built, and the reconnect-disabled + freeze-on combination logs that the freeze is a no-op instead of staying silent. Also drop the test-only is_backbone_connected() accessor, and clean up the tests: get_running_loop(), awaited task cancellation on teardown, and a no-op FakeNotifier.unsubscribe so the sharing-context teardown path unwinds cleanly. Addresses review comments: - #933 (comment) (@zeevmoney) - #933 (comment) (@zeevmoney) - #933 (comment) (@copilot-pull-request-reviewer) - #933 (comment) (@zeevmoney) - #933 (comment) (@zeevmoney) - #933 (comment) (@zeevmoney) - #933 (comment) (@copilot-pull-request-reviewer) - #933 (comment) (@copilot-pull-request-reviewer) - #933 (comment) (@copilot-pull-request-reviewer) - #933 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the fixed sleep after killing the broadcast channel with a poll that waits for a server worker to actually OBSERVE the backbone drop (count-based against a pre-kill baseline, since drop-log lines already appear in earlier test phases) — the freeze only engages once the reader's read cycle exits, so a fixed sleep races it. Poll for the freeze-episode summary instead of asserting it after one fixed delay, and document that the summary normally arrives via the exempt statistics keepalive on the worker that froze. Make the negative log assertions capture `compose logs` output and assert it is non-empty before grepping: piped straight into grep, a failing compose call is indistinguishable from "message absent" and the check silently passes (pipefail would not help — the `if pipeline; then fail; fi` shape falls through on any failure). Addresses review comments: - #933 (comment) (@zeevmoney) - #933 (comment) (@zeevmoney) - #933 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zeevmoney
left a comment
There was a problem hiding this comment.
Re-reviewed at a2a01fc. All findings from the previous round are verified fixed in the actual code, not just marked resolved:
- Freeze episodes no longer merge across back-to-back gaps (
_gap_generationbumped on the connected→disconnected edge;publishflushes the prior episode's summary on a generation change; covered bytest_back_to_back_gaps_get_separate_episodes). - The guardrail now keys off the broadcaster actually built — full truth table traced: silent when no broadcaster, INFO no-op note for the stock broadcaster, WARNING+disable only where the freeze could actually engage; config description matches the code.
- The exempt list now carries the seven configured channel names — all verified to exist on the right config objects (no startup risk) and complete against every server-consumed publish site.
- E2E: fixed sleep replaced by
wait_for_backbone_droppolling (grep patterns match the reader's real log lines), summary assertion polls across the keepalive interval, negative assertions capture logs and fail on empty,_should_freezeis now the single predicatepublishuses, empty/None topic lists can't slip past the gate,get_running_loop, awaited dummy tasks,FakeNotifier.unsubscribe— all in place. - Tests: 64/64 opal-server tests green from this branch's sources; no new warnings.
Two remaining issues, neither blocking:
[MEDIUM] app-tests/run.sh:486, 496 — the new positive freeze assertions cannot actually fail the E2E run. main is invoked as main && break (line 519), which suppresses set -e inside it, and check_servers_logged only returns grep's status without exiting. So "freezing publish…" and "resyncing this worker's clients" are swallowed if they miss. The core no-fleet-split property is still enforced (check_clients_not_logged exits explicitly), but the freeze/resync-fired checks are decorative until check_servers_logged hard-fails on miss (mirror the negative helpers' exit 1).
[LOW] app-tests/run.sh:304-320 — residual race in wait_for_backbone_drop: the poll returns on the first drop observed across all 8 workers, while publish_data lands on one specific worker whose own reader may not be mid-gap yet. Much narrower than the old sleep 3 (SIGKILL severs all backbone connections at once) and absorbed by the retry wrapper, but a miss is still possible; polling until the count rises by the worker count, or retrying the publish until the freeze log appears, would close it.
`main` runs on the left of `&&`, which suppresses `set -e` throughout it, so check_servers_logged/check_clients_logged returning grep's status could never fail the run — the freeze/resync assertions were decorative. Make both helpers exit explicitly on a miss, and run `main` in a subshell so that `exit 1` (from these and the existing negative helpers) returns to the retry loop instead of terminating the script through the EXIT trap — previously any exit-based assertion failure bypassed the retries entirely. Verified: full app-tests E2E green on attempt 1 against branch-built images. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The problem
Follow-up to #915. That PR made a fleet of OPAL servers survive a broadcast-backbone outage (reconnect with backoff, buffer outbound broadcasts, resync clients on recovery). Multi-server validation of it surfaced a consistency gap during the outage:
A server-side
publishfans out two independent ways — local in-process delivery to that worker's own clients, and the broadcaster to peer workers. Only the outbound path is buffered when the backbone is down; local delivery still fires. So an update that reaches one server mid-outage is applied to its clients while every other server's clients hold old data — a fleet split that lasts the whole outage. In a 3-server / 9-client test against a real managed-Postgres backbone stop, a data update published during the outage left the fleet at 3/9 new vs 6/9 old for the entire ~13-minute gap. For an authorization system, that means the same request can be allowed by one PDP and denied by another until the backbone returns.The fix
FreezablePubSubEndpointgates client-facing publishes on live backbone connectivity: while theReconnectingBroadcasteris mid-gap (an established backbone session was lost and is being re-acquired — seeis_in_backbone_gap()),publishis skipped entirely. Clients stay connected but frozen on the last consistent state; the write still lands in its datasource; and the existing reconnect resync (from #915) makes every worker's clients refetch on recovery — so the whole fleet moves together instead of diverging. Skipping the whole publish also keeps the replay buffer out of the recovery path for these updates: convergence is purely the resync refetch.Configurable via
OPAL_BROADCAST_FREEZE_ON_DISCONNECT(defaulttrue; setfalsefor the previous behavior).Scope & semantics (please read if you rely on runtime data updates)
OPAL_DATA_CONFIG_SOURCES/ scope config) and the policy bundle. These are reconciled fleet-wide on recovery.datapayloads, or fetch URLs not part of the configured sources. Consistency is chosen over freshness for these; if you rely on them, set the flag tofalse.__-prefixed channels (statistics protocol, broadcaster keepalive) and the git-webhook trigger topic. These target server-side subscribers, not clients; freezing them would break statistics state and silently drop repo-pull triggers that no resync re-issues.OPAL_BROADCAST_RESYNC_ON_RECONNECT=falseis refused with a warning (freeze disabled) rather than silently losing updates.Hardening (second commit)
An adversarial review of the first commit caught real issues, all addressed:
is_in_backbone_gap()(had a session, lost it, retrying), not mere "not subscribed". Freezing while the reader was never started (idle worker — the reader only runs while a listening context is held) or never yet connected (boot) would silently drop publishes on a healthy backbone with nothing to ever reconcile them; those states delegate to the pre-freeze behavior. Gap state is scoped to the reader task's own lifetime, mirroring the resync's semantics.notify = publishalias re-bound — the upstream endpoint aliasesnotify = publishat class level, which binds the base publish; without re-binding,endpoint.notify(...)bypassed the gate entirely.Validation
notifyalias pin.🤖 Generated with Claude Code