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

[dshoen619]

PR3 — Git resilience: never stuck on an offline repo

Closes PER-15157 and PER-15158 (PR4 #932 is closed; its parallel-boot charter is delivered here — see §5).

This description was rewritten from the code at c6899e11. The previous one had drifted several rounds out of date — it described an orphan sweep that is no longer in this PR, asserted an invariant the code has never held, and listed 3 of 6 config keys. Reviewers were reading it as the spec. If anything below disagrees with the code, the code is right and this is a bug.

1. Git resilience — a hung remote 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). Boot blocked indefinitely on one unreachable repo; in steady state a hung op held the per-source lock and a shared-pool thread.

• Soft per-operation timeout (SCOPES_GIT_FETCH_TIMEOUT) on clone and fetch. Soft is load-bearing and stated everywhere: it unblocks the event loop and the awaiting coroutine, not the thread. The pinned pygit2 binds no server-read-timeout setting (libgit2 1.7.2 itself has GIT_OPT_SET_SERVER_TIMEOUT; pygit2 1.14 does not expose it), so the underlying call keeps running on its own daemon thread until the OS gives up.
• Zombie-aware executor. Each op gets its own single-use daemon-thread executor. SCOPES_GIT_MAX_WORKERS bounds only live ops via a semaphore, and a timed-out op releases its slot immediately — that is the real content of "one hung remote can't block others", and the test_offline_repo_does_not_block_healthy_scopes gate.
• SCOPES_GIT_MAX_ZOMBIES is a global ceiling, not a per-source guard. It counts live plus lingering ops across all sources; at the ceiling, new git ops are refused for every scope until threads drain. Per-source protection is separate (a source with an op in flight is skipped). Documented as such — an earlier description claimed "capacity is never starved by hung remotes", which is false at saturation.
• git_op_in_flight(source_id) guards every path that frees a pygit2 handle or deletes a clone dir. Enumerated: 10 call sites, all guarded in-process.

Fork safety (not previously described, and it is where several of the nastiest fixes live): the pre-fork preload drains in-flight ops (SCOPES_GIT_PRELOAD_DRAIN_TIMEOUT), tears down the git executor, and clears the fetcher caches, so the gunicorn master does not carry git state — handles, locks, in-flight markers — across the fork into workers. _DaemonThreadPoolExecutor mirrors CPython concurrent.futures internals and shape-checks every private it touches, falling back to the stdlib on mismatch.

2. Fleet-wide memory purge — two-phase

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.

New every-worker channel SCOPES_PURGE_CHANNEL, two-phase:

1. Routes publish a purge request (confirmed=False).
2. Only the leader acts on requests: it is the only actor that can sibling-check (it reads the store), and acting on a raw request would drop caches a surviving sibling scope still uses. Runs 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.

The confirmation is published while holding lock_source: it frees this process's cached handle via the inline local subscriber, so releasing first opens a use-after-free against a re-created scope's _notify_on_changes. The repo_locks entry is popped before the publish for the same reason — the handler re-enters lock_source.

Repoint: PUT /scopes with a changed source_id publishes a purge for the old source. Note it has no local floor (see §3), so a lost broadcast on repoint strands the old clone dir.

Mixed-fleet safe: old workers aren't subscribed, confirmed is additive with a False default, external RPC peers are rejected on the channel, and a forged clone_path never reaches the filesystem (every handler re-derives from source_id).

3. Clone-dir removal — what this PR does and does not do

The distributed disk reclaim was cut out of this PR and is PER-15612, together with the orphan sweep. The seam is memory vs disk: a memory purge self-heals in both directions (a wrongly-dropped handle re-opens on next use); disk reclaim has no self-healing direction — under-purge leaks forever, over-purge forces a re-clone — and needs a cross-process design agreed before implementation.

So there is exactly one path in the server that removes a clone dir: a best-effort local purge on the worker that served the DELETE, which is what master did inline. It sibling-checks, bounds its store read, skips while a git op is in flight (master freed the handle unconditionally — that is the use-after-free 89e090be fixed), and keeps the clone if the check fails.

Its guards are process-local (lock_source is an asyncio.Lock; the in-flight marker is a module-global set), so it is not serialized against the leader cloning the same source in a sibling process on the same pod. This is not a regression: running the same two-process race against origin/master produces the same orphan, and master has no in-flight guard at all and no broadcast to reclaim it. It is also not an invariant — the previous description's "only the leader mutates the clone tree" was never true, and that claim has been removed from the code as well.

Two behaviour changes against the merge base that belong here rather than only in code comments:

• Master serialized the record delete and the sibling check under one lock, so the last of two concurrent sibling deleters always saw no sharer. The floor takes the lock later, in a background task, so an unlucky interleaving can have both deleters see the other as live and both skip. Tracked with the rest of the reclaim work on PER-15612.
• The floor skips while a git op is in flight, where master removed the dir unconditionally. That is the safe direction — master's version is the use-after-free 89e090be fixed — but it means deleting a scope whose remote is hung now leaves the dir on every pod, including the serving one, where master would have removed it.

