Repository navigation
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: fork-template cleanup and its forbidden cross-layer preload dependencies must be corrected.
I reviewed the full github/main...HEAD diff and the surrounding ACP lifecycle, provider factory/retry/streaming callers, configuration admission, protocol/session-resume behavior, and relevant history. I also checked backward-compatible direct-provider fallback, test changes, AGENTS.md, CONTEXT-MAP.md/CONTEXT.md terminology and layer constraints, and repository import contracts. I found two blockers inline.
Verification:
uv run pytest tests/test_acp_fork_template.py tests/test_acp_pool_idle_close.py tests/test_provider_relay.py tests/test_provider_auth_method.py tests/test_provider_catalog.py tests/test_provider_prompt_cache.py tests/test_provider_resolution_invariants.py -x: 691 passed, 104 skipped, with one fork-template subprocess-transport warning.uv run pytest tests/test_providers_factory.py tests/test_per_model_provider.py tests/test_provider_protocol.py tests/test_litellm_provider_attribution.py tests/test_litellm_provider_model_names.py tests/test_litellm_provider_model_overrides.py tests/test_litellm_provider_reasoning.py tests/test_litellm_provider_stream.py tests/test_litellm_provider_stream_end.py tests/test_litellm_provider_timeout.py tests/test_provider_stream_fallback.py -x: 575 passed.uv run lint-imports: 10 contracts kept, but the dynamic imports called out inline are invisible to this static check.- Isolated next-loop reproduction: the named fork-template test passes but prints
Loop ... is closed; the larger targeted run recordsPytestUnraisableExceptionWarning: RuntimeError: Event loop is closed.
|
Windows compatibility review — PR #1127 @ Scope: full base One Windows finding (medium), plus what already works. The relay's Unix-socket paths are unguarded on Windows, and the relay tests assume Unix socketsFiles: Mechanism: Windows's asyncio has no Evidence (native Windows 11, CPython 3.12.13): The remaining 4 failures ( Remedy direction: treat "no Unix-socket API on this host" as What already works on Windows: Verification: the command above; base comparison for the inherited four as noted. Limitations: no live host/agent pair was exercised end to end (the TCP relay path is covered by the |
090a5cd to
81b3aba
Compare
|
Windows finding (the relay's Unix-socket paths): fixed in 73dea78, in the direction the review gave.
Not verified on native Windows. Simulated on Linux by running |
|
Windows compatibility re-review — PR #1127 @ Scope: the head moved from the previously reviewed Result: the finding is fixed; no new Windows issue. Verification (native Windows 11, CPython 3.12.13, isolated worktree The skips are the correct ones: the Unix-socket-only host test and the Linux fork-template cases. Previously this run produced 13 errors ( Limitations: no live host/agent pair end to end; the full suite was not run. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the changes since 090a5cdec05d, including the fixes and tests for both findings I opened, the Windows relay fallback, the cross-event-loop lifecycle follow-up, the relevant callers and history, backward-compatible fallback behavior, and the updated CONTEXT.md layer boundaries. I also checked that the template-side tests moved with the code rather than being weakened. Both of my threads are now resolved.
Verification:
uv run pytest tests/test_acp_fork_template.py tests/test_cli_fork_template.py tests/test_acp_pool_idle_close.py tests/test_provider_relay.py -W error::pytest.PytestUnraisableExceptionWarning -x: 110 passed.uv run pytest tests/test_acp_fork_template.py tests/test_cli_fork_template.py tests/test_acp_pool_idle_close.py tests/test_provider_relay.py tests/test_provider_auth_method.py tests/test_provider_catalog.py tests/test_provider_prompt_cache.py tests/test_provider_resolution_invariants.py -x: 700 passed, 104 skipped.uv run lint-imports: 10 contracts kept.uv run ty check raven/acp_client/fork_template.py raven/cli/_fork_template.py raven/providers/relay.py raven/providers/relay_server.py: all checks passed.
|
Not a blocker - non-blocking items from acceptance on Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text.
|
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: idle close must preserve resident scheduled work, and relayed streams must size output under the agent's binding.
The two new blocking reports change my prior clean stance. I confirmed the relevant code paths: _closable treats a quiet load-capable ACP process as disposable without accounting for its in-process cron and resident services, while relayed streaming forwards the not-given max_tokens sentinel and resolves it inside the host server context rather than the agent turn context. The existing focused tests pass but do not cover either cross-boundary case.
Verification: uv run pytest tests/test_acp_pool_idle_close.py tests/test_provider_relay.py tests/test_core_cron_stack.py tests/test_cron_service_wake.py -x — 118 passed. I am not duplicating the other reviewer's findings or re-grading their threads; this comment only updates my stance on the whole record.
|
Thanks for the acceptance pass. All five non-blocking items are fixed in this push:
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the delta from 81b3aba662ea through this head. The resident-work extension now keeps Raven ACP agents while their partition owns armed cron jobs or started plugin services, and the end-to-end test demonstrates the wake survives idle close and the process becomes closable after it fires. Relayed chat and streaming calls now carry the agent turn window, run under that binding in a fresh handler context, and no longer inherit the host turn. The additional acceptance fixes preserve invalid live-config edits, keep agent credentials out of the host environment, authenticate before reading a relay frame, avoid overlong macOS test socket paths, and confine idle closing to the connection's event loop.
Coverage this round: repository rules and CONTEXT terms, the complete post-81b3aba662ea diff, affected callers and commit history, backward compatibility for third-party ACP agents and relay fallback, test changes for weakening, and import-layer architecture. I found no weakened tests or new merge-blocking behavior.
Verification: uv run pytest tests/test_acp_pool_idle_close.py tests/test_acp_methods.py tests/test_provider_relay.py tests/test_core_cron_stack.py tests/test_cron_service_wake.py tests/test_config_live.py -x (405 passed); uv run lint-imports (10 contracts kept); focused uv run ty check ... (passed); git diff --check 81b3aba662ea..HEAD (passed).
|
Windows compatibility re-review — PR #1127 @ Scope: the delta since the previously reviewed head Result: no Windows-specific defect found.
Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun for this delta; Unix-socket transport behaviour cannot be exercised on this host; the relay tests use the scripted |
There was a problem hiding this comment.
Blocking: under the relay, a non-streamed call cannot recover from an upstream that refuses cache_control. A direct agent recovers. The suppression is learned in the agent's process, but the request is built in the host's.
Mechanism.
chat_with_retryhandles arefuses_prompt_cacheverdict by callingprompt_cache.suppress(model), stripping the marks and resending (raven/providers/base.py:796-800).prompt_cache._SUPPRESSEDis process-local (prompt_cache.py:187). Under the relay the agent'sRelayProviderdoes that suppress in the agent process.- The resend is built on the host.
relay_server._chatcalls the host'sLiteLLMProvider.chat, whose gate_supports_cache_controlreads the host's own_SUPPRESSEDand puts its marks back on the stripped retry (litellm_provider.py:312-318).strip()also drops the marks-placed claim, so loop-claimed requests are re-marked too. - The retry is refused again, the one
dropped_cache_controlis spent, a 400 is not retryable, and the call fails. LiteLLMProvider.chatnever callssuppress(onlychat_streamdoes,:499). So the host learns only if a stream happens to hit the same model first.
Reproduced (lane probe, real processes on both sides: agent RelayProvider against a real RelayServer with its real LiteLLMProvider; only acompletion faked to refuse any payload carrying cache_control, as OpenRouter does):
direct (no offer) : CHAT finish=stop content=ok wire: marked -> unmarked
relay, chat first : CHAT finish=error (BadRequestError ... cache_control is not permitted)
wire on the host: marked -> marked (host re-marked the stripped retry)
relay, stream first: ok (the stream taught the host)
base tip control : CHAT finish=stop content=ok
Production non-streamed callers include the curator (context_engine/segments/curator.py:277), the permission judge (permissions/judge.py:145), the consolidator, the heartbeat, an unstreamed turn (agent/harness/action.py:87), and #1092's unstreamed resend (turn_path.py:770/819).
Rule. This PR's own SubagentRuntimeConfig docstring (raven/config/schema.py:2453-2458): "None of it changes what an agent does". And llm_relay: "The agent builds the request, retries and records usage itself".
Fix direction: carry the agent's suppression with the call. For example, send the agent's suppressed set (or a per-call "no cache marks" flag) in the relayed request and have the host build under it. Or have the host's non-streamed path learn from the refusal the way chat_stream does. Add a cross-process test for the chat-first order.
There was a problem hiding this comment.
Fixed in 2f97829.
LiteLLMProvider.chatnow suppresses the model when it classifies a refusal ofcache_control, aschat_streamalready did. Behind the relay the host learns from the first refusal, so the agent's stripped resend goes out unmarked: the same two requests a direct agent makes. The retry stays in the agent'schat_with_retry; the host only learns.- Reproduced first on 48bb4b0 with two processes (host: a real
RelayServerandLiteLLMProvider,acompletionrefusing any payload that carriescache_control; agent:RelayProvider.chat_with_retryin its own process): directmarked -> unmarked, ok; relayed chat firstmarked -> marked, 400; relayed stream first, ok. On this head the relayed chat-first case ismarked -> unmarked, ok. - Tests, both red before the fix:
test_an_agent_recovers_from_a_cache_refusal_through_a_host_in_another_processruns the agent and the host as separate processes, chat first (the in-process fixtures share one suppression set, as the second review noted), andtest_a_bare_chat_learns_a_refusal_as_a_stream_doeschecks that a barechatlearns the refusal and that an unrelated 400 is not taken for one.
|
Not a blocker - delta acceptance on Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. Non-blocking items from this pass:
Tests here on the merged tree: the 9 changed Python test files via the board runner gave 839 passed and 144 skipped. |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: relayed non-streamed calls must preserve prompt-cache suppression across the agent and host processes.
The new blocking report changes my prior clean stance. I confirmed the mechanism in the current code: chat_with_retry() records a cache-control refusal in the agent process and strips the retry payload, but RelayServer._chat() rebuilds that request through the host process's LiteLLMProvider.chat(). Because the host has a separate suppression set, it re-adds cache marks and the one downgrade retry is refused again. This affects ordinary non-streamed relay callers and differs from direct execution.
Verification: uv run pytest tests/test_provider_relay.py tests/test_provider_prompt_cache.py -x — 129 passed. Those tests do not cover the production boundary: the in-process relay fixtures share prompt_cache._SUPPRESSED, while the reported chat-first reproduction uses separate agent and host processes. I am not duplicating or re-grading the other reviewer's finding; this comment updates my stance on the whole record.
48bb4b0 to
2f65eee
Compare
|
Thanks. On the three non-blocking items:
|
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the rebased revision by range-diffing it against the previously reviewed series and reading the four new commits plus their callers and tests. The cache-control blocker is fixed: the host's non-streamed provider now learns the refusal, and the two-process regression test proves the relayed retry changes from marked to unmarked. The new per-loop lock table, agent-specific LLM-environment fallback, and tls_verify relay construction are consistent with the current main-line provider and pool changes.
Named follow-up from the record: pooled acquire remains unaware of event-loop ownership, so a worker loop can reuse a connection owned by another loop or race a same-binding launch. The author identifies this as pre-existing and has explicitly deferred the larger pool separation change; it is nonblocking here.
Coverage this round: repository rules and CONTEXT terms, the full prior series through range-diff plus the post-review delta, affected callers and history, backward compatibility and fallback behavior, tests for weakening, and import-layer architecture. No new plain error was found.
Verification: uv run pytest tests/test_provider_prompt_cache.py tests/test_provider_relay.py tests/test_provider_tls_verify.py tests/test_acp_fork_template.py tests/test_acp_pool_idle_close.py -x (203 passed, with two LiteLLM logging coroutine warnings); uv run lint-imports (10 contracts kept); focused uv run ty check ... (passed); git diff --check github/main...HEAD (passed).
|
Windows compatibility re-review — PR #1127 @ Scope: the branch is rebased onto current main and extended; Result: no Windows-specific defect introduced by this head.
Verification on native Windows 11, detached worktree
Coverage gaps: the full suite was not rerun; the Unix transport cannot be exercised on this host; no live vendor call was made. |
|
@gloryfromca Thanks for the re-review. Your review says the suggestions are marked inline, but none came through: the review has no inline comments attached ( The pooled |
|
No blockers; suggestions only, and they are marked inline. @LivXue Confirmed: there were no inline comments and no pending review comments to post. The fixed nonblocking first-line wording was misleading in this case; the sole nonblocking item was the pooled Verification: |
…other Each product agent under agents/ ran as a whole Raven process of its own, about 240 MiB PSS, nearly all of it private: a host running the five shipped products paid about 1.2 GiB for them. Three mechanisms, each switchable live under subagents.runtime in the host's config: - idleCloseS (default 600): the ACP pool closes an agent's process once nothing is in flight either way and nothing has been said for the budget. Only an agent that advertises loadSession, or one that never opened a session, is closed, so a stateful instance's next turn reopens its session in a fresh process. A held wake is a prompt still waiting for its answer, so it is never idle. - llmRelay (default on): the host makes a Raven agent's LiteLLM calls, so the agent never imports LiteLLM. LiteLLMProvider's LiteLLM-free construction and answers move to LiteLLMShape, which RelayProvider shares; only the single chat / chat_stream attempt and the catalogue lookups that need LiteLLM's tables (the relayed decorator in rates, capabilities, common_models and the Anthropic adapter) cross a local socket with a token. Retry, classification, usage records and traces stay in the agent; the host's calls run under recorded_by_caller. An agent that cannot reach its host loads LiteLLM as before. - forkTemplate (default on, Linux only): a template process that has already imported what agents import forks each Raven agent the host launches, so the loaded pages are shared. It preloads the modules at least two products import, learned per product from the agents themselves (one product's own while only one has been seen), plus the token estimator's table. A launcher's os.execv of python -m raven runs Raven in the same process. Anything else, and any failure, launches the ordinary way. The relay and the template are per event loop, since a CLI turn is one asyncio.run. raven acp takes the relay offer out of its environment at startup, and the token estimator drops the duplicate copy of its ranks that LiteLLM's import used to drop. Measured with five product agents, one turn each (PSS): 1414 -> 724 MiB in total, the agents 1259 -> 547 MiB. One agent alone: 402 -> 371 MiB.
ty does not honour a mypy-style type: ignore on the relayed decorator's return, and the lint job failed on it with invalid-return-type. The wrapper stands in for the decorated function by construction, so it is cast to the decorated type instead of silenced.
…sockets On Windows asyncio has no open_unix_connection and socket has no AF_UNIX, so a unix: offer raised AttributeError past the relay's fallback instead of leaving the call to this process. Both connect paths now answer that case as an unreachable host, and the relay server picks loopback TCP by the same capability rather than by the platform name. The relay tests run the unix transport only where it exists.
The template process loads, by name, the modules agents import -- the CLI, the ACP server, the RPC stack, the agent loop -- and runs python -m raven, which the acp_client shelf may not do. import-linter could not see it because the edges were strings. That side moves to raven/cli/_fork_template.py, a launcher, and the host starts it through Raven's entry point (python -m raven --fork-template), which hands it the arguments. acp_client names no module of it and imports nothing of it; a test holds acp_client to no importlib or runpy loading. Going through the entry also settles the certificate store before the template imports anything. The template used to preload aiohttp, which builds its default TLS context on import, before any switch, so a forked agent kept a context that did not trust the system store. The in-process test that runs python -m raven now opts out of that switch, which had leaked truststore into the test process. The template-process tests move to tests/test_cli_fork_template.py.
…ed it A template was ended by close() or, once its loop had gone, by abandon(), which killed it but neither reaped it nor closed its transport. On a pidfd host the closed loop's watcher could no longer reap it, so it stayed a zombie, and the transport's finalizer later raised RuntimeError: Event loop is closed. A task on the template's own loop now holds it, and when asyncio.run cancels that task at the end of the run it closes the template while the loop can still reap it and close its pipes. close() waits out the group kill as well, and a close cut short by the loop's end still kills the template and closes its pipes. abandon() stays as the fallback for a loop closed some other way, and marks the transport closed so its finalizer does not run again.
…runs A host can run a second event loop beside its own: the RPC server runs a CLI command such as raven agent -m on a worker thread, under its own asyncio.run. When that loop launched a Raven agent, the launch took the first loop's template and relay for ones an ended loop had left behind and abandoned them, and every agent forked from that template died with it through the parent-death signal. Only a template or relay whose loop has stopped is abandoned now. A loop running beside them launches its agents the ordinary way and is offered the relay's socket, which the loop that owns it goes on answering.
The test that plays a misbehaving relay host bound its Unix socket under tmp_path, which on macOS is already past the 104-byte cap on a socket path, so it failed with AF_UNIX path too long. It now binds in a short directory of its own, as the relay server itself does.
… mapping subagent_runtime read anything but a mapping at subagents.runtime as the section having left the file, so an edit such as "runtime": false forgot the last valid settings and brought the defaults back: relay on, fork template on, idle close at 600 s. Only a missing section or a null brings the defaults back now. Any other value is a rejected edit, and the last admitted answer stays, as the reader's docstring says.
…ng its key The relay host built a LiteLLMProvider from the agent's construction, and the constructor wrote the agent's key into os.environ: over the host's own value for a gateway, beside it otherwise. The host's later providers could pick the agent's key up, and every process the host started after inherited it. LiteLLMShape takes export_env, and the host passes False. Every call already passes the key explicitly, and LiteLLM's chat path was checked to send it with the variables unset (zai, fireworks_ai, huggingface, openrouter).
The relay server read a whole frame, up to 512 MiB and with no deadline, before it compared the token. On loopback TCP, where any local account can connect, a peer without the token could hold connections open and make the host buffer large frames. A client now sends the token first, alone on a line, and the server reads exactly that many bytes under a five-second deadline before it reads a frame. A silent peer is dropped, and a wrong token is refused without anything more being read.
The pool kept one idle-check task and restarted it on whichever loop acquired next, and close_idle closed every connection held. When the RPC server ran a CLI command such as raven agent -m on a worker thread, that loop's check could close the main loop's agents, killing them after a stalled teardown and binding the pool's per-name locks to the worker loop. Each connection now records the loop it was made on, each loop runs its own idle check, and a check takes the locks of and closes only its own loop's connections.
A relayed chat_stream resolved max_tokens on the host, through send_max_tokens and the model binding active there. The relay server's handlers ran in a copy of the context the server was started in, so that binding was none or the host turn that first delegated, never the agent's: an agent whose loop reserved 16384 sent 32768. The agent now sends its turn's context window with each call, and the server serves each connection in a fresh contextvars.Context and makes the call under a binding with that window, so both sides size the ceiling with the same function and the same inputs. Measured across two processes with the real provider: 16384 relayed and 16384 direct. CONTEXT.md's LLM relay entry also states the token-first and environment guarantees.
Idle close judged an agent from the wire alone, so a Raven agent that had armed a cron wake or was running a plugin service looked idle and was closed after 600 s: the wake never fired, and the service died with the process. Raven agents now declare raven.residentWork in agentCapabilities._meta and answer _raven/residentWork from their own state: the enabled cron jobs of their partition while the scheduler runs, and the plugin services started. The pool asks only an agent that declared it, only once closing it is otherwise settled, and keeps it while it says yes. An agent that does not answer in time, or never declared it, is closed as before. The default stays 600 s. Resident work is a new term in CONTEXT.md, and the config field and the agent integrations page say the same.
LiteLLMProvider.chat returned a cache_control refusal as an error and left the downgrade to the retry layer, which suppresses the model in its own process. Behind the LLM relay that layer runs in the agent, while the resend it strips is built by the host's provider, which still took the model to accept marks and put them back: the one downgrade was refused again and the call failed. chat now suppresses the model on a refusal, as chat_stream already did, so the host learns from the first refusal and the resend goes out unmarked, the same two requests a direct agent makes. A test runs the agent and the host as separate processes, chat first, which in-process tests cannot show: there the two sides share one suppression set.
The pool kept one asyncio.Lock per agent name for every loop. A lock binds to the loop of its first waiter, so a worker loop's idle check (a CLI command the RPC server runs on a thread) that waited on a lock the main loop held bound it to the worker loop, and every later wait on the main loop raised that the lock was bound to a different event loop. Locks are now kept per running loop, as dag_store keeps its own, and a finished loop's table goes with it.
… the relay A relayed call is made in the host, so LiteLLM reads the host's environment for anything the provider does not pass explicitly: other endpoints and API versions, cloud credentials, proxies and CA bundles. An agent whose configured env set one of these, OPENAI_BASE_URL for instance, had it ignored once relayed. Such an agent is now launched without the relay and makes its calls itself. Only the agent's own map is compared with the host: the login shell differs from a host's in ways nobody chose for one agent, and reading those would turn the relay off for every agent. API keys are left out, since every call passes its key. CONTEXT.md and the agent integrations page say so, and CONTEXT.md's LLM relay entry also notes the host learning a prompt-cache refusal.
The per-section tls_verify option is part of a provider's construction: a section behind a self-signed gateway turns certificate checks off. The relay's spec, from which the host builds an agent's provider, lists the construction by hand and did not carry it, so a relayed agent's calls were checked on the host and refused by its gateway. The spec now carries it, and a test holds the spec to every argument the construction takes, so an argument added there later cannot be dropped the same way.
The test starts a real stub ACP process and waits out the one-second wake it arms, so most of its wall clock is idle: 1.35 s to 3.65 s depending on the machine, against the suite's 3.0 s idle ceiling, which CI's unit shards enforce strictly. A real subprocess that cannot be faked and still runs in the default selection is what the slow marker is for.
A template that did not report ready within 60 s was marked broken, so every later launch on that loop went the ordinary way, without another word, for the life of the host: one slow start on a busy machine gave up the memory the template shares until a restart. The one warning it left ended in an empty reason, since a TimeoutError prints as nothing. A missed deadline now ends that template, and launches go the ordinary way for ten minutes, at once and without a warning each, before a new one is started. The warning names the deadline and the retry. Launches that waited behind a failed start check again under the lock rather than each wait out a start of their own. A template that reports it cannot serve, or fails to start any other way, is still not used again by that loop.
2f65eee to
979c031
Compare
|
@gloryfromca Thanks for confirming. The real-wake test is now marked One more change in this push, which neither of you raised: re-running the full suite on the rebased branch, one run in six failed @0xKT The branch is now at |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
Reviewed the live head 979c0312f (the supplied worktree was still at 2f65eee953d9, so I fetched the PR head). Range-diff confirms the previously reviewed series is patch-identical after the rebase; the only new changes are the real-wake test's slow marker and the fork-template late-start recovery. The marker leaves the test in the default selection while excluding its intentional real-process wait from the strict idle ceiling. A template that misses readiness is now torn down, ordinary launches proceed quietly during a bounded rest, queued launches recheck under the lock, and a later launch retries with a fresh template. I found no new plain error.
Named nonblocking follow-up: #1152 still tracks the pre-existing pooled-acquire event-loop ownership limitation. There are no missing inline comments.
Coverage this round: repository rules and CONTEXT terms, range-diff/history, the complete new delta and callers, cleanup and fallback behavior, backward compatibility, tests for weakening, and import-layer architecture.
Verification: uv run pytest tests/test_acp_pool_idle_close.py --idle-ceiling=0.5 --idle-ceiling-strict -x (21 passed); uv run pytest tests/test_acp_fork_template.py -x (30 passed); uv run lint-imports (10 contracts kept); uv run ty check raven/acp_client/fork_template.py (passed); git diff --check github/main...HEAD (passed). An exploratory combined strict-ceiling run passed all 51 assertions but exited nonzero because four pre-existing fork-template tests exceed the artificial 0.5 s ceiling and are unmarked; the new pool marker behaved as intended.
|
Windows compatibility re-review — PR #1127 @ Scope: the branch is rebased onto main Windows angle: none for this delta, and by design for the changed path. The fork template is gated on Verification (native Windows 11; detached worktree
Coverage gaps: the fork-template rest-and-retry path cannot be exercised on this host (Linux-only), so its behaviour is verified by the Linux-skipped tests only; the relay/provider suites were not rerun because they are unchanged since the last reviewed head. No Windows-specific issue found in this delta. |
Summary
Each product agent under
agents/(Raven-Code, -Design, -Oncall, -PPT, -Research) ran as a whole Raven process of its own: about 240 MiB PSS, nearly all of it private. A host running the five products paid about 1.2 GiB for them. This lets those agents share memory with their host and with each other through three mechanisms, each switchable live undersubagents.runtimein the host's config.idleCloseS(default 600): the ACP pool closes an agent's process once nothing is in flight either way and the agent has been silent for the budget. Only an agent that advertisesloadSession, or one that never opened a session, is closed, so a stateful instance's next turn reopens its session in a fresh process. A held wake is a prompt still waiting for its answer, so it never reads as idle. Before closing a Raven agent the pool asks it (_raven/residentWork, declared inagentCapabilities._meta) whether it holds resident work, an armed cron job of its own partition or a running plugin service, and keeps it while it does; an agent that never declared it, or does not answer in time, is closed as before. Each event loop closes only the connections it made.llmRelay(default on): the host makes a Raven agent's LiteLLM calls, so the agent never imports LiteLLM (about 120 MiB of it).LiteLLMProvider's LiteLLM-free construction and answers move to a new base,LiteLLMShape, which the agent'sRelayProvidershares, so both answer every question from one copy of each rule. Only the singlechat/chat_streamattempt crosses to the host, sized under the agent's own turn window and run in a context of its own there, plus the catalogue lookups that need LiteLLM's tables (marked with therelayeddecorator inrates,capabilities,common_modelsand the Anthropic adapter). Transport: a Unix socket in a private directory (loopback TCP where the system has no Unix sockets, as on Windows) and a per-host token, presented first and under a deadline before any frame is read. Retry, classification, usage records and traces stay in the agent; the host's calls run underrecorded_by_caller, so nothing is billed twice. An agent that cannot reach its host, or is offered a Unix socket on a system without them, loads LiteLLM as before. A relayed call is made with the host's environment, so an agent whose own configuredenvsets an LLM endpoint, a cloud credential, a proxy or a CA bundle is launched without the relay; and a refusal of prompt-cache marks is learned on the host as well, since the host builds the agent's resend. A section'stls_verifyopt-out crosses with the construction, so a relayed call to a self-signed gateway is made on the host without certificate checks, as the agent's own call would be.forkTemplate(default on, Linux only): a template process that has already imported what agents import forks each Raven agent the host launches, so the loaded pages are shared. It preloads the modules at least two products import, learned per product from the agents themselves (one product's own set while only one has been seen), plus the token estimator's table. A product launcher'sos.execvofpython -m ravenruns Raven in the same process. The template ends with the host and its agents end with it. Anything else, and any failure, launches the ordinary way. A template that has not reported ready within a minute, as on a busy machine, is ended and a new one is tried ten minutes later; one that cannot serve is not used again. The template's own process is a launcher beside the CLI,raven/cli/_fork_template.py, because it loads the surfaces by name and runspython -m raven. The ACP client keeps only the host's side inraven/acp_client/fork_template.pyand starts the template through Raven's entry point (python -m raven --fork-template), which settles the certificate store before the template imports anything, so a forked agent's TLS contexts trust what a launched agent's do.Measured with a stub LLM endpoint, one turn per agent, PSS in MiB, on the branch's first version:
At this head, five agents with both on measured 735 MiB in total, with module lists learned on an earlier version. The first run after a code change measures about 794: each agent compiles the bytecode of the modules that changed. With a single agent the template costs about 40 MiB more than the relay alone: the pages an agent writes to are copied, and the originals stay in the template. Two or more agents at a time come out ahead. The defaults stay on; a device that only ever runs one agent can set
forkTemplateto false.Other changes that come with it:
asyncio.run, and a relay left by an ended loop accepts connections nobody answers. The template is closed and reaped by its own loop as that run ends, and its pipes are closed there even when the close is cut short. A loop running beside them in another thread (the RPC server runs a CLI command such asraven agent -mon a worker thread) leaves them alone: it launches its agents the ordinary way and is offered the same relay socket.raven acptakes the relay offer out of its environment at startup, so an MCP server or shell command it starts does not inherit the token.CONTEXT.md: Idle close, LLM relay, Fork template. The settings are documented on the agent integrations page, in English and Chinese.Not covered: agents on another machine, one-shot CLI hosting (
--task), and third-party ACP agents launch as before. Not measured on Windows or macOS.Pre-existing on main and not touched here: under xdist,
tests/test_agents_oncall_flow_submit_tool.pycan fail when a worker has already runtests/test_agent_plugins_ledger.py::test_load_ignores_a_file_that_is_not_an_object. The oncall duplicate check scans two levels above its ledger, which is the worker's temp root, and reads that test's list-shapedledger.json. Reproduced on main by running the two files in that order.Type
Performance (commit type
perf).Verification
pytest -q -n 4under Python 3.12 at this head, rebased ontob05267b2e: 37349 passed, 255 skipped, 8 failed. The 8 fail here only because of this machine (5 proxy tests intest_config_update_providers.pythat read the shell's proxy variables,test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for,test_knowledge_parser_excel.py::test_a_legacy_workbook_is_converted_and_read, which fails on main here too, andtest_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing, which needs a non-root user). The oncall ordering failure described above depends on how xdist distributes the files and did not occur. On9d9bec90b, before the template fix in this head, one of six full runs failed a ninth test,test_acp_fork_template.py::test_an_acp_launch_of_a_raven_agent_is_forked_and_offered_the_relay: the template wrote nothing within its 60 s ready deadline, so the agent was launched the ordinary way. The other five passed, as did the test alone (3 of 3) and with its file (28 of 28). The template was slow rather than stuck: pinned to one CPU beside two busy processes, its stack, dumped every 15 s, was still in imports after 30 s and had moved on between dumps. The failing run was the only one sharing the box with other work of mine in its first minute. It exposed a host-side flaw, fixed here: a template late to report ready was marked broken for the rest of the host's life, with an empty reason in its one warning.The tests added for the review findings, each watched failing before its fix: a template late to report ready is tried again after a ten-minute rest, launches during the rest go the ordinary way quietly, launches waiting behind the late start do not each start one, and the warning names the deadline; the relay's spec carries every argument the provider's construction takes, and a
tls_verifyopt-out reaches the provider the host builds; a relayed agent recovers from a prompt-cache refusal with the agent and the host in separate processes, chat first, and a barechatlearns the refusal; an idle check on another loop leaves an agent's lock usable here; an agent with its ownOPENAI_BASE_URLis launched without the relay while a tool's API key leaves the relay on; a Raven agent holding an armed wake is kept until the wake fires, end to end through the pool with a real stub process, and kept or closed on its_raven/residentWorkanswer; a relayed call is sized under the agent's turn window, and a relay started inside a host turn shows none of it to the call; a peer without the token is dropped before a frame is read (silent, and announcing a 256 MiB frame); building an agent's provider leaves the host's environment alone; a non-mappingsubagents.runtimekeeps the last settings; an idle check on another loop leaves this loop's connections; the bare-host test passes under a 100-character basetemp.Prompt-cache refusal reproduced across two processes (a real
RelayServerandLiteLLMProvider,acompletionrefusing any payload that carriescache_control): at48bb4b004a relayed non-streamed call went outmarked -> markedand failed with a 400 while a direct one recovered; at this head the relayed call goes outmarked -> unmarkedand succeeds, like the direct one.Relayed
max_tokensmeasured across two processes, with a realRelayServerandLiteLLMProviderandacompletionreplaced by a recorder, the agent under a turn window of 32768: 32768 relayed against 16384 direct at81b3aba66, 16384 against 16384 since48bb4b004, whether the server was started outside a turn or inside a host turn.ruff check,ruff format --check,ty checkover themake lint-typestargets,lint-imports(10 contracts kept) anddeptry raven: clean. The module import-cycle count stays at 44, as on main.npx commitlint --from origin/main --to HEAD,scripts/check_commit_messages.py origin/main..HEAD,scripts/check_large_files.py,scripts/check_source_language.py: all pass.Mutation checks, each turning its own tests red: for the late template, a missed deadline marked broken, no rest after it, no second look under the lock, a warning on every launch during the rest, and a warning without its reason; for the round before, a relay spec without
tls_verify; a barechatthat does not learn the refusal; one agent lock shared by every loop; never skipping the relay for an agent's own LLM environment, or counting a tool's API key as one. Earlier rounds: the pool never asking for resident work, the client ignoring the declaration, the agent not declaring it, counting another partition's cron jobs or a stopped scheduler's; the agent sending no window, the host ignoring it, relay handlers left in the start-time context; the server reading a frame before the token; building the host's provider with the environment written; the idle check without its loop filter; removing the per-event-loop rebuild of the template and of the relay; removing theloadSessioncondition on idle close; removingrecorded_by_calleron the host; removing the template's hold task, the wait after the group kill, or thefinallyaround the template's cleanup; removing either running-loop check; removing the no-Unix-socket guard from either connect path, or choosing TCP by platform name again; adding animportlib.import_modulecall underraven/acp_client; taking the template's route in the entry before the certificate-store switch.Windows: the owner's native Windows 11 re-reviews ran
tests/test_provider_relay.py tests/test_acp_pool_idle_close.py tests/test_acp_methods.py: 231 passed, 1 skipped at48bb4b004, and 237 passed, 1 skipped at2f65eee95, the head before the rebase ontob05267b2e(the skip is the Unix-only bare-host test), with the token-first, window-sizing, cross-loop, resident-work andtls_verifyrelay tests running. The full suite has not been run on Windows.End to end at this head: five product agents through the real pool, template and relay, against a stub OpenAI-compatible endpoint. In each of two runs all five forked and completed a turn, none was launched without the relay, and asked for resident work Raven-Oncall answered resident (one running service, its oncall-flow watcher) and the other four did not.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
Behaviour change: with the defaults, product agents launched on Linux run forked from a template and make their LiteLLM calls through the host, and an idle product agent's process is closed after 600 s and relaunched on its next turn, unless it holds an armed cron job or a running plugin service. That costs a cold start of a few seconds. A relayed call uses the host's environment: an agent's own
enventries that LiteLLM reads turn the relay off for that agent, but differences between the login shell and the host process are not detected.Security: the relay listens only in a fresh 0700 directory (loopback TCP plus a token on Windows) and serves only its token. The agent's provider construction, API key included, crosses that local socket. A connection presents the token before the host reads anything else, under a five-second deadline. The host builds an agent's provider without writing the agent's key into its own environment, so neither the host's own providers nor the processes it starts see it. Products inherit the host's LLM settings by default.
Known limits, left as they are: resident work is counted per partition, so while one instance of a product holds an armed cron job, every pooled instance of that product is kept; that holds memory longer, it does not lose a wake. The pool's
acquirestill hands a connection to whichever loop asks, without checking which loop made it, as on main; and now that each loop has its own lock, two loops launching the same agent at the same moment each start a process, and the pool keeps one. Tracked in fix(acp_client): pooled acquire ignores which event loop owns a connection #1152.Rollback: set
subagents.runtimeto{"idleCloseS": 0, "llmRelay": false, "forkTemplate": false}for today's behaviour without a restart, or revert this commit.Security impact considered
Backward compatibility considered
Rollback path is clear for risky changes
Related Issues
N/A