fix(datastore): gracefully handle VACUUM failure in compiled binary (swamp-club#288)

[stack72]

Summary

• CatalogStore.vacuum() now catches errors and returns a boolean instead of throwing when SQLITE_LIMIT_ATTACHED=0 in the canary Deno runtime
• DatastoreCompactData gains a vacuumSkipped field so callers know VACUUM was skipped
• Renderers show a warning when vacuum is skipped (log mode) and include vacuumSkipped in JSON output
• WAL checkpoint (the main space-saving operation) is unaffected — compact still succeeds

Test Plan

• Added unit test for datastoreCompact with vacuum() returning false — asserts vacuumSkipped: true
• Added unit test for CatalogStore.vacuum() — asserts it returns a boolean and does not throw
• Verified reproduction: compiled binary now exits 0 with {"vacuumSkipped": true} instead of crashing
• All 5617 existing tests pass with 0 failures

Closes swamp-club#288

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. The log-mode warning VACUUM skipped (runtime limitation) — WAL checkpoint still reclaimed space is clear and reassuring, but "runtime limitation" is vague for users who may not know what SQLITE_LIMIT_ATTACHED is. A slightly more specific message like VACUUM skipped (SQLite restriction in this runtime) would be marginally clearer — minor, not blocking.

Verdict

PASS — Graceful degradation with correct UX: exits 0, emits a warn-level message in log mode, and exposes vacuumSkipped in JSON output. Field naming is consistent (camelCase, matches walPagesTotal, dbBytesReclaimed). No flags added, no help text needed.

[github-actions]

Code Review

Blocking Issues

None.

Suggestions

1. Broad catch in CatalogStore.vacuum() (src/infrastructure/persistence/catalog_store.ts:574): The catch block swallows all errors, not just the known SQLITE_LIMIT_ATTACHED=0 case. If a future failure has a different root cause (e.g. disk full, permission denied), it will be silently downgraded to a warning. Consider narrowing the catch to match the specific error message/code for the canary runtime limitation, re-throwing unexpected errors. This is non-blocking since the error is logged and the caller knows vacuum was skipped, but it would improve debuggability.

Overall this is a clean, well-structured fix. Layer placement follows DDD conventions correctly — infrastructure concern stays in persistence, the application service (libswamp) abstracts it via the DatastoreCompactDeps interface, and the presentation layer imports only from libswamp/mod.ts. Test coverage is good for both the happy path and the degraded path.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Overly broad catch in vacuum() — src/infrastructure/persistence/catalog_store.ts:574

   The catch block swallows all errors from VACUUM, but the intent (per the docstring and PR description) is to handle specifically SQLITE_LIMIT_ATTACHED=0 in the canary Deno runtime. In practice this also silently downgrades:

   • SQLITE_CORRUPT — database corruption
   • SQLITE_IOERR — disk I/O failure
   • SQLITE_FULL — disk full
   • SQLITE_BUSY — database locked by another connection

   Breaking scenario: The disk fills up mid-operation. VACUUM fails with SQLITE_FULL. The user sees "VACUUM skipped (runtime limitation)" and the command exits 0, masking a real infrastructure problem. The warn-level log line includes the error object, so it's not fully silent, but the UX (exit 0, "runtime limitation" language) is misleading for these cases.

   Why this is MEDIUM, not HIGH: VACUUM is a read-compact-rewrite operation — its failure never corrupts data or loses writes. The WAL checkpoint (the actually important step) runs first and is unaffected. The worst outcome is the user doesn't realize their disk is full until the next write fails.

   Suggested fix: Check the error message for the known canary string (e.g. SQLITE_LIMIT or LIMIT_ATTACHED) and only return false for that. Re-throw everything else. Or at minimum, adjust the warning to include — if this is unexpected, check disk and database health so the user has a breadcrumb.

Low

1. Test only covers happy path for CatalogStore.vacuum() — src/infrastructure/persistence/catalog_store_test.ts:583-593

   The new CatalogStore test asserts that vacuum() returns a boolean and doesn't throw, but only exercises the success path (a working in-memory DB). There is no test that forces VACUUM to fail and asserts false is returned. The compact_test.ts does cover this at the deps-injection level (stubbing vacuum: () => false), but the actual catch-and-return-false code path in CatalogStore is untested. This is low severity because the logic is trivial (try/catch returning a boolean), and the Deno test environment may not make it easy to trigger SQLITE_LIMIT_ATTACHED=0.

Verdict

PASS — The change is well-scoped, the type plumbing is correct across all layers, the new interface field is properly additive, and the only caller is correctly updated. The broad catch is a pragmatic trade-off for a non-destructive operation, though narrowing it would be ideal as a follow-up.