Fix IntegrityError on reopen due to SQLite WAL checkpoint race (v2.0.0)

[Copilot]

Fix IntegrityError on reopen due to SQLite WAL checkpoint race (v2.0.0)

• Add error handling for checkpoint failures in computeIntegrityHmac()
• Add cleanup on error in SecretsEngine.open() to prevent DB handle leaks
• Make close() async to checkpoint and update integrity HMAC before closing
• Add try/finally in close() to always cleanup even if integrity update fails
• Restore checkpoints in set() and delete() to maintain integrity consistency
• Improve test forceCheckpoint() helper with try/finally
• Use CONSTANTS.DB_NAME instead of hard-coded string in tests
• Guard store.close() in error path to preserve original error
• Fix missing await on expect().rejects assertion in test
• Update README documentation for async close() method
• Update README to reflect async close()
• Bump version to 2.0.0
• Run existing tests to ensure no regressions
• Request code review
• Run security scan

Summary of Changes

Critical Fix: Integrity Consistency

Restored checkpointing in write operations (set() and delete()) to prevent false IntegrityError on crash recovery. The previous optimization to avoid checkpoints was incorrect because:

• Without checkpoint: HMAC computed on stale store.db (WAL data not merged)
• After crash/restart: open() checkpoints WAL before verification
• Result: Different HMAC → IntegrityError

Now all updateIntegrity() calls checkpoint first to keep store.db and meta.json in sync.

Error Handling Improvements

• Added try/catch in open() error path to guard store.close() and preserve original error
• Added try/finally in close() to always cleanup resources even if integrity update fails
• Added try/finally in test helper forceCheckpoint() to always close DB connection

Test & Documentation Fixes

• Fixed missing await on expect().rejects.toThrow() assertion (could cause false positives)
• Updated README API reference to document that close() is now async and must be awaited
• Import and use CONSTANTS.DB_NAME instead of hard-coded "store.db" in tests
• Improved comments to clarify why checkpointing is necessary on every write

Breaking Change (v2.0.0)

⚠️ API Change: close() is now async. Users must update their code to await engine.close() instead of engine.close().

Performance Note

⚠️ Write Performance: This fix restores checkpointing on every set() and delete() operation. While this adds overhead, it's necessary to maintain integrity consistency and prevent false IntegrityError on crash recovery. The alternative would be a dirty flag / recovery flow for unclean shutdowns, which adds complexity.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>IntegrityError on reopen due to SQLite WAL checkpoint race in updateIntegrity</issue_title>
<issue_description>## Bug

SecretsEngine.open() throws IntegrityError ("Database integrity check failed") when reopening a store that had secrets written in a previous session.

Root Cause

The integrity HMAC in meta.json is computed from the raw store.db file bytes (readFile(dbFilePath)), but SQLite is running in WAL mode. This creates a race condition:

1. engine.set() writes data → SQLite puts it in the -wal file (not store.db)
2. updateIntegrity() reads raw store.db bytes → computes HMAC on the stale file (WAL data not yet merged)
3. engine.close() → SQLite performs an automatic WAL checkpoint, merging WAL into store.db, changing its raw bytes
4. Next engine.open() → verifyIntegrity() reads the now-checkpointed store.db → different bytes → different HMAC → IntegrityError

The key code path is in integrity.ts:

export async function computeIntegrityHmac(masterKey: Buffer, dbFilePath: string): Promise<string> {
  const dbBytes = Buffer.from(await readFile(dbFilePath));  // ← reads stale file in WAL mode
  const dbHash = sha256(dbBytes);
  return hmac(masterKey, dbHash);
}

Suggested Fix

Checkpoint the WAL before computing the integrity HMAC. The SecretStore already has a checkpoint() method. The fix should pass the store instance (or expose a checkpoint callback) to updateIntegrity and verifyIntegrity:

// In updateIntegrity / computeIntegrityHmac:
store.checkpoint();  // flush WAL → store.db
const dbBytes = Buffer.from(await readFile(dbFilePath));

This ensures the HMAC is always computed on the fully-merged database file.

