From 984083c7e86c7d85d03fef135c177fecc6be378e Mon Sep 17 00:00:00 2001 From: Renn F Date: Fri, 31 Jul 2026 16:52:54 +0200 Subject: [PATCH] fix(gateway): bound the briefing's RAG memory leg; skip cold conventions re-resolution in claim_review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining 120s-wall consumers found tracing live 504s on the redeployed stack (claim_review completing at exactly 120s with a ~74s silent tail after the bounded evidence legs gave up): _institutional_memory's similar_memory call rode the Ollama embedder's 120s read timeout unbounded inside every full briefing (give_me_work / i_will_work_on / i_will_plan / resume pass task=t), so a saturated embedder could eat a claim verb's whole budget. It now runs under asyncio.wait_for with the new institutional_memory_timeout_seconds (default 8s) and degrades to a first-class {status: "timeout", lessons: []} block — the briefing is enrichment, never the thing that kills a verb. claim_review's advisory conventions leg trusted that the diff / files_changed legs had warmed the workspace; when those legs timed out, conventions_check_for_task's unwrapped setup phase re-hit the same cold/contended git ops (30s-bounded subprocesses in sequence, clone repair up to 300s) and ground on until the verb wall. The evidence build now skips the conventions leg whenever the git legs already recorded gaps, emitting the existing could_not_run shape plus an evidence_gaps note instead of a 504. Fail-closed enforcement (i_am_done / pr_pass) is untouched. Gate: 15481 passed, 459 skipped; ruff/mypy/xenon/vulture/bandit/ pip-audit/deptry/import-linter/foundation-check green. --- roboco/config.py | 10 ++ .../services/gateway/choreographer/_impl.py | 25 ++- roboco/services/gateway/choreographer/qa.py | 45 ++++-- .../test_conventions_in_qa_evidence.py | 144 ++++++++++++++++++ .../test_institutional_memory_timeout.py | 79 ++++++++++ 5 files changed, 288 insertions(+), 15 deletions(-) create mode 100644 tests/unit/gateway/test_institutional_memory_timeout.py diff --git a/roboco/config.py b/roboco/config.py index c259ec9e..f5acc90e 100644 --- a/roboco/config.py +++ b/roboco/config.py @@ -1173,6 +1173,16 @@ class Settings(BaseSettings): le=1.0, description="Cosine-similarity floor for injected memory; below it, none.", ) + institutional_memory_timeout_seconds: float = Field( + default=8.0, + gt=0, + description=( + "Deadline for the institutional-memory RAG search (embed + query) " + "during a claim briefing. RAG memory is a nice-to-have enrichment " + "— tight on purpose, so a saturated embedder can never burn the " + "verb's whole timeout budget and 504 the claim." + ), + ) # Sandboxed per-agent-spawn DB/Redis — orchestrator-provisioned throwaway # Postgres/Redis sibling containers so a dev agent's gate runs against an diff --git a/roboco/services/gateway/choreographer/_impl.py b/roboco/services/gateway/choreographer/_impl.py index 7e1c4041..771abdd5 100644 --- a/roboco/services/gateway/choreographer/_impl.py +++ b/roboco/services/gateway/choreographer/_impl.py @@ -11,6 +11,7 @@ injection so later phases just fill in the bodies. from __future__ import annotations +import asyncio import contextlib from dataclasses import dataclass from datetime import UTC, datetime @@ -1181,7 +1182,8 @@ class Choreographer: Returns ``{"status": ..., "lessons": [...]}`` where status is one of ``disabled`` (subsystem off / no task — ponytail: both mean no search ran), - ``error`` (search raised), ``empty`` (search yielded nothing), + ``error`` (search raised), ``timeout`` (searched, but the memory search + didn't answer in time), ``empty`` (search yielded nothing), ``below_floor`` (searched, nothing met the floor), ``ok`` (lessons injected). Lessons is empty unless status is ``ok`` — the status is additive, the injection behavior is unchanged.""" @@ -1200,11 +1202,22 @@ class Choreographer: else: task_type = str(raw_type) query = shape_memory_query(role, title, task_type) - result = await self._deps.evidence_repo.similar_memory( - query=query, - top_k=_settings.org_memory_top_k, - min_score=_settings.org_memory_min_score, - ) + try: + result = await asyncio.wait_for( + self._deps.evidence_repo.similar_memory( + query=query, + top_k=_settings.org_memory_top_k, + min_score=_settings.org_memory_min_score, + ), + timeout=_settings.institutional_memory_timeout_seconds, + ) + except TimeoutError: + logger.warning( + "institutional_memory_timeout", + agent_id=str(agent_id), + task_id=str(getattr(task, "id", "")), + ) + return {"status": "timeout", "lessons": []} return { "status": str(result.get("status", "error")), "lessons": list(result.get("items", [])), diff --git a/roboco/services/gateway/choreographer/qa.py b/roboco/services/gateway/choreographer/qa.py index 4ac3ab67..13eed6e4 100644 --- a/roboco/services/gateway/choreographer/qa.py +++ b/roboco/services/gateway/choreographer/qa.py @@ -229,6 +229,12 @@ class QAMixin(_Base): was already resolved (warmed) by the diff/list_changed_files legs above in the same evidence build — so the one theoretically-unbounded piece (a cold clone) never actually triggers here in practice. + + That warm-workspace assumption only holds when those legs actually + completed. ``_build_qa_claim_evidence`` now checks for the opposite + signal (``evidence_gaps`` already non-empty) and skips calling this + method entirely in that case, instead of trusting a false assumption + and re-hitting the same cold/contended git ops unbounded. """ if not settings.conventions_enabled: return [] @@ -321,15 +327,36 @@ class QAMixin(_Base): # Leaf-only journals stay (include_ancestors defaults False above); # ancestor *descriptions* are the ask, not work-so-far. parent_context = await self.evidence_repo.ancestor_context_for_task(task_id) - convention_findings = await self._qa_convention_findings( - qa_agent_id, - t, - timeout=min( - settings.conventions_validator_advisory_timeout_seconds, - budget.remaining(), - ), - gaps=evidence_gaps, - ) + convention_findings: list[dict[str, Any]] + if evidence_gaps and settings.conventions_enabled: + # The diff/files_changed legs above already timed out, so the + # "workspace was warmed" assumption _qa_convention_findings relies + # on (see its docstring) is false — skip it rather than re-hit the + # same cold/contended git ops and grind toward the 120s verb wall. + convention_findings = [ + { + "could_not_run": True, + "reason": ( + "skipped: git evidence legs timed out " + "(cold/contended workspace)" + ), + } + ] + evidence_gaps.append( + "conventions findings unavailable: skipped because prior " + "evidence legs timed out (cold/contended workspace) — review " + "the diff manually for architecture-convention issues" + ) + else: + convention_findings = await self._qa_convention_findings( + qa_agent_id, + t, + timeout=min( + settings.conventions_validator_advisory_timeout_seconds, + budget.remaining(), + ), + gaps=evidence_gaps, + ) open_findings = await findings_lib.open_findings_for_task( self.task.session, t.id ) diff --git a/tests/unit/gateway/test_conventions_in_qa_evidence.py b/tests/unit/gateway/test_conventions_in_qa_evidence.py index 9c3b56ce..e6aad609 100644 --- a/tests/unit/gateway/test_conventions_in_qa_evidence.py +++ b/tests/unit/gateway/test_conventions_in_qa_evidence.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio from typing import Any from unittest.mock import AsyncMock, MagicMock from uuid import uuid4 @@ -100,3 +101,146 @@ def test_evidence_payload_convention_findings_default_empty() -> None: ev = build_evidence_for_task(_stub_task(), journal_highlights=[], files_changed=[]) assert ev.convention_findings == [] assert "convention_findings" not in ev.as_dict() + + +# --------------------------------------------------------------------------- +# _build_qa_claim_evidence: skip conventions when the git legs above already +# timed out (evidence_gaps non-empty — the warm-workspace assumption failed). +# --------------------------------------------------------------------------- + + +def _stub_empty_ledger(session: MagicMock) -> None: + session.execute = AsyncMock( + return_value=MagicMock( + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) + ) + ) + + +def _evidence_choreographer(git_svc: AsyncMock) -> Choreographer: + """Full Choreographer wired for ``_build_qa_claim_evidence`` directly + (not the whole claim_review verb) — journal/evidence_repo/session + stubbed empty so the build reaches the conventions call site cleanly.""" + task_svc = AsyncMock() + _stub_empty_ledger(task_svc.session) + evidence_repo = AsyncMock() + evidence_repo.journal_highlights_for_task.return_value = [] + evidence_repo.ancestor_context_for_task.return_value = [] + deps = ChoreographerDeps( + task=task_svc, + work_session=AsyncMock(), + git=git_svc, + a2a=AsyncMock(), + journal=AsyncMock(), + audit=AsyncMock(), + evidence_repo=evidence_repo, + ) + return Choreographer(deps) + + +def _evidence_task(task_id: Any) -> MagicMock: + return MagicMock( + id=task_id, + branch_name="feature/backend/abc", + pr_number=8, + pr_url="https://github.com/x/y/pull/8", + commits=[], + dev_notes=None, + acceptance_criteria_status=[], + parent_task_id=None, + description=None, + ) + + +@pytest.mark.asyncio +async def test_conventions_skipped_when_git_legs_already_timed_out( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Both the diff and list_changed_files legs time out (evidence_gaps + ends up non-empty) — _qa_convention_findings (and the real + conventions_check_for_task it would call) must never run; the caller + fills in a could_not_run skip entry and its own evidence_gaps note + instead.""" + monkeypatch.setattr(settings, "conventions_enabled", True) + monkeypatch.setattr(settings, "evidence_assembly_timeout_seconds", 0.02) + + async def _hangs(*_args: object, **_kwargs: object) -> Any: + await asyncio.sleep(5) + return "unreachable" + + git_svc = AsyncMock() + git_svc.diff.side_effect = _hangs + git_svc.list_changed_files.side_effect = _hangs + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_not_awaited() + assert body["convention_findings"] == [ + { + "could_not_run": True, + "reason": "skipped: git evidence legs timed out (cold/contended workspace)", + } + ] + gaps = body["evidence_gaps"] + assert any("pr diff unavailable" in g for g in gaps) + assert any("files_changed unavailable" in g for g in gaps) + assert any("conventions findings unavailable" in g for g in gaps) + + +@pytest.mark.asyncio +async def test_conventions_runs_when_legs_succeed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The skip only engages when evidence_gaps is non-empty — a normal + build (both legs succeed) still runs conventions exactly as before.""" + monkeypatch.setattr(settings, "conventions_enabled", True) + git_svc = AsyncMock() + git_svc.diff.return_value = "diff content" + git_svc.list_changed_files.return_value = ["README.md"] + git_svc.conventions_check_for_task.return_value = { + "findings": [], + "could_not_run": False, + } + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_awaited_once() + assert "evidence_gaps" not in body + + +@pytest.mark.asyncio +async def test_conventions_not_skipped_stub_when_flag_off( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Even with degraded legs, a disabled subsystem must stay empty (no + misleading could_not_run stub implying conventions would otherwise have + run) — the skip guard checks conventions_enabled too.""" + monkeypatch.setattr(settings, "conventions_enabled", False) + monkeypatch.setattr(settings, "evidence_assembly_timeout_seconds", 0.02) + + async def _hangs(*_args: object, **_kwargs: object) -> Any: + await asyncio.sleep(5) + return "unreachable" + + git_svc = AsyncMock() + git_svc.diff.side_effect = _hangs + git_svc.list_changed_files.side_effect = _hangs + c = _evidence_choreographer(git_svc) + task_id = uuid4() + t = _evidence_task(task_id) + + ev = await c._build_qa_claim_evidence(uuid4(), t, task_id) + body = ev.as_dict() + + git_svc.conventions_check_for_task.assert_not_awaited() + assert "convention_findings" not in body + gaps = body["evidence_gaps"] + assert not any("conventions findings unavailable" in g for g in gaps) diff --git a/tests/unit/gateway/test_institutional_memory_timeout.py b/tests/unit/gateway/test_institutional_memory_timeout.py new file mode 100644 index 00000000..e975f536 --- /dev/null +++ b/tests/unit/gateway/test_institutional_memory_timeout.py @@ -0,0 +1,79 @@ +"""``_institutional_memory``'s RAG search is bounded by +``institutional_memory_timeout_seconds`` — a saturated Ollama embedder must +never eat the whole verb timeout budget and 504 a claim. See +``roboco.services.gateway.choreographer._impl.Choreographer._institutional_memory``. +""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, MagicMock +from uuid import uuid4 + +import pytest +from roboco.config import Settings +from roboco.services.gateway.choreographer import Choreographer + +_DEFAULT_TIMEOUT = 8.0 +_OVERRIDE_TIMEOUT = 3.5 + + +def _choreographer(*, similar_memory: AsyncMock) -> Choreographer: + repo = AsyncMock() + repo.similar_memory = similar_memory + task_svc = AsyncMock() + task_svc.agent_for.return_value = MagicMock(role="developer") + choreo = object.__new__(Choreographer) + choreo._deps = MagicMock(evidence_repo=repo, task=task_svc) + return choreo + + +def _task() -> MagicMock: + return MagicMock( + id=uuid4(), title="Add retry backoff", task_type=MagicMock(value="code") + ) + + +class TestInstitutionalMemoryTimeout: + @pytest.mark.asyncio + async def test_slow_search_times_out_without_raising( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr("roboco.config.settings.org_memory_enabled", True) + monkeypatch.setattr( + "roboco.config.settings.institutional_memory_timeout_seconds", 0.01 + ) + + async def _slow(**_kwargs: object) -> dict[str, object]: + await asyncio.sleep(5) + return {"items": [], "status": "ok"} + + choreo = _choreographer(similar_memory=AsyncMock(side_effect=_slow)) + result = await choreo._institutional_memory(uuid4(), _task()) + assert result == {"status": "timeout", "lessons": []} + + @pytest.mark.asyncio + async def test_fast_search_flows_through_unchanged( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr("roboco.config.settings.org_memory_enabled", True) + monkeypatch.setattr( + "roboco.config.settings.institutional_memory_timeout_seconds", 8.0 + ) + lesson = {"kind": "learning", "summary": "s", "source": "src", "score": 0.9} + similar_memory = AsyncMock(return_value={"items": [lesson], "status": "ok"}) + choreo = _choreographer(similar_memory=similar_memory) + result = await choreo._institutional_memory(uuid4(), _task()) + assert result == {"status": "ok", "lessons": [lesson]} + + +class TestInstitutionalMemoryTimeoutConfig: + def test_default_is_eight_seconds(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("ROBOCO_INSTITUTIONAL_MEMORY_TIMEOUT_SECONDS", raising=False) + assert Settings().institutional_memory_timeout_seconds == _DEFAULT_TIMEOUT + + def test_reads_env_override(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv( + "ROBOCO_INSTITUTIONAL_MEMORY_TIMEOUT_SECONDS", str(_OVERRIDE_TIMEOUT) + ) + assert Settings().institutional_memory_timeout_seconds == _OVERRIDE_TIMEOUT