Refuse to file an album the journal cannot record
A failed journal write was printed as a warning and the run carried on. That leaves an album sitting in the archive untracked: library verify and library audit skip past it because they work from the journal, and a later run has no way to know the album is already done, so it offers to file it again. The journal is now proved writable before the first file is copied, by saving it unchanged, and an album is refused outright if that fails. record returns an error instead of printing and continuing, so a write that fails anyway is reported rather than swallowed, and the library-failure path reports both problems when the follow-up record fails as well. Closes 8cc6ff94cc0f27306659a2d7a967a9003b6a3a3b
diff --git a/internal/cli/cli.go b/internal/cli/cli.go
@@ -495,11 +495,10 @@ func (a *App) journal() (*state.Journal, error) {
return j, nil
}
-func (a *App) record(p plan.Plan, phase state.Phase, issues []string, placeholder, libraryBuilt bool) {
+func (a *App) record(p plan.Plan, phase state.Phase, issues []string, placeholder, libraryBuilt bool) error {
j, err := a.journal()
if err != nil {
- fmt.Fprintf(a.Err, "warning: %v\n", err)
- return
+ return fmt.Errorf("opening the journal to record %q: %w", p.AlbumName, err)
}
j.Put(state.Record{
Source: p.Album.Dir,
@@ -521,8 +520,27 @@ func (a *App) record(p plan.Plan, phase state.Phase, issues []string, placeholde
}
}
if err := j.Save(); err != nil {
- fmt.Fprintf(a.Err, "warning: could not save journal: %v\n", err)
+ return fmt.Errorf("recording %q in the journal: %w", p.AlbumName, err)
+ }
+ return nil
+}
+
+// journalReady proves the journal can be written before anything is filed.
+//
+// An album that files and then cannot be recorded is worse than one that never
+// files: it sits in the archive untracked, so library verify and library audit
+// skip past it and a later run offers to file it a second time. Writing the
+// journal unchanged is the cheapest way to exercise the exact write that will
+// matter later, and it happens before the first file is copied.
+func (a *App) journalReady() error {
+ j, err := a.journal()
+ if err != nil {
+ return err
}
+ if err := j.Save(); err != nil {
+ return fmt.Errorf("journal at %s cannot be written: %w", filepath.Join(a.Cfg.StateDir, "state.json"), err)
+ }
+ return nil
}
func (a *App) forget(p plan.Plan) {
diff --git a/internal/cli/dryrun_test.go b/internal/cli/dryrun_test.go
@@ -2,8 +2,11 @@ package cli
import (
"bytes"
+ "context"
+ "intake/internal/plan"
"os"
"path/filepath"
+ "strings"
"testing"
"intake/internal/artist"
@@ -53,3 +56,33 @@ func TestAliasesAreWrittenNormally(t *testing.T) {
}
_ = filepath.Join
}
+
+// An album that files and then cannot be recorded is worse than one that never
+// files: it lands in the archive untracked, so verify and audit skip it and a
+// later run offers to file it again. The journal is proved writable before the
+// first byte is copied, and that has to happen before anything is created.
+func TestFilingRefusesWhenTheJournalCannotBeWritten(t *testing.T) {
+ root := t.TempDir()
+ // A file where the state directory needs to be, so creating it fails.
+ blocker := filepath.Join(root, "blocked")
+ if err := os.WriteFile(blocker, []byte("not a directory"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+
+ a := &App{
+ Cfg: config.Config{Root: root, StateDir: filepath.Join(blocker, "state")},
+ Out: &bytes.Buffer{},
+ Err: &bytes.Buffer{},
+ }
+ archive := filepath.Join(root, "archive", "an-artist", "an-album")
+ _, err := a.FilePlan(context.Background(), plan.Plan{ArchiveDir: archive}, coverChoice{}, "")
+ if err == nil {
+ t.Fatal("filing should have been refused")
+ }
+ if !strings.Contains(err.Error(), "journal") {
+ t.Errorf("the refusal should name the journal: %v", err)
+ }
+ if _, statErr := os.Stat(filepath.Join(root, "archive")); !os.IsNotExist(statErr) {
+ t.Error("the archive was created even though the journal could not be written")
+ }
+}
diff --git a/internal/cli/file.go b/internal/cli/file.go
@@ -2,6 +2,7 @@ package cli
import (
"context"
+ "errors"
"fmt"
"os"
"path/filepath"
@@ -32,6 +33,12 @@ func (a *App) FilePlan(ctx context.Context, p plan.Plan, cover coverChoice, coll
res.ArchiveDir = p.ArchiveDir
res.LibraryDir = p.LibraryDir
+ // Checked first, so an unwritable journal refuses the album rather than
+ // filing it and then discovering it cannot be recorded.
+ if err := a.journalReady(); err != nil {
+ return res, fmt.Errorf("refusing to file: %w", err)
+ }
+
out, err := execute.Run(ctx, p, execute.Options{
KeepStaging: a.Cfg.KeepStaging,
VerifyDecode: a.Cfg.VerifyDecode,
@@ -50,12 +57,20 @@ func (a *App) FilePlan(ctx context.Context, p plan.Plan, cover coverChoice, coll
libraryBuilt := false
if !a.Cfg.NoLibrary {
if err := a.buildLibrary(ctx, p, cover); err != nil {
- a.record(p, state.PhaseArchived, nil, cover.Placeholder, false)
- return res, fmt.Errorf("archive is complete, but building the library copy failed: %w", err)
+ // Record what did land, so the archive is not left untracked, and
+ // report both problems if that fails too.
+ recErr := a.record(p, state.PhaseArchived, nil, cover.Placeholder, false)
+ libraryErr := fmt.Errorf("archive is complete, but building the library copy failed: %w", err)
+ if recErr != nil {
+ return res, errors.Join(libraryErr, recErr)
+ }
+ return res, libraryErr
}
libraryBuilt = true
}
- a.record(p, state.PhaseArchived, nil, cover.Placeholder, libraryBuilt)
+ if err := a.record(p, state.PhaseArchived, nil, cover.Placeholder, libraryBuilt); err != nil {
+ return res, err
+ }
res.Library = libraryBuilt
res.Placeholder = cover.Placeholder