Commit 6498da1
fix(opal-server): memory leak — purge GitPolicyFetcher caches on scope delete + webhook task cleanup (PR2) (#923)
* feat(opal-server): gated /internal git-fetcher cache stats endpoint
Add an off-by-default diagnostics endpoint so tests can observe the
in-memory GitPolicyFetcher cache sizes (repo_locks/repos/repos_last_fetched)
and process RSS that the upcoming memory-leak fix eliminates.
- debug_stats.py: read-only git_fetcher_cache_stats() helper + a
register_internal_stats_route() registrar that mounts
GET /internal/git-fetcher-cache-stats only when enabled.
- config.py: new OPAL_DEBUG_INTERNAL_STATS flag, default False.
- server.py: register the route, gated by the flag, beside /healthcheck.
No production behavior change when the flag is off (the default). Also
ignore .claude/ so private planning artifacts are never committed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): add OPAL git leak/resilience test bed
A self-contained docker-compose stack (opal-server x2 workers + Redis +
Postgres broadcaster + Gitea) plus a pytest harness that reproduces, as
tests that fail on master, the git-fetcher memory leak, the offline-repo
hang, the slow serial boot, and the broadcaster-disconnect gap. These
become the regression gates for the follow-up fixes.
- seed/: idempotent Gitea seeding sidecar (N policy repos) + Dockerfile.
- docker-compose.yml: 4-service stack, opal-server built from the repo's
own docker/Dockerfile (server target), scopes on, Postgres broadcaster.
- helpers.py / conftest.py: HTTP + infra helpers and stack fixtures.
- test_leak.py / test_resilience.py / test_boot.py: the flagship tests.
- README.md: how to run and expected fail-on-master behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): add GiteaAdmin and make_repo_unreachable helpers
Complete the helpers.py surface promised in the plan's file-structure
table. Both are now functional and used, not dead code:
- make_repo_unreachable(name): returns a git URL on a routable-but-dead
TEST-NET-1 host (RFC 5737). test_offline_repo now uses it instead of an
inlined literal.
- GiteaAdmin: host-side Gitea admin client (list_repos / repo_exists /
create_repo / delete_repo), exposed as the `gitea_admin` pytest fixture
for tests that need to inspect or stage repos beyond the seed sidecar.
Gitea is published on host port 13000 (uncommon, to avoid the usual :3000
clash) so GiteaAdmin can reach it; opal_server and the seed sidecar still
use the internal http://gitea:3000. README updated with the helper and
port notes.
Verified live: GiteaAdmin lists the seeded repos and round-trips
create/exists/delete against Gitea over the published port.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): correct postgres-bounce framing (passes on master)
Verified test_server_recovers_after_postgres_bounce against the stack: it
PASSES on master (~14-19s). On a broadcaster drop the affected worker
triggers a graceful shutdown, gunicorn respawns it, and the sibling worker
keeps serving HTTP, so the surface recovers within the window — recovery
happens via gunicorn's in-container worker supervision, not an external
supervisor and not an in-process reconnect.
Reframe #5 as a recovery guard (not a known-broken case) in the docstring
and README; the prior "FAILS on master / needs external supervisor" wording
was wrong. PER-15065's in-process reconnect would avoid the worker churn but
recovery already holds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style(git-leak): apply black/isort/docformatter (pre-commit)
Run the repo's pinned pre-commit formatters (black 23.1.0, isort 5.12.0,
docformatter 1.7.5) over the PR1 files to satisfy the pre-commit CI check.
Formatting only — no behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: scope root pytest collection to packages/ (exclude git-leak bed)
The CI `build` job runs `pytest` from the repo root with no path, which
recursed into app-tests/git-leak/ and ran the flagship tests — these are
designed to FAIL on master (they are the regression gates for PR2-PR5), so
they broke the build job.
Set `testpaths = packages` so the rootdir run collects only the unit tests
under packages/ (matching master's effective behavior, since app-tests/ had
no pytest files before). testpaths only applies when pytest is invoked from
the rootdir with no args, so `cd app-tests/git-leak && pytest` still collects
and runs the test bed. Verified both: root run -> packages only; subdir run
-> all 5 flagship tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): address Copilot review feedback
- stats(): sample the /internal endpoint several times and merge per key
with max(). The caches are per-process on a multi-worker server, so a
single read can hit an empty (non-leader) worker — max-merge avoids both
false negatives (missing a populated leader) and false positives (an
`== 0` drain assertion passing only because it hit an empty worker).
- test_leak: assert the initial-load `_wait_until` succeeded before deleting
/ before taking baseline, so the tests can't pass vacuously when load
never completed.
- refresh_all(): correct the misleading comment — a 404 is a no-op, there
is no client-side fallback.
- conftest: skip the suite cleanly if docker is unavailable (defense in
depth; it's already excluded from the default pytest run via testpaths).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): isolate scopes per test and fix false repeat-sync gate
Two review findings on the regression gates:
- Cross-test contamination: clone paths are keyed by repo URL (source_id =
sha256(url)+branch-shard), not scope_id, so scopes from different tests that
point at the same seeded repo share one GitPolicyFetcher cache entry. With a
session-scoped stack and no teardown, a leftover boot-*/stable-* scope kept
those entries alive and would make test_churn's `repos == 0` drain assertion
fail on fixed code. OpalServerClient now tracks created scopes and the opal
fixture deletes them on teardown (best-effort drain wait, swallows errors so
master — where delete never purges — doesn't fail the passing test).
- False gate: test_repeat_sync_does_not_grow re-syncs identical scopes, which a
path-keyed cache can't grow even on master, so it could never be the leak gate
it claimed. Reframed as an honest idempotency guard (passes on master) that
points at test_churn_releases_caches as the real leak gate; README's
"Expected on master" reclassifies it alongside the postgres-bounce guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): make the regression gates trustworthy (address PR review)
Addresses the CHANGES_REQUESTED review on PR #922. Root cause behind most
findings: a fresh scope's first sync takes the _clone() branch, which only
fills GitPolicyFetcher.repo_locks; repos/repos_last_fetched are filled on a
*second* sync. So the load gates on `repos` hung, and the 2-worker per-process
caches made `== 0` drain assertions unsafe.
Blocking:
- Single worker (UVICORN_NUM_WORKERS=1): deterministic per-process cache reads;
removes the false `== 0` drain class.
- Load gate (CRITICAL + Zivxx HIGH): _load_scopes gates on repo_locks then
refresh_all() to force the second sync, so repos/repos_last_fetched are
actually populated before any drain/purge assertion.
- compose() surfaces captured stdout/stderr on failure.
- Seed completeness asserted in conftest; seed script isolates per-repo
failures and exits non-zero with a count.
Secondary:
- test_repeat_sync asserts an RSS bound (count can't grow for any impl);
churn asserts all three caches drain + a loose RSS backstop.
- blackhole socat sidecar replaces TEST-NET-1 (deterministic hang); offline
test saturates the fetch executor with 40 hung clones and recovers via
OpalServerClient.hard_reset() (stop -> redis FLUSHALL -> start).
- Per-test clean slate deletes all server scopes (fixes orphan-scope leak).
- Postgres-bounce proves broadcast recovery (PUT post-bounce, assert sync)
and uses `up -d --wait`.
- Remove dead 404 branch in refresh_all; boot clock starts at restart;
gitea-admin `|| true` -> "already exists"-only guard; README reworded.
opal-server:
- /internal stats route now takes the JWTAuthenticator dependency (protected
when JWT on, no-op in the test bed); unit test asserts enforcement.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style(git-leak): apply black/isort/docformatter (pre-commit)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): tighten stat polling and pin test-bed images (PR review)
- snapshot a single /internal stats read per poll in the churn-drain and
boot gates (consistent multi-key observation; fewer HTTP round-trips)
- document why a 200 from the healthy scope can't be a masked default
bundle, and why the stats route is intentionally a sync def
- pin alpine/socat and the seed image's pip deps for reproducibility
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): isolate offline-hang healthy probe to a never-cloned repo
Ziv (PR review, round 2) caught that the offline-resilience gate can
false-PASS in the full-suite run. The "healthy" scope pointed at
policy-repo-0000 (list_seeded_repos(1)[0]), but on-disk clones are keyed
by URL-hash and survive compose restart/stop/start (opal_server mounts no
volume at /opal; only `down -v` wipes them). test_boot/test_leak run first
(alphabetical) and already clone every seeded repo, so the healthy scope
hit the existing clone via _discover_repository, skipped _clone(), and
served 200 without ever touching the saturated fetch executor — the gate
that must FAIL on this branch (no PR3 timeout) passed.
Fix: seed a reserved repo (policy-repo-healthy-probe) outside the numeric
policy-repo-NNNN range that no boot/leak test enumerates, and point the
healthy probe at it. A never-cloned repo forces a genuine fresh clone
through the starved pool, so the gate fails correctly. The seed-
completeness check in conftest now covers the reserved repo too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(git-leak): harden harness teardown and tighten assertions (PR review)
Follow-up to PR review of the git-leak/resilience test bed:
- hard_reset: always restart opal_server + wait_healthy via a finally, so a
failed redis FLUSHALL can't leave the server stopped (which would fail every
later session-scoped test and, running in a test finally, mask the result).
- delete_all_scopes drain: a transient /internal read error no longer counts as
a successful drain (was `except: return`); keep polling to the deadline so a
not-yet-drained cache can't leak into the next test once PR2 lands.
- use stats(samples=1) for the zero-waiting drain/empty polls (the peak-merge
only matters for load assertions; this also drops 3x HTTP per poll).
- resilience: narrow broad `except Exception` to requests.RequestException
(and RuntimeError for wait_healthy timeout) so harness bugs surface instead of
masquerading as "never served"/"never recovered".
- resilience: collapse `assert opal.stats()` + redundant re-read into one read.
- debug_stats_test: assert rss_kb > 0 on Linux (was `>= 0`, which passed for the
wrong reason where /proc is absent and RSS reads fall back to 0).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(opal-server): add GitPolicyFetcher.forget_repo to release cached repo handles
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(opal-server): purge git fetcher caches on scope delete
Capture the deleted scope's source before scanning siblings (fixing the
loop-variable shadowing bug where the clone path was computed from the
last-iterated scope). Drop the cached pygit2.Repository handle via
forget_repo and pop repos_last_fetched when no sibling shares the source.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(opal-server): clean all finished webhook tasks (no remove-while-iterating)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(opal-server): actually purge fetcher caches on scope DELETE
The PR2 purge in ScopesService.delete_scope was dead code: the HTTP
DELETE /scopes/{scope_id} route deleted the scope straight from the
ScopeRepository, so the git-leak churn gate stayed red with all cache
entries intact. Wire the route to the service (keeping delete-of-missing
a 204 no-op), and also drop the source_id-keyed repo_locks entry, which
neither forget_repo (path-keyed) nor the service purge released.
Verified against the app-tests/git-leak bed: test_churn_releases_caches
now passes and test_shared_repo_survives_sibling_scope_delete still
guards the shared-clone case.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* style(opal-server): fix import order in delete_scope_route_test (isort)
CI fixes:
- pre-commit: isort wanted `import pytest` in the top import block of the
new test file; it was missed locally because `pre-commit run --all-files`
only covers tracked files and the file was untracked when formatters ran.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): serialize the scope-delete cache purge under the repo lock
delete_scope mutated GitPolicyFetcher's lock-guarded shared state (rmtree
of the clone dir, Repository.free(), cache pops) without holding
repo_locks[source_id], racing in-flight fetches that run on executor
threads — a native use-after-free. Popping the lock entry also let the
next fetcher mint a fresh lock and run unserialized (lock identity
hazard).
Replace _get_repo_lock with GitPolicyFetcher.lock_source(source_id), an
async context manager that re-checks the repo_locks entry after acquiring
and retries on the fresh lock if a delete popped it. delete_scope now
purges under that lock and pops the entry while holding it, so repo_locks
still drains to zero (keeping the PR1 churn gate green) without the
identity hazard.
Also, per the same review pass: guard delete_scope against non-git policy
sources like sync_scope does, restore the sharing scope's id in the
skip-deletion log line, and document that the purge is process-local
best-effort (fleet-wide purge incl. the leader is tracked for PR3).
Addresses review comments:
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): surface rmtree and Repository.free() failures in scope cleanup
rmtree(ignore_errors=True) silently orphaned a deleted tenant's policy
clone on any filesystem failure (EBUSY, permissions, stale NFS handle)
while the scope record was deleted anyway — no log, no way to ever find
the leftovers. Replace with an explicit try/except that tolerates a
missing dir and logs any other OSError at WARNING with the scope id and
path, still proceeding with the scope deletion.
forget_repo likewise swallowed Repository.free() exceptions at DEBUG with
no exception detail or path; a systematic free() failure would be
invisible in production. Log at WARNING with the clone path and error,
and fix the docstring claim about free() availability (the pinned pygit2
always ships it). Add the missing test for the free()-raises branch.
Addresses review comments:
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): retrieve webhook task exceptions before dropping them
The webhook task sweep dropped finished tasks without inspecting
t.exception(), and stop() only gathered self._tasks — so an error raised
inside trigger() never reached the app logger, surfacing only as
asyncio's generic "Task exception was never retrieved" at GC time. Log
failed trigger tasks at ERROR during the sweep and include webhook tasks
in the stop() gather.
Also de-flake the cleanup test: await the freshly scheduled trigger task
directly instead of relying on an asyncio.sleep(0) scheduler tick, and
add a test asserting the failure of a trigger task is retrieved and
logged.
Addresses review comments:
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* feat(opal-server): expose pid and cache keys in internal fetcher stats
Per-worker attribution for the git-leak bed: the caches are per-process,
so the pid identifies which worker answered and the key lists let the
invariant checker compare cache contents against live scopes and disk.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): only stamp repos_last_fetched after a successful fetch
A failed fetch left the pre-written stamp in place, so
_was_fetched_after() suppressed the next webhook-requested forced
refresh and the scope served stale policy until an unrelated force.
Stamp the fetch START time, written only on success.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): pin fetch-START semantics of the repos_last_fetched stamp
Review follow-up: a completion-time regression would have passed the
existing tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): make invalid-clone recovery drop the stale handle and clear partial dirs
The recovery path rmtree'd a bad clone but kept its cached
pygit2.Repository, so the stale handle re-invalidated every fresh clone
(infinite re-clone loop, leaked handle per cycle) — recovery only worked
on a cold cache. forget_repo the handle first, clear any pre-existing
(partial) dir before cloning, and cache the fresh clone's handle.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): delete-then-recreate serializes on the repo lock
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): stop() cancels and gathers in-flight webhook triggers
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): read fetcher cache stats from atomic snapshots
git_fetcher_cache_stats() runs on a Starlette worker thread while the
GitPolicyFetcher repo_locks/repos/repos_last_fetched dicts are mutated on
the event-loop/executor threads; iterating a live dict's keys() across
that race can raise "dictionary changed size during iteration", and
computing len() and sorted(keys()) as two independent reads of a mutating
dict can return a self-contradictory count/keys pair (e.g. repos=1 while
listing 2 keys). Fix by snapshotting each cache once via dict.copy() (a
single, GIL-atomic C-level op) and deriving both the count and the key
list from that same snapshot.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): assert recovery actually free()s the stale handle
test_recovery_forgets_stale_cached_handle only checked that the broken
handle was evicted from the cache, not that it was free()d — a
regression to a bare .pop() would still pass while reintroducing the
fd/mmap leak the recovery path is meant to close.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): invariant checker + pid-stability teardown for the git-leak bed
Every bed test now asserts at teardown: no orphan clone dirs (I1), cache
keys consistent with disk and live scopes (I2-I4), worker pid-set
unchanged (I5 - a crashed worker resets the caches and can fake a clean
drain), server liveness (I6). Red gates exempt named invariants via
marker.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(opal-server): document stats() merge semantics and fix stale hints
Review follow-up (deferred behind in-flight work): counts merge with
max() while pid/key-lists are last-wins, so cross-field consistency is
only guaranteed at samples=1; annotate Dict[str, Any] accordingly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): bed gate for delete/recreate storms on one source
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): warm-boot gate — restart must reuse on-disk clones
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): seeded randomized churn driver with invariant checks
Seeded random put/refresh bursts with settled end-of-round deletes (plus a
ghost delete to keep the 204 no-op path exercised); repo_locks must not
exceed live sources at every settle point. Replay any failure with
CHURN_SEED=<printed seed>.
Two deliberate constraints, both to be lifted when PR3's fleet purge lands
(no silent caps):
- repoints are excluded: a put on a live scope reuses its existing repo,
since a repoint orphans the old source's caches by design today (the red
repoint gate covers it).
- deletes run only after every live scope has settled: the driver's
development run surfaced (deterministically, seed 309006536) that a
DELETE racing an in-flight sync loses its purge — the sync re-populates
the dead scope's caches and clone dir. Same PR3 class; deterministic
red-gate coverage lands in
test_repoint_during_inflight_fetch_drains_old_source.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): characterize upstream force-push and branch-delete behavior
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): delete-during-hung-fetch gates (no-crash green, bounded-return red until PR3)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): repoint-during-hung-fetch gate (red until PR3 update-path purge)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): system gate for corrupt-clone recovery (no re-clone loop)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): corrupt clone contents in place so recovery exercises the warm-handle branch
The previous corruption (rm -rf .git/objects) deleted the directory NODE,
so pygit2.discover_repository returned None and recovery went through the
repo-not-found -> _clone() branch, never touching the cached-handle
(_get_valid_repo -> forget_repo) branch the Bug A fix lives in. Emptying
the objects dir's CONTENTS keeps discovery succeeding and forces recovery
through the warm cached handle; a new assert on the "Deleting invalid
repo" log pins the branch taken.
NOTE: this gate is currently RED — a real finding. With objects/ emptied
in place, _get_valid_repo() still judges the repo valid (open + remote-URL
verification raise no pygit2.GitError), so the invalid-repo recovery
branch never fires; the failure surfaces later in make_bundle
(_get_current_branch_head -> "SHA ... missing") and the scope serves 500
forever with no re-clone. The corrupted-but-discoverable case is not
covered by the landed Bug A fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): detect gutted object stores during clone validation
A clone can be discoverable yet unusable: refs and config intact but the
object store gutted (crash mid-gc, disk corruption). _get_valid_repo()
judged such a repo valid (open + remote-URL verification raise no
GitError), a forced fetch then negotiated "up to date" against the intact
refs and downloaded nothing, and every policy read failed in make_bundle
with "SHA ... missing" — the scope served 500s forever with no self-heal.
Fix: validate that the tracked branch's head object is actually readable
FROM DISK before declaring the repo valid. Crucially the check uses a
short-lived fresh Repository handle, not the cached warm handle: the warm
handle keeps deleted pack files readable through its open mmaps (unlink
does not invalidate them) and reports the object as present even after
the store is emptied on disk — verified empirically in the test bed,
where a cached-handle check never fired. An unreadable head object now
routes the repo to the existing invalid-repo recovery branch
(forget cached handle -> delete dir -> re-clone, exactly once).
Residual: partial corruption deeper in the object graph is not caught
(that would need fsck-grade checks); only head-object readability is
validated.
The system gate added in 52a1fe9 (in-place object-store gutting under a
warm handle cache) flips green with this fix, including its "Deleting
invalid repo" branch-pin assert: detection warning, one re-clone, stable
clone count.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): orphan-dir and redis-wipe boot gates (red until orphan sweep)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): clean the wiped scope's clone dir so later I1 checks stay meaningful
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): boot-while-remotes-down gate (red until PR3 fetch timeout)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): clean stranded blackhole clones after the boot-down gate
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): shard-reconfig boot gate (serves green, orphans red until sweep)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): multi-worker churn gate — every worker must drain (red until PR3)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(opal-server): correct the multi-worker leak mechanism in the churn gate docstring
Purges are process-local; the LEADER syncs accumulate via its watcher (scopes/task.py),
and any bundle-serving worker additionally caches a pygit2 handle. Only the DELETE-serving
worker purges, so every accumulation on a different worker outlives the scope.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(opal-server): extend git-leak gate matrix with lifecycle transition tests
Adds 14 matrix rows for the new test_transitions.py/test_boot_states.py/test_remote_transitions.py tests and an Invariants section documenting I1-I6 and the invariant_exempt/allow_worker_restart markers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): make stats_by_pid min_pids an actual early-exit
Once min_pids distinct pids are observed, take one grace sample to ensure freshness, then break.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): stop make_bundle reading the shared pygit2 handle off-lock
_get_current_branch_head() (called from make_bundle, which is invoked via
run_sync(fetcher.make_bundle, ...) on executor threads by the policy-bundle
route, and directly on the loop by _generate_default_scope_bundle) was
calling self._get_repo(), which reads/populates the shared class-level
repos cache. Both call sites run outside lock_source, so the cached handle
can be concurrently .free()'d by forget_repo() during a scope delete or
invalid-repo recovery — a native use-after-free on the pygit2 handle.
asyncio locks don't exclude executor threads, so the lock around
fetch_and_notify_on_changes does not protect this path.
Fixed by opening a short-lived fresh Repository handle per call (freed in a
finally), the same fresh-probe pattern _get_valid_repo already uses for its
disk-truth check. All remaining shared cached-handle access now happens
under lock_source.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): serialize sibling-check and purge on the source lock (delete TOCTOU)
delete_scope's sibling-scope check ran before lock_source and before the
record delete. Two concurrent deletes of scopes sharing a source_id could
each see the other still present in the scope store and both skip the
purge, permanently orphaning the clone dir, pygit2 handle cache, and
repos_last_fetched entry.
Restructured so the record delete, sibling re-check, and purge all happen
inside a single lock_source(deleted_source_id) critical section, with the
record delete first — so whichever of the two concurrent deletes finishes
last sees no remaining sharer and performs the purge. Also switched the
rmtree call to run_sync so it no longer blocks the event loop.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): log rmtree failures in recovery and clone pre-clean
The invalid-repo recovery rmtree and _clone's partial-dir pre-clean both
used ignore_errors=True, silently swallowing any OSError beyond a simple
missing directory (e.g. a permissions problem or a busy mount) with no
trace in the logs. Replaced both with the same try/except pattern the
delete-scope purge path already uses: tolerate FileNotFoundError (already
gone is the intended end state) but log other OSErrors as a warning so a
persistent failure is discoverable instead of silently retried forever.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): run the clone-validation probe off-loop and stamp recloned repos
_get_valid_repo() opens a fresh Repository handle from disk and walks refs
to detect a gutted object store; called synchronously inline in
fetch_and_notify_on_changes, a slow disk stalls the event loop and every
other request being served on the worker. Route it through run_sync so it
runs on the executor.
Also stamp repos_last_fetched on a successful clone: a reclone just
downloaded current remote state, so without the stamp _was_fetched_after()
sees no record and _should_fetch() forces a redundant fetch on the very
next sync cycle.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): redact credentialed URLs in webhook task failure logs
Webhook trigger failures were logged with repr(exception) verbatim. Git
exceptions from a failed fetch/clone can embed a credentialed remote URL
(https://user:token@host/repo) directly in the exception message, so a
webhook failure would leak the credential into the server logs. Strip any
`://user:pass@`-shaped segment before logging.
Added a unit test covering a trigger failure whose message embeds a
credentialed URL: the logged ERROR must contain the redacted form and must
not contain the raw credential.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): cover _get_valid_repo healthy paths and probe lifecycle
_get_valid_repo's failure paths (stale cached handle, gutted object store)
were already covered, but its two healthy paths were not: (a) the tracked
branch hasn't been fetched yet (probe.lookup_reference raises KeyError,
tolerated as "fetch path handles that"), and (b) the probe confirms the
head object is actually readable from disk. Both must return the cached
repo unchanged and free() the short-lived disk probe.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): purge defensively when the post-delete sibling check fails
delete_scope's sibling re-check runs after the record delete; its all()
does a full store scan + Scope.parse_raw, so a transient store error or
one malformed record raises. That exception escaped the source lock with
NOTHING purged, and because the record is already deleted the client's
retry is a 204 no-op (ScopeNotFoundError) — the purge became permanently
unreachable, orphaning the clone dir and both fetcher caches.
The re-check failure is now downgraded to a warning and treated as "no
sharer" (purge). Over-purging self-heals — a surviving sibling re-clones
on its next sync — while under-purging is a permanent leak with no retry
path. The record-delete itself still propagates failures normally.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): offload recovery rmtrees and widen the PR3 delete-routing note
The invalid-repo recovery rmtree and _clone's partial-dir pre-clean ran
synchronously on the event loop while this same PR offloads the
delete-path rmtree — inconsistent, and a large clone dir on a slow disk
stalls every request on the worker. Both now go through run_sync inside
their existing try/excepts.
Also widened delete_scope's PR3 NOTE: besides the cross-process leader
gap, the same class exists in-process — a sync that loaded the scope
before the delete and acquires the fresh lock after the purge re-clones
and re-populates the caches for the dead scope. PR3's purge routing must
include a scope-liveness check before clone.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): fall back to the default bundle when a clone dir vanishes mid-request
Both bundle-route except clauses (make_bundle and
_generate_default_scope_bundle) caught InvalidGitRepositoryError,
pygit2.GitError, and ValueError but missed GitPython's NoSuchPathError —
raised when a concurrent scope delete or invalid-repo recovery fully
rmtree'd the clone dir before Repo() opens it. That turned a transient
race into a 500 instead of the intended default-scope fallback. Added
NoSuchPathError to both tuples.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): stamp clone start time after a reclone
The reclone stamp added earlier in this PR recorded completion time,
contradicting the start-time rule the fetch path documents at the top of
the same function: negotiation reflects remote state at operation START,
so a clone that started after a webhook's req_time already satisfies it,
while a completion stamp can wrongly suppress a forced refresh requested
mid-clone. Capture clone_started immediately before run_sync(
clone_repository, ...) and stamp that value in the success branch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* refactor(opal-server): reuse redact_url_in_text for webhook failure logs
The hand-rolled _CREDENTIALED_URL_RE duplicated (a weaker form of) the
shared scrubber: it missed ?token=/?access_token= query-string
credentials that redact_url_in_text's SENSITIVE_QUERY_PARAMS path
covers. Use the vetted helper instead; kept local (rather than relying
solely on the global log patcher) because the patcher only exists once
configure_logs() has installed it.
Addresses review comments:
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): keep the delete purge reachable and extract the purge helper
An ambiguous store outcome (record delete committed server-side, error
surfaced to the client) aborted delete_scope before the purge: the
retry then hits ScopeNotFoundError -> 204 no-op, so the clone + cache
entries leaked permanently. Run the purge in a finally so a record-
delete failure can't gate cache cleanup; the error still propagates.
Extracting the sibling-check + purge into
_purge_source_cache_if_unshared / _find_scope_sharing_source also
brings delete_scope from complexity 10 back under the <=8 project
limit, with the critical section unchanged.
Addresses review comments:
- #923 (comment) (@zeevmoney)
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(opal-server): re-check scope liveness right before each queued sync
sync_scopes passed the stale all() snapshot object into sync_scope, so
a DELETE that landed while the scope was still queued re-cloned the
dead scope's repo and re-populated repos/repos_last_fetched/repo_locks
(leaked until restart) — reintroducing a bounded form of the leak this
series closes. Pass scope_id instead so sync_scope re-gets fresh state
right before use, shrinking the window from the rest of the sweep to
the sync itself; the delete race now surfaces as ScopeNotFoundError
and is logged at info. The remaining window (delete landing mid-sync)
is a known limitation deferred to PR3's leader-routed purge +
scope-liveness check.
Addresses review comments:
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(opal-server): cover the clone-vanish default-bundle fallback
The NoSuchPathError -> default-bundle branch of GET /scopes/{id}/policy
(a live scope whose clone dir vanished under a concurrent
delete/recovery) had no test; lock it in alongside the pre-existing
scope-not-found fallback. Returning a retryable error (503) for the
live-scope case instead of silently substituting the default bundle is
deferred to PR3 with the delete-routing work.
Addresses review comments:
- #923 (comment) (@zeevmoney)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: David Shoen <dbshoen@gmail.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: dshoen619 <107557675+dshoen619@users.noreply.github.com>1 parent b108180 commit 6498da1
26 files changed
Lines changed: 2375 additions & 79 deletions
File tree
- app-tests/git-leak
- packages/opal-server/opal_server
- policy/watcher
- scopes
- tests
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
51 | 51 | | |
52 | 52 | | |
53 | 53 | | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
54 | 81 | | |
55 | 82 | | |
56 | 83 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
67 | 68 | | |
68 | 69 | | |
69 | 70 | | |
70 | | - | |
| 71 | + | |
71 | 72 | | |
72 | 73 | | |
73 | 74 | | |
| |||
84 | 85 | | |
85 | 86 | | |
86 | 87 | | |
| 88 | + | |
87 | 89 | | |
88 | 90 | | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
89 | 100 | | |
90 | 101 | | |
91 | 102 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
114 | 114 | | |
115 | 115 | | |
116 | 116 | | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
117 | 121 | | |
118 | 122 | | |
119 | 123 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
53 | | - | |
| 53 | + | |
54 | 54 | | |
55 | 55 | | |
56 | 56 | | |
| |||
59 | 59 | | |
60 | 60 | | |
61 | 61 | | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
62 | 71 | | |
63 | | - | |
| 72 | + | |
64 | 73 | | |
65 | 74 | | |
66 | 75 | | |
67 | 76 | | |
68 | 77 | | |
69 | 78 | | |
70 | | - | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
71 | 85 | | |
72 | 86 | | |
73 | 87 | | |
| |||
234 | 248 | | |
235 | 249 | | |
236 | 250 | | |
237 | | - | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
238 | 257 | | |
239 | 258 | | |
240 | 259 | | |
| |||
250 | 269 | | |
251 | 270 | | |
252 | 271 | | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
253 | 305 | | |
254 | 306 | | |
255 | 307 | | |
| |||
396 | 448 | | |
397 | 449 | | |
398 | 450 | | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
| 490 | + | |
| 491 | + | |
399 | 492 | | |
400 | 493 | | |
401 | 494 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
0 commit comments