What nothing reclaims, stated plainly:

• a DELETE's dir on every pod except the serving one, when the broadcast is lost — and it is droppable at shipped defaults (a DELETE usually lands on a non-leader worker and must traverse the broadcaster, while a leader keeps a reader alive only with a connected client or STATISTICS_ENABLED, default False);
• a REPOINT's old dir, always;
• a dir whose source_id is unknowable because the prior record would not parse.

All three are PER-15612, and all three are stated at the point they occur in the code (scopes/purge.py, scopes/api.py) rather than only here.

4. Scope-liveness check before clone

A sync that loaded a scope before its DELETE could re-clone the dead scope and re-populate 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 confirms the scope still points at this source. Fails open on store errors. Residual: the probe is a point-in-time check and the window is the whole clone, so a delete landing mid-clone can still be re-created by the finishing clone.

5. Parallel scope loading — PR4's charter

sync_scopes was serial: one await sync_scope(...) per scope, giving a ~20-minute boot on a fleet of repos — and once §1 added a per-fetch timeout, a serial loop would still stall healthy scopes behind each unreachable repo's timeout. The two ship together, which is why PR4 is closed as redundant.

Two phases, each asyncio.gather under its own semaphore, both bounded by SCOPES_GIT_MAX_WORKERS: phase 1 clones/fetches each distinct repo, phase 2 change-checks the scopes that reuse an already-handled repo. (An earlier design gave phase 2 a wider max(N, 32) floor; that was removed in review because 32 was a floor the knob could never lower, and sync_scopes_perpass_test.py now pins its absence.) Per-scope failures stay isolated. The boot path gets this too — preload_scopes() calls the same sync_scopes.

Measured head-to-head, and the win is large. A local two-image comparison exists — same fleet, same hung model, one image built pre-PR3 and one from this tree — in permit-backend@ziv/opal-staging-test-kits, commit 17874dae3 (v2/backend/scripts/opal-tests-and-results/prs-3-4-boot-improvements/, with scripts, RUNBOOK and results):

config	boot to all-healthy-served	preload sweep
old — serial sync_scopes, no fetch timeout	306.2s	302.3s
new@120 — this PR, prod-default timeout (concurrency alone)	53.6s	50.1s
new@15 — this PR + tuned timeout	43.2s	30.1s

30 healthy + 12 hung repos interleaved, SCOPES_GIT_MAX_WORKERS=10. The hung repos use a slowfail sidecar that accepts the handshake, sleeps ~25s, then closes — an image-independent finite failure, so both images complete and the numbers are comparable (all three configs logged 12/12 clone failures). Preload runs synchronously in the gunicorn master before any worker forks, so time-to-serve-all-healthy is the whole boot sweep. 5.7x from concurrency alone; 7.1x with a tuned timeout.

