From 028ce45750e66505256b867ea0aac3067943dbb7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 12:29:25 +0000 Subject: [PATCH] runs: record a real changed-file count (v28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runs.file_count means different things per kind. A peer sync records the receiver-verified — i.e. transferred — count, so its no-ops already read as zero; a bucket push records transferred plus already-correct, and an index run records added plus modified plus unchanged, so their no-ops read as the whole volume. No column answered "did this run change anything?" across kinds, which left `runs --changes` and the TUI's `f` fold able to collapse only peer-sync no-ops — and in the reference household's steady state most rows are per-destination bucket pushes. v28 adds runs.changed_count: nullable with no default, so pre-migration history reads back as "unknown" rather than a fabricated zero. Purely additive — no existing column changes meaning and no run row is rewritten. Each driver populates it from what it already knows: rclone's transferred count for bucket pushes and restores, the manifest delta for the content-addressed and packed layouts, the receiver-verified paths for a peer sync (both sides), added + modified + missing for an index walk, the offloaded count for an offload, the applied rows for a durability pull, and the freshly recorded fingerprints for a remote-verify pass. kopia reports only its total file count, so it stays honestly unknown. store.Run.NoOp is now the one rule behind both `runs --changes` and the TUI fold — the recorded count where there is one, today's conservative file_count heuristic where there isn't — so the CLI and the TUI hide exactly the same rows. Closes #182 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QexD2njN4TGvyUSDdgi6qG --- agent/sync.go | 8 ++- cmd/squirrel/agent.go | 4 +- cmd/squirrel/polish_test.go | 29 +++++++--- cmd/squirrel/runs.go | 28 ++++------ design/friction-log.md | 10 +++- index/index.go | 9 +++- index/index_test.go | 56 +++++++++++++++++++ offload/offload.go | 9 ++-- store/migrate_v27_test.go | 6 ++- store/migrate_v28_test.go | 105 ++++++++++++++++++++++++++++++++++++ store/migrations.go | 47 +++++++++++++++- store/runs.go | 88 ++++++++++++++++++++++++------ store/runs_test.go | 83 ++++++++++++++++++++++++++++ store/schema.sql | 4 +- sync/content_addressed.go | 4 ++ sync/finish_changed_test.go | 104 +++++++++++++++++++++++++++++++++++ sync/handler.go | 7 ++- sync/node.go | 8 ++- sync/packed.go | 3 ++ sync/sync.go | 35 +++++++++++- sync/verify_remote.go | 9 +++- tui/runs.go | 15 ++---- tui/runs_polish_test.go | 69 ++++++++++++++++++++---- 23 files changed, 663 insertions(+), 77 deletions(-) create mode 100644 store/migrate_v28_test.go create mode 100644 sync/finish_changed_test.go diff --git a/agent/sync.go b/agent/sync.go index 83a2fda..8518322 100644 --- a/agent/sync.go +++ b/agent/sync.go @@ -1556,7 +1556,8 @@ func (r *peerSyncRouter) handleClose(w http.ResponseWriter, req *http.Request) { // already failing, and the run row's stuck-'running' state is the worse // outcome the caller surfaces, so there's nothing to return through. func (r *peerSyncRouter) finalizeFailedClose(ctx context.Context, runID int64, committed int, cause error) { - err := r.srv.store.FinishRun(ctx, runID, store.RunStatusFailed, cause.Error(), int64(committed)) + err := r.srv.store.FinishRunChanged(ctx, runID, store.RunStatusFailed, cause.Error(), + int64(committed), int64(committed)) if err == nil { return } @@ -1623,7 +1624,10 @@ func (r *peerSyncRouter) closeSession(ctx context.Context, sess *peerSession, st if finishStatus != store.RunStatusSuccess && finishStatus != store.RunStatusPartial && finishStatus != store.RunStatusFailed { finishStatus = store.RunStatusPartial } - if err := r.srv.store.FinishRun(ctx, sess.receiverRunID, finishStatus, "", int64(committed)); err != nil { + // committed is both counts on the receiver side: every row written + // here is a path whose content this session changed locally, so a + // session that committed nothing reads as the no-op it is (#182). + if err := r.srv.store.FinishRunChanged(ctx, sess.receiverRunID, finishStatus, "", int64(committed), int64(committed)); err != nil { return committed, fmt.Errorf("finish run: %w", err) } return committed, nil diff --git a/cmd/squirrel/agent.go b/cmd/squirrel/agent.go index f787923..3911b35 100644 --- a/cmd/squirrel/agent.go +++ b/cmd/squirrel/agent.go @@ -383,7 +383,9 @@ func finishDurabilityPullRun(ctx context.Context, s *store.Store, runID int64, v note := fmt.Sprintf("volume=%s peer=%s fetched=%d applied=%d dropped=%d rewinds=%d", volume, peer, rep.Fetched, rep.Applied, rep.Dropped, len(rep.Rewinds)) auditErr := s.AppendRunAudit(ctx, store.RunAuditEntry{RunID: runID, Transition: store.TransitionPullDurability, Note: note}) - finErr := s.FinishRun(ctx, runID, status, errMsg, int64(rep.Applied)) + // Applied is both counts: a pull that merged no durability rows changed + // nothing locally, so it folds out of the steady-state noise (#182). + finErr := s.FinishRunChanged(ctx, runID, status, errMsg, int64(rep.Applied), int64(rep.Applied)) return agent.DurabilityPullReport{ RunID: runID, Status: status, diff --git a/cmd/squirrel/polish_test.go b/cmd/squirrel/polish_test.go index 627b0ae..5904f2a 100644 --- a/cmd/squirrel/polish_test.go +++ b/cmd/squirrel/polish_test.go @@ -43,6 +43,16 @@ func TestRunIsInteresting(t *testing.T) { {"failed", store.Run{ID: 3, Status: store.RunStatusFailed}, true}, {"refused", store.Run{ID: 4, Status: store.RunStatusRefused}, true}, {"success with conflict", store.Run{ID: 7, Status: store.RunStatusSuccess, FileCount: 0}, true}, + // #182: a bucket push counts already-correct files in file_count, + // so only changed_count can tell the in-sync no-op from the push + // that actually moved content. + {"in-sync bucket push", store.Run{ID: 8, Status: store.RunStatusSuccess, FileCount: 42, + ChangedCount: sql.NullInt64{Int64: 0, Valid: true}}, false}, + {"bucket push that transferred", store.Run{ID: 9, Status: store.RunStatusSuccess, FileCount: 42, + ChangedCount: sql.NullInt64{Int64: 3, Valid: true}}, true}, + // Pre-v28 history has no changed count and keeps the conservative + // file_count rendering. + {"pre-v28 bucket push", store.Run{ID: 10, Status: store.RunStatusSuccess, FileCount: 42}, true}, } for _, tc := range tests { if got := runIsInteresting(tc.r, conflicts); got != tc.want { @@ -54,20 +64,25 @@ func TestRunIsInteresting(t *testing.T) { // TestFilterRuns exercises both filters end to end, including that the // descending order is preserved. func TestFilterRuns(t *testing.T) { + changed := func(n int64) sql.NullInt64 { return sql.NullInt64{Int64: n, Valid: true} } runs := []store.Run{ - {ID: 5, Status: store.RunStatusSuccess, FileCount: 0}, // no-op - {ID: 4, Status: store.RunStatusSuccess, FileCount: 3}, // change - {ID: 3, Status: store.RunStatusFailed}, // failed - {ID: 2, Status: store.RunStatusRefused}, // refused - {ID: 1, Status: store.RunStatusSuccess, FileCount: 0}, // no-op + // A steady-state bucket push and an all-unchanged re-index: both + // consider the whole volume, both changed nothing (#182). + {ID: 7, Status: store.RunStatusSuccess, FileCount: 42, ChangedCount: changed(0)}, + {ID: 6, Status: store.RunStatusSuccess, FileCount: 42, ChangedCount: changed(2)}, // moved content + {ID: 5, Status: store.RunStatusSuccess, FileCount: 0}, // no-op + {ID: 4, Status: store.RunStatusSuccess, FileCount: 3}, // change + {ID: 3, Status: store.RunStatusFailed}, // failed + {ID: 2, Status: store.RunStatusRefused}, // refused + {ID: 1, Status: store.RunStatusSuccess, FileCount: 0}, // no-op } conflicts := map[int64]int{} if ids := runIDs(filterRuns(runs, conflicts, true, false)); !sameIDs(ids, []int64{3, 2}) { t.Fatalf("--failed ids = %v, want [3 2]", ids) } - if ids := runIDs(filterRuns(runs, conflicts, false, true)); !sameIDs(ids, []int64{4, 3, 2}) { - t.Fatalf("--changes ids = %v, want [4 3 2]", ids) + if ids := runIDs(filterRuns(runs, conflicts, false, true)); !sameIDs(ids, []int64{6, 4, 3, 2}) { + t.Fatalf("--changes ids = %v, want [6 4 3 2]", ids) } } diff --git a/cmd/squirrel/runs.go b/cmd/squirrel/runs.go index c3e3f70..a09da7d 100644 --- a/cmd/squirrel/runs.go +++ b/cmd/squirrel/runs.go @@ -194,16 +194,16 @@ func gatherRuns(cmd *cobra.Command, s *store.Store, volumeID *int64, limit int, // conflictCountsForFilter loads only the conflict counts the active filter // needs. --failed consults none — an attention-worthy run is unconditionally -// kept — so it returns an empty map. --changes needs them only for the -// ambiguous rows (a clean success that touched no files), so it counts just -// those rather than the whole unbounded fetch. +// kept — so it returns an empty map. --changes needs them only for the rows +// that would otherwise be hidden as no-ops, so it counts just those rather +// than the whole unbounded fetch. func conflictCountsForFilter(cmd *cobra.Command, s *store.Store, runs []store.Run, onlyFailed bool) (map[int64]int, error) { if onlyFailed { return map[int64]int{}, nil } var candidates []store.Run for _, r := range runs { - if r.Status == store.RunStatusSuccess && r.FileCount == 0 { + if r.NoOp() { candidates = append(candidates, r) } } @@ -238,22 +238,16 @@ func runNeedsAttention(status string) bool { } // runIsInteresting reports whether a run is worth showing under --changes: -// anything needing attention, still running, that touched files, or that -// resolved conflicts. A clean success that touched nothing is the routine +// anything needing attention, still running, that changed files, or that +// resolved conflicts. A clean success that changed nothing is the routine // no-op the filter hides. // -// runs.file_count conflates transferred with already-correct for bucket and -// index runs (it is the total files considered, not the delta), so an -// in-sync bucket sync or an all-unchanged re-index still shows here — the -// filter deliberately errs toward showing rather than risk hiding a run -// that moved content. It reliably folds peer-sync no-ops, whose file_count -// is the receiver-verified (i.e. transferred) count and so is zero when -// nothing moved. +// store.Run.NoOp owns the "changed nothing" rule so this filter and the +// TUI's `f` fold hide exactly the same rows: the run's recorded +// changed_count where it has one, and the conservative file_count +// heuristic for the pre-v28 history and the drivers that report no count. func runIsInteresting(r store.Run, conflicts map[int64]int) bool { - if r.Status != store.RunStatusSuccess { - return true - } - if r.FileCount > 0 { + if !r.NoOp() { return true } return conflicts[r.ID] > 0 diff --git a/design/friction-log.md b/design/friction-log.md index 732afee..b3d6cbb 100644 --- a/design/friction-log.md +++ b/design/friction-log.md @@ -201,7 +201,7 @@ On the nas, `sync photos laptop` (received from laptop) and the TUI — the destination column holds a peer name in both directions. The audit trail answers "what happened" but not "who initiated". -**F19 · S3 — steady-state run noise buries signal.** With compressed +**F19 · S3 — ~~steady-state run noise buries signal.~~ (filters + fold, #179; `runs.changed_count`, #182)** With compressed cadences the runs list is dominated by 0-file no-op rows (one per volume × destination per tick). Nothing distinguishes "checked, nothing to do" from "transferred nothing unexpectedly"; a day of real @@ -211,6 +211,14 @@ TUI-side folding of consecutive no-ops would restore the audit trail's readability. (The `runs` help text also still says "List index runs"; it lists every kind.) +#179 landed the filters and the fold, but both keyed on +`file_count == 0`, which only ever collapsed peer-sync no-ops — a bucket +push or an index run counts what it *considered*, so the bulk of the +noise stayed on screen. #182 added `runs.changed_count`, populated by +each driver from what it actually moved, and both surfaces now key on +that (falling back to the old heuristic for pre-v28 history, which stays +truthfully "unknown" rather than silently folding). + **F20 · S2 — ~~recovering a wrecked destination has no supported path.~~ (`squirrel destination reset` + empty-root guard, #176)** After the F12 bug era the packed-layout guard refused every further s3archive sync ("its history is not packed … point the layout at a diff --git a/index/index.go b/index/index.go index d219c5f..bd0cbb6 100644 --- a/index/index.go +++ b/index/index.go @@ -293,6 +293,12 @@ func (i *indexer) beginRun() error { // and the per-file error count: a fatal failure yields 'failed', per-file // errors with a clean walk yield 'partial', and an entirely clean run yields // 'success'. +// +// file_count is everything the walk considered; changed_count narrows that +// to the observations this run actually recorded — a new path, a path whose +// content was superseded, or a path that went missing. An all-unchanged +// re-index therefore reads as the no-op it is, instead of reporting the +// whole volume (#182). func (i *indexer) finishRun(report *Report, fatalErr error) { report.RunID = i.runID if i.opts.DryRun || i.runID == 0 { @@ -300,7 +306,8 @@ func (i *indexer) finishRun(report *Report, fatalErr error) { } status, errMsg := runStatus(report, fatalErr) fileCount := int64(report.Added + report.Modified + report.Unchanged) - if err := i.store.FinishRun(i.ctx, i.runID, status, errMsg, fileCount); err != nil { + changed := int64(report.Added + report.Modified + report.Missing) + if err := i.store.FinishRunChanged(i.ctx, i.runID, status, errMsg, fileCount, changed); err != nil { // Surface as a per-run error rather than swallowing silently. The // outer caller has already accepted a report and a fatal error (if // any); we can only append. diff --git a/index/index_test.go b/index/index_test.go index 1b78672..595eb1d 100644 --- a/index/index_test.go +++ b/index/index_test.go @@ -564,6 +564,62 @@ func TestIndexRecordsRuns(t *testing.T) { } } +// TestIndexRecordsChangedCount pins the v28 column for index runs (#182): +// the first walk added a file and the second changed nothing, so the two +// rows carry the same file_count but different changed counts — and only +// the second folds out of `runs --changes` and the TUI's fold. A third walk +// after an edit and a delete counts both as changes. +func TestIndexRecordsChangedCount(t *testing.T) { + root := t.TempDir() + writeFile(t, filepath.Join(root, "a.txt"), "hello") + writeFile(t, filepath.Join(root, "b.txt"), "world") + + s := setupStore(t) + ctx := context.Background() + if _, err := Index(ctx, s, root, Options{}); err != nil { + t.Fatalf("first Index: %v", err) + } + if _, err := Index(ctx, s, root, Options{}); err != nil { + t.Fatalf("second Index: %v", err) + } + writeFile(t, filepath.Join(root, "a.txt"), "hello again") + if err := os.Remove(filepath.Join(root, "b.txt")); err != nil { + t.Fatalf("remove b.txt: %v", err) + } + if _, err := Index(ctx, s, root, Options{}); err != nil { + t.Fatalf("third Index: %v", err) + } + + absRoot, _ := filepath.Abs(root) + vol := volumeFor(t, s, absRoot) + runs, err := s.ListRuns(ctx, store.ListRunsOpts{VolumeID: &vol.ID}) + if err != nil { + t.Fatalf("ListRuns: %v", err) + } + if len(runs) != 3 { + t.Fatalf("got %d run rows, want 3: %+v", len(runs), runs) + } + // added a.txt + b.txt, nothing, then a.txt modified + b.txt missing. + wantChanged := []int64{2, 0, 2} + for i, r := range runs { + if !r.ChangedCount.Valid { + t.Fatalf("run %d changed_count is NULL, want %d", i, wantChanged[i]) + } + if r.ChangedCount.Int64 != wantChanged[i] { + t.Errorf("run %d changed_count = %d, want %d", i, r.ChangedCount.Int64, wantChanged[i]) + } + } + if runs[0].NoOp() || !runs[1].NoOp() || runs[2].NoOp() { + t.Errorf("NoOp() = %v, want [false true false]", + []bool{runs[0].NoOp(), runs[1].NoOp(), runs[2].NoOp()}) + } + // The all-unchanged walk still considered every file — the point of + // the new column is that file_count alone couldn't say it was a no-op. + if runs[1].FileCount != 2 { + t.Errorf("no-op run file_count = %d, want 2 (files considered)", runs[1].FileCount) + } +} + // TestIndexRecordsShallowFlag pins that the runs row carries the // Options.Shallow value the call was made with: shallow=true for the // fast (size, mtime) path and shallow=false for the rehash-everything diff --git a/offload/offload.go b/offload/offload.go index d3a8e04..d32788f 100644 --- a/offload/offload.go +++ b/offload/offload.go @@ -372,8 +372,10 @@ func beginRun(ctx context.Context, s *store.Store, volumeID int64, volumeName st // the fatal error, 'partial' when any per-file operation errored, and // 'success' otherwise (skipped files are decisions, so a run that only // skipped is still a success). file_count records how many files were -// actually offloaded. A failed terminal write is surfaced on the report -// — the per-file flips already committed individually and stand. +// actually offloaded, and so does changed_count — an offloaded row is a +// files-row flip, and a run that only skipped changed nothing (#182). A +// failed terminal write is surfaced on the report — the per-file flips +// already committed individually and stand. func finishRun(ctx context.Context, s *store.Store, runID int64, report *Report, fatalErr error) { status, errMsg := store.RunStatusSuccess, "" switch { @@ -382,7 +384,8 @@ func finishRun(ctx context.Context, s *store.Store, runID int64, report *Report, case report.Errors > 0: status = store.RunStatusPartial } - if err := s.FinishRun(ctx, runID, status, errMsg, int64(report.Offloaded)); err != nil { + offloaded := int64(report.Offloaded) + if err := s.FinishRunChanged(ctx, runID, status, errMsg, offloaded, offloaded); err != nil { report.FinishErr = fmt.Errorf("finish offload run %d: %w", runID, err) } } diff --git a/store/migrate_v27_test.go b/store/migrate_v27_test.go index 856415a..c72fe44 100644 --- a/store/migrate_v27_test.go +++ b/store/migrate_v27_test.go @@ -125,8 +125,10 @@ func TestMigrateV26ToV27PreservesEveryRow(t *testing.T) { } want := before if table == "schema_version" { - // The migration records v27 in the table it just rebuilt. - want++ + // The v27 migration records its own version in the table it + // just rebuilt, and Open carries on to SchemaVersion — one + // row per step from the v26 fixture. + want += int64(SchemaVersion - 26) } if after := countsAfter[table]; after != want { t.Errorf("%s rows = %d after migration, want %d", table, after, want) diff --git a/store/migrate_v28_test.go b/store/migrate_v28_test.go new file mode 100644 index 0000000..c35077a --- /dev/null +++ b/store/migrate_v28_test.go @@ -0,0 +1,105 @@ +package store + +import ( + "context" + "database/sql" + "path/filepath" + "testing" +) + +// TestMigrateToV28LeavesHistoryUnknown builds a pre-v28 database holding +// the two run shapes #182 is about — a bucket push whose file_count counts +// the whole volume, and a peer sync whose file_count is already the +// transferred count — and migrates it forward. +// +// The point of the nullable column is that history stays truthful: neither +// row gains a fabricated zero, both read back as "unknown", and Run.NoOp +// therefore keeps rendering them exactly as it did before the migration. +func TestMigrateToV28LeavesHistoryUnknown(t *testing.T) { + dsn := filepath.Join(t.TempDir(), "test.db") + rawDB, err := sql.Open("sqlite", dsn) + if err != nil { + t.Fatalf("raw sql.Open: %v", err) + } + ddl := []string{ + `CREATE TABLE schema_version (version INTEGER NOT NULL PRIMARY KEY)`, + `CREATE TABLE volumes (id INTEGER PRIMARY KEY, name TEXT NOT NULL UNIQUE, path TEXT NOT NULL)`, + `CREATE TABLE nodes (id INTEGER PRIMARY KEY, name TEXT NOT NULL UNIQUE, endpoint TEXT, public_key_fingerprint TEXT)`, + minimalRunsDDL, + // Present since v11, so a v24 database has it; the fixture needs + // it because FinishRun writes the run's transition here. + `CREATE TABLE runs_audit ( + id INTEGER PRIMARY KEY, + run_id INTEGER NOT NULL REFERENCES runs(id), + transition TEXT NOT NULL, + operator TEXT, + at_ns INTEGER NOT NULL, + note TEXT + )`, + `INSERT INTO schema_version (version) VALUES (24)`, + `INSERT INTO volumes (id, name, path) VALUES (1, 'v', '/v')`, + `INSERT INTO nodes (id, name) VALUES (1, 'self')`, + // An in-sync bucket push: nothing moved, but file_count counted + // every already-correct file. + `INSERT INTO runs (id, kind, volume_id, destination, started_at_ns, ended_at_ns, status, file_count) + VALUES (1, 'sync', 1, 'bucket', 100, 200, 'success', 42)`, + // A peer-sync no-op: file_count is the receiver-verified count, so + // it was already zero. + `INSERT INTO runs (id, kind, volume_id, destination, started_at_ns, ended_at_ns, status, file_count) + VALUES (2, 'sync', 1, 'htpc', 110, 210, 'success', 0)`, + } + for _, q := range ddl { + if _, err := rawDB.Exec(q); err != nil { + t.Fatalf("pre-v28 DDL %q: %v", q, err) + } + } + rawDB.Close() + + s, err := OpenWithOptions(dsn, OpenOptions{DisablePreMigrationBackup: true}) + if err != nil { + t.Fatalf("Open (migrates to v%d): %v", SchemaVersion, err) + } + defer s.Close() + ctx := context.Background() + if v, _ := s.CurrentSchemaVersion(ctx); v != SchemaVersion { + t.Fatalf("schema_version = %d, want %d", v, SchemaVersion) + } + + runs, err := s.ListRuns(ctx, ListRunsOpts{}) + if err != nil { + t.Fatalf("ListRuns: %v", err) + } + if len(runs) != 2 { + t.Fatalf("runs after migration = %d, want 2", len(runs)) + } + for _, r := range runs { + if r.ChangedCount.Valid { + t.Errorf("run %d changed_count = %+v, want NULL — history must not gain a fabricated count", + r.ID, r.ChangedCount) + } + } + // The fallback keeps each row's pre-migration rendering: the bucket + // push stays visible (conservative), the peer-sync no-op still folds. + if runs[0].NoOp() { + t.Error("pre-v28 bucket push should stay visible, not fold on a guess") + } + if !runs[1].NoOp() { + t.Error("pre-v28 peer-sync no-op should still fold") + } + + // New runs on the migrated database record the real count. + runID, err := s.BeginRun(ctx, RunKindSync, 1, "bucket", false) + if err != nil { + t.Fatalf("BeginRun: %v", err) + } + if err := s.FinishRunChanged(ctx, runID, RunStatusSuccess, "", 42, 0); err != nil { + t.Fatalf("FinishRunChanged: %v", err) + } + fresh, err := s.GetRun(ctx, runID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if !fresh.NoOp() { + t.Errorf("in-sync bucket push should fold after v28: %+v", fresh.ChangedCount) + } +} diff --git a/store/migrations.go b/store/migrations.go index 81e7dc4..95c0063 100644 --- a/store/migrations.go +++ b/store/migrations.go @@ -10,7 +10,7 @@ import ( ) // SchemaVersion is the schema version this binary writes and reads. -const SchemaVersion = 27 +const SchemaVersion = 28 // freshSchemaBaseline is the version applied to a brand-new database. The // chain in `migrations` continues from here. v1 is no longer reachable from @@ -70,6 +70,7 @@ func buildMigrations(mctx migrationCtx) []migration { {version: 25, up: migrateV24ToV25}, {version: 26, up: migrateV25ToV26}, {version: 27, up: migrateV26ToV27}, + {version: 28, up: migrateV27ToV28}, } } @@ -2250,3 +2251,47 @@ func createContestedPathsV26(ctx context.Context, tx *sql.Tx) error { } return nil } + +// --- v27 → v28 --- + +// migrateV27ToV28 adds runs.changed_count: how many files a run actually +// changed, as opposed to file_count's "files the run considered" (#182, +// completing #161's F19). +// +// file_count means different things per kind. A peer sync records the +// receiver-verified — i.e. transferred — count, so its no-ops already read +// as zero; a bucket push records transferred *plus* already-correct and an +// index run records added plus modified plus unchanged, so their no-ops +// read as the whole volume. No column answered "did this run change +// anything?" across kinds, which left `squirrel runs --changes` and the +// TUI's fold able to collapse only peer-sync no-ops — and in the reference +// household's steady state most rows are per-destination bucket pushes. +// +// The column is nullable with no default, so every pre-migration row reads +// back as NULL, meaning "unknown" rather than a fabricated zero. Run.NoOp +// falls back to the old file_count heuristic for those, keeping history's +// conservative rendering exactly as it is today. Purely additive: no +// existing column changes meaning and no run row is rewritten, so the audit +// trail stays intact. +// +// No index: the column is queried alongside a full listing read, never as a +// selective predicate, and its cardinality is low — an index would go +// unused. +func migrateV27ToV28(ctx context.Context, db *sql.DB) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return err + } + defer tx.Rollback() + + // One line, because SQLite stores the added column's text verbatim in + // sqlite_master and store/schema.sql is generated from it. + if _, err := tx.ExecContext(ctx, + `ALTER TABLE runs ADD COLUMN changed_count INTEGER CHECK (changed_count IS NULL OR changed_count >= 0)`); err != nil { + return fmt.Errorf("add runs.changed_count: %w", err) + } + if _, err := tx.ExecContext(ctx, `INSERT INTO schema_version (version) VALUES (28)`); err != nil { + return fmt.Errorf("record schema v28: %w", err) + } + return tx.Commit() +} diff --git a/store/runs.go b/store/runs.go index de9f38c..32f2c7d 100644 --- a/store/runs.go +++ b/store/runs.go @@ -77,15 +77,22 @@ func isTerminalStatus(status string) bool { // latter carries the *other side's* local run id so the two halves of one // logical sync can be joined offline. type Run struct { - ID int64 - Kind string - VolumeID sql.NullInt64 - Destination sql.NullString - StartedAtNs int64 - EndedAtNs sql.NullInt64 - Status string - Error sql.NullString - FileCount int64 + ID int64 + Kind string + VolumeID sql.NullInt64 + Destination sql.NullString + StartedAtNs int64 + EndedAtNs sql.NullInt64 + Status string + Error sql.NullString + FileCount int64 + // ChangedCount is how many files the run actually changed — + // transferred, added, superseded, offloaded — as opposed to + // FileCount's "files the run considered", which for bucket pushes and + // index runs also counts everything already correct or unchanged. + // NULL (Valid=false) means unknown: rows written before v28, and runs + // whose driver has no honest changed count to report. See Run.NoOp. + ChangedCount sql.NullInt64 PeerNodeID sql.NullInt64 CorrelatedRunID sql.NullInt64 // Shallow is true when the run skipped BLAKE3 verification in @@ -97,6 +104,28 @@ type Run struct { Shallow sql.NullBool } +// NoOp reports whether the run is routine steady-state noise: a clean +// success that changed nothing. It is the one rule behind both +// `squirrel runs --changes` and the TUI's `f` fold, so the CLI and the TUI +// hide exactly the same rows. +// +// A run that recorded a ChangedCount answers directly, whatever its kind. +// For the rest — history written before v28, and drivers with no honest +// count (a kopia snapshot reports only its total file count; a destination +// reset changes recorded state, not files) — it falls back to the +// FileCount == 0 heuristic. That fallback errs toward showing: FileCount +// over-counts for bucket and index runs, so such a run stays visible +// rather than risking a hidden run that moved content. +func (r Run) NoOp() bool { + if r.Status != RunStatusSuccess { + return false + } + if r.ChangedCount.Valid { + return r.ChangedCount.Int64 == 0 + } + return r.FileCount == 0 +} + // BeginRun records the start of a sync or restore run and returns its // id. Callers must pair it with FinishRun (typically via defer with an // error pointer or an explicit terminal call). volumeID must reference @@ -275,10 +304,37 @@ func nullInt64Label(v sql.NullInt64) string { return fmt.Sprintf("%d", v.Int64) } -// FinishRun records the terminal state of a run. errMsg is stored as NULL +// FinishRun records the terminal state of a run whose caller has no +// changed-file count to state: a run reaped by `runs fail`, a preflight +// refusal or a failure that never got as far as moving anything, a driver +// whose tool reports no such number. runs.changed_count is left NULL +// ("unknown"), which keeps the row in Run.NoOp's conservative file_count +// fallback rather than folding it away on a fabricated zero. Callers that +// do know must use FinishRunChanged, or a genuine no-op stays on screen. +// +// See FinishRunChanged for the shared contract (guarded transition, +// runs_audit row, error semantics). +func (s *Store) FinishRun(ctx context.Context, runID int64, status string, errMsg string, fileCount int64) error { + return s.finishRun(ctx, runID, status, errMsg, fileCount, sql.NullInt64{}) +} + +// FinishRunChanged is FinishRun for the drivers that know how many files +// their run actually changed: rclone's transferred count for a bucket +// push, the manifest delta for a content-layout push, the receiver-verified +// paths for a peer sync, added+modified for an index walk, the offloaded +// count for an offload. changedCount is recorded alongside fileCount so a +// clean run that changed nothing reads as the no-op it is regardless of +// kind (#182). +func (s *Store) FinishRunChanged(ctx context.Context, runID int64, status string, errMsg string, fileCount, changedCount int64) error { + return s.finishRun(ctx, runID, status, errMsg, fileCount, + sql.NullInt64{Int64: changedCount, Valid: true}) +} + +// finishRun records the terminal state of a run. errMsg is stored as NULL // when empty. fileCount should be the total number of files the run touched // (added + modified + unchanged) so the runs table doubles as a coarse audit -// log without needing to scan files. +// log without needing to scan files; changedCount narrows that to the files +// the run actually changed, or is invalid when the caller cannot say. // // The transition is guarded: a row already in a terminal status // (success/partial/failed) is never re-finalised — the first terminal @@ -292,7 +348,7 @@ func nullInt64Label(v sql.NullInt64) string { // The status update and a 'finish' runs_audit row are written in one // transaction so the append-only transition log can't diverge from the // run row. The audit note carries the resulting status. -func (s *Store) FinishRun(ctx context.Context, runID int64, status string, errMsg string, fileCount int64) error { +func (s *Store) finishRun(ctx context.Context, runID int64, status string, errMsg string, fileCount int64, changedCount sql.NullInt64) error { tx, err := s.db.BeginTx(ctx, nil) if err != nil { return fmt.Errorf("begin finish run %d: %w", runID, err) @@ -316,9 +372,9 @@ func (s *Store) FinishRun(ctx context.Context, runID int64, status string, errMs errVal = sql.NullString{String: errMsg, Valid: true} } if _, err := tx.ExecContext(ctx, ` - UPDATE runs SET ended_at_ns = ?, status = ?, error = ?, file_count = ? + UPDATE runs SET ended_at_ns = ?, status = ?, error = ?, file_count = ?, changed_count = ? WHERE id = ? - `, atNs, status, errVal, fileCount, runID); err != nil { + `, atNs, status, errVal, fileCount, changedCount, runID); err != nil { return fmt.Errorf("finish run %d: %w", runID, err) } if err := appendRunAuditTx(ctx, tx, @@ -349,12 +405,12 @@ type ListRunsOpts struct { // runColumns is the fixed projection for every read of a runs row. Keeps // the scan order in lockstep with the query order; adding a column means // editing one place. -const runColumns = `id, kind, volume_id, destination, started_at_ns, ended_at_ns, status, error, file_count, peer_node_id, correlated_run_id, shallow` +const runColumns = `id, kind, volume_id, destination, started_at_ns, ended_at_ns, status, error, file_count, changed_count, peer_node_id, correlated_run_id, shallow` func scanRun(scan func(...any) error) (Run, error) { var r Run err := scan(&r.ID, &r.Kind, &r.VolumeID, &r.Destination, &r.StartedAtNs, &r.EndedAtNs, - &r.Status, &r.Error, &r.FileCount, &r.PeerNodeID, &r.CorrelatedRunID, &r.Shallow) + &r.Status, &r.Error, &r.FileCount, &r.ChangedCount, &r.PeerNodeID, &r.CorrelatedRunID, &r.Shallow) return r, err } diff --git a/store/runs_test.go b/store/runs_test.go index 462fad3..fa96919 100644 --- a/store/runs_test.go +++ b/store/runs_test.go @@ -101,6 +101,89 @@ func TestFinishRunWritesAuditRow(t *testing.T) { } } +// TestFinishRunChangedRecordsCount covers the v28 column (#182): a driver +// that knows what it changed records it, and a driver that does not leaves +// runs.changed_count NULL rather than a false zero. +func TestFinishRunChangedRecordsCount(t *testing.T) { + s := openTestStore(t) + ctx := context.Background() + vID := makeVolume(t, s, "/v") + + known := makeRun(t, s, vID) + if err := s.FinishRunChanged(ctx, known, RunStatusSuccess, "", 42, 3); err != nil { + t.Fatalf("FinishRunChanged: %v", err) + } + got, err := s.GetRun(ctx, known) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.FileCount != 42 { + t.Errorf("file_count = %d, want 42", got.FileCount) + } + if !got.ChangedCount.Valid || got.ChangedCount.Int64 != 3 { + t.Errorf("changed_count = %+v, want 3", got.ChangedCount) + } + + unknown := makeRun(t, s, vID) + if err := s.FinishRun(ctx, unknown, RunStatusSuccess, "", 42); err != nil { + t.Fatalf("FinishRun: %v", err) + } + got, err = s.GetRun(ctx, unknown) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.ChangedCount.Valid { + t.Errorf("changed_count = %+v, want NULL when the caller can't say", got.ChangedCount) + } +} + +// TestFinishRunRejectsNegativeChangedCount pins the column's CHECK: a +// changed count is a cardinality, and a negative one is a bug in the +// caller's arithmetic, not a value to store. +func TestFinishRunRejectsNegativeChangedCount(t *testing.T) { + s := openTestStore(t) + ctx := context.Background() + vID := makeVolume(t, s, "/v") + runID := makeRun(t, s, vID) + + if err := s.FinishRunChanged(ctx, runID, RunStatusSuccess, "", 42, -1); err == nil { + t.Fatal("FinishRunChanged with a negative changed count should be rejected") + } + got, err := s.GetRun(ctx, runID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if got.Status != RunStatusRunning { + t.Errorf("status = %q, want the row left untouched at %q", got.Status, RunStatusRunning) + } +} + +// TestRunNoOp pins the shared fold rule behind `runs --changes` and the +// TUI's `f` key: the recorded changed count decides where there is one, and +// the pre-v28 file_count heuristic decides — conservatively — where there +// is not. +func TestRunNoOp(t *testing.T) { + changed := func(n int64) sql.NullInt64 { return sql.NullInt64{Int64: n, Valid: true} } + tests := []struct { + name string + run Run + want bool + }{ + {"in-sync bucket push", Run{Status: RunStatusSuccess, FileCount: 42, ChangedCount: changed(0)}, true}, + {"bucket push that transferred", Run{Status: RunStatusSuccess, FileCount: 42, ChangedCount: changed(1)}, false}, + {"all-unchanged re-index", Run{Status: RunStatusSuccess, FileCount: 900, ChangedCount: changed(0)}, true}, + {"failed run that changed nothing", Run{Status: RunStatusFailed, ChangedCount: changed(0)}, false}, + {"still running", Run{Status: RunStatusRunning, ChangedCount: changed(0)}, false}, + {"pre-v28 peer-sync no-op", Run{Status: RunStatusSuccess, FileCount: 0}, true}, + {"pre-v28 bucket push", Run{Status: RunStatusSuccess, FileCount: 42}, false}, + } + for _, tc := range tests { + if got := tc.run.NoOp(); got != tc.want { + t.Errorf("%s: NoOp() = %v, want %v", tc.name, got, tc.want) + } + } +} + // TestAppendRunAuditRoundTrip exercises the standalone (non-FinishRun) // audit writer the `runs fail` CLI and #77's correlation write use, and // confirms ListRunAudit returns rows oldest-first with operator/note diff --git a/store/schema.sql b/store/schema.sql index 5897ae7..6b396ea 100644 --- a/store/schema.sql +++ b/store/schema.sql @@ -1,6 +1,6 @@ -- Generated by `go test ./store -update-schema` — DO NOT EDIT. -- --- Flattened snapshot of the squirrel index schema at version 27, for humans +-- Flattened snapshot of the squirrel index schema at version 28, for humans -- and agents who want the current shape without replaying the migration -- chain in migrations.go. It is NOT used to create or migrate databases — -- a fresh DB is built by applyV5 plus the migration registry. The golden @@ -214,7 +214,7 @@ CREATE TABLE "runs" ( file_count INTEGER NOT NULL DEFAULT 0, peer_node_id INTEGER REFERENCES nodes(id), correlated_run_id INTEGER, - shallow INTEGER CHECK (shallow IS NULL OR shallow IN (0, 1)), + shallow INTEGER CHECK (shallow IS NULL OR shallow IN (0, 1)), changed_count INTEGER CHECK (changed_count IS NULL OR changed_count >= 0), CHECK ( (kind IN ('index','audit','offload') AND destination IS NULL) OR (kind IN ('sync','restore') AND destination IS NOT NULL AND destination != '') diff --git a/sync/content_addressed.go b/sync/content_addressed.go index 7131140..17376e5 100644 --- a/sync/content_addressed.go +++ b/sync/content_addressed.go @@ -216,6 +216,10 @@ func (h *contentAddressedHandler) push(ctx context.Context, rep *Report, volID, return fmt.Errorf("compute path delta since run %d: %w", watermark, err) } rep.Verification.Files = int64(len(delta)) + // The delta is exactly what this push changes at the destination: the + // paths whose content the manifest segment newly maps. An empty delta + // is a genuine no-op, which is what the runs fold keys on (#182). + rep.Changed = knownChanged(int64(len(delta))) if err := h.uploadObjects(ctx, rep, runID, plannedUploads(delta)); err != nil { return err } diff --git a/sync/finish_changed_test.go b/sync/finish_changed_test.go new file mode 100644 index 0000000..5dacccb --- /dev/null +++ b/sync/finish_changed_test.go @@ -0,0 +1,104 @@ +package sync + +import ( + "context" + "path/filepath" + "testing" + + "github.com/mbertschler/squirrel/store" +) + +// finishTestStore opens an empty index with one volume, returning the store +// and the volume id sync/restore runs can be opened against. It needs no +// rclone binary — these tests drive the terminal-state writer directly with +// a synthesised RunResult. +func finishTestStore(t *testing.T) (*store.Store, int64) { + t.Helper() + s, err := store.Open(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("store.Open: %v", err) + } + t.Cleanup(func() { s.Close() }) + vol, err := s.GetOrCreateVolume(context.Background(), "/pics") + if err != nil { + t.Fatalf("GetOrCreateVolume: %v", err) + } + return s, vol.ID +} + +// TestFinishRunRecordsTransferredAsChanged covers the bucket-push half of +// #182: rclone's file_count is transferred + already-correct, so only the +// transferred count can tell an in-sync push from one that moved content. +// The two runs below differ in nothing else — same file_count, same status +// — yet only the in-sync one folds out of `runs --changes` and the TUI. +func TestFinishRunRecordsTransferredAsChanged(t *testing.T) { + s, volID := finishTestStore(t) + ctx := context.Background() + + inSync := &Report{RcloneResult: RunResult{Checked: 42}} + moved := &Report{RcloneResult: RunResult{Transferred: 3, Checked: 39}} + for _, tc := range []struct { + name string + rep *Report + wantChanged int64 + wantNoOp bool + }{ + {"in-sync push", inSync, 0, true}, + {"push that transferred", moved, 3, false}, + } { + runID, err := s.BeginRun(ctx, store.RunKindSync, volID, "scratch", false) + if err != nil { + t.Fatalf("%s: BeginRun: %v", tc.name, err) + } + finishRun(ctx, s, false, runID, tc.rep) + if tc.rep.FinishErr != nil { + t.Fatalf("%s: FinishErr: %v", tc.name, tc.rep.FinishErr) + } + run, err := s.GetRun(ctx, runID) + if err != nil { + t.Fatalf("%s: GetRun: %v", tc.name, err) + } + if run.FileCount != 42 { + t.Errorf("%s: file_count = %d, want 42 (transferred + already-correct)", tc.name, run.FileCount) + } + if !run.ChangedCount.Valid || run.ChangedCount.Int64 != tc.wantChanged { + t.Errorf("%s: changed_count = %+v, want %d", tc.name, run.ChangedCount, tc.wantChanged) + } + if got := run.NoOp(); got != tc.wantNoOp { + t.Errorf("%s: NoOp() = %v, want %v", tc.name, got, tc.wantNoOp) + } + } +} + +// TestFinishHandlerRunLeavesKopiaChangedUnknown pins the honest gap: a +// kopia snapshot reports its whole tree and no changed count, so its run +// records changed_count NULL rather than a fabricated zero — and keeps the +// conservative file_count rendering instead of folding away. +func TestFinishHandlerRunLeavesKopiaChangedUnknown(t *testing.T) { + s, volID := finishTestStore(t) + ctx := context.Background() + runID, err := s.BeginRun(ctx, store.RunKindSync, volID, "vault", false) + if err != nil { + t.Fatalf("BeginRun: %v", err) + } + + rep := &Report{ + RunID: runID, + Status: store.RunStatusSuccess, + Verification: VerifyResult{Method: VerifyMethodKopia, Files: 42}, + } + finishHandlerRun(ctx, s, rep, nil) + if rep.FinishErr != nil { + t.Fatalf("FinishErr: %v", rep.FinishErr) + } + run, err := s.GetRun(ctx, runID) + if err != nil { + t.Fatalf("GetRun: %v", err) + } + if run.ChangedCount.Valid { + t.Errorf("changed_count = %+v, want NULL for a kopia snapshot", run.ChangedCount) + } + if run.NoOp() { + t.Error("a run with no changed count must stay visible, not fold on a guess") + } +} diff --git a/sync/handler.go b/sync/handler.go index 833a585..0784efc 100644 --- a/sync/handler.go +++ b/sync/handler.go @@ -186,6 +186,11 @@ func (h *peerHandler) sealed() {} // 'refused' via terminalStatus, and rep.Status is updated in place so the // CLI summary and TUI show the refusal as its own condition rather than a // generic failure (#157, F26). +// +// rep.Changed rides along as runs.changed_count where the handler set it +// (the content-layout pushes count their manifest delta); kopia leaves it +// unset, since a snapshot summary reports the whole tree and no honest +// changed count — that run keeps the conservative file_count rendering. func finishHandlerRun(ctx context.Context, s *store.Store, rep *Report, runErr error) { if rep.RunID == 0 { return @@ -195,7 +200,7 @@ func finishHandlerRun(ctx context.Context, s *store.Store, rep *Report, runErr e if runErr != nil { errMsg = runErr.Error() } - if err := s.FinishRun(ctx, rep.RunID, rep.Status, errMsg, rep.Verification.Files); err != nil { + if err := finishRunRow(ctx, s, rep.RunID, rep.Status, errMsg, rep.Verification.Files, rep.Changed); err != nil { rep.FinishErr = err } } diff --git a/sync/node.go b/sync/node.go index 1fbbc90..08113e5 100644 --- a/sync/node.go +++ b/sync/node.go @@ -75,8 +75,14 @@ func runNodeSession(ctx context.Context, s *store.Store, rcl *Rclone, vol *confi if err != nil { errMsg = err.Error() } + // The receiver-verified paths are both the file count and + // the changed count for a peer sync: the Merkle walk never + // sends an identical folder to /plan, so nothing that + // matched already reaches the transfer. Recording it as + // changed_count too states that explicitly rather than + // leaving the fold to infer it (#182). fileCount := int64(len(rep.NodeVerify.Matched)) - if finishErr := s.FinishRun(ctx, rep.RunID, finishStatus, errMsg, fileCount); finishErr != nil { + if finishErr := s.FinishRunChanged(ctx, rep.RunID, finishStatus, errMsg, fileCount, fileCount); finishErr != nil { rep.FinishErr = finishErr } } diff --git a/sync/packed.go b/sync/packed.go index d43bbb9..f5c3939 100644 --- a/sync/packed.go +++ b/sync/packed.go @@ -237,6 +237,9 @@ func (h *packedHandler) push(ctx context.Context, rep *Report, volID, runID int6 return fmt.Errorf("compute path delta since run %d: %w", watermark, err) } rep.Verification.Files = int64(len(delta)) + // Same reading as the content-addressed push: the delta is what this + // run changes at the destination, so an empty one folds as a no-op. + rep.Changed = knownChanged(int64(len(delta))) large, small, err := h.routeBySize(ctx, rep, plannedUploads(delta)) if err != nil { return err diff --git a/sync/sync.go b/sync/sync.go index df6319e..3b81262 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -2,6 +2,7 @@ package sync import ( "context" + "database/sql" "errors" "fmt" "io" @@ -129,6 +130,15 @@ type Report struct { // (the Merkle walk never sends identical folders to /plan, so it // cannot be counted from the disposition list alone). AlreadyCorrect int64 + // Changed is how many files this run actually wrote at its target — + // the destination for a push, the local restore tree for a restore — + // as opposed to the "files covered" counts above, which also include + // everything already correct. It lands in runs.changed_count so + // `squirrel runs --changes` and the TUI fold can collapse an in-sync + // bucket push (#182). Invalid (Valid=false) means the handler has no + // honest count to report — a kopia snapshot knows only its total file + // count — and the run keeps the conservative file_count rendering. + Changed sql.NullInt64 // Verification is the handler's typed durability report for this // push: which comparison backed it, what the tool counted, and — // via Verified() — whether the destination's copy was @@ -518,6 +528,11 @@ func finishRun(ctx context.Context, s *store.Store, dryRun bool, runID int64, re // destination and did not transfer — exactly the already-correct // count the summary reports (F7). Peer syncs set this themselves. rep.AlreadyCorrect = rep.RcloneResult.Checked + // The transfers are the other half of that split, and they are what + // this run actually changed at the destination — the number the + // no-op fold keys on (#182). Recording it here makes `already_correct` + // and the fold two views of one rclone summary. + rep.Changed = knownChanged(rep.RcloneResult.Transferred) if dryRun || runID == 0 { return } @@ -526,11 +541,29 @@ func finishRun(ctx context.Context, s *store.Store, dryRun bool, runID int64, re errMsg = rep.RcloneResult.FailedFiles[0].Message } fileCount := rep.RcloneResult.Transferred + rep.RcloneResult.Checked - if err := s.FinishRun(ctx, runID, rep.Status, errMsg, fileCount); err != nil { + if err := finishRunRow(ctx, s, runID, rep.Status, errMsg, fileCount, rep.Changed); err != nil { rep.FinishErr = err } } +// knownChanged wraps a driver's changed-file count for Report.Changed. +// Handlers that cannot report one leave the field zero-valued, which +// records runs.changed_count as NULL ("unknown") rather than a false zero. +func knownChanged(n int64) sql.NullInt64 { + return sql.NullInt64{Int64: n, Valid: true} +} + +// finishRunRow writes a run's terminal state through the changed-count-aware +// store call when the driver reported one, and through the plain FinishRun +// (leaving runs.changed_count NULL) when it did not. Shared by the rclone +// scaffold and the curated handlers so both make the same choice. +func finishRunRow(ctx context.Context, s *store.Store, runID int64, status, errMsg string, fileCount int64, changed sql.NullInt64) error { + if changed.Valid { + return s.FinishRunChanged(ctx, runID, status, errMsg, fileCount, changed.Int64) + } + return s.FinishRun(ctx, runID, status, errMsg, fileCount) +} + // historyDirInSourceWarning returns a one-line advisory when the source // volume already contains a literal .squirrel-history directory. Sync // filters it out of the rclone transfer so it can't pollute the diff --git a/sync/verify_remote.go b/sync/verify_remote.go index 2901111..641ad47 100644 --- a/sync/verify_remote.go +++ b/sync/verify_remote.go @@ -445,7 +445,14 @@ func recordVerifyOutcome(ctx context.Context, s *store.Store, rep *RemoteVerifyR }); err != nil { return err } - if err := s.FinishRun(ctx, rep.RunID, status, errMsg, int64(rep.Objects+rep.Packs)); err != nil { + // A verification pass moves no content, so what it changes is the + // fingerprints it recorded for the first time: the initial capture pass + // over a destination stays visible, and the cadence passes that only + // re-confirmed what was already recorded fold away as steady-state + // noise (#182). A pass that found a mismatch or a missing object is + // 'partial' or 'failed' and stays visible on status alone. + changed := int64(rep.Populated + rep.PacksPopulated) + if err := s.FinishRunChanged(ctx, rep.RunID, status, errMsg, int64(rep.Objects+rep.Packs), changed); err != nil { return fmt.Errorf("finish verify run %d: %w", rep.RunID, err) } return applyVerifyAlarm(ctx, s, rep, verifyErr) diff --git a/tui/runs.go b/tui/runs.go index 73548b1..b7c7cfb 100644 --- a/tui/runs.go +++ b/tui/runs.go @@ -259,10 +259,11 @@ func (m *runsModel) applyFilter() { for i := 0; i < len(filtered); { // Collapse a maximal run of consecutive no-op rows into one marker, // but only when there is more than one — a lone no-op isn't worth - // a fold line (F19). - if m.fold && runIsNoOp(filtered[i]) { + // a fold line (F19). store.Run.NoOp is the same rule the CLI's + // `runs --changes` filter applies, so both hide the same rows. + if m.fold && filtered[i].NoOp() { j := i - for j < len(filtered) && runIsNoOp(filtered[j]) { + for j < len(filtered) && filtered[j].NoOp() { j++ } if n := j - i; n > 1 { @@ -305,14 +306,6 @@ func foldMarkerRow(n int) table.Row { return table.Row{"⋯", "", fmt.Sprintf("%d no-op runs folded", n), "", "", "", "", "", ""} } -// runIsNoOp reports whether a run is routine no-op noise — a clean success -// that touched no files. Mirrors the CLI's `runs --changes` fold rule; a -// peer-sync no-op has file_count 0, so those collapse, while bucket/index -// no-ops (file_count counts files considered, not moved) stay visible. -func runIsNoOp(r store.Run) bool { - return r.Status == store.RunStatusSuccess && r.FileCount == 0 -} - // destinationCell renders the DESTINATION column, prefixing a peer-sync row // with its initiation-direction arrow so inbound and outbound are // distinguishable (F18): "→" outbound (we pushed), "←" inbound (peer diff --git a/tui/runs_polish_test.go b/tui/runs_polish_test.go index 09792d2..774f8b1 100644 --- a/tui/runs_polish_test.go +++ b/tui/runs_polish_test.go @@ -32,16 +32,67 @@ func TestDestinationCell(t *testing.T) { } } -// TestRunIsNoOp covers the F19 fold rule: a clean success that touched no -// files is a no-op; anything else is not. -func TestRunIsNoOp(t *testing.T) { - if !runIsNoOp(store.Run{Status: store.RunStatusSuccess, FileCount: 0}) { - t.Error("clean 0-file success should be a no-op") +// TestApplyFilterFoldsBucketNoOps covers the F19 fold now that it keys on +// runs.changed_count (#182): consecutive bucket pushes that transferred +// nothing collapse behind one marker even though their file_count counts +// the whole volume, while the push that moved content stays on screen. +func TestApplyFilterFoldsBucketNoOps(t *testing.T) { + bucketRun := func(id, changed int64) store.Run { + return store.Run{ + ID: id, + Kind: store.RunKindSync, + Destination: sql.NullString{String: "s3archive", Valid: true}, + Status: store.RunStatusSuccess, + FileCount: 42, + ChangedCount: sql.NullInt64{Int64: changed, Valid: true}, + } } - if runIsNoOp(store.Run{Status: store.RunStatusSuccess, FileCount: 3}) { - t.Error("success that touched files is not a no-op") + m := newRunsModel(nil) + m.resizeColumns() + m.rows = []store.Run{bucketRun(4, 0), bucketRun(3, 0), bucketRun(2, 3), bucketRun(1, 0)} + m.applyFilter() + + if m.foldedCount != 2 { + t.Errorf("foldedCount = %d, want 2 (the pair of in-sync pushes)", m.foldedCount) + } + // Marker, the run that moved content, the lone trailing no-op. + if len(m.displayRuns) != 3 { + t.Fatalf("displayed %d rows, want 3: %+v", len(m.displayRuns), m.displayRuns) + } + if m.displayRuns[0] != nil { + t.Errorf("row 0 should be the fold marker, got run %+v", m.displayRuns[0]) + } + if m.displayRuns[1] == nil || m.displayRuns[1].ID != 2 { + t.Errorf("row 1 should be run 2 (transferred 3 files), got %+v", m.displayRuns[1]) + } + if m.displayRuns[2] == nil || m.displayRuns[2].ID != 1 { + t.Errorf("row 2 should be the lone no-op run 1, got %+v", m.displayRuns[2]) + } +} + +// TestApplyFilterKeepsPreV28NoOps covers the fallback: history written +// before changed_count existed keeps its conservative rendering, so a pair +// of bucket pushes with a non-zero file_count and no changed count stays +// visible rather than folding on a guess. +func TestApplyFilterKeepsPreV28NoOps(t *testing.T) { + legacy := func(id int64) store.Run { + return store.Run{ + ID: id, + Kind: store.RunKindSync, + Destination: sql.NullString{String: "s3archive", Valid: true}, + Status: store.RunStatusSuccess, + FileCount: 42, + } + } + m := newRunsModel(nil) + m.resizeColumns() + m.rows = []store.Run{legacy(2), legacy(1)} + m.applyFilter() + + if m.foldedCount != 0 { + t.Errorf("foldedCount = %d, want 0 — unknown changed counts must not fold", m.foldedCount) } - if runIsNoOp(store.Run{Status: store.RunStatusFailed}) { - t.Error("failed run is not a no-op") + if len(m.displayRuns) != 2 { + t.Errorf("displayed %d rows, want both runs", len(m.displayRuns)) } }