fix(#904): no false 'token expired' on SIGKILL/OOM + backend call budget (RC-1 + RC-2 + RC-3) - #907
Conversation
|
Testing on my local instance |
dolho
changed the title
…cceeds" + break deadlocks Review feedback on #907: the original 30s queue-acquire timeout would turn calls that previously eventually succeeded into new HTTP 503s under sustained backlog. That's a regression we don't want. Trade-off matrix: * No timeout (wait forever): zero false 503s but agent-to-agent chat chains (chat_with_agent MCP tool, X→Y→Z collaborations) can deadlock when concurrent chain depth exceeds the global semaphore. Each chain holds a slot for its outer caller while waiting on the next hop, which itself wants a slot. With cap=8 and >8 deep parallel chains the system hangs forever. * Short timeout (30s): no deadlocks but every long-tail call near the cap risks 503. * Long timeout (3600s = platform max execution_timeout): pre-#904 worst-case wall-clock was the agent's ~610s HTTP timeout, so 3600s leaves a 6x margin for any task that would have succeeded; deadlocks surface as 503s within an hour and the queue drains. ← chosen. Change: * Default `BACKEND_AGENT_CALL_QUEUE_TIMEOUT_S` 30s → 3600s. * Setting the env var to 0 disables the cap entirely (opt-in "wait forever" — accepts deadlock risk for zero false 503s). Implemented as a fast path that skips `asyncio.wait_for` when the timeout is 0, plus a one-shot "queued > 5s" warning so operators see sustained pressure in Vector logs without spamming. * Test added: `test_default_timeout_is_one_hour` asserts the production default; `test_timeout_zero_opt_in_waits_indefinitely` pins the opt-in behavior. * requirements.md + docker-compose.yml updated to explain both the new default and the deadlock-safety-valve rationale. Related to #904 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
/review reportBranch: Critical Findings (block merge)None. SQL, race conditions, auth boundaries, credential exposure — all clean. No new endpoints; the new exception class flows cleanly through existing FastAPI handlers via the dedicated except branches. Informational Findings (review recommended)[I1] String coupling — Real-world hit rate is low (Anthropic API messages don't typically contain "oom"), but the failure mode — "auto-switch silently fails to fire when it should" — is harder to debug than a false-positive. [I2] Stale comment in [I3] Docstring drift — module top doc still says default 30s [I4] [I5] [I6] Test gap — no coverage for the false-positive case in [I1] [I7] Drift risk — Clean Categories
Summary
The most actionable finding is [I1] The other 6 are documentation drift, test coverage gaps, and code-organization nits — useful to address but not landing blockers. Generated by |
vybe
left a comment
vybe
left a comment
There was a problem hiding this comment.
Validated via /validate-pr. All CI checks green, 21 new unit tests, live e2e verified, clean security scan. Minor doc nits (stale docstrings in agent_call_limiter.py, architecture.md services list) noted but non-blocking — fixable in follow-up.
Issue #904: when an agent container's cgroup OOM-killer fires (or any external SIGKILL / SIGTERM / schedule timeout hits the claude subprocess), the resulting failure was misreported as "Subscription token may be expired or revoked." That false signal then tripped SUB-003 substring matchers in `services/subscription_auto_switch.is_auth_failure` and `src/scheduler/service._is_auth_failure`, firing a futile subscription auto-switch and burning the 2-hour skip-list slot for the alternative — exactly when no auto-switch can help (the new sub has the same memory limit too). Three layered fixes, smallest scope per the issue's "RC-2 + RC-3 first": 1. **Wire `_classify_signal_exit` into the chat path.** The headless executor was already correct (#516) but `claude_code.py:450` did not call the signal classifier — every OOM kill on `/api/chat` fell through to `_diagnose_exit_failure`'s "token expired" diagnostic. Now the chat path classifies SIGKILL/SIGTERM/SIGINT first and raises 504 with the explicit "Execution terminated by SIGKILL after N tool calls / M turns" detail, exactly like the headless path. 2. **Reword the diagnostic surfaces that fed the false positive.** - `headless_executor.py:707` zero-tokens fallback no longer says "possible authentication issue". The dedicated `is_auth_failure` 503 raised on a confirmed auth-pattern match a few lines above remains the only path that surfaces the auth phrasing. - `error_classifier._diagnose_exit_failure` (the OAuth-without-API-key branch, line 155) no longer returns the bare "Subscription token may be expired or revoked. Generate a new one with 'claude setup-token'." The new wording lists OOM kill / schedule timeout / container restart as the most-likely causes and carefully avoids any phrase that `_is_auth_failure_message` matches — so the result can't loop back through `headless_executor`'s auth detection. 3. **Negative markers on `is_auth_failure`.** Even if a future wording regresses, the auth-class trigger now short-circuits to False when the error message contains any unambiguous signal-kill / OOM / timeout marker (`sigkill`, `sigterm`, `exit code -9`, `exit code 137`, `out of memory`, `oom`, `memory cgroup`, `terminated by`, `killed by`, …). Same list mirrored in `src/scheduler/service.py` since the scheduler runs in a separate container and can't import from `services/subscription_auto_switch.py`. Tests: `tests/unit/test_904_sigkill_no_false_auth.py` (14 tests) — backend `is_auth_failure` + scheduler `_is_auth_failure` negative markers, `_diagnose_exit_failure` OAuth-only branch no longer trips `_is_auth_failure_message`, `_classify_signal_exit` correctness for negative codes and shell-encoded 137, static wire-up assertion that the chat path calls the classifier BEFORE the diagnose fallback. Out of scope for this PR (issue lists them; pursue separately): - RC-1: per-agent backend in-flight call limit (worker saturation protection — architectural change, second PR). - RC-4: cgroup `memory.events` read for explicit OOM observability (additive feature, second PR). Closes part of #904 (RC-2 + RC-3 surfaces). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI lint flagged the bare `sys.modules[name] = stub` calls in `_load_backend_is_auth_failure` and `_load_scheduler_is_auth_failure`. The stubs are necessary — both `database` and `apscheduler.*` would pull in heavy initialisation (real `DatabaseManager()`, real APScheduler executors) if not stubbed before importlib's `exec_module`, blowing up the pure-function test. Adopt the sanctioned `_STUBBED_MODULE_NAMES` + autouse `_restore_sys_modules` pattern (precedent: `tests/unit/test_agent_cleanup_parity.py` from PR #765ce6) which `tests/lint_sys_modules.py:_has_stubbed_module_names_helper` explicitly whitelists. Now snapshots + restores `database`, `db_models`, and the `apscheduler.*` subtree per test. Related to #904 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…RC-1) Issue #904 RC-1 — UI freeze on slow agent. Backend's `task_execution_service.agent_post_with_retry` had no fan-out bound, so a single misbehaving agent's 11.5-min HTTP call could leave many backend coroutines `await`ing on `httpx.post` while each emitted periodic synchronous `sqlite3` calls (`db/connection.py:18` — `sqlite3.connect(timeout=30.0)`). Sync DB inside async coroutines stalls the event loop momentarily; with enough concurrent long-runners + writes to the same SQLite file, the writer-lock contention drove the Docker healthcheck past its 10s ceiling and the operator dashboard's parallel API fan-out queued until the offending agent was restarted by hand. This PR adds two layered semaphores around outbound agent calls, keeping the call shape (await on httpx) untouched: 1. **Per-agent semaphore**, sized to the agent's `max_parallel_tasks` (default 3 on lookup miss). Bounds fan-out per agent so one bad citizen can't dominate. 2. **Global semaphore** sized to `BACKEND_AGENT_CALL_LIMIT` env (default 8). Caps total concurrent outbound calls — the backend keeps spare async capacity for dashboard / healthcheck even when every agent is mid-call. Acquire-with-timeout (`BACKEND_AGENT_CALL_QUEUE_TIMEOUT_S`, default 30s) raises `BackendAgentCallBudgetExhausted`; both `task_execution_service.execute_task` and `routers/chat.py` get dedicated except-branches that mark the execution FAILED and (in the chat router) return HTTP 503 with the budget message. SUB-003 auto-switch does NOT fire on this path — the rejection is local to the backend, the subscription is unrelated. Files: - `src/backend/services/agent_call_limiter.py` (new) — primitives: `acquire_agent_call_slot`, `BackendAgentCallBudgetExhausted`, `_reset_for_testing` test hook - `src/backend/services/task_execution_service.py` — wrap each connect-retry attempt in `agent_post_with_retry` with the slot context manager; add dedicated except in `execute_task` - `src/backend/routers/chat.py` — same dedicated except, 503 to the caller - `docker-compose.yml` — pass both env vars to backend (commented defaults: 8 / 30) - `docs/memory/requirements.md` — §10.4.2 explaining behavior + the explicit out-of-scope note (sync→async DB is a separate refactor) Verified live on local instance with `BACKEND_AGENT_CALL_LIMIT=2` and a sleeper-shim agent (replaces `/usr/bin/claude` with a `sleep 300`): 3 concurrent /api/chat calls; 2 acquired immediately, the 3rd returned HTTP 503 in 2176ms with detail "Backend call budget exhausted for rc1-test after 2001ms (agent_cap=3, global_cap=2)". Dashboard /health stayed responsive. Out of scope (separate follow-up): - True sync→async-DB migration (`run_in_executor`-wrapped sqlite3 or `aiosqlite`). The semaphore reduces contention but doesn't eliminate it. - RC-4 cgroup OOM observability. Related to #904 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cceeds" + break deadlocks Review feedback on #907: the original 30s queue-acquire timeout would turn calls that previously eventually succeeded into new HTTP 503s under sustained backlog. That's a regression we don't want. Trade-off matrix: * No timeout (wait forever): zero false 503s but agent-to-agent chat chains (chat_with_agent MCP tool, X→Y→Z collaborations) can deadlock when concurrent chain depth exceeds the global semaphore. Each chain holds a slot for its outer caller while waiting on the next hop, which itself wants a slot. With cap=8 and >8 deep parallel chains the system hangs forever. * Short timeout (30s): no deadlocks but every long-tail call near the cap risks 503. * Long timeout (3600s = platform max execution_timeout): pre-#904 worst-case wall-clock was the agent's ~610s HTTP timeout, so 3600s leaves a 6x margin for any task that would have succeeded; deadlocks surface as 503s within an hour and the queue drains. ← chosen. Change: * Default `BACKEND_AGENT_CALL_QUEUE_TIMEOUT_S` 30s → 3600s. * Setting the env var to 0 disables the cap entirely (opt-in "wait forever" — accepts deadlock risk for zero false 503s). Implemented as a fast path that skips `asyncio.wait_for` when the timeout is 0, plus a one-shot "queued > 5s" warning so operators see sustained pressure in Vector logs without spamming. * Test added: `test_default_timeout_is_one_hour` asserts the production default; `test_timeout_zero_opt_in_waits_indefinitely` pins the opt-in behavior. * requirements.md + docker-compose.yml updated to explain both the new default and the deadlock-safety-valve rationale. Related to #904 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3d93ada to
4206351
Compare
2 participants
Summary
Closes 3 of the 4 root causes from #904. RC-4 (cgroup OOM observability) remains for a follow-up. Consolidated into one PR per reviewer request — same underlying bug, layered fixes.
What's changed
_classify_signal_exitinto the chat path (claude_code.py:451), mirroringheadless_executor.py:683(bug: SIGKILL/timeout terminations of claude subprocess misclassified as authentication failure #516). OOM-on-/api/chat now raises 504 with the explicit "Execution terminated by SIGKILL after N tool calls / M turns" detail instead of "Subscription token may be expired".headless_executor.py:707zero-tokens fallback anderror_classifier._diagnose_exit_failureOAuth-only branch so neither contains any phrase matched by_is_auth_failure_message. Removes the loop-back that re-classified the false signal as 503 auth.NON_AUTH_KILL_MARKERSshort-circuits bothsubscription_auto_switch.is_auth_failureandscheduler.service._is_auth_failureonsigkill/exit code -9/oom/memory cgroup/etc. SUB-003 auto-switch no longer fires on signal kills.asyncio.Semaphorearound outbound agent HTTP calls inservices.task_execution_service.agent_post_with_retry. Per-agent bound =max_parallel_tasks(default 3); global bound =BACKEND_AGENT_CALL_LIMITenv (default 8). Acquire-with-timeout (BACKEND_AGENT_CALL_QUEUE_TIMEOUT_S, default 30s) raisesBackendAgentCallBudgetExhausted→ translated to 503 in chat router, FAILED execution row inexecute_task. SUB-003 NOT triggered on this path.Test plan
test_904_sigkill_no_false_auth.py(RC-2 + RC-3 surfaces) — all passtest_904_agent_call_limiter.py(RC-1) — all passtask_execution,capacity,subscription,chat_router,backlog,error_classification,signal_exit_classification,subscription_auto_switch_pingpong,pipe_close_no_auto_switch)tests/lint_sys_modules.py) — both new test files use sanctioned_STUBBED_MODULE_NAMES+_restore_sys_modulespatternLocal verification (e2e)
/usr/bin/claudewith a 50MB/iter Python memory-bomb in a 384MB-capped container./sys/fs/cgroup/memory.events:oom_killadvanced 0→1; chat response was "Execution terminated by SIGKILL after 0 tool calls / 0 turns (exit code -9). Likely cause: schedule/agent timeout exceeded, OOM kill, or operator cancel...".subscription_rate_limit_eventsrows for the agent: 0.is_auth_failure("Execution terminated by SIGKILL ... unauthorized")→ False; real"HTTP 401 unauthorized"→ True. Same on scheduler side.BACKEND_AGENT_CALL_LIMIT=2, 2s queue timeout, 3 concurrent/api/chatto a sleeper-shim agent. Chat 1 returned HTTP 503 in 2176ms with "Backend call budget exhausted for rc1-test after 2001ms (agent_cap=3, global_cap=2)". Chats 2 & 3 held the 2 slots./healthstayed responsive throughout.Out of scope
memory.eventsread + operator-queue OOM alert. Additive; separate PR.run_in_executor-wrappedsqlite3oraiosqlite). The semaphore reduces contention but doesn't eliminate it.Closes part of #904 (RC-1 + RC-2 + RC-3 surfaces).
🤖 Generated with Claude Code