Skip to content

Commit af8c279

Browse files
committed
backup: #904 v15 - two codex P2 v14 fixes
## Codex P2 v14 (1/2): mismatch cleanup must not delete directories v12 added os.Remove(cfg.outputPath) on the self-test-mismatch branch of writeAndPublish to keep the sidecar+FSM internally consistent. That removal was unconditional. If --output names a directory (an operator typo — e.g. forgot the .fsm filename and passed a directory instead), the normal publish path would have failed at os.Rename, but the mismatch cleanup branch would silently call os.Remove on the directory and, for an empty one, delete it. Destructive behavior specific to the mismatch path. Fix: new helper removeStaleOutputFSM(outputPath, logger) performs an os.Lstat first and only proceeds with os.Remove when info.Mode() reports IsRegular. Non-regular files (directory, symlink, named pipe, etc.) are logged at warn level and left alone. Stat errors other than ErrNotExist are also warn-and-continue. Pinned by TestCLISelfTestMismatchSkipsDirectoryAtOutputPath: pre- places an empty directory at --output, drives a self-test mismatch via the corruption seam, asserts publishErr == errSelfTestMismatch AND the directory is still present + still a directory. ## Codex P2 v14 (2/2): classify corrupt manifests as data failures readInputManifest returns backup.ErrInvalidManifest (invalid JSON or schema-invariant violation) and backup.ErrUnsupportedFormatVersion (format_version unknown) when the input MANIFEST.json is broken. Neither sentinel was mapped in run(), so the CLI exited 1 — treating a broken dump tree as an operator-flag error and breaking runbook recovery paths that branch on exit status to triage corrupt-input vs operator-typo. Fix: extracted the error→exit-code mapping into classifyEncodeError and added ErrInvalidManifest + ErrUnsupportedFormatVersion to the exit-2 set. Switch-with-multiple-cases form keeps the function under nestif/cyclop. Pinned by TestCLIInvalidManifestExitsTwo (subtests: invalid JSON body; unsupported format_version). Both subtests assert run() exits exitDataErr. ## Caller audit per CLAUDE.md semantic-change rule - writeAndPublish self-test mismatch branch: os.Remove call replaced with removeStaleOutputFSM. Sole caller is encodeOne; the publishErr contract is unchanged (still errSelfTestMismatch); on-disk state changes ONLY when --output was a non-regular file, in which case the prior behavior was destructive AND wrong. No legitimate caller is impacted. - run() error classification: extracted to classifyEncodeError. Sole caller is run(). Two additional sentinels now classify as exit-2 (ErrInvalidManifest, ErrUnsupportedFormatVersion); all prior exit-2 sentinels still classify as exit-2; exit-1 paths are strictly a subset of the prior set. No runbook that branches on exit-2 would regress; runbooks that branch on exit-1 will see corrupt-manifest cases move to exit-2 where they belong. ## Lint compliance run() had nestif complexity 5 (six sequential errors.Is branches). classifyEncodeError uses a switch with multiple errors.Is cases per arm, which drops the complexity score below the linter bound. Tests + lint green.
1 parent 71ecc12 commit af8c279

2 files changed

Lines changed: 190 additions & 28 deletions

File tree

cmd/elastickv-snapshot-encode/main.go

Lines changed: 70 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -81,29 +81,42 @@ func run(argv []string, logger *slog.Logger) (int, error) {
8181
return exitUserErr, err
8282
}
8383
if err := encodeOne(cfg, logger); err != nil {
84-
// Errors from the encoder layer that represent data constraints
85-
// (HLC ceiling regression, JSONL layout, self-test mismatch,
86-
// adapter rejecting input-tree contents) are exit 2; flag/path
87-
// errors are exit 1. Runbooks branch on exit status to triage
88-
// bad-dump-data vs operator typos, so this split is part of the
89-
// CLI contract (codex P2 v9 #904 added the adapter-data branch).
90-
if errors.Is(err, backup.ErrSelfTestLowerLastCommitTS) {
91-
return exitDataErr, err
92-
}
93-
if errors.Is(err, backup.ErrEncodeUnsupportedDynamoDBLayout) {
94-
return exitDataErr, err
95-
}
96-
if errors.Is(err, backup.ErrEncodeAdapterData) {
97-
return exitDataErr, err
98-
}
99-
if errors.Is(err, errSelfTestMismatch) {
100-
return exitDataErr, err
101-
}
102-
return exitUserErr, err
84+
return classifyEncodeError(err), err
10385
}
10486
return exitSuccess, nil
10587
}
10688

