Refactor: Opal Server, Repo Watcher, Cloner, Tracker, LeaderElection, BundleMaker, and bundle diffs by asafc · Pull Request #3 · permitio/opal

asafc · 2021-02-28T18:01:51Z

status: almost done

high level changes:

lots of nicer smaller classes + added some docs
can now produce full bundles and diff bundles (i.e: only changes from last bundle / known state)
can now publish only topics of changed directories, not everything in the repo
file contents are now pulled from git db (i.e: git cat-file) and not from disk (disk state /working dir can be unreliable if we are in the middle of a pull)

in a bit more detail:

split policy watcher into smaller classes (almost done): RepoWatcher, RepoCloner, BranchTracker etc.
added "viewer" classes that allows us to view the repo from the perspective of a commit or a diff between commit
refactored and improved the bundle maker

We use a queue and workers to explicitly await on tasks scheduled *after* the thread is started. This approach avoids a bug where we scheduled tasks on the thread's asyncio loop only for them to not be executed for at least 20-30 seconds, because nobody awaited on them. Also more gracefully handling termination of the thread from outside and added some docs.

orweis · 2021-02-28T20:15:08Z

+from opal.common.opa import get_rego_package, is_data_module, is_rego_module
+from opal.common.schemas.policy import DataModule, PolicyBundle, RegoModule, DeletedFiles
+
+


As we are moving to public release we should probably start documenting these more - both inline and doc-strings. There aren't barely any docs on in this code.

orweis · 2021-02-28T22:55:10Z

+logger = get_logger("opal.policy.publisher")


 class PolicyPublisher:


We should probably have a generic publisher class (and instance) to be used by both policy and data

added TopicPublisherThread.

(see in commit: generic publisher, i.e: i only implemented thread because pubsub client is generic enough to be considered the "non-thread" publisher)

orweis · 2021-03-02T17:26:52Z

Then catch and translate the exception at the route level... You can't assume your code will be forever coupled to a specific way of serving... Each class and module should be self contained.

…

On Tue, Mar 2, 2021, 19:20 Asaf Cohen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In opal/server/policy/api.py <https://github.com/authorizon/opal/pull/3#discussion_r585758196>: > - for p in paths: - if p.startswith('/'): - p = p[1:] # remove first slash - repo_path = repo_dir / Path(p) - if not repo_path.exists(): - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"requested path {p} was not found in the policy repo!" - ) - parents.append(repo_path) + # verify all input paths exists under the commit hash + with CommitViewer(repo.head.commit) as viewer: + for path in paths: + if not viewer.exists(path): + raise HTTPException( @orweis <https://github.com/orweis> i do not agree with this comment. in general you are right, but the scope of this error is validating user input to a restful route. the fastapi convention is to throw HTTP Exception with the appropriate code. please notice i do not use http exceptions in generic classes, only inside fastapi routes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/authorizon/opal/pull/3#discussion_r585758196>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAP3ULOP7RACFHRXEIFOR23TBUM5HANCNFSM4YLJIMAA> .

orweis · 2021-03-02T18:22:35Z

+from opal.common.election.base import LeaderElectionBase
+from opal.common.logger import get_logger
+
+class UvicornWorkerPidLeaderElection(LeaderElectionBase):


Maybe there's no need to couple this with Uvicorn...
after all it would probably work the same with Gunicorn , Tornado, on any ASGI

GitPolicyFetcher: Don't leave hanging redis locks on crashes

…-to-opal Added a Cedar policy example.

