fix(serve): dispatch /rpc off the event loop - #590
Merged
Conversation
handle_request reads the whole kb off disk — seconds of work on a large one. called inline from the async endpoint it blocked the loop, so every other request queued behind it. on a 1512-claim kb a liveness probe took 8.4s while a kb.status was in flight, and the console reported the endpoint unreachable while it was answering fine. dispatch now goes through a worker thread, the same way the /mcp surface already runs its sync tools. the actor and trust contextvars move inside the worker so two concurrent calls can't overwrite each other's caller identity. also stops the console serving its spa shell without a cache-control. index.html names the hashed bundle, so with only last-modified a browser applies heuristic freshness and keeps replaying the shell it cached before — after an upgrade that means the previous build running against the new backend, which crashed the render tree once the list envelope changed shape. the shell is no-cache now (still a 304 off the etag); assets/ is content-hashed and stays immutable.
github-actions
Bot
added
review-ui
WalkthroughThe ChangesHTTP serving behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant Threadpool
participant JSONLServer
Client->>HTTPServer: POST /rpc
HTTPServer->>Threadpool: Dispatch synchronous RPC work
Threadpool->>JSONLServer: handle_request(envelope)
JSONLServer-->>Threadpool: Return RPC response
Threadpool-->>HTTPServer: Return result
HTTPServer-->>Client: Send HTTP response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
ci: passing
…o fix/rpc-event-loop-blocking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/test_console.py (1)
115-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wincover the complete cache contract.
These tests only check cache-control substrings. They do not prove that an etag is emitted and that a conditional shell request returns
304./assets/app.jsis also not a content-hashed filename, so the immutable assertion does not exercise the stated hashed-asset boundary. Add a real hashed fixture and conditional-request assertion, or confirm equivalent coverage exists elsewhere in this test file.example conditional-request assertion
for path in ("/", "/review"): res = client.get(path) assert res.status_code == 200 assert "no-cache" in res.headers.get("cache-control", ""), path + if path == "/": + etag = res.headers["etag"] + assert client.get( + path, headers={"if-none-match": etag} + ).status_code == 304🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_console.py` around lines 115 - 137, Expand the cache-contract tests around test_index_is_revalidated_on_every_load and test_hashed_assets_stay_cacheable to assert that the shell response emits an ETag and that a subsequent request with If-None-Match returns 304. Replace the /assets/app.js fixture with a genuinely content-hashed asset filename and verify that asset retains immutable caching, or reuse equivalent existing coverage in this test file.tests/test_http_server.py (1)
240-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSleep-based synchronization risks flakiness; also no coverage for actor-identity isolation.
Two observations on this new test:
time.sleep(0.2)at Line 268 to "let the slow call reach the handler" plus theelapsed < 0.5bound at Line 278 is timing-dependent — under CI contention this can intermittently fail (or pass without actually exercising concurrency, if the RPC thread hasn't reached the handler yet). Athreading.Eventset insideslow()right before thesleep(1.0)would let the test wait deterministically for the slow call to start before hitting/health, and would be more robust than a fixed delay.- The PR objective states actor/trust context is preserved "across concurrent requests," but this test only asserts non-blocking behavior — it doesn't verify that concurrent
/rpccalls with differentX-Vouch-Agent/bearer values keep their actor identity isolated. Given_dispatchis the load-bearing mechanism for that guarantee, a dedicated test (e.g., two concurrent calls with different agents, recordingjsonl_server._actor.get()inside a patched handler) would directly cover the claim.♻️ Sketch for synchronized start instead of fixed sleep
import time real = http_server.jsonl_server.handle_request + started = threading.Event() + def slow(envelope: dict) -> dict: + started.set() time.sleep(1.0) return real(envelope) monkeypatch.setattr(http_server.jsonl_server, "handle_request", slow) gen = _serve(make_server("127.0.0.1", 0)) url = next(gen) try: rpc = threading.Thread( target=_post, args=(url, {"id": "slow", "method": "kb.status"}), daemon=True ) rpc.start() - time.sleep(0.2) # let the slow call reach the handler + assert started.wait(timeout=5), "slow rpc never reached the handler" start = time.monotonic()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_http_server.py` around lines 240 - 278, Make test_slow_rpc_does_not_block_other_requests deterministic by adding a threading.Event that slow() sets before sleeping, then wait on that event instead of using time.sleep(0.2) to coordinate startup while preserving the liveness assertion. Add a separate concurrent RPC test covering _dispatch actor isolation: issue requests with distinct X-Vouch-Agent or bearer identities, record jsonl_server._actor.get() inside the patched handler, and assert each request observes only its own actor context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/http_server.py`:
- Around line 205-229: Serialize `/rpc` and `/mcp` worker execution for mutating
approval requests so concurrent handlers cannot interleave read-then-write
operations. Extend the existing synchronization around `handle_request` and the
approval flow to cover `store.put_*`/`update_*`, `move_proposal_to_decided()`,
and audit indexing, rather than only `log_event`; preserve concurrent handling
for non-mutating requests where possible.
In `@src/vouch/web/console.py`:
- Around line 69-75: Update the added comment block above the SPA shell caching
logic to use lowercase prose throughout, while preserving literal protocol and
technical names such as SPA, Last-Modified, browser, ETag, 304, and Vite as
required.
- Around line 80-81: Update the cache-header decision in the request path
handling to classify the resolved candidate rather than the raw rel value: after
the traversal check, derive the root-relative POSIX path with
candidate.relative_to(root).as_posix() and pass that value to _cache_headers.
Add a regression test covering an in-root dot-segment path such as
/assets/../index.html, ensuring it receives shell caching.
---
Nitpick comments:
In `@tests/test_console.py`:
- Around line 115-137: Expand the cache-contract tests around
test_index_is_revalidated_on_every_load and test_hashed_assets_stay_cacheable to
assert that the shell response emits an ETag and that a subsequent request with
If-None-Match returns 304. Replace the /assets/app.js fixture with a genuinely
content-hashed asset filename and verify that asset retains immutable caching,
or reuse equivalent existing coverage in this test file.
In `@tests/test_http_server.py`:
- Around line 240-278: Make test_slow_rpc_does_not_block_other_requests
deterministic by adding a threading.Event that slow() sets before sleeping, then
wait on that event instead of using time.sleep(0.2) to coordinate startup while
preserving the liveness assertion. Add a separate concurrent RPC test covering
_dispatch actor isolation: issue requests with distinct X-Vouch-Agent or bearer
identities, record jsonl_server._actor.get() inside the patched handler, and
assert each request observes only its own actor context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81eb550c-0810-448f-843c-64563f2fe17c
📒 Files selected for processing (4)
src/vouch/http_server.pysrc/vouch/web/console.pytests/test_console.pytests/test_http_server.py
Comment on lines
205
to
+229
| bearer = trust_mod.matched_bearer_token( | ||
| request.headers.get("authorization"), | ||
| tuple(getattr(request.app.state, "vouch_bearer_tokens", ()) or ()), | ||
| ) | ||
| trust = trust_mod.with_auth_subject(trust_mod.JSONL_HTTP, bearer) | ||
| try: | ||
| with trust_mod.trust_context(trust): | ||
| response = jsonl_server.handle_request(envelope) | ||
| finally: | ||
| if reset is not None: | ||
| jsonl_server._actor.reset(reset) | ||
|
|
||
| def _dispatch() -> dict[str, Any]: | ||
| # The actor and the trust marker are both ContextVars. They're set | ||
| # here, inside the worker, so each request mutates only its own copy | ||
| # of the context — set on the event loop instead, two concurrent | ||
| # calls would overwrite each other's caller identity. | ||
| reset = jsonl_server._actor.set(agent) if agent else None | ||
| try: | ||
| with trust_mod.trust_context(trust): | ||
| return jsonl_server.handle_request(envelope) | ||
| finally: | ||
| if reset is not None: | ||
| jsonl_server._actor.reset(reset) | ||
|
|
||
| # handle_request is synchronous and reads the whole KB off disk — seconds | ||
| # of work on a large one. Called inline it blocks the event loop, so every | ||
| # other request (health probes included) queues behind it and clients see | ||
| # the endpoint as hung. The /mcp surface already dispatches its sync tools | ||
| # through a worker thread; this keeps /rpc consistent with it. | ||
| response = await run_in_threadpool(_dispatch) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing locking/serialization around KB writes now that
# handle_request can run concurrently on real threads.
rg -n 'Lock\(|RLock\(|Semaphore\(|filelock|fcntl|with.*lock' src/vouch --type py -C2Repository: vouchdev/vouch
Length of output: 10649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== affected file outline =="
ast-grep outline src/vouch/http_server.py --view compact || true
echo "== http_server relevant lines =="
sed -n '170,245p' src/vouch/http_server.py
echo "== storage lock import/usages =="
sed -n '565,620p' src/vouch/storage.py
sed -n '480,510p' src/vouch/storage.py
echo "== audit lock definition/usages =="
sed -n '40,75p' src/vouch/audit.py
sed -n '140,165p' src/vouch/audit.py
echo "== storage write path candidates =="
rg -n 'def (create|update|delete|load|save|write|read|jsonl|append|replace|json)|open\(|Path\.|# lock|with .*lock|fsync|flush' src/vouch/storage.py src/vouch/audit.py src/vouch/jsonl_server.py src/vouch/proposals.py -g '*.py' -C2Repository: vouchdev/vouch
Length of output: 20987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== proposals lock/content =="
sed -n '1,260p' src/vouch/proposals.py
rg -n 'lock|with|open\("x"|Path\.read|Path\.write|mcp|lockfile|fcntl|LOCK_EX' src/vouch/proposals.py -C2
echo "== jsonl_server handle_request outline and file storage calls =="
ast-grep outline src/vouch/jsonl_server.py --view names | sed -n '1,220p' || true
rg -n 'def handle_request|proposals\.approve|storage\.update_|storage\.put_|storage\.delete_|storage\.' src/vouch/jsonl_server.py src/vouch/proposals.py src/vouch/storage.py -C3 | sed -n '1,260p'
echo "== anyio version/deps if present =="
if [ -f pyproject.toml ]; then rg -n 'anyio|uvicorn|starlette' pyproject.toml p -C1 || true; fiRepository: vouchdev/vouch
Length of output: 20500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jsonl_server handle_request implementation =="
sed -n '945,1010p' src/vouch/jsonl_server.py
echo "== http imports for run_in_threadpool and thread pool settings =="
rg -n 'from stdlib|run_in_threadpool|threadpool|workers|limit_workers|_rpc|FastAPI' src/vouch/http_server.py src/vouch --type py -g '*.py' -C2
echo "== storage file operations around durable writes =="
sed -n '700,905p' src/vouch/storage.py
sed -n '1030,1145p' src/vouch/storage.py
sed -n '1,220p' src/vouch/storage.py | sed -n '160,215p'
echo "== deterministic shape from source =="
python3 - <<'PY'
from pathlib import Path
for path in ['src/vouch/storage.py','src/vouch/proposals.py','src/vouch/jsonl_server.py','src/vouch/http_server.py','src/vouch/audit.py']:
p=Path(path)
text=p.read_text()
print(f"\n{path}: runs_on_event_loop_line={any('run in_threadpool' in line for line in text.splitlines())}, has_lock_words={any(w in text for w in ['Lock(', 'RLock(', 'Semaphore(', 'fcntl.flock', 'msvcrt.locking', 'with _audit_lock'])}")
PYRepository: vouchdev/vouch
Length of output: 31179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== approve implementation relevant sections =="
sed -n '760,970p' src/vouch/proposals.py
sed -n '995,1050p' src/vouch/proposals.py
sed -n '1220,1235p' src/vouch/proposals.py
echo "== storage delete/update path coverage =="
sed -n '1145,1235p' src/vouch/storage.py
sed -n '938,1038p' src/vouch/storage.py
echo "== candidate tests around MCP sync or /rpc concurrency =="
rg -n 'run_in_threadpool|handle_request|kb\.approve|proposals\.approve|thread|concurrent' tests src/tests tests --type py -C2 | sed -n '1,240p' || trueRepository: vouchdev/vouch
Length of output: 32128
Serialize or lock KB mutations before offloading /rpc to threadpool workers.
handle_request now calls synchronous MCP handlers from real worker threads, and /rpc dispatches the same handlers. The approval flow performs read-then-write checks around store.put_*/update_*, move_proposal_to_decided(), and audit indexing without a cross-thread/global guard, so concurrent /mcp or /rpc writes can interleave and leave artifacts/audit state inconsistent. The existing lock is only around log_event; it should cover the full mutating approval write path or the workers should be serialized at the request boundary before the storage layer changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/http_server.py` around lines 205 - 229, Serialize `/rpc` and `/mcp`
worker execution for mutating approval requests so concurrent handlers cannot
interleave read-then-write operations. Extend the existing synchronization
around `handle_request` and the approval flow to cover `store.put_*`/`update_*`,
`move_proposal_to_decided()`, and audit indexing, rather than only `log_event`;
preserve concurrent handling for non-mutating requests where possible.
Source: Path instructions
Comment on lines
+69
to
+75
| # The SPA shell names the hashed bundle, so it must be revalidated on every | ||
| # load. Served with only Last-Modified a browser applies heuristic freshness | ||
| # and keeps replaying the shell it cached before — after an upgrade that means | ||
| # the previous console build running against the new backend, which fails in | ||
| # whatever way the response shapes changed. `no-cache` still allows a 304 off | ||
| # the ETag, so the cost is one conditional request. Everything vite emits into | ||
| # assets/ is content-hashed and can be kept forever. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
keep the added comment prose lowercase.
The new comment block uses sentence case and uppercase prose. Please lowercase it while preserving literal protocol names where necessary.
As per path instructions, “use lowercase prose in comments and review notes.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/web/console.py` around lines 69 - 75, Update the added comment
block above the SPA shell caching logic to use lowercase prose throughout, while
preserving literal protocol and technical names such as SPA, Last-Modified,
browser, ETag, 304, and Vite as required.
Source: Path instructions
Comment on lines
+80
to
+81
| def _cache_headers(rel: str) -> dict[str, str]: | ||
| return _ASSET_CACHE if rel.startswith("assets/") else _SHELL_CACHE |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | grep -E '(^|/)console\.py$|AGENTS\.md|CLAUDE\.md|pytest|requirements|pyproject|README' | sed -n '1,200p'
echo "== console outline =="
ast-grep outline src/vouch/web/console.py --view expanded || true
echo "== relevant console sections =="
cat -n src/vouch/web/console.py | sed -n '1,120p'
printf '\n--- 180-215 ---\n'
cat -n src/vouch/web/console.py | sed -n '180,215p'
echo "== tests mentioning console/static/assets/cache =="
rg -n "_cache_headers|assets/|static|FileResponse|index.html|dot|\\./|\\.\\." tests src/vouch -S || trueRepository: vouchdev/vouch
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test_console relevant sections =="
cat -n tests/test_console.py | sed -n '88,155p'
printf '\n--- tests mentioning console path params or console app ---\n'
rg -n "build_console_app|full_path|assets/|index\\.html|dot|\\./|\\.\\." tests/test_console.py -S
echo "== behavioral probe: path normalization in current logic =="
python3 - <<'PY'
from pathlib import Path
root = Path("/tmp/webapp/dist")
index = root / "index.html"
for rel in ("assets/app.js", "../index.html", "assets/../index.html"):
candidate = (root / rel).resolve()
try:
resolved_rel = candidate.relative_to(root).as_posix()
except ValueError:
resolved_rel = "(rejected)"
print(f"{rel:25} candidate={candidate} file={candidate.is_file()} rel_prefix={rel.startswith('assets/')} resolved_rel={resolved_rel}")
PYRepository: vouchdev/vouch
Length of output: 5415
classify the resolved path, not the raw request path.
An in-root alias such as /assets/../index.html resolves to the SPA shell, but rel still starts with assets/, so the shell can receive immutable asset caching. Use candidate.relative_to(root).as_posix() for the cache header decision after the traversal check, and add a regression test for dot-segment paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/web/console.py` around lines 80 - 81, Update the cache-header
decision in the request path handling to classify the resolved candidate rather
than the raw rel value: after the traversal check, derive the root-relative
POSIX path with candidate.relative_to(root).as_posix() and pass that value to
_cache_headers. Add a regression test covering an in-root dot-segment path such
as /assets/../index.html, ensuring it receives shell caching.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
handle_request reads the whole kb off disk — seconds of work on a large one. called inline from the async endpoint it blocked the loop, so every other request queued behind it. on a 1512-claim kb a liveness probe took 8.4s while a kb.status was in flight, and the console reported the endpoint unreachable while it was answering fine.
dispatch now goes through a worker thread, the same way the /mcp surface already runs its sync tools. the actor and trust contextvars move inside the worker so two concurrent calls can't overwrite each other's caller identity.
also stops the console serving its spa shell without a cache-control. index.html names the hashed bundle, so with only last-modified a browser applies heuristic freshness and keeps replaying the shell it cached before — after an upgrade that means the previous build running against the new backend, which crashed the render tree once the list envelope changed shape. the shell is no-cache now (still a 304 off the etag); assets/ is content-hashed and stays immutable.
What changed
Why
What might break
VEP
Tests
make checkpasses locally (lint + mypy + pytest)CHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
Performance
Bug Fixes
Tests