89+
// classifyEncodeError maps the encodeOne return value to a CLI exit
90+
// code. Data-correctness sentinels (HLC ceiling regression, JSONL
91+
// layout, adapter rejecting input-tree contents, self-test mismatch,
92+
// corrupt manifest) → exit 2; everything else → exit 1. Runbooks
93+
// branch on exit status to triage bad-dump-data vs operator typos,
94+
// so this mapping is part of the CLI contract.
95+
//
96+
// Sources of each sentinel:
97+
// - ErrSelfTestLowerLastCommitTS: CLI resolveLastCommitTS + library
98+
// validateEncodeOptionsData (codex P2 v2 #904)
99+
// - ErrEncodeUnsupportedDynamoDBLayout: validateEncodeOptionsData
100+
// (codex P2 v7 #904)
101+
// - ErrEncodeAdapterData: runAdapterEncoders mark on adapter
102+
// rejection (codex P2 v9 #904)
103+
// - errSelfTestMismatch: writeAndPublish self-test branch
104+
// - ErrInvalidManifest / ErrUnsupportedFormatVersion: readInputManifest
105+
// surfacing backup.ReadManifest sentinels (codex P2 v14 #904)
106+
func classifyEncodeError(err error) int {
107+
switch {
108+
case errors.Is(err, backup.ErrSelfTestLowerLastCommitTS),
109+
errors.Is(err, backup.ErrEncodeUnsupportedDynamoDBLayout),
110+
errors.Is(err, backup.ErrEncodeAdapterData),
111+
errors.Is(err, errSelfTestMismatch),
112+
errors.Is(err, backup.ErrInvalidManifest),
113+
errors.Is(err, backup.ErrUnsupportedFormatVersion):
114+
return exitDataErr
115+
default:
116+
return exitUserErr
117+
}
118+
}
119+
107120
func parseFlags(argv []string) (*config, error) {
108121
fs := flag.NewFlagSet("elastickv-snapshot-encode", flag.ContinueOnError)
109122
fs.SetOutput(io.Discard)
@@ -312,6 +325,30 @@ func buildEncodeOptions(cfg *config, effectiveTS uint64, manifest backup.Manifes
312325
return encodeOpts
313326
}
314327

328+
// removeStaleOutputFSM removes outputPath ONLY if it exists and is a
329+
// regular file. A directory or special-file at the path is left alone
330+
// (codex P2 v14 #904 — the prior unconditional os.Remove would have
331+
// deleted an empty directory the operator passed in error to --output).
332+
// Errors other than ErrNotExist are downgraded to warn-and-continue so
333+
// the caller's primary mismatch error remains the dominant signal.
334+
func removeStaleOutputFSM(outputPath string, logger *slog.Logger) {
335+
info, err := os.Lstat(outputPath)
336+
if err != nil {
337+
if !errors.Is(err, os.ErrNotExist) {
338+
logger.Warn("stat stale .fsm on self-test mismatch", "err", err)
339+
}
340+
return
341+
}
342+
if !info.Mode().IsRegular() {
343+
logger.Warn("skip stale .fsm cleanup: --output is not a regular file",
344+
"path", outputPath, "mode", info.Mode())
345+
return
346+
}
347+
if rerr := os.Remove(outputPath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) {
348+
logger.Warn("remove stale .fsm on self-test mismatch", "err", rerr)
349+
}
350+
}
351+
315352
// writeAndPublish writes the .fsm to a temp path, runs the optional
316353
// self-test via EncodeSnapshot, and renames temp → output on success.
317354
// On self-test failure: writes mismatch.txt, removes any stale
@@ -338,15 +375,20 @@ func writeAndPublish(cfg *config, encodeOpts backup.EncodeOptions, mismatchPath
338375
logger.Warn("write mismatch.txt", "err", werr)
339376
}
340377
// Remove the stale <output>.fsm if one exists from a prior
341-
// successful run. encodeOne is about to write a fresh
342-
// <output>.encode_info.json with self_test.matched=false and
343-
// a NEW SHA pointing to the unpublished temp snapshot; leaving
344-
// the old bytes on disk would make the sidecar describe an
345-
// FSM that does not exist and violate the "self-test failure
346-
// leaves no restore-visible FSM" contract (codex P2 v10 #904).
347-
if rerr := os.Remove(cfg.outputPath); rerr != nil && !errors.Is(rerr, os.ErrNotExist) {
348-
logger.Warn("remove stale .fsm on self-test mismatch", "err", rerr)
349-
}
378+
// successful run AND is a regular file. encodeOne is about to
379+
// write a fresh <output>.encode_info.json with
380+
// self_test.matched=false and a NEW SHA pointing to the
381+
// unpublished temp snapshot; leaving old bytes on disk would
382+
// make the sidecar describe an FSM that does not exist and
383+
// violate the "self-test failure leaves no restore-visible
384+
// FSM" contract (codex P2 v10 #904).
385+
//
386+
// The mode-check guards against an --output that names a
387+
// directory (or any non-regular file): the normal publish
388+
// path would fail at os.Rename anyway, but the mismatch
389+
// cleanup must not destructively delete a directory the
390+
// operator passed in error (codex P2 v14 #904).
391+
removeStaleOutputFSM(cfg.outputPath, logger)
350392
return result, errors.Wrap(errSelfTestMismatch, "self-test diff (see "+mismatchPath+")")
351393
}
352394
if err := os.Rename(tempPath, cfg.outputPath); err != nil {

cmd/elastickv-snapshot-encode/main_test.go

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,126 @@ func readSidecar(t *testing.T, output string) backup.EncodeInfo {
304304
return info
305305
}
306306

307+
// TestCLISelfTestMismatchSkipsDirectoryAtOutputPath pins codex P2 v14
308+
// #904: the self-test-mismatch cleanup must NOT delete an --output
309+
// path that resolves to a directory (or any non-regular file). The
310+
// prior unconditional os.Remove(cfg.outputPath) would have wiped an
311+
// empty directory the operator passed in error.
312+
//
313+
// The fixture pre-creates an empty directory at the --output path,
314+
// drives a self-test mismatch, asserts publishErr == errSelfTestMismatch,
315+
// and asserts the directory is STILL PRESENT (the destructive
316+
// cleanup did not fire). The normal publish path would have failed
317+
// at os.Rename — this test pins the mismatch-cleanup-specific guard.
318+
func TestCLISelfTestMismatchSkipsDirectoryAtOutputPath(t *testing.T) {
319+
t.Parallel()
320+
rawIn := t.TempDir()
321+
writeSQSFixture(t, rawIn)
322+
emitMinimalManifest(t, rawIn, 7000)
323+
canonicalIn := canonicalizeInput(t, rawIn, 7000)
324+
325+
out := filepath.Join(t.TempDir(), "out.fsm")
326+
// Pre-create a directory at the --output path — an operator
327+
// typo, but the cleanup MUST NOT destructively remove it.
328+
if err := os.Mkdir(out, 0o755); err != nil {
329+
t.Fatalf("Mkdir: %v", err)
330+
}
331+
332+
scratchBase := t.TempDir()
333+
encodeOpts := backup.EncodeOptions{
334+
InputRoot: canonicalIn,
335+
Adapters: backup.AdapterSet{SQS: true},
336+
LastCommitTS: 7000,
337+
ManifestLastCommitTS: 7000,
338+
SelfTest: true,
339+
SelfTestDecodeOptions: backup.DecodeOptions{
340+
OutRoot: scratchBase,
341+
Adapters: backup.AdapterSet{SQS: true},
342+
},
343+
}
344+
encodeOpts.SetSelfTestCorruptHookForTest(func(f *os.File) {
345+
info, ferr := f.Stat()
346+
if ferr != nil {
347+
t.Fatalf("temp Stat: %v", ferr)
348+
}
349+
const headerSkip = 200
350+
if info.Size() <= headerSkip {
351+
t.Fatalf("temp file too small to corrupt: %d", info.Size())
352+
}
353+
buf := make([]byte, info.Size()-headerSkip)
354+
if _, rerr := f.ReadAt(buf, headerSkip); rerr != nil {
355+
t.Fatalf("ReadAt: %v", rerr)
356+
}
357+
for i := 0; i < len(buf); i += 13 {
358+
buf[i] ^= 0xFF
359+
}
360+
if _, werr := f.WriteAt(buf, headerSkip); werr != nil {
361+
t.Fatalf("WriteAt: %v", werr)
362+
}
363+
})
364+
365+
cfg := &config{
366+
inputPath: canonicalIn,
367+
outputPath: out,
368+
adapters: backup.AdapterSet{SQS: true},
369+
selfTest: true,
370+
}
371+
mismatchPath := out + ".mismatch.txt"
372+
373+
_, publishErr := writeAndPublish(cfg, encodeOpts, mismatchPath, quietLogger())
374+
if !errors.Is(publishErr, errSelfTestMismatch) {
375+
t.Fatalf("publishErr = %v, want errSelfTestMismatch", publishErr)
376+
}
377+
info, statErr := os.Stat(out)
378+
if statErr != nil {
379+
t.Fatalf("output path missing after mismatch (codex P2 v14 destructive cleanup regression): %v", statErr)
380+
}
381+
if !info.IsDir() {
382+
t.Errorf("output mode = %s; expected the pre-placed directory to be preserved", info.Mode())
383+
}
384+
}
385+
386+
// TestCLIInvalidManifestExitsTwo pins codex P2 v14 #904: a malformed
387+
// MANIFEST.json (invalid JSON or unsupported format_version) surfaces
388+
// backup.ErrInvalidManifest / backup.ErrUnsupportedFormatVersion from
389+
// readInputManifest, and the CLI MUST map both to exit 2
390+
// (data-correctness). Treating a broken manifest as exit 1 misroutes
391+
// runbook recovery for corrupt-dump scenarios.
392+
func TestCLIInvalidManifestExitsTwo(t *testing.T) {
393+
t.Parallel()
394+
t.Run("invalid JSON body", func(t *testing.T) {
395+
t.Parallel()
396+
in := t.TempDir()
397+
if err := os.WriteFile(filepath.Join(in, "MANIFEST.json"), []byte("{not json"), 0o600); err != nil {
398+
t.Fatalf("WriteFile: %v", err)
399+
}
400+
out := filepath.Join(t.TempDir(), "out.fsm")
401+
code, err := run([]string{"--input", in, "--output", out}, quietLogger())
402+
if err == nil {
403+
t.Fatalf("run succeeded; want manifest parse error")
404+
}
405+
if code != exitDataErr {
406+
t.Errorf("exit code = %d, want %d (invalid manifest is data-correctness)", code, exitDataErr)
407+
}
408+
})
409+
t.Run("unsupported format_version", func(t *testing.T) {
410+
t.Parallel()
411+
in := t.TempDir()
412+
if err := os.WriteFile(filepath.Join(in, "MANIFEST.json"),
413+
[]byte(`{"format_version":99,"last_commit_ts":1}`), 0o600); err != nil {
414+
t.Fatalf("WriteFile: %v", err)
415+
}
416+
out := filepath.Join(t.TempDir(), "out.fsm")
417+
code, err := run([]string{"--input", in, "--output", out}, quietLogger())
418+
if err == nil {
419+
t.Fatalf("run succeeded; want unsupported format_version")
420+
}
421+
if code != exitDataErr {
422+
t.Errorf("exit code = %d, want %d (unsupported manifest format_version is data-correctness)", code, exitDataErr)
423+
}
424+
})
425+
}
426+
307427
// TestCLIWriteAndPublishRemovesStaleFSMOnSelfTestMismatch pins codex
308428
// P2 v10 #904: when a prior successful run left an <output>.fsm on
309429
// disk and a new --self-test invocation produces a mismatch,

0 commit comments

Comments
 (0)