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	c1195de
🔍 Latest deploy log	https://app.netlify.com/projects/opal-docs/deploys/6a7092df056d2a00082dacdb

[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.

[Zivxx]

Round-2 review addressed — 16 of 21, bed-validated

Thanks for the thorough pass @zeevmoney. Pushed the fixes (d2ad0455..37021775, 23 commits, grouped by finding with per-comment trailers) and replied inline on each thread.

Fixed + resolved (16): fork/teardown safety (fork-lock triple, reset_caches in-flight guard, keep-markers, bounded drain); zombie cap + gauge; CPython _worker shape-check + python_requires<3.13; honest 409-vs-503; reason-aware fail-open; always-on orphan-sweep timer; hybrid sweep (one snapshot/pass, off-loop is_dir, logged re-check); freeze-exempt purge channel; wrapped config desc; ported TOCTOU/shard/wiring/route tests + restored the defensive WARNING assertion (and fixed a real CancelledError-swallow the tests surfaced); public docs for the new keys + dropped the private .claude doc.

Validation: opal-server unit suite green, and the git-leak docker bed is 21/21 with zero invariant violations. The bed earned its keep — it caught that an attempted local-memory-purge fix for service.py:213 minted stray repo_locks (I4) and over-purged shared sources, so I reverted it (37021775).

Left open, deferred to a follow-up (lock-lifecycle / "Plan B"): the two HIGH use-after-free items (worker-side lock_source + pop-last ordering + epoch) and the purge-channel auth (fail-closed + server-minted marker) — these want a small delivery/verification design pass; the dead clone_path field rides along with that schema change. And the memory half of service.py:213 (draining a client-less non-leader) lands there too; its disk half is already fixed here via the always-on sweep timer.

Happy to do a quick sync on the purge-delivery split for the UAF fix before I implement it.

[Zivxx]

PR #924 — post-review hardening pass (for @zeevmoney)

Ran a 3-round adversarial + constructive review over the whole branch (correctness,
test coverage, best-practices, observability, security), then verified every finding
adversarially and re-triaged severity by hand. Headline: no critical / data-loss /
crash / fleet-break bugs in the concurrency-heavy diff. Fixes below landed in
ff5904ba; 199 opal-server unit tests + 21 git-leak bed tests green.

Fixed in ff5904ba

Security

• Purge channel is now server-to-server only — addresses your [HIGH] on
  purge.py:73. New _verify_permitted_topics-style channel restriction rejects
  SCOPES_PURGE_CHANNEL publish/subscribe from any external RPC peer. Confirmed the
  full chain: RpcEventServerMethods.publish is client-callable and the
  permitted_topics guard default-allows tokens with no such claim, so a forged
  fleet-wide purge was reachable from any PDP. Legitimate publishers (delete/repoint/
  sweep + the cross-server broadcaster relay) all notify() with channel=None and
  bypass the gate; clients only ever publish STATISTICS_ADD_CLIENT_CHANNEL, so
  no client is affected and no redeploy is required — pinned by a test.

Correctness

• _get_current_branch_head no longer conflates permanent vs transient. A missing
  ref (KeyError) is permanent → BranchHeadNotFoundError → 409; a transiently gutted
  object store (pygit2.GitError) now propagates → retryable 503, instead of telling a
  self-healing scope "not retryable". Serving-path only; get_commit_hash and the
  notify path are untouched. The real raise path is now covered (was mock-only).

Observability (Datadog)

• Log the preload drain timeout instead of discarding drain_git_ops()'s bool —
  that is the exact condition that carries lingering git ops across the fork, and it
  was silent.
• Continuous git_ops_in_flight gauge (not just the one-shot log at the zombie cap).
• Orphan-sweep heartbeat summary so a healthy no-op sweep is visible.
• metrics.event on the 503/409 policy-unavailable responses.

Hygiene

• Clean-log GitConcurrencyLimitExceeded (no per-scope traceback during a zombie-cap
  outage); \Z (not $) in the source_id regex (trailing-newline bypass); skip the
  always-on orphan-sweep timer when polling already sweeps (no duplicate scans/
  broadcasts); fix the misleading ScopePurgeCommand.reason / clone_path comments;
  drop an unused import.

Still deferred to Plan B (unchanged — flagging for clarity)

These remain open with our earlier "deferred" replies; this pass did not implement
them, so they stay open:

• purge.py:62 [HIGH] — worker frees the shared pygit2 handle outside lock_source
  (Plan B lock-discipline).
• purge.py:249 [HIGH] — repo_locks popped before the confirmation publish (same
  Plan B change: pop last).
• service.py:213 [MEDIUM] — DELETE's memory-half still depends on the leader's
  reader; only the disk half (always-on sweep) is done.
• purge.py:44 [LOW] — clone_path's comment is now accurate, but dropping the unread
  wire field is still bundled into the Plan B schema change.

Also from the review, not blocking

One latent medium we chose to measure before fixing: a preload git op that outlasts
the fetch timeout + the drain can persist in the gunicorn master across the fork and
race a worker on the shared clone dir. Trigger is narrow (op must be actively writing,
not just hung) and self-healing; the new drain-timeout log now tells us how often the
window even opens in prod before we invest in the temp-clone+rename fix.

[zeevmoney]

Changes requested — 2 HIGH, 4 MEDIUM.

Blocking:

• HIGH packages/opal-server/opal_server/pubsub.py:388 — the ALL_TOPICS exemption reopens the purge channel on the subscribe path; verified against the installed fastapi_websocket_pubsub that an external peer subscribed as ALL_TOPICS (bare or in a list) receives every ScopePurgeCommand
• HIGH packages/opal-server/opal_server/scopes/purge.py:305 — the sweep's snapshot filter is O(dirs × scopes) with two sha256 per pair and no await; measured 1.25s of blocked event loop at 2000 scopes, growing quadratically

Non-blocking:

• MEDIUM packages/opal-server/opal_server/tests/purge_channel_test.py:112 — the only test guarding the confirmed gate passes with the gate deleted (verified by mutation; whole suite stays green)
• MEDIUM packages/opal-common/setup.py:76 — opal-client was not capped alongside opal-common/opal-server, so pip install opal-client on 3.13 fails on a transitive dependency
• MEDIUM packages/opal-server/opal_server/scopes/task.py:43 — the leader purge subscription has no stop() teardown and _pending_purges is not cancelled or awaited, so a shutdown can abandon an rmtree and skip the confirmation publish
• MEDIUM packages/opal-server/opal_server/tests/purge_channel_test.py:657 — the third test dropped in the delete_scope_cache_purge_test.py rewrite (recreate-after-delete) still has no equivalent

Details are in the inline comments on each line.

---

PR description accuracy (not inline findings — the description does not match the diff):

• The "New config keys" table lists 3 keys; config.py adds 6. SCOPES_GIT_PRELOAD_DRAIN_TIMEOUT (10.0), SCOPES_GIT_MAX_ZOMBIES (40) and SCOPES_ORPHAN_SWEEP_INTERVAL (300) are missing. All six are documented correctly in configuration.mdx — verified programmatically that every documented default matches config.py.
• "The 503 above is the one intentional contract change" — api.py also adds a 409 CONFLICT for BranchHeadNotFoundError, a case that previously fell through to _generate_default_scope_bundle and returned 200. That is a second contract change and it is not in the §5 table. Worth noting for direct consumers that opal-client cannot act on the "non-retryable" distinction: throw_if_bad_status_code turns any non-200 into a ValueError, so 409 and 503 are both retried identically by its tenacity policy.
• "packages/opal-client and packages/opal-common are untouched" — packages/opal-common/setup.py is touched (python_requires narrowed to <3.13), which is a packaging change for downstream consumers.
• The "⚠️ Caveat" says worst-case thread count is SCOPES_GIT_MAX_WORKERS plus lingering timed-out ops, with no bound. SCOPES_GIT_MAX_ZOMBIES now bounds exactly that, so the caveat understates the code.
• "opal-server unit suite: 151 passed" — the suite is 199 on this head (green, re-run locally).
• The linked design doc path docs/superpowers/specs/2026-07-19-pr3-deferred-items-design.md does not exist in the repo.

Verified clean: all 199 unit tests pass on this head; pre-commit (black / isort / docformatter / codespell) passes on the diff; the six new config defaults match their configuration.mdx entries exactly; no new log line carries an unredacted URL or credential (only server-local filesystem paths); _reject_external_purge_channel correctly does not fire for server-internal publishes (PubSubEndpoint.publish passes channel=None, and restrictions run only if channel:); the endpoint's publisher id and subscriber id are distinct, so the "confirmation runs the local subscriber inline" rationale holds; the executor object-lifetime chain keeps a timed-out op's executor alive to completion without leaking, and the live-ops semaphore is released exactly once on both the success and timeout paths.

[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.