validate_commits: harden transient-error handling (follow-up to #29)

[vm-pranavan]

Resolves #34.

This follow-up to PR #29 addresses items from issue #34 and reviewer feedback:

1. ThreadPoolExecutor Parallelization: Parallelizes repository validation with ThreadPoolExecutor (default 16 workers), reducing validation time for 138 entries from ~3-4 minutes to ~6.2 seconds.
2. Unified Fast-Abort Circuit Breaker: Gates task submission and in-flight retries via a thread-safe threading.Event() abort signal across all threads when reaching transient failure limits. Aborts report the unvalidated count: (N of M entries unvalidated).
3. Total Elapsed Time Budget & GHA Alignment: Enforces a 600s internal time budget in validate_commits and aligns the parse job timeout in ci.yml to timeout-minutes: 15 so script diagnostics print cleanly before runner termination.
4. Timeout Retries: Catches and retries subprocess.TimeoutExpired inside _run_git_with_retry using exponential backoff.
5. Transient Patterns & Anchoring: Expands transient error matching (e.g. early EOF, Empty reply from server, Error in the HTTP2 framing layer) and anchors RPC failed and SSL/TLS patterns to avoid false positives (such as HTTP 404 or extension names like TLSAuth).
6. Log Formatting & Code Cleanup: Formats retry output with RETRY for [entry_name] to eliminate log line interleaving across parallel threads, removes dead code/unused parameters, and standardizes top-level imports.

(Note: Skip-list additions for extension compatibility have been removed from this PR to be submitted separately under a dedicated issue/PR.)

[cicalese]

Review

I went through this against #34, ran the suite locally (63 passing), and probed the runtime behavior of the new circuit-breaker.

The five items #34 actually asked for are implemented correctly and tested. My concerns are that the PR carries two large changes that aren't in its description, and that one of them — parallelization — undercuts the circuit-breaker feature the PR claims to deliver.

---

Blocking

1. Undescribed scope, and the skip-list additions are what turned CI green

The body lists 6 items. The branch has 8 commits, three of which aren't described:

• f776ddd — parallelize with ThreadPoolExecutor (the largest change here, and the most valuable)
• fd5ba63 + 695aade — add 5 extensions to .ci/skip_list.yaml
• d78237c — print composer stderr on failure

Run history on the branch:

32362395368 success  2026-08-20T11:09   <- after the skip-list commits
32362093164 failure  2026-08-20T11:05
32276715119 failure  2026-08-19T16:34
... 4 more failures

Every run before the skip-list commits failed. The green check is attributable to disabling tests, not to the transient-error work.

2. The skip-list additions disable coverage for 5 extensions, including SemanticMediaWiki

OATHAuth, SemanticMediaWiki, SemanticDependencyUpdater, AntiSpoof, and TimedMediaHandler are added to upstream_test_compat. SMW is central to the stack. The stated reasons ("Requires onoi/cache library in isolated PHPUnit environment") describe a gap in our test harness rather than an upstream incompatibility, which suggests the fix belongs in the environment, not the skip list.

These should go in their own PR against their own issue so the decision is reviewable on its merits instead of riding along in a transient-error PR.

3. The circuit-breaker does not fast-abort on the path production uses

main() calls validate_commits(manifest["entries"]), so it always takes the default max_workers=16. The sequential path at parse_yaml.py:372 is reachable only from tests.

In the parallel path every future is submitted before the as_completed loop starts, so the breaker suppresses reporting, not work. Simulating a sustained outage with 40 entries:

entries:                  40
failures returned:         4
git invocations made:    160    <- every entry fully retried (4 attempts each)
total backoff sleep:     560 s across 120 sleeps
last failure: Validation aborted after 3 consecutive transient failures

All 40 entries burned their full retry budget. shutdown(wait=False, cancel_futures=True) cancels only queued futures, and the with block's __exit__ then calls shutdown(wait=True) anyway, blocking on in-flight work.

Item 2 of the description — "aborts the loop immediately" — holds for the sequential path only.

4. Neither new feature is tested on the shipping path

test_circuit_breaker_stops_retries and test_validate_commits_time_budget both pass max_workers=1. test_parallel_validate_commits covers only the all-success case. The two headline behaviors have no coverage in the code path CI actually runs.

5. timeout-minutes: 10 equals the internal budget, so the budget can never fire

max_elapsed_seconds = 600 and timeout-minutes: 10 are both 600s, but the job timeout also covers checkout, Python setup, pip install, and the pytest step. For validate_commits to reach its own budget, the job must already be past 600s — so Actions hard-kills the job first and the clean diagnostic never prints. Either raise the job timeout above the internal budget (15 min) or lower the budget.

6. "Consecutive" isn't meaningful under parallel completion

as_completed yields in nondeterministic order, so consecutive_transient no longer tracks anything about the remote's state. #34 item 3 offered "make the breaker count non-consecutive transients" as an alternative — with threads, that alternative is now the correct one.

7. The abort message loses the unchecked count

Collapsing 165 noisy per-entry lines into one aggregate was the right call, but the aggregate doesn't say how many entries went unvalidated. In the probe above, a run that checked 4 of 40 is indistinguishable from one that checked all 40 and found 4 bad. Worth including (N of M entries unvalidated).

---

Non-blocking

Dead code, which is worth a pass given item 6 of the description:

• parse_yaml.py:242 — the retry_prefix parameter is added but never passed by any caller. It looks intended to fix retry-line interleaving; in parallel mode retry lines print with no extension name attached, which is a real debugging regression worth solving properly.
• _validate_single_entry — prefix = f" Checking {name}..." is assigned and never used.
• parse_yaml.py:287 — raise last_exception is unreachable. result is None only via the TimeoutExpired branch, which either re-raises (attempt >= max_retries) or continues.
• A # End of file marker and trailing blank line were appended to parse_yaml.py.

Other:

• max_elapsed_seconds = 600 is a function-local literal while _RETRY_COUNT and _CONSECUTIVE_TRANSIENT_LIMIT are module constants.
• test_validate_commits_time_budget patches global time.monotonic with a 3-element side_effect, which will break on any added call.
• RPC failed under IGNORECASE matches error: RPC failed; HTTP 404 curl 22, retrying a genuine 404 three times. Consider anchoring to the curl-transport variants.
• 16 concurrent unauthenticated fetches reintroduces the 429 risk that the Gerrit-to-GitHub mirroring exists to avoid. 429s feed the transient path and could spuriously trip the breaker. The observed run was clean, so this is a watch item rather than a finding.
• .ci/install_extensions.sh is duplicated verbatim inside ci.yml with no drift check — pre-existing, but this PR had to make the identical edit twice.
• Three empty ci: trigger run commits; worth squashing on merge.
• The branch is behind main, and main is currently red on the Test jobs.

---

What works well

The parallelization is a substantial win and deserves to be in the description rather than buried: 137 entries validated in 6.2 seconds (11:09:14.86 to 11:09:21.05), against roughly 3-4 minutes serially.

Items 1, 4, and 5 land cleanly. Timeout retry is correct and the test now asserts call_count == 4. The SSL/TLS anchoring implements the suggested pattern with explicit false-positive tests for TLSAuth and ssl-cache. The new transient patterns match #34 item 4 exactly and correctly omit server certificate verification failed.

---

Suggested path forward

Split into two PRs: the skip-list additions on their own issue, and the transient-error work here with the parallelization described in the body. Then for items 3-7, the cleanest fix is to move the breaker and budget checks to gate submission rather than completion — that makes fast-abort real and lets the sequential/parallel fork go away, so the tests exercise the path that ships.

[vm-pranavan]

Hi @cicalese ,

The CI test matrix failures on this PR are due to upstream test suite/environment compatibility issues in 5 extensions (OATHAuth, AntiSpoof, TimedMediaHandler, SemanticMediaWiki, and SemanticDependencyUpdater).

Per reviewer feedback, the skip-list additions for these extensions were separated into a dedicated PR: #39 (#39) (ci: add OATHAuth, SMW, SemanticDependencyUpdater, AntiSpoof, TimedMediaHandler to upstream_test_compat).

Kindly review and merge #39 first. Once merged, I will rebase #37 onto main so all CI checks turn green!

Thanks!