fix: declare unique_constraint on audit_log pkey to prevent Ecto.ConstraintError in *_and_log (OPS-4594)#59
Closed
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…traintError in *_and_log paths
- Add @audit_table/@pkey_constraint_name module attrs derived from config (defaults to audit_log_pkey)
- Apply unique_constraint(:id, name: @pkey_constraint_name) in changelog_changeset/1
- This covers log_changes/5 (used by update_and_log, insert_and_log, upsert_and_log, delete_and_log) and log_changes_alone
- Add TDD test that forces a deterministic pkey collision on audit_log and asserts the main operation still succeeds (existing log error path already swallows {:error, changeset})
- Bump version 1.0.3 -> 1.0.4; add concise CHANGELOG entry
Closes OPS-4594
The root cause was the bare repo.insert() of a cast-only Changelog changeset with no constraint declaration; when the audit table sequence produced a duplicate pkey (observed in prod under concurrent load), Ecto raised ConstraintError instead of returning a changeset error that the library already handles gracefully.
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a
unique_constraint(:id, name: <table>_pkey)declaration insidechangelog_changeset/1so the barerepo.insert/1performed bylog_changes/5(andlog_changes_alone) for every*_and_log/logpath turns a duplicate pkey violation into a changeset error instead of letting Ecto raiseEcto.ConstraintError.The error path already existed and was intentionally swallowed (logs the reason and returns
{:ok, reason}), so callers continue to see success for the primary operation while the audit write is best-effort.Files changed:
lib/ecto_trail/ecto_trail.ex:574(core fix + two small module attrs for the constraint name, derived from the sametable_nameconfig the schema already uses)test/unit/ecto_trail_test.exs(TDD test that forces a deterministicaudit_logpkey collision on theupdate_and_logpath and asserts the main update still succeeds)mix.exs(1.0.3 → 1.0.4)CHANGELOG.md(concise entry)Why
Production (eliot-lamosa-gto-prod) hit:
Root cause:
changelog_changeset/1only didChangeset.cast/3; nounique_constraint/3was ever declared for the pkey of the (configurable) audit table. Under concurrent load the sequence can emit a value that another writer has just used, producing the exact pkey collision visible in the stacktrace.See Linear OPS-4594 and the triage analysis (NOTIFY+FIX, code_bug).
Assumptions
idpkey whose Postgres constraint name is<table_name>_pkey(the defaultaudit_logproducesaudit_log_pkey; the observed prod name wasaudit_logs_pkeybecause some tenants use a plural table name via config). The constraint name is computed the same way the schema already derives the table name.log_changes/5andlog_changes_aloneis intentional and sufficient; we only need to stop the exception from escaping.Test plan
mix format(clean)mix compile(clean; only pre-existing redundant-clause warning on an unrelated helper)describe "update_and_log/3 pkey constraint resilience"that:setvalso the nextnextval()inside the library-produced insert will collide on that pkeyupdate_and_log{:ok, updated_resource}(no ConstraintError) and that the main resource row was mutatedmix test— the suite has always required a running Postgres (seetest/test_helper.exs:82). In this k8s per-task pod no Postgres listener is available, so the harness cannot reach the "migrate + sandbox" steps. The compile-time and formatting gates pass; the new test was executed mentally against the exact failure mode from the stacktrace and will be exercised by CI on PR (or by any developer withdocker compose up/ the project's normal test DB). No other verification scripts are declared inmix.exs,AGENTS.md(absent), orpackage.json(none).git diff origin/main..HEAD: only the four files above; no debug prints, no unrelated refactors, no new deps, no scope creep..github/pull_request_template.mdpresent; skeleton not used verbatim).Closes OPS-4594