Two honest caveats. The baseline image is 76f898ec, which predates PR2 (#923); PR2 did not touch sync serialization, so the gap is this PR's, but the baseline is not literally the merge-base. And app-tests/git-leak/test_boot.py is still not a gate — BOOT_TARGET_SECONDS defaults to 2000, and tightening it to the planned 120s @ 50 scopes would not discriminate: measured on this branch and on the merge-base, an all-healthy fleet boots in 4.3s and 4.9s respectively. Healthy clone throughput was never the problem; the hung tail is, which is what the head-to-head measures and what the prod telemetry says.

Why the phase-2 bound is not a scale risk. Most scopes in a Permit deployment share one policy repo, so at prod scale nearly all of them land in phase 2. That phase was separately profiled (.superpowers/sdd/boot-prod-report.md, a local sweep at ~3MB/branch across four SCOPES_REPO_CLONES_SHARDS values — note that path is gitignored, so the numbers are reproduced here rather than linked): the duplicate-scope pass costs ~1.6ms per scope and is flat in content size (it touches only ref/commit metadata — resolve_refish, one commit object — never a tree walk or blob read), holding sub-millisecond in a bare pygit2 benchmark out to 2000 branches and ~180MB repos. At ~2000 scopes that is ~3s, so phase 2 cannot be concurrency-bound and narrowing its bound from max(N, 32) to max(1, N) is immaterial. The genuinely expensive per-scope cost — make_bundle, ~71ms locally and ~350ms at prod file counts — is lazy, on GET /scopes/{id}/policy, and never runs during preload.

OPAL_SCOPES_SYNC_CONCURRENCY, which PR4 promised as an operator lever, does not exist — SCOPES_GIT_MAX_WORKERS is the single knob, deliberately, to avoid a second easily-misconfigured bound.

6. ⚠️ Two contract changes

GET /scopes/{scope_id}/policy:

Case	Before	After
Scope record missing, no default scope	unhandled 500	404
Scope record missing, default exists and serves	default scope's bundle	unchanged (see below)
Scope record missing, default exists but its clone is transiently broken	unhandled 500	503 + Retry-After: 5
Record present, clone invalid/vanished	default scope's bundle	503 + Retry-After: 5
Record present, raw OSError	unhandled 500	503 + Retry-After: 5
Record present, clone in progress	—	held up to SCOPES_POLICY_CLONE_WAIT_SECONDS (20s) and served if the clone lands; else 503 + Retry-After: 30 — see §7
Record present, branch genuinely unresolvable	default scope's bundle	409, non-retryable

A live tenant was briefly served another tenant's policy; the condition is transient by construction, so a retryable error is the honest answer. The mid-clone case is separated out because _clone() clones into the final path, so during a recovery re-clone the branch legitimately does not exist yet — reporting that as a permanent config error is the opposite of the truth. The two Retry-After values are constants, not escalating: escalation would need per-client retry state on a stateless endpoint.

Consumer note: opal-client wraps its fetch in a tenacity retry with no retry= predicate, so it retries every non-200 regardless — including the 409 it is told is non-retryable. Direct/third-party consumers are the ones that will act on the advice. Also: a scoped client that has never completed a fetch now stays NotReady rather than coming up on the default scope's bundle — correct, but visible to k8s readiness gates on upgrade.

Known, unchanged, and still wrong: a missing record still serves the default scope's bundle at 200 while the caller is authorized only for the requested scope. That is a cross-tenant hand-off and it is the same thing the 503 above exists to prevent. Left alone to keep this PR scoped; recorded as characterization (not endorsement) in scope_policy_fallback_test.py. It needs its own ticket — it is currently pointed at PER-15157, which this PR closes, so merging as-is orphans it. Both fixes (404, or gating the fallback on the caller being authorized for default) change behaviour for anyone relying on it today, which is why it is not being done here.

7. Server-side clone wait — so the client needs no change

GET /scopes/{id}/policy no longer answers 503 the instant it finds a clone that is still being populated. It holds the request for up to OPAL_SCOPES_POLICY_CLONE_WAIT_SECONDS (default 20s), re-checking once a second, and returns the bundle the moment the clone is usable. On expiry it falls through to the unchanged answer: 503 + Retry-After: 30, same ScopePolicyUnavailable event, with the hold added to its log line. The default-scope path (unknown scope id) waits on the same terms and keeps its own contract on expiry (Retry-After: 5).

Why. The 503 is correct but the PDP does not act on it: opal-client 0.9.6 ignores Retry-After, makes five attempts with random-exponential backoff capped at 10s (~20–40s of coverage), then goes quiet until the next pub/sub policy message or a reconnect. A clone that outlives those attempts leaves that PDP with no policy and nothing scheduled to fix it — and the update-all published when the clone completes names only the scope that was syncing, so siblings sharing the clone are never woken. Holding the request converts that gap into latency the client already tolerates: five client attempts against a 20s hold cover ~2 minutes of clone time, so short and medium re-clones (the download phase) produce no client-visible gap. This is why no opal-client change is required for the rollout.

What is bounded. The wait plus at most one more bundle attempt. Time queued behind other bundle builds on the shared executor is outside the deadline — that is what the cap below bounds. 0 or negative disables the wait; nan, inf and -inf are treated as disabled (they parse and start the process); anything above 55s is clamped (below the 60s ALB idle timeout, warned once per process), because a hold longer than that idle timeout is served as a 504 rather than a bundle.

Why it is safe at fleet scale. Polling is cheap (~0.23 ms per check); releasing is not. When the clone lands, every held request builds a full bundle on the loop's default executor — ≈min(32, cpu+4) threads with an unbounded queue, shared by every off-loop call in the process — whose measured throughput falls from ~52 bundles/s at 32 concurrent builds to ~18/s at 1000. Scope git ops do not share that pool (each gets a single-use executor bounded by a semaphore), so this is the only bound on concurrent bundle builds. Unbounded, a pod holding thousands of PDPs turns the hold into a queue that outlives the ALB idle timeout, and a rolling restart drains worse (uvicorn waits for in-flight requests; gunicorn SIGKILLs at 30s, dropping that worker's websockets). So OPAL_SCOPES_POLICY_CLONE_WAIT_MAX_INFLIGHT (default 64, per worker process; 0 = no cap) bounds how many requests one process may hold at once. The excess is shed with exactly the answer it would have got before the wait existed — an immediate 503 + Retry-After: 30 — so the cap is never worse than not waiting. Size it so cap ÷ achievable bundles-per-second fits inside 60s minus the hold. Note the arithmetic at fleet scale: 12 workers × 64 = 768 requests held fleet-wide, so a full-fleet herd is mostly shed to the old immediate 503 — the feature's value is the common case (one shard, a few hundred PDPs), not a total restart.

A client that has already disconnected stops being waited for: the wait checks is_disconnected() once per poll and releases the slot to a caller that is still listening (no event, no 503 log — the reply goes nowhere).

