feat(web): address Heid code-review findings — issue #16 (v0.16.0)
Heid panel review (Gróa + Hulda, thread 01KSP5P6CSJH) on v0.15.0/
v0.15.1 surfaced one load-bearing bug + several precision items. This
pass closes them.
Load-bearing fix — cancel paths targeted the wrong turn_id:
- `_TURN_COUNTER` allocates browser-local ids (1, 2, 3…); the real
upstream Worldtree turn_id (e.g. 799) only arrives in the first SSE
event. The v0.15.x cancel/disconnect/shutdown paths posted to
/sessions/{sid}/turns/{LOCAL_ID}/cancel — wrong URL upstream.
- TurnHandle.upstream_response (dead field) → upstream_turn_id: int|None.
Captured from the first event's sse_id.turn_id in the stream
generator. All cancel paths now target it. Cancel before the upstream
stream starts (upstream_turn_id None) is a no-op
({"cancelled": false, "reason": "not_started"}).
- The old cancel tests mocked the local-id URL, so they encoded the bug;
rewritten to assert the UPSTREAM id is targeted.
Behavior change (minor-bump driver) — server-side end_user_id:
- create_app gains end_user_id kwarg; entrypoint reads
RATATOSKR_END_USER_ID and threads it in. POST /api/sessions uses
app.state.end_user_id, IGNORING any browser-supplied value (a client
can't impersonate an arbitrary end-user partition). JS no longer
sends end_user_id.
Precision fixes:
- Entrypoint missing-extras ImportError catch scoped to starlette/
uvicorn ONLY; baseline-dep / first-party import failures now
propagate as real tracebacks instead of masking as exit-12.
- Lifespan shutdown logs per-pending session_id + upstream_turn_id
(was a single aggregate count).
Tests (+18; 376 total):
- disconnect_triggers_upstream_cancel (INV-005 load-bearing — drives
the stream generator directly + cancels the consuming task; would
have caught the turn_id bug)
- cancel_targets_upstream_turn_id, cancel_before_started_is_noop,
cancel_failed_500
- server-side end_user_id: uses / ignores-body / omits-when-unset
- create_app: routes_registered / state_attached / factory_stored
- entrypoint: default_host / port_zero / happy_argv / open / no-open
- real_import_bug_propagates (precision guard)
- full_event_vocab at the stream-endpoint layer
Contract #16 amended: v0.16.0 amendment banner + INV-005/006 reworded
for upstream_turn_id + FN sketches corrected (server-side end_user_id,
upstream_response→upstream_turn_id, manual client lifecycle vs the
non-executable async-with sketch, not-started cancel branch).
This commit is contained in:
@@ -108,14 +108,39 @@ def main(argv: list[str] | None = None) -> int: ... # entrypoint.main
|
||||
|
||||
`entrypoint.main` is the console-script target — parses flags, builds the client factory from env, calls `create_app`, runs uvicorn. The lazy-import discipline lives here: `import starlette` does NOT happen at module top — it lands inside `main()` after arg parsing, with an `ImportError` catch that prints the `pip install ratatoskr[web]` hint and exits non-zero.
|
||||
|
||||
## v0.16.0 amendment (post-Heid-code-review)
|
||||
|
||||
Heid panel review (Gróa + Hulda, thread `01KSP5P6CSJH`) on the
|
||||
v0.15.0/v0.15.1 implementation surfaced three contract-text issues
|
||||
now corrected below:
|
||||
|
||||
1. **Upstream vs local turn_id.** Cancel paths (explicit cancel,
|
||||
browser-disconnect, lifespan shutdown) MUST target the *upstream*
|
||||
(Worldtree-assigned) turn_id captured from the first SSE event's
|
||||
`sse_id.turn_id`, NOT the browser-local `_TURN_COUNTER` value (which
|
||||
is only a registry key). The `TurnHandle.upstream_response` field is
|
||||
replaced by `upstream_turn_id: int | None`. Cancel before the
|
||||
upstream stream starts (upstream_turn_id is None) is a no-op
|
||||
(`{"cancelled": false, "reason": "not_started"}`).
|
||||
2. **`RATATOSKR_END_USER_ID` is server-configured.** `FN main` reads it
|
||||
from env and threads it into `create_app(..., end_user_id=...)`; the
|
||||
`POST /api/sessions` endpoint uses `app.state.end_user_id` server-
|
||||
side. The browser NEVER supplies end_user_id — a client cannot
|
||||
impersonate an arbitrary end-user partition.
|
||||
3. **Stream client lifecycle.** The `async with client_factory() as
|
||||
client:` sketch in `FN stream_turn_endpoint` is not executable for a
|
||||
long-lived async generator that must outlive the handler frame; the
|
||||
implementation uses manual `client = ...; try: ... finally: await
|
||||
client.aclose()`. Sketch corrected below.
|
||||
|
||||
## Invariants
|
||||
|
||||
- **INV-001**: `ratatoskr.web.__init__` and `ratatoskr.web.entrypoint` MUST NOT import `starlette` or `uvicorn` at module top. Import is inside `main()` after flag parsing.
|
||||
- **INV-001**: `ratatoskr.web.__init__` and `ratatoskr.web.entrypoint` MUST NOT import `starlette` or `uvicorn` at module top. Import is inside `main()` after flag parsing. The missing-extras `ImportError` catch is scoped to the OPTIONAL extras (`starlette` / `uvicorn`) ONLY — baseline-dep / first-party import failures propagate as real tracebacks rather than masking as exit-12.
|
||||
- **INV-002**: `ratatoskr.web.server.create_app` MUST accept a `client_factory` callable. The app MUST NOT construct `httpx.AsyncClient` at module top or in route handlers; it MUST call the factory.
|
||||
- **INV-003**: Upstream API key MUST never appear in any browser-visible response. Server proxies upstream calls using the client factory; only the upstream's JSON / SSE payload is forwarded. No header echo.
|
||||
- **INV-004**: Transcript content from upstream `text` SSE events MUST be HTML-escaped before reaching the browser DOM (escape on the wire in the SSE proxy serialization OR escape in the JS rendering — both are acceptable; pick one and stick to it).
|
||||
- **INV-005**: Browser disconnect mid-stream (`asyncio.CancelledError` in the SSE handler) MUST trigger an upstream cancel on the matching `(session_id, turn_id)` via `sse_client.cancel_turn`. If the turn already completed, the cancel is a best-effort no-op (existing `CancelAlreadyCompleted` exception is swallowed).
|
||||
- **INV-006**: Server shutdown (Ctrl-C / SIGTERM) MUST issue upstream cancels for every entry in the turn registry within a 5-second cleanup budget. Entries that don't ack in time are abandoned with a structured log line.
|
||||
- **INV-005**: Browser disconnect mid-stream (`asyncio.CancelledError` in the SSE handler) MUST trigger an upstream cancel via `sse_client.cancel_turn` on the captured `upstream_turn_id` (v0.16.0 — NOT the browser-local turn_id). If the turn already completed, the cancel is a best-effort no-op (`CancelAlreadyCompleted` swallowed). If `upstream_turn_id` is still None (upstream stream never started), the disconnect cancel is skipped — nothing to cancel.
|
||||
- **INV-006**: Server shutdown (Ctrl-C / SIGTERM) MUST issue upstream cancels (on `upstream_turn_id`) for every in-flight registry entry within a 5-second cleanup budget. Handles whose `upstream_turn_id` is None are skipped. Entries that don't ack in time are abandoned with a per-entry structured log line carrying `session_id` + `upstream_turn_id`.
|
||||
- **INV-007**: The turn registry MUST be in-process memory only — no persistence, no shared state across server restarts. Process exit drops the registry.
|
||||
- **INV-008**: Each SSE event serialized to the browser MUST follow the contract enumerated in `tests/fixtures/presentation_contract.json` — one entry per Event type, with the exact JSON shape the browser presenter renders against.
|
||||
- **INV-009**: All wire-layer modules (`sse_client`, `sessions`, `tier3`, `local_agents`) MUST be used unchanged. Any required change to those modules is out of scope for this issue and gets its own ticket.
|
||||
@@ -234,9 +259,10 @@ BRIEF: Proxy POST /sessions to upstream.
|
||||
PRE: [PRE-001 hard] request body has "agent_id" key -- 400 if missing
|
||||
POST: [POST-001 return_value] 201 with SessionInfo on upstream success
|
||||
STEPS:
|
||||
1. [parse] body = await request.json(); agent_id = body["agent_id"]; end_user_id = body.get("end_user_id")
|
||||
2. [proxy] async with client_factory() as client: info = await create_session(client, agent_id, end_user_id=end_user_id)
|
||||
3. [return] JSONResponse(as_dict(info), status_code=201)
|
||||
1. [parse] body = await request.json(); agent_id = body["agent_id"] (400 if missing)
|
||||
2. [server-side] end_user_id = request.app.state.end_user_id # v0.16.0: server-configured, NOT from body
|
||||
3. [proxy] async with client_factory() as client: info = await create_session(client, agent_id, end_user_id=end_user_id)
|
||||
4. [return] JSONResponse(as_dict(info), status_code=201)
|
||||
ERRORS:
|
||||
AgentNotFound -> JSONResponse({"error_code": "agent_not_found"}, 404)
|
||||
SessionApiFailed -> JSONResponse({"error_code": "session_api_failed", "status": exc.status}, exc.status)
|
||||
@@ -244,6 +270,8 @@ TESTS:
|
||||
happy [tracer]: respx mock 201 → endpoint returns 201 with session JSON
|
||||
unknown_agent [error]: respx mock 404 → 404 with agent_not_found envelope
|
||||
missing_agent_id [adversarial]: body without agent_id → 400
|
||||
server_side_end_user_id [v0.16.0]: create_app(end_user_id="X") → upstream body carries end_user_id="X"
|
||||
ignores_body_end_user_id [v0.16.0,security]: body end_user_id is overridden by server value
|
||||
```
|
||||
|
||||
```contract
|
||||
@@ -274,7 +302,7 @@ POST: [POST-002 state_change] app.state.turn_registry has entry for (sid, turn_i
|
||||
STEPS:
|
||||
1. [parse] session_id = path_params["session_id"]; body = await request.json(); content = body["content"]
|
||||
2. [allocate] turn_id = next_turn_id() # process-local monotonic counter
|
||||
3. [register] turn_registry[(session_id, turn_id)] = TurnHandle(content=content, status="queued", upstream_response=None)
|
||||
3. [register] turn_registry[(session_id, turn_id)] = TurnHandle(content=content, status="queued", upstream_turn_id=None) # v0.16.0: was upstream_response
|
||||
4. [return] JSONResponse({"turn_id": turn_id}, status_code=200)
|
||||
TESTS:
|
||||
happy [tracer]: POST {"content": "hi"} → 200 with turn_id; registry populated
|
||||
@@ -290,17 +318,16 @@ POST: [POST-001 side_effect] each upstream event serialized to browser as SSE ev
|
||||
POST: [POST-002 state_change] on completion/disconnect, registry entry removed; upstream cancel if turn still in flight
|
||||
STEPS:
|
||||
1. [validate] sid, tid = path/query params; handle = registry.get((sid, tid)); 404 if None
|
||||
2. [open] async with client_factory() as client:
|
||||
- upstream = stream_turn(client, sid, handle.content)
|
||||
- handle.upstream_response = upstream
|
||||
2. [open] client = client_factory() # v0.16.0: manual lifecycle, NOT `async with` — the generator outlives this frame; closed in finally
|
||||
- handle.status = "streaming"
|
||||
3. [forward] async for event in upstream:
|
||||
3. [forward] async for event in stream_turn(client, sid, handle.content):
|
||||
- IF handle.upstream_turn_id is None: handle.upstream_turn_id = event.sse_id.turn_id # v0.16.0: capture upstream turn id
|
||||
- serialize per fixture: {"type": <ssetype>, "data": <json>}
|
||||
- yield as `event: <type>\\ndata: <json>\\n\\n` bytes
|
||||
4. [terminal] on Done/Error/Cancelled: yield final SSE, mark handle.status, break
|
||||
5. [cleanup] finally:
|
||||
- IF asyncio.CancelledError caught and turn in-flight: upstream cancel via cancel_turn(client, sid, tid)
|
||||
- remove (sid, tid) from registry
|
||||
- IF asyncio.CancelledError caught AND status=="streaming" AND upstream_turn_id is not None: cancel_turn(client, sid, handle.upstream_turn_id) # v0.16.0: upstream id, not tid
|
||||
- remove (sid, tid) from registry; await client.aclose()
|
||||
ERRORS:
|
||||
KeyError -> 404 turn_not_found
|
||||
asyncio.CancelledError -> upstream cancel, propagate
|
||||
@@ -322,15 +349,18 @@ POST: [POST-001 side_effect] upstream cancel call lands; registry entry removed
|
||||
POST: [POST-002 return_value] 200 with {"cancelled": true} or 200 with status reflecting upstream race
|
||||
STEPS:
|
||||
1. [validate] sid, tid = params; handle = registry.get((sid, tid)); 404 if None
|
||||
2. [cancel] async with client_factory() as client:
|
||||
- try: await cancel_turn(client, sid, tid)
|
||||
2. [not-started] IF handle.upstream_turn_id is None: del registry[(sid,tid)]; return 200 {"cancelled": false, "reason": "not_started"} # v0.16.0: upstream never opened
|
||||
3. [cancel] async with client_factory() as client:
|
||||
- try: await cancel_turn(client, sid, handle.upstream_turn_id) # v0.16.0: upstream id, not tid
|
||||
- return 200 {"cancelled": true}
|
||||
3. [race] EXCEPT CancelAlreadyCompleted / CancelTurnNotFound:
|
||||
4. [race] EXCEPT CancelAlreadyCompleted / CancelTurnNotFound:
|
||||
- return 200 {"cancelled": false, "reason": "race_or_completed"}
|
||||
4. [cleanup] del registry[(sid, tid)]
|
||||
5. [cleanup] del registry[(sid, tid)]
|
||||
TESTS:
|
||||
happy [tracer]: registered turn → POST cancel → 200, upstream cancel called
|
||||
happy [tracer]: registered turn (upstream_turn_id set) → POST cancel → 200, upstream cancel at the upstream id
|
||||
unknown_turn [error]: not in registry → 404
|
||||
cancel_before_started [v0.16.0]: upstream_turn_id None → 200 {cancelled:false, reason:not_started}, no upstream call
|
||||
cancel_targets_upstream_turn_id [v0.16.0]: local tid != upstream id → cancel URL uses upstream id
|
||||
already_completed [race]: respx cancel returns 409 → 200 with reason=race_or_completed
|
||||
cancel_failed [error]: respx returns 500 → 500 with cancel_failed envelope
|
||||
```
|
||||
@@ -352,11 +382,11 @@ BRIEF: On Ctrl-C / SIGTERM, drain the turn registry within 5s budget per INV-006
|
||||
POST: [POST-001 side_effect] every in-flight upstream turn gets a cancel attempt within budget
|
||||
POST: [POST-002 side_effect] entries that don't ack in budget logged + abandoned
|
||||
STEPS:
|
||||
1. [collect] handles = list(app.state.turn_registry.values())
|
||||
1. [collect] in_flight = [h for h in registry.values() if h.status == "streaming" and h.upstream_turn_id is not None] # v0.16.0: skip not-yet-started
|
||||
2. [cancel] async with client_factory() as client:
|
||||
- tasks = [cancel_turn(client, h.session_id, h.turn_id) for h in handles if h.status == "streaming"]
|
||||
- done, pending = await asyncio.wait(tasks, timeout=5.0)
|
||||
3. [log] for each pending: log {"kind": "shutdown", "event": "cleanup_timeout", session/turn}
|
||||
- task_to_handle = {create_task(cancel_turn(client, h.session_id, h.upstream_turn_id)): h for h in in_flight} # v0.16.0: upstream id
|
||||
- done, pending = await asyncio.wait(task_to_handle, timeout=5.0)
|
||||
3. [log] for each pending: cancel task + log {"kind": "shutdown", "event": "cleanup_timeout", "session_id": h.session_id, "upstream_turn_id": h.upstream_turn_id}
|
||||
4. [clear] registry.clear()
|
||||
TESTS:
|
||||
happy [tracer]: 2 in-flight turns + shutdown → both upstream cancels called, registry empty
|
||||
|
||||
Reference in New Issue
Block a user