fix(opal-server): git resilience — never stuck on an offline repo (PR3)

[dshoen619]

PR3 — Git resilience + the deferred items from the leak series

Closes PER-15157.

Ships PR3's original charter (no git operation can hang the server) plus the six items deferred out of PR2 (#923), and — because the resilience fix required it — the parallel scope loading originally scoped as PR4 (see §6; PR4 is closed as redundant). Design: docs/superpowers/specs/2026-07-19-pr3-deferred-items-design.md.

1. Git resilience — hung remotes can't starve healthy scopes

Scope clone/fetch used pygit2 with no timeout, on the shared default executor. POLICY_REPO_CLONE_TIMEOUT is wired only to the legacy non-scopes path, so scopes mode had no bound at all: boot (preload_scopes()) blocked indefinitely on one unreachable repo, and in steady state a hung op held the per-source lock and a shared-pool thread.

• Hard per-operation timeout (SCOPES_GIT_FETCH_TIMEOUT) on clone and fetch.
• Zombie-aware executor. Each op runs on its own single-use daemon-thread executor; SCOPES_GIT_MAX_WORKERS bounds only live ops via a semaphore. A timed-out op releases its capacity slot immediately — its pygit2 call lingers on a private thread until the OS gives up, but no longer consumes capacity. (A fixed pool starves once lingering threads exceed its size: 40 blackholed repos vs. 10 slots meant healthy scopes never got a thread. That's the test_offline_repo_does_not_block_healthy_scopes gate.)
• git_op_in_flight(source_id) guards every path that would free a pygit2 handle or delete a clone dir while a thread may still be using it.
• Bounded-concurrency scope sweep (per-scope failures isolated) — this is also the parallel-boot fix; see §6.

2. Fleet-wide cache purge — two-phase (deferred items 1 + 3)

PR2's purge was process-local: only the worker serving the DELETE dropped its caches, so the leader — which populates most of repos/repo_locks/repos_last_fetched — leaked until restart. A non-leader DELETE also rmtree'd the shared clone tree, breaking the "only the leader mutates the clone tree" invariant.

New every-worker channel SCOPES_PURGE_CHANNEL (__opal_scope_purge__, following the __opal_stats_* convention), carrying a two-phase purge:

1. Routes publish a purge request (confirmed=False).
2. Only the leader acts on requests: sibling-check under lock_source, then disk work — in a background task, never on the publish path (publish() awaits subscriber callbacks inline, so an inline handler would put a lock wait on DELETE/PUT latency).
3. The leader broadcasts the confirmation (confirmed=True), and that is what every worker's memory handler acts on.

Phase 2 exists because purging on the raw request over-purges: a source shared by a sibling scope had its cache dropped while the sibling still used it. Only the leader can sibling-check, so only the leader may authorize a memory purge.

• Repoint (item 3): PUT /scopes with a changed source_id publishes a purge for the old source. Same channel, same handlers.
• Deferred-purge split: when a git op is still in flight, the clone dir and pygit2 handle wait for the sweep, but the lock/timestamp entries — which the thread never touches — drain immediately and the purge still confirms.
• Mixed-fleet safe by construction: old workers aren't subscribed; confirmed is additive with a False default.

3. Scope-liveness check before clone (item 2)

A sync that loaded a scope before its DELETE and reached it mid-sync re-cloned the dead scope and re-populated the caches. GitPolicyFetcher now takes a liveness probe, checked under lock_source immediately before the clone; ScopesService supplies one that re-reads from Redis and also confirms the scope still points at this source (so a repointed scope's stale sync doesn't resurrect the old clone either). Fails open on store errors.

4. Orphan clone-dir sweep (item 6)

Leader-only reconciliation after boot, periodic, and refresh-triggered syncs: clone dirs under git_sources/ referencing no live scope are reclaimed. One set-difference covers crash orphans, redis-wiped boots, and old-shard dirs after a SCOPES_REPO_CLONES_SHARDS change. A store-scan error aborts the sweep (a transient Redis error must never read as "no scopes exist"); each orphan is re-checked under its own lock and skipped while a git op is in flight.

5. ⚠️ Behavior change — retryable 503 for a live scope's broken clone (item 5)

GET /scopes/{scope_id}/policy:

Case	Before	After
Scope record missing	default scope's bundle	unchanged
Record present, clone invalid/vanished	default scope's bundle	503 + Retry-After: 5
Record present, make_bundle raises raw OSError	unhandled 500	503 + Retry-After: 5

A live tenant was briefly served another tenant's policy. The condition is transient by construction (recovery or the next sync re-creates the clone), so a retryable error is the honest answer. opal-client tolerates this: its PolicyFetcher retries with tenacity backoff and a failed cycle is skipped, not fatal — it does not read Retry-After (uses its own backoff). Direct consumers of this endpoint should expect 503 and retry.

6. Parallel scope loading — the ~20-minute boot fix (folds in PR4)

sync_scopes (boot preload and every periodic/refresh sweep) was serial: one await sync_scope(...) per scope. With a fleet of repos that meant a ~20-minute boot, and — once §1 added a per-fetch timeout — a serial loop would still stall healthy scopes behind each unreachable repo's timeout. So the resilience fix in §1 requires concurrency to actually deliver "one offline repo can't block the others"; the two ship together and PR4 (which planned this same work as a standalone change) is closed as redundant.

The sweep runs in two phases, each asyncio.gather under its own semaphore:

• Phase 1 — distinct repos (network clone/fetch). Bounded by SCOPES_GIT_MAX_WORKERS. Combined with the zombie-aware executor (§1), a hung remote consumes only its own slot until its timeout, then releases it.
• Phase 2 — scopes reusing an already-handled repo (local change-check only). In the common case _should_fetch returns False, so there's no network fetch — just a disk open + change-check + notify. It therefore gets a separate, wider bound (max(SCOPES_GIT_MAX_WORKERS, 32)) instead of inheriting phase 1's network cap; 32 matches asyncio's default thread-pool ceiling, which is what actually limits those disk opens. Any rare re-fetch phase 2 does trigger (a branch missing after phase 1) is still throttled by the inner live-ops semaphore, so the network is never over-driven.

Per-scope failures stay isolated (ScopeNotFoundError from a mid-sweep delete is skipped; other exceptions are logged), and sync_scope re-reads each scope by id right before use, so a delete that lands after the snapshot never re-clones a dead scope.

No new config key — the split is internal. SCOPES_GIT_MAX_WORKERS remains the single operator lever for git concurrency; a dedicated sync-concurrency knob was considered (PR4's SCOPES_SYNC_CONCURRENCY) and deliberately dropped to avoid a redundant, easily-misconfigured second bound.

⚠️ Caveat — the timeout is soft, not a hard kill

The timeout unblocks the event loop and the awaiting coroutine, but the underlying pygit2 call keeps running on its private daemon thread until the OS network timeout. Daemon threads never block shutdown, and git_op_in_flight keeps anything from freeing that repo meanwhile — but worst-case thread count during an outage is SCOPES_GIT_MAX_WORKERS plus the number of lingering timed-out ops. Hard-kill via subprocess is explicitly out of scope.

New config keys (opal-server, server-only, additive)

Env var	Type	Default	Purpose
OPAL_SCOPES_GIT_FETCH_TIMEOUT	float (s)	120.0	Hard timeout for a single scope git clone/fetch. 0 = no limit.
OPAL_SCOPES_GIT_MAX_WORKERS	int	10	Bounds live git ops. Timed-out ops release their slot, so hung remotes never starve healthy scopes.
OPAL_SCOPES_PURGE_CHANNEL	str	__opal_scope_purge__	Worker-to-worker channel for the fleet-wide cache purge.

Invariants (enforced and tested)

1. repo_locks entries are popped only while holding that source's lock.
2. forget_repo / rmtree never run while a git op is in flight for that source.
3. Only the leader mutates the clone tree.

The purge confirmation is published while holding lock_source — it frees this process's cached handle via the inline local subscriber, so releasing first opened a use-after-free against a re-created scope's _notify_on_changes (which holds the handle across an await, then calls set_target() on it).

Verification

• opal-server unit suite: 151 passed. The §6 per-pass split adds sync_scopes_perpass_test.py (2 tests: phase 2 runs wider than the git cap; phase 1 still respects it) — both green, and the related sync/delete/preload suites pass unchanged.
• app-tests/git-leak docker bed: all 10 acceptance gates green — 19/19 in the main phase plus test_offline_repo_does_not_block_healthy_scopes. The bed (not the unit suite) is the real gate: it caught three product bugs unit tests could not — the purge blocking DELETE/PUT on the publish path, the shared-source over-purge, and the thread-pool starvation.
• Bed changes: OPAL_SCOPES_GIT_FETCH_TIMEOUT=10 for realistic serve windows; the delete-vs-inflight-sync churn exclusion lifted (closed by the liveness probe); the repoint gate rewritten as a green guard (it was racing a purge that now completes in ~4ms); allow_worker_restart on the force-recreating boot test; chown after the restoring compose cp.

Known limitation (pre-existing, not introduced here)

test_server_recovers_after_postgres_bounce can fail in a full-file bed run. A worker receives backbone messages only if its broadcaster reader is running, which happens via STATISTICS_ENABLED (default False) or a connected websocket client — subscribe() alone does not start it. The bed has no opal-client service and statistics off, so a non-leader worker there is deaf to the backbone and a publish it buffers during an outage never replays. With clients connected (or statistics on) the reader runs and the replay works — verified in the logs of a passing run. Unrelated to this PR's changes; tracked separately.

Consumer surface

packages/opal-client and packages/opal-common are untouched. No OPAL_* key renamed or removed; all three new keys are additive with behavior-preserving defaults. The 503 above is the one intentional contract change.

🤖 Generated with Claude Code

[linear-code]

PER-15157

[netlify]

✅ Deploy Preview for opal-docs canceled.

Name	Link
🔨 Latest commit	e614a84
🔍 Latest deploy log	https://app.netlify.com/projects/opal-docs/deploys/6a71deb3290c400008e9124b

[Copilot]

Pull request overview

This PR improves opal-server resilience when syncing scope policy repos by ensuring git clone/fetch operations can’t block indefinitely or starve the server’s shared executor.

Changes:

• Add a dedicated, bounded ThreadPoolExecutor and run_in_git_executor(...) helper to run blocking pygit2 operations with an asyncio.wait_for timeout.
• Apply the helper + new timeout config to scope repo clone and fetch paths.
• Add server config keys for timeout and executor sizing, plus focused unit tests for timeout behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/opal-server/opal_server/git_fetcher.py	Introduces dedicated git executor + timeout helper; routes scope clone/fetch through it.
packages/opal-server/opal_server/config.py	Adds SCOPES_GIT_FETCH_TIMEOUT and SCOPES_GIT_MAX_WORKERS configuration.
packages/opal-server/opal_server/tests/git_executor_test.py	Tests config defaults and run_in_git_executor basic behavior.
packages/opal-server/opal_server/tests/fetch_timeout_test.py	Tests that a hanging git op times out quickly (doesn’t block).
.claude/plans/docs/05-config-reference.md	Internal config reference entry for the new env vars and caveat.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zeevmoney]

Review notes — overlap + gate location (planned with the opal-development skill: references/add-config-key.md, references/debug-pubsub.md)

Faithful to the PR3 plan, with two improvements over it: normalizing asyncio.TimeoutError → builtin TimeoutError (so callers catch the builtin on 3.9/3.10 where they're distinct), and moving repos_last_fetched to after a successful fetch — a timed-out fetch no longer falsely marks the source "fresh," and it's lock-safe because _should_fetch runs inside the per-source repo_lock. The two config keys follow references/add-config-key.md (server-only OpalServerConfig, bare names, mandatory descriptions, no double-prefix).

Three things to resolve:

1. Direct overlap with Fix git clone/fetch hanging indefinitely on unreachable repos #875 (PER-13817). Same root cause — pygit2 clone_repository / remotes.fetch going through run_sync with no timeout — and the same edited blocks in git_fetcher.py (_clone and fetch_and_notify_on_changes). They cannot both merge; whichever lands second will conflict. This PR is the stronger of the two:

   • Default-on protection: OPAL_SCOPES_GIT_FETCH_TIMEOUT=120 vs Fix git clone/fetch hanging indefinitely on unreachable repos #875 reusing POLICY_REPO_CLONE_TIMEOUT (default 0 = no timeout, so Fix git clone/fetch hanging indefinitely on unreachable repos #875 protects nothing unless an operator opts in).
   • Thread isolation: a dedicated bounded ThreadPoolExecutor for scope git work, so a hang can't starve bundle-serving — Fix git clone/fetch hanging indefinitely on unreachable repos #875 stays on the shared default executor. (This is the starvation concern from the offline-repo test in test(opal-server): git leak/resilience test environment (PR1) #922.)
   • Best-effort boot: _clone catches (GitError, TimeoutError), logs, and skips the scope rather than letting the caller crash.

   Recommend closing Fix git clone/fetch hanging indefinitely on unreachable repos #875 in favor of this PR.

2. Its regression gate lives in PR1 (test(opal-server): git leak/resilience test environment (PR1) #922). test_offline_repo_does_not_block_healthy_scopes is the fail-now/pass-after gate for this fix and only exists on test(opal-server): git leak/resilience test environment (PR1) #922. So this can't be validated end-to-end until test(opal-server): git leak/resilience test environment (PR1) #922 merges (or is merged into this branch). Suggested order: test(opal-server): git leak/resilience test environment (PR1) #922 → this.

3. This PR is currently check-blocked by branch protection (required checks / review), not by a merge conflict — needs CI green + an approval.

Minor:

• The dedicated pool reads SCOPES_GIT_MAX_WORKERS once, lazily, and caches the executor for the process lifetime — it isn't runtime-reconfigurable and is never shut down. Matches the plan's design; worth a one-line note in the docstring.
• The line numbers cited in .claude/plans/docs/05-config-reference.md for the two keys are stale vs where they actually land on this branch (cosmetic).

[dshoen619]

Thanks @zeevmoney — addressed in bad21c1.

Minor items

• Executor lifetime docstring — added a note to _get_git_executor: SCOPES_GIT_MAX_WORKERS is read once on first use, the executor is cached for the process lifetime (not runtime-reconfigurable) and is never explicitly shut down, matching the PR3 design.
• Stale line numbers — good catch. They'd drifted after the master merge shifted config.py. Fixed the 05-config-reference.md refs: SCOPES_GIT_FETCH_TIMEOUT 150-156 → 196-202, SCOPES_GIT_MAX_WORKERS 157-163 → 203-209.

Things to resolve

1. Overlap with Fix git clone/fetch hanging indefinitely on unreachable repos #875 (PER-13817) — agreed this is the stronger fix (default-on OPAL_SCOPES_GIT_FETCH_TIMEOUT=120, dedicated bounded pool, best-effort boot). Plan is to close Fix git clone/fetch hanging indefinitely on unreachable repos #875 in favor of this.
2. Regression gate in test(opal-server): git leak/resilience test environment (PR1) #922 — agreed; the fail-now/pass-after gate (test_offline_repo_does_not_block_healthy_scopes) lives on PR1, so the plan is to land test(opal-server): git leak/resilience test environment (PR1) #922 → this and validate end-to-end there.
3. Check-blocked — required checks (E2E, builds 3.9–3.12, pre-commit) were green on the prior head and are re-running on bad21c1e; the remaining blocker is the required approving review. A review once it's green would unblock it.

Also confirmed the two improvements you flagged are in place: asyncio.TimeoutError → builtin normalization, and repos_last_fetched moved to after a successful fetch.

[zeevmoney]

Review (PER-15157 / PR3) — git resilience: never stuck on an offline repo

What this PR does. Makes scope git clone/fetch resilient to unreachable repos. It moves scope git work off the shared default executor onto a dedicated daemon-thread ThreadPoolExecutor (SCOPES_GIT_MAX_WORKERS, default 10), wraps each clone/fetch in a soft per-op timeout (SCOPES_GIT_FETCH_TIMEOUT, default 120s) via run_in_git_executor using asyncio.wait (so a timeout unblocks the event loop without cancelling the still-running pygit2 call), adds a per-repo in-flight guard so a lingering timed-out op is not touched concurrently (pygit2 Repository is not thread-safe), records repos_last_fetched only on fetch success, makes scope sync concurrent (bounded by the pool via a semaphore), and adds fork-safety (os.register_at_fork reset + shutdown_git_executor() after pre-fork preload). Plus two unit-test files and a private config-reference doc.

Verdict: REQUEST_CHANGES. Per the severity rule, there is one Postable HIGH finding (credential-redaction bypass in the new log lines) → REQUEST_CHANGES. Independently, the PR is not mergeable: GitHub reports CONFLICTING and git_fetcher.py has a content conflict with master (see Blockers).

The core design is sound and a real improvement over master: isolating git work onto a dedicated pool means a hung clone/fetch can no longer starve bundle serving or the event loop (on master these share the default executor via run_sync, so one offline repo hangs the whole server). The soft-timeout + single-flight + fork-safety mechanics are correct and well-tested (test_busy_key_stays_in_flight_until_call_returns, test_hanging_git_op_raises_timeout, the config-default tests). The two prior open Copilot threads are already addressed by the current code (asyncio.get_running_loop() replaces get_event_loop; repos_last_fetched is now written only after a successful fetch) — not re-raised.

Findings

Postable:

#	Severity	File:Line	Category	Description
1	HIGH	packages/opal-server/opal_server/git_fetcher.py:412 (also 376, 465)	Security	New skip/timeout/clone-error log lines log the raw self._source.url; master redacts every repo URL in this file via redact_url(). Once merged these are the only un-redacted URL logs → credential exposure. redact_url isn't imported.
2	MEDIUM	.claude/plans/docs/05-config-reference.md:27	Doc accuracy	The ceil(offline / workers) × timeout boot/poll bound is optimistic: timed-out ops keep their pool thread until the OS network timeout, so with offline >= workers healthy repos queue behind lingering threads longer than the stated bound.

Informational (not posted inline):

#	Severity	File:Line	Category	Description
3	LOW	packages/opal-server/opal_server/git_fetcher.py:52-91	Maintainability	_DaemonThreadPoolExecutor._adjust_thread_count reimplements CPython concurrent.futures.thread internals. It falls back to super() if _worker/_threads_queues disappear, but a signature change to _worker (name kept, args changed) would pass the hasattr check yet break. Acceptable given the fallback + # pragma: no cover, but a fragility to track across Python upgrades (repo targets 3.9–3.12).
4	LOW	.claude/plans/docs/05-config-reference.md (new tracked file)	Cross-PR coordination	PR #922 adds .claude/ to .gitignore while this PR tracks a file under .claude/plans/docs/. Not a conflict on master today (.claude/ isn't ignored), but once both land the tracked doc sits under a gitignored path — coordinate.

Design note (not a blocker). The "never stuck" guarantee delivered is: the event loop / HTTP surface / bundle serving never block, and each sync slot stalls at most SCOPES_GIT_FETCH_TIMEOUT. It is not that a fixed pool immediately reclaims capacity on timeout — a timed-out op lingers on its thread until the OS network timeout (by design; pygit2 can't be cancelled). For the realistic case (a few offline repos among many healthy) this is fine — the offline ops each hold one lingering thread and the rest of the pool serves healthy repos. The pathological case (offline repos >= pool size) can saturate the git pool; the daemon threads still let the process exit promptly and other server work is unaffected. Finding #2 asks the doc to reflect this precisely.

Blast radius: Production opal-server git-fetcher + scopes sync path — affects every scoped deployment's boot and poll behavior. No client-facing symbol from references/pdp-impact.md §3 is renamed/removed (new module-level helpers + two config keys only; GitPolicyFetcher public shape unchanged), so no PDP import-surface break. Two new OPAL_* keys are additive with sane defaults, no env-name collision, no OPAL_ double-prefix. Pub/sub topology unchanged — this only changes how the leader fetches git; the scope publish path is untouched, so PDP policy-update propagation is unaffected except that offline repos now fail fast (skip + retry) instead of hanging.

Isolation / scope: Well-isolated. All changes serve the stated purpose (git resilience); no unrelated refactors, no half-done work.

Blockers:

• CONFLICTING / not mergeable. git_fetcher.py has a content conflict with master (master added redact_url() to the log lines this PR also edits). Rebase/merge master and resolve — and when doing so, apply redact_url() to the new log lines too (finding #1). Reviewed here against the merge-base three-dot diff (d30e462...d31a0b63), which is unaffected by the conflict.

[zeevmoney]

Second pass — changes still requested. 2 further HIGH, 5 further MEDIUM.

Blocking:

• HIGH packages/opal-server/opal_server/scopes/purge.py:96 — the every-worker handler calls forget_repo → Repository.free() with no lock, so on every process except the publisher it is the exact use-after-free that purge.py:257-264 locks the confirmation publish to prevent; git_op_in_flight does not cover _notify_on_changes or the run_sync(_get_valid_repo) executor thread
• HIGH packages/opal-server/opal_server/git_fetcher.py:276 — "until the OS network timeout" is enforced by nothing (no GIT_OPT_SET_SERVER_TIMEOUT, no socket read timeout, libgit2 1.7 defaults to none), so a black-holed remote pins its _git_busy marker for process life; at SCOPES_GIT_MAX_ZOMBIES such entries every git op is refused fleet-wide, logged only as warning-level backpressure

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:340 — the sweep rmtrees any subdirectory of git_sources/ without the _SOURCE_ID_RE check every other deletion path uses
• MEDIUM packages/opal-server/opal_server/scopes/task.py:57 — the orphan-sweep timer is skipped when polling is on, contradicting config.py:513-516, task.py:95 and the .mdx
• MEDIUM packages/opal-server/opal_server/scopes/service.py:292 — max(..., 32) is a floor, so SCOPES_GIT_MAX_WORKERS cannot lower phase-2 concurrency; config.py:230-231 claims it can, and phase 2 shares the executor that serves policy bundles
• MEDIUM packages/opal-server/opal_server/scopes/task.py:64 — the boot sync is outside the try that protects the sweep; a store error kills the task with no log and, at the default POLICY_REFRESH_INTERVAL=0, no retry
• MEDIUM packages/opal-server/opal_server/git_fetcher.py:502 — neither the in-flight sync guard nor "a timed-out fetch must not stamp repos_last_fetched" has a test; both mutations leave the suite green

Details are in the inline comments on each line.

---

Correction to the previous pass. The suggested fix on scopes/task.py:43 was wrong and that comment has been updated in place. PubSubEndpoint.subscribe registers all server-side subscriptions under one shared _subscriber_id, and EventNotifier.unsubscribe deletes by subscriber id per topic, so the unsubscribe([SCOPES_PURGE_CHANNEL]) I proposed would also have removed the every-worker handle_purge_message registered at server.py:395. The corrected comment proposes a dedicated subscriber id (or an _is_leader flag) instead. The finding itself stands; only the remedy changed. Also narrowed there: there is no in-process restart path, so the handler cannot double-subscribe — the real exposure is the window between the flock being released and the old worker dying.

Pre-existing, not introduced here, but worth a decision. api.py:308-315 still returns _generate_default_scope_bundle (HTTP 200, carrying the default scope's policy modules) when a scope record is missing, while the new comment at api.py:352-356 states the principle that "serving the default scope's bundle here would hand a live tenant another tenant's policy". _allowed_scoped_authenticator (api.py:89-98) authorizes the caller for the requested scope_id only, never for default. If tenant B's record is briefly absent — a Redis failover, a partial restore — B's PDP receives 200 plus the default scope's policy and loads it into OPA. The PR applies its own stated rule to the broken-clone case and not to the missing-record case, and scope_policy_fallback_test.py:117-132 pins the latter. Out of scope for this PR, but the two halves should not stay inconsistent indefinitely.

Also flagged, not filed inline (lower value, or better handled as a batch): metrics.event on the 503/409 paths is a DogStatsD event rather than a counter, emitted per failed request with a caller-controlled scope_id tag — during a clone outage retrying PDPs bury the signal; the confirmation publish at purge.py:266 and :349 does an unbounded backbone round trip inside lock_source, so a hung broadcaster pins that source's lock (there is mitigating precedent at git_fetcher.py:778); sweep_orphans and purge_source_if_unshared measure at cyclomatic complexity 17 and 11, and fetch_and_notify_on_changes grew from 82 to 133 lines with early returns carrying load-bearing invariants six levels deep; asyncio.gather in sync_scopes materializes one task per scope with no bound on scope count, including in the gunicorn master during preload; handle_purge_message reads the module-global BASE_DIR while LeaderScopePurger uses its injected base_dir; shutdown_git_executor() shuts down no executor and _release_once's guard is currently unreachable; git_executor_test.py mutates the process-global _git_busy without an autouse fixture and asserts on wall-clock elapsed time, which is flaky by construction under -p xdist or a randomizer.

Verified clean on this pass: the two-phase design holds — publish() runs local subscribers inline, and the publisher's and subscriber's ids are distinct so the leader really does receive its own confirmation; the replay case is sound with no epoch token needed (a scope re-created before the sibling check is seen under lock_source, so no confirmation is published at all — I ran this: the recreated scope's clone dir and cached handle both survived, zero confirmations published, while the truly-deleted control purged and confirmed); disk purges lost to a leaderless window are genuinely backstopped by the sweep; _confined_clone_path rejects traversal and the wire-supplied clone_path never reaches rmtree or free(); the publish-direction channel guard works for single, mixed and list topic forms; mixed-fleet is safe in both directions; route authz is unchanged and correct; Python 3.9 and pydantic v1 compatibility hold across all new constructs; the live-ops semaphore is released exactly once on every exit path and no executor leaks.

[Zivxx]

Round-2 review addressed — all 13 threads fixed in fb1b823d

Thanks @zeevmoney — both submissions from this pass (2 HIGH + 4 MEDIUM, then 2 further HIGH + 5 further MEDIUM) are resolved. CI is green on fb1b823d (pre-commit, build 3.9–3.12, E2E + Alpine), and the fixes were additionally verified across the local git-leak bed (invariant I4 clean).

HIGH

• pubsub ALL_TOPICS reopens purge channel — _reject_external_purge_channel now rejects ALL_TOPICS and SCOPES_PURGE_CHANNEL on the subscribe path.
• sweep O(dirs×scopes) blocks the leader loop — precomputed live_source_ids set (O(scopes)) + periodic await asyncio.sleep(0).
• every-worker UAF freeing the pygit2 handle unlocked — handle_purge_message now frees under lock_source on every process, and pops its minted repo_locks entry (I4).
• "until the OS network timeout" enforced by nothing — the claim was the bug: corrected to soft timeout across git_fetcher.py / config.py / configuration.mdx, with the real hung-remote bound (SCOPES_GIT_MAX_ZOMBIES + daemon threads) documented.

MEDIUM

• confirmed-gate test now uses a valid source_id (fails with the gate deleted)
• opal-client capped <3.13 to match its hard dep
• stop() unsubscribes + drains the purger (no abandoned rmtree)
• recreate-after-delete now has a unit test
• sweep rmtree validates via _confined_clone_path
• orphan sweep runs unconditionally on its own interval (docs now match)
• phase-2 concurrency is max(1, SCOPES_GIT_MAX_WORKERS) — a true ceiling
• boot sync_scopes() wrapped so a raise doesn't silently kill the task
• both _get_current_branch_head safety behaviours now have tests

Ready for re-review against fb1b823d.

[zeevmoney]

Changes requested — 5 HIGH, 5 MEDIUM, 2 LOW on the fix commit.

Ten of the thirteen round-2 findings are genuinely fixed and I verified each by mutation rather than by reading the reply. The ALL_TOPICS purge-channel guard now holds against every spelling I could construct (bare sentinel, in a list, mixed list, tuple, set; nested and None shapes fail closed) with no loss of legitimate channel=None fan-out. The phase-2 concurrency knob is a true ceiling and its replacement test discriminates. The confirmed-gate test now fails when the gate is deleted. The in-flight sync guard has real coverage. _periodic_polling genuinely no longer sweeps.

What follows is what the fix commit broke or left.

Blocking:

• HIGH packages/opal-server/opal_server/scopes/task.py:65 — unsubscribe([SCOPES_PURGE_CHANNEL]) also removes the every-worker handle_purge_message; all server-side subscriptions share one _subscriber_id, so the worker goes deaf to fleet purges (verified: 2 handlers → 0)
• HIGH packages/opal-server/opal_server/scopes/task.py:70 — the purge drain is awaited before super().stop() cancels the tasks holding the locks it waits on; shutdown can block for SCOPES_GIT_FETCH_TIMEOUT or indefinitely, inside the leadership lock (PoC hangs)
• HIGH packages/opal-server/opal_server/scopes/purge.py:337 — a successful empty scope scan yields an empty live set, so every clone dir is reclaimed and a confirmed purge is broadcast for each; a wrong Redis DB index or an empty replica destroys the local clone tree fleet-wide
• HIGH packages/opal-server/opal_server/git_fetcher.py:560 — the except TimeoutError early return is still untested; the reply addressed _get_current_branch_head instead, and the mutation ships green
• HIGH packages/opal-server/opal_server/scopes/purge.py:286 — the leader avoids self-deadlock only because this pop precedes the confirmation publish; moving it after deadlocks the leader permanently and the full suite still reports 203 passed

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:104 — the use-after-free lock, the sweep's path validation, and LeaderScopePurger.stop() each ship green when mutated away
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:381 — the orphan-heavy path is still O(orphans × scopes), one full store scan per candidate (2.9 s at N=2000)
• MEDIUM packages/opal-server/opal_server/scopes/task.py:80 — both task.py fixes ship green when reverted
• MEDIUM packages/opal-server/opal_server/scopes/service.py:283 — the comment still argues for the behaviour the same commit reversed
• MEDIUM packages/opal-server/opal_server/git_fetcher.py:291 — a negative SCOPES_GIT_MAX_ZOMBIES refuses every git op fleet-wide
• LOW packages/opal-server/opal_server/scopes/task.py:116 — docstring contradicts the change made 60 lines above
• LOW documentation/docs/getting-started/configuration.mdx:486 — re-breaks adade574's verbatim match for SCOPES_GIT_FETCH_TIMEOUT

Details are in the inline comments on each line.

---

Pre-existing, not inline-postable — but this PR makes the case for fixing it. api.py:309-315 still answers a missing scope record with HTTP 200 carrying the default scope's policy modules. Those lines are byte-identical to master (confirmed against the diff), so they are out of this PR's scope. But this PR removed the identical fallback from the clone-unavailable branch and wrote the reason at api.py:351-356 — "Serving the default scope's bundle here would hand a live tenant another tenant's policy" — so the PR itself establishes the record-missing branch as wrong and leaves it. _allowed_scoped_authenticator (api.py:89-98) authorizes the caller for the id they asked for, never for default, which api.py:390 then loads; and with OPAL_AUTH_PUBLIC_KEY unset the authenticator short-circuits entirely. A tenant whose record is briefly absent — a Redis failover, or the wiped-store case the finding above describes — has its PDP load and enforce another scope's Rego and see a 200 while doing it. scope_policy_fallback_test.py:117 currently pins this as "unchanged contract". Worth a follow-up issue rather than scope creep here.

Two smaller pre-existing items on the same function: api.py:396 runs make_bundle synchronously on the event loop while the live path correctly uses run_sync; and a ScopeNotFoundError escaping the fallback surfaces as an unhandled 500 rather than a 404, since there is no exception handler registered for it.

One operational note for the description: phase-2 sync concurrency drops from a hard 32 to SCOPES_GIT_MAX_WORKERS (default 10) — a 3.2× narrowing of the local change-check pass. Making the knob a real ceiling is right; the default change riding along with it is not called out anywhere.

[Zivxx]

Round-3 review addressed — 12/12 threads (5 HIGH · 5 MEDIUM · 2 LOW)

741ed34e source fixes · 08bd7262 tests · c9968e99 bed scoping + sweep re-read cadence · 20fc128e docstring follow-up. Per-finding detail is in each thread; everything was reproduced before being fixed.

Verification

opal-server unit	226 passed (204 before this round)
opal-common	93 passed
opal-client	1 failure — data_updater_test.py::test_data_updater_with_report_callback; pre-existing, reproduces identically with these files reverted to fb1b823d (2/2)
Mutation matrix	14/14 caught — each new test shown failing under exactly the mutation it pins, passing clean (list in 08bd7262)
pre-commit (py3.11 toolchain)	clean, converged
git-leak bed (docker, --boot-scopes=20, 21 tests)	20 passed, 1 failed

Bed detail: test_redis_wiped_boot_reclaims_clones ✅, test_orphan_clone_dir_is_reclaimed ✅ (reshaped — see below), all four leak gates ✅, test_multiworker_churn_drains_every_worker ✅, test_server_recovers_after_postgres_bounce ✅ in the full run.

Two behaviour changes worth stating outright

1. Phase-2 concurrency drops from a hard 32 to SCOPES_GIT_MAX_WORKERS (default 10) — a ~3.2× narrower local change-check pass for fleets with many duplicate scopes, so a longer boot sync and poll pass on the leader. Keeping the default at 10 (the knob has to be a true ceiling); OPAL_SCOPES_GIT_MAX_WORKERS=32 restores the old throughput, raising phase 1 with it.
2. New OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE (default false) — the orphan sweep now refuses to reclaim when the scope store returns zero scopes while clone dirs exist, and logs it at error level. Consequence to be aware of: a deployment that legitimately deletes every scope no longer has its leftover clone dirs reclaimed until a scope exists again, or the key is turned on. That is the deliberate trade for not letting a misdirected store delete every tenant's clone.

Bed changes, and one gate reshaped

The empty-store opt-in is scoped to the single test that wants it (test_redis_wiped_boot_reclaims_clones flips the env for its own container and restores the default) rather than set bed-wide, so every other bed test exercises the shipped refusal.

test_orphan_clone_dir_is_reclaimed was built on the exact shape the new guard refuses — zero scopes plus one stale dir — so it went red. Rather than opt it in, it now runs with a live scope present: that is the production shape, and it additionally asserts the live clone survives the sweep, which the old version never checked.

Corrections to my own commit messages

• c9968e99 claims the bed-wide reclaim opt-in turned test_server_recovers_after_postgres_bounce red. That attribution is retracted. The gate is intermittent: on this head it failed 3 attempts and passed 2 (including the full-bed run above), both with the key on and off; it passed on fb1b823d. The per-test scoping stands on its own reasoning (the bed should exercise the shipped default), but it did not fix that gate, because that gate was not broken by it. No mechanism in this diff touches assertion (d)'s path — the worker PIDs are unchanged in the failing runs, so the watcher never stopped and none of the lifecycle code ran. Flagging it as one to watch rather than claiming it green.
• The sweep's re-read cadence (_FRESH_READ_EVERY) is justified by reasoning about the staleness window a single per-pass read leaves open, not by an observed failure, as that commit message implies.

Latent bed bug found while verifying (not from this PR)

The one bed failure is test_scope_repoint_releases_old_repo_cache, and it fails at its precondition (test_leak.py:236, "scope never switched to serving the re-pointed content") — never reaching the cache-leak assertion the gate exists for. Root cause, measured rather than inferred:

seed/seed_gitea.py writes byte-identical example.rego/data.json to every repo with a fixed author and no pinned commit date, so a repo's commit sha is decided by the wall-clock second it is created in, and the seeder recreates the repos on each stack start. Sampled live just now:

policy-repo-0000: head=1fa166d5537c committed=2026-07-30T00:12:51Z
policy-repo-0001: head=97b179b6c6ad committed=2026-07-30T00:12:52Z
policy-repo-0002: head=97b179b6c6ad committed=2026-07-30T00:12:52Z
policy-repo-0003: head=97b179b6c6ad committed=2026-07-30T00:12:52Z

When repo_a and repo_b land in the same second (both were 1c26e784a510 during the full-bed run) their bundles are byte-identical, so resp.content != content_a can never become true and the test burns its 300s poll. It passes isolated on this head (31s) and on fb1b823d (17s), where the two happened to straddle a second boundary. The server behaved correctly throughout: the repoint purge fired (Purging local caches for source 6b3ee15e… (scope repoint, repoint)), the old source's entries drained, one clone dir remained and it was the new source's.

Fix would be one line in the seeder — make the seeded content per-repo distinct (e.g. embed the repo name in data.json), which also makes the gate's discriminator meaningful instead of accidental. Left untouched here because that file has unrelated in-flight edits locally; happy to push it as a separate change if you'd like it in this PR.

[zeevmoney]

Changes requested — 2 HIGH, 5 MEDIUM, 1 LOW.

All 12 round-3 findings are fixed, and this time the fixes are properly pinned: every mutation I ran against them now fails a specific named test. The two stop() HIGHs are correct, including the super().stop()-first ordering and the bounded drain — my round-3 hang PoC now returns instead of wedging, and the leader unsubscribe no longer takes the every-worker handler with it (2 handlers → 1, was 2 → 0). The except TimeoutError return, the use-after-free lock, the sweep's path validation, LeaderScopePurger.stop(), the boot-sync wrapper, the sweep-timer gate and the zombie clamp are each now mutation-detected. The new docs-drift guard genuinely catches a reworded description, and all 8 scopes keys round-trip verbatim.

Two of the fixes, however, introduced new problems, and that is now the third consecutive round where fixing a finding created a fresh one on the same surface. Both are in sweep_orphans, which has grown to 152 lines at cyclomatic complexity ~18 against the project's ≤100 / ≤8 — the next fix lands in that function too. Extracting the candidate-build pre-pass and the per-candidate reclaim tail, and sharing that tail with the leader path, would remove the duplication that let the pop-ordering invariant get pinned at one of two sites.

Blocking:

• HIGH packages/opal-server/opal_server/scopes/purge.py:506 — batching the fresh read moved it out from under lock_source, so a source re-claimed mid-pass has its freshly-cloned dir deleted and a confirmed=true purge broadcast for it. Reproduced three times independently; the same probe passes on fb1b823d, so it is a regression from this round. At the shipped POLICY_REFRESH_INTERVAL=0 there is no periodic re-sync, so the tenant 503s until a PUT, webhook or restart. The comment at :510-514 still asserts the old guarantee, and test_sweep_issues_one_fresh_read_for_the_whole_candidate_batch now pins the stale behaviour.
• HIGH packages/opal-server/opal_server/scopes/purge.py:392 — the new gate fires only on a zero-scope read, so a wrong-but-non-empty keyspace still reclaims everything: 5/5 production clone dirs deleted with the flag at its safe default. The key's description names "a REDIS_URL pointed at the wrong DB index" as covered; it is covered only when that DB is empty. The round-3 suggestion's second half — a per-pass ceiling — is what catches this, and is the part that did not ship.

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:543 — the sweep's pop-before-publish ordering and its per-candidate lock_source both ship green when removed; the leader twin of the first is pinned, the sweep is not
• MEDIUM packages/opal-server/opal_server/tests/config_docs_drift_test.py:60 — two vacuous-pass modes: 7 skipped if the docs file moves, and 7 passed with the verbatim text pasted under another key's heading
• MEDIUM packages/opal-server/opal_server/scopes/task.py:114 — this comment and two others promise a boot-sweep backstop that the default-off gate makes unreachable once the last scope is deleted (verified: dir survives three sweeps)
• MEDIUM app-tests/git-leak/test_boot_states.py:175 — the destructive opt-in is exported before the try, so one setup failure leaves it enabled for the rest of the bed run
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:404 — the refusal is log-only with no metric, and a mid-pass abort discards both the reclaim count and the heartbeat
• LOW packages/opal-server/opal_server/scopes/purge.py:538 — the OSError branch continues past the repo_locks.pop, leaking a stray lock per permanently-unreclaimable dir

Details are in the inline comments on each line.

---

Also noted, not filed inline. _FRESH_READ_EVERY = 200 serves two opposing purposes (freshness bound vs yield cadence) and is not a config key — folded into the first HIGH, since fixing that one should split them. git_fetcher.py:191 still reads SCOPES_GIT_MAX_ZOMBIES raw while :294 reads it clamped; inert today only because the clamp makes the negative case unreachable, so it is dead-but-wrong code and the clamp comment ("clamped like every sibling knob") overstates it — worth hoisting into one helper. _scope_sharing_source's docstring still claims the orphan sweep reuses it, which stopped being true this round, and its excluded_scope_id parameter is now never passed non-None outside tests. _may_reclaim receives raw dir_names before source-id validation, so a single unrelated entry under git_sources/ (a backup/, a lost+found) makes a legitimately-empty store log a false ERROR every 300 s with a wrong count — the same one-line change as the second HIGH fixes both. test_recreate_after_delete_serializes_and_sees_clean_caches is still tautological (built with pubsub_endpoint=None, so publish never runs; it passes with lock_source removed from purge_source_if_unshared entirely) — the behaviour it names is covered by three other tests, so this is a misleading artefact rather than a coverage hole, but it will mislead the next reader. One latent trap: _may_reclaim gates on the raw record count while the delete decision uses the git-filtered set, so if Scope.policy ever gains a second union member, a deployment using only the new type reads non-empty and reclaims its whole tree — gating on the derived set closes it now, cheaply.

Still open, pre-existing, unchanged this round. api.py:309-315 continues to answer a missing scope record with HTTP 200 carrying the default scope's policy modules, while _allowed_scoped_authenticator only ever authorizes the caller for the id they requested. api.py is not in this round's diff, so it stays out of scope here — but the PR removed the identical fallback from the clone-unavailable branch and documented why at api.py:351-356, so the argument for fixing the twin is already written into the file. Worth a follow-up issue.

[Zivxx]

Round-4 addressed — 8/8 threads (2 HIGH · 5 MEDIUM · 1 LOW)

4247d377 source · 6a5b6cf6 tests · 1b98b2bd bed fixture.

The HIGH on the batched read was a real regression, and it was mine

Round 3's batching moved the authoritative read out of lock_source. I documented the staleness window and then mis-rated its consequence as self-healing; it isn't, at the shipped defaults (POLICY_REFRESH_INTERVAL=0 ⇒ nothing re-clones ⇒ 503 until a webhook). The read is back inside each candidate's own lock.

Worth being explicit about the trajectory on that one block, since it has now moved three times: round 2 "O(M²), too slow" → round 3 "batch it" → round 4 "batching deletes live clones". The endpoint satisfies both constraints at once rather than trading them: the precomputed set stays as a pre-filter, so an all-live pass is still ONE scan (now pinned by test_all_live_pass_costs_one_scan, not asserted in a comment), and only dirs that already look orphaned pay for an authoritative read. The new plausibility ceiling caps how many of those there can be in a single pass, so the O(orphans × scopes) shape is bounded structurally rather than by a cadence constant. There is no targeted source_id lookup to make it cheaper — ScopeRepository is keyed by scope_id, all() is a SCAN, no reverse index — so the suggested _source_is_claimed isn't available and this is the stated fallback.

Verification

opal-server unit	236 passed (226 before this round)
opal-common	93 passed
Code mutations	7/7 caught — hoisted read, ceiling removed, ceiling vetoing the opt-in, continue past the pop, pop below the publish, neutralised per-candidate lock, heartbeat only on complete
Docs attacks on the drift guard	3/3 caught — file renamed, heading renamed, body paraphrased with the verbatim text parked under another key
git-leak bed (--boot-scopes=20)	7/7 — orphan-dir, wiped-boot (via the new fixture), shard-reconfig (single-candidate reclaim, unaffected by the ceiling), boot-with-unreachable, warm boot, corrupt clone, churn

Two of the eight were pre-existing, not from round 3: the OSError-skips-the-pop leak and the sweep's untested lock/pop invariants both date to fb1b823d or earlier.

New key

OPAL_SCOPES_ORPHAN_SWEEP_MAX_RECLAIM_FRACTION (default 0.5). A single candidate is always allowed; an opted-in empty-store reclaim skips the ceiling (otherwise that flag became a no-op on any tree bigger than one dir — caught by a test on the way in); 0/1 disables it for a deliberate reshard. Documented verbatim and added to the drift guard.

Things I found by applying your own review lenses to my diff before pushing

Since three lenses — mutation-test every guard, check every comment against the code, check observability — produced five of this round's eight findings, I ran them myself first. They caught, in my own new code: a heartbeat claim of "every exit path" that the no-clone-dir return didn't honour (now does, pinned); a missing metric on the scan_failed abort; the ceiling silently vetoing the empty-store opt-in; and a heading match so loose that #### OPAL_FOO matched #### OPAL_FOO_RENAMED, which had left my first fix for the drift guard still passing a renamed heading.

One judgement call I made differently

On task.py:114 I took the documentation option, not the observed-non-empty-transition one: that memory is per-process state a restart clears, and the leak case (last scope deleted, broadcast lost, leader SIGTERMed) usually involves a restart — so it would be absent exactly when needed, while also silently re-enabling the destructive path on a store that was populated and then wrongly repointed. Reasoning is on the thread; happy to be argued out of it.

[zeevmoney]

Changes requested — 4 HIGH, 9 MEDIUM.

All eight round-4 findings are addressed in the code, and the fixes are pinned better than in any previous round: the per-candidate freshness fix works (a source re-claimed mid-sweep now keeps its clone dir), the ceiling refuses the exact wrong-keyspace scenario, both previously-unpinned sweep guards now fail named tests when reverted, the drift guard's heading-binding and .mdx-rename holes are genuinely closed, and the bed fixture now owns the destructive opt-in for its whole lifetime. e281db4e's self-audit was a good instinct and two of its three claims verify cleanly.

The pattern that has held for three rounds still holds here: the fixes introduced four new HIGH-severity issues on the same surface. Three of them are in _sweep_pass, which is now 153 lines at cyclomatic complexity ~22 against the project's ≤100/≤8 — it was 152/~18 last round, so the split moved code without reducing it. This function has produced a fresh HIGH in each of rounds 3, 4 and 5. Extracting the per-candidate body (lock → classify → in-flight → delete → publish) into its own coroutine, and sharing the reclaim tail with the leader path, is the change most likely to end that cycle — every one of these findings has been an interaction between guards that live too far apart to be read together.

Blocking:

• HIGH packages/opal-server/opal_server/scopes/purge.py:454 — the ceiling's denominator counts dirs that can never be candidates, so six non-source-id directories restore the round-4 wipe in full: 5 junk dirs refuses, 6 deletes all 5 production clones and broadcasts 5 purges
• HIGH packages/opal-server/opal_server/scopes/purge.py:419 — a share-based ceiling only saturates on total store loss; a right-but-incomplete read (replica lag, allkeys-lru evicting scope keys, partial restore) deletes live tenants' clones inside the allowance — 40 of 100 measured, and the under-lock re-check reads the same degraded store
• HIGH packages/opal-server/opal_server/scopes/purge.py:390 — a store read exceeding _STORE_READ_TIMEOUT, or one record whose source_id() raises, turns every candidate into "claimed" permanently; both emit warnings with no metric and then log the pass as complete
• HIGH packages/opal-server/opal_server/tests/orphan_sweep_test.py:444 — the dedicated pin for the round-4 freshness fix never enters the code path (it patches source_id after seeding the fillers, so the pass aborts on the ceiling); the mutation its own docstring names leaves it green

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:456 — the refusal tells the operator to raise the fraction, which can never permit a whole-tree reclaim; and 0/1 disable the ceiling silently while typos warn
• MEDIUM packages/opal-server/opal_server/tests/config_docs_drift_test.py:86 — three remaining holes: a docs-tree move still skips, Default: is asserted against the runtime value so any OPAL_SCOPES_* in the environment reds it, and _TRACKED_KEYS is hand-maintained
• MEDIUM packages/opal-server/opal_server/tests/preload_reset_test.py:150 — assert "3" in warned[0] is satisfied by the timestamp and line number
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:329 — cancelling mid-rmtree deletes the dir but never publishes the confirmation, and the sweep is not covered by the shutdown drain
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:582 — the new Datadog event ships a raw exception repr from the scope store, on a channel log redaction does not cover
• MEDIUM app-tests/git-leak/README.md:63 — the section's premise is false (PR1 is on master, so the bed does run there), and the postgres-bounce reclassification claims more than its evidence supports
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:469 — the refusal metric has no test on any of its three paths
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:286 — the self-audit bounded the sweep's read and left the identical unbounded read under lock_source in the leader delete path
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:385 — restoring per-candidate freshness restored the O(orphans × scopes) cost the previous round removed; the two fixes are oscillating

Details are in the inline comments on each line.

---

On the oscillation specifically. Round 4 batched the store read for speed and that caused live clone deletion; round 5 un-batched it for correctness and that restored the quadratic cost. Both properties are required, and neither round could have both because the read answers the wrong question — it fetches every scope in order to decide one source. A source_id -> scope_id index (written on scope put/delete) makes the per-candidate check O(1) and ends the trade. That is the one structural change I would prioritise over any individual finding above.

Not a finding, for the record. An automated pass flagged the python_requires=">=3.9,<3.13" cap on opal-client as an out-of-scope change. That cap is the fix requested in the round-2 review — leaving opal-client uncapped while its hard dependency opal-common was capped made pip install opal-client fail on 3.13 against a transitive package. It is correct as landed.

Still open, pre-existing, unchanged. api.py:309-315 continues to answer a missing scope record with HTTP 200 carrying the default scope's policy modules, while _allowed_scoped_authenticator only authorizes the caller for the id they requested. Out of scope for this PR — the file is untouched again this round — but the argument for fixing it is already written into the file at api.py:351-356.

[zeevmoney]

Follow-up detail on one already-filed thread — no new findings, and no change to the previous verdict.

The _STORE_READ_TIMEOUT thread understated how reachable that condition is: ScopeRepository.all() issues an awaited GET per key, so a single read is O(scopes) sequential round trips rather than one command. That moves "the sweep silently becomes a permanent no-op" from large stores to ordinary scale, and it means tuning the timeout is the wrong lever in both directions. Details in the reply on that thread.

[Zivxx]

Round-5 addressed — 13/13 threads (4 HIGH · 9 MEDIUM)

08c49416 sweep redesign · 51c5e753 tests + docs · c1195de1 bed gates.

The four HIGHs were three findings about one thing

purge.py:454, :419 and :385 were all the round-4 share-based ceiling: its denominator counted dirs that could never be candidates (six stray entries restored the full wipe), a share only saturates on total store loss so a right-but-incomplete read deleted live tenants inside the allowance, and per-candidate freshness had reinstated the O(orphans × scopes) cost.

Rather than patch the arithmetic a third time, the ceiling is deleted — and SCOPES_ORPHAN_SWEEP_MAX_RECLAIM_FRACTION with it, so net config surface goes down. In its place, the two bounds from the :419 thread:

• Corroboration — a dir must look orphaned across consecutive passes. Replica lag, an LRU eviction of permit.io/Scope:*, a partial restore: all answer correctly again by the next pass, so a transient gap costs one pass of delay instead of a tenant outage. This is what a fresher read can never catch, because the under-lock re-check reads the same degraded store.
• A per-pass count cap (SCOPES_ORPHAN_SWEEP_MAX_RECLAIM_PER_PASS, default 3) — bounds the blast radius of a persistently wrong store, and bounds cost: only eligible dirs pay for a read, so a pass is O(cap) reads, not O(orphans). Both properties that have been traded against each other for three rounds now hold together.

The finding that mattered most was about my verification, not the code

orphan_sweep_test.py:444 — the freshness regression test could not fail: it patched source_id after seeding the fillers, so every dir became a candidate and the pass refused before entering the loop. I had reported it as mutation-verified.

The real defect was in how I verify: my mutation matrix asserted "does the suite go red", so when the hoisted-read mutation failed two other tests it counted as caught while the test that exists to pin it passed. It now names the test that must fail. Every fix this round is verified that way — the mutation fails its specific test in isolation.

Two more of my assertions turned out to be dead on inspection: assert "3" in warned[0] (the timestamp supplies the 3) and the drift guard's Default: check comparing the runtime value, so exporting any OPAL_SCOPES_* reddened a doc-vs-source test with a doc-vs-environment mismatch.

Verification

opal-server unit	245 passed
opal-common	93 passed
Mutation checks	each fix fails its own pinning test in isolation; hoisted read, dropped corroboration, dropped cap, inline reclaim, "claimed" instead of "undecided", removed metric, removed drain warning
Drift-guard attacks (docs broken, guard unmutated)	moved tree 10 failed (was 8 skipped) · env override 10 passed (was a false failure) · new undocumented key 1 failed (was silently unguarded)
git-leak bed	test_boot_states.py 6/6, test_leak.py 4/4

One behaviour change, stated plainly

A leaked dir is now reclaimed on the second consecutive pass, so it lingers up to two sweep intervals (~10 min at the shipped 300s) instead of one. That is the price of not deleting a live tenant's clone when the store answers wrongly; the DELETE path still removes dirs inline, so this only affects what the backstop catches. Both bed gates that encoded the one-pass contract were updated — and now also assert the dir survives the first pass, so they would catch corroboration being removed.

One point I pushed back on

When a scope record's source_id will not derive, the candidate is kept, not reclaimed. "Poisons only itself" is achievable for the pass (fixed — it no longer aborts) but not for the decision: the unresolvable record might be the one referencing that dir. It is now visible (degraded outcome + unresolvable_scope metric) rather than silent and pass-wide. Reasoning is on the :390 thread — happy to switch it, since it trades a leak against a delete and that is your call.

Not done deliberately

The source_id -> scope_id index from the :385 thread. It is the right long-term answer for making the liveness question O(1), but it adds persistent state on the scope write paths plus a backfill, on a PR that is already large — and with reads now capped per pass it buys latency, not correctness. Happy to open it as a follow-up if you want it tracked.

[zeevmoney]

Changes requested — 2 CRITICAL, 4 HIGH, 5 MEDIUM.

A lot landed correctly this round, and it is worth saying so precisely because the headline is negative. The denominator finding is genuinely fixed — junk dilution now changes nothing at K=0..12, where six stray dirs previously restored a full wipe. The freshness pin is de-vacuumed and now dies under two different hoisted-read mutations, with a working non-vacuity guard. The drift guard's three remaining holes are closed and I could not defeat it in five adversarial attempts. preload_reset_test's dead assertion, the cancel-mid-rmtree gap, the store-read timeout, the Datadog exception repr, and the O(orphans x scopes) read are all fixed. The bed's reclaim gates were strengthened rather than made permissive, and the postgres-bounce row now claims only what its evidence supports.

The problem is what replaced the ceiling. Round 5's control could say no; this one can only say slower, and the "slower" depends on state that resets whenever leadership moves. The two together mean there is no configuration in which the sweep both reclaims real orphans and refuses a store that is lying to it.

Blocking:

• CRITICAL packages/opal-server/opal_server/scopes/purge.py:515 — _eligible_for_reclaim has no refusal state. A wrong-keyspace store still deletes every production clone (5 passes, identical at every junk count) and a persistently degraded one still deletes every affected tenant's clone, one interval later than before. The correct denominator the old ceiling lacked now exists at purge.py:764 and is not used for this.
• CRITICAL packages/opal-server/opal_server/scopes/purge.py:41 — corroboration counts passes, not time, and passes are wire-triggerable via POST /scopes/refresh with no rate limit. Two concurrent triggers on a cold purger delete cap dirs, reducing the delay to zero; five concurrent passes broadcast 12 purges for 3 deletions.
• HIGH packages/opal-server/opal_server/scopes/purge.py:214 — the streak is per-purger and the purger is built inside the leadership block, so the boot sweep can never reclaim (0/4 at 1, 3 and 10 boots; 4/4 with a long-lived purger). SCOPES_ORPHAN_SWEEP_INTERVAL=0, documented as "boot and refresh-all still sweep once", is a no-op that logs a healthy complete.
• HIGH packages/opal-server/opal_server/scopes/purge.py:725 — the empty-store opt-in skips both new bounds: 20/20 dirs deleted on a single read.
• HIGH packages/opal-server/opal_server/scopes/purge.py:576 — below the cap the destruction is silent (3/3 deleted, zero warnings, zero metrics), and the capped event goes quiet on the pass that completes a wipe.
• HIGH packages/opal-server/opal_server/tests/orphan_sweep_test.py:870 — the pin for this round's headline property passes under the exact mutation its docstring names; eight other tests notice, that one does not.

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:544 — the "consecutive" half of corroboration is unpinned; accumulating instead of resetting leaves the suite green, and a flapping store defeats the cumulative version
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:595 — the cap slices candidates rather than reclaims, so non-deleting exits spend budget and then report the pass complete
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:562 — no configuration value stops the sweep, and 0 on the rate-limit knob removes the last bound
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:462 — three of six refusal-metric reasons, the unresolvable_scope behaviour, the leader-path read timeout and the Datadog egress fix all ship green when reverted
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:698 — _sweep_pass is now 166 lines at CC ~21, up from 153; it is where every finding above lives

Details are in the inline comments on each line.

---

One claim I could not reproduce, recorded so it is not treated as established. Two independent reviews of this round reported that undeletable dirs (EACCES, a symlink, a wedged git op) permanently hold cap slots and starve genuine orphans forever. I tried three mechanisms — symlinks whose rmtree fails, adversarial naming to force enumeration order, and permanently in-flight git ops — and in every configuration the genuine orphans still drained (4/4 within 25 passes, with up to five stuck dirs against a cap of three). os.scandir order is not stable enough across passes to pin the same prefix. The underlying waste is real and is filed on purge.py:595; the permanent-starvation form is not established and I have not filed it as such.

On the direction of the fix. The cap is a good rate limiter and the wrong ceiling. The two suggestions that would close both CRITICALs together: restore a refusal above the cap, judged against clone_dirs (the validated-names denominator that already exists); and corroborate the store rather than the dir — keep the previous pass's scope-id set and refuse when records that were present have vanished while their clone dirs remain. That distinguishes "a scope was deleted" from "the store lost records", which is the distinction this guard has needed since round 4 and which neither the share threshold nor the streak makes.

Still open, pre-existing, unchanged. api.py:309-315 continues to answer a missing scope record with HTTP 200 carrying the default scope's policy modules. api.py is untouched again this round, so it stays out of scope — but the argument for fixing it is written into the file at api.py:351-356.

[zeevmoney]

Correction to the review I submitted a few minutes ago — one finding was under-rated by me, and the verdict body says something that is now wrong.

I filed purge.py:595 (the per-pass cap slicing candidates rather than reclaims) as MEDIUM, and wrote in the verdict that two independent reviews had reported permanent starvation but that I could not reproduce it across three mechanisms, so I was filing only the weaker "budget is wasted" form.

That was wrong, and both of my harnesses were broken:

1. To make the pass proceed I had set SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE=true with an empty store — which takes the deliberate_mass_reclaim path and skips _eligible_for_reclaim entirely, so the cap I was testing never applied.
2. After fixing that, my undeletable dirs did not happen to occupy the leading enumeration slots, so a spare slot always existed and everything drained.

Re-run correctly, with a non-empty store and the enumeration order controlled:

undeletable=2 (cap=3) -> genuine orphans remaining 0/5
undeletable=3 (cap=3) -> genuine orphans remaining 5/5   STARVED
undeletable=4 (cap=3) -> genuine orphans remaining 5/5   STARVED

The condition is n_undeletable >= cap with those dirs in the leading positions. That is not a mitigation: readdir order on ext4 is stable for a given directory state, and undeletable dirs are by definition never deleted, so the order never changes — whether a deployment lands in the starving arrangement is a coin flip that, once lost, is permanent. Below the cap it drains, progressively more slowly.

purge.py:595 is HIGH, not MEDIUM. The inline comment has been updated with the reproduction and the reasoning; the suggested fix (count reclaims rather than candidates, and de-prioritise dirs whose reclaim already failed) is unchanged and removes the ordering dependence entirely.

Nothing else in the review changes. Round-6 tally is therefore 2 CRITICAL, 5 HIGH, 4 MEDIUM.

[Zivxx]

Splitting the orphan sweep out of PR3 (507905a7)

All 11 findings from this round are the reconciliation sweep. Rather than fix them in place, the sweep is removed from this PR and tracked as PER-15612, where the reclaim policy gets agreed before implementation.

To be explicit about what this is and is not: the sweep is needed — a clone dir whose purge broadcast was lost, crash leftovers, and post-reshard trees all still leak, and closing that is the point of PER-15612. What this change says is only that it does not belong in this PR. Every finding below is carried into that ticket as a design input; none is dismissed, and none is deferred to nowhere.

Why, with the numbers

round	findings	on the sweep	severity
07-22	21	5	4 HIGH
07-28	13	5	4 HIGH
07-29	12	4	5 HIGH
07-30	8	5	2 HIGH
08-02	13	10	4 HIGH
08-03	11	11	2 CRITICAL, 5 HIGH

Last three rounds: 26 of 32 findings were the sweep. This round: 11 of 11, and the first with CRITICALs. The sweep has been redesigned twice inside review — a share-based ceiling, then corroboration plus a per-pass cap — and each redesign produced a fresh CRITICAL/HIGH set, including two on the design shipped one round earlier. Over the same period no other part of this PR drew a finding.

That is not a review problem. Deciding when it is safe to delete a tenant's clone directory, using a store that may be lying, is a design question — what evidence justifies a delete, what the refusal state is and how it clears, whether corroboration is measured in passes or wall-clock, how concurrent passes interact, whether liveness can be asked in O(1). Settling it one comment at a time means every answer ships new surface for the next round. Both of this round's CRITICALs are that pattern precisely: I replaced a fail-closed control with a rate limiter, and I measured corroboration in a unit (passes) that callers can manufacture.

What stays in PR3

Everything it is titled for: the scope git fetch timeout and zombie cap, the fleet-wide cache purge on delete/repoint, the pre-clone liveness probe, the retryable 503 on clone-vanish, pre-fork drain and cache reset, and the watcher's bounded shutdown drain. Zero of this round's findings touch any of it.

Kept with its name corrected: the store-read timeout added last round for the DELETE path's sibling check — a purge fix that had borrowed a sweep-scoped key. Now SCOPES_STORE_READ_TIMEOUT.

Also corrected: four comments that promised the sweep as a backstop. With it gone those were false, and they now say the dir stays until the next purge for that source — which is master's behaviour today, so this PR takes nothing away that master has. What it does not add, for now, is reclamation of a dir whose purge broadcast was lost.

Bed

The three sweep-dependent gates (test_orphan_clone_dir_is_reclaimed, test_redis_wiped_boot_reclaims_clones, the red half of test_shard_reconfig_still_serves_but_orphans_old_clones) are reverted to master's red shape — red by design, owned by PER-15612, and the README matrix says so. Everything else green: test_leak 4/4, test_transitions 6/6, the offline-repo gate, plus 206 opal-server and 93 opal-common unit tests.

The follow-up: PER-15612

Filed, with this round's findings written up as the design constraints. I would rather start it with your agreement on the policy than with more code — the questions it has to answer, all of them from findings on this PR:

1. Refusal. What signal makes the sweep refuse outright rather than rate-limit, and how does that state clear? (Your :515 suggestion — corroborated set judged against validated clone_dirs — is the obvious starting point.)
2. Corroboration. Wall-clock against SCOPES_ORPHAN_SWEEP_INTERVAL, not pass count, and the sweep serialised behind a lock so triggers cannot manufacture passes or duplicate broadcasts.
3. Streak durability. Leader-local state means a leader change resets it; does corroboration need to survive that, and if so where does it live?
4. Liveness in O(1). A source_id -> scope_id index written on scope put/delete removes the scan entirely, and with it the cost pressure that started this oscillation.
5. Starvation. An undeletable dir must not permanently occupy the cap.

PER-15612 carries these plus the acceptance criteria (the three bed gates flipping green, no live clone deleted when the store is wrong rather than empty, a refusal on a wrong keyspace, and every guard pinned by a test proven to fail when that guard alone is removed).

Happy to write the design note first for sign-off, or to open the follow-up PR with the policy stated at the top and the code beneath it — whichever you prefer to review.

[zeevmoney]

Changes requested — 4 HIGH, 5 MEDIUM.

Splitting the orphan sweep out is the right call after six rounds on that surface, and the code removal is clean: every removed symbol is gone, no dead callers, no orphaned config keys, the four SCOPES_ORPHAN_SWEEP_* keys are removed from both config.py and configuration.mdx, containment is intact (every remaining rmtree/free() still derives from _confined_clone_path, and 12 hostile source ids are still rejected), the purge-channel guard including its ALL_TOPICS case is untouched and still rejects every spelling, and the surviving invariants — pop-before-publish, confirmation-under-lock, the _stopping guard, the stop() teardown — all still fail when mutated. Baseline 205 green.

The problem is that the split stopped at the code. It did not follow through to the prose that justified the deliberate leaks, or to the coverage the deleted tests were carrying for surviving behaviour.

The single sentence that matters for this PR, and it should be in the description: the only production path that removes a clone dir for a source no live scope references is the synchronous purge at purge.py:319. There is no other — opal_server contains zero directory-enumeration calls, and the two other rmtree sites re-clone a live source into the same path. So whenever that call does not run, a deleted tenant's policy repo persists on disk indefinitely, surviving restart, redeploy and leader failover. Six branches decline to run it and none records anything that could retry.

Blocking:

• HIGH packages/opal-server/opal_server/scopes/purge.py:13 — the new NOTE says a dropped broadcast leaves the dir "exactly as on master today". Master's delete_scope removed it inline in the DELETE-serving process, depending on no broadcast at all. This PR moves that onto a message that is droppable at shipped defaults, so that case is a regression against the merge base. Per-pod leadership makes it worse than 1:1: master always reclaimed at least the serving pod's copy; this PR can reclaim zero.
• HIGH packages/opal-server/opal_server/scopes/purge.py:309 — the in-flight branch permanently leaks the clone dir and the leader's pygit2 handle, drops the bookkeeping that could identify the pending removal, and publishes confirmed=true so the fleet believes it succeeded. Both replacement comments describe recoveries that cannot occur for a deleted scope.
• HIGH packages/opal-server/opal_server/tests/scopes_task_wiring_test.py:1 — three still-live task.py behaviours lost their only pins; each mutation ships at 205 passed. The module docstring still describes the sweep wiring it no longer tests.
• HIGH packages/opal-server/opal_server/tests/scope_policy_fallback_test.py:118 — this PR pins the missing-record → HTTP 200 with the default scope's policy as "the contract", three lines below new code stating why that exact behaviour is wrong.

Non-blocking:

• MEDIUM packages/opal-server/opal_server/scopes/purge.py:209 — cites SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE, a key this commit deleted, as an operator-actionable mitigation
• MEDIUM packages/opal-server/opal_server/scopes/purge.py:271 — the store-read-timeout branch reverses master's documented direction ("under-purging is a permanent leak") and leaves a repo_locks entry behind
• MEDIUM app-tests/git-leak/README.md:16 — contradicts line 12 in the same paragraph, and e614a84d left "currently unowned" beside the PER-15612 label it just added
• MEDIUM packages/opal-server/opal_server/config.py:354 — the shipped config reference still advertises orphan reclamation; the drift guard cannot catch it because both copies match each other while both are wrong
• MEDIUM packages/opal-server/opal_server/tests/config_docs_drift_test.py:99 — degrades to a green skip when its derived key list comes back empty, and has no reverse direction

Details are in the inline comments on each line.

---

Not filed inline, for the record. The bed's test_orphan_clone_dir_is_reclaimed revert dropped the over-purge half (the orphan-live scope and the live_dirs <= clone_dirs() assertion), so a future sweep that deletes every clone would satisfy the gate; those lines are identical to master so there is nothing in this diff to comment on, but PER-15612 will want them back. test_shard_reconfig_still_serves_but_orphans_old_clones also lost its chown step, which that work will re-derive. And five test docstrings plus api.py:133, git_fetcher.py:919 and six further purge.py comments still name the sweep as a backstop — rg -n "orphan sweep|sweep backstops|boot sweep" packages/opal-server/opal_server/ finds the set.

Two smaller notes. reason="orphan" is now a dead enum value whose field comment still declares it load-bearing, and pubsub.py:230/:368 still name orphan-reclaim as a legitimate publisher. _SOURCE_ID_RE uses \d, which is Unicode for str patterns — containment is unaffected (the derived path stays confined and cannot exist), but [0-9]+ is what was meant.

What I would do before merging. The cheapest way to close the two regressions is to keep master's inline best-effort rmtree on the DELETE-serving worker as a floor. The leader's sibling-checked purge is strictly additive on top of it, the sibling check master used is still available, and it restores the property that a delete always reclaims at least the serving pod's copy regardless of broadcaster state. Everything else here is prose and test coverage.

[zeevmoney]

[HIGH] The NOTE claims parity with master for the dropped-broadcast case, but master deleted the clone dir inline — this PR turns an unconditional local rmtree into a droppable message

Problem: the new NOTE says:

A purge broadcast that never arrives therefore leaves the dir on disk, exactly as on master today, until PER-15612 lands.

That is not what master does. On master, ScopesService.delete_scope calls _purge_source_cache_if_unshared from a finally, and that helper does the removal inline, in the process serving the DELETE:

# origin/master:packages/opal-server/opal_server/scopes/service.py
        try:
            await self._scopes.delete(scope_id)
        finally:
            ...
            await self._purge_source_cache_if_unshared(...)
                # -> await run_sync(shutil.rmtree, str(scope_dir))

No broadcaster, no leader, no message. In this PR delete_scope only publishes, and the rmtree lives solely in LeaderScopePurger.purge_source_if_unshared, reachable only through the leader's subscription. The PR's own test pins the new behaviour: delete_scope_cache_purge_test.py:114 asserts clone.exists() after delete_scope.

So for the case the NOTE is about — the broadcast not arriving — master removed the dir and this PR does not. That is a regression against the merge base, not parity.

And the broadcast is droppable at shipped defaults. SERVER_WORKER_COUNT defaults to CPU-core count, so a DELETE usually lands on a non-leader worker and must traverse the broadcaster; the global listening context that keeps a worker's reader alive independently of connected clients is created only when STATISTICS_ENABLED, which defaults to False. This PR already documents that hazard for workers (git_fetcher.py:899-901, scopes/task.py:203-207: "a client-less non-leader to pin the handle for life") — it now applies to the leader, which is the only actor that deletes the dir. SCOPES_PURGE_CHANNEL is additionally freeze-exempt, so a publish during a backbone gap is attempted and lost rather than deferred.

Failure scenario: 4-worker server, statistics off, no PDP connected yet (boot window, or a scopes-only control plane). DELETE /scopes/tenant-a lands on worker 2; the leader is worker 0 with no reader running. The record is deleted and the purge published; LeaderScopePurger.handle never fires. The clone dir — a full git clone, MBs to GBs on a PVC that survives restarts — stays forever, no confirmation is published so no worker drops repos/repos_last_fetched either, and the scope record is gone so nothing will ever name that source again. Master removed it.

The multi-pod case makes this sharper. Leadership is a host-local fcntl.flock and each pod's leader clones every scope, so every pod holds its own copy of every tenant's repo. A single delete therefore has to be received by P leader workers. Master removed the dir on the DELETE-serving worker's own pod unconditionally, so master always reclaimed at least one pod's copy. This PR can reclaim zero — if the serving worker is not a leader and no leader has a reader running, no pod removes anything, and nothing logs it.

Suggestion: pick one, but the sentence cannot ship as written — a reviewer or operator reading it will conclude this PR cannot regress dir cleanup.

1. Keep a best-effort inline rmtree on the DELETE-serving worker as a floor. It is exactly what master does, and the leader's sibling-checked purge is strictly additive on top of it. The sibling check is the reason it was moved, but a local removal guarded by the same _find_scope_sharing_source call master already used preserves that.
2. Or correct the NOTE to say the lost-broadcast case is worse than master, and state the operational consequence plainly.
3. Or hold PR3 until PER-15612 lands.

[zeevmoney]

[HIGH] The deferred branch permanently leaks both the clone dir and the leader's pygit2 handle, and the two lines written to replace the sweep reference describe recoveries that cannot happen

Problem: when a lingering timed-out git op holds the source, this branch deliberately skips the rmtree and the handle free, sets confirm = True, and publishes the confirmation. The two comments that used to say "the orphan sweep reclaims it" were rewritten by 507905a7 to:

• here — "a later purge removes it"
• purge.py:71 — "a skipped free self-heals on the next validity probe for that source"

Neither can occur for the case that reaches this branch:

• "a later purge" — a purge is only published by a scope delete or repoint. The scope record was already deleted, so nothing will ever name this source_id again. I confirmed the mechanism: after the deferred purge the dir survives; a second purge command for the same source does remove it — but for a deleted scope that second command never arrives.
• "the next validity probe" — forget_repo is reached from the invalid-repo branch of fetch_and_notify_on_changes, i.e. during a sync of that source, and sync is gated on a live scope record. There is no next probe. The claim is self-contradictory: purge_local_memory is only called for sources being purged, which by construction have no live scope.

This is not a rare race — it is the delete-a-broken-scope workflow. git_op_in_flight stays True for the whole lingering window after a timeout, "until the blocking pygit2 call actually returns" (git_fetcher.py:199-206), which against a black-holed remote is indefinite. Deleting the scope whose remote is hung lands here by construction.

Failure scenario: tenant-a points at a hung remote; a fetch times out and its daemon thread lingers, so the source is in _git_busy. Operator runs DELETE /scopes/tenant-a. The leader takes this branch, logs "a later purge removes it", drains repos_last_fetched and repo_locks, and publishes confirmed=True — so every worker now believes the source is purged. On the leader the clone dir remains on disk and GitPolicyFetcher.repos[<path>] still holds the live pygit2.Repository with its mmapped packfiles, for the life of the process. Nothing revisits either.

Suggestion: the honest text is "kept until PER-15612's sweep lands — for a deleted scope no later purge will name this source". Better than documenting it: retry the deferred removal once the in-flight marker clears, as a short bounded follow-up owned by _pending_purges, so the branch is not a permanent leak in its most likely trigger.

[zeevmoney]

[HIGH] The removal deleted the only pins for three still-live behaviours in this file

Problem: 507905a7 removed six tests from scopes_task_wiring_test.py. Four were sweep-only and correct to delete. Three others each carried a second assertion about code that survives, and that half went with them. Each mutation below leaves the suite at 205 passed on this head, and fails at c1195de1:

mutation	at e614a84d	deleted test that caught it
drop if POLICY_REFRESH_INTERVAL > 0: create_task(self._periodic_polling()) (task.py:74-75)	205 passed	test_orphan_sweep_timer_starts_even_when_polling_is_enabled
strip the try/except around await self._service.sync_scopes() (task.py:120-123)	205 passed	test_sync_all_then_sweep_still_sweeps_when_sync_raises
replace await self._sync_all() in trigger's refresh-all branch (task.py:157)	205 passed	test_refresh_all_trigger_sweeps

The polling one is the worst: app-tests/git-leak/docker-compose.yml sets OPAL_POLICY_REFRESH_INTERVAL: "0", so the bed never starts that task either. After this removal nothing anywhere covers it.

The try/except one is pointed differently: the five-line comment immediately above it (task.py:115-119) still explains why it must exist — "an unhandled raise here would die silently … not even asyncio's 'never retrieved' warning until GC" — with nothing enforcing it.

Failure scenario: a later refactor drops the polling start while collapsing start(); the leader's scopes watcher stops re-syncing scope repos entirely, policy updates land only on webhook/refresh-all, and the suite is green. Or sync_scopes() raises at boot on an unreachable Redis, the wrapper is gone, the fire-and-forget task dies with the exception never retrieved because stop() gathers with return_exceptions=True, and boot sync silently does not happen with no log line.

This file's own module docstring still describes what was deleted: "sync-then-sweep ordering on the boot and refresh-all paths (the bed's orphan gates depend on the trigger path sweeping; unit-pins the wiring so a refactor can't silently drop it)." No test in the file does any of that now — and the three pins above, which it is describing, are exactly the ones that went.

Also in this file: test_single_scope_trigger_does_not_sweep kept its name and assertion after _Recorder.sweep_orphans was deleted, so events can never contain "sweep" and the "does not sweep" half is unfalsifiable. It is not vacuous overall — forcing trigger's dict branch unreachable does fail it, so it still pins the single-scope dispatch — but the name no longer corresponds to anything the code can do, and the risk is that someone deletes it as sweep residue and takes the live pin with it. Rename to test_single_scope_trigger_does_not_sync_all.

Suggestion: re-add the three assertions with their sweep halves stripped. The _Recorder / _bare_task scaffolding they need is still in the file, so each is about three lines:

async def test_periodic_polling_starts_when_enabled(monkeypatch):
    monkeypatch.setattr(opal_server_config, "POLICY_REFRESH_INTERVAL", 30)
    t = _bare_task(events); await t.start()
    assert "_periodic_polling" in {x.get_coro().__name__ for x in t._tasks}

async def test_sync_all_survives_a_raising_sync():
    await _bare_task(events, fail_sync=True)._sync_all()      # must not raise
    assert events == ["sync"]

async def test_refresh_all_trigger_syncs():
    await _bare_task(events).trigger(topic=None, data=None)
    assert events == ["sync"]

[zeevmoney]

[MEDIUM] This comment points the operator at a config key the same commit deleted

Problem: the _stopping branch tells the reader that the abandoned work is recovered:

The next leader's boot sweep reclaims it — unless the store reads empty by then (the last scope's dir), which that sweep refuses to act on by default (see SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE).

Both halves are now wrong. There is no boot sweep in this PR, and SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE no longer exists in config.py or configuration.mdx — setting OPAL_SCOPES_ORPHAN_SWEEP_RECLAIM_ON_EMPTY_STORE is now silently ignored. This is the last surviving reference to that key anywhere in the repo, and it reads as an operator-actionable mitigation for a dir that is in fact unrecoverable.

The same file has five further now-void sweep references (purge.py:263, :267-268, :280-281, :301, :334), plus purge.py:338 — "Published under the lock, like sweep_orphans" — a cross-reference to a function that no longer exists in the file. api.py:133 and git_fetcher.py:919 carry the same class of stale claim, and five test docstrings assert that a leak is acceptable because the sweep reclaims it (delete_scope_route_test.py:80, delete_scope_cache_purge_test.py:121, repoint_purge_test.py:186, liveness_probe_test.py:131, and scopes_task_wiring_test.py's module docstring, which still describes sync-then-sweep ordering that no test in the file exercises).

Failure scenario: an operator hits the stopping-purger case, reads this comment, and goes looking for a knob that does not exist. A reviewer auditing the deliberate-leak decisions sees four tests asserting a backstop exists and concludes the leak is bounded.

Suggestion: the removal followed through in the code and stopped at the prose. Every one of these should name PER-15612 as the thing that will reclaim it, and say plainly that until then the dir persists. rg -n "orphan sweep|orphan-sweep|sweep backstops|boot sweep" packages/opal-server/opal_server/ finds the full set.

[zeevmoney]

[MEDIUM] Two fail-open branches log a recovery that cannot occur, and leave a repo_locks entry behind

Problem: both rewritten log strings promise a later purge:

• the sibling-check timeout branch — "keeping the clone; it is removed by the next purge for this source"
• the repoint scan-failure branch (purge.py:282-286) — "keeping the clone (a later purge for this source removes it)"

Same defect as the in-flight branch. For reason="delete" the record is already gone; for reason="repoint" the old source_id is no longer referenced by any scope. In both cases no future delete or repoint will ever name that source, so the "next purge" does not come.

Both branches also return with the repo_locks entry that lock_source minted still in the dict — neither path pops it. That is invariant I4 (app-tests/git-leak/invariants.py:75-80), which the sweep's finally: GitPolicyFetcher.repo_locks.pop(name, None) used to drain. Nothing does now. It is small — one asyncio.Lock per occurrence, for process lifetime — but it is a re-opened instance of the exact invariant this series was built around.

Failure scenario: Redis is slow for longer than SCOPES_STORE_READ_TIMEOUT during a DELETE /scopes/x. The leader logs "it is removed by the next purge for this source"; there is no next purge. The dir and the repo_locks[sid] entry persist for the life of the process, and an operator who trusts the log stops looking.

This one is also a direction reversal against master. Master's _find_scope_sharing_source purged defensively on any sibling-check failure, and its docstring says why:

"Over-purging self-heals (a surviving sibling re-clones on its next sync); under-purging is a permanent leak."

This PR keeps that reasoning for a generic Exception on a delete (purge.py:288-292) but carves out asyncio.TimeoutError and takes the opposite direction — keep the clone, publish no confirmation, return. So a Redis blip longer than SCOPES_STORE_READ_TIMEOUT at delete time now leaves the dir and leaves every worker's repos / repos_last_fetched entries populated for a scope that no longer exists. Under master's own stated rationale that is the strictly worse choice, and for reason="delete" the record is already gone so there is no sibling to protect.

Reserve the keep-the-clone direction for reason="repoint", where a live record genuinely still exists; purge on timeout for a delete.

Suggestion: say what actually happens ("kept; reclaimed by PER-15612's sweep when it lands"), and pop repo_locks on both paths — a finally around the async with lock_source(...) body covers all exits at once, which is what the removed sweep did.

[zeevmoney]

[MEDIUM] The status paragraph contradicts itself, and the commit that exists to name PER-15612 left "currently unowned" in the same row

Problem: two things in one file.

1. Self-contradiction inside the rewritten paragraph. Line 12 now opens "Status as of PR3: the three orphan-sweep gates are RED again by design." Line 16 — four lines later, same paragraph, same hunk — still reads "Every gate below passes on the PR3 head except assertion (d) of test_server_recovers_after_postgres_bounce." Both cannot be true, and rows 76, 77 and 79 of the same table say three gates FAIL. The removal replaced the "20/21 green" sentence but not the "every gate below passes" one that followed it, and no new pass count was put in its place.

2. The ownership fix is half-applied. e614a84d's entire stated purpose is to name PER-15612 as the sweep's owner. It rewrote the Role column (gate (orphan sweep, unowned) → gate (orphan sweep, PER-15612)) and did not touch the Behaviour column beside it, which still ends "no orphan sweep exists yet (PR3+, currently unowned)". test_boot_states.py:111 — "RED until an orphan sweep exists (PR3+, currently unowned)" — was not touched at all, so the test docstring and its README row now assert opposite ownership. rg -n "currently unowned" app-tests/ returns exactly those two.

This is the README-row-versus-docstring divergence earlier rounds flagged, reintroduced by the commit whose sole purpose was to fix it.

Failure scenario: a reviewer reads §Status, concludes only the flaky postgres assertion is red, runs the bed, and gets four failures with no way to tell which are expected — the file's own text supports both readings. Separately, the follow-up gets triaged as unowned work off the docstring and duplicated, or PER-15612 is closed as having no owner recorded.

Suggestion: line 16 → "Every gate below passes on the PR3 head except the three orphan-sweep gates above and assertion (d) of …", restate the count, and replace "currently unowned" with "tracked as PER-15612" in both places.

Related, same file: line 69 says the churn guard's repoint ops are "covered separately by the red repoint gates", while rows 62 and 72 say both repoint gates PASS since PR3's repoint purge.

[zeevmoney]

[MEDIUM] A shipped, operator-facing config description still advertises orphan reclamation

Problem: SCOPES_PURGE_CHANNEL's description says the channel is used

"when a scope is deleted, repointed to a new source, or its clone dir is reclaimed as an orphan"

Nothing publishes an orphan reclaim any more — _reclaim_and_confirm was its only producer and it was removed. The same string is copied verbatim into documentation/docs/getting-started/configuration.mdx, so this ships to operators in the public config reference.

The four SCOPES_ORPHAN_SWEEP_* keys were correctly removed from both files, and SCOPES_STORE_READ_TIMEOUT is consistent in both — this description is the one config-vs-code mismatch the removal missed, and the drift guard cannot catch it because the guard compares the two files to each other, not either one to the behaviour.

Related, same class: reason="orphan" is now a dead enum value on ScopePurgeCommand. Its field comment still declares it load-bearing ("delete/orphan purge") and the type comment still lists it, and pubsub.py:230 / pubsub.py:368 still name "orphan-sweep" as a legitimate publisher in the authz docstring. The gate itself is unaffected — only the prose.

Failure scenario: an operator reading the shipped documentation expects clone dirs to be reclaimed automatically and does not provision anything to do it — which, given the leaks in the other threads, is the specific wrong conclusion to draw from this PR.

Suggestion: drop the orphan clause from the description in both files (it is the same string), and narrow the reason type to "delete" | "repoint" with the matching comment changes in purge.py and pubsub.py.

[zeevmoney]

[MEDIUM] The drift guard skips silently when its derived key list comes back empty, and has no reverse direction

Problem: two holes, both demonstrated. The guard is otherwise in good shape — I re-ran all five adversarial attacks from earlier rounds against the now-7-key set and every one still fails correctly (docs tree renamed → 7 failed, not skipped; OPAL_SCOPES_* env vars exported → 7 passed; undocumented key added → 1 failed; heading renamed → 1 failed; description moved under another heading → 1 failed).

1. Empty derivation degrades to a green skip. _tracked_keys() derives from re.findall(r"\n (SCOPES_\w+) = confi\.", source) and feeds the result straight to @pytest.mark.parametrize with nothing asserting it is non-empty. Simulating a refactor that renames the Confi handle inside the class (confi. → _confi., with _confi = confi above): config.py imports fine and every key still resolves, and the guard reports 1 skipped — got empty parameter set for (key). Lines 104-117 already turn a missing .mdx into a hard pytest.fail; the structurally identical "guarding nothing" state one level up is a silent skip. That is exactly the failure the docstring says the derivation exists to prevent.

Most plausible trigger for this series specifically: the scopes keys move to opal_server/scopes/config.py. The path constant still points at opal_server/config.py, zero keys derive, the guard skips, and every description is free to drift again.

2. No reverse direction. The parametrization is config.py → .mdx only, so a doc entry for a key that no longer exists is invisible. Pasting a #### OPAL_SCOPES_ORPHAN_SWEEP_INTERVAL section with a plausible default and description back into configuration.mdx gives 7 passed. The .mdx is in fact clean on this head — I verified the 7 declared keys map 1:1 to the 7 #### OPAL_SCOPES_* headings with no ORPHAN_SWEEP residue — so this is a hole, not a live violation. But this PR just deleted three keys from both files, which is precisely when the reverse direction matters.

Suggestion: two small additions.

_TRACKED_KEYS = _tracked_keys(_CONFIG_PY_PATH)
assert _TRACKED_KEYS, "no SCOPES_* keys derived from config.py — this guard is no longer guarding anything"

def test_no_documented_scopes_key_is_undeclared():
    documented = set(re.findall(r"^#### OPAL_(SCOPES_\w+)$", _MDX.read_text(), re.M))
    assert documented <= set(_TRACKED_KEYS)

[zeevmoney]

[HIGH] This PR promotes a cross-tenant policy-serving path from an inherited wart to an asserted contract

Problem: this docstring line is added by this PR. GET /scopes/{scope_id}/policy, when the record is missing, returns HTTP 200 carrying the default scope's policy modules — and _allowed_scoped_authenticator (api.py:89-98) authorizes the caller for the requested scope_id only. It says nothing about "default".

The behaviour is pre-existing and I have raised it in earlier rounds as out of scope. What changed is that this PR removed the sibling fallback — invalid clone → default bundle — and replaced it with a 503, writing the reason into the code at api.py:352-356:

"Serving the default scope's bundle here would hand a live tenant another tenant's policy — tell the client to retry instead."

That reasoning applies verbatim to the branch three lines above, which was kept. And this test now pins it as intended behaviour. An inherited inconsistency that a future reader might have fixed is now an invariant they have to argue their way out of.

Failure scenario: a PDP holds a token with allowed_scopes: ["acme"]. acme is deleted, or Redis is briefly unreachable so scopes.get raises ScopeNotFoundError. GET /scopes/acme/policy returns 200 with the default scope's bundle; the PDP loads it into OPA and enforces another tenant's authorization rules. No 4xx, no alert — the only signal is a ScopeNotFound metric event. Nothing prevents a real tenant's scope from being named default.

Suggestion: either 404 the missing scope — which is what get_scope (api.py:208-211) and refresh_scope (api.py:275-278) already do for the same condition — or, if the fallback must stay for compatibility, gate it on the caller being authorized for "default":

        except ScopeNotFoundError:
            if not authenticator.enabled or "default" in claims.get("allowed_scopes", []):
                return await _generate_default_scope_bundle(scope_id)
            raise HTTPException(status.HTTP_404_NOT_FOUND, detail=f"No such scope: {scope_id}")

At minimum, change this docstring so it records the behaviour without blessing it — "unchanged, and inconsistent with the 503 introduced below; tracked as " — so the next reader is not told it is the contract.