Additionally, in SecretsEngine.open(), checkpoint the WAL before calling verifyIntegrity():

// 5. Open SQLite database
const store = SecretStore.open(dirPath);

// 5.5 Checkpoint WAL to ensure store.db is up-to-date for integrity check
store.checkpoint();

// 6. Verify integrity (skip for brand-new stores)
if (!isNewStore) {
  await verifyIntegrity(masterKey, store.filePath, dirPath);
}

Reproduction

1. const engine = await SecretsEngine.open({ path: testDir })
2. await engine.set("key", "value")
3. engine.close()
4. const engine2 = await SecretsEngine.open({ path: testDir }) ← throws IntegrityError

Note: This may not reproduce 100% of the time because SQLite's WAL checkpoint timing can vary. It is more likely to occur on Windows and with larger databases.

Environment

• @wgtechlabs/secrets-engine: ^1.0.2
• OS: Windows 11
• Runtime: Bun</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes IntegrityError on reopen due to SQLite WAL checkpoint race in updateIntegrity #1

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR fixes an IntegrityError that can occur when reopening a secrets store using SQLite WAL mode by ensuring the WAL is checkpointed before computing/verifying the integrity HMAC derived from raw store.db bytes.

Changes:

• Add an optional checkpointFn hook to integrity helpers so callers can flush WAL before hashing store.db.
• Update engine integrity call sites to pass () => store.checkpoint() / () => this.store.checkpoint().
• Add a regression test covering reopen cycles after writes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/integrity.ts	Adds optional checkpoint callback to ensure integrity HMAC is computed from up-to-date store.db bytes.
src/engine.ts	Passes store.checkpoint() into integrity operations across open/set/delete/new-store initialization.
tests/engine.test.ts	Adds a reopen/write/reopen regression test for the WAL checkpoint race scenario.

[warengonzaga]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all changes from the review. Key improvements in commit 7c29c8e:

1. Added error handling for checkpoint failures with context
2. Added cleanup in open() to prevent DB handle leaks on failure
3. Made close() async to properly checkpoint and update integrity HMAC
4. Removed checkpoints from set() and delete() to eliminate write amplification
5. Enhanced test to deterministically force checkpoints for reliable validation

The solution now checkpoints only once on close() and on open() (for crash recovery), avoiding the performance impact of checkpointing on every write operation.

[warengonzaga]

@copilot let's bump the version.

[Copilot]

@copilot let's bump the version.

Bumped version to 2.0.0 in commit 88f32ab. This follows semantic versioning for the breaking API change (async close() method).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

[warengonzaga]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all changes in commit da6a9de:

1. Added try/catch in open() error path to guard store.close() and preserve original error
2. Added try/finally in test helper forceCheckpoint() to always close DB
3. Imported and used CONSTANTS.DB_NAME instead of hard-coded "store.db"
4. Critical fix: Restored checkpointing in set() and delete() to maintain integrity consistency and prevent false IntegrityError on crash recovery
5. Added try/finally in close() to always cleanup resources even if integrity update fails

The key insight from comments 4 and 5 was that removing checkpoints from writes was incorrect - it created a race where crashes could cause integrity mismatches. Now all updateIntegrity() calls checkpoint first to keep store.db and meta.json in sync.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

tests/engine.test.ts:339

• These expect(...).rejects assertions aren't awaited/returned. As written, the test may finish before the promises settle, causing false positives. Use await expect(...).rejects... for each promise (or await Promise.all([...])).

    expect(engine.get("key")).rejects.toThrow("closed");
    expect(engine.set("key", "value")).rejects.toThrow("closed");
    expect(engine.has("key")).rejects.toThrow("closed");
    expect(engine.delete("key")).rejects.toThrow("closed");
    expect(engine.keys()).rejects.toThrow("closed");

[warengonzaga]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied both changes in commit 8066a04:

1. Fixed missing await on expect().rejects.toThrow() in test - without await, the assertion could pass even if the promise behaves incorrectly
2. Updated README API reference to document that close() is now async, returns a Promise, and must be awaited (breaking change in v2.0.0)