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.

Problem

Scope git clone/fetch use pygit2 with no timeout, dispatched through run_sync(...) onto the shared default executor with no time limit on the await:

• clone_repository(...) in GitPolicyFetcher._clone
• repo.remotes[self._remote].fetch(...) in fetch_and_notify_on_changes

POLICY_REPO_CLONE_TIMEOUT is wired only to the legacy non-scopes path, so it does not apply in scopes mode. The consequences:

• Boot (gunicorn_conf → preload_scopes() → asyncio.run(sync_scopes(...))) blocks indefinitely on a single unreachable repo.
• Normal operation: a hung op holds the per-source lock and a shared-pool thread. Enough hung ops starve bundle serving and other server work that shares the default executor.

Fix

A hard per-operation timeout plus an isolated, bounded thread pool so a hung repo fails fast, is skipped, and never starves anything else.

• New module-level dedicated ThreadPoolExecutor (opal-git threads, lazily built) for scope git work, isolated from the default executor.
• New helper run_in_git_executor(func, *args, timeout=..., **kwargs) that runs the blocking call on that pool and wraps the await in asyncio.wait_for. timeout <= 0 means no limit.
• Clone + fetch now go through the helper with SCOPES_GIT_FETCH_TIMEOUT.
• _clone error handling broadened to (pygit2.GitError, asyncio.TimeoutError) — a hung clone is logged and the scope skipped instead of crashing the caller. Per-scope isolation in ScopesService.sync_scope (try/except Exception) already handles a timed-out fetch, making boot best-effort once operations can no longer hang forever.
• Dropped the now-unused run_sync import from git_fetcher.py.

New config keys (opal-server, server-only)

Env var	Type	Default	Purpose
OPAL_SCOPES_GIT_FETCH_TIMEOUT	float (s)	120.0	Hard timeout for a single scope git clone/fetch. On timeout the op is abandoned and the scope marked failed, so one unreachable repo can never block boot or other scopes. 0 = no timeout.
OPAL_SCOPES_GIT_MAX_WORKERS	int	10	Size of the dedicated thread pool for scope git operations; isolates git work from the default executor.

⚠️ Caveat — the timeout is soft, not a hard kill

asyncio.wait_for cancels the await — unblocking the event loop and the awaiting coroutine — but the underlying pygit2 call keeps running on its pool thread until the OS network timeout. The dedicated bounded pool (OPAL_SCOPES_GIT_MAX_WORKERS) isolates those lingering threads so they cannot affect bundle serving or other scopes. Hard-kill via subprocess is explicitly out of scope (spec §6).

Tests

• tests/git_executor_test.py — config defaults; helper returns value; helper times out; timeout=0 means no limit.
• tests/fetch_timeout_test.py — a hanging op surfaces TimeoutError and wait_for unblocks promptly (< 2s with a 0.2s timeout).

Local run (Python 3.12 venv, editable installs of all three packages):

• New tests: 5 passed
• Full opal-server suite: 14 passed
• Full opal-common suite: 64 passed (warnings are pre-existing deprecations)

Not runnable in this PR's CI yet

Task 4 of the plan includes the PR1 regression gate app-tests/git-leak/test_resilience.py::test_offline_repo_does_not_block_healthy_scopes and a live /healthcheck smoke test. Both depend on PR1 (test environment) being merged — the stated prerequisite — and a running stack, so they should be exercised once PR1 lands. This PR ships after PR2 and is independent of it.

Review checklist (opal-development)

• Config keys: server-only consumption (correct component); bare env names (no OPAL_ double-prefix); mandatory description= present (covered by test_config.py); exactly one declaration per env name (no collision).
• PDP impact: server-only keys; no opal_client/opal_common symbol touched; no PDP override of these names.
• Pub/sub: no topic, broadcaster, or fan-out change.
• No new dependency — stdlib concurrent.futures + asyncio only.
• Interlock with PR4: PR4's parallel sync_scopes relies on this PR's guarantee that each fetch terminates, so its bounded gather can never wait on an infinite member.

🤖 Generated with Claude Code

[linear-code]

PER-15157

[netlify]

