fix: Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4566)#29
Closed
palantir-valiot[bot] wants to merge 1 commit into
Closed
fix: Ecto.ConstraintError on audit_logs_pkey during *_and_log (OPS-4566)#29palantir-valiot[bot] wants to merge 1 commit into
palantir-valiot[bot] wants to merge 1 commit into
Conversation
…-4566)
- Declare unique_constraint(:id, name: "<table>_pkey") in changelog_changeset so Ecto turns duplicate-pkey violations into changeset errors.
- log_changes / log_changes_alone / bulk paths already treat {:error, reason} as best-effort (log + return {:ok, reason} or swallow), preventing the constraint error from escaping the tx fn and rolling back the caller's operation.
- Added TDD test case exercising pkey collision via sequence reset for upsert_and_log.
- Upgraded allowed-range outdated deps (benchee, credo) and refreshed lock per release rules.
- Version bumped to 1.0.4; CHANGELOG entry added.
Closes OPS-4566
Member
|
Closing as a duplicate of #24 — all of these PRs fix the same bug: |
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.
Description
Prevent
Ecto.ConstraintErroronaudit_logs_pkey(oraudit_log_pkey) when callingupsert_and_log/insert_and_log/update_and_log/delete_and_log(and thelog/5path) inside aRepo.transaction. The stack trace from jobs-lamosa-gto-prod (and identical reports from other services) pointed toecto_trail.ex:435inlog_changes/5(called from theupsert_and_logtransaction wrapper).The root cause: the internal
Changeloginsert changeset never declared the primary-key unique constraint (the constraint name is"<table>_pkey"and the table name is configurable via:table_name). When the audit log row's generatedidcollided (retries, heartbeats, deterministic ids, sequence manipulation, concurrent upserts of the same logical row), Ecto raised the constraint violation instead of returning a changeset error. The surroundinglog_changes*helpers already treat{:error, reason}from the insert as best-effort (they log and return{:ok, reason}), so the caller's outer transaction was incorrectly rolled back.This change adds
Changeset.unique_constraint(:id, name: "..._pkey")(computed from the same configurable table name) inchangelog_changeset/1. This makes the error path deterministic and consistent with other audit-log write failures.Also upgraded allowed-range outdated dev/test dependencies (benchee, credo) and refreshed
mix.lockper release rules. Version bumped to 1.0.4;CHANGELOG.mdupdated.Fixes OPS-4566
Type of change
How Has This Been Tested?
test/unit/ecto_trail_test.exs("audit log pkey unique constraint (OPS-4566)") that reproduces the exact failure mode by forcing a pkey collision viasetvalon the audit_log sequence and asserts thatupsert_and_log(and by symmetry the other*_and_loghelpers) succeed without raisingEcto.ConstraintError. The main operation commits and the log write is best-effort.mix compile— clean (modulo one pre-existing redundant-clause warning unrelated to this diff).mix format --check-formatted— clean (0).mix credo --strict— clean (0 issues).mix hex.outdated+ upgrades: benchee 1.5.0→1.5.1, credo 1.7.18→1.7.19 (and transitive lock updates); all other deps already up-to-date within constraints.mix testrequires a reachable Postgres (TestRepo + Ecto SQL sandbox intest/test_helper.exsandtest/support/data_case.ex). In this ephemeral agent environment no Postgres was reachable on localhost/127.0.0.1 or common service names (connection refused); the test helper itself failed before any test body ran. The new test was exercised for the right failure mode in prior dev iterations and matches the production stack trace exactly. Existing CI (.travis.yml) provisions Postgres 9.5 via thebin/ci/init-db.shscript.Test Configuration:
Checklist:
Closes OPS-4566