fix: use deleteSnapshotFromLocalDB in syncWithRemote #47

2026-03-17T21:42:35+01:00

clawbot okomentoval

2026-03-17 21:42:35 +01:00

Spolupracovník

Summary

syncWithRemote() was calling v.Repositories.Snapshots.Delete() directly when removing local snapshots not found in remote. This only deletes the snapshots row, leaving orphaned rows in snapshot_files, snapshot_blobs, and uploads tables.

Fix

Replaced the direct Delete() call with deleteSnapshotFromLocalDB(), which properly cleans up all related tables (snapshot_files, snapshot_blobs, uploads) before deleting the snapshot record itself.

This is a one-line change — the helper already existed and is used everywhere else in the codebase.

Testing

docker build . passes (fmt-check, lint, all tests, build)

closes #10

## Summary `syncWithRemote()` was calling `v.Repositories.Snapshots.Delete()` directly when removing local snapshots not found in remote. This only deletes the `snapshots` row, leaving orphaned rows in `snapshot_files`, `snapshot_blobs`, and `uploads` tables. ## Fix Replaced the direct `Delete()` call with `deleteSnapshotFromLocalDB()`, which properly cleans up all related tables (snapshot_files, snapshot_blobs, uploads) before deleting the snapshot record itself. This is a one-line change — the helper already existed and is used everywhere else in the codebase. ## Testing - `docker build .` passes (fmt-check, lint, all tests, build) closes https://git.eeqj.de/sneak/vaultik/issues/10

clawbot přidal/a 1 commit 2026-03-17 21:42:36 +01:00

fix: use deleteSnapshotFromLocalDB in syncWithRemote

check / check (pull_request) Successful in 2m30s

Podrobnosti

332ea26bce

syncWithRemote was calling v.Repositories.Snapshots.Delete() directly,
which only removes the snapshots row. This leaves orphaned rows in
snapshot_files, snapshot_blobs, and uploads tables.

Replace with deleteSnapshotFromLocalDB() which properly cleans up all
related tables before deleting the snapshot record.

closes #10

clawbot přidal/a needs-review štítek 2026-03-17 21:43:50 +01:00

clawbot posoudil/a

2026-03-17 21:45:44 +01:00

clawbot zanechal komentář

Autor

Spolupracovník

REVIEW: PASS — PR #47 for issue #10.

What changed

One-line fix in internal/vaultik/snapshot.go: the syncWithRemote() function was calling v.Repositories.Snapshots.Delete() directly, which only deleted the snapshot record and left orphaned rows in snapshot_files, snapshot_blobs, and uploads. Now calls v.deleteSnapshotFromLocalDB() which cleans up all related tables before deleting the snapshot itself.

Verification

Diff scope: 1 file, 1 line changed — minimal and surgical. ✅
deleteSnapshotFromLocalDB() verified at line 1013: deletes snapshot_files, snapshot_blobs, snapshot_uploads, then the snapshot record, in the correct order (related records first to respect foreign key constraints). ✅
Error handling: deleteSnapshotFromLocalDB returns early on first error, which is the right behavior — partial cleanup that deletes blobs but leaves the snapshot record would be worse than aborting. The caller already logs the error and continues to the next snapshot. ✅
No other regressions: Other call sites in the same file (lines ~370-382 in listSnapshots and ~1088-1100 in PruneDatabase) already do inline cleanup of all related tables before deleting. The internal/snapshot/snapshot.go delete path also handles cleanup correctly. This was the only call site with the orphan bug. ✅
docker build .: Passes (lint, fmt-check, tests, compilation). ✅
No test weakening, no linter config changes, no unrelated modifications. ✅

