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.

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

[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