Skip to content

fix(selfhost): REPLACE_CONFLICT_KEYS omits ams_signals, crashing AMS telemetry ingest on the self-host Postgres backend #8382

Description

@JSONbored

Context

src/selfhost/pg-dialect.ts translates SQLite-dialect SQL (what drizzle-orm/d1 and every raw env.DB.prepare(...) call in this codebase emit) to Postgres for the self-host Postgres backend. translateInsertOr() rewrites INSERT OR REPLACE INTO <table> (...) to Postgres's INSERT ... ON CONFLICT (<key>) DO UPDATE SET ..., and it needs to know each such table's real conflict key up front — Postgres has no equivalent of SQLite's "replace by any unique constraint" semantics, so the key must be named explicitly:

const REPLACE_CONFLICT_KEYS: Record<string, string[]> = {
  system_flags: ["key"],
  tunables_overrides: ["project"],
  tunables_overrides_shadow: ["project"],
  orb_export_cursor: ["instance_hash"],
  orb_signals: ["instance_id", "repo_hash", "pr_hash"],
};

When a table using INSERT OR REPLACE isn't in this map, translateInsertOr throws: `pg_dialect: INSERT OR REPLACE into '${table}' has no known conflict key`.

src/ams/ingest.ts issues exactly this pattern against a table that is not in the map:

// OR REPLACE: a re-exported PR (e.g. a decision that changed) upserts the freshest outcome on the
// (instance_id, pr_hash) dedup key.
const result = await db
  .prepare(
    `INSERT OR REPLACE INTO ams_signals
     (instance_id, repo_hash, pr_hash, decision, reason_bucket, closed_at, received_at)
     VALUES (?, ?, ?, ?, ?, ?, CURRENT_TIMESTAMP)`,
  )
  ...

(migrations/0148_ams_signals.sql defines ams_signals with UNIQUE (instance_id, pr_hash).) On any self-host deployment running the Postgres backend (DATABASE_URL set, per pg-adapter.ts/preflight.ts), the first AMS telemetry-ingest write throws this error and the ingest request fails outright — this is a live, reachable code path (src/ams/ingest.ts), not dead code, and it is untested against the Postgres translation layer today.

Separately: orb_signals — the structurally analogous table for the Orb (maintainer) product, explicitly described in ams_signals's own migration comment as the pattern ams_signals mirrors ("mirroring orb_signals' pattern for the miner product") — was deliberately widened from a 2-column UNIQUE (instance_id, pr_hash) to the current 3-column UNIQUE (instance_id, repo_hash, pr_hash) in migrations/0060_orb_fleet_collector.sql, whose own comment states plainly: "SQLite can't ALTER away the old table-level UNIQUE(instance_id, pr_hash) — which is wrong (two instances reviewing owner/repo#123 collide) — so recreate with the correct key." ams_signals (added later, in migrations/0148_ams_signals.sql) was given the exact 2-column key that orb_signals was explicitly fixed away from for a documented correctness reason. Whether ams_signals is exposed to the same collision risk orb_signals had needs the same scrutiny orb_signals got — this issue asks for that evaluation as part of the fix, not a silent assumption either way.

Requirements

  • Add an ams_signals entry to REPLACE_CONFLICT_KEYS in src/selfhost/pg-dialect.ts so the self-host Postgres backend can translate INSERT OR REPLACE INTO ams_signals without throwing.
  • Before picking the entry's key columns, evaluate whether ams_signals's current UNIQUE (instance_id, pr_hash) constraint has the same collision exposure migrations/0060_orb_fleet_collector.sql documents for orb_signals's old key of the identical shape:
    • If the collision reasoning applies equally to ams_signals (i.e. pr_hash alone cannot disambiguate repos the way orb_signals's fix required repo_hash for), add a new migration widening ams_signals's unique constraint to UNIQUE (instance_id, repo_hash, pr_hash) (mirroring migrations/0060_orb_fleet_collector.sql's orb_signals recreate — SQLite can't ALTER away a table-level UNIQUE, so this needs the same drop-and-recreate shape, or an equivalent that preserves existing rows if any exist in production), update src/ams/ingest.ts's INSERT OR REPLACE INTO ams_signals column list accordingly if it changes, and set REPLACE_CONFLICT_KEYS.ams_signals = ["instance_id", "repo_hash", "pr_hash"].
    • If the reasoning genuinely doesn't apply (document why in the PR — e.g. if pr_hash's hash input already disambiguates by repo in a way that makes the 2-column key safe for this table specifically), set REPLACE_CONFLICT_KEYS.ams_signals = ["instance_id", "pr_hash"] matching the existing constraint, and say so explicitly in the PR body so this isn't silently re-litigated later.
  • If a migration is added, follow the house migration conventions: a contiguous migrations/NNNN_*.sql file, and regenerate/verify npm run db:schema-drift:check and npm run db:migrations:check pass (this table has no Drizzle schema.ts entry, so schema-drift is not applicable here, but the migration-number/collision checks are).

Deliverables

  • ams_signals added to REPLACE_CONFLICT_KEYS in src/selfhost/pg-dialect.ts with the correct conflict key (per the evaluation above).
  • If the key widens: a new migrations/NNNN_*.sql recreating ams_signals with UNIQUE (instance_id, repo_hash, pr_hash), and src/ams/ingest.ts updated to match.
  • A short note (PR body or code comment) recording the collision-risk evaluation outcome either way.

Test Coverage Requirements

src/selfhost/pg-dialect.ts and src/ams/ingest.ts are under src/**, Codecov patch-gated at 99%, branch-counted:

  • test/unit/selfhost-pg-dialect.test.ts's "translates INSERT OR IGNORE / REPLACE to ON CONFLICT" test already asserts translateInsertOr throws /no known conflict key/ for an unmapped table and correctly rewrites system_flags. Add a matching assertion for ams_signals: translateInsertOr("INSERT OR REPLACE INTO ams_signals (instance_id, repo_hash, pr_hash, decision, reason_bucket, closed_at, received_at) VALUES (...)") now produces a valid ON CONFLICT (...) DO UPDATE SET ... clause instead of throwing.
  • If the migration widens the unique key: a regression test on the self-host Postgres integration path (or the existing D1/SQLite-vs-Postgres parity test harness, if one already exercises ams_signals) confirming an INSERT OR REPLACE for a second PR under a different repo but colliding pr_hash-only value no longer overwrites the first row's data.
  • A regression test named for this bug (e.g. "pg-dialect: INSERT OR REPLACE into ams_signals no longer throws (regression for <issue-number>)").

Expected Outcome

AMS telemetry ingest (src/ams/ingest.ts) no longer throws on the self-host Postgres backend, and ams_signals's dedup key has been explicitly evaluated against — and if necessary fixed to match — the exact collision class orb_signals was already fixed for.

Links & Resources

  • src/selfhost/pg-dialect.tsREPLACE_CONFLICT_KEYS to extend, translateInsertOr for the throw path.
  • src/ams/ingest.ts — the live, reachable INSERT OR REPLACE INTO ams_signals call site.
  • migrations/0148_ams_signals.sql — current ams_signals definition.
  • migrations/0060_orb_fleet_collector.sql — the precedent: orb_signals's own documented fix from the identical 2-column key to a 3-column key, with the exact collision reasoning to re-evaluate against ams_signals.

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions