fix(opal-server): memory leak — purge GitPolicyFetcher caches on scope delete + webhook task cleanup (PR2)

[Zivxx]

PR2 — Memory leak fix (GitPolicyFetcher caches + webhook task list)

Linear: PER-15156

Originally stacked on PR1 (PER-15155); PR1 has since merged (#922) and master is merged into this branch, so the diff now targets master directly. PR1's git-leak bed is the regression gate this PR turns green.

What

Stops OPAL-server memory growth by releasing the leaked pygit2.Repository handle when a scope is deleted, and fixes the webhook task-list cleanup bug. Serial only — no concurrency introduced (the delete/fetch race is deferred to PR4).

Changes

• GitPolicyFetcher.forget_repo(path) (git_fetcher.py) — pops repos[path] and calls Repository.free() when available (guarded; falls back to GC), releasing OS fds + mmapped pack indexes.
• ScopesService.delete_scope (scopes/service.py) — captures the deleted scope's source before scanning siblings (fixes a latent loop-variable shadowing bug that computed the clone path from the wrong scope), then removes the clone dir + forget_repo + pops repos_last_fetched and repo_locks when no sibling shares the source.
• DELETE /scopes/{scope_id} route (scopes/api.py, server.py) — now actually calls ScopesService.delete_scope (92c8515). Previously the route deleted the scope straight from the ScopeRepository, so the purge above was dead code in a running server — unit tests passed (they call the service directly) while the deployed DELETE path never purged anything. init_scope_router receives the service via the factory; deleting a missing scope remains a 204 no-op.
• BasePolicyWatcherTask._on_webhook (policy/watcher/task.py) — replaces remove-while-iterating (which skipped elements, leaking finished tasks) with a list rebuild.

Review-driven fixes (beyond the original plan)

• The clone-deletion gate originally used url_shared, but clones are keyed strictly per-source_id (base_dir/git_sources/<source_id>). With OPAL_SCOPES_REPO_CLONES_SHARDS > 1, same-url/different-branch scopes resolve to different clone dirs — so url_shared would skip deleting a clone that no sibling actually uses. Both gates now use source_id_shared, with a dedicated SHARDS>1 test.
• An earlier revision of this PR deferred repo_locks purging ("lock-identity hazard"). The merged git-leak bed's churn gate asserts all three caches drain, and an un-popped asyncio.Lock per deleted repo is still an unbounded leak — so the purge now pops repo_locks[source_id] too. The delete path already removes the clone dir out from under any in-flight sync, so the lock pop does not add a new race class; true delete/fetch serialization stays deferred to PR4.

Tests

Unit tests (TDD): forget_repo_test.py (3), delete_scope_cache_purge_test.py (3, incl. the shards>1 divergence case, now seeding/asserting repo_locks incl. the sibling-survives case), webhook_task_cleanup_test.py (1), delete_scope_route_test.py (2, new) — proves DELETE /scopes/{id} drains the caches through the real route and that delete-of-missing stays 204.

Verification (2026-07-12, commit 92c8515)

• opal-server unit suite: 81 passed
• git-leak bed (app-tests/git-leak/, real docker stack, 20 scopes): test_churn_releases_caches ✅ test_shared_repo_survives_sibling_scope_delete ✅ test_repeat_sync_rss_stays_bounded ✅
• Still red by design: test_scope_repoint_releases_old_repo_cache (purge on scope update/repoint — out of this PR's scope, currently unowned) and test_offline_repo_does_not_block_healthy_scopes (PR3's gate).
• ⚠️ Correction: an earlier revision of this description claimed the churn gate had been verified green pre-review. That claim was wrong — the DELETE route bypassed the service in every committed revision, so the gate could not have passed as described. It was caught by re-running the merged (hardened) bed; see the run-by-run breakdown in the review notes.
• pre-commit run --all-files (pinned black/isort/docformatter): clean; zero opal_common/opal_client changes; no config key added.

🤖 Generated with Claude Code

---

Second wave (2026-07-15, commits 36b020f5..bf53477a): lifecycle test matrix + 3 more fixes

Follow-up to the review: we enumerated the scope/clone/cache lifecycle as a state machine and added tests for the transitions and start states the end-state gates never drove (both review findings lived there). The new tests then caught three real bugs, each confirmed by a failing test before its fix:

1. Failed fetch suppressed the next forced refresh — repos_last_fetched was stamped before the fetch, so a raising fetch still looked fresh to _was_fetched_after(), silently skipping the webhook-requested retry. Now stamps the fetch start time, written only on success (6b443515).
2. Invalid-clone recovery looped forever on a warm handle cache — recovery never dropped the cached pygit2.Repository and never cached the fresh clone's handle, so the stale handle re-invalidated every fresh clone (infinite re-clone loop, one leaked handle per cycle); _clone() also now clears partial dirs a failed clone leaves behind, which previously wedged every retry (138deeb4).
3. Corrupted-but-discoverable clones never healed — validation only checked "opens + remote matches"; a gutted object store passed, fetch negotiated "up to date" against intact refs, and bundle serving 500'd forever. The warm handle's open mmaps hide the corruption, so detection probes disk truth via a short-lived fresh Repository (freed in finally) and routes to recovery (8ccd3ace).

New test infrastructure (app-tests/git-leak/)

• Invariant checker (invariants.py) at every test's teardown: no orphan clone dirs, cache keys consistent with disk + live scopes, worker pid-set stable (a crashed worker resets the caches and could previously fake a clean drain), liveness.
• 14 new bed tests: green guards (delete/recreate storm, seeded randomized churn, delete-vs-hung-fetch no-crash, warm boot, corruption recovery, upstream force-push / branch-delete characterizations) and 7 documented red gates whose docstrings name what flips them: fetch timeout (bounded DELETE, boot-under-unreachable-remotes), update-path/broadcast purge (repoint-during-fetch, multi-worker drain), orphan sweep (orphan dir, redis-wipe boot, shard-reconfig orphan half). The gate matrix in the bed's README has the full map.
• The internal stats endpoint (off by default) now reports per-worker pid + cache keys via atomic snapshots (fixes a dictionary changed size during iteration 500 under concurrent load and self-inconsistent count/keys responses).

Verification (commit bf53477a)

• opal-server unit suite: 96 passed
• Full git-leak bed (docker, 20 scopes): 12 passed; 9 failed = exactly the documented red gates (the 2 pre-existing + the 7 new), zero unexpected failures, zero cross-test invariant violations.
• Also confirmed & documented in code comments for the follow-up PR: a DELETE racing its scope's in-flight sync loses the purge (found by the randomized driver, deterministically replayable); the recovery free() vs unlocked bundle-read race window.

Round-3 follow-ups (commits bd6c86d5..bf72ca5f)

All 5 non-blocking round-3 review comments addressed: the webhook failure log now reuses the shared redact_url_in_text helper, the delete purge runs in a finally (an ambiguous store outcome can no longer orphan it), delete_scope is back under the complexity limit via _purge_source_cache_if_unshared, sync_scopes re-checks scope liveness right before each queued sync, and the clone-vanish → default-bundle fallback is regression-tested.

Known limitation (until PR3)

A 204 DELETE is not a durable disk-cleanup guarantee under concurrent sync: a sync that loaded the scope before the delete and reaches it mid-sync can still re-clone the dead scope's repo and re-populate the fetcher caches (bounded, same-tenant, never mis-served — GET /{scope_id}/policy 404s the dead scope). The liveness re-check added here shrinks the window to the sync itself; the full fix (leader-routed purge + scope-liveness check before clone) lands in PR3. Relatedly, a live scope whose clone dir transiently vanishes mid-request is briefly served the default scope's bundle rather than a retryable error; switching that to a 503 is also deferred to PR3.

[linear-code]

PER-15156

[zeevmoney]

Review notes (planned with the opal-development skill)

Faithful to the plan, and the source_id_shared gate (instead of the plan's url_shared) is a genuine improvement: with SCOPES_REPO_CLONES_SHARDS > 1, same-URL/different-branch siblings get different source_ids and therefore different clone dirs, so gating clone deletion on the URL would have skipped deleting the deleted scope's own clone — leaking exactly what this PR fixes. The added shards=4 test covers it, and the loop-variable shadowing bug is fixed by capturing the deleted source before the sibling scan. repo_locks correctly left untouched.

Three small notes (none blocking):

1. Its only effective gate in PR1 (test(opal-server): git leak/resilience test environment (PR1) #922) is test_churn_releases_caches. The sibling test_repeat_sync_does_not_grow is non-gating — caches are source_id-keyed, so a re-sync of identical scopes can't grow the counts for any implementation; it's fine as a loose RSS guard but shouldn't be read as proof of this fix.
2. forget_repo's except / logger.debug GC-fallback branch is untested — i.e. the path where Repository.free() itself raises. A one-line test (a fake repo whose free() raises, asserting the entry is still popped and no exception escapes) would close it.
3. Tracking note: deferring the repo_locks purge is the right call (the lock-identity hazard is real), but it does leave a tiny, bounded residual leak — one asyncio.Lock per distinct source ever seen. Worth keeping the structural-cleanup follow-up on the board so it isn't forgotten.

Depends on #922 for its end-to-end gate; suggested merge order is #922 → this.

[netlify]

✅ Deploy Preview for opal-docs canceled.

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

[zeevmoney]

Review summary — 3-agent consolidation (python-pro · fastapi-pro · security-auditor)

Reviewed at 8d5cfdf. The two genuinely-fixed bugs — the webhook remove-while-iterating cleanup and the old loop-variable shadowing in delete_scope — are real and correctly fixed (confirmed by reverting each hunk against the new tests). But the core deliverable (purge GitPolicyFetcher caches on scope delete) has correctness gaps that block approval.

Requesting changes — highest severity is CRITICAL.

Blocking

• CRITICAL — scopes/service.py — delete_scope mutates GitPolicyFetcher's lock-guarded shared state (rmtree + forget_repo/Repository.free() + cache pops) without acquiring repo_locks[source_id], racing an in-flight run_in_executor git fetch → native pygit2.Repository.free() use-after-free / clone corruption, plus a lock-identity hazard from popping repo_locks. The "serial only — no concurrency introduced" framing does not hold: run_sync is asyncio.get_event_loop().run_in_executor, so the fetch runs on a thread while the loop is free to run the DELETE.
• HIGH — scopes/service.py / scopes/api.py — the purge is process-local. In the default multi-worker topology it only affects the worker serving the DELETE, so the leader's handles (the bulk of the leak) persist until restart. Other scope mutations broadcast to the leader over pubsub; this one doesn't.

Should-fix (MEDIUM)

• service.py:191 — rmtree(ignore_errors=True) silently orphans deleted tenants' policy clones on any fs failure (newly live via the route wiring).
• git_fetcher.py:389 — forget_repo swallows free() exceptions at DEBUG with no context.
• policy/watcher/task.py:34 — finished webhook tasks dropped without retrieving exception().
• tests — no coverage for free() raising or for the delete-vs-fetch race.

Minor (LOW)

• service.py:166 — the deleted scope is cast to GitPolicyScopeSource unconditionally while siblings are isinstance-guarded; currently unreachable given the single-member schema, but inconsistent and would raise a confusing AttributeError rather than fail fast.
• service.py:187 — the new skip log dropped the sharing scope id that the old message included (minor diagnostic regression).

Per-finding detail is in the inline comments. Review consolidated from three specialist agents; every claim was re-verified against the code.

🤖 Reviewed with Claude Code

[zeevmoney]

Re-review @ bf53477a — 4-reviewer consolidation (python-pro · fastapi-pro · security-auditor · opal-domain)

Re-reviewed after the rework (10 commits since the last review at 8d5cfdf). All 8 prior findings are genuinely resolved or honestly re-scoped — verified in code, not just from commit messages:

• prior CRITICAL (delete-vs-fetch race) → fixed by the new lock_source (serializes fetch vs delete; the post-acquire is lock re-check closes the lock-identity hazard).
• prior HIGH (multi-worker) → documented as process-local best-effort deferred to PR3 (but see MEDIUM below — the framing undersells it).
• prior M1/M2/M3/L1/L2 → all fixed (warning+context on free(); rmtree OSError logged; webhook exceptions retrieved + gathered in stop(); non-git guard; sharing-scope-id restored) and now unit-tested.

However, the rework introduces new correctness issues that block approval.

Requesting changes — highest severity is CRITICAL.

Blocking

• CRITICAL scopes/api.py — the make_bundle / GET /scopes/{id}/policy path (the client-polled hot path) reads the cached pygit2 handle on a run_in_executor thread without lock_source, so the PR's new forget_repo→Repository.free() (from a concurrent DELETE / self-heal on the same worker) can free it mid-resolve_refish → use-after-free / worker crash. .free() is new in this PR, so the fix introduces this hazard on a path it doesn't cover.
• HIGH scopes/service.py — the sibling-source check runs before lock_source and isn't re-checked under it; two concurrent DELETEs of scopes sharing a source_id both skip the purge → the exact clone+handle leak this PR exists to close, reintroduced (found independently by all three general reviewers; untested).

Should-fix (MEDIUM)

• service.py:213 — delete does off-leader, cross-process rmtree on the shared tree, breaking the "only the leader mutates the working tree" invariant that put_scope/refresh_scope honor via pub/sub; the PR3 deferral note frames this as only a leader-cache leak.
• git_fetcher.py:235 & :251 — the rmtree(ignore_errors=True) silent-orphan anti-pattern just fixed in delete_scope is reintroduced in the invalid-repo self-heal and _clone pre-clean.
• git_fetcher.py:295 — the gutted-object-store probe opens a fresh Repository synchronously on the event loop every sync cycle, per scope (N per shared source_id), blocking the loop.
• tests — _get_valid_repo's healthy branches, the concurrent-sibling-delete race, and the make_bundle race are all uncovered.

Minor (LOW)

• policy/watcher/task.py:38 — logger.error(...{t.exception()!r}...) isn't URL-redacted (credentialed remote URL can leak via the classic watcher path).
• git_fetcher.py:270 — corruption-triggered reclone never refreshes repos_last_fetched (stale timestamp → one redundant forced fetch).
• Also noted (not posted inline): lock_source setdefault(sid, asyncio.Lock()) eagerly allocates a throwaway Lock on the hot path; debug_stats return type narrowed to bare Dict despite mixed int/list values; lock_source's while True has a theoretical (impractical) starvation under adversarial delete/recreate churn; the /internal stats payload now includes absolute clone paths + source_id keys (behind DEBUG_INTERNAL_STATS + auth, but amplifies the pre-existing missing-AUTH_PUBLIC_KEY bypass); the probe's "unlink doesn't invalidate mmap" comment is POSIX-specific.

Per-finding detail is in the inline comments. Consolidated from four specialist reviewers (including an OPAL-domain pass using the repo's contributor skill); every claim re-verified against the code.

🤖 Reviewed with Claude Code

[zeevmoney]

Round-3 review @ a295717d — 4-reviewer consolidation (python-pro · fastapi-pro · security-auditor · opal-domain)

Approving. The 10 commits since round 2 resolve all 8 round-2 findings — verified against code (python-pro mutation-tested the purge/TOCTOU fixes: the new tests fail on the old code, pass on the new), not just commit messages:

• CRITICAL (make_bundle UAF) → fixed: _get_current_branch_head opens a fresh short-lived Repository per call and frees it, so the client-polled bundle path never touches the cached handle; audited — the cached handle is now only ever read/written/freed under lock_source.
• HIGH (sibling-delete TOCTOU) → fixed: record-delete + sibling re-check + purge are one critical section under lock_source, record deleted first, so the last concurrent deleter always purges exactly once (regression-tested).
• MEDIUM/LOW round-2 items (silent rmtree, probe blocking, off-leader note, credential redaction, reclone timestamp, tests) → all fixed.

No CRITICAL, no crash, no corruption, no cross-tenant exposure, and no security regression remain in the current code. The highest-severity remaining item — the delete-vs-sync re-population leak — is a bounded, same-tenant, never-mis-served resource leak, documented with a seeded churn repro and explicitly deferred to PR3 (leader-routed purge + scope-liveness check); 3 of 4 reviewers (including the security pass) consider that an acceptable scope boundary for PR2 of the series.

Non-blocking follow-ups (posted inline)

• MEDIUM task.py — the round-2 credential regex is redundant and weaker than the existing global opal_common.logger scrubber; remove it / use redact_url_in_text.
• MEDIUM service.py — delete_scope is now ~93 lines / complexity 10, over the project's ≤8 hard limit; extract the purge helper.
• MEDIUM service.py — the deferred delete-vs-sync leak: take the cheap scope_id=-in-sync_scopes mitigation now, add a delete-specific red gate, and surface it as an explicit known-limitation (a 204 DELETE isn't yet a durable cleanup guarantee under concurrent sync).
• MEDIUM service.py/api.py — the defensive over-purge can transiently serve the default bundle to a live sibling; the NoSuchPathError→default-bundle path is untested; consider a retryable error over silent policy substitution for a still-live scope.
• LOW service.py — the delete-record-first reorder can orphan the purge on an ambiguous store failure; run the purge in a finally.
• LOW — fresh-handle-per-bundle-request perf on the hot path (necessary UAF tradeoff; optionally cache the head hash); defensive except could log a traceback.

Consolidated from four specialist reviewers including an OPAL-domain pass using the repo's contributor skill; every claim re-verified against the code.

🤖 Reviewed with Claude Code

[zeevmoney]

1 additional non-blocking MEDIUM (delete_scope complexity over the project limit), posted inline. Overall verdict remains Approve.