Skip to content

Commit 6f231fe

Browse files
committed
backup: accept unknown manifest fields for same-major evolution (PR #712, round 3)
Codex P1 #205 (round 2): DisallowUnknownFields broke the documented same-major minor-evolution path. A newer producer that adds an optional field at the same format_version turns older readers into hard read failures during mixed-version operation, even though both nodes are on the "same major" by contract. The format_version field IS the breaking-change signal. Major bumps are gated by ErrUnsupportedFormatVersion; same-major minor evolution must silently tolerate optional new fields. Removed dec.DisallowUnknownFields() from ReadManifest. Replaced TestReadManifest_RejectsUnknownFields with TestReadManifest_AcceptsUnknownFieldsForSameMajorMinorEvolution. The trailing-bytes guard (dec.More() check) is preserved -- it catches concatenation bugs which is a different concern from optional-field tolerance.
1 parent e4cd7d8 commit 6f231fe

2 files changed

Lines changed: 24 additions & 8 deletions

File tree

internal/backup/manifest.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,17 @@ func WriteManifest(w io.Writer, m Manifest) error {
202202
func ReadManifest(r io.Reader) (Manifest, error) {
203203
var m Manifest
204204
dec := json.NewDecoder(r)
205-
dec.DisallowUnknownFields() // surface format drift loudly
205+
// We intentionally do NOT call DisallowUnknownFields here.
206+
// The format-version contract (Codex P1, follow-up) is:
207+
// - format_version > CurrentFormatVersion -> hard refuse
208+
// (the major break signal)
209+
// - format_version == CurrentFormatVersion AND extra unknown
210+
// fields appear -> a newer minor version added them; the
211+
// older reader silently ignores. That's the documented
212+
// same-major minor-evolution path.
213+
// Rejecting unknown fields outright would turn every minor
214+
// optional-field addition into a hard read failure during
215+
// mixed-version operation.
206216
if err := dec.Decode(&m); err != nil {
207217
return Manifest{}, errors.Wrap(ErrInvalidManifest, err.Error())
208218
}

internal/backup/manifest_test.go

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,13 @@ func TestReadManifest_RejectsZeroFormatVersion(t *testing.T) {
141141
}
142142
}
143143

144-
func TestReadManifest_RejectsUnknownFields(t *testing.T) {
144+
func TestReadManifest_AcceptsUnknownFieldsForSameMajorMinorEvolution(t *testing.T) {
145145
t.Parallel()
146-
// Format drift safety: an unknown field surfaces loudly rather than
147-
// being silently ignored.
146+
// Same-major minor evolution: a newer producer adds an optional
147+
// field; older readers must silently ignore it rather than fail
148+
// the read. Codex P1 #205 (round 2) caught the earlier
149+
// DisallowUnknownFields strictness which broke the documented
150+
// same-major compatibility model.
148151
body := `{
149152
"format_version": 1,
150153
"phase": "phase0-snapshot-decode",
@@ -158,11 +161,14 @@ func TestReadManifest_RejectsUnknownFields(t *testing.T) {
158161
"s3_meta_suffix": ".elastickv-meta.json",
159162
"s3_collision_strategy": "leaf-data-suffix",
160163
"dynamodb_layout": "per-item",
161-
"unknown_field": "ahoy"
164+
"future_optional_field": "added in v1.minor"
162165
}`
163-
_, err := ReadManifest(strings.NewReader(body))
164-
if !errors.Is(err, ErrInvalidManifest) {
165-
t.Fatalf("err=%v want ErrInvalidManifest", err)
166+
got, err := ReadManifest(strings.NewReader(body))
167+
if err != nil {
168+
t.Fatalf("unknown optional field must be silently accepted: %v", err)
169+
}
170+
if got.FormatVersion != 1 {
171+
t.Fatalf("format_version = %d", got.FormatVersion)
166172
}
167173
}
168174

0 commit comments

Comments
 (0)