feat(workers): deny a request that outlived its turn and then ran out of time (#1635) - #1858
Conversation
jhgaylor
left a comment
There was a problem hiding this comment.
Requesting changes for a one-line format break that this PR introduces itself.
apps/fountain/lib/fountain/conversations.ex
13 13 | alias Fountain.Audit
14 +|
14 15 | alias Fountain.Conversations.{
The diff deletes the blank line after alias Fountain.Audit that #1856 left in place. Elixir static analysis is red here and on #1859 and #1860 as a result. Put it back and three PRs go green.
The sweeper itself is good and I have no substantive changes to ask for.
- Leaving the row untouched when
_unsafe_resume_gate/1says:busyor the account is refused, and coming back in a minute, is the same trade #1856 makes at the answer door and it's the right one. A deadline that has passed is owed either way; a denial resolved into a prompt nobody delivers is gone. {:error, :gone}being the exception — resolve, record, don't try to open a turn — is correct. No turn will ever carry that denial, so leaving the card up forever would be the worse failure.actor: "system:detached_request_sweeper"rather than reusingsprite. The vocabulary in 0013 is closed andsystem:<worker>is the right slot; the docstring saying why (no human was at the keyboard, and claiming otherwise lies about who decided) is the part that keeps it from drifting back.@sweep_limit 25withorder_by permission_deadlineso a backlog can't wake a hundred sandboxes in one tick, and the minute grain justified against deadlines measured in hours.
One thing I'd like confirmed rather than changed: record_permission_denied/4 grew an opts with a default, and the /3 callers are unchanged. Worth a glance that the in-turn path still records sprite — the existing tests should cover it, I just didn't trace every caller.
6fc7078 to
21bb795
Compare
d8bd8cf to
9142523
Compare
21bb795 to
96567c9
Compare
9142523 to
b62990f
Compare
BinaryBourbon
left a comment
There was a problem hiding this comment.
Approving. The sweep is the right mechanism and the reasons are in the moduledoc where the next person will find them: a process timer cannot span a two-day wait that the sandbox parks through, a deploy in the middle would drop it silently, so the deadline is a column and this is what reads it.
The details that make it safe rather than merely correct:
- The gate runs before the row is touched, same as the answer door, with
{:error, :gone}as the deliberate exception — a terminated conversation will never carry the denial, so resolving it is what stops the sweep finding the same row every minute forever. Both branches are tested, including the "and then it finds nothing" follow-up assertion. @sweep_limit 25, so a backlog cannot wake a hundred sandboxes in one tick.- The partial index is the query.
where waiting == true and permission_deadline <= now order by permission_deadline limit 25matchesturns_waiting_permission_deadline_indexfrom #1854 on its leading column, which is what makes a per-minute cron defensible. actor: "system:detached_request_sweeper". Within the closed vocabulary CLAUDE.md defines, andrecord_permission_denied/4gaining the opt rather than hardcodingspritekeeps ADR 0013's "only record what happened" — no human decided this, and sayingspritewould have been the wrong lie rather than no lie.- The "a turn that is not waiting is never a candidate, whatever its deadline" test is the guard against this sweep reaching into a request the process timer owns. Worth having explicitly.
One observation, no change requested: the sweep counts Enum.count(&expire/1) and logs at :info per expiry plus a batch line, so a 25-row backlog writes 26 info lines a minute. Fine at this grain given how rarely the query is non-empty.
No findings.
96567c9 to
e906967
Compare
b62990f to
25c80cf
Compare
Fixed on the current head (25c80cf). The blank line after alias Fountain.Audit is back (conversations.ex:13-14) and Elixir static analysis is green here.
The thing you wanted confirmed rather than changed: I traced it. record_permission_denied/4's opts defaults to [], the in-turn callers are unchanged /3 calls, and only the sweeper passes actor: "system:detached_request_sweeper". The in-turn path still records sprite.
Re-reviewed and approved.
e906967 to
f777ce7
Compare
… of time (#1635) A detached deadline cannot be a process timer: the sandbox parks and the server stops while the request waits, and a deploy in the middle of a two-day wait would drop it with nothing to notice. So the deadline is on the row, and this Oban sweep reads it once a minute through the partial index. Expiry is a denial, picked from the options the agent itself offered, audited to the sweep because no human decided, and delivered the way an answer is, as a resume turn. A conversation that cannot take that turn yet keeps the row for the next sweep; one that is over is resolved without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S9jevFQT5MkF3rJUieeiHW
25c80cf to
d6c69ed
Compare
The Oban sweep that denies a detached request past its deadline, from the options the agent itself offered, audited to the sweep, delivered as a resume turn. A conversation that cannot take the turn keeps the row for the next minute.
Part 7 of 9 for #1635, on #1857.
Part of #1635