No deadline survives the turn path, from mavweb down to llama-server #188

2026-08-06T21:00:17+02:00

claude commented

2026-08-06 21:00:17 +02:00

Closes Vikunja #638.

Acceptance criteria and quality gate are in TASK.md on this branch.
Review the review, not the diff — leave comments and the agent will apply them via task start 638.

Closes Vikunja #638. Acceptance criteria and quality gate are in `TASK.md` on this branch. Review the review, not the diff — leave comments and the agent will apply them via `task start 638`.

claude added 5 commits 2026-08-06 21:00:17 +02:00

the reply seam takes the turn's context (V-638) 1607009215

voice.Replier.Reply had no context, so llmReplier phrased under
context.Background() and the only bound on a reply was phraser.timeout,
60s in deploy. Both call sites already held a context.

The stub ignores it: it makes no model call.

an ipc call can be cancelled and always has a deadline (V-638) 123b9aa961

roundtrip set no connection deadline and call checked the context once,
before sending, so a daemon that read the frame and stopped answering
parked the caller for as long as the socket stayed open.

The deadline comes from the caller's context, falling back to 120s, the
convention internal/voice/client.go already had. A watchdog closes the
conn on ctx.Done(); it closes rather than calling drop, because drop
wants c.mu and call is holding it.

The retry split is unchanged and now also declines a retry the caller
has stopped waiting for. A cancelled mutation stays ErrAmbiguousOutcome,
because it may have committed. A cancelled read reports the cancellation.

Three tests against a server that accepts and never answers. There was
no test for this, which is why it went unnoticed.

a request context on the ipc server, cancelled by Close (V-638) cbd8077d2c

serveConn dispatched under context.Background(), so a dispatch in flight
during shutdown could not be told to stop and closeGrace could only
abandon it. The server now carries a context, Close cancels it, and each
conn derives its own so nothing outlives the connection.

A zero-value Server built outside Listen falls back to Background; two
wiring tests do that.

Client.Close read c.conn with no lock while roundtrip re-dialed and
dropped it, which -race caught on the new test. The conn field now has
its own mutex, held only across a read or an assignment, so Close and
the watchdog reach the connection without queueing behind the call they
are interrupting.

chat gets its own core connection, so a turn stops blocking every page (V-638) 60e64dd83c

ipc.Client serialises every call on one mutex, and mavweb routed 28
handlers plus /api/chat through the shared client. A turn is bounded
only by phraser.timeout, 60s in deploy, so a page load behind one could
wait that long.

/models already had its own connection for this reason. Chat gets the
third one. A failed dial logs and falls back to the shared client, which
is how it behaved before.

/api/ptt needs nothing here: it proxies to the voice port and never
touches this client.

A connection pool inside ipc.Client is the general form and stays
unbuilt until a second module is measured queueing.

the plan doc records what landed and what came out differently (V-638) 2e6b274bff

kami reviewed 2026-08-06 21:04:21 +02:00

internal/ipc/client.go Outdated

						
				@@ -106,1 +119,4 @@

				// it cannot block behind the call it is interrupting.

				func (c *Client) Close() error {

					c.connMu.Lock()

					defer c.connMu.Unlock()

kami commented

2026-08-06 21:04:21 +02:00

it's defer and where do we call/reclaim it?

kami referenced this issue from a commit

2026-08-06 21:05:55 +02:00

Close takes connMu by hand, not by defer (V-638)

kami added 1 commit 2026-08-06 21:05:55 +02:00

Close takes connMu by hand, not by defer (V-638) bd53372616

Review on PR #188. The deferred unlock made it unclear where the lock
was released, and it held connMu across conn.Close(), which contradicts
the invariant stated one line above it: the field accesses only. A close
on a tcp conn can block, and connMu is on the path of every call.

kami added 1 commit 2026-08-06 21:09:08 +02:00

a cancelled call does not leave its conn for the next one (V-638) 9cdec11346

The watchdog and the end of the call race by construction: a cancellation
landing as the reply arrives closes a conn the call had already finished
with, and c.conn still pointed at the closed socket.

It was survivable before this. A write to a closed socket is
errWriteLost, which re-dials and retries, and that retry is safe because
nothing was sent. So this buys one round trip, not a correctness fix, and
the new test says so rather than pretending to catch a break.

kami merged commit 69d0f5ee78 into master

2026-08-06 21:11:42 +02:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kami/Maven#188