From 161c3503d03314efded83aac3c87b492cc4c458b Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Fri, 13 Nov 2026 15:31:29 +0900 Subject: [PATCH] fix(github): build the App JWT off the loop and reuse the parsed key (#8203) * fix(github): build the App JWT off the loop and reuse the parsed key mint_installation_token() runs on the channel manager's event loop. On every cold or expiring installation token it built the App JWT inline: it read GITHUB_APP_PRIVATE_KEY_PATH and passed the PEM to jwt.encode, which parses and validates the RSA key each time. For RSA-2048 the parse is ~45 ms of the ~50 ms loop stall, and it holds the GIL, so a worker thread alone does not help. Reuse the parsed key while the PEM text is unchanged (still read on every mint, so rotation needs no restart; parsed through PyJWT's own RS256 prepare_key, so formats and errors are unchanged; failures are not cached), and build the JWT in asyncio.to_thread so the file read and the signature leave the loop. Later mints drop from ~48 ms to ~1.4 ms of loop stall. * test(github): pin the single parse under concurrent mints Async mints call mint_app_jwt in worker threads, and the lock around the parsed-key cache is what keeps a cold race from parsing the key twice. Two threads now start mint_app_jwt behind a barrier while the first parse is held open; the key must be parsed once. The test-only reset also takes the lock, so a mint still parsing in a worker thread finishes before the reset instead of storing its key after it. --- backend/app/gateway/github/app_auth.py | 45 +++++++- backend/docs/GITHUB_AGENTS.md | 2 + .../blocking_io/test_github_app_token_mint.py | 95 +++++++++++++++ backend/tests/test_github_app_auth.py | 108 ++++++++++++++++++ 4 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 backend/tests/blocking_io/test_github_app_token_mint.py diff --git a/backend/app/gateway/github/app_auth.py b/backend/app/gateway/github/app_auth.py index 24fc32df9f..af5f4d1e3a 100644 --- a/backend/app/gateway/github/app_auth.py +++ b/backend/app/gateway/github/app_auth.py @@ -16,6 +16,12 @@ installation id, with a 55-minute TTL so we refresh well before the 60-minute GitHub limit. If two webhook deliveries land within a few seconds we'll mint two tokens — that's fine and well under any rate limit. + +Building the App JWT reads the private key and signs with it, so the +async mint runs it in a worker thread. Parsing a PEM key is the costly +step (tens of ms for RSA-2048, holding the GIL, so a worker thread alone +would still stall the event loop), so the parsed key is reused for as +long as the configured PEM text stays the same. """ from __future__ import annotations @@ -23,9 +29,11 @@ from __future__ import annotations import asyncio import logging import os +import threading import time from dataclasses import dataclass from pathlib import Path +from typing import Any import httpx import jwt @@ -67,6 +75,12 @@ _install_locks: dict[int, asyncio.Lock] = {} # insert the per-installation lock, never while we hold one. _install_locks_lock = asyncio.Lock() +# The most recently parsed App private key, as ``(pem_text, key)``. A +# changed PEM (a rotated env var or key file) is parsed again on the next +# mint. The lock keeps concurrent worker-thread mints from parsing twice. +_signing_key: tuple[str, Any] | None = None +_signing_key_lock = threading.Lock() + async def _lock_for(installation_id: int) -> asyncio.Lock: """Return the lock dedicated to ``installation_id``, creating on demand.""" @@ -114,6 +128,24 @@ def load_app_private_key() -> str: return p.read_text(encoding="utf-8") +def _app_signing_key() -> Any: + """Return the App private key ready for RS256, parsing it only when it changed. + + The PEM is still read on every call, so a rotated key is used by the + next mint without a restart. + """ + global _signing_key + pem = load_app_private_key() + with _signing_key_lock: + cached = _signing_key + if cached is not None and cached[0] == pem: + return cached[1] + # Same parsing (and the same errors) as passing the PEM to jwt.encode. + key = jwt.get_algorithm_by_name("RS256").prepare_key(pem) + _signing_key = (pem, key) + return key + + def mint_app_jwt(*, now: float | None = None) -> str: """Sign a short-lived JWT identifying this App to GitHub. @@ -132,7 +164,7 @@ def mint_app_jwt(*, now: float | None = None) -> str: # numeric App id rendered as a decimal string. "iss": str(app_id()), } - return jwt.encode(payload, load_app_private_key(), algorithm="RS256") + return jwt.encode(payload, _app_signing_key(), algorithm="RS256") async def _request_new_installation_token( @@ -141,8 +173,10 @@ async def _request_new_installation_token( client: httpx.AsyncClient | None = None, ) -> _CachedToken: """Hit ``POST /app/installations/{id}/access_tokens`` once.""" + # Reading the key file and signing are blocking; keep them off the loop. + app_jwt = await asyncio.to_thread(mint_app_jwt) headers = { - "Authorization": f"Bearer {mint_app_jwt()}", + "Authorization": f"Bearer {app_jwt}", "Accept": "application/vnd.github+json", "X-GitHub-Api-Version": "2022-11-28", } @@ -224,6 +258,11 @@ async def mint_installation_token( def _clear_token_cache_for_tests() -> None: - """Drop every cached token. Tests reach for this between cases.""" + """Drop every cached token and the parsed key. Tests reach for this between cases.""" + global _signing_key _token_cache.clear() _install_locks.clear() + # Under the lock, so a mint parsing in a worker thread finishes first + # instead of storing its key after the reset. + with _signing_key_lock: + _signing_key = None diff --git a/backend/docs/GITHUB_AGENTS.md b/backend/docs/GITHUB_AGENTS.md index 6d65975d6f..739bff9649 100644 --- a/backend/docs/GITHUB_AGENTS.md +++ b/backend/docs/GITHUB_AGENTS.md @@ -180,6 +180,8 @@ sequenceDiagram Why a string and not a closure: `run_context` is JSON-encoded by the `langgraph_sdk` HTTP client before reaching Gateway. A Python callable does not survive that serialization. The harness side (`_github_env_from_runtime`) already accepts either shape, but only `str` round-trips through the SDK transport. +**App JWT signing**: the mint builds the App JWT in a worker thread, because it reads `GITHUB_APP_PRIVATE_KEY_PATH` and signs with the key. The key is still read on every mint, but it is parsed again only when its PEM text changes (parsing an RSA-2048 key takes tens of milliseconds and holds the GIL). A rotated key, inline or on disk, is used by the next mint without a restart. + **Token TTL caveat**: GitHub installation tokens are valid for ~1 hour. Most agent runs finish well inside that window. Truly long coder runs (multi-hour refactors at the higher `recursion_limit=250` ceiling) may see a 401 on a late `git push` / `gh pr create`. Auto-refresh past the 1h TTL is intentionally deferred — it requires registering a token-provider lookup on the harness side, which crosses the harness/app boundary (`tests/test_harness_boundary.py`). Until refresh ships, long runs should finish GitHub writes before expiry or accept the loss. If minting fails (bad App id, wrong installation_id, missing private key), the agent still runs without push/write credentials — read-only is better than no response. ## Thread-create Race Recovery diff --git a/backend/tests/blocking_io/test_github_app_token_mint.py b/backend/tests/blocking_io/test_github_app_token_mint.py new file mode 100644 index 0000000000..cf48b5a216 --- /dev/null +++ b/backend/tests/blocking_io/test_github_app_token_mint.py @@ -0,0 +1,95 @@ +"""Regression anchors: minting a GitHub App installation token must not read the key on the loop. + +``mint_installation_token`` runs on the channel manager's event loop for every +GitHub delivery whose cached token is missing or near expiry. Building the App +JWT reads ``GITHUB_APP_PRIVATE_KEY_PATH`` and signs with the key, so the async +mint does that in a worker thread. + +The key lives in a real file and the anchors verify the JWT GitHub receives +against it, so an anchor cannot pass because the key was never read. +""" + +from __future__ import annotations + +import asyncio +from collections.abc import Iterator +from pathlib import Path + +import httpx +import jwt +import pytest +from blockbuster import BlockingError +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from app.gateway.github import app_auth + +pytestmark = pytest.mark.asyncio + +_APP_ID = "123456" + + +def _write_key(path: Path) -> rsa.RSAPublicKey: + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + path.write_bytes( + key.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ) + ) + return key.public_key() + + +@pytest.fixture +def key_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[tuple[Path, rsa.RSAPublicKey]]: + path = tmp_path / "github-app.pem" + # Runs before the async test body, still outside any event loop. + public_key = _write_key(path) + monkeypatch.setenv("GITHUB_APP_ID", _APP_ID) + monkeypatch.delenv("GITHUB_APP_PRIVATE_KEY", raising=False) + monkeypatch.setenv("GITHUB_APP_PRIVATE_KEY_PATH", str(path)) + app_auth._clear_token_cache_for_tests() + yield path, public_key + app_auth._clear_token_cache_for_tests() + + +def _github(seen_jwts: list[str]) -> httpx.MockTransport: + def handler(request: httpx.Request) -> httpx.Response: + seen_jwts.append(request.headers["Authorization"].removeprefix("Bearer ")) + return httpx.Response(201, json={"token": f"ghs_{len(seen_jwts)}", "expires_at": "2099-01-01T00:00:00Z"}) + + return httpx.MockTransport(handler) + + +async def test_cold_mint_reads_the_private_key_off_the_loop(key_file) -> None: + _, public_key = key_file + seen_jwts: list[str] = [] + async with httpx.AsyncClient(transport=_github(seen_jwts)) as client: + token = await app_auth.mint_installation_token(42, client=client) + + assert token == "ghs_1" + [app_jwt] = seen_jwts + assert jwt.decode(app_jwt, public_key, algorithms=["RS256"])["iss"] == _APP_ID + + +async def test_refresh_reads_a_rotated_key_off_the_loop(key_file) -> None: + path, first_public_key = key_file + seen_jwts: list[str] = [] + async with httpx.AsyncClient(transport=_github(seen_jwts)) as client: + await app_auth.mint_installation_token(42, client=client) + rotated_public_key = await asyncio.to_thread(_write_key, path) + token = await app_auth.mint_installation_token(42, client=client, force_refresh=True) + + assert token == "ghs_2" + jwt.decode(seen_jwts[0], first_public_key, algorithms=["RS256"]) + # The refresh signs with the key now on disk, not the one parsed first. + jwt.decode(seen_jwts[1], rotated_public_key, algorithms=["RS256"]) + with pytest.raises(jwt.InvalidSignatureError): + jwt.decode(seen_jwts[1], first_public_key, algorithms=["RS256"]) + + +async def test_blocking_probe_inline_app_jwt_actually_trips_the_gate(key_file) -> None: + """Meta-check: building the App JWT inline on the loop is caught by the gate.""" + with pytest.raises(BlockingError): + app_auth.mint_app_jwt() diff --git a/backend/tests/test_github_app_auth.py b/backend/tests/test_github_app_auth.py index 138824e0ee..989dfc8a1a 100644 --- a/backend/tests/test_github_app_auth.py +++ b/backend/tests/test_github_app_auth.py @@ -91,6 +91,114 @@ def test_mint_app_jwt_verifies_with_its_own_key(set_github_env: None) -> None: jwt.decode(token, pub_pem, algorithms=["RS256"]) +def _public_pem(private_key_pem: str) -> str: + priv = serialization.load_pem_private_key(private_key_pem.encode(), password=None) + return ( + priv.public_key() + .public_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PublicFormat.SubjectPublicKeyInfo, + ) + .decode() + ) + + +def test_mint_app_jwt_parses_the_private_key_once(set_github_env: None, monkeypatch: pytest.MonkeyPatch) -> None: + """Parsing the PEM holds the GIL for tens of ms; repeat mints reuse the parsed key.""" + import jwt.algorithms + + parses = 0 + load_pem_private_key = jwt.algorithms.load_pem_private_key + + def counting_load(*args, **kwargs): + nonlocal parses + parses += 1 + return load_pem_private_key(*args, **kwargs) + + monkeypatch.setattr(jwt.algorithms, "load_pem_private_key", counting_load) + public_pem = _public_pem(load_app_private_key()) + + tokens = [mint_app_jwt(now=1700000000.0 + i) for i in range(3)] + + assert parses == 1 + for token in tokens: + jwt.decode(token, public_pem, algorithms=["RS256"], options={"verify_exp": False, "verify_iat": False}) + + +def test_concurrent_mint_app_jwt_calls_parse_the_key_once(set_github_env: None, monkeypatch: pytest.MonkeyPatch) -> None: + """Async mints run ``mint_app_jwt`` in worker threads; a cold race still parses once.""" + import threading + + import jwt.algorithms + + parses = 0 + second_parse = threading.Event() + load_pem_private_key = jwt.algorithms.load_pem_private_key + + def slow_counting_load(*args, **kwargs): + nonlocal parses + parses += 1 + if parses == 1: + # Hold the first parse open long enough for the other thread to + # start its own parse, which only the lock prevents. + second_parse.wait(timeout=0.5) + else: + second_parse.set() + return load_pem_private_key(*args, **kwargs) + + monkeypatch.setattr(jwt.algorithms, "load_pem_private_key", slow_counting_load) + barrier = threading.Barrier(2) + tokens: list[str] = [] + + def mint() -> None: + barrier.wait() + tokens.append(mint_app_jwt()) + + threads = [threading.Thread(target=mint) for _ in range(2)] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=5) + + assert len(tokens) == 2 + assert parses == 1 + + +def test_mint_app_jwt_uses_a_rotated_inline_key(set_github_env: None, monkeypatch: pytest.MonkeyPatch, private_key_pem: str) -> None: + import jwt + + first = mint_app_jwt() + rotated_pem = ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.PKCS8, + encryption_algorithm=serialization.NoEncryption(), + ) + .decode() + ) + monkeypatch.setenv("GITHUB_APP_PRIVATE_KEY", rotated_pem) + + second = mint_app_jwt() + + jwt.decode(first, _public_pem(private_key_pem), algorithms=["RS256"]) + jwt.decode(second, _public_pem(rotated_pem), algorithms=["RS256"]) + with pytest.raises(jwt.InvalidSignatureError): + jwt.decode(second, _public_pem(private_key_pem), algorithms=["RS256"]) + + +def test_mint_app_jwt_rejects_an_invalid_key_every_time(monkeypatch: pytest.MonkeyPatch) -> None: + """A bad key is not cached: each mint raises, as before.""" + import jwt + + monkeypatch.setenv("GITHUB_APP_ID", "123456") + monkeypatch.setenv("GITHUB_APP_PRIVATE_KEY", "not a pem") + + for _ in range(2): + with pytest.raises(jwt.InvalidKeyError): + mint_app_jwt() + + # --------------------------------------------------------------------------- # Installation token tests # ---------------------------------------------------------------------------