Review Summary

Overall this is a well-structured PR that addresses real pain points (Windows EBUSY, stale singletons after restore, incomplete factory reset). The approach of using app.relaunch() instead of renderer-only reload is the right call, and the new App_ResetData IPC channel cleanly separates the factory reset logic.

Key findings:

1. Error resilience in close sequences — The three close() calls in both ipc.ts and BackupManager.ts run sequentially without individual error guards. If memoryService.close() throws (it lacks internal try/catch unlike the other two), the remaining close calls and fs.promises.rm won't execute. Consider .catch() guards or Promise.allSettled.

2. Race condition in DatabaseManager.close() — The method nulls DatabaseManager.instance last, creating a small window where concurrent getInstance() calls could return a stale instance with null client. Nulling the singleton first would be safer.

3. (db as any).client in KnowledgeService — Acknowledged in the PR description as a necessary encapsulation break. Would benefit from a tracking issue reference and a warn-level log when client is not accessible (to catch upstream changes).

4. Inconsistent singleton access — BackupManager.ts uses MemoryService.getInstance().close() while ipc.ts uses a module-level memoryService reference. Not a bug, but worth standardizing.

5. Factory reset UX — Unlike the restore paths which show a success toast before relaunching with a 1s delay, the factory reset path relaunches immediately with no user feedback.

None of these are blockers — the core logic is sound and addresses the bugs correctly. The suggestions above are improvements for robustness.

Re-review: All feedback addressed ✅

All five review comments have been addressed. Summary of the fixes:

1. Error handling ✅ — .catch() guards added on memoryService.close() and KnowledgeService.closeAll() in both ipc.ts and BackupManager.ts, ensuring fs.promises.rm always runs.

2. KnowledgeService encapsulation ✅ — warn-level log when client is not accessible, plus a TODO comment tracking the need for upstream close() support in LibSqlDb.

3. DatabaseManager race condition ✅ — DatabaseManager.instance is now nulled first, so concurrent getInstance() calls will create a fresh connection instead of getting a stale one.

4. BackupManager singleton access ✅ — Author clarified: using MemoryService.getInstance() is intentional here since BackupManager is an independent class without a module-level memoryService reference. Makes sense.

5. Factory reset UX ✅ — Now shows toast.success and uses the same setTimeout(() => relaunchApp(), 1000) pattern as restore paths. i18n translations for message.reset.success added across all locales.

LGTM. Clean, well-addressed feedback. No further concerns.

Review: Gap Analysis — Are all file handles released?

The core approach is sound — relaunchApp() over reload() is the right architectural call, and the new App_ResetData IPC cleanly separates the factory reset logic. The three issues are correctly root-caused and addressed. However, I spotted two gaps that could still cause EBUSY on Windows:

1. FileStorage chokidar watcher not stopped before fs.remove() (Medium severity)

Affected paths: BackupManager.restore() (line ~413) and App_ResetData IPC handler in ipc.ts

FileStorage uses chokidar to watch Data/Files/ and Data/Notes/ directories (FileStorage.ts:1674). This watcher holds open file handles via fs.watch/FSEvents underneath. Before calling fs.remove(destPath) in BackupManager or fs.promises.rm(getDataPath()) in the reset handler, the watcher is not stopped.

On Windows, chokidar's underlying ReadDirectoryChangesW holds directory handles that will cause EBUSY when trying to delete the directory — the exact same class of bug this PR is fixing for DB connections.

FileStorage already exposes stopFileWatcher() (FileStorage.ts:1798). Consider calling it before fs.remove:

// In BackupManager.restore(), before fs.remove(destPath):
await fileStorage.stopFileWatcher()

// Similarly in App_ResetData handler, before fs.promises.rm():
await fileStorage.stopFileWatcher()

Since the app relaunches immediately after, there's no need to restart the watcher.

2. KnowledgeService.closeAll() silently skips unclosable handles (Medium severity)

Location: KnowledgeService.ts, closeAll() method

When (db as any).client is not accessible (e.g., upstream LibSqlDb changes its internals), the current code logs a warning and continues:

if (client && typeof client.close === 'function') {
  client.close()
} else {
  logger.warn(`Cannot close database instance for id: ${id} — client not accessible`)
  continue  // ← caller thinks all handles released
}

The caller (both BackupManager and App_ResetData) will proceed to fs.remove() believing all handles are released, but the skipped DBs still hold file locks. On Windows this means EBUSY — silently masking the very bug this PR fixes.

Suggestion: throw an error (or collect failures and throw at the end) so the caller is aware that not all handles were released:

} else {
  const msg = `Cannot close database instance for id: ${id} — client not accessible`
  logger.error(msg)
  throw new Error(msg)
}

Or at minimum, track failures and return a result that callers can check:

public closeAll = async (): Promise<{ failed: string[] }> => {
  const failed: string[] = []
  // ...
  if (!client || typeof client.close !== 'function') {
    failed.push(id)
  }
  // ...
  return { failed }
}

Minor note

The inconsistent singleton access pattern (MemoryService.getInstance().close() in BackupManager vs module-level memoryService in ipc.ts) is fine given the different module contexts, just calling it out for awareness.

Summary

The PR correctly fixes the root causes of all three issues. The two gaps above are worth addressing before merge to avoid residual EBUSY failures on Windows, particularly the FileStorage watcher which is a near-certain issue when the Notes panel has been opened during the session.