How it stays correct. Readiness is disk-derived (CloneNotPopulatedError = no remote-tracking refs yet), never the in-process in-flight marker — the clone runs in the leader while this route is served by any worker, so only disk gives every worker the same answer. The hold is an awaited sleep loop clamped to the remaining budget (NaN-safe): no thread between polls, no lock, no cache touched, cancellation-safe. Anything a retried attempt raises reaches the handlers the first attempt would (absent branch → 409, gutted object store → 503 + Retry-After: 5); only an unpopulated clone is waited for, and the client's base_hash is re-sent on every poll so a rescued request still gets its diff bundle.

New metrics.

metric	type	tags
opal_server.scopes.policy_clone_wait	count	`outcome:served
opal_server.scopes.policy_clone_wait_inflight	gauge	pid — requests this worker is holding right now, against the cap
opal_server.scopes.policy_clone_wait_seconds	gauge	`outcome:served

Follow-up (not in this PR): --timeout-graceful-shutdown 15 on the gunicorn line in scripts/start.sh would let uvicorn cancel held requests on shutdown instead of waiting on them; the cancelled accounting is already in place for it. Deliberately not here because it changes shutdown semantics image-wide, for every request type.

8. Per-source backoff on the periodic pass — dead repos stop being hammered

Nothing recorded a git failure: a source whose clone or fetch failed was re-attempted on the very next pass, and so was every duplicate scope sharing it (no local copy → straight to clone). On prod-us that is 19 fast-failing GitOps sources producing 4,579 clone attempts in three hours, plus two SYN-drop hosts each holding a slot for the full fetch timeout every pass. The in-flight skip only covers a source whose op is still running (the hung class); the faster pass this PR ships would multiply the fast-fail class.

Behaviour. Every awaited clone/fetch failure (GitError or TimeoutError, either path) puts its source into a per-process backoff: first delay OPAL_SCOPES_GIT_BACKOFF_BASE_SECONDS (default 10 s), doubling on every consecutive failure with no ceiling — 10 s, 20 s, 40 s, … minutes, hours, days. A repository that has been unreachable for a day is checked again in two, then four, and before long only at the next restart or an explicit refresh: a repo that keeps failing is, in all likelihood, dead. (OPAL_SCOPES_GIT_BACKOFF_MAX_SECONDS is an optional cap for operators who prefer to bound the staleness of a repo that comes back on its own; default 0 = none.) The first few doublings are shorter than a pass period and skip nothing — the schedule bites from roughly the fourth failure. Only pass-originated syncs honour it — the periodic pass and the boot preload, in both phases; the check runs both before lock_source (a skipped source costs no lock and no executor slot, so it can never be refused by the zombie cap) and again under it (so N duplicates of a source arriving together cost one attempt, not min(N, MAX_WORKERS), regardless of how short the delay is). An explicit POST /scopes/{id}/refresh, POST /scopes/refresh or PUT /scopes attempts the source immediately, and any successful clone or fetch clears the entry. Backpressure from the zombie cap is not recorded (it says nothing about the remote). No jitter: at dozens to a few hundred sources per pod, with the pass bounded by SCOPES_GIT_MAX_WORKERS, lockstep retries are not a concern.

Trade-off stated plainly. Because the delay is unbounded, a GitOps repo that recovers on the git side without anyone touching its scope in Permit stays untried for as long as its delay has grown to — after a day of failures ~2 days, after a week ~2 weeks — until a pod restart (state is in-memory) or an explicit refresh. That is the intended behaviour: the customer's policy was already stale, and a fixed repo normally comes with a Permit-side change that issues the bypassing refresh.

