Refuse launches inside an enclosing transaction

[jhgaylor]

Create, attach and wake could start a worker or contact its provider inside a caller's still-open database transaction. A later rollback could remove the rows after that work escaped. Refuse these entrances with {:error, :provider_transaction_open} before writes or external work; callers must commit before launching.

Two files, +70/-3, extracted from #1754 onto #1796. No new tables or worker protocol.

Validation: 65 create/attach/transaction tests pass. All three new regression cases fail on the unchanged callers. Tests commit the enclosing transaction after refusal and verify no rows or sandbox status changed. Full mix precommit --seed 828329 passes: 4,888 tests +6 doctests, zero failures. Staged secret scan passes. CI passes (run). Follow-up #1831 scopes initial worker-start failures.

Part of #1864

[BinaryBourbon]

Approving. Small, correct, and the regression tests genuinely hold the line.

I verified the "all three new regression cases fail on the unchanged callers" claim rather than taking it: replaced require_provider_commit_boundary/0's body with a bare :ok and re-ran the file.

1) test create refuses an enclosing transaction before external work or row changes
2) test wake   refuses an enclosing transaction before external work or row changes
3) test attach refuses an enclosing transaction before external work or row changes
3 tests, 3 failures

All three, and 0 failures with the guard restored. That's a real guard, not one that passes either way.

The bug is worth fixing: a nested Repo.transaction doesn't commit, so a worker started or a provider contacted inside a caller's still-open transaction escapes a rollback that then removes the rows it was started for. Putting the check as the first clause of each with — before get_agent, before any write, before any provider call — is the right position, because the whole point is to refuse before anything leaks.

Things I checked:

• The three existing callers (Team.start_fresh/6, Team.open_on_new_sandbox/4, Schedules.run_one_off/2) are not inside transactions, so nothing legitimate starts returning :provider_transaction_open.
• Repo.in_transaction?/0 is false at the top level under DataCase's SQL Sandbox, so this doesn't refuse every test — confirmed by the suite passing and by the tests needing an explicit Repo.transaction to provoke the refusal.

The test design deserves a note: committing the enclosing transaction after the refusal, rather than letting the error roll it back, is what actually proves nothing pending was written. A test that rolled back would pass even if the guard did nothing. Same for the reject/2 calls on start_child, Sprites.get/create/resume — they assert no external work was attempted, which is the half a row count can't see.

[BinaryBourbon]

Approving. Small, and it closes a real hole: start_conversation/2 and wake_conversation/2 start a ConversationServer under Horde and make provider calls. Inside a caller's transaction those escape a boundary that can still roll back — the sandbox row vanishes, the worker does not.

The guard is placed as the first clause of each with, so the refusal happens before get_agent, before any reservation and before any row is written. The test asserts that directly by rejecting Horde.DynamicSupervisor.start_child/2 and the three Sprites entry points and then comparing Conversation/Sandbox counts across all three paths. The comment on the test is the part I liked most: the caller commits rather than rolling back, so the assertion proves nothing pended rather than proving the rollback cleaned up.

Two notes, neither blocking:

• {:error, :provider_transaction_open} has no FallbackController clause, so if it ever escapes to a controller it is a generic 500. That is arguably correct — it is a programmer error, not a client error — but it does mean a caller can pattern-match it away and carry on. If the intent is "this must never happen," raise is the stronger signal; if the intent is "a caller may legitimately probe," it wants a clause. Worth one sentence in the docstring either way.
• The private helper is named for the boundary, not the check, which reads well at the call sites. Keep it that way when #1768 adds its own Repo.in_transaction? refusal to reset_sandbox/2 — that one currently open-codes the same test and returns the same atom. One helper, two callers.