fix: canonical schema.sql + integration test integrity

.github/workflows/ci.yml

@@ -199,18 +199,29 @@ jobs:
       - name: Cache Rust build artefacts
         uses: Swatinem/rust-cache@v2
 
-      - name: Apply database schema
+      # Apply the real migration chain (0001..latest) — the same artifact CI's
+      # `rust` job and production apply. This validates the migrations as an
+      # ordered, idempotent chain and can never silently depend on a stale
+      # database/schema.sql mirror (which had drifted from migrations).
+      - name: Apply database migrations
         env:
           PGPASSWORD: trident
         run: |
-          psql -h localhost -U postgres -d trident_test \
-            -f database/schema.sql
+          set -euo pipefail
+          for f in database/migrations/*.sql; do
+            echo "Applying $f"
+            psql -v ON_ERROR_STOP=1 -h localhost -U postgres -d trident_test -f "$f"
+          done
 
       - name: Run integration tests
         env:
           DATABASE_URL: postgres://postgres:trident@localhost:5432/trident_test
           TEST_DATABASE_URL: postgres://postgres:trident@localhost:5432/trident_test
           TEST_REDIS_URL: redis://localhost:6379
+          # Turns a missing TEST_DATABASE_URL/TEST_REDIS_URL into a hard test
+          # failure instead of a silent skip, so a misconfigured integration
+          # job cannot report green without running the DB/Redis tests.
+          REQUIRE_TEST_SERVICES: "1"
           CI: "true"
         run: cargo test --all -- --include-ignored --test-threads=1
 

REPORT.md

@@ -0,0 +1,129 @@
+# Trident Hardening Sweep — Findings Report
+
+Branch: `hardening-sweep` (cut from `pr-189-fix` @ `b93316a`, which is 78 commits ahead of
+`origin/main` and is the "green under fire" work; `main` itself is behind and has red history).
+
+## Baseline (Phase 0)
+
+- HEAD `b93316a` = **all 8 CI jobs green** (Rust, Rust integration tests, Go, TypeScript,
+  OpenAPI, Docker build, E2E smoke, Helm). Verified via check-runs API.
+- Repo map: `crates/{api,indexer,backfill,common}` (Rust), `services/api` (Go REST + gRPC
+  client), `sdk/{typescript,rust,go,python,react}`, `database/{schema.sql,migrations/0001-0009}`,
+  `docker/*compose*`, `helm/trident`, `.github/workflows/{ci,release}.yml`.
+
+## How "green" hides gaps (Phase 1 — proven from CI logs)
+
+- The env-gated Rust tests use `require_services!` which does `eprintln!("SKIP…"); return;` on
+  missing `TEST_DATABASE_URL`/`TEST_REDIS_URL`. cargo **hides passing-test stderr**, so
+  "no SKIP lines in the log" proves nothing. The real evidence is timing:
+  the same 61-test suite ran in **0.20s in the `rust` job** (env absent → early-returned/skipped)
+  vs **9.42s in `rust-integration`** (env present → real DB/Redis work). So the DB/Redis
+  integration tests genuinely execute **only** in `rust-integration`, and that job is
+  `if`-gated to push-to-main/dev or PR-with-base-main/dev.
+
+---
+
+## Findings (ranked by severity)
+
+### HIGH
+
+**H1 — `database/schema.sql` is not a canonical mirror of the migration chain; a DB
+bootstrapped from it is materially broken.**
+`database/schema.sql:1` claims "Canonical definition. Migrations mirror this file." It has drifted hard:
+- Missing table **`audit_log`** (migration `0006`). → Go audit middleware
+  (`services/api/middleware/audit.go:165`), admin analytics (`services/api/handlers/admin.go:151`),
+  and the 90-day cleanup (`services/api/main.go:290`) all hit `relation "audit_log" does not exist`.
+- Missing table **`parse_errors`** (migration `0008`). → indexer parse-error isolation write
+  (`crates/indexer/src/db/mod.rs:246`) and the Go `/status` count
+  (`services/api/handlers/status.go:110`) fail at runtime.
+- Missing `system_state` columns **`last_poll_at`, `last_alert_at`** (+`last_ledger_indexed`,
+  `events_indexed_total`, `events_in_last_poll`, `poll_duration_ms`, `alert_fired`) from
+  migrations `0002`/`0003`. → indexer health write `UPDATE system_state SET last_poll_at…`
+  (`crates/indexer/src/db/mod.rs:159`) and alert-state read/write
+  (`crates/indexer/src/db/mod.rs:201`) fail.
+- **soroban_events indexes fully diverged.** schema.sql defines 9 legacy indexes
+  (`idx_soroban_events_contract_id`, `…_topic_0`, `…_topic_1`, `…_topics_gin`,
+  `…_contract_network`, `…_ledger_sequence`, `…_contract_topic_0`) that **no migration creates**,
+  and is missing the migration `0009` high-cardinality perf indexes actually deployed to
+  production (`idx_soroban_events_contract_ledger`, `…_id_desc`, `…_contract_topic0` partial,
+  `…_ledger_timestamp`) plus `0004`'s `…_network_contract`.
+- **Extra constraint** `uq_soroban_events_tx_index UNIQUE(transaction_hash,event_index)`
+  (`database/schema.sql:42`) exists in no migration. Insert path uses `ON CONFLICT (id)`
+  (`crates/indexer/src/db/mod.rs:66`), so it is redundant divergence.
+
+*Failure scenario:* the **`rust-integration` CI job builds `trident_test` from schema.sql**
+(`.github/workflows/ci.yml:206`), so every code path touching `audit_log`, `parse_errors`, or the
+`system_state` health/alert columns is **not integration-covered** — tests are green only because
+they never exercise those paths against the DB. Anyone who runs `psql -f schema.sql` for local/dev
+gets a schema on which audit logging, parse-error isolation, health/alert tracking, and the
+`/stats` + `/status` endpoints crash on first use.
+
+### MEDIUM
+
+**M1 — Integration tests can silently skip even inside the integration job.**
+`require_services!` (`crates/indexer/src/streamer/mod.rs:400`,
+`crates/api/src/services/events.rs:367`, `crates/indexer/src/db/mod.rs:304`) `return`s instead of
+failing when the env is absent. GitHub Actions sets `CI=true` in *every* job, so `CI` can't
+distinguish the two jobs. *Failure scenario:* if `TEST_DATABASE_URL` is ever misconfigured in the
+`rust-integration` job (typo, service rename, port change), all DB/Redis tests skip and the job
+still reports **green** — a false pass with zero signal.
+
+**M2 — Indexer resume passes a ledger sequence where the RPC expects a paging-token cursor.**
+`crates/indexer/src/streamer/mod.rs:186-190`: on restart (`*cursor != 0`) it calls
+`get_events(start_ledger=None, cursor=Some(cursor.to_string()))`, but the persisted cursor is a
+**ledger sequence** (`db::set_cursor` stores `last.ledger` at `streamer/mod.rs:317`), while
+in-loop pagination correctly uses the RPC `paging_token` (`streamer/mod.rs:223,349`). The Soroban
+`getEvents` `cursor` field (`crates/indexer/src/rpc/mod.rs:93`) is a paging token, not a ledger
+number. *Failure scenario:* after any indexer restart the first poll sends
+`cursor:"12345"`; the RPC rejects it as an invalid cursor and the poll loop errors/retries,
+stalling ingestion. **Flagged for review — changes runtime behavior of the resume feature; not
+auto-fixed.** Likely correct fix: resume via `start_ledger = Some(cursor + 1)` instead of the
+ledger-as-cursor.
+
+**M3 — Auth Redis cache is never invalidated on key revocation.**
+`services/api/middleware/auth.go:99-123` caches a successful lookup for 5 min
+(`authCacheTTL`) keyed by `sha256(key)` and does not delete/expire it when a key is revoked.
+*Failure scenario:* a revoked/compromised API key keeps authenticating for up to 5 minutes after
+revocation. **Flagged for review** (acceptable if documented; fix = delete cache key on revoke).
+
+**M4 — Floating Docker base image breaks build reproducibility.**
+`crates/api/Dockerfile:4` and `crates/indexer/Dockerfile:4` use `FROM rust:1-slim` (floating
+major). *Failure scenario:* a new upstream `rust:1` publish silently changes the toolchain,
+producing non-reproducible builds and potential clippy/edition breakage unrelated to any commit.
+Pin to an explicit patch (e.g. `rust:1.83-slim`). Go/alpine bases are already pinned.
+
+### LOW
+
+**L1 — `rust-integration` job is entirely skipped on feature-branch pushes.**
+`.github/workflows/ci.yml:168-170` `if:` gate means integration tests only run on push to
+main/dev or PR based on main/dev. Green on a feature branch does not mean the DB tests ran.
+Acceptable by design; documented here so it isn't mistaken for coverage.
+
+**L2 — Obsolete compose `version:` keys.** `docker/docker-compose.yml:1` (`version: "3.9"`) and
+`docker/docker-compose.dev.yml:1` — Compose v2 ignores and warns on these.
+
+**L3 — `pgbouncer` service still defined in `docker/docker-compose.yml` /
+`docker-compose.ci.yml`** though a prior fix removed it from E2E `depends_on`. Dead scaffolding in
+the CI stack.
+
+**L4 — Generated gRPC "not implemented" stubs** in `services/api/gen/trident_grpc.pb.go:95-103`
+are normal protoc `Unimplemented*Server` output (the Go service is a gRPC *client*), not a real
+gap. Noted to pre-empt false alarms. No other real-code TODO/FIXME/unimplemented in the tree.
+
+---
+
+## Fix plan (Phase 7)
+
+1. **H1** — rewrite `database/schema.sql` to faithfully mirror migrations `0001–0009`
+   (add `audit_log`, `parse_errors`, the `system_state` health/alert columns; replace the
+   soroban_events index set with the `0004`+`0009` canonical indexes; drop the non-migration
+   `uq_soroban_events_tx_index`).
+2. **H1/M1 (durable)** — switch the `rust-integration` job to build `trident_test` from the
+   **migration chain** instead of `schema.sql`, so CI validates the real deploy artifact and can
+   never again silently depend on a stale mirror; and make `require_services!` **panic** (fail
+   loud) when a dedicated `REQUIRE_TEST_SERVICES` flag (set only in that job) is present but the
+   URLs are missing.
+3. **M4 / L2 / L3** — pin the Rust Docker base, drop obsolete `version:` keys and the dead
+   pgbouncer service (verified via the Docker build + E2E CI jobs).
+4. **M2 / M3** — reported only; flagged for maintainer review because they change the runtime
+   behavior of existing features.

crates/api/src/services/events.rs

@@ -364,23 +364,25 @@ mod tests {
 
     use super::*;
 
+    // When REQUIRE_TEST_SERVICES is set (the rust-integration CI job sets it),
+    // a missing URL is a hard failure instead of a silent skip — otherwise a
+    // misconfigured integration job would go green without running anything.
     macro_rules! require_services {
         () => {{
-            let db = match std::env::var("TEST_DATABASE_URL") {
-                Ok(url) => url,
-                Err(_) => {
-                    eprintln!("SKIP: TEST_DATABASE_URL not set");
+            let required = std::env::var("REQUIRE_TEST_SERVICES").is_ok();
+            match (
+                std::env::var("TEST_DATABASE_URL"),
+                std::env::var("TEST_REDIS_URL"),
+            ) {
+                (Ok(db), Ok(rd)) => (db, rd),
+                _ if required => panic!(
+                    "TEST_DATABASE_URL and TEST_REDIS_URL must be set when REQUIRE_TEST_SERVICES is set"
+                ),
+                _ => {
+                    eprintln!("SKIP: TEST_DATABASE_URL / TEST_REDIS_URL not set");
                     return;
                 }
-            };
-            let rd = match std::env::var("TEST_REDIS_URL") {
-                Ok(url) => url,
-                Err(_) => {
-                    eprintln!("SKIP: TEST_REDIS_URL not set");
-                    return;
-                }
-            };
-            (db, rd)
+            }
         }};
     }
 

crates/indexer/src/db/mod.rs

@@ -303,6 +303,11 @@ mod tests {
     async fn insert_event_is_idempotent() {
         let db_url = match std::env::var("TEST_DATABASE_URL") {
             Ok(url) => url,
+            // Hard-fail under the rust-integration CI job (REQUIRE_TEST_SERVICES)
+            // so a misconfigured DB URL cannot silently skip and go green.
+            Err(_) if std::env::var("REQUIRE_TEST_SERVICES").is_ok() => {
+                panic!("TEST_DATABASE_URL must be set when REQUIRE_TEST_SERVICES is set");
+            }
             Err(_) => {
                 eprintln!("SKIP: TEST_DATABASE_URL not set");
                 return;

crates/indexer/src/streamer/mod.rs

@@ -396,24 +396,26 @@ mod tests {
     use wiremock::matchers::{body_partial_json, method, path};
     use wiremock::{Mock, MockServer, ResponseTemplate};
 
-    // Skip the test and return early if required env vars are missing.
+    // Return the (db, redis) URLs, or skip the test when they are absent.
+    // When REQUIRE_TEST_SERVICES is set (the rust-integration CI job sets it),
+    // a missing URL is a hard failure instead of a silent skip — otherwise a
+    // misconfigured integration job would go green without running anything.
     macro_rules! require_services {
         () => {{
-            let db = match std::env::var("TEST_DATABASE_URL") {
-                Ok(v) => v,
-                Err(_) => {
-                    eprintln!("SKIP: TEST_DATABASE_URL not set");
+            let required = std::env::var("REQUIRE_TEST_SERVICES").is_ok();
+            match (
+                std::env::var("TEST_DATABASE_URL"),
+                std::env::var("TEST_REDIS_URL"),
+            ) {
+                (Ok(db), Ok(rd)) => (db, rd),
+                _ if required => panic!(
+                    "TEST_DATABASE_URL and TEST_REDIS_URL must be set when REQUIRE_TEST_SERVICES is set"
+                ),
+                _ => {
+                    eprintln!("SKIP: TEST_DATABASE_URL / TEST_REDIS_URL not set");
                     return;
                 }
-            };
-            let rd = match std::env::var("TEST_REDIS_URL") {
-                Ok(v) => v,
-                Err(_) => {
-                    eprintln!("SKIP: TEST_REDIS_URL not set");
-                    return;
-                }
-            };
-            (db, rd)
+            }
         }};
     }
 

database/schema.sql

@@ -1,5 +1,8 @@
 -- Trident PostgreSQL Schema
--- Canonical definition. Migrations in ./migrations/ mirror this file incrementally.
+-- Convenience full-schema snapshot for local/dev bootstrap and documentation.
+-- The migration chain in ./migrations/ (0001-0009) is the source of truth and is
+-- what CI and production apply; this file must mirror the end state of that chain.
+-- Keep in sync whenever a migration is added.
 
 CREATE EXTENSION IF NOT EXISTS "pgcrypto";
 
@@ -23,35 +26,35 @@ CREATE TABLE IF NOT EXISTS soroban_events (
     created_at          TIMESTAMPTZ NOT NULL DEFAULT NOW()
 );
 
--- Single-column indexes
-CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_id       ON soroban_events (contract_id);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_sequence   ON soroban_events (ledger_sequence);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_timestamp  ON soroban_events (ledger_timestamp);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_network           ON soroban_events (network);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_topic_0           ON soroban_events (topic_0);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_topic_1           ON soroban_events (topic_1);
-
--- Composite indexes
-CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_topic_0  ON soroban_events (contract_id, topic_0);
-CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_network  ON soroban_events (contract_id, network);
-
--- GIN index for arbitrary topic containment queries
-CREATE INDEX IF NOT EXISTS idx_soroban_events_topics_gin        ON soroban_events USING GIN (topics);
-
--- Unique constraint: a given event position within a transaction is immutable
-ALTER TABLE soroban_events
-    ADD CONSTRAINT uq_soroban_events_tx_index
-    UNIQUE (transaction_hash, event_index);
+-- Indexes — canonical set produced by migrations 0004 and 0009.
+-- network isolation (0004)
+CREATE INDEX IF NOT EXISTS idx_soroban_events_network          ON soroban_events (network);
+CREATE INDEX IF NOT EXISTS idx_soroban_events_network_contract ON soroban_events (network, contract_id);
+-- high-cardinality query patterns (0009)
+CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_ledger  ON soroban_events (contract_id, ledger_sequence DESC);
+CREATE INDEX IF NOT EXISTS idx_soroban_events_contract_topic0  ON soroban_events (contract_id, topic_0)
+    WHERE topic_0 IS NOT NULL;
+CREATE INDEX IF NOT EXISTS idx_soroban_events_id_desc          ON soroban_events (id DESC);
+CREATE INDEX IF NOT EXISTS idx_soroban_events_ledger_timestamp ON soroban_events (ledger_timestamp DESC);
 
 -- ---------------------------------------------------------------------------
 -- system_state
 -- Persistent cursor tracking so the indexer can resume after restart without
 -- re-scanning from genesis.
 -- ---------------------------------------------------------------------------
 CREATE TABLE IF NOT EXISTS system_state (
-    key         TEXT PRIMARY KEY,
-    value       TEXT NOT NULL,
-    updated_at  TIMESTAMPTZ NOT NULL DEFAULT NOW()
+    key                   TEXT PRIMARY KEY,
+    value                 TEXT        NOT NULL,
+    updated_at            TIMESTAMPTZ NOT NULL DEFAULT NOW(),
+    -- indexer health columns (migration 0002)
+    last_poll_at          TIMESTAMPTZ,
+    last_ledger_indexed   BIGINT,
+    events_indexed_total  BIGINT      NOT NULL DEFAULT 0,
+    events_in_last_poll   INT         NOT NULL DEFAULT 0,
+    poll_duration_ms      INT         NOT NULL DEFAULT 0,
+    -- alerting state columns (migration 0003)
+    last_alert_at         TIMESTAMPTZ,
+    alert_fired           BOOLEAN     NOT NULL DEFAULT FALSE
 );
 
 -- Seed the cursor row so the indexer can always do an UPDATE rather than
@@ -114,6 +117,43 @@ CREATE INDEX IF NOT EXISTS idx_api_keys_key_hash ON api_keys (key_hash);
 CREATE INDEX IF NOT EXISTS idx_api_keys_active ON api_keys (key_hash)
     WHERE revoked_at IS NULL;
 
+-- ---------------------------------------------------------------------------
+-- audit_log
+-- Per-request audit trail for API key usage (migration 0006).
+-- ---------------------------------------------------------------------------
+CREATE TABLE IF NOT EXISTS audit_log (
+    id           BIGSERIAL   PRIMARY KEY,
+    api_key_id   UUID        REFERENCES api_keys(id) ON DELETE SET NULL,
+    endpoint     TEXT        NOT NULL,
+    method       TEXT        NOT NULL,
+    ip           INET,
+    user_agent   TEXT,
+    status_code  INT         NOT NULL,
+    duration_ms  INT         NOT NULL,
+    result_count INT,
+    request_id   TEXT        NOT NULL,
+    network      TEXT,
+    ts           TIMESTAMPTZ NOT NULL DEFAULT NOW()
+);
+
+CREATE INDEX IF NOT EXISTS idx_audit_log_key_ts ON audit_log (api_key_id, ts DESC);
+CREATE INDEX IF NOT EXISTS idx_audit_log_ts     ON audit_log (ts DESC);
+
+-- ---------------------------------------------------------------------------
+-- parse_errors
+-- Audit trail for events that failed XDR decoding (migration 0008).
+-- ---------------------------------------------------------------------------
+CREATE TABLE IF NOT EXISTS parse_errors (
+    id               UUID        PRIMARY KEY DEFAULT gen_random_uuid(),
+    ledger_sequence  BIGINT      NOT NULL,
+    event_index      INT         NOT NULL,
+    raw_payload      TEXT        NOT NULL,
+    error_message    TEXT        NOT NULL,
+    occurred_at      TIMESTAMPTZ NOT NULL DEFAULT NOW()
+);
+
+CREATE INDEX IF NOT EXISTS idx_parse_errors_occurred_at ON parse_errors (occurred_at DESC);
+
 -- ---------------------------------------------------------------------------
 -- webhook_subscriptions
 -- ---------------------------------------------------------------------------