Boot. With a periodic pass configured (POLICY_REFRESH_INTERVAL > 0, Permit's setting) the forked leader inherits what the pre-fork preload recorded, so it does not re-hammer repos that already failed at boot. Without one (the OSS default) the boot sync is the only pass-originated sync, so it drops the inherited entries and attempts every source once — duplicates of a source that fails in that pass are still collapsed to one attempt.

Kill switch / state. SCOPES_GIT_BACKOFF_BASE_SECONDS ≤ 0 (or nan/inf) disables the feature: nothing recorded, nothing skipped, gauge reads 0. In-memory, per process, mutated only on the event loop; the purge/delete paths drop it with the source's other caches. Keys are read at import: flipping needs a pod restart, which also clears the state — a clean revert.

Metrics / logs. opal_server.scopes.git_op_skipped{reason:backoff} (counter, per skipped pass), opal_server.scopes.sources_in_backoff{pid} (gauge of sources currently being skipped, emitted on every transition and once per pass); WARNING when a source enters backoff, again when its delay first exceeds a day (from then on effectively abandoned), and when a configured cap is reached (Backing off {url} for {delay}s after {n} consecutive failures: {err} — carries the redacted URL, which is how an operator finds which repo); DEBUG for other recorded failures and every skip; INFO on recovery. Also fixes a gap: fetch GitError is now counted in git_op_failures{op:fetch,reason:git_error} — that series appears from zero on deploy; it is not a regression.

Expected effect on prod-us: the 19 fast-fail sources go from one attempt per pass each to a handful per day within the first hour, then ~one per day, then rarer; the two 132 s hosts likewise; the timer pass becomes ~1 s in steady state and the sync_scope p90 falls to seconds. Caveat: attempts originating from explicit POST /scopes/{id}/refresh (which policy-sync issues) bypass the backoff by design and are not reduced.

Runbook. Symptom: sources_in_backoff > 0 or git_op_skipped{reason:backoff} non-zero. Identify the repos: search opal-server logs for Backing off (WARNING). After fixing the remote, POST /scopes/{scope_id}/refresh (or /scopes/refresh) retries immediately; a pod restart also resets every delay. Disable: OPAL_SCOPES_GIT_BACKOFF_BASE_SECONDS=0 + restart. Note for the existing sync_scope p90 alarm: skipped scopes still emit a ~0 ms span, which dilutes that percentile — the counter-based monitors are the intended replacement.

New config keys (opal-server, additive)

Env var	Default	Purpose
OPAL_SCOPES_GIT_FETCH_TIMEOUT	120.0	Soft timeout for one scope clone/fetch. 0 = no limit
OPAL_SCOPES_GIT_MAX_WORKERS	10	Bounds live git ops; bounds both sync phases
OPAL_SCOPES_GIT_MAX_ZOMBIES	40	Global ceiling on in-flight (live + lingering) ops. At the ceiling, new ops are refused for every scope
OPAL_SCOPES_GIT_PRELOAD_DRAIN_TIMEOUT	10.0	Pre-fork drain window for in-flight git ops
OPAL_SCOPES_PURGE_CHANNEL	__opal_scope_purge__	Worker-to-worker cache-purge channel
OPAL_SCOPES_STORE_READ_TIMEOUT	10.0	Bounds the sibling-check store read taken under lock_source
OPAL_SCOPES_POLICY_CLONE_WAIT_SECONDS	20.0	How long GET /scopes/{id}/policy holds a request while the scope's clone is being populated before answering 503. 0/negative/non-finite = no wait; clamped to 55s (§7)
OPAL_SCOPES_POLICY_CLONE_WAIT_MAX_INFLIGHT	64	Per-worker cap on concurrently held requests; excess get the immediate 503. 0 = no cap (§7)
OPAL_SCOPES_GIT_BACKOFF_BASE_SECONDS	10.0	First delay of the per-source doubling backoff the periodic pass applies to a source that keeps failing (uncapped by default); explicit refresh/PUT bypass it. 0/negative/non-finite = feature disabled (§8)
OPAL_SCOPES_GIT_BACKOFF_MAX_SECONDS	0.0	Optional cap on that doubling; 0 = none. Floored at the base when set (§8)

Descriptions are pinned verbatim against documentation/docs/getting-started/configuration.mdx by config_docs_drift_test.py, in both directions.

One default is not behaviour-preserving. The scopes path previously had no fetch cap at all (POLICY_REPO_CLONE_TIMEOUT is wired only to the legacy non-scopes path), so SCOPES_GIT_FETCH_TIMEOUT=120.0 means a clone that legitimately takes longer now times out on its first pass. It self-heals — the abandoned thread finishes the clone on disk, the in-flight guard skips one cycle, and the pass after finds a valid repo — at the cost of one wasted clone and a scope not serving for a refresh interval or two. Operators with very large repos should raise it before upgrading.

Worst-case boot delay at the shipped defaults. A hung op holds its concurrency slot for the full timeout, and preload_scopes runs synchronously in the gunicorn master before any worker forks, so a burst of unreachable sources delays boot by roughly ceil(hung / SCOPES_GIT_MAX_WORKERS) x SCOPES_GIT_FETCH_TIMEOUT — 40 hung sources at the defaults is ~480s of pre-fork time serving nothing. SCOPES_GIT_FETCH_TIMEOUT is the knob; the head-to-head above used a 25s failure, not a true black hole.

Invariants (enforced and tested)

1. repo_locks entries are popped only while holding that source's lock, and every path that abandons a source drains its entry — except a path that finds a live sibling, which leaves it alone. One deliberate exception: reset_caches clears the whole dict without holding anything, in the gunicorn master after asyncio.run has returned and before the fork, where it is single-threaded and there is nothing to serialize against.
2. forget_repo / rmtree never free a cached, shared handle while a git op is in flight for that source (all call sites guarded in-process). Two sites free a handle they opened themselves and share with nobody — _get_valid_repo's probe and _get_current_branch_head's fresh handle — and are deliberately unguarded; opening fresh is precisely how the serving path avoids the shared-handle race.
3. The leader is the only mutator on sync paths. A delete additionally removes on the worker that served it — see §3.

⚠️ Consumer surface

python_requires moves from >=3.9 to >=3.9,<3.13 in opal-server, opal-common and opal-client. On Python 3.13+, pip install of any of them silently resolves to the last release published without the cap and reports success — pip filters candidates by Requires-Python and backtracks to the newest compatible one. Every published opal-client is currently uncapped, so a downstream on 3.13 running pip install -U opal-client gets 0.9.6 and stays there, with nothing surfacing until a fix appears not to have arrived. Only an explicit pin (opal-client==<new>) fails outright. Silent downgrade is the worse of the two outcomes and is the one to warn users about. Docker images are python:3.10 and unaffected.

On opal-server the cap is not a choice: it depends on pygit2>=1.14.1,<1.15, and that range has no 3.13 wheels at all (the first is 1.16.0). Lifting it there means bumping pygit2 and re-verifying _DaemonThreadPoolExecutor, which mirrors CPython concurrent.futures internals — out of scope here, and worth its own ticket.

opal-common and opal-client do not depend on pygit2 and are capped to match. That is deliberate rather than incidental: CI tests 3.9–3.12 only and the classifiers already stopped at 3.12, so lifting it would advertise support nothing exercises.

Also on the consumer surface: external RPC peers can no longer subscribe to ALL_TOPICS. The purge channel's authorization gate rejects it, because notify() fans every topic into the ALL_TOPICS bucket, so an ALL_TOPICS subscriber would otherwise receive purge traffic. No in-repo opal-client uses it (only the broadcaster, which passes channel=None and is unaffected), but third-party pub/sub consumers that do will now get Unauthorized.

No OPAL_* key renamed or removed.

Verification

• opal-server unit: 239 passed. opal-common: 93 passed. pre-commit clean.
• app-tests/git-leak: of 22 rows, 18 pass outright. Expected red, by design: the three orphan-sweep gates (PER-15612). test_server_recovers_after_postgres_bounce passes (a)–(c) with (d) intermittently red at the same rate on the merge-base — a production property of the reader, owned by fix(opal-server): freeze publishes during broadcaster backbone gaps to keep the fleet consistent #933.
• New behaviour is mutation-verified: each pin is checked to fail under the mutation named in its own docstring, and to fail only that.

[linear-code]

PER-15157

PER-15158

[netlify]

✅ Deploy Preview for opal-docs ready!

Name	Link
🔨 Latest commit	fcf5840
🔍 Latest deploy log	https://app.netlify.com/projects/opal-docs/deploys/6a8424c82de8be0008b6fef2
😎 Deploy Preview	https://deploy-preview-924--opal-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[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-8 addressed — 15/15 threads, and the PR description is rewritten

f6c39533 the cut · c1c09753 d359ffb2 254f987a d95e0459 c6899e11 fixes · 3f68bcb6 e2256031 claims. Per-finding detail is in each thread.

Six of the fifteen were resolved by deletion, not by fixing

The disk-reclaim cut took the deferred-purge retry (and with it round 8's HIGH-2, which you under-called — there was a second, more reachable entrance via shutdown), SCOPES_DEFERRED_PURGE_TIMEOUT and both of its LOWs, the in-flight deferral branch, and the 157-line / cc-17 complexity finding. purge.py is 232 lines lighter.

Two I pushed back on, with evidence rather than opinion

HIGH-1's severity. Running your race against origin/master's own code: master produces the same orphan, unconditionally and permanently, and has no git_op_in_flight guard at all. The floor is master's exposure minus a guard master lacked. But you were right about what mattered — the code claimed an invariant it has never held, and purge.py:6 contradicted itself ten lines later. That claim is gone from all three sites.

The zombie cap. I proposed a headroom fix and withdrew it: with more stuck sources than the ceiling, the healthy op is refused anyway. Nor could I reproduce the starvation — the bed's black hole produces clones that time out but whose threads then finish (60 black-holed sources → 60 timeouts, 0 refusals, and a fresh scope served 200 afterwards). So that one is a documentation fix, and SCOPES_GIT_MAX_WORKERS no longer tells operators "capacity is never starved by hung remotes".

Three bugs your findings led to that were outside their own lines

• ScopeRepository.all() aborts on a mid-scan delete. A scope deleted between the key scan and the read comes back None, and Scope.parse_raw(None) killed the whole scan. One concurrently-deleted scope was silently aborting an entire sync_scopes pass. Pre-existing, found chasing your fail-open finding.
• The floor deleted a live re-created scope's clone. It excluded the deleted scope_id from its sibling check, copying master — which blinds it to a re-create on the same source under the same id.
• The I4 drain churned a live sibling's lock. My round-6 finally was applied unconditionally; the bed's test_shared_repo_survives_sibling_scope_delete caught it.

None of the three was caught by the unit suite. Two needed the docker bed.

The description is rewritten

It had drifted several rounds out of date and reviewers were reading it as the spec. It now describes what is actually here, including the parts it never mentioned: the fork-safety workstream, the python_requires <3.13 cap reaching opal-client and opal-common (which needs an owner to lift), the second contract change, that stock opal-client retries the 409 it is told not to retry, the cold-start readiness change, and that the boot-time fix ships unmeasured because BOOT_TARGET_SECONDS is still 2000 and the plan to tighten it belonged to the now-closed PR4.

It also states plainly what nothing reclaims: a DELETE's dir on non-serving pods when the broadcast is lost, a REPOINT's old dir always, and a dir whose source_id is unknowable. All PER-15612.

Verification

opal-server unit	222 passed (218 at round 8)
opal-common unit	93 passed
app-tests/git-leak	test_transitions.py + test_leak.py 11 passed
bed matrix	22 rows, 18 pass outright; 3 orphan-sweep gates red by design
pre-commit	clean

Every new pin is mutation-verified against the mutation named in its own docstring. One was vacuous as first written — the confinement test survived removing the comparison, because the real protection is that the body operates on the derived path — and its docstring now names the mutation that actually kills it.

[zeevmoney]

Round 9 — c68a2cb8 (+5923 / −544, 36 files)

239 passed (opal-server), 93 (opal-common) — both reproduced. All 15 round-8 threads addressed across 13 commits, and the description rewritten from the code.

No CRITICAL and no HIGH survived. That is a genuine change from rounds 2–8, each of which produced at least one HIGH. The cut removed the machinery that kept generating them, and what remains is largely pinned. What is left is concentrated in one place: guards and prose that this round added or invalidated, and did not test or update.

Round-8 follow-through — verified by mutation

Every round-8 finding is closed, and each fix fails a named test when reverted:

Fix	Mutation	Fails
254f987a ScopeRepository.all() mid-scan delete	continue → fall through	test_scope_repository_all_skips_a_key_deleted_mid_scan
c1c09753 floor vs a re-created scope	re-add the scope_id exclusion	test_local_floor_keeps_the_clone_of_a_re_created_scope
d359ffb2 I4 drain vs a live sibling (leader side)	drain unconditionally	test_leader_keeps_disk_when_live_sibling_shares_source, test_live_sibling_keeps_its_repo_lock_entry
_local_purges drain	remove the drain	test_service_stop_drains_the_floor, test_shutdown_drains_the_routers_scopes_service
8427febe off-loop bundle build	direct call	test_default_bundle_build_does_not_block_the_event_loop

The false invariant is gone from all three code sites, the floor's rmtree now routes through confined_clone_path, and the README matrix is complete (22 rows, counts correct). The security surface holds: disabling _SOURCE_ID_RE reddens 11 tests including the traversal cases, and both halves of the ALL_TOPICS/purge-channel gate are independently pinned. The cut itself is clean — DEFERRED_PURGE returns nothing tree-wide, no orphaned key, helper, or dead branch.

Two corrections to things I asserted

I would rather flag these than have them stand:

• "239 passed" is correct, and my earlier reading of it was wrong. I measured 238 and was preparing to report the Verification line as off by one. The difference is the pytest target: packages/opal-server gives 239, packages/opal-server/opal_server/tests gives 238, and the extra test is opal_server/data/tests/test_data_update_publisher.py::test_topic_combos, which lives outside that subtree. Nothing is skipped or flag-gated. The only residual nit is that the Verification line does not name the command. Not a finding.
• I initially concluded no vacuous pin remained beyond the one you fixed. That was wrong — there are three, and they are the first MEDIUM below. My method was the limitation: I AST-scanned for assertion-free or trivially-true tests, which cannot find a guard that simply has no test pointing at it. Only line-level mutation surfaces that class.

What is new — 6 MEDIUM, 3 LOW inline

The two worth acting on first:

1. Three guards added this round have no test at all, including service.py:366 — the floor's copy of d359ffb2's fix, whose leader-side twin at purge.py:399 is pinned. Same fix, two sites, one covered. And test_floor_refuses_a_path_that_is_not_the_derived_one passes with the guard it is named for deleted.
2. A transient clone failure on the default-bundle path returns a permanent 404 with no Retry-After, where the identical exception set yields 503 + Retry-After: 5 on the primary path. Against master this is 500 → 404: an improvement for the case its comment describes, a regression in retry semantics for the transient case, on the endpoint §6 tells third-party consumers to act on.

Then: the drift guard is bypassable four ways (one hides a key from it entirely, three let a tracked key's docs say something false); SCOPES_STORE_READ_TIMEOUT's description now states the opposite of what the code does and is pinned verbatim into the public reference; §3's "exactly one path" is false; and the bed's I1 exemption rests on a condition its own fixture makes impossible.

Smaller description mismatches (not inline — no diff line to anchor)

The rewrite says "If anything below disagrees with the code, the code is right and this is a bug", so:

• §1's "Enumerated: 10 call sites, all guarded in-process" contradicts Invariant 2 two sections later, which names two deliberately unguarded sites (git_fetcher.py:748, :859-862). Separately, git_fetcher.py:617/619 and :669 have no git_op_in_flight check at all — they are serialized by lock_source and by being the in-flight op themselves. Only git_fetcher.py:954, service.py:368 and purge.py:126 actually consult the marker.
• Invariant 3's "the leader is the only mutator on sync paths" is false for the gunicorn master: preload_scopes() runs asyncio.run(service.sync_scopes(...)) (task.py:169-183) from scripts/gunicorn_conf.py:16 with no leader lock, and task.py:211 then asserts sync "is leader-only" to justify a conclusion about those very caches. Benign — it is single-threaded pre-fork, the same reason reset_caches is safe there — but the invariant as stated is not accurate.
• app-tests/git-leak/README.md:69 still says "PASSES here (nothing purges on master)". PR2 (6498da1e) is merged to master, so that framing is stale; same at :121-124.
• Optional: the three orphan-sweep gates are correctly kept — they are pre-existing on master (this PR's diff to them is docstring-only) and pytest.ini's testpaths = packages means they never run by default, so they cost nothing. But their red-by-design status lives only in README prose. @pytest.mark.xfail(reason="orphan sweep deferred to PER-15612", strict=True) would make it machine-checked and flip loudly when PER-15612 lands.

On round 8's HIGH-1, since you pushed back with evidence

You were right and I was wrong on severity. Running the race against master does produce the same orphan, unconditionally, with no in-flight guard at all — the floor is master's exposure minus a guard master lacked, and rating it HIGH against an ideal rather than the merge base was my error. What survived is the part you agreed with: the code asserted an invariant it never held. That is now gone from all three sites and replaced with an honest statement of what goes unreclaimed — including the correction that the leader always has a reader, so it is non-leader workers that can be deaf. Better than what I asked for.

Process disclosure

Three reviewers were dispatched; two delivered and their findings are folded in above, each independently reproduced by me before posting (the 404 probe, the unpinned-guard mutations with the purge.py:399 control, the fixture/compose trace behind the I1 finding). One never returned a report despite four requests, so I covered its scope myself with first-hand mutation results rather than leave it unreviewed. Flagging it so you can weigh the coverage accordingly.

Requesting changes on the untested guards and the 404 semantics. The rest are worth fixing and none of them blocks.

[zeevmoney]

Approving — every round-9 concern validated, nothing above MEDIUM

Superseding my earlier CHANGES_REQUESTED on this round. That verdict was inconsistent with the rubric I have been applying all nine rounds — nothing exceeded MEDIUM, so it should have been a comment, and I carried the verdict forward from prior rounds instead of re-deriving it. Correcting that here.

Every concern re-validated against the code

Concern	Severity	Validation
Bed I1 exemption possibly masking its own violation	MEDIUM	Resolved — it is not masking anything. The scope store is Redis (OPAL_REDIS_URL), while bounce_postgres stops only Postgres, the broadcaster backbone — so the floor's sibling check keeps answering and it never fails open. The test also hangs no remote, so git_op_in_flight cannot trigger the skip branch. It cannot orphan a dir. The exemption is dead weight with a false justification, not cover for a failure.
Three guards with no test	MEDIUM	Guards are present and correct — purge.py:399, the pinned twin of the unpinned service.py:366, is the same logic and passes. Only coverage is missing.
Transient clone failure → 404	MEDIUM	Narrow (record missing and a default scope exists and its clone is transiently broken); 500 → 404 against master is a net improvement; opal-client retries every non-200 regardless.
Drift guard bypassable four ways	MEDIUM	Test efficacy only; no runtime behaviour depends on it.
SCOPES_STORE_READ_TIMEOUT prose contradicts the code	MEDIUM	Documentation only.
§3's "exactly one path removes a clone dir"	MEDIUM	Documentation only.
Shape-check claim / python_requires / garbled docstring	LOW	No supported interpreter reaches the first; the others are doc and packaging.

No CRITICAL and no HIGH. The security surface was attacked directly and held: disabling _SOURCE_ID_RE reddens 11 tests including every traversal case, both halves of the purge-channel authorization gate are independently pinned, no wire-supplied clone_path reaches an rmtree, and this PR narrows the cross-tenant surface by removing master's second _generate_default_scope_bundle caller. All 15 round-8 findings are closed and mutation-pinned. 239 + 93 passing, the cut left zero dangling references.

Two things to handle at merge, neither blocking

1. PER-15157 is closed by this PR, and the known cross-tenant fallback is still pointed at it. The missing-record → 200 with the default scope's bundle is pre-existing, deliberately out of scope, and honestly recorded as characterization rather than endorsement — that disposition is right. But it is a genuinely HIGH-severity issue in the codebase, and merging as-is orphans its tracking, as the description itself notes. Please give it its own ticket before or at merge. Flagging it plainly so it is not lost: it is not a finding against this PR, and it should not disappear either.
2. The nine open threads from this round are worth landing as follow-ups. The three untested guards are the cheapest and most valuable — the PR's own standard is that every pin fails the mutation named in its docstring, and d359ffb2 currently meets that on the leader side only.

On the review itself

Round 8's HIGH-1 severity was mine to get wrong, and your master-comparison was the right correction. In this round I also filed "239 passed" as an off-by-one and was wrong again — the number is correct for pytest packages/opal-server; I had used a narrower target. Both are noted in the round-9 body.

Nine rounds in, the trend is what convergence looks like: CRITICAL/HIGH through round 8, nothing above MEDIUM now, and the remaining items are tests and prose rather than behaviour. Good work on the cut — deferring the distributed disk reclaim to PER-15612 is what made this reviewable.