fix(gateway): three sub-tier findings — seal race, unbounded pending, silent unsigned

[mdheller]

Three sub-tier findings verified real at main@55fb4c5a, each with a
discriminating test that would flip on regression.

1. Seal-window race in receipts.py

File: apps/compute-gateway/src/compute_gateway/receipts.py:47-79

Failure: The seal() window (_CHAINS.setdefault → read prev → hash body
→ attest → append → persistence.save_receipt) had no lock at all.
persistence._LOCK only covers the SQLite write itself, not the read of prev
or the id computation. Two concurrent seals for the same project could compute
the same prev, both append, and both persist under seq=N — INSERT OR REPLACE in save_receipt silently dropped one, or the surviving pair had a
broken prev-link and verify() reported valid: False, reason: "prev-link broken" after the race.

Fix: Per-project threading.Lock held across the entire window — bounded
to hash + attest + persist, all pure/local (no arbitrary caller code inside).
Concurrent seals across different projects still run in parallel; the
lock is minted lazily per project (double-check for the common path). Chose
threading.Lock rather than asyncio.Lock because seal is sync and callers
include FastAPI's threadpool as well as pure-sync config_plane.

Test: test_concurrent_seals_for_same_project_never_lose_a_receipt fires
32 threads at one project through a barrier; asserts len(chain) == 32 and
every prev-link resolves. A second test confirms different-project seals
don't serialize on each other.

2. Unbounded pending queue in nugget_extractor/emitter.py

File: apps/nugget-extractor/src/nugget_extractor/emitter.py:145-208
Pod-crash surface: submit() did self._pending.append(_PendingBatch(...))
with no ceiling.

Failure: hellgraph outage stalled drain() at the first failing op
(batch.cursor never advanced past it) while POST /v1/extract kept
admitting new documents — every call added a batch to _pending and the pod
was OOM'd downstream.

Fix: MAX_PENDING = int(os.environ.get('NUGGET_EXTRACTOR_MAX_PENDING', '1000')). submit() gate-checks before building nuggets or advancing the
logical clock; over-cap returns BatchResult(status='degraded', reason=...)
as a pure no-op the caller can safely retry. server.py translates
status='degraded' into HTTP 503. Matches the device-service.run_once
skip-with-reason pattern.

Test: test_submit_refuses_with_degraded_status_when_pending_at_ceiling
patches MAX_PENDING=2 with a wedged graph and asserts the third submit
returns degraded with a pending queue full reason, and _pending did
not grow. Recovery test confirms admissions resume after drain frees the
queue.

3. Silent-unsigned regime in signing.py + /healthz

File: apps/compute-gateway/src/compute_gateway/signing.py:41-88,
apps/compute-gateway/src/compute_gateway/server.py:33-64

Failure: Any exception decoding GATEWAY_SIGNING_KEY returned None, the
seal proceeded unsigned, and verify() reported valid: True because it only
fails on a present-but-invalid signature. An operator with a bad key lost
signatures across the fleet with a green health check.

