mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
Fix/backend/flow verb timeout row lock (#326)
* feat(sandbox): pluggable per-engine registry (postgres/redis/mongo) Replaces the hardcoded postgres+redis branches in the provisioner and the env emitter with a registry of SandboxEngine specs (image, run args, readiness probe, connection, ROBOCO_TEST_* env) in a pure low module (roboco/models/sandbox.py). VALID_SANDBOX_SERVICES is derived from the registry — single source of truth — and the provisioner + orchestrator iterate it, so adding an engine is one class + one registry line, not another branch. Adds a mongo:8-alpine engine (ROBOCO_TEST_MONGO_*) as the third service alongside postgres/redis. Also fixes the cold-pull loop that stranded v0.19.0 board agents with empty error strings: docker run pulled inline under a 20s deadline, so a NAS cold pull was killed, cancelled, and re-pulled from scratch forever. _ensure_image now inspects + pulls (300s) before run; provisioning errors log type+message so a bare TimeoutError no longer shows as "". Panel edit-project dialog: postgres/redis toggles -> a Set<string> multi-select driven by a SANDBOX_SERVICES catalog, so new engines appear in the UI by adding to the catalog. Tests: engine parity (allowlist==registry, unique slugs/images, no None leak in env, SandboxInfo aggregates every engine), mongo provision + env injection, plus the existing postgres/redis provision/env/spawn/janitor suite updated to the registry shape. 821 unit / 5 skip green; ruff + mypy (360 files) clean. * docs(sandbox): reflect pluggable engine registry + mongo across docs CHANGELOG (0.19.0): Added entry for the pluggable sandbox engine registry (postgres/redis/mongo) + Fixed entry for the cold-pull loop/empty-error strand that boarded v0.19.0 board agents. docs/map (9 files): sandbox subsystem blurbs, SandboxProvisioner rows, _maybe_provision_sandbox/_append_sandbox_env rows, feature-flag rows, the migration-057 row + v0.17.0 delta, and the models.md VALID_SANDBOX_SERVICES note — all retitled to DB/Redis/Mongo via the engine registry (roboco/models/sandbox.py), with the one-class-one-line extension story and the _ensure_image cold-pull fix. Production-network (roboco_data) lines left as postgres+redis — mongo is sandbox-only, not a prod service. docs/rag (3 files): sandbox-db.md rewritten around the registry (engine list, generic _provision_engine, image pre-pull, ROBOCO_TEST_DB_*/REDIS_*/MONGO_* incl. MONGO_AUTH_DB=admin, single emit_env); config-reference sandbox flag row + subsection retitled; db-network-isolation framing broadened to postgres/redis/mongo. preconditions-and-rejections left untouched (its hit was an unrelated gateway see-also link). * test(e2e): harden umbrella close terminal reads with bounded wait-for-state The MegaTask umbrella close test flaked once on CI (ceo-approve returned 200 but the re-fetch saw awaiting_pm_review) then passed on re-run. The production path is deterministic: complete -> main_pm_complete -> submit_pm_review -> escalate_to_ceo -> ceo_approve -> commit, all on one session, all awaited; the fire-and-forget completion hooks are isolated (own session, best-effort, never touch task.status or the request session). 20 local runs could not reproduce it. The one real surface is the read pattern: the e2e stack commits on the uvicorn thread's loop and reads via a separate loop (run_db -> asyncio.run with a fresh engine), so a terminal single point-read can race a still-draining completion hook on a contended runner. Replace the two terminal point-reads with a bounded wait_for_status poll. Strictly better than a one-shot read: absorbs the transient, and a genuine state bug still surfaces via the timeout branch asserting against the last-read state. * fix(gateway): bound hung flow-verbs with a server-side timeout A gateway intent-verb whose request transaction held the SELECT ... FOR UPDATE lock on the task row never committed: uvicorn does not cancel the endpoint coroutine on client disconnect and get_db only rolled back on Exception (not a hang/cancellation), so the row lock was held indefinitely and every later task-row write on that task wedged (2026-07-07 kimi-k2.7-code:cloud agent on task 79d686f0). Reads (evidence) and journal writes (note) stayed fast — the symptom that pointed at a task-row lock. Fix: pure-ASGI FlowVerbTimeoutMiddleware wraps each /api/v1/flow/* request in asyncio.timeout(flow_verb_timeout_seconds, default 120s). On expiry the inner app is cancelled; CancelledError now propagates through get_db (which catches it alongside Exception and rolls back), releasing the FOR UPDATE lock, and a retryable 504 gateway_timeout envelope is returned. Pure ASGI (not BaseHTTPMiddleware) so cancellation reaches the route coroutine + get_db dependency directly, with no spawned-task gap. Registered innermost so correlation + logging still wrap the 504. E2E: two fault-injection scenarios in tests/e2e_smoke/test_flow_verb_timeout.py. A hang is injected inside the verb's own transaction (claim acquires the FOR UPDATE lock, then set_plan sleeps past the timeout; only the first set_plan call runs — a retry short-circuits as idempotent re-entry). - ARMED (server timeout 1s): verb-1 returns a bounded 504 gateway_timeout, verb-2 re-acquires the row and reaches the post-claim gate (tracing_gap) — proving the lock was released by verb-1's cancellation. - DISARMED (server timeout 1000s, MCP client timeout 3s): verb-1 holds the lock past the client's HTTP timeout — the empirical reproduction of the wedge on the same branch, by turning the fix off. Full e2e suite green (32 passed). * feat(sandbox): pluggable per-engine registry (postgres/redis/mongo) (#324) (#325) * feat(sandbox): pluggable per-engine registry (postgres/redis/mongo) Replaces the hardcoded postgres+redis branches in the provisioner and the env emitter with a registry of SandboxEngine specs (image, run args, readiness probe, connection, ROBOCO_TEST_* env) in a pure low module (roboco/models/sandbox.py). VALID_SANDBOX_SERVICES is derived from the registry — single source of truth — and the provisioner + orchestrator iterate it, so adding an engine is one class + one registry line, not another branch. Adds a mongo:8-alpine engine (ROBOCO_TEST_MONGO_*) as the third service alongside postgres/redis. Also fixes the cold-pull loop that stranded v0.19.0 board agents with empty error strings: docker run pulled inline under a 20s deadline, so a NAS cold pull was killed, cancelled, and re-pulled from scratch forever. _ensure_image now inspects + pulls (300s) before run; provisioning errors log type+message so a bare TimeoutError no longer shows as "". Panel edit-project dialog: postgres/redis toggles -> a Set<string> multi-select driven by a SANDBOX_SERVICES catalog, so new engines appear in the UI by adding to the catalog. Tests: engine parity (allowlist==registry, unique slugs/images, no None leak in env, SandboxInfo aggregates every engine), mongo provision + env injection, plus the existing postgres/redis provision/env/spawn/janitor suite updated to the registry shape. 821 unit / 5 skip green; ruff + mypy (360 files) clean. * docs(sandbox): reflect pluggable engine registry + mongo across docs CHANGELOG (0.19.0): Added entry for the pluggable sandbox engine registry (postgres/redis/mongo) + Fixed entry for the cold-pull loop/empty-error strand that boarded v0.19.0 board agents. docs/map (9 files): sandbox subsystem blurbs, SandboxProvisioner rows, _maybe_provision_sandbox/_append_sandbox_env rows, feature-flag rows, the migration-057 row + v0.17.0 delta, and the models.md VALID_SANDBOX_SERVICES note — all retitled to DB/Redis/Mongo via the engine registry (roboco/models/sandbox.py), with the one-class-one-line extension story and the _ensure_image cold-pull fix. Production-network (roboco_data) lines left as postgres+redis — mongo is sandbox-only, not a prod service. docs/rag (3 files): sandbox-db.md rewritten around the registry (engine list, generic _provision_engine, image pre-pull, ROBOCO_TEST_DB_*/REDIS_*/MONGO_* incl. MONGO_AUTH_DB=admin, single emit_env); config-reference sandbox flag row + subsection retitled; db-network-isolation framing broadened to postgres/redis/mongo. preconditions-and-rejections left untouched (its hit was an unrelated gateway see-also link). * test(e2e): harden umbrella close terminal reads with bounded wait-for-state The MegaTask umbrella close test flaked once on CI (ceo-approve returned 200 but the re-fetch saw awaiting_pm_review) then passed on re-run. The production path is deterministic: complete -> main_pm_complete -> submit_pm_review -> escalate_to_ceo -> ceo_approve -> commit, all on one session, all awaited; the fire-and-forget completion hooks are isolated (own session, best-effort, never touch task.status or the request session). 20 local runs could not reproduce it. The one real surface is the read pattern: the e2e stack commits on the uvicorn thread's loop and reads via a separate loop (run_db -> asyncio.run with a fresh engine), so a terminal single point-read can race a still-draining completion hook on a contended runner. Replace the two terminal point-reads with a bounded wait_for_status poll. Strictly better than a one-shot read: absorbs the transient, and a genuine state bug still surfaces via the timeout branch asserting against the last-read state. --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com> --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
Renzo F
@@ -0,0 +1,240 @@
|
||||
"""Scenario: a hung gateway flow-verb must release its task-row FOR UPDATE
|
||||
lock within a bounded server-side timeout — not hold it forever.
|
||||
|
||||
Reproduces the 2026-07-07 wedge (a ``kimi-k2.7-code:cloud`` agent on task
|
||||
79d686f0): a verb whose request transaction held the ``SELECT ... FOR
|
||||
UPDATE`` lock on the task row never committed. uvicorn does not cancel the
|
||||
endpoint coroutine on client disconnect, and ``get_db`` only rolled back on
|
||||
``Exception`` (not a hang), so the row lock was held indefinitely — every
|
||||
later task-row write on that task blocked on the lock and timed out, while
|
||||
plain reads (``evidence``) and journal writes (``note``, a different row)
|
||||
stayed fast. The fix is a server-side ``asyncio.timeout`` on flow verbs
|
||||
(``FlowVerbTimeoutMiddleware``): on expiry the inner app is cancelled,
|
||||
``get_db`` rolls back (releasing the lock), and a clean retryable 504
|
||||
``gateway_timeout`` envelope is returned.
|
||||
|
||||
The hang is injected INSIDE the verb's own transaction: ``claim`` acquires
|
||||
the FOR UPDATE lock, then ``set_plan`` sleeps past the server timeout. The
|
||||
sleep fires on the FIRST ``set_plan`` call only — a retry ``i_will_plan``
|
||||
short-circuits as idempotent re-entry (the umbrella scenario's claim-time
|
||||
gate dance relies on the same behavior: the composed sequence runs once,
|
||||
retries skip it), so the hang has to be on the one call that actually runs.
|
||||
|
||||
Two tests give the empirical proof:
|
||||
|
||||
- ``test_flow_verb_timeout_releases_row_lock`` (fix ARMED, server timeout
|
||||
1s): verb-1's hang is cancelled at 1s, ``get_db`` rolls back (lock
|
||||
released), a 504 ``gateway_timeout`` comes back, and verb-2 re-acquires
|
||||
the row and reaches the post-claim gate (``tracing_gap``). Reaching the
|
||||
gate is the success marker — the verb completed its composed transaction.
|
||||
- ``test_flow_verb_holds_lock_when_timeout_disarmed`` (fix DISARMED, server
|
||||
timeout 1000s + MCP client timeout 3s): verb-1's hang is NOT cancelled,
|
||||
so it holds the FOR UPDATE lock past the client's HTTP timeout — the
|
||||
empirical reproduction of the wedge on the same branch, by turning the
|
||||
fix off.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
import httpx
|
||||
from tests.e2e_smoke.arcs import (
|
||||
origin_branch,
|
||||
seed_company,
|
||||
seed_project,
|
||||
seed_task,
|
||||
set_branch_name,
|
||||
)
|
||||
from tests.e2e_smoke.harness import ScriptedAgent, expect_error
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from uuid import UUID
|
||||
|
||||
import pytest
|
||||
from tests.e2e_smoke.arcs import Company
|
||||
from tests.e2e_smoke.harness import E2EStack
|
||||
|
||||
# Pydantic's IWillPlanRequest.approach enforces >= 150 chars at the HTTP
|
||||
# boundary, so every i_will_plan call needs a compliant approach.
|
||||
_APPROACH = (
|
||||
"Plan and delegate the page-scoped refresh button work to the frontend "
|
||||
"cell: land the provider/hook, add the navbar button, remove the inline "
|
||||
"buttons, and route one planning subtask to fe-pm for delivery."
|
||||
)
|
||||
_SUB_TASKS = [
|
||||
{
|
||||
"title": "Frontend cell: refresh button",
|
||||
"description": (
|
||||
"Delegate the navbar refresh button to fe-pm: land the "
|
||||
"provider/hook and wire the click handler into the page."
|
||||
),
|
||||
}
|
||||
]
|
||||
_PLAN = "Land the refresh button via the frontend cell."
|
||||
# set_plan sleeps this long inside the verb's own transaction. On the fix
|
||||
# (1s server timeout) the sleep is cancelled well before this; disarmed
|
||||
# (1000s server timeout, 3s client timeout) the client trips first.
|
||||
_HANG_SECONDS = 8.0
|
||||
_SERVER_TIMEOUT_SECONDS = 1.0
|
||||
_DISARMED_SERVER_TIMEOUT_SECONDS = 1000.0
|
||||
# The MCP client's HTTP timeout for the disarmed reproduction — must be less
|
||||
# than _HANG_SECONDS so the client trips before the sleep ends. Applied
|
||||
# AFTER priming the per-agent flow_server reload (a pre-call patch is
|
||||
# clobbered by the reload on the first flow() call).
|
||||
_MCP_CLIENT_TIMEOUT_SECONDS = 3.0
|
||||
|
||||
|
||||
def _seed_planning_root(
|
||||
stack: E2EStack, company: Company
|
||||
) -> tuple[ScriptedAgent, UUID]:
|
||||
"""Seed a PENDING MAIN_PM planning root + its origin branch for the test."""
|
||||
from roboco.models import Team
|
||||
from roboco.models.base import TaskStatus, TaskType
|
||||
|
||||
project_id, _project_slug = seed_project(stack, company)
|
||||
main_pm = ScriptedAgent(stack, company.main_pm_id, "main-pm", "main_pm")
|
||||
task_id = seed_task(
|
||||
stack,
|
||||
title="Root: page-scoped refresh button",
|
||||
description="Frontend-only root: provider/hook + navbar button.",
|
||||
acceptance_criteria=["the refresh button lands on master"],
|
||||
task_type=TaskType.PLANNING,
|
||||
team=Team.MAIN_PM,
|
||||
project_id=project_id,
|
||||
created_by=company.main_pm_id,
|
||||
assigned_to=company.main_pm_id,
|
||||
status=TaskStatus.PENDING,
|
||||
)
|
||||
branch = f"feature/main_pm/{str(task_id)[:8]}"
|
||||
origin_branch(stack, branch, start="master")
|
||||
set_branch_name(stack, task_id, branch)
|
||||
return main_pm, task_id
|
||||
|
||||
|
||||
def _patch_hang_in_set_plan(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""Make the first ``TaskService.set_plan`` call sleep past the timeout.
|
||||
|
||||
The hang is inside the verb's own transaction: claim has already
|
||||
acquired the FOR UPDATE row lock when set_plan runs, so the sleep holds
|
||||
the lock until the transaction commits or rolls back. Only the FIRST
|
||||
call sleeps — a retry i_will_plan short-circuits as re-entry and never
|
||||
re-runs set_plan, so a per-call counter would never reach a second hang.
|
||||
"""
|
||||
from roboco.services.task import TaskService
|
||||
|
||||
real_set_plan = TaskService.set_plan
|
||||
hung = {"done": False}
|
||||
|
||||
async def hung_set_plan(
|
||||
self: TaskService, task_id: UUID, plan: str | dict[str, Any]
|
||||
) -> Any:
|
||||
if not hung["done"]:
|
||||
hung["done"] = True
|
||||
await asyncio.sleep(_HANG_SECONDS)
|
||||
return await real_set_plan(self, task_id, plan)
|
||||
|
||||
monkeypatch.setattr(TaskService, "set_plan", hung_set_plan)
|
||||
|
||||
|
||||
def test_flow_verb_timeout_releases_row_lock(
|
||||
e2e_stack: E2EStack, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Fix ARMED: a hung verb is cancelled, its lock released, retry proceeds."""
|
||||
from roboco.config import settings
|
||||
|
||||
stack = e2e_stack
|
||||
company = seed_company(stack)
|
||||
main_pm, task_id = _seed_planning_root(stack, company)
|
||||
|
||||
# Server-side verb timeout: 1s. A hung verb is cancelled and its
|
||||
# transaction rolled back at this boundary instead of holding the FOR
|
||||
# UPDATE lock forever.
|
||||
monkeypatch.setattr(settings, "flow_verb_timeout_seconds", _SERVER_TIMEOUT_SECONDS)
|
||||
_patch_hang_in_set_plan(monkeypatch)
|
||||
|
||||
# 1. verb-1: claim -> FOR UPDATE lock -> set_plan HANGS. The server
|
||||
# cancels at 1s, get_db rolls back (releasing the lock), and a 504
|
||||
# gateway_timeout envelope comes back within the default client
|
||||
# window (the fix makes the hang bounded; no client-side race).
|
||||
env1 = main_pm.flow(
|
||||
"i_will_plan",
|
||||
task_id=str(task_id),
|
||||
plan=_PLAN,
|
||||
approach=_APPROACH,
|
||||
sub_tasks=_SUB_TASKS,
|
||||
)
|
||||
expect_error(env1, "gateway_timeout", "verb-1 must return a bounded 504, not hang")
|
||||
|
||||
# 2. verb-2: the task was rolled back to PENDING by verb-1's
|
||||
# cancellation, so claim re-enters and acquires the row (only
|
||||
# possible because verb-1 released the lock). set_plan runs for
|
||||
# real, start commits, and the post-claim tracing gate fires (no
|
||||
# decision note) -> tracing_gap. Reaching the gate is the success
|
||||
# marker: on master the row would still be locked by verb-1's stuck
|
||||
# coroutine and this claim would hang on the FOR UPDATE.
|
||||
env2 = main_pm.flow(
|
||||
"i_will_plan",
|
||||
task_id=str(task_id),
|
||||
plan=_PLAN,
|
||||
approach=_APPROACH,
|
||||
sub_tasks=_SUB_TASKS,
|
||||
)
|
||||
expect_error(
|
||||
env2,
|
||||
"tracing_gap",
|
||||
"verb-2 reaches the post-claim gate — the row lock was released",
|
||||
)
|
||||
|
||||
|
||||
def test_flow_verb_holds_lock_when_timeout_disarmed(
|
||||
e2e_stack: E2EStack, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Fix DISARMED: with no bounded server timeout, the hung verb holds the
|
||||
lock past the client's HTTP timeout — the empirical reproduction of the
|
||||
2026-07-07 wedge on the same branch, by turning the fix off.
|
||||
"""
|
||||
from roboco.config import settings
|
||||
from roboco.mcp import flow_server
|
||||
|
||||
stack = e2e_stack
|
||||
company = seed_company(stack)
|
||||
main_pm, task_id = _seed_planning_root(stack, company)
|
||||
|
||||
# Disarm the server-side timeout: 1000s. The hung verb is NOT cancelled
|
||||
# by the server, so it holds the FOR UPDATE lock for the full sleep.
|
||||
monkeypatch.setattr(
|
||||
settings, "flow_verb_timeout_seconds", _DISARMED_SERVER_TIMEOUT_SECONDS
|
||||
)
|
||||
_patch_hang_in_set_plan(monkeypatch)
|
||||
|
||||
# Prime the per-agent flow_server reload with a no-op verb (i_am_idle
|
||||
# touches no task). The reload resets module globals (_TIMEOUT=30), so a
|
||||
# pre-call patch would be clobbered; after this call the module is
|
||||
# pinned to this agent and the patch below survives.
|
||||
main_pm.flow("i_am_idle")
|
||||
monkeypatch.setattr(flow_server, "_TIMEOUT", _MCP_CLIENT_TIMEOUT_SECONDS)
|
||||
|
||||
# verb-1: claim -> FOR UPDATE lock -> set_plan HANGS. The server does
|
||||
# not cancel (1000s timeout), so the verb holds the lock past the
|
||||
# client's 3s HTTP timeout — the wedge. httpx raises, never returning
|
||||
# an envelope.
|
||||
verb1_hung = False
|
||||
try:
|
||||
main_pm.flow(
|
||||
"i_will_plan",
|
||||
task_id=str(task_id),
|
||||
plan=_PLAN,
|
||||
approach=_APPROACH,
|
||||
sub_tasks=_SUB_TASKS,
|
||||
)
|
||||
except httpx.TimeoutException:
|
||||
verb1_hung = True
|
||||
assert verb1_hung, (
|
||||
"verb-1 did NOT hang with the server timeout disarmed — the wedge is "
|
||||
"not reproduced; either the hang injection broke or the server is "
|
||||
"cancelling despite the disarmed timeout"
|
||||
)
|
||||
Reference in New Issue
Block a user