Zeev's gate-coverage review found only 2 of 5 flagship tests (churn #1, offline #4) were genuine fail-now / pass-after gates. Lift the other three: - #5 broadcaster: run 2 workers (OPAL_TEST_WORKERS) so the Postgres backbone is actually fanned out cross-worker (references/debug-pubsub.md §3-4), and assert the gunicorn worker PIDs are unchanged across a transient bounce -- the in-place-reconnect signal that distinguishes #915 (PER-15065) from a plain worker respawn. Prove recovery via a servable post-bounce scope, not /internal cache counts (per-process, non-deterministic on 2 workers). - #3 boot: key completion on "all scopes served" (GET /scopes/{id}/policy == 200) instead of repo_locks (set at fetch start, so it undercounts the final clone); document the PR4 tight-BOOT_TARGET_SECONDS carry-forward. - #2 repeat-sync: rename to test_repeat_sync_rss_stays_bounded and drop the tautological len(repos) assertion; RSS is the sole (load-bearing) gate. Adds worker_pids() (/proc-based, matched host-side so the scan can't count its own sh -c wrapper) and the opal_multiworker fixture (recreate to 2 workers, restore to 1 on teardown). Validated live (--boot-scopes=50): #2/#3 pass, #5 passes (worker PIDs held across the bounce, post-bounce scope served), #1/#4 fail for the right reason (PR2/PR3 not landed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Addresses the HIGH and all MEDIUM findings from the opal-development / python-pro / backend-architect microreview: - H1 (#5 vacuous pass): the broadcaster gate now positively verifies a disconnect+reconnect actually happened, not just that no respawn occurred. New helpers.broadcaster_connect_count() counts the reconnecting broadcaster's "listener connected to channel" log line; the test asserts it increased across the bounce (paired with worker PIDs unchanged = in-place, not respawn). - M1 (#4): move the 40 executor-saturating PUTs inside the try/finally so a PUT failure still runs hard_reset() instead of leaking hung clone threads into the session stack. - M2 (#1/#2): _wait_until now treats a transient requests error from opal.stats() as "not yet" and retries, instead of ERRORing the test. - M3 (#3): measure a deterministic pure-cold boot (--force-recreate wipes the ephemeral FS -> preload cold-clones all N from Redis) instead of a nondeterministic warm/cold mix, so PR4's tight BOOT_TARGET_SECONDS can gate. - M4: verify the single-worker invariant -- opal_multiworker teardown asserts the stack is back to 1 worker, and the opal fixture asserts single-worker at setup, so a botched restore fails loudly instead of silently breaking cache gates. - M5 (#4): correct the reserved-probe comment (serving shares the fetch executor, so a shared repo would be starved on serve too; the probe additionally exercises the clone). - M6: gitea-admin retries on "database is locked" (CLI mutating live SQLite); rewritten as a `|` literal block so the create call stays on one line. Validated live (--boot-scopes=20): #2/#3 pass, #5 passes (reconnect count increased across the bounce + PIDs unchanged + scope served), #1/#4 fail for the right reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…publish guard (review items 1-4) Coverage additions from the test-coverage review pass: 1. Unit tests for the server.py wiring: build the real OpalServer app and assert /internal/git-fetcher-cache-stats is mounted iff DEBUG_INTERNAL_STATS is on — removing the register call from _init_fast_api_app now fails the unit suite. 2. test_shared_repo_survives_sibling_scope_delete: deleting one of two scopes sharing a repo URL must not purge the URL-keyed cache entry or break the survivor (guards PR2 against over-purging; churn only covers the all-scopes-gone direction). 3. test_scope_repoint_releases_old_repo_cache: PUT /scopes is create-or-update, so re-pointing a scope orphans the old URL's cache entries — a leak path churn (delete-only) never takes. Red until PR2's purge covers updates too. 4. Postgres-bounce test: bounce_postgres gained a during= callback (with a finally that restores Postgres even if it raises) and the test now PUTs a scope mid-outage, asserting (d) it becomes servable after recovery — its sync trigger must be buffered/replayed across the gap, not silently dropped. Uses a distinct seeded repo from (c) so a stale on-disk clone can't satisfy it vacuously. Validated live (--boot-scopes=2): #2 and the bounce test pass; #3 fails for the right reason (caches stuck at 2/2/2 after the delete — PR2 not landed). Unit suite: 52 passed; pre-commit clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* feat(opal-server): gated /internal git-fetcher cache stats endpoint Add an off-by-default diagnostics endpoint so tests can observe the in-memory GitPolicyFetcher cache sizes (repo_locks/repos/repos_last_fetched) and process RSS that the upcoming memory-leak fix eliminates. - debug_stats.py: read-only git_fetcher_cache_stats() helper + a register_internal_stats_route() registrar that mounts GET /internal/git-fetcher-cache-stats only when enabled. - config.py: new OPAL_DEBUG_INTERNAL_STATS flag, default False. - server.py: register the route, gated by the flag, beside /healthcheck. No production behavior change when the flag is off (the default). Also ignore .claude/ so private planning artifacts are never committed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): add OPAL git leak/resilience test bed A self-contained docker-compose stack (opal-server x2 workers + Redis + Postgres broadcaster + Gitea) plus a pytest harness that reproduces, as tests that fail on master, the git-fetcher memory leak, the offline-repo hang, the slow serial boot, and the broadcaster-disconnect gap. These become the regression gates for the follow-up fixes. - seed/: idempotent Gitea seeding sidecar (N policy repos) + Dockerfile. - docker-compose.yml: 4-service stack, opal-server built from the repo's own docker/Dockerfile (server target), scopes on, Postgres broadcaster. - helpers.py / conftest.py: HTTP + infra helpers and stack fixtures. - test_leak.py / test_resilience.py / test_boot.py: the flagship tests. - README.md: how to run and expected fail-on-master behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): add GiteaAdmin and make_repo_unreachable helpers Complete the helpers.py surface promised in the plan's file-structure table. Both are now functional and used, not dead code: - make_repo_unreachable(name): returns a git URL on a routable-but-dead TEST-NET-1 host (RFC 5737). test_offline_repo now uses it instead of an inlined literal. - GiteaAdmin: host-side Gitea admin client (list_repos / repo_exists / create_repo / delete_repo), exposed as the `gitea_admin` pytest fixture for tests that need to inspect or stage repos beyond the seed sidecar. Gitea is published on host port 13000 (uncommon, to avoid the usual :3000 clash) so GiteaAdmin can reach it; opal_server and the seed sidecar still use the internal http://gitea:3000. README updated with the helper and port notes. Verified live: GiteaAdmin lists the seeded repos and round-trips create/exists/delete against Gitea over the published port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): correct postgres-bounce framing (passes on master) Verified test_server_recovers_after_postgres_bounce against the stack: it PASSES on master (~14-19s). On a broadcaster drop the affected worker triggers a graceful shutdown, gunicorn respawns it, and the sibling worker keeps serving HTTP, so the surface recovers within the window — recovery happens via gunicorn's in-container worker supervision, not an external supervisor and not an in-process reconnect. Reframe #5 as a recovery guard (not a known-broken case) in the docstring and README; the prior "FAILS on master / needs external supervisor" wording was wrong. PER-15065's in-process reconnect would avoid the worker churn but recovery already holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(git-leak): apply black/isort/docformatter (pre-commit) Run the repo's pinned pre-commit formatters (black 23.1.0, isort 5.12.0, docformatter 1.7.5) over the PR1 files to satisfy the pre-commit CI check. Formatting only — no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test: scope root pytest collection to packages/ (exclude git-leak bed) The CI `build` job runs `pytest` from the repo root with no path, which recursed into app-tests/git-leak/ and ran the flagship tests — these are designed to FAIL on master (they are the regression gates for PR2-PR5), so they broke the build job. Set `testpaths = packages` so the rootdir run collects only the unit tests under packages/ (matching master's effective behavior, since app-tests/ had no pytest files before). testpaths only applies when pytest is invoked from the rootdir with no args, so `cd app-tests/git-leak && pytest` still collects and runs the test bed. Verified both: root run -> packages only; subdir run -> all 5 flagship tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): address Copilot review feedback - stats(): sample the /internal endpoint several times and merge per key with max(). The caches are per-process on a multi-worker server, so a single read can hit an empty (non-leader) worker — max-merge avoids both false negatives (missing a populated leader) and false positives (an `== 0` drain assertion passing only because it hit an empty worker). - test_leak: assert the initial-load `_wait_until` succeeded before deleting / before taking baseline, so the tests can't pass vacuously when load never completed. - refresh_all(): correct the misleading comment — a 404 is a no-op, there is no client-side fallback. - conftest: skip the suite cleanly if docker is unavailable (defense in depth; it's already excluded from the default pytest run via testpaths). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): isolate scopes per test and fix false repeat-sync gate Two review findings on the regression gates: - Cross-test contamination: clone paths are keyed by repo URL (source_id = sha256(url)+branch-shard), not scope_id, so scopes from different tests that point at the same seeded repo share one GitPolicyFetcher cache entry. With a session-scoped stack and no teardown, a leftover boot-*/stable-* scope kept those entries alive and would make test_churn's `repos == 0` drain assertion fail on fixed code. OpalServerClient now tracks created scopes and the opal fixture deletes them on teardown (best-effort drain wait, swallows errors so master — where delete never purges — doesn't fail the passing test). - False gate: test_repeat_sync_does_not_grow re-syncs identical scopes, which a path-keyed cache can't grow even on master, so it could never be the leak gate it claimed. Reframed as an honest idempotency guard (passes on master) that points at test_churn_releases_caches as the real leak gate; README's "Expected on master" reclassifies it alongside the postgres-bounce guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): make the regression gates trustworthy (address PR review) Addresses the CHANGES_REQUESTED review on PR #922. Root cause behind most findings: a fresh scope's first sync takes the _clone() branch, which only fills GitPolicyFetcher.repo_locks; repos/repos_last_fetched are filled on a *second* sync. So the load gates on `repos` hung, and the 2-worker per-process caches made `== 0` drain assertions unsafe. Blocking: - Single worker (UVICORN_NUM_WORKERS=1): deterministic per-process cache reads; removes the false `== 0` drain class. - Load gate (CRITICAL + Zivxx HIGH): _load_scopes gates on repo_locks then refresh_all() to force the second sync, so repos/repos_last_fetched are actually populated before any drain/purge assertion. - compose() surfaces captured stdout/stderr on failure. - Seed completeness asserted in conftest; seed script isolates per-repo failures and exits non-zero with a count. Secondary: - test_repeat_sync asserts an RSS bound (count can't grow for any impl); churn asserts all three caches drain + a loose RSS backstop. - blackhole socat sidecar replaces TEST-NET-1 (deterministic hang); offline test saturates the fetch executor with 40 hung clones and recovers via OpalServerClient.hard_reset() (stop -> redis FLUSHALL -> start). - Per-test clean slate deletes all server scopes (fixes orphan-scope leak). - Postgres-bounce proves broadcast recovery (PUT post-bounce, assert sync) and uses `up -d --wait`. - Remove dead 404 branch in refresh_all; boot clock starts at restart; gitea-admin `|| true` -> "already exists"-only guard; README reworded. opal-server: - /internal stats route now takes the JWTAuthenticator dependency (protected when JWT on, no-op in the test bed); unit test asserts enforcement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(git-leak): apply black/isort/docformatter (pre-commit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): tighten stat polling and pin test-bed images (PR review) - snapshot a single /internal stats read per poll in the churn-drain and boot gates (consistent multi-key observation; fewer HTTP round-trips) - document why a 200 from the healthy scope can't be a masked default bundle, and why the stats route is intentionally a sync def - pin alpine/socat and the seed image's pip deps for reproducibility Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): isolate offline-hang healthy probe to a never-cloned repo Ziv (PR review, round 2) caught that the offline-resilience gate can false-PASS in the full-suite run. The "healthy" scope pointed at policy-repo-0000 (list_seeded_repos(1)[0]), but on-disk clones are keyed by URL-hash and survive compose restart/stop/start (opal_server mounts no volume at /opal; only `down -v` wipes them). test_boot/test_leak run first (alphabetical) and already clone every seeded repo, so the healthy scope hit the existing clone via _discover_repository, skipped _clone(), and served 200 without ever touching the saturated fetch executor — the gate that must FAIL on this branch (no PR3 timeout) passed. Fix: seed a reserved repo (policy-repo-healthy-probe) outside the numeric policy-repo-NNNN range that no boot/leak test enumerates, and point the healthy probe at it. A never-cloned repo forces a genuine fresh clone through the starved pool, so the gate fails correctly. The seed- completeness check in conftest now covers the reserved repo too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): harden harness teardown and tighten assertions (PR review) Follow-up to PR review of the git-leak/resilience test bed: - hard_reset: always restart opal_server + wait_healthy via a finally, so a failed redis FLUSHALL can't leave the server stopped (which would fail every later session-scoped test and, running in a test finally, mask the result). - delete_all_scopes drain: a transient /internal read error no longer counts as a successful drain (was `except: return`); keep polling to the deadline so a not-yet-drained cache can't leak into the next test once PR2 lands. - use stats(samples=1) for the zero-waiting drain/empty polls (the peak-merge only matters for load assertions; this also drops 3x HTTP per poll). - resilience: narrow broad `except Exception` to requests.RequestException (and RuntimeError for wait_healthy timeout) so harness bugs surface instead of masquerading as "never served"/"never recovered". - resilience: collapse `assert opal.stats()` + redundant re-read into one read. - debug_stats_test: assert rss_kb > 0 on Linux (was `>= 0`, which passed for the wrong reason where /proc is absent and RSS reads fall back to 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): make PR4/PR5 tests genuine gates (address PR review) Zeev's gate-coverage review found only 2 of 5 flagship tests (churn #1, offline #4) were genuine fail-now / pass-after gates. Lift the other three: - #5 broadcaster: run 2 workers (OPAL_TEST_WORKERS) so the Postgres backbone is actually fanned out cross-worker (references/debug-pubsub.md §3-4), and assert the gunicorn worker PIDs are unchanged across a transient bounce -- the in-place-reconnect signal that distinguishes #915 (PER-15065) from a plain worker respawn. Prove recovery via a servable post-bounce scope, not /internal cache counts (per-process, non-deterministic on 2 workers). - #3 boot: key completion on "all scopes served" (GET /scopes/{id}/policy == 200) instead of repo_locks (set at fetch start, so it undercounts the final clone); document the PR4 tight-BOOT_TARGET_SECONDS carry-forward. - #2 repeat-sync: rename to test_repeat_sync_rss_stays_bounded and drop the tautological len(repos) assertion; RSS is the sole (load-bearing) gate. Adds worker_pids() (/proc-based, matched host-side so the scan can't count its own sh -c wrapper) and the opal_multiworker fixture (recreate to 2 workers, restore to 1 on teardown). Validated live (--boot-scopes=50): #2/#3 pass, #5 passes (worker PIDs held across the bounce, post-bounce scope served), #1/#4 fail for the right reason (PR2/PR3 not landed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): address PR review round 3 (fixture robustness + cleanup) Zeev's latest inline batch: - pytest.ini: self-root the suite (app-tests/git-leak/pytest.ini) so `cd app-tests/git-leak && pytest --boot-scopes=N` is deterministic across pytest versions/cwd. (The documented command already works -- testpaths only applies when pytest runs from the rootdir -- but this makes it explicit.) - compose(): add a subprocess timeout (default 1200s) so a wedged up/wait/build fails fast instead of hanging session-scoped fixture setup to the CI job limit (pytest-timeout does not cover fixture setup). - delete_all_scopes(): cut the drain wait 20s -> 3s; on master the caches can't purge (the leak this gates), so the old wait burned ~40s of dead time per test across setup+teardown. - seed_gitea.py: inject push creds scheme-agnostically (urllib.parse) instead of string-replacing "http://"; drop the unused /seed-output token artifact and the seed-output volume (host uses basic auth, never the token). The order-dependent bounce signal (test_resilience.py) was already fixed in 8e24cb0 (asserts GET /scopes/post-bounce/policy == 200, not a delta on a shared process-global counter). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): harden gates per multi-agent review (H1 + M1-M6) Addresses the HIGH and all MEDIUM findings from the opal-development / python-pro / backend-architect microreview: - H1 (#5 vacuous pass): the broadcaster gate now positively verifies a disconnect+reconnect actually happened, not just that no respawn occurred. New helpers.broadcaster_connect_count() counts the reconnecting broadcaster's "listener connected to channel" log line; the test asserts it increased across the bounce (paired with worker PIDs unchanged = in-place, not respawn). - M1 (#4): move the 40 executor-saturating PUTs inside the try/finally so a PUT failure still runs hard_reset() instead of leaking hung clone threads into the session stack. - M2 (#1/#2): _wait_until now treats a transient requests error from opal.stats() as "not yet" and retries, instead of ERRORing the test. - M3 (#3): measure a deterministic pure-cold boot (--force-recreate wipes the ephemeral FS -> preload cold-clones all N from Redis) instead of a nondeterministic warm/cold mix, so PR4's tight BOOT_TARGET_SECONDS can gate. - M4: verify the single-worker invariant -- opal_multiworker teardown asserts the stack is back to 1 worker, and the opal fixture asserts single-worker at setup, so a botched restore fails loudly instead of silently breaking cache gates. - M5 (#4): correct the reserved-probe comment (serving shares the fetch executor, so a shared repo would be starved on serve too; the probe additionally exercises the clone). - M6: gitea-admin retries on "database is locked" (CLI mutating live SQLite); rewritten as a `|` literal block so the create call stays on one line. Validated live (--boot-scopes=20): #2/#3 pass, #5 passes (reconnect count increased across the bounce + PIDs unchanged + scope served), #1/#4 fail for the right reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(git-leak): add PR2 over-purge/update-path gates + during-outage publish guard (review items 1-4) Coverage additions from the test-coverage review pass: 1. Unit tests for the server.py wiring: build the real OpalServer app and assert /internal/git-fetcher-cache-stats is mounted iff DEBUG_INTERNAL_STATS is on — removing the register call from _init_fast_api_app now fails the unit suite. 2. test_shared_repo_survives_sibling_scope_delete: deleting one of two scopes sharing a repo URL must not purge the URL-keyed cache entry or break the survivor (guards PR2 against over-purging; churn only covers the all-scopes-gone direction). 3. test_scope_repoint_releases_old_repo_cache: PUT /scopes is create-or-update, so re-pointing a scope orphans the old URL's cache entries — a leak path churn (delete-only) never takes. Red until PR2's purge covers updates too. 4. Postgres-bounce test: bounce_postgres gained a during= callback (with a finally that restores Postgres even if it raises) and the test now PUTs a scope mid-outage, asserting (d) it becomes servable after recovery — its sync trigger must be buffered/replayed across the gap, not silently dropped. Uses a distinct seeded repo from (c) so a stale on-disk clone can't satisfy it vacuously. Validated live (--boot-scopes=2): #2 and the bounce test pass; #3 fails for the right reason (caches stuck at 2/2/2 after the delete — PR2 not landed). Unit suite: 52 passed; pre-commit clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: zivxx <zivxx1@gmail.com>

