claude
69d0f5ee78
Co-authored-by: claude <no-reply@agents.claude.kvmx.ru> Co-committed-by: claude <no-reply@agents.claude.kvmx.ru>
100 lines
5.0 KiB
Markdown
100 lines
5.0 KiB
Markdown
# No deadline on the turn path
|
|
|
|
Last verified: 06-08-2026 @ 60e64dd
|
|
|
|
**All four steps landed on 06-08-2026.** What follows describes the defect as it was and
|
|
the work as it was planned. Two things came out differently. `Client.Close` read the conn
|
|
field with no lock while `roundtrip` re-dialed and dropped it. `-race` caught that on the
|
|
new cancellation test. So the conn field now has a mutex of its own, held only across a
|
|
read or an assignment. And `/api/ptt` needed nothing: it proxies to the voice port and never
|
|
touches the shared client, so only `/api/chat` got the extra connection. The pool inside
|
|
`ipc.Client` is still unbuilt and still waiting on a second module measured queueing.
|
|
|
|
V-638. Sibling of V-607, which is the same class of bug in `internal/worker`.
|
|
Reads with `docs/offload.md` and `docs/protocol.md`.
|
|
|
|
## What is missing
|
|
|
|
A chat turn starts in a mavweb HTTP handler and ends at llama-server. Nothing between those
|
|
two points can be cancelled, and one hop has a timeout.
|
|
|
|
Four places, all on the same path.
|
|
|
|
`voice.Replier.Reply` takes no context (`internal/voice/replier.go:41`). So `llmReplier`
|
|
calls `PhraseReply(context.Background(), d)` at `cmd/mavend/replier_llm.go:42`. The turn
|
|
cannot deadline its own reply. The only bound is `phraser.timeout`, 60s in deploy.
|
|
|
|
`ipc.Client.roundtrip` sets no connection deadline (`internal/ipc/client.go:202`). A daemon
|
|
that stops answering parks the caller for as long as the socket stays open.
|
|
|
|
`ipc.Client.call` checks the context once, before sending (`client.go:149`), then blocks in
|
|
`roundtrip`. Cancelling mid-call does nothing.
|
|
|
|
`ipc.Server.serveConn` dispatches under `context.Background()` (`internal/ipc/server.go:253`).
|
|
A client that hangs up does not cancel the turn, and neither does `Server.Close`.
|
|
|
|
## And every call queues behind the slowest one
|
|
|
|
`ipc.Client` serialises on one connection and one mutex. mavweb routes `/api/chat` and
|
|
`/api/ptt` through the shared client, so one turn blocks all 28 handlers while it runs.
|
|
Worst case is a 60s page load.
|
|
|
|
This is understood for exactly one route already. `cmd/mavweb/main.go:57` opens a second
|
|
connection for `/models`, and the comment there says why. A model swap is a multi-minute
|
|
call, and sharing the connection would freeze every other page.
|
|
|
|
## The pattern is already in the repo
|
|
|
|
`internal/voice/client.go:101` derives a connection deadline from the caller's context,
|
|
falls back to 120s, and clears it with a defer. `internal/ipc/client.go` never learned it.
|
|
Copy that rather than inventing a second convention.
|
|
|
|
## The work
|
|
|
|
One commit each.
|
|
|
|
**Context on the reply seam.** `phraser.Replier.PhraseReply` already takes a context and the
|
|
interface has two implementations, so this is small. Change `Reply` to take a context, have
|
|
`StubReplier` ignore it, and pass it through `llmReplier` to `PhraseReply`. Both call sites
|
|
already hold one: `cmd/mavend/voice.go:461` and `cmd/mavend/clarify.go:574`.
|
|
|
|
**Deadlines and cancellation on the client.** Pass the context into `roundtrip` and set
|
|
`SetDeadline` from it. For cancellation mid-call, a watchdog goroutine that calls `c.drop()`
|
|
on `ctx.Done()` is enough. `drop` exists, and the retry split already separates a lost write
|
|
from a lost read. So a cancelled call lands in `errReadLost` and is never retried for a
|
|
mutation. Check that against `internal/ipc/maperr_test.go`.
|
|
|
|
**A request context on the server.** `serveConn` should derive from a server-scoped context
|
|
so `Close` cancels a dispatch in flight. `Server` already carries `done` and a conn registry
|
|
for this class of problem. The registry comment records what the last version of it cost:
|
|
eleven days of stale ciphertext.
|
|
|
|
**Stop serialising mavweb.** Give `/api/chat` and `/api/ptt` their own connection, the way
|
|
`/models` has one. Roughly ten lines, and it changes no shared code.
|
|
|
|
A connection pool inside `ipc.Client` is the general form and is deliberately not the first
|
|
step. Each connection is already its own request and response stream. So a pool preserves
|
|
frame pairing by construction. It still has to keep re-dial on drop, the
|
|
`errWriteLost` and `errReadLost` split, and `Close`. Do the narrow fix, measure, and reach
|
|
for the pool only if a second module turns out to queue.
|
|
|
|
## How it is judged
|
|
|
|
`make test` stays green. It is green at `06c1cf2`.
|
|
|
|
Nothing here changes routing or recall, so `make eval-router` and `make eval-recall` are
|
|
unchanged rather than re-measured.
|
|
|
|
By hand: load `/dash` while a chat turn is in flight. Before the change it waits for the
|
|
length of the turn.
|
|
|
|
There is no test today that a cancelled context aborts an in-flight `ipc.Client` call. That
|
|
absence is why two of these four went unnoticed, so the test is part of the work.
|
|
|
|
## What is not done here
|
|
|
|
The store is still `SetMaxOpenConns(1)` (`internal/store/store.go:99`) under WAL. WAL is
|
|
built for concurrent readers against one writer, and the cap makes every read queue.
|
|
`Store.DB(ctx)` hands the digestion worker a read transaction on that same connection. This
|
|
plan does not touch it. It is measurable first and should be measured before it is changed.
|