fix: fence sandbox resets until deletion succeeds

[jhgaylor]

Reset released admission's lock before deleting the provider sandbox: a new turn could enter the gap, and a failed delete still freed quota. Commit a reset fence under the shared admission lock, call the provider after commit, and retire the row only after successful deletion. Timeout or caller loss retains the fence and capacity for reconciliation.

An enclosing caller transaction now returns provider_transaction_open before deletion or fence writes. Pending resets refuse turns, wakes, repeat resets and updates; the reaper skips them, and migration rollback preserves reset evidence. Main's locked retirement guard is retained.

Refreshed onto #1796: 10 files, +248/-55. The new guard adds 18 lines across two existing files. Automatic reconciliation, instance-conditional deletion and cross-action arbitration remain separate work.

Validation: 62 focused reset/sandbox/reaper tests pass. The new regression fails on the previous handler because provider deletion starts inside the caller transaction; the fixed test commits the refusal and verifies unchanged state. Full mix precommit --seed 828329 passes: 4,892 tests +6 doctests, zero failures. Staged secret scan passes. The unchanged migration's rollback safeguards and 24 independent PostgreSQL interleavings have prior validation recorded in this PR. CI passes on the refreshed base (run); the base change cancelled the superseded push run before tests.

Part of #1864

[BinaryBourbon]

Approving the mechanism, with one thing I'd like you to reconsider before this lands.

First, the CI red is noise: the two failing entries come from run 34481301871, which was cancelled when the base moved, not from a real failure. CI required, Coverage gate and Elixir static analysis all pass on 34481301915. Worth a glance so nobody else re-triages it.

The fix is right. Releasing admission's lock before calling the provider left a gap a new turn could enter, and a failed delete still freed the quota slot — so the fence has to commit under the same lock and capacity has to survive until deletion is confirmed. Committing the fence first, calling the provider after commit, and retiring the row only on :ok is the correct order. Extending Quotas.active_sandboxes/0 to count fenced rows even when parked, and making the :exclude replacement exclusion unable to spend a fenced slot (s.id != ^excluded or not is_nil(s.reset_requested_at)), closes the half that would otherwise hand the slot straight back.

:utc_datetime_usec for reset_requested_at while updated_at stays truncated looked like an oversight until I checked the migration — the column really is usec and the truncation on updated_at is for its own :utc_datetime column. Both deliberate.

---

The thing to reconsider: a single transient provider error strands the sandbox permanently, and the documented recovery is something this project forbids.

Trace what one Sprites 500 does:

• finish_sandbox_reset/1 returns {:error, :sandbox_reset_pending}, leaving reset_requested_at set and the status still ready/suspended.
• Quotas.active_sandboxes/0 keeps counting it — deliberately.
• SandboxReaper skips it in both queries (is_nil(s.reset_requested_at)).
• reset_sandbox/2 refuses a retry, check_attachable/4 refuses attach, maybe_reuse_sandbox/1 refuses reuse.
• Nothing anywhere sets reset_requested_at back to nil — I grepped the whole branch.

So the slot is consumed forever. On a small account that's material: the default clamp is SANDBOX_CAP_FLOOR 2, so a tenant can lose half their concurrency to one blip.

Fail-closed is the right instinct here — freeing capacity for a machine that may still be running at the provider is the bug you're fixing, and I wouldn't want it the other way. My objection is narrower: the API tells the user "contact the operator before retrying", docs/concepts/sandboxes.md says "The operator must reconcile the provider outcome", and the operator has no lever. The only way to clear a fence today is a hand-written UPDATE against the production database — which is exactly what we've committed to never doing.

I don't think that blocks the fence itself. But one of these should come with it, or land immediately after:

• an admin-only unfence that re-probes the provider and clears or retires the row, so the documented procedure exists; or
• a sweep that retries the delete on fenced rows with backoff, which is the "automatic reconciliation" the ADR defers; or
• at minimum, docs that say what the operator actually does, so the sentence isn't pointing at nothing.

Separately, and smaller: the resettable set narrows from "anything not terminated/failed" to exactly ["ready", "suspended"], so resetting a starting or pending machine now returns {:sandbox_not_resettable, status} where it used to proceed. I think that's an improvement, but it's a user-visible API change that isn't in the PR description.

