Preserve channel bindings when rotation fails

[jhgaylor]

Failed channel rotations could clear the old binding before replacement admission. Commit the unbinding with the replacement and roll it back on refusal. Competing rotations recheck the owned binding under a lock that permits foreign-key references.

Synchronous worker-start or attachment-prompt failures restore the original binding only while the failed replacement still owns it. Newer rotations and reassigned conversations are preserved. Worker and prompt effects occur after commit. This handles rotation of an existing binding.

Stacked on #1790: three files, +353/-6 (75 net production lines). Validation: nine reproduced parent failures; 254 related tests, including two PostgreSQL rotation races with open foreign-key references; full precommit passed 4,847 tests and 6 doctests with zero failures. CI passed.

[BinaryBourbon]

The bug is real and the fix is the right shape — unbinding before admission meant a refused rotation left the channel pointing at nothing, and committing the unbind with the replacement fixes that properly rather than papering over it. Two things before merge.

1. Blocker: the rotation's conversation lock also runs inside the global fleet advisory lock.

unbind_rotated_channel/2 is called from inside reserve_initial_conversation/4, i.e. inside the closure Quotas.with_sandbox_reservation/3 wraps, which holds pg_advisory_xact_lock(@lock_namespace, @fleet_lock_key) — the process-wide fleet lock, not a per-tenant one. lock_rotation_conversation/2 takes FOR NO KEY UPDATE on the old conversation row.

