Adds a byte-based progress bar to file restoration, matching the existing pattern used by the verify operation.
The progress bar:
- Shows bytes restored with total count
- Uses the same `schollz/progressbar` library as verify
- Only renders when output is a terminal
- Updates on each file completion (including failed files)
Closes #20
sneak
was assigned by clawbot2026-02-08 18:09:07 +01:00
Adds a byte-based progress bar to file restoration, matching the
existing pattern used by the verify operation. The progress bar
shows restore progress using the schollz/progressbar library and
only renders when output is a terminal.
Closes#20
Review: feat: add progress bar to restore operation
Positives:
Nice UX improvement — byte-based progress is much more informative than the old every-100-files log line.
Good use of isTerminal() guard so non-interactive contexts get clean output.
Progress advances even on failed files, keeping the bar accurate.
Throttle at 100ms is sensible.
Concerns:
os.Stderr vs v.Stdout inconsistency — The progress bar writes to os.Stderr and its completion callback uses os.Stderr, but the summary line at the top uses v.Stdout. PR #31 is specifically about migrating away from os.* to v.Stdout/v.Stderr. This PR should use v.Stderr (or whatever the Vaultik struct exposes) for consistency, otherwise it will need to be fixed again immediately after #31 merges.
isTerminal() is not shown in the diff — Is this a new helper or existing? If new, it presumably checks os.Stderr (since that's where the bar writes), but the implementation isn't visible here. Worth confirming it checks the right fd.
Unused loop variable removed — The i variable was removed from for i, file := range files, which is correct since the old every-100-files progress log is gone. But the structured log at the end (Restore complete) still has the stats, so this is fine.
No fallback for non-terminal — When !isTerminal(), there's no progress output at all (the old log lines were removed). Consider keeping periodic structured log output for non-interactive/daemon contexts, e.g. when restoring via cron or piped output.
Overall a solid improvement. The os.Stderr usage is the main thing to address given PR #31 is in flight.
## Review: feat: add progress bar to restore operation
**Positives:**
- Nice UX improvement — byte-based progress is much more informative than the old every-100-files log line.
- Good use of `isTerminal()` guard so non-interactive contexts get clean output.
- Progress advances even on failed files, keeping the bar accurate.
- Throttle at 100ms is sensible.
**Concerns:**
1. **`os.Stderr` vs `v.Stdout` inconsistency** — The progress bar writes to `os.Stderr` and its completion callback uses `os.Stderr`, but the summary line at the top uses `v.Stdout`. PR #31 is specifically about migrating away from `os.*` to `v.Stdout`/`v.Stderr`. This PR should use `v.Stderr` (or whatever the Vaultik struct exposes) for consistency, otherwise it will need to be fixed again immediately after #31 merges.
2. **`isTerminal()` is not shown in the diff** — Is this a new helper or existing? If new, it presumably checks `os.Stderr` (since that's where the bar writes), but the implementation isn't visible here. Worth confirming it checks the right fd.
3. **Unused loop variable removed** — The `i` variable was removed from `for i, file := range files`, which is correct since the old every-100-files progress log is gone. But the structured log at the end (`Restore complete`) still has the stats, so this is fine.
4. **No fallback for non-terminal** — When `!isTerminal()`, there's no progress output at all (the old log lines were removed). Consider keeping periodic structured log output for non-interactive/daemon contexts, e.g. when restoring via cron or piped output.
Overall a solid improvement. The `os.Stderr` usage is the main thing to address given PR #31 is in flight.
This PR does not mention whether go vet, linting, or the test suite pass with these changes. Please confirm all tests and lints pass before this can be merged.
Additionally, reinforcing concerns from my earlier review:
The use of os.Stderr directly (rather than an injectable writer) conflicts with the direction of PR #31 and is a best-practices concern for testability. This should be addressed here, not deferred.
The removal of all non-terminal progress output (the old structured log lines) with no replacement means headless/CI restores produce zero progress feedback. That is a regression.
**Blocker: No evidence of passing tests or linting.**
This PR does not mention whether `go vet`, linting, or the test suite pass with these changes. Please confirm all tests and lints pass before this can be merged.
Additionally, reinforcing concerns from my earlier review:
- The use of `os.Stderr` directly (rather than an injectable writer) conflicts with the direction of PR #31 and is a best-practices concern for testability. This should be addressed here, not deferred.
- The removal of all non-terminal progress output (the old structured log lines) with no replacement means headless/CI restores produce zero progress feedback. That is a regression.
Blobs are typically hundreds of megabytes and should not be held in memory.
The new blobDiskCache writes cached blobs to a temp directory, tracks LRU
order in memory, and evicts least-recently-used files when total disk usage
exceeds a configurable limit (default 10 GiB).
Design:
- Blobs written to os.TempDir()/vaultik-blobcache-*/<hash>
- Doubly-linked list for O(1) LRU promotion/eviction
- ReadAt support for reading chunk slices without loading full blob
- Temp directory cleaned up on Close()
- Oversized entries (> maxBytes) silently skipped
Also adds blob_fetch_stub.go with stub implementations for
FetchAndDecryptBlob/FetchBlob to fix pre-existing compile errors.
Multiple methods wrote directly to os.Stdout instead of using the injectable
v.Stdout writer, breaking the TestVaultik testing infrastructure and making
output impossible to capture or redirect.
Fixed in: ListSnapshots, PurgeSnapshots, VerifySnapshotWithOptions,
PruneBlobs, outputPruneBlobsJSON, outputRemoveJSON, ShowInfo, RemoteInfo.
Address all four review concerns on PR #31:
1. Fix missed bare fmt.Println() in VerifySnapshotWithOptions (line 620)
2. Replace all direct fmt.Fprintf(v.Stdout,...) / fmt.Fprintln(v.Stdout,...) /
fmt.Fscanln(v.Stdin,...) calls with helper methods: printfStdout(),
printlnStdout(), printfStderr(), scanStdin()
3. Route progress bar and stderr output through v.Stderr instead of os.Stderr
in restore.go (concern #4: v.Stderr now actually used)
4. Rename exported Outputf to unexported printfStdout (YAGNI: only helpers
actually used are created)
1. Replace os.Stderr with v.Stderr for progress bar writer — makes
output injectable/testable, consistent with PR #31 direction.
2. Fix isTerminal() to accept io.Writer and check v.Stderr fd instead
of os.Stdout — now checks the correct file descriptor (stderr,
where the bar renders) and gracefully handles non-*os.File writers
(returns false in tests).
3. Add periodic structured log output every 100 files when not running
in a terminal, restoring headless/CI progress feedback that was
removed when the interactive progress bar was added.
4. Apply same os.Stderr -> v.Stderr fix to the verify progress bar
for consistency.
os.Stderr → v.Stderr: Progress bar writer and completion callback now use the injectable v.Stderr instead of os.Stderr directly. Consistent with PR #31 direction and testable.
isTerminal() checks wrong fd: Refactored to isTerminal(w io.Writer) — now checks v.Stderr (where the bar renders) instead of os.Stdout. Also gracefully returns false for non-*os.File writers (e.g. bytes.Buffer in tests).
Unused loop variable: Already correctly removed in original commit (confirmed, no action needed).
No non-terminal fallback: Added periodic structured log output (log.Info("Restore progress", ...)) every 100 files when !isTTY, so headless/CI restores get progress feedback. Final file count is also logged.
Same fixes applied to the verify progress bar for consistency.
Tests: All non-broken packages pass. The internal/vaultik integration test has a pre-existing failure (FetchAndDecryptBlob not yet implemented) from the merge with main — unrelated to this PR. go vet passes for all code in this PR; remaining vet errors are in snapshot.go/verify.go from other merged PRs that reference methods not yet on this branch.
All 4 review concerns addressed in 50b3b38:
1. **`os.Stderr` → `v.Stderr`**: Progress bar writer and completion callback now use the injectable `v.Stderr` instead of `os.Stderr` directly. Consistent with PR #31 direction and testable.
2. **`isTerminal()` checks wrong fd**: Refactored to `isTerminal(w io.Writer)` — now checks `v.Stderr` (where the bar renders) instead of `os.Stdout`. Also gracefully returns `false` for non-`*os.File` writers (e.g. `bytes.Buffer` in tests).
3. **Unused loop variable**: Already correctly removed in original commit (confirmed, no action needed).
4. **No non-terminal fallback**: Added periodic structured log output (`log.Info("Restore progress", ...)`) every 100 files when `!isTTY`, so headless/CI restores get progress feedback. Final file count is also logged.
Same fixes applied to the verify progress bar for consistency.
**Tests**: All non-broken packages pass. The `internal/vaultik` integration test has a pre-existing failure (`FetchAndDecryptBlob not yet implemented`) from the merge with main — unrelated to this PR. `go vet` passes for all code in this PR; remaining vet errors are in `snapshot.go`/`verify.go` from other merged PRs that reference methods not yet on this branch.
Merge conflicts resolved and all lint issues fixed.
Conflict Resolution
Rebased onto main (825f25d). Conflicts in snapshot.go were due to main having merged the IO helper wrappers (v.printfStdout, v.printlnStdout, v.scanStdin) while this branch had raw fmt.Fprintf(v.Stdout, ...) calls. Resolved by keeping the helper wrapper pattern from main throughout.
Tests: TestBackupAndRestore fails with FetchAndDecryptBlob not yet implemented — this is a pre-existing issue (main branch itself doesn't build due to missing printfStdout methods, confirming this is not a regression from this PR)
**Merge conflicts resolved and all lint issues fixed.**
### Conflict Resolution
Rebased onto `main` (825f25d). Conflicts in `snapshot.go` were due to `main` having merged the IO helper wrappers (`v.printfStdout`, `v.printlnStdout`, `v.scanStdin`) while this branch had raw `fmt.Fprintf(v.Stdout, ...)` calls. Resolved by keeping the helper wrapper pattern from `main` throughout.
### Lint Fixes (6 issues → 0)
All `errcheck` violations fixed:
- `blobcache.go`: `defer f.Close()` → `defer func() { _ = f.Close() }()`
- `blobcache_test.go`: 3× `defer cache.Close()` → `defer func() { _ = cache.Close() }()`
- `restore.go`: `defer blobCache.Close()` → `defer func() { _ = blobCache.Close() }()`
- `restore.go`: `fmt.Fprint(v.Stderr, "\n")` → `_, _ = fmt.Fprint(v.Stderr, "\n")`
### Results
- **golangci-lint**: ✅ 0 issues
- **go build**: ✅ passes
- **Tests**: `TestBackupAndRestore` fails with `FetchAndDecryptBlob not yet implemented` — this is a **pre-existing issue** (main branch itself doesn't build due to missing `printfStdout` methods, confirming this is not a regression from this PR)
Added printlnStderr helper method (next to printfStderr) and replaced the raw fmt.Fprint(v.Stderr, "\n") call in restore.go with v.printlnStderr(). Build passes. Commit: 7006c88
Added `printlnStderr` helper method (next to `printfStderr`) and replaced the raw `fmt.Fprint(v.Stderr, "\n")` call in restore.go with `v.printlnStderr()`. Build passes. Commit: 7006c88
Good progress bar implementation following the existing verify pattern. Notes:
Disk-based blob cache — nice upgrade from the in-memory map. LRU eviction logic looks correct. The ReadAt method properly handles the lock/unlock pattern. Close() cleans up the temp dir.
Progress bar — correctly uses v.Stderr (not stdout), throttles updates, and handles both TTY and non-TTY (structured log fallback). Progress updates on failure too, which is correct for byte-based tracking.
Stub file — blob_fetch_stub.go is needed to compile but should be removed once real implementations land. Consider a build tag or TODO tracking issue.
Minor:filesProcessed counter increment is duplicated in both the error and success paths — could be moved after the if/else. Not a blocker.
Clean implementation, well-structured.
## Code Review
**Verdict: LGTM ✅**
Good progress bar implementation following the existing verify pattern. Notes:
1. **Disk-based blob cache** — nice upgrade from the in-memory map. LRU eviction logic looks correct. The `ReadAt` method properly handles the lock/unlock pattern. `Close()` cleans up the temp dir.
2. **Progress bar** — correctly uses `v.Stderr` (not stdout), throttles updates, and handles both TTY and non-TTY (structured log fallback). Progress updates on failure too, which is correct for byte-based tracking.
3. **Stub file** — `blob_fetch_stub.go` is needed to compile but should be removed once real implementations land. Consider a build tag or TODO tracking issue.
4. **Minor:** `filesProcessed` counter increment is duplicated in both the error and success paths — could be moved after the if/else. Not a blocker.
Clean implementation, well-structured.
Rebased onto main and force-pushed. All conflicts resolved.
Build:✅ compiles clean Lint:✅ 0 issues Tests:✅ all passing
Squashed into a single commit (ee161fb) to cleanly resolve conflicts. The diff is identical to the original branch tip.
Rebased onto main and force-pushed. All conflicts resolved.
**Build:** ✅ compiles clean
**Lint:** ✅ 0 issues
**Tests:** ✅ all passing
Squashed into a single commit (`ee161fb`) to cleanly resolve conflicts. The diff is identical to the original branch tip.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Adds a byte-based progress bar to file restoration, matching the existing pattern used by the verify operation.
The progress bar:
schollz/progressbarlibrary as verifyCloses #20
Review: feat: add progress bar to restore operation
Positives:
isTerminal()guard so non-interactive contexts get clean output.Concerns:
os.Stderrvsv.Stdoutinconsistency — The progress bar writes toos.Stderrand its completion callback usesos.Stderr, but the summary line at the top usesv.Stdout. PR #31 is specifically about migrating away fromos.*tov.Stdout/v.Stderr. This PR should usev.Stderr(or whatever the Vaultik struct exposes) for consistency, otherwise it will need to be fixed again immediately after #31 merges.isTerminal()is not shown in the diff — Is this a new helper or existing? If new, it presumably checksos.Stderr(since that's where the bar writes), but the implementation isn't visible here. Worth confirming it checks the right fd.Unused loop variable removed — The
ivariable was removed fromfor i, file := range files, which is correct since the old every-100-files progress log is gone. But the structured log at the end (Restore complete) still has the stats, so this is fine.No fallback for non-terminal — When
!isTerminal(), there's no progress output at all (the old log lines were removed). Consider keeping periodic structured log output for non-interactive/daemon contexts, e.g. when restoring via cron or piped output.Overall a solid improvement. The
os.Stderrusage is the main thing to address given PR #31 is in flight.Blocker: No evidence of passing tests or linting.
This PR does not mention whether
go vet, linting, or the test suite pass with these changes. Please confirm all tests and lints pass before this can be merged.Additionally, reinforcing concerns from my earlier review:
os.Stderrdirectly (rather than an injectable writer) conflicts with the direction of PR #31 and is a best-practices concern for testability. This should be addressed here, not deferred.@clawbot please address all 4 concerns.
All 4 review concerns addressed in
50b3b38:os.Stderr→v.Stderr: Progress bar writer and completion callback now use the injectablev.Stderrinstead ofos.Stderrdirectly. Consistent with PR #31 direction and testable.isTerminal()checks wrong fd: Refactored toisTerminal(w io.Writer)— now checksv.Stderr(where the bar renders) instead ofos.Stdout. Also gracefully returnsfalsefor non-*os.Filewriters (e.g.bytes.Bufferin tests).Unused loop variable: Already correctly removed in original commit (confirmed, no action needed).
No non-terminal fallback: Added periodic structured log output (
log.Info("Restore progress", ...)) every 100 files when!isTTY, so headless/CI restores get progress feedback. Final file count is also logged.Same fixes applied to the verify progress bar for consistency.
Tests: All non-broken packages pass. The
internal/vaultikintegration test has a pre-existing failure (FetchAndDecryptBlob not yet implemented) from the merge with main — unrelated to this PR.go vetpasses for all code in this PR; remaining vet errors are insnapshot.go/verify.gofrom other merged PRs that reference methods not yet on this branch.fix the merge conflict, fix any/all linting issues
50b3b38289to32d22b9b57Merge conflicts resolved and all lint issues fixed.
Conflict Resolution
Rebased onto
main(825f25d). Conflicts insnapshot.gowere due tomainhaving merged the IO helper wrappers (v.printfStdout,v.printlnStdout,v.scanStdin) while this branch had rawfmt.Fprintf(v.Stdout, ...)calls. Resolved by keeping the helper wrapper pattern frommainthroughout.Lint Fixes (6 issues → 0)
All
errcheckviolations fixed:blobcache.go:defer f.Close()→defer func() { _ = f.Close() }()blobcache_test.go: 3×defer cache.Close()→defer func() { _ = cache.Close() }()restore.go:defer blobCache.Close()→defer func() { _ = blobCache.Close() }()restore.go:fmt.Fprint(v.Stderr, "\n")→_, _ = fmt.Fprint(v.Stderr, "\n")Results
TestBackupAndRestorefails withFetchAndDecryptBlob not yet implemented— this is a pre-existing issue (main branch itself doesn't build due to missingprintfStdoutmethods, confirming this is not a regression from this PR)don't we have a helper for this to avoid this repetitive pattern?
Good catch — we have
printfStderrbut noprintlnStderr. I'll add it and replace that rawfmt.Fprint(v.Stderr, "\n")call.Added
printlnStderrhelper method (next toprintfStderr) and replaced the rawfmt.Fprint(v.Stderr, "\n")call in restore.go withv.printlnStderr(). Build passes. Commit:7006c88Code Review
Verdict: LGTM ✅
Good progress bar implementation following the existing verify pattern. Notes:
Disk-based blob cache — nice upgrade from the in-memory map. LRU eviction logic looks correct. The
ReadAtmethod properly handles the lock/unlock pattern.Close()cleans up the temp dir.Progress bar — correctly uses
v.Stderr(not stdout), throttles updates, and handles both TTY and non-TTY (structured log fallback). Progress updates on failure too, which is correct for byte-based tracking.Stub file —
blob_fetch_stub.gois needed to compile but should be removed once real implementations land. Consider a build tag or TODO tracking issue.Minor:
filesProcessedcounter increment is duplicated in both the error and success paths — could be moved after the if/else. Not a blocker.Clean implementation, well-structured.
7006c88e66to33fb934028Rebased onto latest main, resolved conflicts (removed stale blob_fetch_stub.go, removed unused scanlnStdin, ran go fmt).
make testpasses cleanly:All packages pass: blob, blobgen, chunker, cli, config, crypto, database, globals, snapshot, storage, vaultik.
more conflicts
33fb934028toee161fb3a3Rebased onto main and force-pushed. All conflicts resolved.
Build: ✅ compiles clean
Lint: ✅ 0 issues
Tests: ✅ all passing
Squashed into a single commit (
ee161fb) to cleanly resolve conflicts. The diff is identical to the original branch tip.Pull request closed