From ae2d3ff41fb9d5b9cdeedbb2cc391daf532932e6 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:51:41 +0000 Subject: [PATCH] Commit guest metadata with configuration recovery Stage guest metadata with the archive's settings and credentials, publish the live cache only after a successful commit, and serialize it with pending identity snapshots. Restore newly created targets and an original moved aside before a failed replacement. Preserve normalization, scope, archive compatibility and ordinary metadata-writer ownership. Contract-Neutral: Repairs the existing all-or-nothing configuration recovery behaviour; archive formats, API response shape, tenant authority and normal metadata mutation contracts are unchanged. Change-source: pulse-maintainer --- internal/config/export.go | 7 +- internal/config/guest_metadata_import.go | 52 +++++++ internal/config/guest_metadata_import_test.go | 134 ++++++++++++++++++ internal/config/import_transaction.go | 11 +- 4 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 internal/config/guest_metadata_import.go create mode 100644 internal/config/guest_metadata_import_test.go diff --git a/internal/config/export.go b/internal/config/export.go index 26da1b366d..23c453f22d 100644 --- a/internal/config/export.go +++ b/internal/config/export.go @@ -258,16 +258,11 @@ func (c *ConfigPersistence) ImportConfig(encryptedData string, passphrase string } } - if err := tx.Commit(); err != nil { + if err := c.GetGuestMetadataStore().commitImport(tx, exportData.GuestMetadata); err != nil { return fmt.Errorf("failed to commit import transaction: %w", err) } committed = true - // Import guest metadata into the active persistence scope. - if err := c.GetGuestMetadataStore().ReplaceAll(exportData.GuestMetadata); err != nil { - log.Warn().Err(err).Msg("Failed to import guest metadata") - } - return nil } diff --git a/internal/config/guest_metadata_import.go b/internal/config/guest_metadata_import.go new file mode 100644 index 0000000000..afd4b8bd6d --- /dev/null +++ b/internal/config/guest_metadata_import.go @@ -0,0 +1,52 @@ +package config + +import ( + "encoding/json" + "fmt" + "path/filepath" +) + +// commitImport includes guest metadata in the same file transaction as settings +// and credentials. Keep the store's writer and mutation locks through commit so +// an older background snapshot cannot overwrite the imported file, and readers +// cannot observe imported metadata before its configuration has committed. +func (s *GuestMetadataStore) commitImport(tx *importTransaction, metadata map[string]*GuestMetadata) error { + s.writeMu.Lock() + defer s.writeMu.Unlock() + s.mu.Lock() + defer s.mu.Unlock() + if s.closed.Load() { + return ErrGuestMetadataStoreClosed + } + + working := make(map[string]*GuestMetadata, len(metadata)) + for id, meta := range metadata { + if meta == nil { + continue + } + clone := cloneGuestMetadata(meta) + clone.ID = id + if clone.Tags == nil { + clone.Tags = []string{} + } + working[id] = clone + } + data, err := json.Marshal(working) + if err != nil { + return fmt.Errorf("serialize imported guest metadata: %w", err) + } + if err := tx.StageFile(filepath.Join(s.dataPath, "guest_metadata.json"), data, 0600); err != nil { + return fmt.Errorf("stage imported guest metadata: %w", err) + } + if err := tx.Commit(); err != nil { + return err + } + + // Publish only after every file committed. A pending writer will now see + // that this revision is durable instead of replaying a pre-import snapshot. + s.metadata = working + s.revision++ + s.attemptedRevision = s.revision + s.lastWriteErr = nil + return nil +} diff --git a/internal/config/guest_metadata_import_test.go b/internal/config/guest_metadata_import_test.go new file mode 100644 index 0000000000..433b51ba88 --- /dev/null +++ b/internal/config/guest_metadata_import_test.go @@ -0,0 +1,134 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "reflect" + "sync" + "testing" + "time" +) + +type importBlockedMetadataFS struct { + FileSystem + once sync.Once + entered chan struct{} + release chan struct{} +} + +func (f *importBlockedMetadataFS) WriteFile(path string, data []byte, perm os.FileMode) error { + if filepath.Base(path) == "guest_metadata.json.tmp" { + f.once.Do(func() { close(f.entered); <-f.release }) + } + return f.FileSystem.WriteFile(path, data, perm) +} + +func TestGuestMetadataImportOrdersPendingSnapshotsAndCopiesInput(t *testing.T) { + dir := t.TempDir() + fs := &importBlockedMetadataFS{FileSystem: defaultFileSystem{}, entered: make(chan struct{}), release: make(chan struct{})} + store := NewGuestMetadataStore(dir, fs) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(fs.release) }) } + t.Cleanup(func() { + release() + if err := store.Close(time.Second); err != nil { + t.Error(err) + } + }) + if err := store.SetAsync("old:101", &GuestMetadata{Description: "Old snapshot"}); err != nil { + t.Fatal(err) + } + select { + case <-fs.entered: + case <-time.After(time.Second): + t.Fatal("pending metadata writer did not enter") + } + if err := store.RememberIdentity("old:101", "Old guest", "qemu"); err != nil { + t.Fatal(err) + } + tx, err := newImportTransaction(dir) + if err != nil { + t.Fatal(err) + } + defer tx.Cleanup() + settings := filepath.Join(dir, "system.json") + if err := os.WriteFile(settings, []byte("old settings"), 0600); err != nil { + t.Fatal(err) + } + if err := tx.StageFile(settings, []byte("imported settings"), 0600); err != nil { + t.Fatal(err) + } + metadata := map[string]*GuestMetadata{"new:202": {ID: "wrong", Description: "Imported snapshot", Tags: []string{"imported"}, Notes: []string{"note"}}} + done := make(chan error, 1) + go func() { done <- store.commitImport(tx, metadata) }() + select { + case err := <-done: + t.Fatalf("import overtook an in-flight metadata snapshot: %v", err) + case <-time.After(50 * time.Millisecond): + // A bounded negative observation, followed by the positive joined proof. + } + if data, err := os.ReadFile(settings); err != nil || string(data) != "old settings" { + t.Fatal("configuration committed while the old metadata write was still in flight") + } + release() + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(5 * time.Second): + t.Fatal("import did not complete after the prior snapshot") + } + if !store.WaitForPendingWrites(time.Second) { + t.Fatal("pending writer did not drain") + } + expected := map[string]*GuestMetadata{"new:202": {ID: "new:202", Description: "Imported snapshot", Tags: []string{"imported"}, Notes: []string{"note"}}} + metadata["new:202"].Tags[0] = "caller mutation" + metadata["new:202"].Notes[0] = "caller mutation" + if !reflect.DeepEqual(store.GetAll(), expected) { + t.Fatal("import retained old records or caller-owned slices") + } + data, err := os.ReadFile(filepath.Join(dir, "guest_metadata.json")) + if err != nil { + t.Fatal(err) + } + var persisted map[string]*GuestMetadata + if err := json.Unmarshal(data, &persisted); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(persisted, expected) { + t.Fatal("pending snapshot overwrote the imported metadata file") + } + if data, err := os.ReadFile(settings); err != nil || string(data) != "imported settings" { + t.Fatal("metadata import did not commit its settings") + } + if err := store.Load(); err != nil { + t.Fatalf("committed revision was left pending: %v", err) + } +} + +func TestGuestMetadataImportStageFailurePreservesMemory(t *testing.T) { + cp := seedImportValidationState(t) + store := cp.GetGuestMetadataStore() + before := store.GetAll() + tx, err := newImportTransaction(cp.configDir) + if err != nil { + t.Fatal(err) + } + // An occupied staging pathname is an actual filesystem failure before + // commit, not a simulated successful persistence result. + if err := os.Remove(tx.stagingDir); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(tx.stagingDir, []byte("block staging"), 0600); err != nil { + t.Fatal(err) + } + defer tx.Cleanup() + if err := store.commitImport(tx, map[string]*GuestMetadata{"new:202": {Description: "Do not publish"}}); err == nil { + t.Fatal("metadata staging failure was accepted") + } + if !reflect.DeepEqual(before, store.GetAll()) || tx.committed { + t.Fatal("staging failure changed live metadata or committed configuration") + } +} diff --git a/internal/config/import_transaction.go b/internal/config/import_transaction.go index 81949b9e0e..d8d85fe2c8 100644 --- a/internal/config/import_transaction.go +++ b/internal/config/import_transaction.go @@ -170,6 +170,12 @@ func (tx *importTransaction) Commit() error { continue } tx.backups[target] = "" + } else { + // This target did not exist before the import. Restoring its + // absence matters just as much as restoring an overwritten file. + if err := os.Remove(target); err != nil && !os.IsNotExist(err) { + joinRestoreErr(fmt.Errorf("remove new target %s during restore: %w", target, err)) + } } } return restoreErr @@ -203,11 +209,12 @@ func (tx *importTransaction) Commit() error { return failWithRestore(fmt.Errorf("stat destination %s: %w", target, err)) } + // Track the target before replacement: a failed rename after moving + // the original into a backup must restore that original too. + applied = append(applied, target) if err := os.Rename(stagedPath, target); err != nil { return failWithRestore(fmt.Errorf("apply staged file to %s: %w", target, err)) } - - applied = append(applied, target) } // Successful commit: remove backups. Keep success behavior, but surface cleanup failures in logs.