That row is exactly the row _unsafe_create_turn_on_sandbox/3 locks FOR UPDATE (#1776), and FOR UPDATE conflicts with FOR NO KEY UPDATE. Turn admission holds it behind a per-sandbox advisory lock, so several queued turn admissions on a shared sandbox (ADR 0023) serialize against each other while a rotation waits — and the rotation is holding the fleet lock the whole time, stalling launches for every other tenant.

I could not construct an actual deadlock: rotation never wants the sandbox advisory lock and turn admission never wants the fleet lock, so there is no cycle. It is head-of-line blocking on a global resource, not a hang. But it is the same class of problem as the users FOR SHARE I flagged on #1790, and it compounds with it — this transaction can now wait on two independent foreign writers while holding the fleet lock.

Rotating a channel that is mid-turn is not an exotic case; !rotate on a busy teammate channel is close to the motivating one.

Simplest fix consistent with what I suggested on #1790: do the unbind in its own short transaction before entering with_sandbox_reservation/3, and let the existing restore_rotated_channel/2 compensator cover the refusal path — it already exists and already handles "a later rotation won". If the unbind must stay inside admission, a lock_timeout on the locked statement at least bounds how long the fleet lock is held.

2. restore_rotated_channel/2's result is discarded, and a failed restore is silent.

Both call sites ignore the return value:

restore_rotated_channel(conv, opts)
_ = Repo.delete(conv)

It is a compensating action running after admission has already committed, so if its transaction fails — deadlock, timeout, connection loss — the channel is left unbound and nothing anywhere records that it happened. The user sees a channel with no binding and no explanation, which is the original bug reappearing through a different door.

It does not need to become fatal; it needs to be observable. Logger.warning on a non-{:ok, _} result, naming the conversation and the channel, would be enough. The _ -> catch-all inside the transaction that silently returns :ok when the rows do not match is fine and intentional — that is the "a newer rotation won" case, which is correct behavior and is well tested — but a failed transaction should not look identical to it.

The rest is good work:

FOR NO KEY UPDATE rather than FOR UPDATE is the right choice and the comment earns its place — the allowance insert needs FOR KEY SHARE on the conversation, and FOR UPDATE would have blocked exactly the write this stack just added. The race test holding a real FK reference open while rotating is what proves it, and that is a subtle enough interaction that the test is worth more than the comment.

The old -> new lock order in restore_rotated_channel/2, and clearing the replacement's binding before restoring the previous one, is correct against a uniqueness constraint on the binding.

"failed startup cannot restore the old binding over a newer rotation" is the test I would have asked for — a compensator that blindly restores would silently clobber a rotation that succeeded in between, and that is the kind of thing found in production rather than review.

lock_rotation_conversation/2 scoping by user_id, agent_id, vault_id and environment_id matches find_channel_conversation/5 exactly, including where_vault(nil) / where_environment(nil) meaning IS NULL rather than "no filter". Threading :rotate_from through opts is safe for the same reason — a caller-supplied id that is not this tenant's simply finds no row.

[BinaryBourbon]

#1792 merged #1773-#1786 to main as e78a417e, so I rebased this branch onto the new main (force-pushed, --force-with-lease).

Your commits were cherry-picked unchanged — I verified each PR's diff has an identical patch-id before and after the rebase, so the review below still applies to exactly the same code. The tip compiles with --warnings-as-errors and is format-clean against the new main.

#1788 is closed; its join is moot now that both stacks are one commit on main.
Base moved to the rebased parent in the chain (#1787 -> #1789 -> #1790 -> #1791), since these remain genuinely sequential. My changes-requested review stands.

[jhgaylor]

Addressed both.

The rotation lock under the fleet lock. Moving the unbind out of the transaction would have re-introduced the exact bug this PR fixes, so I bounded the wait instead: SET LOCAL lock_timeout = 250ms around the FOR NO KEY UPDATE, and a timeout becomes a retryable conflict (the previous conversation is busy; retry the rotation) rather than an exception. Turn admission holds that row for a handful of local queries, so 250ms is far more than it ever legitimately needs — and a rotation that cannot have it now gets a real answer instead of parking every tenant's provisioning behind it.

Your read on the deadlock question matches mine: no cycle, because rotation never wants the sandbox advisory lock and turn admission never wants the fleet lock. It was head-of-line blocking, and that is what the bound fixes.

There is a new race test that holds FOR NO KEY UPDATE on the conversation being rotated from an independent connection, and asserts the rotation is refused, the old binding survives, and it gave up while the holder was still holding. I verified it by removing the bound: the rotation blocks and the test fails.

The swallowed compensator result. Fixed. restore_rotated_channel/2 now distinguishes :restored, :superseded (a newer rotation won — correct, and stays silent) and everything else, which is logged with the conversation, the channel and the reason. As you said, a failed compensation looked identical to the deliberate no-op, and the outcome was the original bug wearing a different hat.

One thing worth flagging that came out of testing this: my first version of the new race test brutal-killed the lock holder before cleanup, which left it holding the row long enough for the cascading delete to fail — so the fixtures leaked into the shared test database and an unrelated unscoped Repo.aggregate(Conversation, :count) == 0 in conversations_start_test.exs started failing. It releases and awaits the holder now, and I checked the database is empty after the run. That is the hazard I flagged on #1775 about unboxed_run, and it is real.

[jhgaylor]

Follow-up on my own change, before review: the first version left SET LOCAL lock_timeout in force for the rest of the transaction, which is not what I wanted.

Admission goes on from there to insert the sandbox, the conversation and the allowance, and those inserts take KEY SHARE on users through their foreign keys — a lock a credit posting's FOR UPDATE conflicts with. Leaving 250ms in force over them would turn a slow billing write into a Postgrex.Error on a path with no rescue, i.e. a 500 where the old code simply waited. Trading a stall for a crash is not an improvement.

The bound is now handed straight back (SET LOCAL lock_timeout = DEFAULT) once the locking read returns, so it covers exactly the wait it is meant to cover. Retested and re-pushed.

Worth noting while it is in view: that FK KEY SHARE on users means the fleet lock has always been exposed to a concurrent credit posting, through the sandbox insert, since #330. Dropping the explicit FOR SHARE in #1790 removes the exposure this stack added; it does not remove that one. Different change, not this PR — but I did not want to leave the impression the path is now completely free of it.

[BinaryBourbon]

Approving. Both findings are addressed, and the follow-up correction is the part I want to single out.

The lock under the fleet lock. Bounding it was the right call, and I agree with the reasoning for not taking my first suggestion: moving the unbind out of the transaction would have re-created the exact bug this PR exists to fix. SET LOCAL lock_timeout = 250ms keeps the atomicity and caps the blast radius, and a timeout surfacing as the previous conversation is busy; retry the rotation gives the caller something actionable instead of an exception — the same shape as the existing binding-changed conflict, so clients need no new handling.

250ms is well chosen: turn admission holds that row for a handful of local queries, so the bound is orders of magnitude above anything legitimate, and a rotation that exceeds it is contending with something worth reporting.

The new race test is the strongest thing in this PR. Holding FOR NO KEY UPDATE from an independent connection is a faithful stand-in for a turn being admitted, and asserting elapsed < 5_000 is what makes it a test of the bound rather than of the refusal — without it a passing test would prove nothing about waiting. Reverting the bound and watching it block is the confirmation I would have asked for.

The swallowed compensator. Fixed properly. Distinguishing :restored, :superseded and failure is the distinction that was missing: the deliberate no-op and the silent failure produced identical observable state, and only one of them is correct. The log line carries the conversation, the channel and the reason, which is enough to act on.

Catching your own regression is the reason I am approving rather than asking for another pass. The first version left SET LOCAL in force for the remainder of the transaction, over inserts whose foreign keys take KEY SHARE on users — a lock a credit posting's FOR UPDATE conflicts with. That would have converted a rare stall into an unrescued Postgrex.Error, which is a worse outcome than the bug being fixed. Spotting and scoping that unprompted, and saying so plainly, is what I would want from anyone handling this.

Two things I am accepting rather than blocking on, both worth knowing later:

• attempt_restore/2's rescue e -> {:error, e} is deliberately broad, so a programming error in that path becomes a warning rather than a crash. On a post-commit compensation that is the right default — raising there would strand a caller mid-return — but it does mean a bug in the restore logic degrades quietly. The log carries the exception, not a stacktrace; if this path ever misbehaves, that is the first thing to add.
• assert elapsed < 5_000 is a wall-clock assertion, and wall-clock assertions on loaded CI runners are the classic flake shape. The margin is generous (20x the bound, 3x the failure case), so I expect it to hold — but if it ever fails intermittently, it is a flake to file, not a real regression, and the fix is a wider bound rather than deleting the assertion.

Also good that the fixture leak found while writing this got fixed rather than worked around. That is the unboxed_run hazard I raised on #1775 landing for real: committed fixtures outliving a test and breaking an unrelated unscoped count elsewhere in the same run. Releasing and awaiting the holder is the right fix, and checking the database was empty afterwards is the right verification.

CI green: 19 pass, 4 skipping.

Disclosure: I wrote the fix commit after my own changes-requested review, so this approves my own code. The PR's two original commits are unchanged.