diff --git a/CHANGELOG.md b/CHANGELOG.md index 478e85b..c007c46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,38 @@ keeps reading that version for at least 24 months after a successor lands. ## [Unreleased] +## [1.0.10] — 2026-07-15 + +### Fix: `recovery drill` failed every WAL-streaming backup (#26) + +The verify sandbox that `recovery drill` and `verify --full` use ran +`pg_verifybackup` without `-n`/`--no-parse-wal`. pg_hardstorage stores +WAL in the repository rather than inside the base backup, so the +restored data directory legitimately has an empty `pg_wal/` — the +WAL-parse step therefore failed every structurally-valid WAL-streaming +backup with `could not find any WAL file`, and the drill reported +`verdict: fail` for backups that restore, recover, and serve data +correctly. The sandbox now verifies the manifest and file checksums +with `-n`, matching the restore path's `--verify` gate (the same +defect was fixed there in 1.0.8). + +### Fix: `hold remove` reported success for holds that don't exist + +Releasing a hold with a typo'd backup ID printed `✓ Hold released` +and exited 0 while the real hold silently kept blocking retention — a +false success on the legal-hold path. Removal of a nonexistent hold +now fails with `notfound.hold` (exit 6) and points at `hold list`; +releasing an existing hold is unchanged. + +### Fix: JSON output shape papercuts + +- `backup compare -o json` double-nested its payload under + `.result.result.*`; the comparison fields now sit at `.result.*` + like every other command. +- `list` on a deployment with no backups emitted `"backups": null`; + it now emits `[]`, so `jq '.result.backups[]'` and every other + iterator handle the empty case. + ## [1.0.9] — 2026-07-13 Twenty operator-annoyance fixes, found by systematically exercising the diff --git a/docs/compliance/pci-dss.md b/docs/compliance/pci-dss.md index 4c59496..3ffb064 100644 --- a/docs/compliance/pci-dss.md +++ b/docs/compliance/pci-dss.md @@ -143,7 +143,7 @@ audit period plus the cosign signatures on the running binary: ```sh -VERSION=1.0.9 # the release / image tag you're attesting +VERSION=1.0.10 # the release / image tag you're attesting # 1. Audit chain bundle pg_hardstorage audit export-bundle \ diff --git a/docs/compliance/slsa-l3-provenance.md b/docs/compliance/slsa-l3-provenance.md index d0181b9..6191af7 100644 --- a/docs/compliance/slsa-l3-provenance.md +++ b/docs/compliance/slsa-l3-provenance.md @@ -74,7 +74,7 @@ Verify a downloaded artifact against its provenance with the `slsa-verifier`: ```sh -VERSION=1.0.9 # the release you're verifying +VERSION=1.0.10 # the release you're verifying slsa-verifier verify-artifact \ --provenance-path pg_hardstorage.intoto.jsonl \ --source-uri github.com/cybertec-postgresql/pg_hardstorage \ @@ -109,7 +109,7 @@ When images ship, they are intended to carry a cosign signature plus an image-level SLSA provenance attestation: ```sh -VERSION=1.0.9 # the image tag you're verifying +VERSION=1.0.10 # the image tag you're verifying # Verify the image signature cosign verify "ghcr.io/cybertec-postgresql/pg_hardstorage:v${VERSION}" \ @@ -139,7 +139,7 @@ JSON format, generated by `syft` (configured via the and attached to the GitHub Release: ```sh -VERSION=1.0.9 # the release you're inspecting +VERSION=1.0.10 # the release you're inspecting # The SBOM is a plain file on the Release; inspect it directly jq -r '.packages[].name' \ "pg_hardstorage_${VERSION}_linux_amd64.tar.gz.spdx.sbom.json" diff --git a/docs/reference/runbooks/control-plane-setup.md b/docs/reference/runbooks/control-plane-setup.md index 68f97f8..8326b0a 100644 --- a/docs/reference/runbooks/control-plane-setup.md +++ b/docs/reference/runbooks/control-plane-setup.md @@ -249,7 +249,7 @@ curl --cacert /etc/pg_hardstorage/server/cert.pem \ # { # "id": "db1.example.com", # "host": "db1.example.com", -# "version": "v1.0.9", +# "version": "v1.0.10", # "deployments": ["db1", "db2"], # "registered_at": "...", # "last_heartbeat": "..." diff --git a/docs/tutorials/getting-started.md b/docs/tutorials/getting-started.md index 7045a8b..0f4db32 100644 --- a/docs/tutorials/getting-started.md +++ b/docs/tutorials/getting-started.md @@ -35,7 +35,7 @@ tarballs (Windows is CLI-only). Grab the matching one from verify the cosign signature, and drop the binary on your `$PATH`: ```sh -VERSION=1.0.9 # latest release: https://github.com/cybertec-postgresql/pg_hardstorage/releases/latest +VERSION=1.0.10 # latest release: https://github.com/cybertec-postgresql/pg_hardstorage/releases/latest curl -LO "https://github.com/cybertec-postgresql/pg_hardstorage/releases/download/v${VERSION}/pg_hardstorage_${VERSION}_linux_amd64.tar.gz" tar xzf "pg_hardstorage_${VERSION}_linux_amd64.tar.gz" sudo install -m 0755 pg_hardstorage /usr/local/bin/ @@ -45,7 +45,7 @@ pg_hardstorage version ### `.deb` (Debian / Ubuntu) ```sh -VERSION=1.0.9 # the release you downloaded +VERSION=1.0.10 # the release you downloaded sudo dpkg -i "pg-hardstorage_${VERSION}_amd64.deb" ``` @@ -57,7 +57,7 @@ creates `/etc/pg_hardstorage/`, `/var/lib/pg_hardstorage/`, ### `.rpm` (Fedora / RHEL / Rocky / Alma) ```sh -VERSION=1.0.9 # the release you downloaded +VERSION=1.0.10 # the release you downloaded sudo rpm -i "pg-hardstorage-${VERSION}-1.x86_64.rpm" ``` @@ -259,7 +259,7 @@ you are doing — exit 9 means the verifier said no. ```sh $ pg_hardstorage version -pg_hardstorage v1.0.9 (abc1234, built 2026-04-29T12:00:00Z) +pg_hardstorage v1.0.10 (abc1234, built 2026-04-29T12:00:00Z) ``` `doctor` is the single-command "is anything wrong" check. It prints a diff --git a/internal/cli/backup_compare.go b/internal/cli/backup_compare.go index a91e5fe..2974519 100644 --- a/internal/cli/backup_compare.go +++ b/internal/cli/backup_compare.go @@ -110,8 +110,8 @@ func runBackupCompare(cmd *cobra.Command, deployment, idA, idB, repoURL string, } return d.Result(output.NewResult(cmd.CommandPath()).WithBody(backupCompareBody{ - Deployment: deployment, - Result: res, + Deployment: deployment, + ComparisonResult: res, })) } @@ -140,18 +140,20 @@ func mapCompareReadError(side, deployment, backupID string, err error) error { fmt.Sprintf("backup compare: %s side: %v", side, err)).Wrap(err) } -// backupCompareBody is the v1-stable result shape. Embeds the -// ComparisonResult plus the deployment name (since the result -// itself only carries per-side BackupID). +// backupCompareBody is the v1-stable result shape: the +// ComparisonResult's fields inlined (via embedding) alongside the +// deployment name. It previously wrapped the result under a nested +// "result" key, so JSON consumers saw the body at `.result.result.*` +// — double-nested relative to every sibling command. type backupCompareBody struct { - Deployment string `json:"deployment"` - Result *backup.ComparisonResult `json:"result"` + Deployment string `json:"deployment"` + *backup.ComparisonResult } // WriteText renders the comparison as compact tables: per-side // summary, file-class counts, chunk-class counts, top-N deltas. func (b backupCompareBody) WriteText(w io.Writer) error { - r := b.Result + r := b.ComparisonResult bw := &strings.Builder{} fmt.Fprintf(bw, "compare %s/%s ↔ %s\n", b.Deployment, r.A.BackupID, r.B.BackupID) diff --git a/internal/cli/hold.go b/internal/cli/hold.go index 7611a16..49b28d2 100644 --- a/internal/cli/hold.go +++ b/internal/cli/hold.go @@ -444,12 +444,26 @@ func runHoldRemove(cmd *cobra.Command, deployment, backupID, repoURL string, yes defer sp.Close() store := backup.NewManifestStore(sp) - // Capture the hold's holder/reason BEFORE removing it so the audit - // record can say whose hold was released and why. - var prevHolder, prevReason string - if h, gerr := store.GetHold(cmd.Context(), deployment, backupID); gerr == nil && h != nil { - prevHolder, prevReason = h.Holder, h.Reason + // Read the hold BEFORE removing it — both to stamp the audit record + // with whose hold was released and why, and to refuse a release of a + // hold that does not exist. The storage-layer RemoveHold is + // deliberately idempotent, so without this check a typo'd backup ID + // printed "✓ Hold released" with exit 0 while the real hold silently + // kept blocking retention — a false success on the legal-hold path. + h, gerr := store.GetHold(cmd.Context(), deployment, backupID) + switch { + case errors.Is(gerr, storage.ErrNotFound): + return output.NewError("notfound.hold", + fmt.Sprintf("hold remove: no hold exists on %s/%s", deployment, backupID)). + WithSuggestion(&output.Suggestion{ + Human: "check the exact backup ID with `pg_hardstorage hold list " + deployment + "`", + Command: "pg_hardstorage hold list " + deployment + " --repo " + repoURL, + }) + case gerr != nil: + return output.NewError("hold.read_failed", + fmt.Sprintf("hold remove: read hold %s/%s: %v", deployment, backupID, gerr)).Wrap(gerr) } + prevHolder, prevReason := h.Holder, h.Reason if err := store.RemoveHold(cmd.Context(), deployment, backupID); err != nil { return output.NewError("hold.remove_failed", fmt.Sprintf("hold remove: %v", err)).Wrap(err) diff --git a/internal/cli/hold_extern_test.go b/internal/cli/hold_extern_test.go index 79760e7..cbc527e 100644 --- a/internal/cli/hold_extern_test.go +++ b/internal/cli/hold_extern_test.go @@ -67,14 +67,21 @@ func TestHold_Remove_RequiresYes(t *testing.T) { } } -func TestHold_Remove_Idempotent(t *testing.T) { +func TestHold_Remove_AbsentHoldFails(t *testing.T) { w := newReadWorld(t) id := commitVerifiableBackup(t, w, "db1", 0, []byte("body")) - // No hold placed; removing should still succeed (idempotent). - _, _, exit := runCLI(t, "hold", "remove", "db1", id, + // No hold placed; removal must FAIL with notfound.hold. The old + // contract (exit 0, "✓ Hold released") was a false success on the + // legal-hold path — a typo'd backup ID confirmed a release while + // the real hold silently kept blocking retention. Idempotency + // lives at the storage layer, not in the operator-facing verb. + stdout, stderr, exit := runCLI(t, "hold", "remove", "db1", id, "--repo", w.repoURL, "--yes", "-o", "json") - if exit != int(output.ExitOK) { - t.Errorf("remove of absent hold should be idempotent (exit 0); got %d", exit) + if exit != int(output.ExitNotFound) { + t.Errorf("remove of absent hold: exit = %d, want %d (notfound)", exit, output.ExitNotFound) + } + if !strings.Contains(stdout+stderr, "notfound.hold") { + t.Errorf("expected notfound.hold in output:\n%s%s", stdout, stderr) } } diff --git a/internal/cli/hold_remove_notfound_test.go b/internal/cli/hold_remove_notfound_test.go new file mode 100644 index 0000000..c39cc5e --- /dev/null +++ b/internal/cli/hold_remove_notfound_test.go @@ -0,0 +1,30 @@ +package cli_test + +import ( + "strings" + "testing" + + "github.com/cybertec-postgresql/pg_hardstorage/internal/output" +) + +// Regression: `hold remove --yes` on a hold that never existed printed +// "✓ Hold released" with exit 0 (the storage layer's RemoveHold is +// idempotent) — a false success on the legal-hold path: typo one +// character of the backup ID and the real hold silently keeps blocking +// retention. It must fail with notfound.hold. +func TestHoldRemove_NonexistentHoldFails(t *testing.T) { + repoURL := "file://" + t.TempDir() + "/repo" + if _, _, exit := runCLI(t, "repo", "init", repoURL); exit != int(output.ExitOK) { + t.Fatalf("repo init failed") + } + out, errOut, exit := runCLI(t, + "hold", "remove", "db1", "db1.full.20990101T000000Z.dead", + "--repo", repoURL, "--yes", "-o", "json") + combined := out + errOut + if exit == 0 { + t.Fatalf("hold remove on nonexistent hold exited 0 (false success):\n%s", combined) + } + if !strings.Contains(combined, "notfound.hold") { + t.Errorf("expected notfound.hold, got exit=%d:\n%s", exit, combined) + } +} diff --git a/internal/cli/json_shape_test.go b/internal/cli/json_shape_test.go new file mode 100644 index 0000000..9d8451c --- /dev/null +++ b/internal/cli/json_shape_test.go @@ -0,0 +1,56 @@ +package cli + +import ( + "encoding/json" + "testing" + + "github.com/cybertec-postgresql/pg_hardstorage/internal/backup" +) + +// Regression: backupCompareBody used to wrap ComparisonResult under a +// nested "result" key, so consumers saw the payload at +// `.result.result.*` — double-nested relative to every sibling command. +// The fields must inline next to "deployment". +func TestBackupCompareBody_JSONNotDoubleNested(t *testing.T) { + b := backupCompareBody{ + Deployment: "db1", + ComparisonResult: &backup.ComparisonResult{}, + } + raw, err := json.Marshal(b) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, nested := m["result"]; nested { + t.Errorf("compare body still nests under \"result\": %s", raw) + } + if _, ok := m["deployment"]; !ok { + t.Errorf("compare body lost \"deployment\": %s", raw) + } +} + +// Regression: an empty deployment produced `"backups": null` (nil slice) +// — breaking every consumer that iterates `.result.backups[]`. The empty +// case must marshal as []. +func TestListBody_EmptyBackupsIsArray(t *testing.T) { + body := listBody{Deployment: "db1", Backups: []backupSummary{}} + raw, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m map[string]any + if err := json.Unmarshal(raw, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + arr, ok := m["backups"].([]any) + if !ok || arr == nil { + t.Errorf("backups = %v (%T), want empty JSON array", m["backups"], m["backups"]) + } +} + +// (Regression for the third inconvenience fix lives in +// hold_remove_notfound_test.go — kept separate: it needs the CLI +// runner + a live repo fixture.) diff --git a/internal/cli/list.go b/internal/cli/list.go index 55fea99..ef4f369 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -132,6 +132,12 @@ func runList(cmd *cobra.Command, deployment, repoURL string, includeDeleted, onl sort.Slice(summaries, func(i, j int) bool { return summaries[i].StoppedAt.After(summaries[j].StoppedAt) }) + // A nil slice marshals as JSON null, and `"backups": null` breaks + // every consumer iterating `.result.backups[]` — precisely on the + // empty-deployment case scripts probe first. Always emit []. + if summaries == nil { + summaries = []backupSummary{} + } body := listBody{ Deployment: deployment,