✅ Deploy Preview for opal-docs canceled.

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

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

[dshoen619]

Good catch — switched to asyncio.get_running_loop() in 5dd53a2.

[dshoen619]

Fixed in 5dd53a2 — repos_last_fetched is now updated only after the fetch completes successfully, so a timeout/error leaves it stale and won't suppress a later force_fetch via _was_fetched_after.

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

[zeevmoney]

[HIGH] New git-fetcher log lines leak un-redacted repo URLs (bypass redact_url)

Problem: On origin/master every repo URL in this file's logs is redacted via redact_url(self._source.url) (git_fetcher.py:110, 113, 140, 189, 199, 224, 235, 237) — a deliberate credential-redaction control, since a scope git URL can embed user:token@host. This PR adds three new log statements that log the raw URL: the single-flight skip warning (line 376), this fetch-timeout error (line 412), and the clone-error handler (line 465). redact_url is not imported here. This branch was cut before that redaction landed, so its diff looks clean in isolation — but the file currently conflicts with master, and once the merge is resolved these will be the only un-redacted URL logs in the module, on exactly the offline/error paths this PR adds → credential exposure in logs.

Suggestion: import redact_url and wrap every self._source.url used as a log arg:

from opal_common.http_utils import redact_url
...
logger.error(
    "Timed out fetching {url}, skipping: {err}",
    url=redact_url(self._source.url),
    err=repr(exc),
)

Apply the same at lines 376 and 465. When resolving the git_fetcher.py merge conflict, also preserve master's redact_url(...) on the pre-existing context lines (395, 423, 449, 469).

[dshoen619]

Fixed — merged master (which brought in the credential redaction from #921) and resolved the git_fetcher.py conflict in f4c54f13.

• redact_url is now imported from opal_common.http_utils, and all three new log lines wrap the URL: single-flight skip, fetch-timeout, and clone-error. Master's redact_url(...) on the pre-existing context lines is preserved.
• The fetch path keeps this PR's soft-timeout run_in_git_executor flow (dropped master's run_sync double-fetch and its separate except pygit2.GitError clause during the resolution).
• The only bare self._source.url uses left are the actual args to clone_repository / verify_found_repo_matches_remote and the sha256 hash input — no log line emits a raw URL. CONFLICTING is cleared; the branch now contains master.

[zeevmoney]

[MEDIUM] Boot/poll worst-case bound is optimistic — timed-out threads are not reclaimed

Problem: the timeout is soft. run_in_git_executor uses asyncio.wait and deliberately does not cancel the future, so a timed-out clone/fetch keeps its pool thread until the OS network timeout (the caveat just above says this, and test_busy_key_stays_in_flight_until_call_returns asserts it). So when the number of simultaneously-offline repos is >= SCOPES_GIT_MAX_WORKERS, the lingering threads keep the pool fully occupied and healthy repos submitted afterward are queued behind them until the OS network timeout — not merely ceil(offline / workers) × timeout. That formula assumes each wave's threads are reclaimed before the next wave starts, which is not the case with lingering (uncancellable) ops.

Suggestion: state that queued git ops wait for a freed pool thread, so with offline >= workers a healthy repo can be delayed up to the OS/TCP timeout of the lingering ops, not just ceil × timeout. The guarantee this PR actually delivers is event-loop isolation (bundle serving and the HTTP surface never block) plus a bounded per-slot stall — worth stating explicitly. (This also bears on the app-tests/git-leak offline test, which uses 40 offline repos against the default 10 workers.)

[dshoen619]

Fixed in 904bf7d1. Rewrote the "Boot / concurrency" note:

• Dropped the optimistic ceil(offline / workers) × timeout bound and explained that queued git ops wait for a freed pool thread — so with offline >= workers a healthy repo can be delayed up to the OS/TCP timeout of the lingering (uncancellable) ops, not ceil × timeout.
• Stated the guarantee this actually delivers: event-loop isolation (HTTP surface + bundle serving never block on a hung repo) plus a bounded per-slot stall.
• Called out the app-tests/git-leak 40-offline / 10-worker case explicitly, and fixed the two config.py line refs while there.