rubyforgood / rubyforgood/alongwithyou

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

Open
#135 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug severity:significant
Dominant language
TypeScript
Stars
9
Forks
4
Avg merge
9h 34m
Merged PRs (30d)
16

Description

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
  • Belongs with #116 ("Delete all my data" in-app control) — the promise made to the user is what sets the bar here
  • Found in review of #128; latent, not a regression
  • Whatever the caller does with a failure, it must not be a silent success — that is the only outcome this issue rules out
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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with destroyJournalDatabase() and closeJournalDatabase() in mobile/src/lib/db/database.ts, then read the related assertions around database.test.ts:153-159. Run the mobile test suite and inspect the existing mocks for closeAsync, deleteDatabaseAsync, and deleteDatabaseKey. Done means close, missing-file, and deletion-failure paths are distinguishable, key cleanup is covered, and the ordering comments and tests agree.

Written by the indexing model from the issue text.

Assessment

Tech stack
sqlite, typescript
Domain
database, mobile, testing
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.