Skip to content

fix(core): scope the state-sync truncate to the tables the snapshot restores - #551

Open
rickyrombo wants to merge 1 commit into
mainfrom
mjp-statesync-truncate-scope
Open

fix(core): scope the state-sync truncate to the tables the snapshot restores#551
rickyrombo wants to merge 1 commit into
mainfrom
mjp-statesync-truncate-scope

Conversation

@rickyrombo

Copy link
Copy Markdown
Contributor

The bug

Before loading a snapshot, the restore truncates every table in the public schema:

"SELECT tablename FROM pg_tables WHERE schemaname='public' ORDER BY tablename"
// then: TRUNCATE TABLE <each> CASCADE

The reason it truncates at all is narrow, and the comment says so: the node runs migrations at startup, which populates core_db_migrations, and pg_restore's COPY then fails on duplicate keys.

But only stateSyncSnapshotTables is ever dumped. Truncating a table the dump doesn't contain cannot prevent a COPY conflict, because nothing will COPY into it. Every table outside that list was being destroyed for no reason at all.

What that costs

Core and mediorum share one OPENAUDIO_DB_URL and therefore one public schema, so the collateral is mediorum's data. Any node that state syncs loses:

Table Recovery
blobs relists from the bucket
uploads eventually refills via scrollUploadsFromPeers
audio_previews recomputed from audiofindMissedJobsgenerateAudioPreviewForUpload
qm_audio_analyses recomputed on demand
upload_cursors, repair_trackers, delist tables reset

Nothing is permanently lost, but the last two are real CPU, on every node that joins. This matters most during a fleet-wide event where many nodes state sync at once — they all start regenerating simultaneously.

The fix

Truncate the intersection of stateSyncSnapshotTables with the tables that actually exist, in a single statement, without CASCADE.

The intersection is needed because pgRestore("pre-data") deliberately tolerates schema drift ("pre-data errors (missing tables from schema drift) are non-fatal"), so a listed table may not exist locally. Naming a missing table would fail the whole statement.

Dropping CASCADE is deliberate. CASCADE follows foreign keys outward: a key from a non-snapshot table into a snapshot table would silently truncate the non-snapshot table too — this same bug through a different door. Truncating the set together in one statement satisfies foreign keys among its members, and if something outside the set references something inside it, this now fails loudly. That's the outcome we want.

Tests

Three, all database-free so they run in CI, following the existing state_sync_snapshot_tables_test.go pattern:

  • non-snapshot tables (mediorum's, etl_*) are never named, while core_db_migrations — the table that motivated truncating in the first place — still is
  • only tables that exist are named; empty input yields no statement
  • no CASCADE, and exactly one TRUNCATE

Verified they fail against the previous behaviour, not just pass against the new one:

--- FAIL: TestTruncateStmtSkipsTablesTheSnapshotDoesNotCarry
    truncate statement names "uploads", which no snapshot restores -- truncating it only destroys data
    truncate statement names "blobs", ...
    truncate statement names "audio_previews", ...

Scope

Behaviour change is confined to which tables get truncated during a state-sync restore. The dump, the COPY, and the restore sequence are untouched.

One thing deliberately left out: etl_* tables are chain-scoped and preserving them across a chain switch leaves stale rows. Today they are cleared as a side effect of this workaround — right for the wrong reason. If that needs to hold it should be an explicit "chain changed, clear ETL state" check rather than a byproduct of a COPY fix, and it is moot while no node runs the ETL.

…estores

Before loading a snapshot, the restore truncated every table in the public
schema. The reason it truncates at all is narrow: the node runs migrations at
startup, which populates core_db_migrations, and COPY then fails on duplicate
keys. Truncating a table the dump does not contain cannot prevent that -- nothing
COPYs into it -- so every table outside stateSyncSnapshotTables was destroyed for
no reason.

Postgres is shared between core and mediorum (one OPENAUDIO_DB_URL, one public
schema), so the collateral was mediorum's uploads, blobs, audio_previews and
qm_audio_analyses on every node that state synced. blobs is rebuilt by relisting
the bucket, and uploads eventually refills via scrollUploadsFromPeers, but the
previews and analyses are recomputed from audio -- real CPU, network-wide,
whenever a node joins.

Truncate the intersection of stateSyncSnapshotTables with the tables that exist,
in one statement without CASCADE. The intersection matters because the pre-data
restore deliberately tolerates schema drift, so a listed table may be absent.
Dropping CASCADE matters because it follows foreign keys outward: a key from a
non-snapshot table into a snapshot table would silently truncate the
non-snapshot table too, which is this same bug through a different door. Failing
loudly is the outcome we want there.

Tests cover the scoping, the intersection, and the absence of CASCADE, and were
confirmed to fail against the previous behaviour.
rickyrombo added a commit that referenced this pull request Aug 26, 2026
…d with the code

Six places where the reference either contradicted the runbook or stated
something the code does not do:

- The facts table gave prod's validator count as 9. That is the genesis list;
  the live set is 67 of 72 registered nodes, and the runbook's quorum and
  jailing math is over the live set.
- Section 7 said to leave BlockInterval at its default, which the runbook now
  overrides with 20,000. Records why the default is wrong here, that it is a
  producer-side setting, and that retention is Keep x BlockInterval so it cannot
  be lowered freely.
- The state-sync truncation table listed blobs but not uploads, audio_previews
  or qm_audio_analyses, which are also wiped. Adds them with how each recovers,
  and notes #551 makes the row moot.
- Section 3 asserted the bootstrap validator holds power 100. Its own
  registration rewrites that to ValidatorVotingPower, 10 on mainnet, because the
  writer never seeds core_registered_nodes.
- The cursor discussion now points at api#1018, which is open and must land
  before flushing is first enabled.
- Section 11 omitted the chain-aware fallback to core_indexed_blocks, which is
  only reached on ErrNoRows and so never runs on a database that has indexed the
  old chain. That is why the failure is a silent stall.
rickyrombo added a commit that referenced this pull request Aug 28, 2026
Every step that cannot be executed without a code change now names it, so the
dependency is visible where the work happens rather than only in a list at the
top: #553 for the binary (step 3), api#1029 for play routing (5 and 13), #551
before the fleet state syncs (10), api#1018 before flushing (11), api#1028 for
the indexer bounds (12).

Also records two things found while deriving the bootstrap's identity for #553:
the node key is the comet key, so a node's P2P id is its validator address and
the bootstrap's is derivable before it runs; and the bootstrap must not inherit
node_key.json from the writer output, or its id will not match the peer list.
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