Conversation
Eight findings from the review of this PR, each with a test that fails
without its fix.
Queued work was destroyed by conditions that clear on their own. The
drainer released the claim for `sandbox_quota_exceeded` and `fleet_full`
and marked everything else `failed`, erasing the prompt. But
`start_conversation/2` answers `:provisioning` while a persistent
identity's home is still building, and `Team.send_message/5` answers
`:busy`, `:sandbox_at_capacity` or `:runner_offline` mid-turn. Those four
now return the claim to `queued`, and the drain moves past that request
rather than re-claiming it, so one busy teammate does not hold up the
tenant. It is the list `Workers.TeamScheduleRun` snoozes on, for the same
reason.
`run_schedule/2` queued for both its callers, so the synchronous "Run
now" answered a person 429 while a conversation started on its own up to
an hour later, with no request id to watch or cancel. ADR 0042 decision 3
buys the queue with "a cron firing has nobody there to retry it", so the
gate is now the audit actor.
The poke at `update_sandbox/2` was an unrescued `Repo.all` plus one Oban
insert per waiting tenant, running after the row committed on the choke
point every sandbox status change goes through, where nearly every caller
matches `{:ok, _}`. It is one insert now, the scan moved into the job,
and the whole thing rescues the way metering does beside it.
Also: `release/1` raised a MatchError instead of tolerating a claim that
`recover_stuck_claims/1` had already taken back, failing the job and
stranding the tenant's other requests; the depth bound was check-then-
insert with no lock, the #330 shape, and now takes a per-tenant advisory
lock; queued launch attributes are the keys `start_conversation/2` reads
rather than every key a caller sent, since `ConversationCreateRequest`
sets no `additionalProperties: false`; `sandbox_requests` gets a 30 day
window on terminal rows only, and its gauge covers live rows, because a
`last_value` over history only climbs; and `position/1` counts `starting`
rows, having reported `position: 1` to a caller with someone in front of
it.
The queue's advisory lock is namespace 4317. 4316 is already
`Conversations`, and sharing it would let a `phash2` collision between a
user id and a sandbox id block an unrelated writer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015TGtUAsTwsLRf2q9Tcjnj2
Two follow-ups on the review fixes. The fan-out this branch put on `Conversations.update_sandbox/2` traded one problem for another. Replacing the per-tenant scan with a single Oban insert took the unbounded work off the choke point, but it inserted on every slot-freeing transition, including the overwhelming majority where no tenant has anything queued and the old code inserted nothing at all. `SandboxQueue.any_active_requests?/0` is an existence probe, so an empty queue costs that and nothing else, and a non-empty one costs the probe plus one insert with the scan still inside the job. The new endpoints move `sdk/typescript/src/generated/openapi.ts`, so the release gate is right that nothing would publish without a bump. 1.22.0, leaving 1.21.0 to the OAuth self-service branch that already claims it; those two have to merge in ascending order, because `sdk-publish.yml` runs a bare `npm publish` and npm moves `latest` to whatever it published last. The client gains no method: `* /api/sandbox-queue*` is in `omissions.json` on purpose, so what ships is the generated types and the `queue` flag on the create body. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TGtUAsTwsLRf2q9Tcjnj2
dd8fe28 to
043f50a
Compare
BinaryBourbon
left a comment
There was a problem hiding this comment.
Requesting changes — same two reasons as #1489: the branch can't be reviewed in this state, and the test story needs resolving.
207 commits behind main, conflicting in 11 files: conversations.ex, fountain_web/telemetry.ex, decisions/index.md, docs/api.md, docs/configuration.md, sdk/contract/contract.json, sdk/typescript/src/generated/openapi.ts, both SDK version files and sdk/typescript/src/http.ts. Last run September 3rd. decisions/index.md conflicting is a reminder that the index is generated — it wants scripts/decisions-index.sh on the rebased tree, not a manual merge.
~2,171 lines across 44 files, which is over the line. A bounded queue splits along its own architecture almost too neatly: the table + migration, the claim/drain state machine, the capacity-release event wiring and Oban backstop, queue: true on the API, then the list/outcome/cancel endpoints with their OpenAPI and SDK surface. Five PRs, each with its own tests, each reviewable in an evening.
And this:
full ExUnit suite: 4,194 tests; five timing/integration failures all passed on targeted retry after branch-specific fixes
Five timing failures on a PR whose entire subject is queueing under capacity pressure is not a footnote — that's the feature's own failure mode showing up in its test suite. "Passed on targeted retry" is the specific thing CLAUDE.md tells us not to accept: keep the failed run's evidence, work out what the test asserts on versus what it waits on, and file it if it's real. If those five were genuinely ambient, they should have flake issues. If they weren't, they're the bug.
Worth pinning down before a rebase, because the answer probably changes the design rather than the test.
Recommendation: close and re-cut on current main as a stack, carrying the queue semantics forward. The product shape described here reads well — bounded FIFO, atomic claims, stale-claim recovery, :fleet_full staying a 503 rather than a 402 — and none of that is in question. It's the delivery that needs changing.
|
Closing in favour of a chained stack of seven small PRs on current
Every decision in this PR is carried. Four things are deliberately different:
On the five timing failuresThe verification notes here read "full ExUnit suite: 4,194 tests; five timing/integration failures all passed on targeted retry after branch-specific fixes". That is not a footnote in a PR whose subject is queueing under capacity pressure, and a targeted retry is exactly what CLAUDE.md says not to accept, so it was treated as a finding rather than noise. They never happened in CI. Both CI runs on That rules out the first hypothesis, a race between the event-driven drain and the Oban backstop. What is left is the local-only contention this feature is unusually exposed to.
The repo has already written this failure signature down twice, from both ends. One failure was reproducible, and it was the feature's own bug. This PR's test "a claim recovered mid-attempt is released without failing the job" stubs No The stack's own verification, rebased onto 🤖 Generated with Claude Code |
|
Correction to the verification line above: the 4,934 / 0 failures figure was the run against the stack at its original base. The stack has since been rebased onto The rebase also produced the SDK version collision the comment anticipated: 1.27.0 had been published to npm by the #1637 stack, the release gate refused it ( |
Summary
This is a clean implementation on current main. It uses the useful product shape from #1054 as reference without rebasing or reviving that branch, and incorporates the review feedback around current credit semantics, fleet-full handling, claim-before-start, channel-aware replay, terminal prompt removal, and SDK contract coverage.
Verification
Closes #1033