fix: let offline backups copy without live-work pacing #1261
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "ftw": patch | ||
| --- | ||
|
|
||
| Let the offline backup helper copy large histories without the live 100 ms pause that made a two-hour export deadline unreachable. Keep live backup yielding so charging-goal saves stay within their latency limit, and refuse to start when the destination cannot hold the raw export, compressed archive and verification extract. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,9 @@ func Create(ctx context.Context, opts CreateOptions) (Info, error) { | |
| if err := os.MkdirAll(outputDir, 0o700); err != nil { | ||
| return Info{}, fmt.Errorf("backup: create output dir: %w", err) | ||
| } | ||
| if err := state.EnsureDiskSpace(outputDir, state.BackupArchiveScratch(opts.State.BackupSourceBytes(), 0)); err != nil { | ||
| return Info{}, err | ||
| } | ||
| if opts.Maintenance != nil { | ||
| opts.Maintenance.Lock() | ||
| defer opts.Maintenance.Unlock() | ||
|
|
@@ -245,7 +248,7 @@ func Create(ctx context.Context, opts CreateOptions) (Info, error) { | |
| } | ||
|
|
||
| tmpPath := filepath.Join(outputDir, "."+id+".tmp") | ||
| if err := writeArchive(ctx, tmpPath, manifest, sources, !opts.State.OfflineBackup()); err != nil { | ||
| _ = os.Remove(tmpPath) | ||
| return Info{}, err | ||
| } | ||
|
|
@@ -381,7 +384,7 @@ func describeSource(ctx context.Context, dataDir string, source sourceEntry) (Fi | |
| return entry, err | ||
| } | ||
|
|
||
| func writeArchive(ctx context.Context, dst string, manifest Manifest, sources []sourceEntry, live bool) error { | ||
| f, err := os.OpenFile(dst, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -393,7 +396,7 @@ func writeArchive(ctx context.Context, dst string, manifest Manifest, sources [] | |
| _ = os.Remove(dst) | ||
| } | ||
| }() | ||
| zw, err := gzip.NewWriterLevel(state.NewMaintenanceWriterPaced(ctx, f, live), gzip.BestSpeed) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -491,7 +494,7 @@ func Verify(archivePath string) (Manifest, error) { | |
| want[entry.Path] = entry | ||
| } | ||
| seen := make(map[string]bool, len(want)) | ||
| tmpDir, err := os.MkdirTemp(filepath.Dir(archivePath), ".ftw-backup-verify-") | ||
|
There was a problem hiding this comment.
When an archive is stored on a read-only USB mount or in a directory the invoking user cannot modify, creating this temporary directory fails even though the archive is readable and the restore destination is writable. Because both AGENTS.md reference: AGENTS.md:L24-L25 Useful? React with 👍 / 👎. |
||
| if err != nil { | ||
| return Manifest{}, err | ||
| } | ||
|
|
@@ -569,7 +572,7 @@ func Verify(archivePath string) (Manifest, error) { | |
| if len(seen) != len(want) { | ||
| return Manifest{}, errors.New("backup: archive is missing one or more manifest files") | ||
| } | ||
| if err := verifyCompressedDatabase(dbGzip, tmpDir); err != nil { | ||
| return Manifest{}, err | ||
| } | ||
| return manifest, nil | ||
|
|
@@ -1055,8 +1058,8 @@ func extractArchive(archivePath, staging string) error { | |
| return verifyDatabase(filepath.Join(staging, filepath.FromSlash(manifest.DatabaseFile))) | ||
| } | ||
|
|
||
| func verifyCompressedDatabase(src, workDir string) error { | ||
| tmp, err := os.CreateTemp(workDir, ".ftw-backup-db-*.sqlite") | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| package state | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "errors" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| func TestBackupWorkContextOmitsDeadlineWhenOffline(t *testing.T) { | ||
| offline := &Store{offlineBackup: true} | ||
| ctx, cancel := offline.backupWorkContext() | ||
| defer cancel() | ||
| if _, ok := ctx.Deadline(); ok { | ||
| t.Fatal("offline backup must not inherit the live export deadline") | ||
| } | ||
|
|
||
| live := &Store{} | ||
| ctx, cancel = live.backupWorkContext() | ||
| defer cancel() | ||
| deadline, ok := ctx.Deadline() | ||
| if !ok { | ||
| t.Fatal("live backup must keep an export deadline") | ||
| } | ||
| until := time.Until(deadline) | ||
| if until > liveBackupTimeout || until < liveBackupTimeout-time.Minute { | ||
| t.Fatalf("live backup deadline = %v", until) | ||
| } | ||
| } | ||
|
|
||
| func TestLiveBackupCopyYieldsBetweenBatches(t *testing.T) { | ||
| s := freshStore(t) | ||
| pauses := 0 | ||
| s.backupPause = func(context.Context) error { pauses++; return nil } | ||
| if err := s.BulkRecordHistory(pacedBackupPoints(2500)); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| dst := filepath.Join(t.TempDir(), "export.db") | ||
| if err := s.copyStateForBackup(context.Background(), dst); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := s.exportHistoryToSQLite(context.Background(), dst); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if pauses == 0 { | ||
| t.Fatal("live backup must yield between copy batches") | ||
| } | ||
| } | ||
|
|
||
| func TestOfflineBackupCopyDoesNotYieldBetweenBatches(t *testing.T) { | ||
| s := freshStore(t) | ||
| if err := s.BulkRecordHistory(pacedBackupPoints(2500)); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| path := s.mainDBPath | ||
| if err := s.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| src, err := OpenBackupSource(path) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer src.Close() | ||
| if !src.OfflineBackup() { | ||
| t.Fatal("OpenBackupSource must mark the helper offline") | ||
| } | ||
| pauses := 0 | ||
| src.backupPause = func(context.Context) error { | ||
| pauses++ | ||
| return nil | ||
| } | ||
| dst := filepath.Join(t.TempDir(), "export.db") | ||
| if err := src.copyStateForBackup(context.Background(), dst); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := src.exportHistoryToSQLite(context.Background(), dst); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if pauses != 0 { | ||
| t.Fatalf("offline backup paused %d times; live 100ms yield must not apply", pauses) | ||
| } | ||
| db, err := sql.Open("sqlite", dst) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer db.Close() | ||
| var n int | ||
| if err := db.QueryRow(`SELECT COUNT(*) FROM history_hot`).Scan(&n); err != nil || n != 2500 { | ||
| t.Fatalf("offline export lost rows: %d %v", n, err) | ||
| } | ||
| } | ||
|
|
||
| func TestBackupArchiveScratchReservesRawGzipAndVerify(t *testing.T) { | ||
| const source, extra int64 = 100, 7 | ||
| got := BackupArchiveScratch(source, extra) | ||
| if got < 3*source+extra+backupScratchHeadroom { | ||
| t.Fatalf("scratch %d omits a coexistence phase", got) | ||
| } | ||
| // Issue #1259: 174,629,397 sample rows with a 100 ms pause every 1,024 | ||
| // rows spend 4h44m paused, so the live 2h export deadline must fire. | ||
| const rows int64 = 174629397 | ||
| livePause := time.Duration(rows/1024) * maintenancePause | ||
| if livePause <= liveBackupTimeout { | ||
| t.Fatalf("live pause floor %v no longer exceeds the %v export deadline", livePause, liveBackupTimeout) | ||
| } | ||
| if backupCopyScratch(source) < 2*source { | ||
| t.Fatal("copy scratch must hold raw export and gzip together") | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureDiskSpaceRejectsShortFilesystem(t *testing.T) { | ||
| orig := backupDiskAvail | ||
| t.Cleanup(func() { backupDiskAvail = orig }) | ||
| needed := BackupArchiveScratch(100, 0) | ||
| backupDiskAvail = func(string) (int64, error) { return needed - 1, nil } | ||
| if err := EnsureDiskSpace(t.TempDir(), needed); err == nil { | ||
| t.Fatal("accepted a filesystem smaller than the export scratch") | ||
| } | ||
| backupDiskAvail = func(string) (int64, error) { return needed, nil } | ||
| if err := EnsureDiskSpace(t.TempDir(), needed); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| func TestEnsureDiskSpaceSkipsUnknownProbe(t *testing.T) { | ||
| orig := backupDiskAvail | ||
| t.Cleanup(func() { backupDiskAvail = orig }) | ||
| backupDiskAvail = func(string) (int64, error) { return 0, errors.New("unsupported") } | ||
| if err := EnsureDiskSpace(t.TempDir(), 1<<40); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } | ||
|
|
||
| func TestBackupToCompressedPreflightUsesSourceSize(t *testing.T) { | ||
| s := freshStore(t) | ||
| if err := s.SaveConfig("goal", "80%"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| orig := backupDiskAvail | ||
| t.Cleanup(func() { backupDiskAvail = orig }) | ||
| backupDiskAvail = func(string) (int64, error) { return 1, nil } | ||
| err := s.BackupToCompressed(filepath.Join(t.TempDir(), "full.gz")) | ||
| if err == nil { | ||
| t.Fatal("published a backup without room for scratch files") | ||
| } | ||
| } | ||
|
|
||
| func pacedBackupPoints(n int) []HistoryPoint { | ||
| points := make([]HistoryPoint, n) | ||
| for i := range points { | ||
| points[i] = HistoryPoint{TsMs: int64(i + 1), GridW: float64(i)} | ||
| } | ||
| return points | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
dataDircontains substantial cold Parquet history or other persistent files, this preflight counts only the SQLite state/history files becauseextraBytesis always zero, whilecollectSourceslater adds those files to the archive. A target with room for three copies of a small database but not the cold archive therefore passes the promised refusal check and fills during creation; inventory and include the non-database source sizes before checking free space.AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.