Failure to decode a block (fully or partially) will never be corrected

[EvanHahn]

Description

The problem

If a device fails to fully decode a block, the missing data will be forever missing from the SQLite index, and therefore the app. I think this can happen if there's a total or partial failure to decode a block.

Total failure to decode

The simplest example: when IndexWriter fails to decode a block, it discards it:

https://github.com/digidem/mapeo-core-next/blob/75a312d01d0d74ce64640816407a2c5d0a371a2f/src/index-writer/index.js#L71-L78

Imagine that my device is on an older version than yours, and that your device has a data type I've never heard of. decode() will throw and the block will be "silently ignore"d.

We will never return to this block. This block will sync to my Hypercore but will be permanently missing from my SQLite index, even if I update.

Partial failure to decode

I believe a similar issue can happen when a data type gets a new field. A device will only add fields it understands:

// Snippet from https://github.com/digidem/mapeo-sqlite-indexer/blob/4e9484442204157a792d808da333768f49774c4d/index.js#L74-L77
this.#writeDocSql = db.prepare(
  `REPLACE INTO ${docTableName} (${docColumns.join(',')})
  VALUES (${docColumns.map((name) => `@${name}`).join(',')})`
)

If a new field is added, we'll never go back to insert it.

Possible solutions

I can think of a few solutions:

1. When we detect a version change (e.g., a new field), completely rebuild the index from scratch.
   • Pros: should be simple to drop everything and reset.
   • Cons: will this be slow? Will it open a long-running transaction that blocks other database operations? Is it easy to detect these changes reliably? Will it require a "migrating" screen to be added to CoMapeo? Should there be a way to manually rebuild the index in some advanced settings in case we make a mistake?
2. Store non-decoded blocks and fields for later processing. At some point (e.g., startup of a new app version), reprocess them.
   • Pros: does minimal work, which should be good for performance.
   • Cons: may require a patch of protobufjs to handle unknown fields depending on our approach. Probably more complex to implement.
3. Some other option I'm not thinking of.
   • Pros: ???
   • Cons: ???

I think this is worth discussing, but I think option 1 is probably best in the short term. It seems fairly easy to implement, and we can always implement option 2 if performance becomes a concern. But I may be wrong!

Tasks

Assuming we take my preferred approach:

• Add tests
  • Test a block that completely fails to decode (e.g., new data type)
  • Test a block that fails to decode a field (e.g., a new field)
• Add mechanism to detect version changes (may already be done?)
• Add function to completely rebuild the index from scratch
• Work with frontend to figure out best API for this. Should it be done automatically, or should it expose APIs so the frontend can display some UI? Should there be events fired?

[EvanHahn]

Seems related to #27.

[gmaclennan]

I think option #1 seems preferable. We don't have to worry about storing versions as per #27 for the MVP, since we will know what versions the MVP ships with, so can add it in the next iteration. It would be good to build some metrics that run on mobile devices so that we can see how long a complete re-index takes. It is the same time that it takes to sync and add a new device to a project. I think it is ok that a user sees an "upgrading your database" message after an app update, but we should check that with the co-design team after we know how long it might take (big difference between 15 seconds and 10 minutes)

[EvanHahn]

I sketched this out in a branch and want to know whether the following is reasonable:

Here's the pseudocode I think we'd like:

if (areSqlTablesOutOfDate) {
  resetMulticoreIndexer()
  deleteRowsFromSqlite()
}

areSqlTablesOutOfDate is the tricky part, IMO. I toyed with adding a version number to our indexed tables, but that's prone to human error and requires maintaining additional state. Instead, I think we should migrate the database and check if anything changed. For example:

function migrate(db): boolean {
  if (isDatabaseNew(db)) return false
  const hashBefore = hashMigrationsTable(db)
  migrate(db)
  const hashAfter = hashMigrationsTable(db)
  return hashBefore !== hashAfter
}

// ...

const areSqlTablesOutOfDate = migrate(db)
if (areSqlTablesOutOfDate) {
  // ...
}

resetMulticoreIndexer() is a call to fs.rm(INDEXER_STORAGE_FOLDER_NAME, { recursive: true }). We could embed this logic into the multi-core-indexer package if we want, but I don't think it's needed.

deleteRowsFromSqlite() looks like for (const table of indexedTables) db.delete(table).run(). Again, we could stuff this into @mapeo/sqlite-indexer but I don't think this is necessary.

Does that seem like a reasonable approach? Again, you can check out an incomplete branch that implements a lot of this.

[gmaclennan]

Thanks. Thoughts:

• I don't think any of this is needed for MVP. It will be needed the first release post-mvp that makes a schema change.
• To detect migration changes we can count rows in the drizzle migration table before and after running Drizzle's migrate command. It's called __drizzle_migrations by default, or we can configure it (see here).
• To delete indexed tables we can use indexer.deleteAll().
• For multi-core indexer, we already have the indexer.unlink() command. I think it's much safer to use this rather than mess with the files ourselves. We need to figure out how to reset the multi-core-indexer state though - probably close it and re-open it or create a new instance.

[gmaclennan]

Just saw the branch - sorry, didn't look at it before writing the comment.

• Not sure we need our own version of drizzle's migrate function, I think it's safe to rely on the assumption that each migration creates a new row, so if (initialRowCount > 0 && postMigrationRowCount > initialRowCount) { reIndexData() }
• I see that this is happening before we create instances of sqlite-indexer or multi-core-indexer, so I guess it can make sense to do it as you are. I'm a little nervous we might forget about something, but I think maybe ok? But I do think we need to delete the backlink tables too.

[gmaclennan]

Re. the isNameSafe function, since we're doing all our interaction with sqlite through Drizzle, I'm pretty sure it does all those checks?