amadan
release: separate store errors from manifest decode/validate errors in program check
PreviousProgram now distinguishes between three error cases: - Missing manifest (os.ErrNotExist): skip (in-flight release) - Store error: return error (fail closed) - Decode or validation error: skip with log.Printf (corrupt manifest) This prevents a single corrupt manifest from permanently blocking all future ships. Changed to call GetQualified directly and decode/validate manifests inline, instead of relying on GetManifest which conflates store errors with parse errors. Added TestPreviousProgramSkipsAnInFlightRelease (no manifest yet) and TestPreviousProgramSkipsACorruptManifest (unparseable manifest). Both verify that older valid manifests are still found.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2 files changed,
+75
−5
internal/release/program.go+28 −5internal/release/program_test.go+47 −0
diff --git a/internal/release/program.go b/internal/release/program.go| index 4bc42d714..5f274fbfe 100644 |
| --- a/internal/release/program.go |
| +++ b/internal/release/program.go |
| @@ -13,8 +13,10 @@ import ( |
| "bytes" |
| "context" |
| "debug/buildinfo" |
| + "encoding/json" |
| "errors" |
| "fmt" |
| + "log" |
| "os" |
| "sort" |
| "strings" |
| @@ -65,8 +67,9 @@ func ReleaseProgram(files map[string][]byte) (string, error) { |
| } |
| // PreviousProgram is the program of the newest release (among the 20 newest) |
| -// whose manifest declares one, and that release's version. It skips only missing |
| -// manifests (os.ErrNotExist), returning any other error to fail the check closed. |
| +// whose manifest declares one, and that release's version. It skips missing |
| +// manifests (os.ErrNotExist) and manifests that don't parse or validate |
| +// (logging each), returning store errors to fail the check closed. |
| func PreviousProgram(ctx context.Context, s Store, acct, app string) (string, string, error) { |
| vers, _, err := VersionCandidates(ctx, s, acct, app, 20) |
| if err != nil { |
| @@ -74,18 +77,38 @@ func PreviousProgram(ctx context.Context, s Store, acct, app string) (string, st |
| } |
| var best *Manifest |
| for _, v := range vers { |
| - m, err := GetManifest(ctx, s, acct, app, v) |
| + // Read the manifest using GetQualified, exactly as GetManifest does. |
| + raw, _, err := GetQualified(ctx, s, ManifestKey(acct, app, v)) |
| if err != nil { |
| if errors.Is(err, os.ErrNotExist) { |
| + // In-flight release with no manifest yet; skip it. |
| continue |
| } |
| + // Store error; fail closed. |
| return "", "", err |
| } |
| - if m == nil || m.Program == "" { |
| + |
| + // Decode and validate the manifest ourselves, separating parse errors |
| + // (which we skip) from store errors (which fail closed). |
| + var m Manifest |
| + if err := json.Unmarshal(raw, &m); err != nil { |
| + log.Printf("program check: manifest %s/%s: decode error: %v", app, v, err) |
| + continue |
| + } |
| + if err := validateManifest(&m); err != nil { |
| + log.Printf("program check: manifest %s/%s: validation error: %v", app, v, err) |
| + continue |
| + } |
| + if m.App != app || m.Version != v { |
| + log.Printf("program check: manifest %s/%s: declares wrong app/version", app, v) |
| + continue |
| + } |
| + |
| + if m.Program == "" { |
| continue |
| } |
| if best == nil || m.BuiltAt > best.BuiltAt { |
| - best = m |
| + best = &m |
| } |
| } |
| if best == nil { |
diff --git a/internal/release/program_test.go b/internal/release/program_test.go| index 6d3bac9e4..c39cfe04d 100644 |
| --- a/internal/release/program_test.go |
| +++ b/internal/release/program_test.go |
| @@ -159,3 +159,50 @@ func TestPreviousProgramFailsClosedOnStoreError(t *testing.T) { |
| t.Fatalf("CheckShipProgram = ProgramMismatchError, want other error: %v", checkErr) |
| } |
| } |
| + |
| +// An in-flight release with no manifest yet is skipped gracefully. |
| +func TestPreviousProgramSkipsAnInFlightRelease(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + console := tinyProgram(t, "example.com/console") |
| + |
| + // Ship v1 with console program. |
| + if err := shipWithProgram(t, s, "v1", console, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // Ship an artifact for v2 without a manifest (in-flight, manifest still being written). |
| + // Write the artifact file directly. |
| + artKey := ArtifactKey("bab", "console", "v2", adoptPlatform) |
| + if _, err := s.Put(context.Background(), artKey, console, ""); err != nil { |
| + t.Fatalf("put artifact: %v", err) |
| + } |
| + |
| + // PreviousProgram should skip v2 (no manifest) and return v1. |
| + prog, ver, err := PreviousProgram(context.Background(), s, "bab", "console") |
| + if err != nil || prog != "example.com/console" || ver != "v1" { |
| + t.Fatalf("PreviousProgram = %q %q %v, want example.com/console from v1", prog, ver, err) |
| + } |
| +} |
| + |
| +// A corrupt (unparseable) manifest is skipped, and an older valid one is found. |
| +func TestPreviousProgramSkipsACorruptManifest(t *testing.T) { |
| + s := NewDirStore(t.TempDir()) |
| + console := tinyProgram(t, "example.com/console") |
| + |
| + // Ship v1 with console program. |
| + if err := shipWithProgram(t, s, "v1", console, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + // Write a corrupt manifest for v2 (not valid JSON). |
| + manifestKey := ManifestKey("bab", "console", "v2") |
| + if _, err := s.Put(context.Background(), manifestKey, []byte("not json at all"), ""); err != nil { |
| + t.Fatalf("put corrupt manifest: %v", err) |
| + } |
| + |
| + // PreviousProgram should skip v2 (corrupt manifest) and return v1. |
| + prog, ver, err := PreviousProgram(context.Background(), s, "bab", "console") |
| + if err != nil || prog != "example.com/console" || ver != "v1" { |
| + t.Fatalf("PreviousProgram = %q %q %v, want example.com/console from v1", prog, ver, err) |
| + } |
| +} |