fix(opal-server): PR3 follow-ups from the staging campaign (rc.3) - #945
Conversation
… so fleet purges reach client-less workers (P5) The purge handler was subscribed on every worker, but a worker's EventBroadcaster reader only ran while it had a WebSocket client (or under STATISTICS_ENABLED). Workers with no client never received the confirmed purge and kept stale GitPolicyFetcher cache entries for the life of the process. Hold the global listening context on every worker whenever a broadcaster is configured and SCOPES (or statistics) is on; enter it once.
…he scope preload (P1) The master emits git metrics during the pre-fork preload but never configured the client, so they reached Datadog without the permit.opal namespace and were invisible to every dashboard/monitor. One helper, configure_server_metrics(), used by both the worker app and the preload.
…only at DEBUG (P6) Per broken source per pass the sync logged ~40 traceback lines; with a broken tail of dozens of sources the container log rotated within minutes of a boot and the boot markers were lost, and in prod this is the mechanism behind opal-server being the org's largest log producer. The ERROR line keeps scope, remote and reason; the traceback moves to DEBUG.
…nc task — say so at startup and in the reference (P3) In scopes mode the flag decides whether ScopesPolicyWatcherTask (periodic sync_scopes pass, boot sync-all, fleet purger) ever runs. A fleet booted with it off registered scopes, stayed Ready and never cloned anything. Warn loudly (do not refuse: an upgrade must not break a deliberate read-only replica) and document it.
- __aexit__(None, None, None): the real EventBroadcasterContextManager has no defaults; a zero-arg call raised TypeError in the un-awaited background task on every scopes leader whose watcher stopped (listen count stuck, reader never cancelled). Test double is strict about arity and the exit path is asserted. - The scopes reason arms the global listening context only on the ReconnectingBroadcaster (lazy connect); the legacy EventBroadcaster connects eagerly in __aenter__ and would abort the whole background task with the backbone down at boot -> one INFO line instead. Belt and braces: a raising __aenter__ is logged as WARNING, the context dropped, and the purge subscription / leadership / watcher still run. - Comments/docstrings (server.py, purge.py, task.py) describe the post-fix state; mdx no longer claims an unconditional periodic pass; statsd test fixture restores host/port/socket/aggregation.
…adog.initialize closes the old one; the setter probes it)
…listening-context enter - A raising __aenter__ leaves the library's _listen_count at 1 (it increments before starting the reader); dropping the reference there meant every later client context counted 2, 3, ... and the reader never started again on that worker. The except handler now calls __aexit__(None, None, None) before dropping the reference (decrements to 0, no task to cancel). - The 'purge delivery not guaranteed' INFO line is emitted only when the context is really absent (statistics on the legacy broadcaster arms it). - Tests: the double mirrors the shared listen count; failed enter asserts exited == 1 and count == 0; legacy+statistics -> armed and quiet; exit path through the mainline watcher shape; _GrantingLock attribute order.
✅ Deploy Preview for opal-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
… not depend on a current event loop preload_reset_test failed on Python 3.9 with "There is no current event loop in thread 'MainThread'": the new preload_metrics_namespace_test (which sorts right before it) calls preload_scopes(), whose asyncio.run() unsets the main thread's loop on exit. preload_reset_test then evaluates the REAL ScopeRepository(RedisDB(...)) — call arguments are built before the stubbed ScopesService sees them — and redis.asyncio's ConnectionPool constructs an asyncio.Lock() in __init__, which on 3.9 (and only 3.9) calls get_event_loop() and raises. Two isolation fixes, no product-code change: * preload_reset_test: an autouse fixture gives every test a fresh event loop and stubs RedisDB/ScopeRepository — a unit test of preload wiring must not construct a real redis client. * preload_metrics_namespace_test: restore a fresh loop in a finally after the preload call, so it never poisons whatever sorts after it. 342 tests pass locally (3.11); the failing chain is deterministic from the traceback on 3.9 (Lock.__init__ -> get_event_loop). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nxg7e1Jtk5qykAdYoYENdw
…on the Postgres backbone) 0.2.7 enables TCP keepalive on every pooled asyncpg connection of the broadcaster's Postgres backend (on by default: idle 30 s, interval 10 s, count 3; tunable with libpq-style keepalives_* parameters in the broadcast URL). Without it, a LISTEN connection whose server address goes silent — an RDS Multi-AZ failover — stays ESTABLISHED forever: the worker's broadcaster reader is deaf while its publisher has long reconnected to the new primary. With it, the dead connection errors within ~60 s and the existing reconnect loop recovers. Verified end to end on the staging scale bed (rc.2 + 0.2.7 overlay image, forced Multi-AZ failover): every worker detected the dead listener in ~30 s, reconnected and resynced in 2-5 s, and a fleet-wide publish converged with zero intervention. See permitio/broadcaster#28. Full test suite passes against the published 0.2.7 (342 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nxg7e1Jtk5qykAdYoYENdw
zeevmoney
left a comment
zeevmoney
left a comment
There was a problem hiding this comment.
Review — f592b4ec (+998 / −41, 13 files)
Reviewed against origin/master (#924 merged as f6c3d831). Measured here: master 325 → PR 342, delta +17, matching the description's "17 new" (both absolute numbers read one higher in my environment than yours — a collection difference, not a discrepancy in your counting).
Method: seven independent review dimensions, each finding then handed to an adversarial verifier instructed to refute it and to rate against the merge base rather than against an ideal. 34 of 45 candidate findings were refuted and discarded; what follows survived. I re-verified every posted finding myself before posting.
No CRITICAL and no HIGH. Six MEDIUM, two LOW. Verdict is a comment rather than changes-requested on that basis.
What holds up
The five P5 mutations the description claims are all real — I ran each and each fails a named test: zero-arg __aexit__, scopes-no-longer-arming, dropping context = None after a failed enter, removing the unwind, and inverting the isinstance gate. The unwind logic in particular is correct and well-reasoned: the library increments _listen_count before starting the reader, so a raising __aenter__ genuinely would leave the count at 1 and starve every later client context.
The dependency bump verifies clean at the artifact level. Diffing the published 0.2.6 and 0.2.7 wheels, exactly two files differ — __init__.py (version) and _backends/postgres.py. No new files, no other backends touched, no metadata or entry-point changes, and the keepalive implementation matches the description precisely (30/10/3 defaults, keepalives=0 disables, libpq-style names, params stripped before asyncpg, never raises on a setsockopt failure, cross-platform fallbacks).
P1 is clean and its central claim checks out — configure_metrics prepends permit., so namespace="opal" really does produce permit.opal. P6's ERROR line carries scope, redact_url'd remote, exception type and message.
Findings
Six MEDIUM inline. The two I would act on first are independent of each other:
- The P3 warning is false about the boot sync.
REPO_WATCHER_ENABLEDhas exactly one functional read in the repo;preload_scopes()is gated only onSCOPESand clones/fetches every scope from the gunicorn master on every boot. The warning,config.py:432and the publicconfiguration.mdx:792all say there is no boot sync-all. A warning added to shorten diagnosis currently points the reader away from the phase doing the work. - The rollout number is 10× low. Entering the context opens 10 Postgres connections per worker, not 1 — asyncpg's
min_sizedefaults to 10 and is never overridden, andBROADCASTER_PG_MAX_POOL_SIZEbelow 10 raises. That is the figure operators will sizemax_connectionsagainst, and the spike lands exactly on a rolling restart.
Then: the keepalives* knob the description advertises is rejected by the version it replaces, so setting it makes any rollback a backbone outage with green probes; the arity fix introduces a self-SIGTERM on the leader fall-through when statistics are on; and the SCOPES half of the arming gate is unpinned, so widening P5 to every broadcaster deployment would pass CI.
Two more, not anchored to a changed line
- P5 invalidates a premise still documented in
scopes/service.py:275-277, which still says a non-leader worker only has a broadcaster reader if it has a connected client. The sibling paragraphs inpurge.pyandtask.pywere updated in this PR; this one and two test docstrings were not. Same class as the P3 texts above — the code moved and one of its explanations did not. - The post-gap client-resync path now runs on client-less workers, emitting a WARNING about recycling clients that do not exist plus a pinned settle sleep per worker per backbone gap. Cosmetic today, but it makes the gap logs noisier on exactly the workers P5 adds.
Refuted — including two of my own
Worth recording, since I raised the last two myself before the verification pass killed them:
- "Every worker now SIGTERMs on a backbone outage" — no.
BROADCAST_RECONNECT_MAX_RETRIESdefaults to0= retry forever, so the give-up path does not fire at shipped defaults. - "The operational note exists only in the PR body, so operators lose it at merge" — the observation is true but it is master parity. The identical lifetime-held context already exists on master gated on
STATISTICS_ENABLED, master documents its connection cost no better, and a worker holds 0-or-1 LISTEN connections before and after. The PR raises utilization of an already-required budget (your staging numbers: ~11/16 → 16/16 workers), not the ceiling. The number is still wrong, which is why the 10× finding stands on its own footing. - "0.2.7 was pinned 94 seconds after upload with no hashes or attestations" — parity again. The repo has no hash, constraints or lock mechanism for any Python dependency, and
permit-broadcasteris the only exact==pin in it; every other requirement is an open range that re-resolves on each CI install, a strictly larger unverified-input surface. 0.2.6 has no provenance either. The malicious-upload scenario is disproven by the wheel diff above.
Also refuted: that the failed-enter unwind leaves a non-working reader, that P5 switches on the publish freeze for previously client-less workers, that _sync_one's ERROR line omits the remote, and that P6's traceback removal never reaches the boot phase.
One thing I could not check
I was given a link to the staging campaign's conclusions but could not open it — it is served to me as a public (non-member) reader, which is not enabled. So I verified this PR against its own description and the code, but I could not confirm that the five fixes cover what the campaign actually found, or that the items not carried here (P2, P4, and the T2 boot-structure follow-ups) are the only ones deliberately deferred. If that punch list is shareable, that cross-check is the one piece of this review still missing.
zeevmoney
left a comment
zeevmoney
left a comment
There was a problem hiding this comment.
Approving — nothing above MEDIUM survived verification
Following up my comment review: no CRITICAL and no HIGH, so this should not sit behind a verdict. Six MEDIUM and two LOW, all of them accuracy or coverage issues rather than defects in the shipped path, and each of the five P5 mutations the description claims is real — I ran every one and each fails a named test.
Seven review dimensions with adversarial verification refuted 34 of 45 candidate findings, including two I had raised myself (the "operational note only lives in the PR body" concern and the 94-second dependency pin — both master parity, detailed in the previous review).
Worth fixing at or before merge, none blocking
Two are operator-facing accuracy problems, and both mislead in the direction of the incidents this PR exists to prevent:
- The P3 warning says there is no boot sync-all, and that is false.
preload_scopes()is gated only onSCOPES, so the gunicorn master clones/fetches every scope on every boot regardless ofREPO_WATCHER_ENABLED. The same claim is inconfig.py:432and in the publicconfiguration.mdx:792. A warning added to shorten a 21-minute diagnosis currently points the reader away from the phase doing the work. - The rollout figure is 10× low — 10 Postgres connections per worker at boot, not 1, because asyncpg's
min_sizedefaults to 10 andBROADCASTER_PG_MAX_POOL_SIZEcannot go below it. That is the number operators will sizemax_connectionsagainst, and the spike lands on a rolling restart.
The keepalives* rollback hazard is the third I would not leave undocumented, since the knob is advertised in the description but appears nowhere in the repo.
What this approval does not cover
I could not open the staging campaign's punch list — it is served to me as a public (non-member) reader. So this approval is based on the code and the PR description, and it does not include a check that the five fixes actually cover what the campaign found, or that P2, P4 and the T2 boot-structure items are the only deliberate deferrals. If that cross-check matters before merge, it is the one piece still outstanding and it needs the punch list.
Separately verified clean and worth recording: diffing the published 0.2.6 and 0.2.7 wheels, exactly two files differ — __init__.py and _backends/postgres.py — with no new files, no other backends and no metadata changes, and the keepalive implementation matches the description including the 30/10/3 defaults and the keepalives=0 disable.
…oot-cost + keepalives-rollback docs, no self-SIGTERM on clean exit, gate pins
Six review findings on the rc.3 changeset:
* REPO_WATCHER_ENABLED texts (warning, config description, docs) claimed
"no boot sync-all" — but the gunicorn master's pre-fork preload clones/
fetches every registered scope on every boot regardless of the flag. All
three texts now say exactly what the flag gates (the leader's periodic
pass, post-leadership sync-all and fleet purge) and that boot-time git
traffic and clone-tree growth are expected either way.
* The P5 rollout note undercounted the backbone cost 10x: asyncpg's
create_pool defaults min_size to 10 EAGER connections per worker at boot
(decaying to ~1 after ~300 s), and BROADCASTER_PG_MAX_POOL_SIZE can only
raise the ceiling. The figure now lives where operators look: a comment
at the arming gate and the OPAL_BROADCAST_URI docs ("budget for
10 x workers x pods at a rolling restart"). Lowering min_size at source
is a permit-broadcaster follow-up.
* The keepalives* URI parameters advertised with the 0.2.7 bump are
REJECTED by 0.2.6 (forwarded to Postgres as session settings -> every
connection refused) — a rollback with the parameters set would be a
fleet-wide backbone outage with green probes. Documented under
OPAL_BROADCAST_URI with an explicit version constraint; the defaults
need no URI change at all.
* Fixing the __aexit__ arity made the statistics done-callback actually
run — including on the exit path's own clean cancellation, a
self-SIGTERM master never had (boot restart loop of the leader slot
under statistics on + keepalive off + watcher off). The callback now
fires only when the reader task completed WITHOUT being cancelled;
reader death still restarts the worker.
* The SCOPES half of the arming gate and the statistics-None guard were
untested (dropping either kept CI green). The reader-task double is now
stable (the real get_reader_task returns the same task every call), the
fake context cancels the reader on last exit like the library, and four
new tests pin: SCOPES=False arms nothing, statistics-off attaches no
callback, clean exit does not restart, reader death does. All three
guard-removal mutations now fail named tests (verified).
* The preload_reset autouse fixture kept only its load-bearing half (the
redis stub — the 3.9 CI failure it pins) with a docstring that says on
which Python it can fail and why the sibling file stubs per-test.
346 tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nxg7e1Jtk5qykAdYoYENdw
2 participants
fix(opal-server): PR3 follow-ups from the staging campaign (rc.3)
Four narrow fixes found while running the PR3 stability campaign on staging and on a prod-shaped fleet on 2026-08-18 (evidence in the private staging test kit's punch list, items P1/P3/P5/P6), plus the
permit-broadcaster0.2.6 → 0.2.7 pin bump (P9). None changes a public route or a wire format; all are behind existing config. Branch ismaster(#924 merged) + these commits.P9 — dependency bump: permit-broadcaster 0.2.7 (TCP keepalive on the Postgres backbone)
What: one line in
requires.txt. 0.2.7 (permitio/broadcaster#28) enables TCP keepalive on every pooled asyncpg connection of the broadcaster's Postgres backend — on by default (idle 30 s / interval 10 s / count 3), tunable or disableable with libpq-stylekeepalives*parameters in the broadcast URL. Rollback constraint: only set those parameters once every server sharing the URI runs ≥ rc.3 — 0.2.6 forwards them to Postgres as session settings and every backbone connection is refused (documented with a danger callout underOPAL_BROADCAST_URI). The defaults need no URI change.Why: a
LISTENconnection is idle by nature; when the database's address goes silent without closing the socket (a Multi-AZ failover), the reader staysESTABLISHED-and-deaf forever while the publisher reconnects by DNS — the process ends up notifying the new primary while listening to the old one. asyncpg exposes no keepalive option (MagicStack/asyncpg#606), so the library sets it on the socket.Verified: on a prod-shaped staging fleet (base image + only this library change), a forced Multi-AZ failover: every worker detected the dead listener in ~30 s, reconnected and resynced in 2–5 s, and a fleet-wide publish converged with zero intervention; post-failover socket tables show all listener connections on the new primary with the keepalive timer armed. Full test suite passes against the published 0.2.7.
P5 — fleet purge must reach workers with no WebSocket client
What: when
OPAL_SCOPESis on and a broadcaster is configured, every worker now holds the global broadcast listening context for its lifetime (the same context the statistics path already used), so server-side subscriptions — the fleet purge handler — actually receive messages on workers that have zero clients.Why:
subscribe_worker_purge_handlerwas registered on every worker, but a worker'sEventBroadcasterreader only ran while it had a WS client (or underSTATISTICS_ENABLED, off in typical deployments). Client-less workers never received the confirmed purge and kept staleGitPolicyFetcher.repo_locksentries for the life of the process. Measured on a staging deployment (~23 WS connections over 16 workers): 5 of 8 workers per pod never logged a single purge across 12 purge events; after a full drain 8 non-leader pids still held the deleted source's lock key (test_keys_zeroFAIL in T1.2/T1.3). Bytes are negligible; the "fleet purge" guarantee (invariant I4) was not.How:
server.py— the context is created wheneverbroadcaster_uriand (STATISTICS_ENABLED, orSCOPESon theReconnectingBroadcaster— the default); entered exactly once per worker before the leadership lock (in a try/except: a raising enter is one WARNING, the half-entered context is unwound with__aexit__(None, None, None)so the library's shared listen count goes back to 0 — otherwise later client contexts would count 2, 3, … and the reader would never start on that worker — and the worker still subscribes the purge handler, takes leadership and runs the watcher); exited with the correct__aexit__(None, None, None)arity on the way out. LegacyEventBroadcaster(BROADCAST_RECONNECT_ENABLED=false) connects eagerly and would abort the background task with the backbone down at boot, so it gets one INFO line ("purge delivery to client-less workers not guaranteed") instead of a reader. Restart-on-give-up for scopes-only workers is_wire_broadcaster_give_up(fires on give-up, never on clean cancellation); the statistics done-callback stays statistics-only. Single-process deployments (no broadcaster) are untouched.Also fixed on the way: the pre-existing zero-arg
__aexit__()call — the real context manager has no defaults, so it raisedTypeErrorinside the un-awaited background task; with the widened guard that path is now hit by every scopes leader whose watcher stops.Operational note: each previously client-less worker opens the broadcaster pool: 10 eager connections at boot (asyncpg default
min_size;BROADCASTER_PG_MAX_POOL_SIZEonly raises the ceiling), decaying to ~1 after ~300 s. Budgetmax_connectionsfor 10 × workers × pods at a rolling restart.Tests:
scopes_worker_listening_context_test.py(8) — non-leader with SCOPES on/STATISTICS off enters once; SCOPES+STATISTICS enters once (no double-enter); no broadcaster → no context; leader path enters and EXITS once (strict-arity double that mirrors the shared listen count), both through the fall-through shape and the mainline watcher shape (fake watcher stops → graceful shutdown → exit); legacy broadcaster → no scopes reader + one INFO; legacy broadcaster + statistics → context armed and no INFO; raising__aenter__→ WARNING, unwound (exited == 1, listen count 0), purge subscription and leadership still run. (Unit-level aroundstart_server_background_tasks; the bed's git-leak gates don't model client-less workers, so no bed gate was added.)P1 — DogStatsD configured in the gunicorn master before the scope preload
What: new
opal_server/metrics_setup.py:configure_server_metrics(), called by the worker app (unchanged behaviour) AND byScopesPolicyWatcherTask.preload_scopes()before the first sync.Why: the master runs the preload from
scripts/gunicorn_conf.py:when_readybefore any worker exists and never configured the client, so its metrics went out without thepermit.opalnamespace: after the staging rollout Datadog showedopal_server.scopes.git_ops_in_flight{pid:7}/opal_server.scopes.count(master) next to the workers'permit.opal.opal_server.scopes.*— the boot phase was invisible to every dashboard/monitor built on the namespaced names.Note for dashboard readers: the master's pid-tagged series (
git_ops_in_flight,sources_in_backoff,git_ops_refusedfor the master pid) now appear namespaced during preload and go NO DATA after fork (the master stops emitting) — expected; the kit monitors usenotify_no_data=false.Tests:
preload_metrics_namespace_test.py— namespacepermit.opalafter configure; fail-silent when metrics disabled; the serialized packet for a gauge starts withpermit.opal.opal_server.scopes.count:;preload_scopescalls configure BEFOREsync_scopes.P6 — a failed scope sync is one ERROR line; the traceback moves to DEBUG
What:
scopes/service.pysync_scope(and the per-pass_sync_onewrapper) logCould not fetch policy for scope <id> (remote: <url>): <ExcType>: <msg>at ERROR without a traceback, and the traceback at DEBUG.Why: per broken source per pass the previous
logger.exceptionemitted ~40 traceback lines. On the prod-shaped fleet (64 broken repos) the container log rotated (10 MiB) within minutes of a boot and the boot markers were gone before anyone could read them; in production this same mechanism dominates opal-server's log volume. The per-source backoff (#924) makes it decay, but the first hour after any boot was still a firehose. The clone/fetch paths ingit_fetcher.pyalready logged without a traceback. At the usual production log level (INFO) the DEBUG traceback is effectively never emitted — that is the intent: the ERROR line carries scope, remote and reason, and anyone chasing one source turns DEBUG on for that pod.Tests:
sync_failure_log_volume_test.py— a repeated failure yields ERROR records naming scope/remote/reason with no exception attached, and one DEBUG record with the traceback per failure; same for the pass wrapper.P3 —
OPAL_REPO_WATCHER_ENABLEDgates the scopes sync task: say soWhat: startup WARNING when
SCOPES=trueandREPO_WATCHER_ENABLED=false("this server will REGISTER and SERVE scopes but never SYNC or PURGE them"); config description andconfiguration.mdxexplain that in scopes mode the flag enables the periodicsync_scopespass, the boot sync-all and the fleet purger.Why: the flag reads as "single-repo watcher". A prod-shaped fleet booted with it off registered 2160 scopes, reported Ready and never cloned anything (leader parked on the keepalive) — 21 minutes to diagnose. A warning rather than a refusal so an upgrade cannot break a deliberate read-only replica. The docs paragraph is precise about what the task does: boot sync-all, the periodic pass only when
OPAL_POLICY_REFRESH_INTERVAL > 0(default 0 skips it), and the fleet purge handler.Tests:
scopes_watcher_flag_warning_test.py— warning present with the flag off, absent with it on; the description mentions scopes/purge. Docs-drift test still green.Not in this PR (deliberately) / follow-ups
BROADCAST_RECONNECT_ENABLED=false(legacyEventBroadcaster) P5 does not apply: the per-client reader lifecycle is preserved as before (a reader only while a client is connected), and the server logs one INFO line at startup saying purge delivery to client-less workers is not guaranteed in that mode. Enabling reconnect (the default) is the fix there.min_sizeis 10 eager connections at boot, decaying to ~1 after ~300 s (BROADCASTER_PG_MAX_POOL_SIZEcan only raise the ceiling). Budget the broadcast database'smax_connectionsfor 10 × workers × pods during a rolling restart, not the steady state. Loweringmin_sizeat source is a permit-broadcaster follow-up.POST /scopes/{id}/data/updatepayloads are ignored by scoped opal-clients (server rewrites entry topics todata:<t>, client filters on<scope>:data:<t>): public-route semantics, needs its own discussion; publishers that use/data/configwith fully-qualified topics are unaffected.initialDelaySeconds300→60,preStop sleep 120vs 30 s grace) — permit-deployments values PR.Verification
packages/opal-server: 324 tests on master → 341 here (17 new), all green; each new test mutation-checked (fix reverted → test fails → restored; for P5 specifically: zero-arg__aexit__→ fails, scopes no longer arming the context → fails, try/except removed → fails, isinstance gate inverted → fails, unwind after a failed enter removed → fails, INFO guard made inaccurate → fails). pre-commit (black/isort/codespell/docformatter) clean.Release note (rc.3)
opal-server 0.9.9-rc.3: fleet purge now reaches workers without WebSocket clients; boot-phase metrics carry the
permit.opalnamespace; failed scope syncs log one line (traceback at DEBUG);OPAL_REPO_WATCHER_ENABLED=falsewith scopes on warns at startup.🤖 Generated with Claude Code
🤖 Generated with Claude Code
https://claude.ai/code/session_01Nxg7e1Jtk5qykAdYoYENdw