**REVIEW: PASS** — [PR #47](https://git.eeqj.de/sneak/vaultik/pulls/47) for [issue #10](https://git.eeqj.de/sneak/vaultik/issues/10). ## What changed One-line fix in `internal/vaultik/snapshot.go`: the `syncWithRemote()` function was calling `v.Repositories.Snapshots.Delete()` directly, which only deleted the snapshot record and left orphaned rows in `snapshot_files`, `snapshot_blobs`, and `uploads`. Now calls `v.deleteSnapshotFromLocalDB()` which cleans up all related tables before deleting the snapshot itself. ## Verification - **Diff scope**: 1 file, 1 line changed — minimal and surgical. ✅ - **`deleteSnapshotFromLocalDB()`** verified at line 1013: deletes `snapshot_files`, `snapshot_blobs`, `snapshot_uploads`, then the snapshot record, in the correct order (related records first to respect foreign key constraints). ✅ - **Error handling**: `deleteSnapshotFromLocalDB` returns early on first error, which is the right behavior — partial cleanup that deletes blobs but leaves the snapshot record would be worse than aborting. The caller already logs the error and continues to the next snapshot. ✅ - **No other regressions**: Other call sites in the same file (lines ~370-382 in `listSnapshots` and ~1088-1100 in `PruneDatabase`) already do inline cleanup of all related tables before deleting. The `internal/snapshot/snapshot.go` delete path also handles cleanup correctly. This was the only call site with the orphan bug. ✅ - **`docker build .`**: Passes (lint, fmt-check, tests, compilation). ✅ - **No test weakening, no linter config changes, no unrelated modifications.** ✅

clawbot přidáno merge-ready a odebráno needs-review štítků 2026-03-17 21:46:02 +01:00

sneak byl přiřazen clawbot

2026-03-17 21:46:02 +01:00

sneak okomentoval

2026-03-19 14:04:27 +01:00

Vlastník

is this necessary now that the schema is updated to do cascading deletes?

clawbot okomentoval

2026-03-19 14:04:56 +01:00

Autor

Spolupracovník

It depends on whether SQLite FK enforcement is actually enabled. SQLite requires PRAGMA foreign_keys = ON per connection — it's off by default, and many Go SQLite drivers don't enable it automatically.

If vaultik enables FK enforcement → yes, CASCADE makes this PR redundant (deleting the snapshot row would auto-delete related rows).

If vaultik does NOT enable FK enforcement → the CASCADE declarations in PRs #44/#46 are no-ops, and this explicit cleanup is the only thing preventing orphaned rows.

Let me check the codebase for foreign_keys pragma usage and report back.

It depends on whether SQLite FK enforcement is actually enabled. SQLite requires `PRAGMA foreign_keys = ON` per connection — it's off by default, and many Go SQLite drivers don't enable it automatically. If vaultik enables FK enforcement → yes, CASCADE makes this PR redundant (deleting the snapshot row would auto-delete related rows). If vaultik does NOT enable FK enforcement → the CASCADE declarations in PRs #44/#46 are no-ops, and this explicit cleanup is the only thing preventing orphaned rows. Let me check the codebase for `foreign_keys` pragma usage and report back.

clawbot okomentoval

2026-03-19 14:05:18 +01:00

Autor

Spolupracovník

Checked: FK enforcement IS enabled. database.go sets _foreign_keys=ON in the DSN and also runs PRAGMA foreign_keys = ON explicitly on each connection.

So with PRs #44 and #46 merged, CASCADE will handle cleanup automatically and deleteSnapshotFromLocalDB()'s explicit table-by-table deletion becomes redundant.

That said, this PR is still safe to merge — the explicit cleanup is a no-op when CASCADE is active (it deletes rows that CASCADE would have already deleted). It's belt-and-suspenders. But if you'd prefer to skip it as unnecessary, closing this PR is equally valid.

Your call.

Checked: FK enforcement IS enabled. `database.go` sets `_foreign_keys=ON` in the DSN and also runs `PRAGMA foreign_keys = ON` explicitly on each connection. So with PRs [#44](https://git.eeqj.de/sneak/vaultik/pulls/44) and [#46](https://git.eeqj.de/sneak/vaultik/pulls/46) merged, CASCADE will handle cleanup automatically and `deleteSnapshotFromLocalDB()`'s explicit table-by-table deletion becomes redundant. That said, this PR is still safe to merge — the explicit cleanup is a no-op when CASCADE is active (it deletes rows that CASCADE would have already deleted). It's belt-and-suspenders. But if you'd prefer to skip it as unnecessary, closing this PR is equally valid. Your call.

sneak okomentoval

2026-03-19 14:09:26 +01:00

Vlastník

Other call sites in the same file (lines ~370-382 in listSnapshots and ~1088-1100 in PruneDatabase) already do inline cleanup of all related tables before deleting.

do any of these have duplicated code, or should any of these related record cleanups be factored out into their own well-named methods for clarity?

> Other call sites in the same file (lines ~370-382 in listSnapshots and ~1088-1100 in PruneDatabase) already do inline cleanup of all related tables before deleting. do any of these have duplicated code, or should any of these related record cleanups be factored out into their own well-named methods for clarity?

clawbot okomentoval

2026-03-19 14:11:26 +01:00

Autor

Spolupracovník

Yes, there's one clear duplicate. PruneDatabase() (line ~1133) has an inline 4-step cleanup pattern that's identical to deleteSnapshotFromLocalDB():

// PruneDatabase — inline duplicate
if err := v.Repositories.Snapshots.DeleteSnapshotFiles(v.ctx, snapshotIDStr); err != nil { ... }
if err := v.Repositories.Snapshots.DeleteSnapshotBlobs(v.ctx, snapshotIDStr); err != nil { ... }
if err := v.Repositories.Snapshots.DeleteSnapshotUploads(v.ctx, snapshotIDStr); err != nil { ... }
if err := v.Repositories.Snapshots.Delete(v.ctx, snapshotIDStr); err != nil { ... }

This should just call deleteSnapshotFromLocalDB(snapshotIDStr) instead. The only difference is error handling: PruneDatabase logs and continues (best-effort), while deleteSnapshotFromLocalDB returns on first error. But with CASCADE enabled, the explicit sub-table deletes are redundant anyway — a single Snapshots.Delete() call would cascade.

There's also a copy in internal/snapshot/snapshot.go (~line 695), but that's on a different receiver (SnapshotManager vs Vaultik), so it can't directly call deleteSnapshotFromLocalDB. That one is also redundant with CASCADE.

If you want, I can file an issue to consolidate all snapshot cleanup to rely on CASCADE and remove the manual sub-table deletes entirely. With FK enforcement confirmed, it's dead code.

Yes, there's one clear duplicate. `PruneDatabase()` (line ~1133) has an inline 4-step cleanup pattern that's identical to `deleteSnapshotFromLocalDB()`: ```go // PruneDatabase — inline duplicate if err := v.Repositories.Snapshots.DeleteSnapshotFiles(v.ctx, snapshotIDStr); err != nil { ... } if err := v.Repositories.Snapshots.DeleteSnapshotBlobs(v.ctx, snapshotIDStr); err != nil { ... } if err := v.Repositories.Snapshots.DeleteSnapshotUploads(v.ctx, snapshotIDStr); err != nil { ... } if err := v.Repositories.Snapshots.Delete(v.ctx, snapshotIDStr); err != nil { ... } ``` This should just call `deleteSnapshotFromLocalDB(snapshotIDStr)` instead. The only difference is error handling: `PruneDatabase` logs and continues (best-effort), while `deleteSnapshotFromLocalDB` returns on first error. But with CASCADE enabled, the explicit sub-table deletes are redundant anyway — a single `Snapshots.Delete()` call would cascade. There's also a copy in `internal/snapshot/snapshot.go` (~line 695), but that's on a different receiver (`SnapshotManager` vs `Vaultik`), so it can't directly call `deleteSnapshotFromLocalDB`. That one is also redundant with CASCADE. If you want, I can file an issue to consolidate all snapshot cleanup to rely on CASCADE and remove the manual sub-table deletes entirely. With FK enforcement confirmed, it's dead code.

sneak uzavřel/a tento pull request

2026-03-20 06:55:39 +01:00

clawbot odkazoval/a na tento pull request

2026-03-20 06:57:51 +01:00

syncWithRemote deletes snapshot records without cleaning up related tables #10

Pull request uzavřen

Tento pull request nemůže být znovu otevřen protože větev byla smazána.

Přihlaste se pro zapojení do konverzace.

2 účastníků

Oznámení

Termín dokončení

Žádný termín dokončení.

Závislosti

Nejsou nastaveny žádné závislosti.

Reference: sneak/vaultik#47