@zeevmoney

…phan-sweep safety/perf, zombie clamp, doc drift Five HIGH / five MEDIUM / two LOW findings, all reproduced before fixing. Lifecycle (HIGH #1, #2): - The watcher's leader purge subscription now uses its OWN subscriber id. PubSubEndpoint files every server-side subscription under one shared id and EventNotifier.unsubscribe deletes that id's whole callback list for a topic, so unsubscribing by topic in stop() also dropped the every-worker handle_purge_message registered once at boot in server.py — leaving the process deaf to fleet purges for life, with nothing to re-add it. - stop() now cancels its tasks BEFORE draining in-flight purges, and bounds the drain (_PURGE_DRAIN_TIMEOUT=5s). The old order awaited lock_source held by a sync across a whole clone/fetch — unbounded when SCOPES_GIT_FETCH_TIMEOUT is 0 — whose release required the very cancellation the drain was blocking, while still holding the leadership lock. LeaderScopePurger gains signal_stop() and its "awaiting here cannot hang" docstring claim is gone (it was false). - stop() is idempotent, and the comment claiming start()/stop() run exactly once is corrected: __aexit__ and stop_server_background_tasks both call it. Orphan sweep (HIGH #3, MEDIUM #7): - A SUCCESSFUL empty store read is no longer taken as "everything is an orphan". ScopeRepository.all() is a Redis SCAN loop that returns zero keys and no error against a wrong or empty keyspace, so a REDIS_URL on the wrong DB index, a failover to an empty replica or a stray FLUSHDB would rmtree every tenant's clone and broadcast confirmed purges fleet-wide. Refused (error-logged) unless the new OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE opts in; the git-leak bed sets it, since its FLUSHALL gate wants exactly that reclaim. - The under-lock re-check now takes ONE fresh scopes.all() for the whole candidate batch instead of one per candidate (a full SCAN + a parse per record + two sha256 per live scope, each time): 2903ms -> ~10ms at 2000 all-orphan scopes, O(scopes + dirs) instead of O(orphans x scopes) Redis round trips. Validation and docs (MEDIUM #10, #9, LOW #11, #12): - SCOPES_GIT_MAX_ZOMBIES is clamped with max(0, ...). Unclamped, a negative value is truthy and `count >= cap` holds with nothing in flight, so the first git op was refused and no scope ever synced — while the one error line said "remotes appear stuck". - Deleted the service.py prose still arguing that phase 2 must NOT inherit phase 1's cap, which the same commit reversed. NOTE: phase-2 concurrency did drop from a hard 32 to SCOPES_GIT_MAX_WORKERS (default 10) — the knob is now a true ceiling, but that default change was not advertised. - Rewrote _periodic_orphan_sweep's docstring, which still described the sweep as living in _periodic_polling. - Restored OPAL_SCOPES_GIT_FETCH_TIMEOUT's configuration.mdx text verbatim from config.py (drifted again after adade57 fixed it once). - Documented the pop-before-publish invariant at both sites: publish() runs local subscribers inline, so handle_purge_message re-enters lock_source; the pop is what makes it mint a fresh lock instead of deadlocking. Addresses review comments: - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) - #924 (comment) (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…3) (#924) * feat(opal-server): add scope git fetch timeout + dedicated pool config Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(opal-server): dedicated git thread pool with hard per-op timeout Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): bound scope clone/fetch with timeout, skip on hang Wire scope clone/fetch through run_in_git_executor with SCOPES_GIT_FETCH_TIMEOUT, and broaden the _clone except to catch asyncio.TimeoutError so a hung clone is logged and the scope skipped instead of crashing the caller. Drop the now-unused run_sync import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): normalize asyncio.TimeoutError to builtin + format On Python < 3.11 asyncio.TimeoutError is a distinct class from the builtin TimeoutError, so run_in_git_executor's wait_for timeout was not caught by `pytest.raises(TimeoutError)` — failing build (3.9)/(3.10). Normalize to the builtin TimeoutError so the documented contract holds on every supported Python, and update the _clone catch site to match. Also apply black/isort/docformatter formatting to satisfy pre-commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): address Copilot review on git resilience - run_in_git_executor: use asyncio.get_running_loop() instead of the deprecated get_event_loop() inside an async function - fetch_and_notify_on_changes: set repos_last_fetched only after a successful fetch so a timeout/error does not wrongly suppress a later force_fetch via _was_fetched_after - fetch_timeout_test: measure elapsed time with time.monotonic() Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): clean fetch-timeout logging, trim test thread sleeps The fetch path let TimeoutError propagate to sync_scope's catch-all, which logged a full traceback at ERROR level for the expected unreachable-repo case — inconsistent with the clone path's quiet logger.error. Catch TimeoutError at the fetch site and log without a traceback, then skip (repos_last_fetched stays stale so the next cycle retries). Also shorten the hanging-thread sleeps in the timeout tests so the lingering pool thread doesn't delay process teardown. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(opal-server): address Zeev review on git resilience - git_fetcher: document that the dedicated scope-git ThreadPoolExecutor reads SCOPES_GIT_MAX_WORKERS once on first use, caches for the process lifetime (not runtime-reconfigurable), and is never explicitly shut down — matches the PR3 design. - 05-config-reference: fix stale config.py line refs after the master merge shifted the keys — SCOPES_GIT_FETCH_TIMEOUT 150-156 -> 196-202, SCOPES_GIT_MAX_WORKERS 157-163 -> 203-209. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): harden git resilience — fork-safety, single-flight, concurrent sync Addresses review findings on PR3 (never stuck on an offline repo): - CRITICAL: reset the dedicated git ThreadPoolExecutor after fork (os.register_at_fork) and shut it down at the end of preload_scopes. A pool built in the pre-fork gunicorn master was inherited with dead worker threads by every worker, so the leader's scope sync stalled forever (silent policy staleness). Verified with a fork repro on 3.12. - HIGH: never use the non-thread-safe pygit2 Repository from two threads. A timed-out clone/fetch keeps running on its pool thread while the per-source_id lock is released; a per-source_id in-flight guard now skips a cycle while a prior op is still lingering. run_in_git_executor switches asyncio.wait_for -> asyncio.wait so a timeout never cancels the future (the thread runs to completion and clears the in-flight marker). - HIGH: sync scopes concurrently, bounded by SCOPES_GIT_MAX_WORKERS, so one unreachable repo no longer serially blocks boot and other scopes. - MEDIUM: daemon-thread pool so a lingering git op can't block interpreter shutdown. - MEDIUM: stamp repos_last_fetched with the fetch start time on success (was completion time, which could wrongly suppress a force_fetch whose req_time falls within an in-flight fetch). - rmtree(ignore_errors) for the abandoned-clone race; harden the env-sensitive config-defaults test; correct config/doc wording ("logged and skipped" instead of "marked failed"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(opal-server): correct git-resilience worst-case boot bound (review #2) The `ceil(offline / workers) × timeout` bound was optimistic: the soft timeout unblocks the awaiting coroutine but the timed-out op keeps its pool thread until the OS network timeout, so with offline >= workers a healthy repo queues behind lingering threads up to the OS/TCP timeout, not `ceil × timeout`. Restate the guarantee this actually delivers (event-loop isolation + a bounded per-slot stall), reference the app-tests/git-leak 40-offline/10-worker case, and fix the two config.py line refs (196-203, 204-211). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(opal-server): add SCOPES_PURGE_CHANNEL config key Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): scope purge command + every-worker memory purge handler Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): subscribe every worker to the scope purge channel Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): leader-side disk purge handler on the scope purge channel Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(opal-server): pin leader purge handler garbage-payload tolerance; consolidate purge imports Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): route scope delete purge through the fleet-wide channel Rewires ScopesService.delete_scope to publish a ScopePurgeCommand on SCOPES_PURGE_CHANNEL instead of rmtree'ing the clone dir inline. The record delete now happens before local-memory purge + publish (both in a finally: so an ambiguous delete outcome still purges/publishes), and disk mutation + sibling-check are owned entirely by the leader's purge.py handler wired in earlier tasks. Drops the now-dead _purge_source_cache_if_unshared/_find_scope_sharing_source and the shutil/run_sync imports they needed. Updates the stale PR3-pending comment on the DELETE route and the fixture in delete_scope_route_test that monkeypatched the removed shutil import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): broadcast purge for the old source when a scope is repointed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): unreadable prior record must not block scope PUT Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): check scope liveness before clone to close the resurrection race Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(opal-server): leader orphan clone-dir sweep after sync passes Reconciliation backstop: after boot sync and each periodic sync pass, the leader lists the clone-sources dir and rmtrees any entry no live scope references, publishing a "orphan" purge command so every worker drops its in-memory cache too. Guards: a store-scan exception aborts the whole sweep (never read a transient error as "no scopes exist"); per-orphan liveness is re-checked under lock_source before deleting; git_op_in_flight defers the reclaim to a later pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(opal-server): make sweep store-error abort test discriminating; log boot sweep failures - Strengthen test_store_error_aborts_sweep_without_deleting to use BrokenOnceRepo (raises only on first .all() call) instead of always-raising BrokenRepo, ensuring the test discriminates whether the sweep aborts at initial scan (all_calls==1) vs continuing to per-entry recheck (all_calls>1). - Wrap boot-time sweep_orphans() in try/except to log exceptions, preventing silent failure while periodic polling retries on schedule. - Mutation verified: changing purge.py return to live=set() makes test fail (all_calls==3, both clones deleted); restoring return makes it pass. - All 18 tests pass (orphan_sweep_test.py + purge_channel_test.py). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): retryable 503 when a live scope's clone is transiently broken Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(git-leak): lift delete-vs-sync churn exclusion closed by PR3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): run leader disk purge off the publish path (bounded DELETE/PUT) LeaderScopePurger.handle ran purge_source_if_unshared inline, awaiting lock_source(source_id) on the pub/sub publish path. A hung fetch holding that lock (up to SCOPES_GIT_FETCH_TIMEOUT, 120s default) meant DELETE /scopes/{id} and repoint PUT latency was unbounded. handle now schedules the purge as a background task and returns immediately; the purge itself still serializes on the source lock as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): two-phase purge — leader sibling-check gates all memory purges Routes and the leader's own request now publish ScopePurgeCommand with confirmed=False; every-worker handle_purge_message ignores unconfirmed requests. Only the leader's purge_source_if_unshared acts on requests (sibling-check under lock_source), and republishes the same command with confirmed=True after a successful (or already-gone) disk purge — that confirmation is the only thing that drops in-memory cache entries anywhere, including on the leader itself and the deleting worker. The orphan sweep, which already IS the leader, publishes confirmed=True directly. This closes the over-purge bed regression where deleting one scope dropped cache entries a sibling scope was still using, because memory purges ran unconditionally on both the DELETE route and every worker's handler with no sibling check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): sweep orphans on refresh-triggered syncs; log background purge failures - Rename _boot_sync_then_sweep to _sync_all_then_sweep (runs on boot and refresh-all triggers) - Update trigger() else-branch to call _sync_all_then_sweep instead of sync_scopes - Update log message from "Boot-time orphan sweep failed" to "Orphan sweep failed" - Add _purge_and_log wrapper to catch and log background task exceptions - Update handle() to use _purge_and_log to prevent asyncio noise on purge failures - Add scopes_task_wiring_test.py to verify sync-then-sweep ordering on all paths Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): zombie-aware git executor — timed-out ops stop consuming capacity Replace the shared fixed-size git thread pool with a per-loop asyncio semaphore bounding LIVE (non-timed-out) ops at SCOPES_GIT_MAX_WORKERS. Each op now runs on its own single-use daemon-thread executor; on timeout the semaphore slot is released immediately so a lingering ("zombie") op no longer consumes capacity — only the OS network timeout bounds zombie count. Fixes the offline-repo bed gate where N hung clones > pool size permanently starved healthy scopes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(opal-server): correct executor capacity model in config description and preload comment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(git-leak): allow_worker_restart marker on force-recreating boot test The mid-test --force-recreate resets container-local /proc PIDs; the I5 before/after pid comparison spans two container generations and reported a false worker-crash. Same exemption as test_boot_states.py's force-recreate tests. Root-caused in .superpowers/sdd/fix-d-investigation.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): drain lock/timestamp entries and confirm when disk purge defers to the sweep Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(git-leak): bed timeout env, repoint gate flipped to green guard, chown after compose cp Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): publish purge confirmation under the source lock (use-after-free) purge_source_if_unshared published the confirmation AFTER exiting lock_source. Exiting the context manager is an await point, so a re-created scope's sync could acquire the lock, cache a fresh pygit2 Repository handle, and enter _notify_on_changes (which holds that handle across an await) before the stale confirmation ran inline through handle_purge_message -> purge_local_memory -> forget_repo and freed it out from under the concurrent caller (use-after-free). Move the confirmation publish to the end of the locked block, matching sweep_orphans, which already publishes under its lock. No deadlock: the only local subscribers are handle_purge_message (never takes lock_source) and LeaderScopePurger.handle (returns early on confirmed=True). Added a discriminating regression test that captures the source lock object before the purge runs and checks .locked() from inside the fake pubsub endpoint's publish() (same-task state check, since publish() runs on the purge's own task): True only if publish happens before the lock is released. Verified by mutation - moving the publish back outside the lock makes the test fail again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): liveness probe rejects a repointed scope's stale source _scope_still_exists only checked whether the scope record still existed. After a PUT repoints a scope from source A to source B, a sync that captured source A before the repoint still passes the probe and re-clones A after the leader already purged it, orphaning a dir until the next sweep. The probe now also confirms the fresh record still points at the source this fetcher is syncing (compares source_id), not just that the scope_id exists. Non-ScopeNotFoundError exceptions still propagate to git_fetcher's fail-open catch, unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: correct git executor capacity model in the config reference Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: apply docformatter/black to PR3 files Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): derive purge clone path from source_id, never trust the wire path (arbitrary rmtree) A forged __opal_scope_purge__ pub/sub message's clone_path field was fed unvalidated into shutil.rmtree (leader) and forget_repo/Repository.free (workers). The channel's only gate is OPAL's fail-open topic ACL, so any client JWT lacking permitted_topics could publish a forged purge and make the leader rmtree an arbitrary path as the opal user, or free a live repo handle on workers. Derive the clone path locally from the validated source_id (sha256hex-<shardindex>, confined to base_dir/git_sources) instead, in both the worker handler and the leader's purge_source_if_unshared. The wire clone_path field stays in the model for logging/mixed-fleet additivity but is never used for filesystem I/O. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): don't register daemon git threads in the atexit join queue The _DaemonThreadPoolExecutor used daemon=True so a lingering (timed-out) git call could not block interpreter shutdown — but it still registered every worker in concurrent.futures' global _threads_queues, which _python_exit joins unconditionally at exit, daemon or not. A hung fetch therefore blocked graceful shutdown / rolling restart: the 'stuck on an offline repo' hang this executor prevents, relocated to process exit. Drop the registration; normal shutdown uses self._threads + queue sentinels and is unaffected. Verified on py3.11. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): release the git live-op semaphore slot on cancellation run_in_git_executor released its live-ops semaphore permit via per-branch _release_once() calls after await sem.acquire(), but the await asyncio.wait( {fut}, timeout=timeout) on the timeout>0 path was not wrapped, so a cancellation landing inside that await propagated CancelledError without releasing the permit, permanently shrinking live-op capacity. Wrap the entire post-acquire body in a single try/finally so the slot is always freed regardless of where the cancellation lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): publish repoint purge even on an ambiguous scope PUT outcome delete_scope was hardened with a try/finally so a record delete that commits server-side but raises to the client still publishes the purge; the repoint path in put_scope had no equivalent. If scopes.put() commits but raises, the old-source purge was skipped, and a client retry sees old_source_id == new_source_id (store already updated) and never re-triggers it -- self-healing only via the slower orphan sweep. Move the repoint-purge publish into a finally on the put() call, mirroring delete_scope; over-publishing is safe since the leader sibling-checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test+docs(opal-server): close review-flagged coverage and clarity gaps - Cover the liveness probe's delete-resurrection branch through the real ScopesService closure (previously only exercised via a hand-rolled probe). - Assert the boot-sweep's failure path actually logs, so deleting the log call would break the test. - Fix SCOPES_GIT_FETCH_TIMEOUT's description: it's a soft timeout, not hard. - Comment sweep_orphans' in-flight continue to explain why it doesn't mirror purge_source_if_unshared's immediate lock/timestamp drain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(opal-server): clear the fetcher cache in the master pre-fork so workers don't inherit unpurgeable handles Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: apply isort/docformatter to review-fix files Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(opal-server): restore event loop after preload_scopes asyncio.run (py3.9 isolation) preload_scopes() runs asyncio.run(), which closes its loop and leaves the main thread with no current loop. On Python 3.9 (unlike 3.10+) asyncio primitives grab the loop at __init__, so later sync tests constructing asyncio.Lock() raised 'no current event loop' — build(3.9) went red while 3.10-3.12 stayed green. Restore a fresh loop in a finally so cross-test isolation holds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(opal-server): bound the local scope-sync pass separately from the git pass sync_scopes ran both passes under one asyncio.Semaphore sized to SCOPES_GIT_MAX_WORKERS. Phase 2 (scopes reusing an already-handled repo) does no network fetch in the common case — _should_fetch returns False, so it is only a disk open + local change-check + notify — yet it inherited the network cap meant for phase 1's concurrent clones. Split the bound: phase 1 (distinct repos, network) stays capped at SCOPES_GIT_MAX_WORKERS; phase 2 gets a wider bound (max(git, 32)) so the cheap local checks are not throttled to the conservative git number. Any rare re-fetch phase 2 still triggers (branch missing after phase 1) remains throttled by the inner git-op semaphore in run_in_git_executor, so the network is never over-driven. 32 matches asyncio's default thread-pool ceiling, which is the real limiter on phase-2 disk opens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(opal-server): freeze-exempt SCOPES_PURGE_CHANNEL by configured name A non-'__' operator override of the purge channel would be frozen during a backbone gap, silently dropping fleet-wide cache purges. Add it to the explicit freeze_exempt_topics list alongside the other overridable coordination channels. Addresses review comments: - config.py:312 (@zeevmoney) * style(opal-server): wrap SCOPES_GIT_MAX_WORKERS description Addresses review comments: - config.py:226 (@zeevmoney) * feat(opal-server): cap in-flight git ops via SCOPES_GIT_MAX_ZOMBIES Export git_busy_count() as a zombie gauge and refuse new scope git ops (log-once) once in-flight ops reach SCOPES_GIT_MAX_ZOMBIES (default 4x MAX_WORKERS=40), bounding thread growth during a remote outage. (libgit2 transport-timeout half deferred.) Addresses review comments: - git_fetcher.py:229 (@zeevmoney) * fix(opal-server): shape-check CPython worker before daemon-thread spawn Validate cf_thread._worker arity (==4) and self._initializer before mirroring CPython internals; fall back to the stdlib on mismatch instead of spawning a malformed thread. Cap python_requires at <3.13 until newer internals are verified. Addresses review comments: - git_fetcher.py:71 (@zeevmoney) * fix(opal-server): non-retryable 409 for permanently-broken scope branch Bind and repr-log the scope-policy exception; route the permanent 'branch head not found' case to a non-retryable 409 instead of an endless 503 retry loop. Transient clone-vanished / OSError stays on 503 + Retry-After. Addresses review comments: - api.py:342 (@zeevmoney) * style(opal-server): apply black/isort to cluster C5 files No behavior change; black-wraps multi-line conditionals/calls and isort-sorts new imports introduced across the C5.1/C5.2/C5.3 commits. * fix(opal-server): make _git_busy_lock fork-safe with full at-fork triple Acquire the lock before fork and reinit (not re-acquire) it in the child so a lock inherited mid-mutation cannot deadlock a worker. Addresses review comments: - git_fetcher.py:135 (@zeevmoney) * fix(opal-server): skip freeing in-flight repo handles in reset_caches Mirror purge_local_memory's guard: a pygit2 handle whose git op is still lingering on a daemon thread must not be free()'d by the pre-fork cache reset, only dropped from the cache. Addresses review comments: - task.py:117 (@zeevmoney) * fix(opal-server): keep in-flight markers across pre-fork teardown shutdown_git_executor no longer clears _git_busy so reset_caches sees which sources have a lingering git op; the forked child clears the stale markers. Addresses review comments: - task.py:117 (@zeevmoney) * feat(opal-server): bounded drain of in-flight git ops before fork preload_scopes waits up to SCOPES_GIT_PRELOAD_DRAIN_TIMEOUT for lingering clone/fetch ops to clear their markers before the pre-fork teardown; ops still busy at the bound are left running and protected by reset_caches's in-flight guard. Deferred follow-up (gated on boot-test Run B): subprocess isolation (option B). Addresses review comments: - task.py:117 (@zeevmoney) * style(opal-server): apply black/isort/docformatter to cluster C3 files Cosmetic only (docstring rewrap, import sort, blank-line/spacing style) — no behavior change. * feat(opal-server): always-on orphan-sweep timer independent of POLICY_REFRESH_INTERVAL Addresses review comments: - purge.py:195 (@zeevmoney) - service.py:213 (@zeevmoney) * feat(opal-server): best-effort local memory purge on the serving worker at delete Drop this worker's in-memory fetcher caches for the deleted source (memory only; disk stays leader-gated) so a client-less non-leader does not pin the pygit2 handle for life. Fix the now-accurate DELETE route comment. Addresses review comments: - service.py:213 (@zeevmoney) * fix(opal-server): reason-aware fail-open — repoint keeps clone on a raising sibling scan Addresses review comments: - api.py:157 (@zeevmoney) * perf(opal-server): hybrid orphan sweep — snapshot-filter live dirs, re-check candidates under lock One scopes.all() filters the bulk of clearly-live dirs (no per-orphan scan); only candidate-orphans get a fresh under-lock re-check before deletion, which serializes against a PUT's sync so a re-claimed clone is kept. is_dir() moved off the event loop via os.scandir; a raising re-check is logged, not swallowed. Also inverts test_delete_route_does_not_purge_fetcher_caches_without_pubsub (delete_scope_route_test.py, out of the brief's declared file list but a direct route-level duplicate of the C4.2 behavior change) so the full opal-server suite stays green. Addresses review comments: - purge.py:265 (@zeevmoney) - purge.py:270 (@zeevmoney) * style(opal-server): apply black/docformatter to cluster C4 test files Cosmetic-only: line-wraps a long monkeypatch.setattr call and a long ScopePurgeCommand call in purge_channel_test.py, and re-wraps one docstring in delete_scope_cache_purge_test.py. No behavior change. * test(opal-server): port purge TOCTOU + shard-distinct + defensive-warning regressions Addresses review comments: - packages/opal-server/opal_server/tests/purge_channel_test.py:209 (@zeevmoney) * test(opal-server): cover start() leader wiring; defer _periodic_polling Addresses review comments: - packages/opal-server/opal_server/tests/scopes_task_wiring_test.py:27 (@zeevmoney) Only C7.5 (start() subscribes the leader purge handler to SCOPES_PURGE_CHANNEL) landed here. C7.4's two _periodic_polling cancellation tests were withheld, not committed red or weakened: they deterministically fail against current code (task.py:82's outer `except asyncio.CancelledError:` logs and does not re-raise, so a cancelled task completes normally instead of raising to its awaiter — confirmed with a standalone asyncio repro, independent of pytest/timing). A NOTE in the test file and the cluster-C7 report document the gap for follow-up. * test(opal-server): cover DELETE route purge publish-through Addresses review comments: - packages/opal-server/opal_server/tests/delete_scope_route_test.py:71 (@zeevmoney) * docs(opal-server): document scope-git config keys; drop private planning doc; fix dangling ref Addresses review comments: - .claude/plans/docs/05-config-reference.md:1 (@zeevmoney) - .claude/plans/docs/05-config-reference.md:24 (@zeevmoney) * fix(opal-server): re-raise CancelledError in periodic loops; land _periodic_polling tests The outer except-CancelledError in _periodic_polling and _periodic_orphan_sweep logged without re-raising, swallowing cancellation (harmless today via stop()'s gather(return_exceptions=True) but a latent footgun). Re-raise to honor the cancellation contract; land the previously-withheld C7.4 tests. Addresses review comments: - packages/opal-server/opal_server/tests/scopes_task_wiring_test.py:27 (@zeevmoney) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(opal-server): match OPAL_SCOPES_GIT_FETCH_TIMEOUT description to config.py verbatim * revert(opal-server): drop C4.2 local memory purge on the serving worker The bed's I4 invariant caught it: purge_local_memory under lock_source mints a repo_locks entry it never pops, so churn accumulates stray repo_locks keys with no live scope; it also over-purges a shared source's memory locally. Draining memory on a client-less non-leader is deferred to the lock-lifecycle work (Plan B / C1); C4.1's always-on sweep timer already backstops the disk half. Reverts part of 82c7a4ea. Re: service.py:213 (@zeevmoney) * fix(opal-server): address PR3/PR4 review findings — purge-channel authz, transient-branch 409/503, observability Post-review hardening from a 3-round adversarial+constructive review of the boot-resilience branch. Security - Reject SCOPES_PURGE_CHANNEL publish/subscribe from external RPC peers. The channel is server-internal (a purge evicts GitPolicyFetcher caches, and via the leader deletes clone dirs, fleet-wide), but _verify_permitted_topics default-allows tokens with no permitted_topics claim, so any connected client/PDP could forge a fleet-wide purge. Legitimate server + broadcaster publishes are channel=None and bypass the gate; clients only ever publish STATISTICS_ADD_CLIENT_CHANNEL, so no client is affected and no redeploy is needed. (+tests pinning that client traffic is untouched) Correctness - _get_current_branch_head now distinguishes a PERMANENT missing branch (KeyError -> BranchHeadNotFoundError -> non-retryable 409) from a TRANSIENT gutted object store (pygit2.GitError -> propagates -> retryable 503). It was collapsing both to a 409, telling clients "not retryable" for a scope the sync path is actively self-healing. (+tests exercising the real raise path) Observability (Datadog) - Log the preload drain timeout instead of discarding drain_git_ops()'s result — that is the one condition that carries lingering git ops across the fork, and it was silent. - Continuous git_ops_in_flight gauge, not just the one-shot log at the cap. - Orphan-sweep heartbeat summary so a healthy no-op sweep is visible. - metrics.event on the 503/409 policy-unavailable responses. Hygiene - Clean-log GitConcurrencyLimitExceeded (no per-scope traceback during a zombie-cap outage). - \Z (not $) in the source_id regex — block the trailing-newline bypass. - Skip the always-on orphan-sweep timer when periodic polling already sweeps (avoid duplicate scans + purge broadcasts). - Fix misleading ScopePurgeCommand.reason/clone_path comments; drop an unused import. 199 opal-server unit tests + 21 git-leak bed tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QXknyVRt9oHeJ4BHAauu5c * style(opal-server): apply pre-commit formatting + fix py3.9 event-loop isolation in purge-channel tests CI fixups on ff5904ba: - black/isort/docformatter formatting (the local commit bypassed the hooks). - purge_channel_test F1 tests now await the restriction under @pytest.mark.asyncio instead of asyncio.run(), which on Python 3.9 leaves the current event loop set to None and broke later SYNC tests that call asyncio.get_event_loop() (reconnecting_broadcaster_test) — the same py3.9 isolation trap as 8400a75e. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QXknyVRt9oHeJ4BHAauu5c * fix(opal-server): address Zeev's round-2 review (13 findings) — purge UAF/authz, sweep perf/validation, lifecycle, concurrency knob, docs Security / correctness - pubsub: also reject ALL_TOPICS from external peers on the purge-channel gate (the gate guards subscribe too, and notify() fans every topic into the ALL_TOPICS bucket, so an ALL_TOPICS subscriber would still receive purge traffic; no opal-client subscribes to ALL_TOPICS, so it's safe). - purge: the every-worker handler now purges under lock_source (closing the use-after-free the leader's confirmation-under-lock prevents, previously unguarded on every non-publishing process) and pops the repo_locks entry it mints (invariant I4). - purge sweep: validate each dir name via _confined_clone_path before rmtree (the source-id SECURITY invariant every other deletion path enforces); the in-flight-defer branch drains the repo_locks entry it mints (I4). - git_fetcher: _get_current_branch_head distinguishes a permanent missing branch (KeyError -> 409) from a transient gutted object store (pygit2.GitError -> retryable 503). Performance - purge sweep: precompute the live source_id set once (O(scopes)) and filter dirs by O(1) membership with a periodic yield, replacing an O(dirs x scopes) double-sha256 scan that blocked the leader's event loop. Concurrency knob (finding 11) - service.py: bound phase-2 by max(1, SCOPES_GIT_MAX_WORKERS) instead of max(MAX_WORKERS, 32). 32 was a floor the knob could not lower, and it over-subscribed the default executor that also serves policy bundles. Docs in config.py + configuration.mdx corrected to match. Lifecycle - task: orphan sweep is always-on (independent of POLICY_REFRESH_INTERVAL, as documented); _periodic_polling no longer also sweeps (no duplicate scans / broadcasts); the boot sync_scopes is wrapped so a store hiccup can't kill the watcher task silently; stop() unsubscribes the purge handler and drains in-flight purges so shutdown can't abandon an rmtree. Docs (finding 8) - The per-fetch timeout is documented honestly as SOFT: it unblocks the event loop, not the thread; the pinned libgit2 enforces no read timeout, so a black-holed remote can pin the thread for the process's life, and SCOPES_GIT_MAX_ZOMBIES is the real bound (git_fetcher.py, config.py, configuration.mdx). Packaging - opal-client capped python_requires <3.13 to match opal-common/opal-server (it hard-depends on the capped opal-common, so 3.13 installs failed). Tests - cover the sync-path git_op_in_flight guard, the confirmed gate (was masked by an invalid source_id), recreate-after-delete under the fleet-purge design, the repo_locks-leak regressions, and the phase-2 knob bound; update the sweep/polling/perpass tests for the behavior changes. Bed: add a repo_locks-drain settle to hard_reset so the offline-repo test's invariant check can't race the post-restart boot sweep (flaky I4). 203 opal-server unit tests green; git-leak bed clean of invariant violations (the repoint/postgres timing tests flake intermittently on a loaded local box, unrelated to these changes — the broadcaster path is untouched). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QXknyVRt9oHeJ4BHAauu5c * fix(opal-server): address Zeev's round-3 review — purge lifecycle, orphan-sweep safety/perf, zombie clamp, doc drift Five HIGH / five MEDIUM / two LOW findings, all reproduced before fixing. Lifecycle (HIGH #1, #2): - The watcher's leader purge subscription now uses its OWN subscriber id. PubSubEndpoint files every server-side subscription under one shared id and EventNotifier.unsubscribe deletes that id's whole callback list for a topic, so unsubscribing by topic in stop() also dropped the every-worker handle_purge_message registered once at boot in server.py — leaving the process deaf to fleet purges for life, with nothing to re-add it. - stop() now cancels its tasks BEFORE draining in-flight purges, and bounds the drain (_PURGE_DRAIN_TIMEOUT=5s). The old order awaited lock_source held by a sync across a whole clone/fetch — unbounded when SCOPES_GIT_FETCH_TIMEOUT is 0 — whose release required the very cancellation the drain was blocking, while still holding the leadership lock. LeaderScopePurger gains signal_stop() and its "awaiting here cannot hang" docstring claim is gone (it was false). - stop() is idempotent, and the comment claiming start()/stop() run exactly once is corrected: __aexit__ and stop_server_background_tasks both call it. Orphan sweep (HIGH #3, MEDIUM #7): - A SUCCESSFUL empty store read is no longer taken as "everything is an orphan". ScopeRepository.all() is a Redis SCAN loop that returns zero keys and no error against a wrong or empty keyspace, so a REDIS_URL on the wrong DB index, a failover to an empty replica or a stray FLUSHDB would rmtree every tenant's clone and broadcast confirmed purges fleet-wide. Refused (error-logged) unless the new OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE opts in; the git-leak bed sets it, since its FLUSHALL gate wants exactly that reclaim. - The under-lock re-check now takes ONE fresh scopes.all() for the whole candidate batch instead of one per candidate (a full SCAN + a parse per record + two sha256 per live scope, each time): 2903ms -> ~10ms at 2000 all-orphan scopes, O(scopes + dirs) instead of O(orphans x scopes) Redis round trips. Validation and docs (MEDIUM #10, #9, LOW #11, #12): - SCOPES_GIT_MAX_ZOMBIES is clamped with max(0, ...). Unclamped, a negative value is truthy and `count >= cap` holds with nothing in flight, so the first git op was refused and no scope ever synced — while the one error line said "remotes appear stuck". - Deleted the service.py prose still arguing that phase 2 must NOT inherit phase 1's cap, which the same commit reversed. NOTE: phase-2 concurrency did drop from a hard 32 to SCOPES_GIT_MAX_WORKERS (default 10) — the knob is now a true ceiling, but that default change was not advertised. - Rewrote _periodic_orphan_sweep's docstring, which still described the sweep as living in _periodic_polling. - Restored OPAL_SCOPES_GIT_FETCH_TIMEOUT's configuration.mdx text verbatim from config.py (drifted again after adade574 fixed it once). - Documented the pop-before-publish invariant at both sites: publish() runs local subscribers inline, so handle_purge_message re-enters lock_source; the pop is what makes it mint a fresh lock instead of deadlocking. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3672441745 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672441854 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672441962 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442150 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442409 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442643 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442803 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442945 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672443042 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(opal-server): mutation-verified pins for the round-3 findings Every guard the round-2 commit added shipped green when mutated away — verified on this head, full suite each time (204 passed with each fix reverted). This commit closes that: 21 tests, each one demonstrated to FAIL under exactly the mutation it pins and pass on clean code. | mutation reverted | pinning test | |---|---| | fetch-timeout `return` -> fallthrough | test_timed_out_fetch_does_not_stamp_last_fetched_or_notify | | worker handler drops lock_source | test_handle_purge_message_waits_for_the_source_lock | | repo_locks.pop moved after the confirm publish | test_inline_confirmation_delivery_does_not_deadlock | | LeaderScopePurger.stop skips the drain | test_stop_awaits_a_slow_pending_purge | | stop() unsubscribes by topic (shared id) | test_stop_does_not_unsubscribe_the_every_worker_purge_handler | | drain awaited before super().stop() | test_stop_cancels_lock_holders_before_draining_purges | | boot sync loses its try/except | test_sync_all_then_sweep_still_sweeps_when_sync_raises | | sweep timer re-gated on POLICY_REFRESH_INTERVAL | test_orphan_sweep_timer_starts_even_when_polling_is_enabled | | sweep skips dir-name validation | test_sweep_leaves_non_source_id_dirs_untouched | | empty-store guard removed | test_empty_store_with_clone_dirs_refuses_to_reclaim | | fresh re-read restored per candidate | test_sweep_issues_one_fresh_read_for_the_whole_candidate_batch | | negative zombie cap unclamped | test_negative_max_zombies_is_treated_as_no_cap | | configuration.mdx description reworded | config_docs_drift_test (new, parametrized over all 7 scopes keys) | Two of these needed a real EventNotifier-backed PubSubEndpoint rather than the recording fake every other purge test uses — inline local delivery is what makes the re-entrancy and the shared-subscriber-id bugs observable at all. Existing sweep tests were adapted to the new empty-store policy (they used an empty FakeScopeRepository with clone dirs present, which is now refused) and to the batched re-read: test_redis_wiped_boot_reclaims_everything became the refuse-by-default pair, and the raising-re-check test now asserts the pass aborts rather than keeping one dir. 225 passed (204 before). opal-common 93 passed. The one opal-client failure (data_updater_test.py::test_data_updater_with_report_callback) reproduces identically with these files reverted to HEAD — pre-existing, not from this series. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3672442062 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442285 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442490 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672442150 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672441745 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3672441854 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(git-leak): scope the empty-store reclaim opt-in to the one test that needs it; bound the sweep's re-read staleness Setting OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE bed-wide was wrong twice over: every other test then exercised the behaviour the previous commit made opt-in rather than the shipped default, and it turned test_server_recovers_after_postgres_bounce red — the sweep reclaimed the clone of the scope PUT during the broadcaster outage, so it never became servable (503). That is the exact "deletes a live clone" mode the guard exists to prevent, caught by the bed. Verified: passes on fb1b823d, failed 2/2 with the key on bed-wide, passes again with it off by default. - docker-compose.yml now interpolates ${OPAL_TEST_RECLAIM_ON_EMPTY_STORE:-false}, and test_redis_wiped_boot_reclaims_clones flips it on for its own container (force-recreate to pick up the env, then seed the clone, since a recreate empties the clone tree) and restores the default on teardown — the same idiom as OPAL_TEST_WORKERS in the multiworker fixture. - test_orphan_clone_dir_is_reclaimed now runs with a live scope present instead of an empty store. Its old shape (zero scopes + one stale dir) is precisely what the new guard refuses, and the shape worth gating is the production one: real scopes alongside a stale dir, where reclaiming the stale dir must not touch the live clone — which it now also asserts. - The sweep's fresh live-set read is re-taken every _FRESH_READ_EVERY=200 candidates instead of once per pass. A single read per pass leaves the whole pass's duration as a window in which a PUT that re-claims a source is unseen and its just-cloned dir reclaimed — the failure above. The cadence bounds that window while keeping the pass O(scopes x dirs/200 + dirs) rather than the O(orphans x scopes) the per-candidate read cost; it also serves as the loop's event-loop yield. Extracted as _fresh_live_source_ids (returns None to abort, keeping the conservative bias on a raising scan or an underivable scope). - test_sweep_refreshes_the_live_set_on_a_cadence pins it (fails when the read is pinned to `i == 0`), alongside the existing one-read-per-batch assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(opal-server): fix sweep_orphans docstring left describing a single fresh read The "Hybrid" paragraph still said the re-check uses ONE fresh scopes.all() for the whole candidate batch, while the paragraph below it (and the code) describe a read per batch of _FRESH_READ_EVERY candidates — the same two-sides-of-a-decision prose problem flagged at service.py:283 in the round-3 review, introduced by the follow-up commit's own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(opal-server): round-4 review — restore per-candidate sweep freshness, add a reclaim ceiling, make refusals observable Round-3's batched fresh read was my change and it was a real regression: the read moved OUT of the per-candidate lock_source, so the set backing each delete decision aged by every lock acquisition and rmtree before it. A PUT re-claiming a source in that window was invisible and a LIVE tenant's clone dir was deleted, plus a confirmed orphan purge broadcast fleet-wide. My own docstring called that "a spurious reclaim plus a re-clone on the scope's next sync" — wrong at the shipped defaults: POLICY_REFRESH_INTERVAL=0 means _periodic_polling never runs, nothing re-clones, and the scope serves 503 until a webhook or manual refresh-all. Reviewer's reproduction matched, and the probe passes on fb1b823d. - The authoritative read is back inside `async with lock_source(name)`, per candidate (`_classify_candidate`), returning claimed / orphan / abort. There is no targeted lookup to make it cheaper: ScopeRepository is keyed by scope_id and `all()` is a SCAN, so this is the reviewer's stated fallback — batched set as pre-filter only. The round-3 perf win is kept where it actually mattered: an all-live pass still costs ONE scan (pinned by test), because only dirs that already look orphaned reach the per-candidate read. - `_FRESH_READ_EVERY` is gone. It was asked to be both a freshness bound and the event-loop yield cadence, which pull opposite ways; the yield cadence is now `_YIELD_EVERY` and means only what its name says. - NEW: `_reclaim_is_plausible` + OPAL_SCOPES_ORPHAN_SWEEP_MAX_RECLAIM_FRACTION (default 0.5). The zero-scope guard only catches a store that returns NOTHING; a store pointed at a wrong-but-populated keyspace answers successfully with someone else's scopes, none of whose source_ids match, so every local clone looks orphaned and the whole tree goes. The ceiling needs no way to tell a wrong store from a right one — it only notices that one pass is about to delete an implausible share. A single candidate is always allowed (the ordinary case), and an operator can raise or disable the fraction for a deliberate SCOPES_REPO_CLONES_SHARDS reshard. An opted-in empty-store reclaim skips the ceiling: that flag is already a declaration of intent for a mass reclaim. - Observability: metrics.event("ScopeOrphanSweepRefused") on every refusal (empty_store / implausible_share / scan_failed), and the heartbeat now fires on EVERY exit path with an explicit outcome and the partial reclaim count — an aborted pass no longer reads like a healthy "swept, found nothing", and a pass that deleted N dirs before aborting no longer reports nothing about them. - A failed rmtree no longer `continue`s past `repo_locks.pop`: the pop is in a `finally` (still before the confirmation publish, per the documented ordering), so a permanently-unreclaimable dir stops leaking one stray lock per pass (invariant I4). A symlink where a clone dir should be is now logged at error as the anomaly it is, not as a recurring reclaim failure. - Three comments promised that the shutdown drain's abandoned dirs are reclaimed by the next boot's sweep. With the empty-store refusal at its default that is false when the abandoned dir was the last one; all three now name the exception. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3682404581 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682404689 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682405015 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682405247 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682405349 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(opal-server): pin the round-4 fixes; close the drift guard's vacuous passes Nine tests added or replaced, and the two that pinned the stale batched design are gone (they would have failed the fix — the reviewer called that out). Mutation-verified, full suite each time; each mutation fails exactly the test that pins it: | mutation | caught by | |---|---| | hoist the fresh read out of the per-candidate lock (the round-3 shape) | test_candidate_that_goes_live_mid_pass_is_kept | | drop the plausibility ceiling | test_wrong_but_populated_keyspace_is_refused_by_the_ceiling | | let the ceiling veto an opted-in empty-store reclaim | test_opted_in_empty_store_reclaim_is_not_vetoed_by_the_ceiling | | failed rmtree continues past the repo_locks pop | test_failed_rmtree_leaves_no_stray_repo_lock | | sweep's pop moved below the confirmation publish | test_sweep_inline_confirmation_does_not_deadlock | | neutralise the per-candidate lock_source | test_sweep_waits_for_the_source_lock_before_deleting | | heartbeat only on the "complete" path | test_heartbeat_reports_outcome_and_partial_count_on_abort | The two sweep invariants that had NO coverage at all — the pop-before-publish ordering and the per-candidate lock — are covered now. Both needed a real EventNotifier-backed PubSubEndpoint: every sweep test used FakePubSubEndpoint, which appends to a list and never delivers inline, so no sweep test could observe the re-entrancy those guards exist for. The deadlock mutation now fails fast via asyncio.wait_for instead of wedging CI. test_all_live_pass_costs_one_scan keeps the round-3 perf property pinned (one scan for an all-live tree) so restoring per-candidate freshness cannot silently reintroduce O(dirs x scopes) for the common case. Drift guard: both demonstrated vacuous-pass modes are closed, and verified by breaking the DOCS against the unmutated guard rather than by mutating the test (mutating a test's own strictness is undetectable by construction): - docs file moved/renamed -> now FAILS inside a checkout instead of skipping all 7 cases (it only skips where documentation/ is genuinely absent, i.e. an installed package); - a key's heading renamed -> FAILS. This needed anchoring the heading match with its trailing newline: `#### OPAL_FOO` is a prefix of `#### OPAL_FOO_BAR`, so my first attempt at this fix still passed on a renamed heading; - a key's body paraphrased while the verbatim text sits under another key's heading -> FAILS, because assertions are now sliced to the key's own section. Plus each key's documented `Default:` must match config.py. 236 passed (226 before). opal-common 93 passed. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3682404791 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682404920 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682404581 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3682404689 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(git-leak): a fixture owns the destructive reclaim opt-in for its whole lifetime OPAL_TEST_RECLAIM_ON_EMPTY_STORE was exported before the `try:` whose `finally:` restores it, leaving the recreate, wait_healthy, put_scope and a 300s wait_until outside the protected region. Any of those raising left the variable "true" for the rest of the pytest process — and compose() inherits os.environ, so every later test that recreates opal_server would boot with the destructive reclaim on, which is exactly what this test's own docstring rules out and what the README's gate matrix depends on. The bed already caught this class of mistake once. The `reclaim_on_empty_store` fixture now owns the variable from the moment it is set: its teardown runs whether setup succeeded or not, and it pop()s rather than writing "false" so the compose default stays authoritative. Bed verified with the round-4 source changes (--boot-scopes=20): 7/7 — test_orphan_clone_dir_is_reclaimed, test_redis_wiped_boot_reclaims_clones (via this fixture), test_shard_reconfig_still_serves_but_orphans_old_clones (its single-candidate reclaim is unaffected by the new plausibility ceiling), test_boot_with_unreachable_remotes_still_serves_healthy, warm boot, corrupt clone, and test_churn_releases_caches. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3682405139 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(opal-server): self-audit against the reviewer's own lenses — stray locks on the sweep's keep/abort paths, unbounded read under the lock, unpinned guards Ran the 12 lenses reconstructed from all 56 review findings on this PR against the round-4 code before waiting for a round 5. Five defects, four of them in the code I wrote yesterday. 1. **Stray repo_locks entries (invariant I4).** The pop only covered the deletion attempt, so the two paths that DECLINE to delete — keep-on-error and the mid-pass abort — each left behind the entry lock_source minted for a candidate, i.e. a lock with no live scope. Exactly the class already filed twice (the in-flight branch, then the OSError branch). PoC before/after, both paths: `I4-violating paths: 2/2` -> `0/2`. The pop is now a `finally` around the whole locked block (still before the confirmation publish, per the documented ordering), which also makes the in-flight branch's explicit pop redundant. 2. **Unbounded store read held under a source lock.** `_classify_candidate`'s read has to be inside `lock_source` to be authoritative, but `RedisDB` is built with no socket_timeout/socket_connect_timeout — so an unreachable store would pin that source's lock for the life of the process and block every sync for it. Now bounded by `_STORE_READ_TIMEOUT` (10s); expiry keeps the candidate, so no deletion decision is ever made on a timed-out read. Deliberately a constant and not a knob: there is nothing to tune when the failure mode is already safe. 3. **A comment asserting something false** — the candidate loop claimed "every check below is O(1) or a bounded await" while that same read was unbounded. Rewritten. This is the lens that has produced 9 findings on this PR. 4. **An out-of-range reclaim fraction silently disabled a safety ceiling.** 1.5, 2 or -1 all disable it; disabling is the right reading of the intent, but doing it silently turns a typo into a disabled guard — the shape of the unclamped negative SCOPES_GIT_MAX_ZOMBIES finding. Warns once now (one-shot latch, mirroring `_zombie_cap_logged`), never per-pass. 5. **An unpinned guard.** Mutation-testing all ten load-bearing guards in the PR found nine pinned and one not: deleting the preload drain-timeout warning left the suite green, and that warning is the only signal that git threads survived into the forked workers. Two tests now (fires on timeout, silent on a clean drain, so it cannot pass by always-warning). Also hardened `test_failed_rmtree_leaves_no_stray_repo_lock`, which could have passed vacuously if a future guard stopped the candidate from ever reaching the deletion — it now proves the reclaim was attempted. Same critique the reviewer made of the drift guard, applied to my own test. The refusal log for the mass-reclaim ceiling now names bulk-delete-with-lost- broadcasts alongside a reshard, so on-call is not sent to REDIS_URL when the store is fine. Mutation-verified: F3 (PoC + test), F4, F4b (the latch), F1, and F2 (drop the wait_for -> the hung-store test fails) all fail their pinning test and pass clean. 241 passed (236 before). opal-common 93. The three docs attacks on the drift guard still fail it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(git-leak): make seeded repos distinct so the repoint gate can pass; refresh the matrix for post-PR3 reality `test_scope_repoint_releases_old_repo_cache` — PR3's update-path gate — has been failing for a reason that has nothing to do with the server: it never reached the cache-drain assertion it exists for, dying instead at its precondition ("scope never switched to serving the re-pointed content", test_leak.py:236). Cause: the seeder wrote byte-identical content into every repo and committed with a fixed author and message, so a repo's commit sha was determined solely by the wall-clock second it was pushed in. `docker compose down` drops the gitea volume, so every teardown forces a full re-seed, and 20 repos pushed in a loop routinely share a second. Two repos with the same sha serve byte-identical bundles, and the test's "did the content change?" check can then never become true — it burns its 300s poll and fails. Measured: a 3-way sha collision in one sample; after the fix, 20/20 repos have distinct shas. That also explains why it passed in isolation (reusing a volume whose two repos happened to straddle a second) and failed in full runs. `_data_json_for(name)` embeds the repo name in data.json, so the tree — and the sha — is unique per repo. The repoint gate is the only consumer of bundle bytes in the bed, and the other app-test suite uses its own repo, so nothing else is affected. PR3's repoint purge itself was always working: during a wedged run the server logged the purge firing and drained the old source's entries, leaving one clone dir (the new source's). Full bed after the fix: 20/21, with the repoint gate green. Matrix refreshed to describe the post-PR3 state instead of the pre-PR2 one: ten rows that still said "FAILS"/"unowned" now say what they guard and since when, keeping the "fails without X" wording as the reason each gate exists. Six docstrings that still said "RED until PR3" updated likewise. `test_boot_loads_all_scopes` deliberately still points forward — it is PR4's gate via BOOT_TARGET_SECONDS. The one row that is honestly red is documented as such: assertion (d) of test_server_recovers_after_postgres_bounce. See that row for the mechanism — it is a gap in the merged broadcaster work, not in PR3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(git-leak): back the postgres-bounce row with a measured pre-PR3 baseline The previous wording asserted this gate was "not owned by PR3" on the strength of one passing run at fb1b823d — which is a commit ON this branch (the round-2 state), not a pre-PR3 baseline. That comparison could not support the claim. Measured properly against origin/master, which IS this branch's merge-base (master has not moved since the fork) and already carries the bed, the debug-stats endpoint and the #933 broadcaster work: origin/master (pre-PR3): 1 pass / 4 runs PR3 head: 2 passes / 6 runs Same assertion (d), same line (test_resilience.py:228), same message on both. The only difference is the status code in the failure text — 500 on master, 503 on this branch — because PR3 turns the clone-vanish case into a retryable 503, which is an improvement, not a regression. So the conclusion stands but is now evidence-backed rather than assumed: the gate is intermittently red on master and PR3 neither introduced nor worsened it. The test file itself is untouched by this branch, as is every BROADCAST_* setting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(git-leak): the postgres-bounce (d) failure is a bed-shape artifact, not a broadcaster gap Correcting my own row from the previous commit, which called (d) "a genuine gap in the already-merged broadcaster work" needing "a follow-up on the broadcaster". That overstates it, and the PR description already says so in its "Known limitation" section — which Zeev's round-2 comment even pointed at. A worker receives backbone messages only if its broadcaster reader is running, and that starts via STATISTICS_ENABLED (default off) or a connected websocket client (PubSubEndpoint.main_loop enters the broadcaster context). subscribe() alone does not start it. This bed runs with no opal-client service and statistics off, so the non-leader worker is deaf to the backbone and a publish it buffers during the outage never replays. The leader DOES have a reader, via the watcher's listening context — which is exactly why assertions (a)-(c) pass and only (d) fails. In a deployment with clients connected (PDPs hold long-lived websockets across the workers) or statistics on, the reader runs and the replay works. The measured baseline from the previous commit stands and is kept: origin/master 1 pass / 4 runs vs the PR3 head 2 passes / 6 runs, same assertion, same line. The only PR3-attributable difference remains the status code in the failure text (500 -> a retryable 503). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(opal-server): replace the share-based sweep ceiling with corroboration + a per-pass cap The round-4 ceiling was wrong-shaped, and four of this round's findings are it. Rather than patch the share arithmetic again, this replaces it — and DELETES the key it introduced, so net config surface goes down. - **The denominator counted dirs that could never be candidates.** `len(dir_names)` was the raw listing while the loop dropped every name `_confined_clone_path` rejects, so each junk entry raised the ceiling for free: measured, 6 stray dirs (a `clone.bak`, a `lost+found` on a PVC, a symlink — scandir follows them) let a wrong-keyspace read delete all 5 production clones. The same raw count also made a legitimately empty store report "1 clone dirs exist" and refuse forever. Both guards now judge validated names only. - **A share can only saturate on TOTAL store loss.** A store that is correct but incomplete — a replica seconds behind a failover, an LRU eviction of `permit.io/Scope:*` (SET with no TTL, so evictable), a partial restore — produces a sub-threshold orphan set that sails through, and the under-lock re-check reads the same degraded store so it confirms the wrong answer. Measured at the old default: 40 of 100 tenants missing from the read cost 40 live clones. Replaced by corroboration: a dir must look orphaned for _REQUIRED_ORPHAN_STREAK consecutive passes, so a transient gap costs one pass of delay instead of a tenant outage. - **O(orphans x scopes) is gone for good.** Per-candidate freshness is kept — it is what stops a stale set deleting a live tenant's clone — but only dirs that are corroborated AND within SCOPES_ORPHAN_SWEEP_MAX_RECLAIM_PER_PASS pay for a read, so a pass is O(cap) reads, not O(orphans). The two properties that have been traded against each other for three rounds now both hold. - **The disable values no longer hide.** 0/1 silently disabled the old fraction while typos warned — backwards for a safety control. Disabling the cap now logs once, and "MAX_RECLAIM_FRACTION=0 means no ceiling" is gone with the key. Also in the sweep, from the same round: - Two keep-on-error paths returned "claimed" forever and the pass then reported `complete` — a monitor watching the heartbeat saw success while the backstop was off. They now return "undecided", carry a metric (`store_read_timeout`, `recheck_failed`, `unresolvable_scope`), and make the outcome `degraded (N)`. A record whose source_id will not derive is handled per scope, so it no longer aborts the whole pass. - `_STORE_READ_TIMEOUT` is a config key (`SCOPES_ORPHAN_SWEEP_STORE_READ_TIMEOUT`): 10s is not a universal constant for a read that is a SCAN plus a GET per key. - The leader delete path held `lock_source` across the same unbounded `find_scope_sharing_source` read the self-audit bounded in the sweep. Same wrapper, same keep-the-clone fail-safe. - Reclaim + confirm is now one task registered in the set the watcher's bounded drain awaits, and shielded. run_sync dispatches rmtree to the default executor, so a cancellation deleted the dir and skipped the confirmation — leaving every other worker holding a handle for a directory that no longer exists. - The scan-failure metric no longer ships a raw exception repr to Datadog (a pydantic error over a tenant record, on a channel the log redactor does not cover); the type goes in a tag, counts move to the message. Addresses review comments: - https://github.com/permitio/opal/pull/924#discussion_r3700005706 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3700005746 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3700005761 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3700005906 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3700005957 (@zeevmoney) - https://github.com/permitio/opal/pull/924#discussion_r3700006010 (@zeevmoney) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(opal-server): de-vacuum the freshness pin, verify per-test, close the drift guard's last holes The freshness regression test could not fail. It patched `source_id` AFTER seeding the filler clone dirs, so their on-disk names never matched the patched live set; all 8 dirs became candidates, the pass refused before entering the loop, and the victim survived for a reason unrelated to freshness. Confirmed by running it against its own named mutation — the suite went red, but on two OTHER tests while this one passed. That is a flaw in how I verified, not just in the test: my mutation matrix checked "does the suite go red", so another test failing counted as caught. It now names the test that must fail. Re-verified per test: the hoisted-read mutation fails THIS test in isolation, and it asserts the deletion loop was actually entered so it cannot silently go vacuous again. It is also order-independent — which dir goes live is chosen from whichever the pass deletes first, since os.scandir order is not guaranteed. New behaviour pinned (each mutation-verified against the specific test): corroboration keeps a live cl…

asafc added 9 commits February 27, 2021 20:58

fixed a missing await

6b689b7

remove duplicate definition of AsyncioEventLoopThread

a896b7b

fix client cached policies (moved from single to bundle)

fae7450

extract opa utils

ceed680

Refactor repo watcher to use callbacks and smaller classes

432d735

commit viewer

c937dbf

bundle maker refactor

a5ec96a

diff viewer + diff bundle

9db41a0