Skip to content

feat(conversations): store versioned execution allowances - #1775

Closed
jhgaylor wants to merge 1 commit into
feat/execution-limit-policyfrom
feat/versioned-execution-allowances
Closed

jhgaylor wants to merge 1 commit into
feat/execution-limit-policyfrom
feat/versioned-execution-allowances

Conversation

@jhgaylor

@jhgaylor jhgaylor commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Store each conversation's resolved allowance separately from ordinary conversation metadata. Partial updates retain omitted controls; widening fails; revision checks reject stale writes. A second database writer cannot replace a committed tighter allowance. Duplicate inserts fail, and rollback refuses to discard saved allowances.

Storage primitive only: this adds no user setting, endpoint, admission caller or runtime enforcement. Based on #1773; a focused replacement for part of #1745/#1754. Tenant-scoped admission and later-turn/recovery integration remain in #1732, alongside the requested-limit refusal in #1774.

Validation: 46 affected tests passed, including separate PostgreSQL connections with an observed lock wait. Removing revision checking produces four regression failures. Empty migration down/up passed; occupied rollback preserved both the allowance and migration version. The first full run caught retained test sandboxes; cleanup now removes them explicitly and asserts their absence. Full precommit passed 4,719 tests and six doctests; the dedicated database was empty afterward. Secret scanning passed. CI passed on 241d884f. No provider calls or live execution-limit acceptance.

Signed-off-by: Jake Gaylor <jhgaylor@gmail.com>

@BinaryBourbon BinaryBourbon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving.

The storage design is right: separate table so an ordinary conversation update cannot touch policy, opaque UUID revision instead of a counter, optimistic_lock on every narrowing, and new_changeset constrained by the PK so a duplicate insert cannot replace a committed allowance. The "stale omission cannot restore another field's old allowance" test is the one that matters most and it is there.

Two things I want on the record rather than blocking on:

unboxed_run is new to this repo. Nothing else under apps/ or ee/ uses it. The race test commits rows to fountain_test outside the SQL Sandbox. Within one mix test run this is safe — ExUnit runs sync cases after async ones, so nothing overlaps — and the after block cleans up. The exposure is two suites sharing one database, which happens locally with parallel worktrees: a committed conversation is visible to another run's unscoped assertions (conversations_start_test.exs:361 asserts Repo.aggregate(Conversation, :count) == 0). It is also unrecoverable if the BEAM is killed mid-test. Worth a line in the moduledoc saying the fixtures are committed on purpose and must be cleaned, so the next person adding a race test here copies the discipline and not just the shape.

The raising down is also new. No other migration in priv/repo/migrations raises on rollback. Refusing to discard saved allowances is defensible, but it means a rollback during an incident fails hard rather than degrading, and the operator has to know to empty the table first. Given the current state (nothing writes these rows) the risk is nil; it is worth remembering when this becomes load-bearing.

Minor: insert_allowance/1 is duplicated verbatim in both test modules.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants