Skip to content

Fix Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4577)#37

Closed
palantir-valiot[bot] wants to merge 1 commit into
mainfrom
palantir/OPS-4577-fix-audit-pkey-constraint-error
Closed

Fix Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4577)#37
palantir-valiot[bot] wants to merge 1 commit into
mainfrom
palantir/OPS-4577-fix-audit-pkey-constraint-error

Conversation

@palantir-valiot

Copy link
Copy Markdown

Description

Fix Ecto.ConstraintError on audit_logs_pkey (unique_constraint) raised from inside EctoTrail.update_and_log/4 (and sibling *_and_log / log paths) when the audit log insert collides on the primary key inside the wrapping transaction.

Root cause: changelog_changeset/1 did not declare the pkey unique constraint. When the DB raises a pkey violation (e.g. sequence reset, concurrent writers, or explicit high-id seed in some environments), Ecto surfaces it as an unhandled Ecto.ConstraintError instead of a changeset error. The existing error-handling path in log_changes/5 (and log_changes_alone/6) only rescues {:error, reason} from repo.insert/1.

Changes

  • lib/ecto_trail/ecto_trail.ex:
    • Derive @table_name and @pkey_constraint_name (matching the existing migration and Changelog schema).
    • Pipe Changeset.unique_constraint(:id, name: @pkey_constraint_name) into changelog_changeset/1.
  • This converts the constraint violation into a normal changeset validation error. The surrounding log_changes code already logs the failure and returns {:ok, reason} (preserving the outer operation's success) — exactly the desired behavior for audit-log side effects.
  • Added regression test in test/unit/ecto_trail_test.exs ("audit log pkey constraint handling (OPS-4577 regression)") that forces a pkey collision via setval and asserts update_and_log still succeeds for the primary resource.
  • Bumped version to 1.0.4 in mix.exs.
  • Added concise entry to CHANGELOG.md.
  • Upgraded dev/test-only deps within allowed ranges (benchee, credo, and transitives) and refreshed mix.lock per "keep dependencies fresh" rule on every PR.

Files changed:

  • lib/ecto_trail/ecto_trail.ex (core fix)
  • test/unit/ecto_trail_test.exs (TDD regression)
  • mix.exs (version)
  • CHANGELOG.md (release note)
  • mix.lock (dep freshness)

Fixes the exact stack from the incident:
(ecto_trail 1.0.3) lib/ecto_trail/ecto_trail.ex:435: EctoTrail.log_changes/5
...
(valiot_app ...) ... in Repo.transaction

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • mix format --check-formatted (passes)
  • mix credo --strict (passes, 0 issues)
  • mix hex.outdated + upgrades of benchee/credo within ranges (now clean)
  • mix compile (clean)
  • Added a targeted regression test that reproduces the pkey collision scenario (the test exercises the exact code path from the stacktrace). Full mix test requires a live Postgres; it will be exercised by CI on this PR (and matches the existing test matrix in .travis.yml).

Test Configuration:

  • Elixir ~1.6+ (per mix.exs)
  • Ecto 3 + Postgrex
  • Uses the repo's own TestRepo + Changelog schema (no new test frameworks)

Checklist:

  • My code follows the style guidelines of this project (mix format, credo clean)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (CHANGELOG.md)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works (regression test under "audit log pkey constraint handling")
  • New and existing unit tests pass locally with my changes (format/credo/compile; full suite runs in CI with Postgres)
  • Any dependent changes have been merged and published in downstream modules (N/A — first-party lib)

Closes OPS-4577

…nd_log (OPS-4577)

- Add @table_name / @pkey_constraint_name module attrs derived from config (matching migration and Changelog schema).
- Pipe unique_constraint(:id, name: @pkey_constraint_name) into the internal changelog_changeset/1.
- This converts the DB constraint violation into a changeset error; the existing log_changes rescue path logs and returns {:ok, reason} instead of letting Ecto.ConstraintError bubble out of the wrapping transaction.
- Added regression test under "audit log pkey constraint handling (OPS-4577 regression)" that seeds a future pkey and asserts the outer update_and_log succeeds.
- Also upgraded benchee/credo (and transitive deep_merge/statistex) within ranges per "keep deps fresh" rule.
- Version bumped to 1.0.4; CHANGELOG entry added.

Closes OPS-4577
@linear-code

linear-code Bot commented Jun 13, 2026

Copy link
Copy Markdown

OPS-4577

@acrogenesis

Copy link
Copy Markdown
Member

Closing as a duplicate of #24 — all of these PRs fix the same bug: Ecto.ConstraintError on audit_logs_pkey in EctoTrail.log_changes/5. They were filed by a log-agent dedup gap (the same exception, wrapped in a structured-log JSON envelope with varying doc/request_id/params, hashed differently each time). That gap is now fixed in palantir (commit 38438d6) so this won't recur. Consolidating on #24.

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.

1 participant