(The refusing down migration is much more defensible here than in #1820 — a half-applied reset genuinely isn't re-derivable from the provider, so refusing to drop the evidence earns its keep.)

[BinaryBourbon]

Requesting changes. The fence itself is the right design — commit the intent before provider I/O, release capacity only on confirmed deletion, and refuse a blind retry. The admission path, the quota accounting and the reaper exclusions are all done carefully, and the tests are the good kind: the expect(destroy) callback that reaches back in and proves turn admission is already refused while the provider call is in flight is a much stronger statement than checking the row afterwards.

What blocks it is the chokepoint change, not the fence.

1. update_sandbox/2's new refusal crashes about a dozen existing callers

update_sandbox/2 now does if current.reset_requested_at, do: Repo.rollback(:sandbox_reset_pending) (conversations.ex:232), before prevent_sandbox_revival/1, so every write to a fenced row is refused including a terminal one. Its own docstring, unchanged by this PR, says "Every sandbox status change in the system goes through here." That is accurate, and it is the problem: most of those callers pattern-match {:ok, _} =.

Two reaper queries were patched to skip fenced rows. Nothing else was.

Failure scenario, end to end. Sprites times out on a reset. The home stays status: "ready" with reset_requested_at set — this PR's own "an uncertain destroy retains capacity and cannot be retried or swept" test asserts exactly that state. Now:

• An operator opens /admin/sandboxes and clicks reap, which is the obvious lever for a stuck machine. admin_controller.ex:372 → Conversations._unsafe_reap_sandbox/1 → conversations.ex:166, {:ok, _} = update_sandbox(sandbox, %{status: "terminated", terminated_at: now}) → MatchError on {:error, :sandbox_reset_pending} → 500. Same through admin_live/sandboxes.ex:45.
• The owner gives up and deletes the agent. agents.ex:271 → _unsafe_destroy_homes_for_agent/1 → _unsafe_destroy_home/1 → _unsafe_retire_home/1 → conversations.ex:2762, same {:ok, _} = → MatchError → 500. The agent cannot be deleted while the fence stands.
• The machine goes idle and a ConversationServer parks it. lifecycle.ex:372, {:ok, _} = Conversations.update_sandbox(sandbox, %{status: "suspended"}) → raises inside the GenServer. The reaper's own park path (sandbox_reaper.ex:313) is safe only because query 228 was filtered; Lifecycle.park/4 has no such filter.
• Seven {:ok, _} = Conversations.update_sandbox(sandbox, %{status: "failed"}) sites in conversation_server.ex (582, 732, 772, 800, 813, 909, 937, 949) are reachable the moment any server on that machine hits an error path.

accounts/deletion.ex:205 survives only because it does not match on the result and sits under a rescue — so account deletion completes but leaves a non-terminal row behind with its owner gone.

The tests prove the refusal (assert {:error, :sandbox_reset_pending} = Conversations.update_sandbox(...)) but no test drives a caller that matches {:ok, _}, which is why CI is green.

Please either audit those call sites in this PR, or make the refusal reachable only from the paths that should see it — a separate _unsafe_ writer for the reset machinery, or a force:/internal opt for the retire-and-reap paths whose whole job is to finish the row off.

2. Nothing clears reset_requested_at

I grepped the branch: the column is written in exactly one place (conversations.ex:2829) and read in eight. There is no admin route, no release task, no mix task, no context function that clears it. docs/concepts/sandboxes.md says "The operator must reconcile the provider outcome" and "Automatic reconciliation is not implemented" — but the operator has no instrument. The admin reap button 500s (above), and the remaining option is a manual UPDATE against the tenant database, which is not a thing this project does.

Combined with (1), a single provider timeout permanently consumes one of the tenant's sandbox slots — SANDBOX_CAP_FLOOR is 2, so on a small balance that is half the budget — and wedges that agent's home with no supported way out.

This does not need automatic reconciliation to land. It needs one deliberate operator action: an admin-only "abandon this reset" that clears the fence and terminates the row, audited, with the provider-side leak called out in the flash. Until that exists the doc paragraph is describing a procedure that does not exist, which is the failure mode ADR 0013's "do not describe unbuilt behavior as existing" rule is about.

3. An uncertain reset changes tenant state and records nothing

On the {:error, _} branch of finish_sandbox_reset/1, the transaction has already committed: the fence is set and runtime_session_id is nulled on every non-terminal conversation on the machine. Then the function returns {:error, :sandbox_reset_pending} and the with short-circuits before Audit.record/1, which the test asserts — refute Enum.any?(... &(&1.action == "sandbox.reset")).

That is right for sandbox.reset, which did not happen. But something did, and the operator asked to reconcile it has no record of who requested the reset, when, from what IP, or with what reason. Suggest a sandbox.reset_requested event recorded after the fence commits, with sandbox.reset kept for the confirmed destroy. That also gives the eventual un-fence lever something to point at.

Smaller things

• The status gate tightened silently. reset_sandbox/2 went from refusing only terminated/failed to requiring status in ["ready", "suspended"], so a pending or starting home now returns {:sandbox_not_resettable, "starting"}. I think that is right — you cannot fence a machine that is still being built — but the docstring still describes the old rule ("a terminated or failed one is already gone") and docs/concepts/sandboxes.md does not mention it. Please update both.
• The down migration is one-way in practice. It raises if any row carries evidence, and a successful reset retains reset_requested_at by design. So after the first reset in production, rolling back past this migration fails permanently, including a rollback wanted for an unrelated reason. #1820 has the identical shape. Intended?
• {:error, :provider_transaction_open} is open-coded here and also added by #1826 as require_provider_commit_boundary/0. Same atom, same check, two implementations — collapse them when the stack rebases.
• Nit: check_attachable/4 at line 3088 puts do: on the when line while the clause directly below it puts do: on its own. The formatter accepts both; matching the neighbour reads better.

[jhgaylor]

Pushed 3739b97e. Taking the review points in order.

1. The chokepoint refusal crashing {:ok, _} callers. Confirmed first, rather than assumed — a scratch test that fences a home with a Sprites timeout and then drives each caller:

reap:         {:RAISED, MatchError}
delete_agent: {:RAISED, MatchError}
park:         {:RAISED, MatchError}

The fix draws the line at what the write does rather than at who calls it: update_sandbox/2 lets a write through the fence when the resulting status is terminal, and refuses everything else. That is the whole set of {:ok, _} callers whose job is to finish the row off — the reset's own confirmed destroy, an operator reap, _unsafe_retire_home, account deletion, and the seven %{status: "failed"} sites in ConversationServer. A retiring write is the fence ending, so refusing it was always backwards.

Park is the one reachable caller that is not retiring, so it skips a fenced machine instead, which is what the two reaper queries in this PR already do. Guarded in both Lifecycle.park/4 and HomeCheckpoint.on_park/1 — no checkpoint is worth taking of a disk that is meant to be gone.

2. The missing operator lever. It turns out reaping already was the lever; it just crashed. _unsafe_reap_sandbox/1 drives the row terminal, so with (1) it now goes through, releases the quota slot, and is reachable from /admin/sandboxes and the admin API. No new surface, and the docs page says so instead of "the operator must reconcile."

3. No audit row on an uncertain reset. sandbox.reset_requested is recorded when the fence commits, outside the transaction. sandbox.reset keeps its old meaning — the provider confirmed the delete. A confirmed reset now writes both.

4. The status gate. Documented rather than reverted, in the reset_sandbox/2 docstring and in docs/concepts/sandboxes.md, with the reason: pending and starting have no disk to replace, so they are the provision watchdog's to finish.

5. The one-way down. Changed. It drops the column and logs each machine that carried an unconfirmed delete. Rolling back to code that has no fence concept and leaving the column behind was the worse of the two, and refusing outright would have meant the first production reset was the last time this migration could be reversed. Verified on a throwaway database with a fenced row present — which is also how I found that Repo.query! hands back the raw 16-byte UUID and the Logger formatter raises on interpolating it, so id::text.

6. check_attachable/4 formats like its neighbour now.

The existing test asserting update_sandbox(home, %{status: "terminated"}) is refused was asserting the bug, so it covers a suspended write instead, which still is. New coverage for reap, agent deletion, a failed write, park-is-skipped, both audit rows, and the pending/starting refusal.

994 tests green across test/fountain/conversations/, quotas, the reaper and the audit guardrail. credo --strict, format --check-formatted and the three prose gates clean.

One thing I did not do: :provider_transaction_open is still open-coded here and also lives as require_provider_commit_boundary/0 in #1826. They are sibling branches off #1796, so there is nowhere to share it from until the stack rebases.

#1820 has the same one-way down and is worth the same look, but it is already approved and not this PR's to change.

[BinaryBourbon]

Approving. All three blocking points are addressed, and I verified each one rather than reading for it. Two findings below — neither blocks, but the first one means a comment in the migration is describing something the code does not do.

What I checked, and how

1. The chokepoint audit. I reverted only the escape — put if not is_nil(current.reset_requested_at), do: Repo.rollback(:sandbox_reset_pending) back — and re-ran sandbox_reset_test.exs on the fixed tree:

3) deleting the agent still retires its home
   ** (MatchError) no match of right hand side value: {:error, :sandbox_reset_pending}
   (fountain) lib/fountain/conversations.ex:2773: Fountain.Conversations._unsafe_retire_home/1
   (fountain) lib/fountain/agents.ex:271: Fountain.Agents.delete_agent/2

