DB: enforce a UNIQUE natural key on soroban_events (tx_hash, event_index) - #331
Merged
Depo-dev merged 3 commits intoJul 31, 2026
Merged
Conversation
…event_index, network)
- Add migration 0010 that adds UNIQUE (transaction_hash, event_index, network)
constraint to soroban_events. Includes a pre-flight duplicate check that
raises an exception if any conflicting rows exist before applying the DDL.
- Decision record (in migration + db/mod.rs docs):
The UUIDv5 id is derived from (contract_id, ledger_sequence, event_index)
and is NOT a pure function of the Stellar protocol natural key
(transaction_hash, event_index). The new constraint is therefore a
complementary, independent correctness guard that enforces the protocol-
level uniqueness guarantee regardless of the id derivation scheme.
The constraint is network-scoped to allow the same tx hash to appear on
different networks (testnet / mainnet) in multi-network deployments and to
align with the partition-key candidate identified in Telocel-Labs#244.
- Update schema.sql to mirror migration 0010 end state (bump comment from
0001-0009 to 0001-0010, add constraint).
- Propagate network parameter through both insert paths:
crates/indexer/src/db/mod.rs — insert_event now takes &str network;
populates the network column explicitly instead of relying on the
DEFAULT 'testnet' column default.
crates/backfill/src/db.rs — same signature change.
crates/indexer/src/streamer/mod.rs — passes self.config.network.
crates/backfill/src/main.rs — clones args.network into each worker.
- Add two new integration tests in crates/indexer/src/db/mod.rs:
event_uuid_does_not_include_transaction_hash — documents that the UUID
key is distinct from the natural key.
natural_key_constraint_rejects_duplicate_tx_event_index — inserts two
events with different contract_ids but the same (tx_hash, event_index,
network) and asserts the second insert is rejected by the constraint.
Closes #<issue>. Targets dev.
|
@brite-side0 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Collaborator
|
@brite-side0 Please fix your CIs |
Collaborator
|
@brite-side0 Please fix the merge conflicts |
Depo-dev
added a commit
that referenced
this pull request
Jul 29, 2026
…in prod The duplicate 0018/0019 pair reached dev because nothing checked for it: CI's integration job applies migrations with a `for f in *.sql` psql loop rather than the sqlx migrator, so the primary-key collision on _sqlx_migrations only shows up when someone runs `sqlx migrate run` against a real database. This is not hypothetical going forward — three open PRs would each reintroduce it on merge: #331 and #351 both add a second 0010, and #367 adds a second 0022. The failure mode is easy to hit honestly, since contributors branch from an older dev and take "the next number". The check runs in the env-reference job (already cheap and dependency free) and prints which files collide plus how to renumber safely. Refs #371
Brings the branch up to date with dev, which has moved a long way since it was opened. Resolutions: - crates/indexer/src/db/mod.rs — took dev. It is 1152 lines against this branch's 476, and the branch's change here was documentation plus a `network` parameter dev already carries. - crates/indexer/src/streamer/mod.rs — took dev. It has replaced per-event inline insert + Redis publish with batched page commits and a transactional outbox relay (Telocel-Labs#199, Telocel-Labs#200). The natural-key constraint this PR adds is a database-layer guard and does not depend on the old insert path. - crates/backfill/src/db.rs — combined both. Kept dev's `ON CONFLICT (ledger_sequence, id)`, which is required because soroban_events is RANGE-partitioned on ledger_sequence (migration 0017) so the partition key must appear in the conflict target, and restored this branch's `network` column, which the constraint is scoped by. Carried over the branch's doc comment explaining how the two keys complement each other, since that is the actual insight in this PR. - database/schema.sql — took dev, then corrected three stale migration references it still carried (event_outbox is 0011 not 0010, usage_rollup is 0024 not 0010, and the header range is 0001-0025 not 0001-0013). Renumbered the migration 0010 -> 0025. Its original number now collides with 0010_token_events on dev, and sqlx derives a migration's version from the filename prefix, so the pair would collide on the _sqlx_migrations primary key. scripts/check-migration-versions.sh caught it and now reports 25 migrations with no duplicates. The insert strategy is unchanged, as this PR's own decision record specifies: `ON CONFLICT (id) DO NOTHING` stays primary, and UNIQUE (transaction_hash, event_index, network) is the independent safety net. cargo fmt --check, cargo clippy --workspace --all-targets -D warnings, and cargo test --workspace (281 tests) all pass on the merged result.
The Rust job failed at "Run migrations". 0025 tried
ALTER TABLE soroban_events
ADD CONSTRAINT uq_soroban_events_tx_index_network
UNIQUE (transaction_hash, event_index, network);
but migration 0017 — which landed on dev after this branch was opened — made
soroban_events RANGE-partitioned by ledger_sequence, and PostgreSQL requires
every unique constraint on a partitioned table to include the partition key.
The statement is rejected outright, so no fresh database can apply the chain.
Added ledger_sequence to the constraint. This is the same trade-off 0017
already made and documented for the primary key, which became
(ledger_sequence, id) for exactly this reason.
The cost is recorded in the migration rather than glossed over: the
constraint no longer catches the same (transaction_hash, event_index,
network) triple appearing under two different ledger_sequence values. That
case would mean the indexer attributed one protocol event to two ledgers — a
different bug from the duplicate-insert this migration targets, and one the
pre-flight duplicate check above still catches on existing data. Within a
ledger, which is where replays and overlapping code paths actually produce
duplicates, the guarantee is unchanged.
Depo-dev
added a commit
to Emrys02/Trident
that referenced
this pull request
Jul 31, 2026
…el-Labs#331/Telocel-Labs#353/Telocel-Labs#359/Telocel-Labs#378/Telocel-Labs#380) Six PRs landed on dev since the first merge, so this branch went dirty again. One conflict: services/api/handlers/health_test.go, add/add — both sides created it. The two files are not alternatives. dev's is `package handlers_test` with a single table-driven TestHealthHandler_TableDriven; this branch's is `package handlers` with six tests covering the /v1/health and /v1/ready split it introduces (Telocel-Labs#243). Kept this branch's file and dropped dev's, having checked what that costs. dev's four cases — all dependencies reachable, db down, grpc down, redis down — map one-to-one onto TestReady_AllHealthy_Returns200, TestReady_PostgresDown_Returns503, TestReady_GRPCDown_Returns503, and TestReady_RedisDown_Returns503. This branch adds a fifth (TestReady_NilDependencies_Returns503) and TestHealth_AlwaysReturns200. Keeping dev's file was not an option regardless: it asserts against handlers.HealthResponse, a type this branch deliberately split into LivenessResponse and ReadyResponse, and it expects /v1/health to return 503 when a dependency is down — the exact behaviour Telocel-Labs#243 changes, since a liveness probe must not fail because Postgres is unreachable. Verified: go vet and go test across all twelve services/api packages, cargo fmt --check, cargo clippy --workspace --all-targets -D warnings, and the REST handler coverage gate added by Telocel-Labs#380 still passes (ListEvents 100.0%, GetEvent 90.5%, Health 100.0%).
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
2 participants
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.
Closes #248 DB: enforce a UNIQUE natural key on soroban_events (tx_hash, event_index)
…event_index, network)
Add migration 0010 that adds UNIQUE (transaction_hash, event_index, network) constraint to soroban_events. Includes a pre-flight duplicate check that raises an exception if any conflicting rows exist before applying the DDL.
Decision record (in migration + db/mod.rs docs): The UUIDv5 id is derived from (contract_id, ledger_sequence, event_index) and is NOT a pure function of the Stellar protocol natural key (transaction_hash, event_index). The new constraint is therefore a complementary, independent correctness guard that enforces the protocol- level uniqueness guarantee regardless of the id derivation scheme. The constraint is network-scoped to allow the same tx hash to appear on different networks (testnet / mainnet) in multi-network deployments and to align with the partition-key candidate identified in DB: partition soroban_events by ledger range (or time) for scale + retention #244.
Update schema.sql to mirror migration 0010 end state (bump comment from 0001-0009 to 0001-0010, add constraint).
Propagate network parameter through both insert paths: crates/indexer/src/db/mod.rs — insert_event now takes &str network; populates the network column explicitly instead of relying on the DEFAULT 'testnet' column default.
crates/backfill/src/db.rs — same signature change.
crates/indexer/src/streamer/mod.rs — passes self.config.network.
crates/backfill/src/main.rs — clones args.network into each worker.
Add two new integration tests in crates/indexer/src/db/mod.rs: event_uuid_does_not_include_transaction_hash — documents that the UUID key is distinct from the natural key. natural_key_constraint_rejects_duplicate_tx_event_index — inserts two events with different contract_ids but the same (tx_hash, event_index, network) and asserts the second insert is rejected by the constraint.
Closes #. Targets dev.