[Bug]: switching legacy AutoMigrate DB to new migration pattern does not work

[parkan]

Description

the "meta-migration" to gomigrate does not work:

2025-10-16T12:34:06.834Z	INFO	database	database/connstring_cgo.go:56	Opening postgres database
2025-10-16T12:34:06.877Z	INFO	model	model/migrate.go:69	Initializing database
2025-10-16T12:34:06.901Z	INFO	model	model/migrate.go:77	Manually creating versioned migration table in existing database
2025-10-16T12:34:06.920Z	ERROR	database	database/util.go:81	SELECT * FROM "migrations" LIMIT 1	{"rowsAffected": -1, "elapsed": 0.000419528, "err": "insufficient arguments"}

Steps to Reproduce

admin init or admin migrate against a legacy db

Version

6.x

Operating System

Linux

Database Backend

PostgreSQL

Additional context

No response

[parkan]

the comments here contradict each other and the implementation: https://github.com/data-preservation-programs/singularity/blob/develop/model/migrate.go#L58

the comment at the top of the function has the correct logic, but the actual implementation (no initial migration insert) is incorrect

EDIT: see below

[parkan]

ok the fix was not effective, iterating

[parkan]

ok this seems seems to be an intersection of MANY issues, critical ones highlighted with **:

• gormigrate init is explicitly aimed at fresh databases, not anything with existing schema and especially data
• multiple issues with how InitSchema is used:
  • ** the purpose is to atomically create the final database schema (squash of all migrations); actually calling Migrate() inside this has undefined behavior
  • the whole "recreate parts of gormigrate" approach is brittle, the private methods are private for a reason -- this also sent me on a wild goose chase regarding manual application/insertion of migrations (InitSchema writes the full migration set to table after returning nil)
  • the much simpler workaround would have been to check for legacy state and upsert the initial migration only; migrations run normally from here (this wouldn't have fully prevented breakage due to below points but it would have made debugging a lot easier)
• ** UseTransaction was explicitly set to off regardless of dialect so fatal errors during a migration leave behind partially applied changes
• ** wallet PK migration written in a convoluted way with temporary table which is not resumable or idempotent = any failure means broken DB state (this should never have been accepted without transaction wrapper, whether it should have been added at all is very questionable)
• ** the above migration tries to drop the old wallets table to swap in new_wallets; fk_deals_wallet is dropped but oops we forgot about fk_wallet_assignments_wallet -- this is accepted on empty DB because there's nothing in the join table but in production the DROP fails
• ** trying to replay migration now results in duplicate key errors in new_wallets
• (other migrations also not written idempotently even though they could be, however they're less brittle)
• ** gorm appears to have scanning into empty struct bugs during AutoMigrate under certain conditions in certain combinations with driver/postgres: Upgrade 1.25.5->1.25.7: AutoMigrate returns the error "insufficient arguments" go-gorm/gorm#6886 (comment)
• all of the above put together broke idempotency for singularity admin init -- previously this was always safe and cheap to call (no-op AutoMigrate) so our cluster runs it as a pre-start script with auto-retry, as a result the migration path was getting hammered every ~30 seconds (with backoff) until the legacy heuristic migration cleared, which was changing state under my hotfixes

and these are the errors that I made:

• trusted that approved/merged code was tested instead of verifying myself first with a realistic DB, went directly to production (was being lazy about pulling down a 100GB+ snapshot or manually creating a realistic local test db)
• did not stop the process the moment I saw that migration bootstrap wasn't applying cleanly
• did not immediately stop the application task group while trying to rescue DB, which resulted in repeated admin init calls intereleaved with my rescue queries

[parkan]

high level:

• gormigrate appears to have been introduced for (1) nonspecific best practices reason and/or (2) implementing wallet PK migration, which is not possible with AutoMigrate (speculative, this is not documented anywhere I can find, comments may be lost due to force-pushing of the repo)
• the PK change likewise has no clear/documented motivation, it's still impossible to insert the same actor twice under the new schema (unique constraint on address), reviewing the codebase has not revealed anything that would require this -- @ianconsolata if you can think of something let me know
• all other migrations can be handled adequately by AutoMigrate (with some dialect-specific SQL and possibly manual invocation of the deadlock fixes), benefits of versioning do not outweigh the complexity and brittleness

my recommendation would be to remove gormigrate if we are keeping any of the code in the develop branch at all

[ianconsolata]

Thanks for looking into this, it sounds like a mess.

PK means private key? I am not sure why it was added gormigrate was added, and if you think it should be removed I support your choice. As long as we save a copy of the branch somewhere, we can always pull the code from git history or re-implement things more cleanly if we do end up needing it.

[parkan]

Thanks for looking into this, it sounds like a mess.

PK means private key? I am not sure why it was added gormigrate was added, and if you think it should be removed I support your choice. As long as we save a copy of the branch somewhere, we can always pull the code from git history or re-implement things more cleanly if we do end up needing it.

sorry, have databases on the brain so PK = primary key, FK = foreign key constraint

wallets table prior to 2ba6325 use short agent IDs as primary keys, then this complex migration was done to add an autoincrement primary key instead, add back the agent ID as a separate field AgentID, and rebuild references to wallets from other tables

I think I figured out the motivation though (would have been great to have it documented) -- the wallet create command added at the same time generates a key offline, which has a private key and address, but no actor ID until a message is sent and an actor is provisioned so you don't have a value to fill in as the old primary key

this is a bit of an annoyance at the intersection of the filecoin actor design (no actor ID until message, most types reference the short id not address) and postgres referential integrity requirements, however the invariants are in our favor and I'll think of a cleaner way to solve this problem

I suspect the real fix is reworking the wallet/actor abtraction, especially in light of various new onboarding mechanisms that we may want to support

EDIT: yep ok I think separating wallet and actor is the way to go, this will also give us a much cleaner treatment of the tracking feature I implemented in #550

[parkan]

closing this since we're going to drop gormigrate