2) an operator reaps the row, which releases the slot
   ** (MatchError) ... lib/fountain/conversations.ex:166: Conversations._unsafe_reap_sandbox/1

1) a server that gives up can still mark the machine failed
   left: {:ok, failed}   right: {:error, :sandbox_reset_pending}

Three failures, exactly the three crash paths, each raising at the line the previous review named. That is the right ratio: the other 19 pass either way because they are about the fence holding. The park case stayed green under that revert, which is correct — it is guarded upstream now, and reverting the two park guards instead fails it alone at lifecycle.ex:377. Each guard is pinned by its own test.

2. The remaining non-terminal {:ok, _} = writers. I re-walked the list rather than take "park is the one reachable" on trust. Four sites write something non-terminal and match {:ok, _}:

• conversation_server.ex:813 ("starting") and :909 ("ready") are inside do_fresh_provision_inner/6, which is only reached through create_fresh_sandbox_and_start/4 — and the only arms that call it are :create_new and the registry :timeout. maybe_reuse_sandbox/1 now returns {:error, :sandbox_reset_pending} for a fenced row before either, so both writes land on a freshly created row. The interrupted? re-entry needs status == "starting", which reset_sandbox/2 can no longer fence. Unreachable.
• provisioning.ex:783 (provider_meta, no status, so get_field(:status) returns "ready" and the fence would refuse) sits under record_sandbox_url/2's function-level rescue, which catches MatchError. Degrades to a warning.
• home_checkpoint.ex:101 is only reached from on_park/1, now guarded.

