Skip to content

destroyJournalDatabase() can report success when the journal file was not deleted #135

Description

@Jberma23

Description

destroyJournalDatabase() in mobile/src/lib/db/database.ts is the mechanism behind the "Delete all my data" control that #116 tracks and ADR 0007 requires. It can report success while leaving the encrypted journal file on disk.

// database.ts:101-105
export async function destroyJournalDatabase(): Promise<void> {
  await closeJournalDatabase();
  await SQLite.deleteDatabaseAsync(DATABASE_NAME).catch(() => undefined);
  await deleteDatabaseKey();
}

1. The blanket catch hides a real failure. expo-sqlite's iOS implementation throws two different errors from that call (node_modules/expo-sqlite/ios/SQLiteModule.swift, deleteDatabase(databasePath:)):

  • DatabaseNotFoundException — the file is already gone. Benign, and what the test at database.test.ts:159 intends to cover.
  • DeleteDatabaseException — thrown when the database is still in the module's connection cache, i.e. it was not closed. Not benign.

Both are swallowed identically, so a genuinely failed deletion resolves as success and the UI tells the user their data is gone when it is not.

2. A failure to close skips key deletion entirely. closeJournalDatabase() awaits db?.closeAsync() unguarded (database.ts:89). If that rejects, destroyJournalDatabase rejects before either deletion runs, leaving the file and the key — the one outcome where the user keeps a fully readable journal after asking for it to be destroyed.

3. The stated ordering rationale does not hold. The comment at database.ts:96-99, repeated at database.test.ts:153, says file-before-key means an interruption "leaves an unreadable database rather than a readable one with no key". A database with no key is unreadable by definition, so that sentence describes a state which cannot exist. Deleting the key first is the stronger order: it guarantees unreadability at the earliest possible moment, and it survives the case where file deletion then fails.

Also worth handling while in here: deleteDatabaseAsync removes only the main file, not the -wal / -shm sidecars. A clean close checkpoints and removes those, so it only matters on the path where the close failed — which is exactly the path in (2).

Nothing calls destroyJournalDatabase yet, so this is latent. It is probably best done as part of #116, so the error handling is designed alongside the UI that has to report it.

Acceptance Criteria

  • A failed file deletion is surfaced to the caller rather than swallowed; only "already absent" counts as success
  • A failure to close the handle does not prevent the key from being deleted
  • Key deletion happens before file deletion, or the comment is rewritten to describe the guarantee that actually holds
  • The comments in database.ts and the assertion in database.test.ts agree with whichever order is chosen
  • Tests cover: close fails, file deletion fails, file already absent
  • The feature/s being implemented are covered by unit tests - If not, create tests for them on this ticket

Additional Info and Resources

QA

  • With deleteDatabaseAsync mocked to reject, confirm the caller can distinguish failure from success
  • With closeAsync mocked to reject, confirm the key is still deleted
  • cd mobile && npm test — green

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingseverity:significantShould be fixed before shipping

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions