feat(conversations): answer a request that outlived its turn (#1635) - #1856
Conversation
jhgaylor
left a comment
There was a problem hiding this comment.
Approving the delta. Its Elixir static analysis red is inherited from #1855's stray blank line, not anything here — it goes green once that's fixed.
This is the PR I expected to find a hole in and didn't. The ordering argument is the load-bearing one and it's both correct and written down: every gate the wake would apply runs before the row is touched, because resolving into a prompt nobody delivers loses the answer with no second copy to retry from. Handing the caller a 409 that says "somebody else answered" when that isn't what happened would be worse than the failure itself, and :answer_not_delivered existing as its own outcome is the right call.
Other things I checked and liked:
_unsafe_resolve_detached_request/3enforcing first-answer-wins in thewhereclause rather than via a process holding the request. A client, the sweep, and a client racing the sweep all converge oncount == 1or nothing, andpublish_stagefires only for the winner.- Looking at the detached row before asking the server. Asking the server first would answer a peer whose turn is over, report success, and never open the turn that actually carries the answer. The comment says exactly that.
- The
status not in ["idle", "running"]guard inanswer_permission_request/5, so a conversation that can't carry an answer back leaves the request where it is rather than resolving it into nothing. - Both
_unsafe_calls carry the adjacent-scoped-fetch comment naming which fetch established ownership.
Nit, cosmetic: record_permission_answered/5 sits in the with chain but can only return :ok, so it reads like a gate that isn't one. A bare call before resume_after_request/5 would say the same thing more plainly.
44615c1 to
29f603c
Compare
06b9c24 to
53ec487
Compare
29f603c to
eb5d49d
Compare
53ec487 to
43eb5bd
Compare
BinaryBourbon
left a comment
There was a problem hiding this comment.
Approving. The ordering decision in answer_detached_permission/5 is the right one and the comment states it better than I would:
Every gate the wake path would apply is applied first, before the row is touched. Resolving and then failing to deliver loses the answer with nothing to retry from, and hands the caller a 409 that says somebody else answered — which is a lie about what happened.
That is also why _unsafe_resume_gate/1 is worth being a public, documented function with three distinguishable errors rather than an inlined cond: :busy is retryable now, :gone never is, and an account refusal is retryable once the account is not. #1858's sweep consumes the same three, which is what keeps the two doors from drifting.
_unsafe_resolve_detached_request/3 enforcing first-answer-wins in the update_all where — on id and waiting and the request id the caller read — rather than in a process is the correct translation of the in-turn rule to a world with no process. A client racing the sweep gets {:error, :no_pending_permission} and the row is touched once. Good.
:answer_not_delivered deserving its own error rather than folding into the 409 is the detail I would have missed: the 409 tells a client to give up on a request somebody else took, and this case has to say the opposite — the answer landed, the agent has not heard it, send a prompt. Tested both ways.
Two things I checked and am satisfied by:
- The new
status not in ["idle", "running"]refusal also sits in front of the in-turn path, which previously had no status gate.Conversation.@statusesispending running idle failed terminated, an in-turn request is only ever held whilerunning(oridle, for an out-of-turn ask), and:not_runningfolds into the same 409permission_request_resolvedthat:no_pending_permissionalready produced. So no status-code change for any reachable in-turn case. - The detached row is consulted before the server, and the comment gives the reason: asking the server first would answer a peer whose turn is over, report success, and never open the turn that actually carries the answer.
The _unsafe_ naming, the adjacency to the scoped get_conversation/2, and the WARNING docstrings are all per CLAUDE.md. The drained-balance test grants exactly the balance back so the top-up clears zero rather than a hole is the kind of care that stops this being a flake later.
No findings.
eb5d49d to
20665da
Compare
43eb5bd to
c819383
Compare
993d7c6 to
7fb8d48
Compare
c819383 to
539cdf9
Compare
The answer door is the same one, POST-shaped and audited the same way; what differs is where the answer goes. The detached row is looked at before the server, because the peer that raised the request may still be idle on the sandbox holding the JSON-RPC id and would report an answer landed that reached nobody. Every gate the wake would apply runs before the row is touched, first answer wins by the update's own where clause, and the resolution reaches the agent as a new turn whose prompt is one line of JSON, which wakes a suspended sandbox on the way. GET-shaped listing of what a conversation waits on comes with it, unserved for now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
539cdf9 to
a8f28a0
Compare
Answering a detached request through the existing door: the row is looked at before the server, every gate runs before the row is touched, first answer wins by the update's own
where, and the answer opens a resume turn that wakes the sandbox.Part 5 of 9 for #1635, on #1855.
Part of #1635