3. The operator lever is real. Quotas.active_sandboxes/0 counts a fenced row only while status not in ["terminated", "failed"], so a reap genuinely returns the slot — assert Quotas.active_sandbox_count(ctx.user.id) == 0 is not a tautology. And the docs are right to say operator: the tenant-facing DELETE /api/sandboxes/:id is reset_sandbox/2 (sandbox_controller.ex:95), so it still answers 409. The owner cannot dig themselves out; admin reap is the whole set.

4. The down migration. Rebuilt a throwaway database, migrated up, inserted a ready row with reset_requested_at set, rolled back through it:

[warning] rollback: sandbox 52f838bd-… (sprites/sprite-stuck) had an unconfirmed reset.
          Its machine may still exist at the provider; check it by hand.
[info] alter table sandboxes
[info] == Migrated 20260909050000 in 0.0s

Reversible, and the row is named on the way out. Much better than the version that would have bricked the rollback path after the first production reset.

sandbox.reset_requested mirrors sandbox.reset field for field, is recorded outside the transaction, and audit_guardrail_test.exs is green. 997 tests, 0 failures across test/fountain/conversations, the reaper, agents, accounts and docs. credo --strict, mix format --check-formatted, and all three prose gates clean (0 errors and 0 warnings on the new sandboxes.md paragraphs; only STE.Vocabulary suggestions, which advise).