Fix: Keep the fail-to-None behaviour (the "we never fabricate a
signature" contract is correct) but expose the state:

• New signing.signing_state() → 'signed' | 'unsigned' | 'error'.
• /healthz now emits signing.{state, signed, count, signed_ratio} — the
  live signed / total across all in-memory chains, plus the state so a
  configured-but-broken key surfaces as error distinct from an unconfigured
  key's unsigned.

Test: test_healthz_reports_signing_error_and_signed_ratio stubs a
malformed key, seals a receipt, and asserts /healthz reports
signing.state == 'error' AND signed == 0. Also asserts verify() still
reports valid: True (chain integrity is unaffected) — so /healthz is
the only loud surface for this misconfiguration. That's the whole point.

---

Test results

• apps/compute-gateway: 172 passed (7 new, all pre-existing pass)
• apps/nugget-extractor: 84 passed (4 new, all pre-existing pass)

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Hardens compute-gateway and nugget-extractor against three verified production failure modes: receipt sealing races, unbounded pending growth under downstream outages, and silent signing misconfiguration.

Changes:

• Add per-project locking around receipts.seal() to prevent concurrent prev-link/id collisions and persistence overwrite.
• Cap nugget-extractor pending batches (MAX_PENDING) and surface back-pressure as a degraded result / HTTP 503.
• Expose signing configuration state via signing_state() and include signing posture + signed ratio in /healthz.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
apps/nugget-extractor/src/nugget_extractor/emitter.py	Adds MAX_PENDING cap and “degraded” BatchResult path for back-pressure.
apps/nugget-extractor/src/nugget_extractor/server.py	Translates degraded submit into HTTP 503 and updates server state accordingly.
apps/nugget-extractor/tests/test_pending_cap.py	Adds regression tests for pending cap default/override and degraded behavior.
apps/compute-gateway/src/compute_gateway/receipts.py	Introduces per-project seal locks to close the read-modify-write race window.
apps/compute-gateway/src/compute_gateway/signing.py	Adds signing_state() and expands docs around unsigned/error regimes.
apps/compute-gateway/src/compute_gateway/server.py	Extends /healthz with signing telemetry and computes signed ratio over chains.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

apps/compute-gateway/src/compute_gateway/receipts.py:124

• This docstring claims setdefault on _CHAINS "contends" with _LOCKS_LOCK, but in the current implementation _CHAINS.setdefault(...) is not protected by _LOCKS_LOCK (only _LOCKS creation is). Either update the implementation to make that statement true (preferred, see prior comment), or adjust the docstring to accurately describe the actual synchronization; otherwise it misleads future maintainers about the safety properties.

def snapshot_all() -> list[tuple[str, list[Receipt]]]:
    """A consistent (project, chain) snapshot for read-only aggregators like
    /healthz's signed_ratio. Copilot #1106: iterating `_CHAINS.values()` in the
    server hits `RuntimeError: dictionary changed size during iteration` when a
    concurrent seal() runs setdefault() on a new project. Take the dict snapshot
    under `_LOCKS_LOCK` (which setdefault contends with because _project_lock()
    holds it while creating a new entry) and per-chain snapshots under the
    per-project seal lock so we do not observe a chain mid-append."""
    with _LOCKS_LOCK:
        projects = list(_CHAINS.keys())

apps/compute-gateway/src/compute_gateway/signing.py:84

• base64.b64decode(raw) is permissive by default (it may ignore non-base64 characters). That can cause signing_state() to incorrectly return 'signed' for a malformed env value that happens to decode to 32 bytes after character-stripping/ignoring. Use strict decoding (e.g., base64.b64decode(raw, validate=True) and handle binascii.Error) so 'error' reliably indicates invalid base64.

    raw = os.getenv("GATEWAY_SIGNING_KEY", "").strip()
    if not raw:
        return "unsigned"
    try:
        seed = base64.b64decode(raw)
    except Exception:  # noqa: BLE001
        return "error"
    if len(seed) != 32:
        return "error"

apps/compute-gateway/src/compute_gateway/server.py:55

• /healthz now performs cryptographic signature verification across every in-memory receipt on each call. This can become expensive as chains grow and may slow liveness/ready checks (or increase CPU) under load. Consider caching aggregated signing counters at seal-time (or maintaining them incrementally), or moving full verification to a less frequently polled endpoint/metric while keeping /healthz O(1) or bounded.

    total = 0
    signed = 0
    for _project, chain in receipts.snapshot_all():
        for r in chain:
            total += 1
            if r.signature is not None and r.statement is not None and signing.verify_signature(
                    r.statement, r.signature, r.public_key):
                signed += 1

apps/compute-gateway/tests/test_sub_tier_hardening.py:60

• This test can hang indefinitely if the barrier is never satisfied (e.g., a thread fails/crashes before reaching barrier.wait()). To make the suite more robust, pass a timeout to barrier.wait(timeout=...) and handle BrokenBarrierError by recording it in errors (or failing fast).

    barrier = threading.Barrier(N)
    errors: list[BaseException] = []

    def seal_one(i: int) -> None:
        try:
            barrier.wait()                # maximise race window
            receipts.seal(
                "race-project", kind="notebook", backend="forge", runtime="python3",
                inputs={"i": i}, outputs=[{"i": i}], status="ok", actor="tester",
                epistemic_status="derived")
        except BaseException as e:  # noqa: BLE001 — surface racy exceptions
            errors.append(e)

apps/nugget-extractor/tests/test_pending_cap.py:116

• Reloading the module to test env parsing is integration-valid but brittle (it mutates module globals and can interact poorly with other tests if more module-level state is later added). Since _parse_max_pending() is now a pure helper, consider unit-testing _parse_max_pending(None) == 1000 and _parse_max_pending('5') == 5 directly, and keep only a single minimal reload-based test to assert the wiring from env → MAX_PENDING.

def test_max_pending_default_is_1000():
    """Guard against silent tuning drift — the default is contracted."""
    # Read the current module value, but also verify the env-driven default.
    from importlib import reload
    prev = os.environ.pop("NUGGET_EXTRACTOR_MAX_PENDING", None)
    try:
        reload(em)
        assert em.MAX_PENDING == 1000
    finally:
        if prev is not None:
            os.environ["NUGGET_EXTRACTOR_MAX_PENDING"] = prev
        reload(em)

[mdheller]

Merge-gate disposition (Copilot 3-channel + adversarial pass)

The three sub-tier fixes each have discriminating tests with real teeth (32-thread seal race → chain-length + prev-link; malformed key → signing_state()=='error' + signed==0 on /healthz; pending ceiling → degraded, queue capped). Two adversarial findings were fixed on-branch before merge: the /healthz RuntimeError: dictionary changed size during iteration (now snapshot_all()) and the MAX_PENDING parse (non-int → default, negative → 0).

One sub-tier finding was NOT in the merged head: the degraded (queue-full) refusal misreports which dependency stalled — it hardcodes hellgraph_ok=False/gateway_ok=False and a hellgraph-centric reason even though the queue fills just as readily at the compute-gateway receipt step, and the server overwrites STATE from a call that observed no dependency (attempted=False). Fix (with red→green tests, incl. a concurrent seal-during-/healthz test) is in follow-up #1127.

Verdict: three findings resolved; the degraded-attribution fix lands via #1127.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (10)

apps/compute-gateway/src/compute_gateway/receipts.py:155

• seal() is now correctly serialized per project, but chain() and verify() still read _CHAINS without taking the per-project lock. That allows readers to observe a chain mid-seal (e.g., after chain.append(receipt) but before persistence.save_receipt(...) completes) and verify() can compute transient, misleading failures if called concurrently with a seal. Consider wrapping these read paths with with _project_lock(project): (and iterating over a copied list) so readers see a consistent view aligned with the new concurrency model.

    ch = _CHAINS.get(project, [])

apps/compute-gateway/src/compute_gateway/signing.py:49

• The docstring includes a placeholder reference (Copilot #TBD). Please replace it with a real issue/reference or remove it so the documentation doesn’t ship with unresolved placeholders.

    fabricate a signature). Callers who need to distinguish "no key configured"
    from "key present but bad" should use `signing_state()` — quietly downgrading a
    misconfigured key to unsigned was masking a real operational fault (Copilot #TBD:
    a bad key silently produced unsigned receipts whose `verify()` reported
    `valid: True` because verify only fails on a *present-but-invalid* signature).

apps/compute-gateway/src/compute_gateway/signing.py:84

• base64.b64decode(raw) is permissive by default and can accept/alter malformed inputs (e.g., ignoring non-alphabet characters), which can undermine the goal of reliably surfacing “bad key” as error. Consider using strict decoding (e.g., validate=True) and ideally centralizing the parsing/validation so load_signing_key() and signing_state() can’t diverge over time.

    raw = os.getenv("GATEWAY_SIGNING_KEY", "").strip()
    if not raw:
        return "unsigned"
    try:
        seed = base64.b64decode(raw)
    except Exception:  # noqa: BLE001
        return "error"
    if len(seed) != 32:
        return "error"

apps/compute-gateway/src/compute_gateway/server.py:55

• /healthz re-verifies Ed25519 signatures for every in-memory receipt on every call, which can become expensive as chains grow and as health checks are polled frequently. If the intent is operational visibility rather than tamper detection, consider counting “signed” based on receipt fields known at seal time (or caching a per-receipt “signature verified” flag at seal time) and reserving full cryptographic verification for explicit audit/verify paths.

    total = 0
    signed = 0
    for _project, chain in receipts.snapshot_all():
        for r in chain:
            total += 1
            if r.signature is not None and r.statement is not None and signing.verify_signature(
                    r.statement, r.signature, r.public_key):
                signed += 1

apps/compute-gateway/src/compute_gateway/receipts.py:124

• seal() is now correctly serialized per project, but chain() and verify() still read _CHAINS without taking the per-project lock. That allows readers to observe a chain mid-seal (e.g., after chain.append(receipt) but before persistence.save_receipt(...) completes) and verify() can compute transient, misleading failures if called concurrently with a seal. Consider wrapping these read paths with with _project_lock(project): (and iterating over a copied list) so readers see a consistent view aligned with the new concurrency model.

def chain(project: str) -> list[Receipt]:
    return list(_CHAINS.get(project, []))

apps/compute-gateway/src/compute_gateway/receipts.py:146

• seal() is now correctly serialized per project, but chain() and verify() still read _CHAINS without taking the per-project lock. That allows readers to observe a chain mid-seal (e.g., after chain.append(receipt) but before persistence.save_receipt(...) completes) and verify() can compute transient, misleading failures if called concurrently with a seal. Consider wrapping these read paths with with _project_lock(project): (and iterating over a copied list) so readers see a consistent view aligned with the new concurrency model.

def verify(project: str) -> dict:

apps/nugget-extractor/src/nugget_extractor/emitter.py:227

• The refusal reason specifically blames “hellgraph downstream,” but _pending can also stall at the gateway receipt step (GatewayError), in which case this message (and the implied attribution) becomes misleading. Suggest making the message dependency-agnostic (hellgraph or gateway downstream may be unavailable), or deriving attribution from the last drain attempt (e.g., persist last-known hellgraph_ok/gateway_ok on the emitter and reflect that in reason).

        if len(self._pending) >= MAX_PENDING:
            reason = (
                "nugget-extractor pending queue full "
                f"({len(self._pending)}/{MAX_PENDING}) — hellgraph downstream may be "
                "unavailable; retry after drain")
            log.warning("submit REFUSED — %s (doc=%s)", reason, doc_ref)
            return BatchResult(
                documents=0, extracted=0, emitted=0, validation_failures=0,
                pending=sum(len(b.nuggets) for b in self._pending),
                status="degraded", reason=reason,
                hellgraph_ok=False,   # the cap only trips when drain is stalled
                gateway_ok=False,
            )

apps/nugget-extractor/src/nugget_extractor/server.py:250

• This introduces an error response where HTTPException.detail is a dict, while other error paths in the same endpoint use a string (detail=f"{type(e).__name__}: {e}"). If clients assume detail is always a string, this becomes a breaking inconsistency. Consider standardizing error response shape (e.g., always a structured JSON body via a response model/JSONResponse, or always a string plus separate headers/fields) so callers don’t need per-status parsing logic.

        raise HTTPException(status_code=503, detail={
            "status": "degraded",
            "reason": result.reason,
            "pending": result.pending,
            "hellgraph_ok": result.hellgraph_ok,
            "gateway_ok": result.gateway_ok,
        })

apps/compute-gateway/tests/test_sub_tier_hardening.py:24

• These environment variables are set at import time and never restored, which can leak state across the test suite and create order-dependent failures. Prefer using pytest’s monkeypatch (possibly via an autouse fixture scoped to this module) to set and restore env vars deterministically per test/module.

os.environ["GATEWAY_TOKEN"] = "t"
os.environ["COMPUTE_ENTITLEMENTS"] = "demo"
os.environ["GATEWAY_WRITE_PROVENANCE"] = "false"

apps/nugget-extractor/tests/test_pending_cap.py:116

• Reloading the module to test env/default behavior can introduce side effects (module-level state resets) and can be brittle if other tests rely on emitter state. Since _parse_max_pending() is now a dedicated parser, consider testing it directly for default/override/invalid cases; that keeps these tests purely functional and avoids module reload churn.

def test_max_pending_default_is_1000():
    """Guard against silent tuning drift — the default is contracted."""
    # Read the current module value, but also verify the env-driven default.
    from importlib import reload
    prev = os.environ.pop("NUGGET_EXTRACTOR_MAX_PENDING", None)
    try:
        reload(em)
        assert em.MAX_PENDING == 1000
    finally:
        if prev is not None:
            os.environ["NUGGET_EXTRACTOR_MAX_PENDING"] = prev
        reload(em)

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (9)

apps/compute-gateway/src/compute_gateway/signing.py:41

• The docstring references Copilot #TBD, which looks like a placeholder. Please replace it with a real tracking link/ID or remove it so the documentation doesn’t ship with unresolved placeholders.

def load_signing_key() -> Ed25519PrivateKey | None:

apps/compute-gateway/src/compute_gateway/signing.py:63

• base64.b64decode(raw) is not strict by default, so certain malformed inputs can be silently accepted and still decode to 32 bytes, potentially producing an incorrect 'signed' state. Consider decoding with validation enabled (e.g., validate=True) (and mirroring the same strictness in load_signing_key()), so 'error' reliably indicates invalid base64 content rather than only padding/length issues.

def signing_state() -> str:

apps/compute-gateway/src/compute_gateway/signing.py:84

• base64.b64decode(raw) is not strict by default, so certain malformed inputs can be silently accepted and still decode to 32 bytes, potentially producing an incorrect 'signed' state. Consider decoding with validation enabled (e.g., validate=True) (and mirroring the same strictness in load_signing_key()), so 'error' reliably indicates invalid base64 content rather than only padding/length issues.

    raw = os.getenv("GATEWAY_SIGNING_KEY", "").strip()
    if not raw:
        return "unsigned"
    try:
        seed = base64.b64decode(raw)
    except Exception:  # noqa: BLE001
        return "error"
    if len(seed) != 32:
        return "error"

apps/compute-gateway/src/compute_gateway/signing.py:48

• The docstring references Copilot #TBD, which looks like a placeholder. Please replace it with a real tracking link/ID or remove it so the documentation doesn’t ship with unresolved placeholders.

    misconfigured key to unsigned was masking a real operational fault (Copilot #TBD:
    a bad key silently produced unsigned receipts whose `verify()` reported

apps/compute-gateway/src/compute_gateway/server.py:55

• /healthz now does per-receipt cryptographic verification across all in-memory chains on every probe. If chains can grow large and health checks are frequent (typical in Kubernetes), this can become a CPU hotspot and can make health reporting itself flaky under load. A more scalable approach is to maintain counters at seal-time (e.g., total receipts + verifying-signed receipts) and have /healthz report those, or to cache signature-verification results per receipt id.

    total = 0
    signed = 0
    for _project, chain in receipts.snapshot_all():
        for r in chain:
            total += 1
            if r.signature is not None and r.statement is not None and signing.verify_signature(
                    r.statement, r.signature, r.public_key):
                signed += 1

apps/compute-gateway/src/compute_gateway/receipts.py:124

• seal() is now protected by a per-project lock, but chain() returns a snapshot without taking that same lock. That means callers can observe a chain while it’s being appended to, producing inconsistent snapshots (and rare heisenbugs in callers that iterate and assume stability). Consider taking with _project_lock(project): around the list copy to ensure read-side snapshots are consistent with the new write-side locking model.

def chain(project: str) -> list[Receipt]:
    return list(_CHAINS.get(project, []))

apps/compute-gateway/src/compute_gateway/receipts.py:37

• _LOCKS_LOCK now guards both _LOCKS and mutations of _CHAINS (via the setdefault path). The name suggests it only protects _LOCKS, which makes lock ordering and invariants harder to reason about. Consider renaming it to something that reflects its broadened purpose (e.g., _GLOBAL_LOCK / _CHAINS_LOCK), or splitting into two locks with explicit responsibilities.

_LOCKS_LOCK = threading.Lock()
_LOCKS: dict[str, threading.Lock] = {}

apps/compute-gateway/src/compute_gateway/receipts.py:102

• _LOCKS_LOCK now guards both _LOCKS and mutations of _CHAINS (via the setdefault path). The name suggests it only protects _LOCKS, which makes lock ordering and invariants harder to reason about. Consider renaming it to something that reflects its broadened purpose (e.g., _GLOBAL_LOCK / _CHAINS_LOCK), or splitting into two locks with explicit responsibilities.

        with _LOCKS_LOCK:
            chain = _CHAINS.setdefault(project, [])

apps/nugget-extractor/src/nugget_extractor/server.py:250

• Raising HTTPException with a dict detail will produce a response shaped like { \"detail\": { ... } }, which is often surprising for clients and can complicate OpenAPI expectations for error responses (commonly detail: string). If the intent is a structured top-level payload, consider returning a JSONResponse(status_code=503, content={...}) (or keep detail as a string and add a documented error model/response schema).

        raise HTTPException(status_code=503, detail={
            "status": "degraded",
            "reason": result.reason,
            "pending": result.pending,
            "hellgraph_ok": result.hellgraph_ok,
            "gateway_ok": result.gateway_ok,
        })