sneak/vaultik
1
0
Rozštěpit 0
Zdrojový kód Úkoly 38 Pull requesty 5 Akce Balíčky Projekty Vydání Wiki Aktivita

Return errors from deleteSnapshotFromLocalDB instead of swallowing them (closes #25) #30

Sloučený
sneak sloučil 2 commity z větve fix/issue-25 do větve main před 2026-02-20 11:18:31 +01:00
Konverzace 9 Commity 2 Změněné soubory 1
+4 -4
clawbot okomentoval 2026-02-08 21:05:02 +01:00
Spolupracovník
Kopírovat odkaz
Copy Source
K dispozici není žádný popis.
sneak byl přiřazen clawbot 2026-02-08 21:05:02 +01:00
clawbot přidal/a 1 commit 2026-02-08 21:05:02 +01:00
fix: return errors from deleteSnapshotFromLocalDB instead of swallowing them 9a6a8caff9 
Previously, deleteSnapshotFromLocalDB logged errors but always returned nil,
causing callers to believe deletion succeeded even when it failed. This could
lead to data inconsistency where remote metadata is deleted while local
records persist.

Now returns the first error encountered, allowing callers to handle failures
appropriately.
clawbot zanechal komentář
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

Review: Return errors from deleteSnapshotFromLocalDB instead of swallowing them

Positives:

  • Clear, correct fix. Swallowing errors in a delete cascade is a real bug — callers had no way to know the DB was left in an inconsistent state.
  • Error messages use consistent "deleting X for %s: %w" format with %w wrapping. Clean.
  • Minimal, focused diff.

Concerns:

  1. Partial deletion on early return — If DeleteSnapshotFiles succeeds but DeleteSnapshotBlobs fails, you've now deleted files but left blobs and the snapshot record behind. The old code at least attempted all four deletes regardless of individual failures. Consider wrapping all errors and returning them together (e.g. errors.Join), or using a transaction so the whole thing is atomic. At minimum, document that callers should be aware of partial state.

  2. Callers may not handle the new errors — Previously this function effectively never failed from the caller's perspective. Check that all call sites handle the returned error appropriately (log it, surface it to the user, retry, etc.).

Despite the partial-delete concern, returning errors is strictly better than silently swallowing them. The partial-delete issue existed before too (just hidden). Ship it, and consider the transaction/multi-error approach as a follow-up.

## Review: Return errors from deleteSnapshotFromLocalDB instead of swallowing them **Positives:** - Clear, correct fix. Swallowing errors in a delete cascade is a real bug — callers had no way to know the DB was left in an inconsistent state. - Error messages use consistent `"deleting X for %s: %w"` format with `%w` wrapping. Clean. - Minimal, focused diff. **Concerns:** 1. **Partial deletion on early return** — If `DeleteSnapshotFiles` succeeds but `DeleteSnapshotBlobs` fails, you've now deleted files but left blobs and the snapshot record behind. The old code at least attempted all four deletes regardless of individual failures. Consider wrapping all errors and returning them together (e.g. `errors.Join`), or using a transaction so the whole thing is atomic. At minimum, document that callers should be aware of partial state. 2. **Callers may not handle the new errors** — Previously this function effectively never failed from the caller's perspective. Check that all call sites handle the returned error appropriately (log it, surface it to the user, retry, etc.). Despite the partial-delete concern, returning errors is strictly better than silently swallowing them. The partial-delete issue existed before too (just hidden). Ship it, and consider the transaction/multi-error approach as a follow-up.
clawbot okomentoval 2026-02-16 05:57:24 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

Blocker: No evidence of passing tests or linting.

Please confirm go vet, linting, and the full test suite pass. Additionally:

  • Are there tests covering the error-return paths? The behavior change (previously silent, now returns errors) could break callers that do not expect failures here. Unit tests exercising each failure branch would be expected for this kind of change.
**Blocker: No evidence of passing tests or linting.** Please confirm `go vet`, linting, and the full test suite pass. Additionally: - Are there tests covering the error-return paths? The behavior change (previously silent, now returns errors) could break callers that do not expect failures here. Unit tests exercising each failure branch would be expected for this kind of change.
sneak okomentoval 2026-02-16 06:11:10 +01:00
Vlastník
Kopírovat odkaz
Copy Source

@clawbot please run tests and linting checks per usual and comment with results here.

@clawbot please run tests and linting checks per usual and comment with results here.
clawbot okomentoval 2026-02-16 06:13:26 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

CI Check Results for fix/issue-25

Commit: 9a6a8ca

FAIL: Build / Format

Merge conflict markers found in internal/vaultik/snapshot.go (lines 1129-1150). go fmt and compilation both fail.

FAIL: Lint (golangci-lint)

New errors introduced by this PR (not on main):

  • prune.go:6: os imported and not used
  • prune.go:201: undefined: v
  • restore.go:22: golang.org/x/term imported and not used
  • restore.go:631: undefined: v / too many arguments in call to isTerminal

Pre-existing (also on main): 4x unused functions in io.go

PASS: go vet

Tests (go test ./...)

Most packages pass. One failure: TestBackupAndRestore (blob not found during restore, likely pre-existing).

Summary

This PR has merge conflict markers and compilation errors. Needs fixes in snapshot.go (conflict), prune.go (unused import, undefined var), and restore.go (unused import, undefined var).

## CI Check Results for fix/issue-25 Commit: 9a6a8ca ### FAIL: Build / Format Merge conflict markers found in internal/vaultik/snapshot.go (lines 1129-1150). go fmt and compilation both fail. ### FAIL: Lint (golangci-lint) New errors introduced by this PR (not on main): - prune.go:6: os imported and not used - prune.go:201: undefined: v - restore.go:22: golang.org/x/term imported and not used - restore.go:631: undefined: v / too many arguments in call to isTerminal Pre-existing (also on main): 4x unused functions in io.go ### PASS: go vet ### Tests (go test ./...) Most packages pass. One failure: TestBackupAndRestore (blob not found during restore, likely pre-existing). ### Summary This PR has merge conflict markers and compilation errors. Needs fixes in snapshot.go (conflict), prune.go (unused import, undefined var), and restore.go (unused import, undefined var).
clawbot okomentoval 2026-02-16 06:18:26 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

Correction: The previous CI check report was incorrect. The merge conflict markers and compilation errors were artifacts of a dirty local working directory (multiple sub-agents sharing the same clone). The actual branch (fix/issue-25, commit 9a6a8ca) on Gitea is clean — no conflict markers, merges cleanly into current main.

I will re-run the CI checks in a fresh, isolated clone and post corrected results.

**Correction:** The previous CI check report was incorrect. The merge conflict markers and compilation errors were artifacts of a dirty local working directory (multiple sub-agents sharing the same clone). The actual branch (`fix/issue-25`, commit 9a6a8ca) on Gitea is clean — no conflict markers, merges cleanly into current main. I will re-run the CI checks in a fresh, isolated clone and post corrected results.
clawbot okomentoval 2026-02-16 06:19:48 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

CI re-check test

CI re-check test
clawbot okomentoval 2026-02-16 06:20:13 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

CI Re-check Results (branch fix/issue-25)

Build (go build ./...) - FAIL

internal/vaultik/restore.go:478:19: v.FetchAndDecryptBlob undefined (type *Vaultik has no field or method FetchAndDecryptBlob)
internal/vaultik/verify.go:290:22: v.FetchBlob undefined (type *Vaultik has no field or method FetchBlob)

Vet (go vet ./...) - FAIL

Same compilation errors as above.

Tests (go test ./...) - PARTIAL FAIL

Passing: blob, blobgen, chunker, config, crypto, database, globals, models, pidlock, s3, snapshot
Failing (build failed): cmd/vaultik, internal/cli, internal/vaultik

Formatting (gofmt -l .) - PASS

No formatting issues.

Summary

Two undefined methods block compilation:

  • v.FetchAndDecryptBlob in restore.go:478
  • v.FetchBlob in verify.go:290

These methods are called on *Vaultik but not defined on the type. The PR needs to add these methods or update the call sites.

## CI Re-check Results (branch fix/issue-25) ### Build (go build ./...) - FAIL internal/vaultik/restore.go:478:19: v.FetchAndDecryptBlob undefined (type *Vaultik has no field or method FetchAndDecryptBlob) internal/vaultik/verify.go:290:22: v.FetchBlob undefined (type *Vaultik has no field or method FetchBlob) ### Vet (go vet ./...) - FAIL Same compilation errors as above. ### Tests (go test ./...) - PARTIAL FAIL Passing: blob, blobgen, chunker, config, crypto, database, globals, models, pidlock, s3, snapshot Failing (build failed): cmd/vaultik, internal/cli, internal/vaultik ### Formatting (gofmt -l .) - PASS No formatting issues. ### Summary Two undefined methods block compilation: - v.FetchAndDecryptBlob in restore.go:478 - v.FetchBlob in verify.go:290 These methods are called on *Vaultik but not defined on the type. The PR needs to add these methods or update the call sites.
clawbot okomentoval 2026-02-19 06:38:33 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

Code Review

Verdict: LGTM ✅

Clean, straightforward fix. The change correctly converts silent error-swallowing (log.Error + continue) into proper error returns with fmt.Errorf and %w wrapping.

One consideration: The early-return pattern means that if DeleteSnapshotFiles fails, the snapshot record, blobs, and uploads remain — leaving partial state. The previous code (logging and continuing) would at least attempt all deletions. Whether this is desirable depends on whether callers retry or whether partial cleanup is worse than leaving orphaned child records. If the caller retries the whole operation, early-return is correct. If not, you might want to accumulate errors with errors.Join and attempt all four deletions regardless.

Otherwise the error messages are clear and consistent. Good to merge as-is if callers handle retry.

## Code Review **Verdict: LGTM ✅** Clean, straightforward fix. The change correctly converts silent error-swallowing (`log.Error` + continue) into proper error returns with `fmt.Errorf` and `%w` wrapping. **One consideration:** The early-return pattern means that if `DeleteSnapshotFiles` fails, the snapshot record, blobs, and uploads remain — leaving partial state. The previous code (logging and continuing) would at least attempt all deletions. Whether this is desirable depends on whether callers retry or whether partial cleanup is worse than leaving orphaned child records. If the caller retries the whole operation, early-return is correct. If not, you might want to accumulate errors with `errors.Join` and attempt all four deletions regardless. Otherwise the error messages are clear and consistent. Good to merge as-is if callers handle retry.
clawbot vynucené nahrání fix/issue-25 od 9a6a8caff9 do ddc23f8057 2026-02-20 08:56:43 +01:00 Porovnat
clawbot přidal/a merge-ready štítek 2026-02-20 09:17:17 +01:00
clawbot okomentoval 2026-02-20 11:13:12 +01:00
Autor
Spolupracovník
Kopírovat odkaz
Copy Source

make test passes cleanly (no rebase needed):

golangci-lint run — 0 issues
go fmt — clean
go test — all PASS
`make test` passes cleanly (no rebase needed): ``` golangci-lint run — 0 issues go fmt — clean go test — all PASS ```
sneak přidal/a 1 commit 2026-02-20 11:18:08 +01:00
Merge branch 'main' into fix/issue-25 1e2eced092
sneak sloučený commit 597b560398 do main 2026-02-20 11:18:31 +01:00
Přihlaste se pro zapojení do konverzace.
Posuzovatelé
Žádní posuzovatelé
clawbot
Štítky
Zrušit štítky
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No labels merge-ready
Milník
Žádné položky
Bez milníku
Projekty
Vymazat projekty
No projects
Zpracovatelé
Smazat zpracovatele
clawbot sneak (Jeffrey Paul)
Bez zpracovatelů sneak
2 účastníků
Oznámení
Termín dokončení
Žádný termín dokončení.
Závislosti

Nejsou nastaveny žádné závislosti.

Reference: sneak/vaultik#30