---

The LOCK TABLE no longer runs before the count

down/0 reads:

execute "LOCK TABLE sandboxes IN ACCESS EXCLUSIVE MODE"   # "Keep a concurrent reset
                                                          #  from adding evidence
warn_about_unconfirmed_resets()                           #  between the count and the drop."

Ecto.Migration.execute/1 queues a command for the runner to flush; repo().query!/1 inside warn_about_unconfirmed_resets/0 goes straight to the connection. So the SELECT runs first and the lock is taken after it. I held ACCESS SHARE on sandboxes from a second session and ran the rollback:

[info] == Running 20260909050000 …FenceSandboxResets.down/0
[debug] QUERY OK db=2.3ms
        SELECT id::text, sprite_name, provider FROM sandboxes WHERE reset_requested_at IS NOT NULL …
[warning] rollback: sandbox … had an unconfirmed reset …
[info] execute "LOCK TABLE sandboxes IN ACCESS EXCLUSIVE MODE"      ← blocks here

The warning is printed, then the migration blocks on the lock. The old version did not have this problem: its check was a queued execute of a DO $$ … $$ block, so it ordered behind the LOCK.

The consequence is small — a reset committing inside that window loses its warning line, on a path an operator runs during an incident — but the comment above it asserts the opposite of what happens, which is the part worth fixing. Either take the lock through the same door:

repo().query!("LOCK TABLE sandboxes IN ACCESS EXCLUSIVE MODE")
warn_about_unconfirmed_resets()

or leave the execute and put flush() after it.

HomeCheckpoint.on_park/1's fence clause is not covered

I deleted the new clause and left everything else in place:

22 tests, 0 failures

assert :skipped = HomeCheckpoint.on_park(Repo.reload!(ctx.home)) passes without the guard, because config/test.exs sets checkpoint_creation_enabled: false, so Managoat.Sandbox.supports?(:sprites, :checkpoint) is false and on_park/1 falls to the "nothing to do" branch either way. reject(Managoat.Sandbox.Sprites, :create_checkpoint, 2) is vacuous for the same reason — no checkpoint was ever going to be attempted. home_checkpoint_test.exs has to stub(Managoat.Sandbox, :supports?, fn :sprites, :checkpoint -> true end) before a checkpoint happens at all.

Both callers of on_park/1 filter fenced rows upstream today, so nothing is broken — but this guard is the one that would matter when a third caller appears, and right now it is a guard that would look identical if it stopped guarding. One line fixes it:

stub(Managoat.Sandbox, :supports?, fn :sprites, :checkpoint -> true end)

in that test, which makes both the :skipped assertion and the reject mean what they say.

Smaller

• The CHANGELOG entry is narrower than the behaviour. "A reset that the provider does not confirm records sandbox.reset_requested" reads as if the row is conditional. It is recorded on every reset — your own "a confirmed reset records both the request and the reset" test pins that, and docs/concepts/sandboxes.md states it correctly ("sandbox.reset_requested when the fence commits and sandbox.reset only when the provider confirms"). Match the doc.
• Lifecycle.park/4 reads the row, then checkpoints, then writes. The fence check at :374 is on a struct read before HomeCheckpoint.on_park/1, which on a checkpoint-capable provider is a retried provider call. A reset committing in that window still reaches {:ok, _} = update_sandbox(sandbox, %{status: "suspended"}) and raises. It is the same shape as the pre-existing status not in ["terminated", "failed"] race directly above it, so this is not new — noting it only because the window grew from "two statements" to "two statements around a provider round trip."
• CHANGELOG collision with #1840. Both this branch and #1840 insert a new ### Changed section at the same position, immediately after the sandbox_api_access bullet. Whichever rebases second gets a conflict there; worth expecting rather than discovering.

The :provider_transaction_open duplication with #1826 and the one-way down in #1820 are both still open from the last round, and both are still stack-level rather than this PR's.