From bcc869b0ca47e7c7dc1a2ccdaed488e0a0a3764b Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 8 Sep 2026 11:17:51 +0100 Subject: [PATCH] fix(alerts): avoid unchanged checkpoint directory chmod The JSON mirror fast path avoided file replacement but still dirtied directory metadata on every save through chmod. Check the current permissions before repairing them, retaining correction of unsafe modes. A Linux regression reproduces the ctime change and verifies unchanged metadata and permission repair. This is a narrow contributor to #1966, not a claim that aggregate write amplification is resolved. Change-source: pulse-maintainer --- .../v6/internal/subsystems/alerts.md | 10 ++++ .../active_mirror_metadata_linux_test.go | 51 +++++++++++++++++++ internal/alerts/active_persistence.go | 12 ++++- internal/alerts/alerts_test.go | 42 +++++++++++++++ 4 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 internal/alerts/active_mirror_metadata_linux_test.go diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 3c7a2add5..38fbadbed 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1294,6 +1294,16 @@ synchronous durability for this authority, and the recovery mirror fsyncs its temporary file plus a platform-native durable rename barrier (parent-directory sync on Unix and write-through replacement on Windows), so the contract covers host power loss rather than only orderly process restart. +An unchanged JSON recovery checkpoint preserves already-correct directory +permissions without issuing chmod, avoiding redundant Linux directory metadata +mutation. It still repairs unsafe access permissions and special mode bits; +permission repair must not replace identical JSON. Directory sync and atomic +file replacement durability remain unchanged. This is not a guarantee of zero +aggregate process writes. `TestActiveMirrorUnchangedRepairsDirectoryPermissions` +in `internal/alerts/alerts_test.go` pins permission repair and inode retention; +`TestActiveMirrorUnchangedPreservesDirectoryMetadata` in +`internal/alerts/active_mirror_metadata_linux_test.go` pins stable Linux ctime. + `active-alerts.json` remains an atomic recovery mirror, not a competing healthy read authority. A new or recreated database imports the readable mirror. A failed SQLite checkpoint writes a durable degraded marker, and the next startup diff --git a/internal/alerts/active_mirror_metadata_linux_test.go b/internal/alerts/active_mirror_metadata_linux_test.go new file mode 100644 index 000000000..58ff17c48 --- /dev/null +++ b/internal/alerts/active_mirror_metadata_linux_test.go @@ -0,0 +1,51 @@ +//go:build linux + +package alerts + +import ( + "os" + "syscall" + "testing" + "time" +) + +// Skipping the JSON rename must not dirty the containing directory's metadata +// through an unconditional chmod on every checkpoint. +func TestActiveMirrorUnchangedPreservesDirectoryMetadata(t *testing.T) { + dir := t.TempDir() + m := &Manager{alertsDir: dir} + alerts := []*Alert{{ID: "a"}} + save := func() { + t.Helper() + if err := m.writeActiveAlertsRecoveryMirror(alerts); err != nil { + t.Fatal(err) + } + } + ctime := func() syscall.Timespec { + t.Helper() + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + return info.Sys().(*syscall.Stat_t).Ctim + } + save() + before := ctime() + time.Sleep(20 * time.Millisecond) + save() + if after := ctime(); after != before { + t.Fatalf("unchanged checkpoint changed directory ctime: %v -> %v", before, after) + } + // Still repair a directory made accessible to other users. + if err := os.Chmod(dir, 0755); err != nil { + t.Fatal(err) + } + save() + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != alertsDirPerm { + t.Fatalf("directory mode = %v", info.Mode()) + } +} diff --git a/internal/alerts/active_persistence.go b/internal/alerts/active_persistence.go index 1d4b686f6..77118f78b 100644 --- a/internal/alerts/active_persistence.go +++ b/internal/alerts/active_persistence.go @@ -160,8 +160,16 @@ func (m *Manager) writeActiveAlertsRecoveryMirrorLocked(alerts []*Alert) error { if err := os.MkdirAll(alertsDir, alertsDirPerm); err != nil { return fmt.Errorf("failed to create alerts directory: %w", err) } - if err := os.Chmod(alertsDir, alertsDirPerm); err != nil { - return fmt.Errorf("failed to set alerts directory permissions: %w", err) + // Even chmod to the existing mode dirties directory metadata on Linux. + // Preserve the no-change checkpoint path while still repairing permissions. + info, err := os.Stat(alertsDir) + if err != nil { + return fmt.Errorf("failed to stat alerts directory: %w", err) + } + if info.Mode().Perm() != alertsDirPerm || info.Mode()&(os.ModeSetuid|os.ModeSetgid|os.ModeSticky) != 0 { + if err := os.Chmod(alertsDir, alertsDirPerm); err != nil { + return fmt.Errorf("failed to set alerts directory permissions: %w", err) + } } // Snapshots originate from maps. Canonicalise the complete records rather diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 41d66feaa..bf72356b7 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -21280,3 +21280,45 @@ func TestBackupDivergentDefaultsOnLoad(t *testing.T) { }) } } + +// The unchanged-content fast path must not accept unsafe directory permissions. +func TestActiveMirrorUnchangedRepairsDirectoryPermissions(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX directory permissions") + } + for _, mode := range []os.FileMode{0755, 0700 | os.ModeSticky, 0700 | os.ModeSetgid} { + t.Run(mode.String(), func(t *testing.T) { + dir := t.TempDir() + m := &Manager{alertsDir: dir} + alerts := []*Alert{{ID: "a"}} + if err := m.writeActiveAlertsRecoveryMirror(alerts); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, "active-alerts.json") + before, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if err := os.Chmod(dir, mode); err != nil { + t.Fatal(err) + } + if err := m.writeActiveAlertsRecoveryMirror(alerts); err != nil { + t.Fatal(err) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm() != alertsDirPerm || info.Mode()&(os.ModeSetuid|os.ModeSetgid|os.ModeSticky) != 0 { + t.Fatalf("unsafe directory mode survived: %v", info.Mode()) + } + after, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if !os.SameFile(before, after) { + t.Fatal("directory permission repair replaced unchanged JSON") + } + }) + } +}