From 68e1e47534fe65bd89097e6edeb755c45216ea84 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 2 Nov 2026 20:21:00 -0500 Subject: [PATCH] fix: a new server takes a backup after its first scan, and restarts to load the restored settings (#6786) A one-off end-to-end run of the :dev image (scratch Postgres, generated FLAC files, no outbound network) found that the web restore, done the way docs/hosting.md describes, was refused with not_empty: a new server's startup scan fills the library before anyone signs up, and Restore counted tracks as occupied. The command-line restore passed the same run. - Restore now asks only that the instance has no accounts. The library the first scan found is replaced by the backup's (every foreign key into artists/albums/tracks cascades or sets null), and the restore locks the library tables so a running scan waits rather than adding to them. - Settings are read once at boot by about ten services, the library folders among them, so after a web restore the server ran on its defaults until restarted. The restore endpoint now answers, then asks the process to restart in place (syscall.Exec of the same binary, arguments and environment), which needs no container restart policy. The startup scan of the restarted server replaces the scan the endpoint used to start. - The restore form says Minstrel is restarting; the docs say a scanned new server still takes a backup. Co-Authored-By: Claude Opus 5.5 --- cmd/minstrel/main.go | 39 ++++++++++++++++++- docs/hosting.md | 9 +++-- internal/api/api.go | 6 ++- internal/api/auth_restore.go | 36 ++++++++--------- internal/api/auth_restore_test.go | 15 ++++++- internal/backup/backup_test.go | 11 +++++- internal/backup/restore.go | 27 ++++++------- internal/server/server.go | 5 ++- web/src/lib/auth/store.svelte.ts | 2 +- .../lib/components/auth/RestoreForm.svelte | 11 +++--- web/src/routes/register/register.test.ts | 6 +-- 11 files changed, 118 insertions(+), 49 deletions(-) diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 27103396..c77c7e84 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -69,12 +69,32 @@ func main() { return } } - if err := run(); err != nil { + err := run() + if errors.Is(err, errRestart) { + err = restartInPlace() + } + if err != nil { fmt.Fprintf(os.Stderr, "minstrel: %v\n", err) os.Exit(1) } } +// errRestart is run's answer when the server stopped in order to start again: +// after a web restore, so the restored settings are read at boot. +var errRestart = errors.New("restart requested") + +// restartInPlace replaces this process with a fresh start of the same binary, +// arguments and environment. The process id stays the same, so a container +// keeps running without needing a restart policy. +func restartInPlace() error { + exe, err := os.Executable() + if err != nil { + return fmt.Errorf("restart: %w", err) + } + // Exec returns only when it failed. + return fmt.Errorf("restart: %w", syscall.Exec(exe, os.Args, os.Environ())) +} + // safetyNetScanInterval backstops the fsnotify watcher with a low-frequency // full delta walk, catching anything the watcher missed (inotify watch-limit // exhaustion, dropped events). Fixed — there is no operator configuration. @@ -485,6 +505,13 @@ func run() error { go backups.Loop(ctx) srv.Backups = backups srv.StreamSecret = cfg.StreamSecret + restart := make(chan struct{}, 1) + srv.Restart = func() { + select { + case restart <- struct{}{}: + default: + } + } httpServer := &http.Server{ Addr: cfg.Server.Address, Handler: srv.Router(), @@ -510,14 +537,20 @@ func run() error { close(errCh) }() + restarting := false select { case <-ctx.Done(): logger.Info("shutdown signal received") + case <-restart: + logger.Info("restarting to load the restored backup") + restarting = true case err := <-errCh: if err != nil { return fmt.Errorf("server: %w", err) } } + // Ends the background work (scans, workers, schedules) as a signal would. + stop() shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -526,6 +559,10 @@ func run() error { } // A backup still running is stopped and records that it was cut off. backups.Close() + if restarting { + logger.Info("minstrel stopped; starting again") + return errRestart + } logger.Info("minstrel stopped") return nil } diff --git a/docs/hosting.md b/docs/hosting.md index 20e25e65..2e553d8a 100644 --- a/docs/hosting.md +++ b/docs/hosting.md @@ -119,8 +119,9 @@ to the music as well. ### Restoring -A backup restores into a new Minstrel: an empty database, with no accounts -and no library yet. +A backup restores into a new Minstrel: one nobody has signed up to yet. If +it has already scanned the music, that's fine; the backup's library takes +the place of what the scan found. 1. **Put the music back first**, at the path the library had. The restored settings name the old library folders, and each track is found again by @@ -130,7 +131,9 @@ and no library yet. 2. **Load the backup**, either way: - In the web app, choose **Create account**, then **Restore a backup**. Pick a `minstrel-….tar` file from the backup's `database/` folder and - enter the setup token from the log (see *First account*). + enter the setup token from the log (see *First account*). Minstrel + restarts by itself once the backup is in, so it runs on the restored + settings. - Or, with the server stopped, run `docker compose run --rm minstrel restore /backups/database/minstrel-20261102T030000Z.tar`, with the file on a volume the container can read. It brings the database diff --git a/internal/api/api.go b/internal/api/api.go index e807c05a..688fa1a2 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -41,7 +41,7 @@ import ( // Mount attaches /api/* handlers to r. Public endpoints (login) are outside // RequireUser; everything else is gated by the middleware. The events writer // is shared with the Subsonic mount so /rest/scrobble feeds the same store. -func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, requestSvc *requests.Service, quarantineSvc *quarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte, netSettings *netsettings.Service, reacqSettings *reacquisition.SettingsService, fpSettings *library.FingerprintSettingsService, loudSettings *library.LoudnessSettingsService, acoustIDLookup *library.AcoustIDLookupWorker, backups *backup.Service) { +func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playevents.Writer, recCfg config.RecommendationConfig, recSettings *recsettings.Service, requestSvc *requests.Service, quarantineSvc *quarantine.Service, tracksSvc *tracks.Service, playlistsSvc *playlists.Service, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, tagSettings *tags.SettingsService, scanner *library.Scanner, scanCfg library.RunScanConfig, dataDir string, sender mailer.Sender, bus *eventbus.Bus, playlistScheduler *playlists.Scheduler, streamSecret []byte, netSettings *netsettings.Service, reacqSettings *reacquisition.SettingsService, fpSettings *library.FingerprintSettingsService, loudSettings *library.LoudnessSettingsService, acoustIDLookup *library.AcoustIDLookupWorker, backups *backup.Service, restart func()) { rng := rand.New(rand.NewSource(rand.Int63())) setupToken, err := auth.NewSetupToken() if err != nil { @@ -76,6 +76,7 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev loudnessSettings: loudSettings, acoustIDLookup: acoustIDLookup, backups: backups, + restart: restart, acquisition: acquire.NewStore(pool), catalogue: cat, wanted: wanted.New(pool, cat, logger), @@ -439,6 +440,9 @@ type handlers struct { acoustIDLookup *library.AcoustIDLookupWorker // backups is the server's own backup: settings, status, test and run. backups *backup.Service + // restart stops the server and starts it again in place, so a restored + // backup's settings are read at boot. Nil in tests. + restart func() // acquisition is the configured indexers and download clients. acquisition *acquire.Store // catalogue is the release catalogue read from MusicBrainz, and diff --git a/internal/api/auth_restore.go b/internal/api/auth_restore.go index 8a8e39a2..f43e5ca5 100644 --- a/internal/api/auth_restore.go +++ b/internal/api/auth_restore.go @@ -1,14 +1,12 @@ package api import ( - "context" "errors" "net/http" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/backup" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/library" ) // SetupTokenHeader carries the setup token on a restore upload, whose body is @@ -19,20 +17,24 @@ type restoreResp struct { // Accounts is how many accounts came back; everyone signs in with the // password they had. Accounts int64 `json:"accounts"` - // ScanStarted says the library scan that refills what the backup leaves - // out is under way. - ScanStarted bool `json:"scan_started"` + // Restarting says the server restarts now to load the restored settings. + // The scan at its start finds each music file and fills in what the + // backup leaves out. + Restarting bool `json:"restarting"` } // handleRestore implements POST /api/auth/restore: a new server, with no // accounts yet, loads a backup file sent as the request body. It asks for the // setup token from the server log, as creating the first account does, since -// the backup's admins become this server's admins. Then it starts a library -// scan, which finds each file at its path and fills in what the backup left -// out. +// the backup's admins become this server's admins. A library the server's +// first scan found is replaced by the backup's. +// +// Then the server restarts. Settings are read once, at boot, so the restored +// ones (the library folders among them) take effect the same way they do +// after `minstrel restore`, and the startup scan finds each file at its path. // // 200 restored · 400 not_a_backup · 403 setup_token_invalid · -// 409 not_empty (accounts or a library already) · 409 newer_backup +// 409 not_empty (accounts already) · 409 newer_backup func (h *handlers) handleRestore(w http.ResponseWriter, r *http.Request) { ctx := r.Context() q := dbq.New(h.pool) @@ -63,7 +65,7 @@ func (h *handlers) handleRestore(w http.ResponseWriter, r *http.Request) { } switch err := backup.Restore(ctx, h.pool, contents); { case errors.Is(err, backup.ErrNotEmpty): - writeErr(w, apierror.Conflict("not_empty", "this server already has accounts or a library; a backup restores into a new one")) + writeErr(w, apierror.Conflict("not_empty", "this server already has accounts; a backup restores into a new one")) return case errors.Is(err, backup.ErrNewerSchema): writeErr(w, apierror.Conflict("newer_backup", "the backup was made by a newer Minstrel; update this server first")) @@ -82,13 +84,11 @@ func (h *handlers) handleRestore(w http.ResponseWriter, r *http.Request) { } h.logger.Info("restored a backup", "made_at", manifest.CreatedAt, "build", manifest.Build, "schema", manifest.Schema, "accounts", out.Accounts) - if h.scanner != nil { - started, _, err := library.TryStartScan(context.Background(), h.pool, h.scanner, h.coverart, - h.logger.With("source", "restore"), h.scanCfg) - if err != nil { - h.logger.Warn("restore: start the library scan", "err", err) - } - out.ScanStarted = started - } + out.Restarting = h.restart != nil writeJSON(w, http.StatusOK, out) + if h.restart != nil { + // The server drains this request before it stops, so the answer + // reaches the browser first. + h.restart() + } } diff --git a/internal/api/auth_restore_test.go b/internal/api/auth_restore_test.go index fa23be36..e70c366e 100644 --- a/internal/api/auth_restore_test.go +++ b/internal/api/auth_restore_test.go @@ -40,6 +40,8 @@ func TestRestore_ANewServerTakesABackupWithTheSetupToken(t *testing.T) { } h.setupToken = token h.requireSetupToken = true + restarts := 0 + h.restart = func() { restarts++ } restore := func(setupToken string, body []byte) *httptest.ResponseRecorder { req := httptest.NewRequest(http.MethodPost, "/api/auth/restore", bytes.NewReader(body)) req.Header.Set("Content-Type", "application/x-tar") @@ -65,14 +67,20 @@ func TestRestore_ANewServerTakesABackupWithTheSetupToken(t *testing.T) { if err := pool.QueryRow(ctx, "SELECT count(*) FROM users").Scan(&n); err != nil || n != 0 { t.Fatalf("a refused restore left %d accounts (%v)", n, err) } + if restarts != 0 { + t.Errorf("a refused restore restarted the server %d times", restarts) + } w := restore(token.Value(), file.Bytes()) if w.Code != http.StatusOK { t.Fatalf("restore: status = %d body = %s", w.Code, w.Body.String()) } var out restoreResp - if err := json.NewDecoder(w.Body).Decode(&out); err != nil || out.Accounts != 1 { - t.Errorf("restore answered %+v (%v), want one account", out, err) + if err := json.NewDecoder(w.Body).Decode(&out); err != nil || out.Accounts != 1 || !out.Restarting { + t.Errorf("restore answered %+v (%v), want one account and a restart", out, err) + } + if restarts != 1 { + t.Errorf("the restore restarted the server %d times, want once so the restored settings load", restarts) } var admin bool if err := pool.QueryRow(ctx, "SELECT is_admin FROM users WHERE username = $1", @@ -84,4 +92,7 @@ func TestRestore_ANewServerTakesABackupWithTheSetupToken(t *testing.T) { !strings.Contains(w.Body.String(), "not_empty") { t.Errorf("a second restore: status = %d body = %s, want 409 not_empty", w.Code, w.Body.String()) } + if restarts != 1 { + t.Errorf("a refused second restore restarted the server (%d restarts)", restarts) + } } diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index fbb9164f..5727ad53 100644 --- a/internal/backup/backup_test.go +++ b/internal/backup/backup_test.go @@ -189,7 +189,16 @@ func TestARestoredInstanceHoldsWhatTheBackupHeld_Integration(t *testing.T) { t.Fatalf("restore from a newer schema: err = %v, want ErrNewerSchema", err) } - mustExec(t, pool, "TRUNCATE users, artists, library_changes CASCADE") + // A new server scans its music before anyone signs up, so it already has + // a library, here with a track the backup doesn't know. It still takes + // the backup: the backup's library replaces what the scan found, which the + // next scan finds again under the backup's ids. + mustExec(t, pool, "TRUNCATE users CASCADE") + scannedArtist := mustID(t, pool, `INSERT INTO artists (name, sort_name) VALUES ('Scanned', 'scanned') RETURNING id::text`) + scannedAlbum := mustID(t, pool, `INSERT INTO albums (title, sort_title, artist_id) VALUES ('Scanned', 'scanned', $1) RETURNING id::text`, scannedArtist) + scanned := mustID(t, pool, `INSERT INTO tracks (title, album_id, artist_id, duration_ms, file_path, file_size, file_format) + VALUES ('Scanned', $1, $2, 1000, '/music/b.flac', 10, 'flac') RETURNING id::text`, scannedAlbum, scannedArtist) + mustExec(t, pool, `INSERT INTO library_changes (entity_type, entity_id, op) VALUES ('track', $1, 'upsert')`, scanned) if err := backup.Restore(ctx, pool, first); err != nil { t.Fatalf("restore: %v", err) } diff --git a/internal/backup/restore.go b/internal/backup/restore.go index 31d191dd..46969de5 100644 --- a/internal/backup/restore.go +++ b/internal/backup/restore.go @@ -13,9 +13,8 @@ import ( ) var ( - // ErrNotEmpty is a restore into an instance that already has accounts or - // a library. - ErrNotEmpty = errors.New("backup: this Minstrel already has accounts or a library; a backup restores into an empty one") + // ErrNotEmpty is a restore into an instance that already has accounts. + ErrNotEmpty = errors.New("backup: this Minstrel already has accounts; a backup restores into a new one") // ErrNewerSchema is a backup written by a newer Minstrel than this one. ErrNewerSchema = errors.New("backup: the backup was written by a newer version of Minstrel; upgrade this one first") @@ -30,10 +29,12 @@ const batchRows = 500 // Restore loads a backup into an empty instance, in one transaction: either // all of it lands or none of it does. // -// The instance must have no accounts and no tracks. Its other rows in the -// tables the backup holds are replaced, which is what a fresh instance's -// seeded settings need. A table the backup doesn't hold, because it was -// written before that table existed, keeps what the instance has. +// The instance must have no accounts. Its rows in the tables the backup holds +// are replaced: a fresh instance's seeded settings, and the library its +// startup scan found before anyone signed up, which the scan after the +// restore finds again under the backup's ids. A table the backup doesn't +// hold, because it was written before that table existed, keeps what the +// instance has. // // A row's columns are those the backup and the table both have: an older // backup's rows take the defaults of columns added since, and a column dropped @@ -52,15 +53,15 @@ func Restore(ctx context.Context, pool *pgxpool.Pool, c Contents) error { if c.Schema > schema { return ErrNewerSchema } - // Taken before the emptiness check, so a first account or a scan starting - // at the same moment waits for the restore, then finds the data. - if _, err := tx.Exec(ctx, "LOCK TABLE users, tracks IN EXCLUSIVE MODE"); err != nil { + // Taken before the emptiness check, so a first account made at the same + // moment waits for the restore, then finds the data. A scan still running + // waits too, rather than adding to the library while it is replaced. + if _, err := tx.Exec(ctx, "LOCK TABLE users, artists, albums, tracks IN EXCLUSIVE MODE"); err != nil { return fmt.Errorf("backup: lock: %w", err) } var occupied bool - if err := tx.QueryRow(ctx, - "SELECT EXISTS (SELECT 1 FROM users) OR EXISTS (SELECT 1 FROM tracks)").Scan(&occupied); err != nil { - return fmt.Errorf("backup: check the instance is empty: %w", err) + if err := tx.QueryRow(ctx, "SELECT EXISTS (SELECT 1 FROM users)").Scan(&occupied); err != nil { + return fmt.Errorf("backup: check the instance is new: %w", err) } if occupied { return ErrNotEmpty diff --git a/internal/server/server.go b/internal/server/server.go index d452841c..c9d66873 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -104,6 +104,9 @@ type Server struct { // leave it nil leave the cookie path intact and reject all signed // tokens (HMAC of empty key matches nothing a client could mint). StreamSecret []byte + // Restart asks the process to stop and start again in place. A web + // restore calls it so the restored settings are read at boot. + Restart func() } func New(logger *slog.Logger, pool *pgxpool.Pool, scanner ScanTrigger, subCfg subsonic.Config, eventsCfg config.EventsConfig, recCfg config.RecommendationConfig, dataDir string, brandingCfg config.BrandingConfig, coverEnricher *coverart.Enricher, coverSettings *coverart.SettingsService, libraryScanner *library.Scanner, scanCfg library.RunScanConfig) *Server { @@ -208,7 +211,7 @@ func (s *Server) Router() http.Handler { if backups == nil { backups = backup.NewService(s.Pool, s.Logger, buildinfo.Version) } - api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, requestSvc, quarantineSvc, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret, netSettings, reacqSettings, fpSettings, loudSettings, acoustIDLookup, backups) + api.Mount(r, s.Pool, s.Logger, writer, s.RecommendationCfg, recSettings, requestSvc, quarantineSvc, tracksSvc, playlistsSvc, s.CoverEnricher, s.CoverSettings, s.TagSettings, s.LibraryScanner, s.ScanCfg, s.DataDir, smtpSender, bus, s.PlaylistScheduler, s.StreamSecret, netSettings, reacqSettings, fpSettings, loudSettings, acoustIDLookup, backups, s.Restart) // /api/admin/scan is the only admin route owned by the server package // (it needs the Scanner). Register it as a single inline-middleware // route — using r.Route("/api/admin", ...) here would create a second diff --git a/web/src/lib/auth/store.svelte.ts b/web/src/lib/auth/store.svelte.ts index efb626a8..59351d3a 100644 --- a/web/src/lib/auth/store.svelte.ts +++ b/web/src/lib/auth/store.svelte.ts @@ -75,7 +75,7 @@ export async function register(opts: { void sendTimezoneIfStale(); } -export type RestoreResult = { accounts: number; scan_started: boolean }; +export type RestoreResult = { accounts: number; restarting: boolean }; /** * Loads a backup file into a new server, which has no accounts yet. The file diff --git a/web/src/lib/components/auth/RestoreForm.svelte b/web/src/lib/components/auth/RestoreForm.svelte index 646bb26b..fc39711e 100644 --- a/web/src/lib/components/auth/RestoreForm.svelte +++ b/web/src/lib/components/auth/RestoreForm.svelte @@ -22,7 +22,7 @@ case 'newer_backup': return 'That backup was made by a newer Minstrel. Update this server first, then restore it.'; case 'not_empty': - return 'This server already has accounts or a library, so it can’t take a backup.'; + return 'This server already has accounts, so it can’t take a backup.'; case 'too_large': return 'That file is larger than a backup can be.'; default: @@ -50,12 +50,13 @@ {#if done}

- Restored {plural(done.accounts, 'account', 'accounts')}. Sign in with the password you had. + Restored {plural(done.accounts, 'account', 'accounts')}. + {done.restarting + ? 'Minstrel is restarting to load them; in a few seconds, sign in with the password you had.' + : 'Sign in with the password you had.'}

- {done.scan_started - ? 'The library scan has started: it finds each music file and fetches the artwork and tags again.' - : 'Start a library scan under Admin › Library once you’re signed in.'} + The library scan finds each music file and fetches the artwork and tags again.

Sign in
diff --git a/web/src/routes/register/register.test.ts b/web/src/routes/register/register.test.ts index 19f0c29c..d04f3783 100644 --- a/web/src/routes/register/register.test.ts +++ b/web/src/routes/register/register.test.ts @@ -166,7 +166,7 @@ describe('/register page', () => { test('a new server restores a backup with the setup token, then sends you to sign in', async () => { (getSetupStatus as ReturnType).mockResolvedValue({ setup_required: true }); - (restoreBackup as ReturnType).mockResolvedValue({ accounts: 2, scan_started: true }); + (restoreBackup as ReturnType).mockResolvedValue({ accounts: 2, restarting: true }); render(RegisterPage); await fireEvent.click(await screen.findByRole('button', { name: /restore a backup/i })); expect(screen.getByRole('heading', { name: /restore a backup/i })).toBeInTheDocument(); @@ -179,8 +179,8 @@ describe('/register page', () => { await waitFor(() => expect(restoreBackup).toHaveBeenCalledWith(file, '0123abcd')); const done = await screen.findByTestId('restore-done'); - expect(done.textContent).toMatch(/Restored 2 accounts\. Sign in with the password you had\./); - expect(done.textContent).toMatch(/library scan has started/); + expect(done.textContent).toMatch(/Restored 2 accounts\.\s+Minstrel is restarting to load them; in a few seconds, sign in with the password you had\./); + expect(done.textContent).toMatch(/library scan finds each music file/); expect(within(done).getByRole('link', { name: /^sign in$/i })).toHaveAttribute('href', '/login'); });