mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-11-13 06:31:47 +00:00
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.
This commit is contained in:
parent
067c9736b6
commit
161c3503d0
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
95
backend/tests/blocking_io/test_github_app_token_mint.py
Normal file
95
backend/tests/blocking_io/test_github_app_token_mint.py
Normal file
@ -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()
|
||||
@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user