Skip to content

Retirement races match the changeset error list exactly, in four places #2039

Description

@jhgaylor

What

The #1766 campaign handles a sandbox retiring mid-flight by matching the changeset update_sandbox/2 rejects with:

{:error, %Ecto.Changeset{errors: [status: {"sandbox is retired", []}]}} ->

That literal appears four times — three in conversation_server.ex (#1943, #1944, #1945) and one in lifecycle.ex (#1946) — and the #1767 chain adds more of the same shape.

Why it is fragile

The pattern only matches when the retirement error is the sole error on the changeset. prevent_sandbox_revival/1 adds it to whatever Sandbox.changeset(current, attrs) already produced, so one more validation firing beside it — a new field constraint, a stricter status enum — makes errors a two-element list and every one of these sites falls through to the path it was written to avoid:

  • do_fresh_provision/6 and the wake path raise MatchError and take the conversation down;
  • the provision-completion site fails the conversation and releases a session a replacement may own;
  • park/4 raises inside the reclaim path.

No test would catch it. Each PR's own negative test ("an unrelated ready-write error still fails provisioning") asserts a different single error, so it passes either way.

Suggested shape

A predicate beside the error's producer in Fountain.Conversations, plus a wrapper the call sites use:

def sandbox_retired?(%Ecto.Changeset{errors: errors}),
  do: errors[:status] == {"sandbox is retired", []}

def sandbox_retired?(_), do: false

@spec claim_sandbox(Sandbox.t(), map()) :: {:ok, Sandbox.t()} | :retired | {:error, term()}
def claim_sandbox(sandbox, attrs) do
  case update_sandbox(sandbox, attrs) do
    {:ok, updated} -> {:ok, updated}
    {:error, error} -> if sandbox_retired?(error), do: :retired, else: {:error, error}
  end
end

Call sites then match the atom :retired, which is shorter than the pattern it replaces and robust to a second error appearing. {:error, :sandbox_reset_pending} passes through unchanged, so park/4 keeps its separate clause.

Not doing it now

The #1766 chain carries 22 approved stack:1767 PRs on top of it, so rewriting those four branches to swap the call sites would force a rebase of the whole campaign. This is a follow-up for after it lands — the conversion is mechanical once the file has stopped moving.

Found while reviewing stack:1766 for merge readiness. Refs #1766, #1767, #2032.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:conversationsConversation lifecycle, turns, streaming, reattach, and orchestration

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions