diff --git a/.github/workflows/dependabot-automerge.yml b/.github/workflows/dependabot-automerge.yml index a4676472..212d2cf3 100644 --- a/.github/workflows/dependabot-automerge.yml +++ b/.github/workflows/dependabot-automerge.yml @@ -17,7 +17,7 @@ jobs: steps: - name: Set up Node.js 24 - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # pinned from actions/setup-node@v4 + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # pinned from actions/setup-node@v6.4.0 with: node-version: '24' diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml index 3a2f2f4b..bbf876d8 100644 --- a/.github/workflows/dependency-review.yml +++ b/.github/workflows/dependency-review.yml @@ -21,7 +21,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd - name: Dependency Review - uses: actions/dependency-review-action@2031cfc080254a8a887f58cffee85186f0e49e48 + uses: actions/dependency-review-action@a1d282b36b6f3519aa1f3fc636f609c47dddb294 # pinned from actions/dependency-review-action@v5.0.0 with: # Blocca solo severity critical (zero-touch per gli altri) fail-on-severity: critical diff --git a/.github/workflows/security-ultimate.yml b/.github/workflows/security-ultimate.yml index 18fe733c..ec022fd7 100644 --- a/.github/workflows/security-ultimate.yml +++ b/.github/workflows/security-ultimate.yml @@ -88,7 +88,7 @@ jobs: # UPLOAD SARIF ######################################## - name: Upload GoSec SARIF - uses: github/codeql-action/upload-sarif@68bde559dea0fdcac2102bfdf6230c5f70eb485e + uses: github/codeql-action/upload-sarif@9e0d7b8d25671d64c341c19c0152d693099fb5ba with: sarif_file: gosec.sarif @@ -104,7 +104,7 @@ jobs: # CODEQL ######################################## - name: Initialize CodeQL - uses: github/codeql-action/init@68bde559dea0fdcac2102bfdf6230c5f70eb485e + uses: github/codeql-action/init@9e0d7b8d25671d64c341c19c0152d693099fb5ba with: languages: go @@ -114,4 +114,4 @@ jobs: go build ./... - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@68bde559dea0fdcac2102bfdf6230c5f70eb485e + uses: github/codeql-action/analyze@9e0d7b8d25671d64c341c19c0152d693099fb5ba diff --git a/cmd/proxsave/base_dir_test.go b/cmd/proxsave/base_dir_test.go index 4d29adca..683aecfc 100644 --- a/cmd/proxsave/base_dir_test.go +++ b/cmd/proxsave/base_dir_test.go @@ -12,13 +12,13 @@ func forceDetectedBaseDirForTest(t *testing.T, baseDir string) { origOnce := execInfoOnce execPath := filepath.Join(baseDir, "build", "proxsave") - execInfo = ExecInfo{ + execInfo = &ExecInfo{ ExecPath: execPath, ExecDir: filepath.Dir(execPath), BaseDir: baseDir, HasBase: true, } - execInfoOnce = sync.Once{} + execInfoOnce = &sync.Once{} execInfoOnce.Do(func() {}) t.Cleanup(func() { diff --git a/cmd/proxsave/install.go b/cmd/proxsave/install.go index 99ae3e09..e77cda77 100644 --- a/cmd/proxsave/install.go +++ b/cmd/proxsave/install.go @@ -453,7 +453,6 @@ func runConfigWizardCLI(ctx context.Context, reader *bufio.Reader, configPath, t if skipConfigWizard { return installConfigResult{SkipConfigWizard: true}, nil } - template = config.RemoveRuntimeDerivedEnvKeys(template) logging.DebugStepBootstrap(bootstrap, "install config wizard (cli)", "configuring secondary storage") if template, err = configureSecondaryStorage(ctx, reader, template); err != nil { diff --git a/cmd/proxsave/runtime_helpers.go b/cmd/proxsave/runtime_helpers.go index 9c04fa55..feb6ab61 100644 --- a/cmd/proxsave/runtime_helpers.go +++ b/cmd/proxsave/runtime_helpers.go @@ -32,15 +32,16 @@ type ExecInfo struct { } var ( - execInfo ExecInfo - execInfoOnce sync.Once + execInfo = &ExecInfo{} + execInfoOnce = &sync.Once{} ) func getExecInfo() ExecInfo { execInfoOnce.Do(func() { - execInfo = detectExecInfo() + info := detectExecInfo() + execInfo = &info }) - return execInfo + return *execInfo } func detectExecInfo() ExecInfo { diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 12e63a1a..2bb42f3b 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -865,7 +865,7 @@ If `EMAIL_ENABLED` is omitted, the default remains `false`. The legacy alias `EM | `sendmail` | The node already has a local MTA such as Postfix, Exim, or Sendmail. | In the local MTA. ProxSave calls `/usr/sbin/sendmail`. | | `pmf` | You explicitly want Proxmox Notifications via `proxmox-mail-forward`. | In Proxmox (`Datacenter -> Notifications` on PVE, or the PBS notification UI/config). ProxSave does not ask for SMTP host/port/user/password. | -`pmf` may also be written as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`. +The value `pmf` may also be written as `proxmox`, `proxmox-notifications`, or `proxmox-mail-forward`; ProxSave normalizes those aliases to `pmf`. **Notes**: - Allowed values for `EMAIL_DELIVERY_METHOD` are: `pmf`, `relay`, `sendmail` (invalid values will skip Email with a warning). diff --git a/go.mod b/go.mod index 3016aaff..675faca5 100644 --- a/go.mod +++ b/go.mod @@ -6,9 +6,9 @@ require ( filippo.io/age v1.3.1 github.com/gdamore/tcell/v2 v2.13.9 github.com/rivo/tview v0.42.0 - golang.org/x/crypto v0.50.0 - golang.org/x/term v0.42.0 - golang.org/x/text v0.36.0 + golang.org/x/crypto v0.51.0 + golang.org/x/term v0.43.0 + golang.org/x/text v0.37.0 ) require ( @@ -17,5 +17,5 @@ require ( github.com/gdamore/encoding v1.0.1 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect - golang.org/x/sys v0.43.0 // indirect + golang.org/x/sys v0.44.0 // indirect ) diff --git a/go.sum b/go.sum index ca48a79f..f7e152f1 100644 --- a/go.sum +++ b/go.sum @@ -19,8 +19,8 @@ github.com/rivo/uniseg v0.4.7/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUc github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/crypto v0.50.0 h1:zO47/JPrL6vsNkINmLoo/PH1gcxpls50DNogFvB5ZGI= -golang.org/x/crypto v0.50.0/go.mod h1:3muZ7vA7PBCE6xgPX7nkzzjiUq87kRItoJQM1Yo8S+Q= +golang.org/x/crypto v0.51.0 h1:IBPXwPfKxY7cWQZ38ZCIRPI50YLeevDLlLnyC5wRGTI= +golang.org/x/crypto v0.51.0/go.mod h1:8AdwkbraGNABw2kOX6YFPs3WM22XqI4EXEd8g+x7Oc8= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -36,20 +36,20 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.43.0 h1:Rlag2XtaFTxp19wS8MXlJwTvoh8ArU6ezoyFsMyCTNI= -golang.org/x/sys v0.43.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= +golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= -golang.org/x/term v0.42.0 h1:UiKe+zDFmJobeJ5ggPwOshJIVt6/Ft0rcfrXZDLWAWY= -golang.org/x/term v0.42.0/go.mod h1:Dq/D+snpsbazcBG5+F9Q1n2rXV8Ma+71xEjTRufARgY= +golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= +golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= -golang.org/x/text v0.36.0 h1:JfKh3XmcRPqZPKevfXVpI1wXPTqbkE5f7JA92a55Yxg= -golang.org/x/text v0.36.0/go.mod h1:NIdBknypM8iqVmPiuco0Dh6P5Jcdk8lJL0CUebqK164= +golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= +golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= diff --git a/internal/backup/archiver.go b/internal/backup/archiver.go index 8f9a7691..3dc16b23 100644 --- a/internal/backup/archiver.go +++ b/internal/backup/archiver.go @@ -726,7 +726,7 @@ func (a *Archiver) attachStderrLogger(cmd *exec.Cmd, algo string) error { func drainTarWriterAfterCompressorStartFailure(pw *io.PipeWriter, errChan <-chan error, startErr error) { _ = pw.CloseWithError(startErr) - _ = <-errChan + <-errChan } func (a *Archiver) pipeTarThroughCommand(ctx context.Context, sourceDir, outputPath string, cmd *exec.Cmd, algo string) (err error) { diff --git a/internal/notify/email.go b/internal/notify/email.go index 16d3b458..5f1c002b 100644 --- a/internal/notify/email.go +++ b/internal/notify/email.go @@ -197,8 +197,8 @@ func (e *EmailNotifier) Send(ctx context.Context, data *NotificationData) (*Noti detectedRecipient, err := e.detectRecipient(ctx) if err != nil { e.logger.Warning("WARNING: Failed to detect email recipient: %v", err) - switch { - case e.config.DeliveryMethod == EmailDeliveryPMF: + switch e.config.DeliveryMethod { + case EmailDeliveryPMF: e.logger.Info(" Proceeding anyway because EMAIL_DELIVERY_METHOD=pmf routes via Proxmox Notifications; recipient is only used for the To: header") default: e.logger.Warning("WARNING: Email notification skipped because no valid recipient is available") @@ -218,8 +218,8 @@ func (e *EmailNotifier) Send(ctx context.Context, data *NotificationData) (*Noti recipient = strings.TrimSpace(recipient) if recipient == "" { - switch { - case e.config.DeliveryMethod == EmailDeliveryRelay || e.config.DeliveryMethod == EmailDeliverySendmail: + switch e.config.DeliveryMethod { + case EmailDeliveryRelay, EmailDeliverySendmail: e.logger.Warning("WARNING: Email recipient is empty after configuration/detection") e.logger.Info(" Configure EMAIL_RECIPIENT or set an email address for root@pam inside Proxmox") result.Success = false diff --git a/internal/orchestrator/additional_helpers_test.go b/internal/orchestrator/additional_helpers_test.go index 905ce07a..056369d9 100644 --- a/internal/orchestrator/additional_helpers_test.go +++ b/internal/orchestrator/additional_helpers_test.go @@ -597,8 +597,14 @@ func TestFinalizeAndCloseLogWithoutLogFile(t *testing.T) { } type stubNotifierChannel struct { - name string - called bool + name string + called bool + logger *logging.Logger + warnOnNotify bool + errorCount int + warningCount int + categoryCount int + exitCode int } func (s *stubNotifierChannel) Name() string { @@ -607,6 +613,26 @@ func (s *stubNotifierChannel) Name() string { func (s *stubNotifierChannel) Notify(ctx context.Context, stats *BackupStats) error { s.called = true + if stats != nil { + s.errorCount = stats.ErrorCount + s.warningCount = stats.WarningCount + s.categoryCount = len(stats.LogCategories) + s.exitCode = stats.ExitCode + } + if s.warnOnNotify && s.logger != nil { + s.logger.Warning("%s notification warning after snapshot", s.name) + } + return nil +} + +type warningStorageTarget struct { + logger *logging.Logger +} + +func (w *warningStorageTarget) Sync(ctx context.Context, stats *BackupStats) error { + if w.logger != nil { + w.logger.Warning("storage warning before notification snapshot") + } return nil } @@ -721,6 +747,76 @@ func TestDispatchNotificationsUsesNameMappingNotRegistrationOrder(t *testing.T) } } +func TestDispatchPostBackupSnapshotsIssuesImmediatelyBeforeNotifications(t *testing.T) { + logger := logging.New(types.LogLevelInfo, false) + var buf bytes.Buffer + logger.SetOutput(&buf) + + logPath := filepath.Join(t.TempDir(), "run.log") + if err := logger.OpenLogFile(logPath); err != nil { + t.Fatalf("OpenLogFile: %v", err) + } + t.Cleanup(func() { + _ = logger.CloseLogFile() + }) + + email := &stubNotifierChannel{name: "Email", logger: logger, warnOnNotify: true} + telegram := &stubNotifierChannel{name: "Telegram"} + o := &Orchestrator{ + logger: logger, + cfg: &config.Config{ + EmailEnabled: true, + TelegramEnabled: true, + GotifyEnabled: false, + WebhookEnabled: false, + }, + storageTargets: []StorageTarget{ + &warningStorageTarget{logger: logger}, + }, + notificationChannels: []NotificationChannel{ + email, + telegram, + }, + } + stats := &BackupStats{ + LogFilePath: logPath, + SecondaryEnabled: true, + SecondaryStatus: "ok", + CloudStatus: "disabled", + EmailStatus: "unknown", + TelegramStatus: "unknown", + } + + if err := o.dispatchPostBackup(context.Background(), stats); err != nil { + t.Fatalf("dispatchPostBackup returned error: %v", err) + } + + if !email.called || !telegram.called { + t.Fatalf("expected both notifiers to be called (email=%v telegram=%v)", email.called, telegram.called) + } + if email.warningCount != 1 || telegram.warningCount != 1 { + t.Fatalf("notifiers should see the same pre-notification warning snapshot, got email=%d telegram=%d", email.warningCount, telegram.warningCount) + } + if email.errorCount != 0 || telegram.errorCount != 0 { + t.Fatalf("notifiers should not see errors, got email=%d telegram=%d", email.errorCount, telegram.errorCount) + } + if email.categoryCount == 0 || telegram.categoryCount == 0 || len(stats.LogCategories) == 0 { + t.Fatalf("expected pre-notification log categories to be snapshotted") + } + if stats.WarningCount != 1 { + t.Fatalf("stats.WarningCount should remain at the pre-notification snapshot, got %d", stats.WarningCount) + } + if stats.ExitCode != types.ExitGenericError.Int() { + t.Fatalf("stats.ExitCode should reflect pre-notification warnings, got %d", stats.ExitCode) + } + if email.exitCode != types.ExitGenericError.Int() || telegram.exitCode != types.ExitGenericError.Int() { + t.Fatalf("notifiers should see warning exit code, got email=%d telegram=%d", email.exitCode, telegram.exitCode) + } + if got := logger.WarningCount(); got != 2 { + t.Fatalf("final logger warning count should include storage and notification warnings, got %d", got) + } +} + func TestLogRetentionPolicyDetailsSimpleVsGFS(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) var buf bytes.Buffer diff --git a/internal/orchestrator/backup_run_helpers.go b/internal/orchestrator/backup_run_helpers.go index 32103742..e05631d3 100644 --- a/internal/orchestrator/backup_run_helpers.go +++ b/internal/orchestrator/backup_run_helpers.go @@ -60,11 +60,9 @@ func (o *Orchestrator) parseFailedBackupLogCounts(stats *BackupStats) { } o.logger.Debug("Parsing log file for error/warning counts after failure: %s", stats.LogFilePath) - _, errorCount, warningCount := ParseLogCounts(stats.LogFilePath, 0) - stats.ErrorCount = errorCount - stats.WarningCount = warningCount - if errorCount > 0 || warningCount > 0 { - o.logger.Debug("Found %d errors and %d warnings in log file (failure path)", errorCount, warningCount) + o.refreshLogIssuesFromFile(stats, false) + if stats.ErrorCount > 0 || stats.WarningCount > 0 { + o.logger.Debug("Found %d errors and %d warnings in log file (failure path)", stats.ErrorCount, stats.WarningCount) } } diff --git a/internal/orchestrator/backup_run_phases.go b/internal/orchestrator/backup_run_phases.go index e21e88af..b44b9b10 100644 --- a/internal/orchestrator/backup_run_phases.go +++ b/internal/orchestrator/backup_run_phases.go @@ -339,26 +339,25 @@ func (o *Orchestrator) finalizeBackupStats(run *backupRunContext) { stats := run.stats stats.Duration = stats.EndTime.Sub(stats.StartTime) - if stats.LogFilePath != "" { - o.logger.Debug("Parsing log file for error/warning counts: %s", stats.LogFilePath) - _, errorCount, warningCount := ParseLogCounts(stats.LogFilePath, 0) - stats.ErrorCount = errorCount - stats.WarningCount = warningCount - if errorCount > 0 || warningCount > 0 { - o.logger.Debug("Found %d errors and %d warnings in log file", errorCount, warningCount) - } - } else { + if o.dryRun { + o.finalizeDryRunIssueStats(stats) + } +} + +func (o *Orchestrator) finalizeDryRunIssueStats(stats *BackupStats) { + if stats.LogFilePath == "" { o.logger.Debug("No log file path specified, error/warning counts will be 0") + applyIssueExitCode(stats) + o.logger.Debug("Aggregated exit code based on log analysis: %d", stats.ExitCode) + return } - switch { - case stats.ErrorCount > 0: - stats.ExitCode = types.ExitBackupError.Int() - case stats.WarningCount > 0: - stats.ExitCode = types.ExitGenericError.Int() - default: - stats.ExitCode = types.ExitSuccess.Int() + o.logger.Debug("Parsing log file for error/warning counts: %s", stats.LogFilePath) + o.refreshLogIssuesFromFile(stats, false) + if stats.ErrorCount > 0 || stats.WarningCount > 0 { + o.logger.Debug("Found %d errors and %d warnings in log file", stats.ErrorCount, stats.WarningCount) } + applyIssueExitCode(stats) o.logger.Debug("Aggregated exit code based on log analysis: %d", stats.ExitCode) } diff --git a/internal/orchestrator/encryption_exported_test.go b/internal/orchestrator/encryption_exported_test.go index 6ad2b956..c03a21ae 100644 --- a/internal/orchestrator/encryption_exported_test.go +++ b/internal/orchestrator/encryption_exported_test.go @@ -251,7 +251,11 @@ func TestRunAgeSetupWizard_ExitReturnsAborted(t *testing.T) { if err != nil { t.Fatalf("open stdin: %v", err) } - defer f.Close() + t.Cleanup(func() { + if err := f.Close(); err != nil { + t.Errorf("close stdin fixture: %v", err) + } + }) origIn := os.Stdin t.Cleanup(func() { os.Stdin = origIn }) @@ -279,7 +283,11 @@ func TestRunAgeSetupWizard_Option1WritesFile(t *testing.T) { if err != nil { t.Fatalf("open stdin: %v", err) } - defer f.Close() + t.Cleanup(func() { + if err := f.Close(); err != nil { + t.Errorf("close stdin fixture: %v", err) + } + }) origIn := os.Stdin t.Cleanup(func() { os.Stdin = origIn }) diff --git a/internal/orchestrator/extensions.go b/internal/orchestrator/extensions.go index df5c46a8..619fc504 100644 --- a/internal/orchestrator/extensions.go +++ b/internal/orchestrator/extensions.go @@ -115,6 +115,57 @@ func (o *Orchestrator) dispatchNotifications(ctx context.Context, stats *BackupS } } +// startNotificationGroup is the notification boundary. Keep the issue snapshot +// immediately adjacent to dispatchNotifications; no logging belongs between them. +func (o *Orchestrator) startNotificationGroup(ctx context.Context, stats *BackupStats) { + if o == nil { + return + } + o.snapshotPreNotificationIssues(stats) + applyIssueExitCode(stats) + o.dispatchNotifications(ctx, stats) +} + +func (o *Orchestrator) snapshotPreNotificationIssues(stats *BackupStats) { + o.refreshLogIssuesFromFile(stats, true) +} + +func (o *Orchestrator) refreshLogIssuesFromFile(stats *BackupStats, includeCategories bool) { + if stats == nil || strings.TrimSpace(stats.LogFilePath) == "" { + return + } + + categoryLimit := 0 + if includeCategories { + categoryLimit = 10 + } + categories, errorCount, warningCount := ParseLogCounts(stats.LogFilePath, categoryLimit) + stats.ErrorCount = errorCount + stats.WarningCount = warningCount + if includeCategories { + stats.LogCategories = categories + } else { + stats.LogCategories = nil + } +} + +func applyIssueExitCode(stats *BackupStats) { + if stats == nil { + return + } + + if stats.ErrorCount > 0 { + if stats.ExitCode == types.ExitSuccess.Int() || stats.ExitCode == types.ExitGenericError.Int() { + stats.ExitCode = types.ExitBackupError.Int() + } + return + } + + if stats.WarningCount > 0 && stats.ExitCode == types.ExitSuccess.Int() { + stats.ExitCode = types.ExitGenericError.Int() + } +} + // DispatchEarlyErrorNotification sends notifications for errors that occurred before backup started // This creates a minimal BackupStats with error information for notification purposes func (o *Orchestrator) DispatchEarlyErrorNotification(ctx context.Context, earlyErr *EarlyErrorState) *BackupStats { @@ -179,7 +230,8 @@ func (o *Orchestrator) DispatchEarlyErrorNotification(ctx context.Context, early stats.LogFilePath = logPath } - // Dispatch notifications with minimal stats + // Dispatch notifications with minimal stats. Early errors are already + // represented in stats and may not be present in the log file yet. o.dispatchNotifications(ctx, stats) return stats @@ -208,7 +260,7 @@ func (o *Orchestrator) dispatchNotificationsAndLogs(ctx context.Context, stats * // Notification errors are logged but never propagated fmt.Println() o.logStep(7, "Notifications - dispatching channels") - o.dispatchNotifications(ctx, stats) + o.startNotificationGroup(ctx, stats) } func (o *Orchestrator) dispatchPostBackup(ctx context.Context, stats *BackupStats) error { diff --git a/internal/orchestrator/extensions_additional_test.go b/internal/orchestrator/extensions_additional_test.go index 61e90df5..8b0dc804 100644 --- a/internal/orchestrator/extensions_additional_test.go +++ b/internal/orchestrator/extensions_additional_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "path/filepath" "strings" "testing" "time" @@ -12,6 +13,80 @@ import ( "github.com/tis24dev/proxsave/internal/types" ) +func TestApplyIssueExitCode(t *testing.T) { + applyIssueExitCode(nil) + + tests := []struct { + name string + stats BackupStats + wantExit int + wantErrors int + wantWarns int + }{ + { + name: "no issues keeps success", + stats: BackupStats{ExitCode: types.ExitSuccess.Int()}, + wantExit: types.ExitSuccess.Int(), + }, + { + name: "warnings promote success to generic error", + stats: BackupStats{WarningCount: 1, ExitCode: types.ExitSuccess.Int()}, + wantExit: types.ExitGenericError.Int(), + wantWarns: 1, + }, + { + name: "warnings preserve specific failure", + stats: BackupStats{WarningCount: 1, ExitCode: types.ExitStorageError.Int()}, + wantExit: types.ExitStorageError.Int(), + wantWarns: 1, + }, + { + name: "errors promote success to backup error", + stats: BackupStats{ErrorCount: 1, ExitCode: types.ExitSuccess.Int()}, + wantExit: types.ExitBackupError.Int(), + wantErrors: 1, + }, + { + name: "errors promote generic error to backup error", + stats: BackupStats{ErrorCount: 1, ExitCode: types.ExitGenericError.Int()}, + wantExit: types.ExitBackupError.Int(), + wantErrors: 1, + }, + { + name: "errors preserve specific failure", + stats: BackupStats{ErrorCount: 1, ExitCode: types.ExitStorageError.Int()}, + wantExit: types.ExitStorageError.Int(), + wantErrors: 1, + }, + { + name: "errors take precedence over warnings", + stats: BackupStats{ErrorCount: 1, WarningCount: 1, ExitCode: types.ExitSuccess.Int()}, + wantExit: types.ExitBackupError.Int(), + wantErrors: 1, + wantWarns: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stats := tt.stats + applyIssueExitCode(&stats) + + if stats.ExitCode != tt.wantExit { + t.Fatalf("ExitCode=%d; want %d", stats.ExitCode, tt.wantExit) + } + if stats.ErrorCount != tt.wantErrors || stats.WarningCount != tt.wantWarns { + t.Fatalf("issue counts changed to errors=%d warnings=%d; want errors=%d warnings=%d", + stats.ErrorCount, + stats.WarningCount, + tt.wantErrors, + tt.wantWarns, + ) + } + }) + } +} + func TestDispatchEarlyErrorNotification_ReturnsNilWhenNoError(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) logger.SetOutput(io.Discard) @@ -77,3 +152,37 @@ func TestDispatchEarlyErrorNotification_PopulatesMinimalStats(t *testing.T) { t.Fatalf("Hostname is empty") } } + +func TestDispatchEarlyErrorNotificationPreservesSyntheticIssueWithLogFile(t *testing.T) { + logger := logging.New(types.LogLevelDebug, false) + logger.SetOutput(io.Discard) + logPath := filepath.Join(t.TempDir(), "early.log") + if err := logger.OpenLogFile(logPath); err != nil { + t.Fatalf("OpenLogFile: %v", err) + } + t.Cleanup(func() { + _ = logger.CloseLogFile() + }) + + orch := &Orchestrator{logger: logger} + early := &EarlyErrorState{ + Phase: "config", + Error: errors.New("boom"), + ExitCode: types.ExitConfigError, + Timestamp: time.Unix(1700000000, 0), + } + + stats := orch.DispatchEarlyErrorNotification(context.Background(), early) + if stats == nil { + t.Fatalf("expected stats, got nil") + } + if stats.ExitCode != types.ExitConfigError.Int() { + t.Fatalf("ExitCode=%d; want %d", stats.ExitCode, types.ExitConfigError.Int()) + } + if stats.ErrorCount != 1 { + t.Fatalf("ErrorCount=%d; want synthetic early error count to be preserved", stats.ErrorCount) + } + if stats.LogFilePath != logPath { + t.Fatalf("LogFilePath=%q; want %q", stats.LogFilePath, logPath) + } +} diff --git a/internal/orchestrator/mount_guard.go b/internal/orchestrator/mount_guard.go index a99f8c00..8b5c7dfe 100644 --- a/internal/orchestrator/mount_guard.go +++ b/internal/orchestrator/mount_guard.go @@ -34,7 +34,7 @@ func guardMountPoint(ctx context.Context, guardTarget string) error { if err != nil { return err } - mounted, err := ensureGuardTargetUnmounted(target) + mounted, err := isMounted(target) if err != nil { return fmt.Errorf("check mount status: %w", err) } @@ -63,14 +63,6 @@ func normalizeGuardMountRequest(ctx context.Context, guardTarget string) (string return target, nil } -func ensureGuardTargetUnmounted(target string) (bool, error) { - mounted, err := isMounted(target) - if err != nil { - return false, err - } - return mounted, nil -} - func ensureGuardDirectories(guardDir, target string) error { if err := mountGuardMkdirAll(guardDir, 0o755); err != nil { return fmt.Errorf("mkdir guard dir: %w", err) diff --git a/internal/orchestrator/notification_adapter.go b/internal/orchestrator/notification_adapter.go index d68fcbb8..e0f6c8f7 100644 --- a/internal/orchestrator/notification_adapter.go +++ b/internal/orchestrator/notification_adapter.go @@ -175,17 +175,13 @@ func (n *NotificationAdapter) convertBackupStatsToNotificationData(stats *Backup secondaryPercent = formatPercentString(calculateUsagePercent(stats.SecondaryUsedSpace, stats.SecondaryTotalSpace)) } - // Parse log file for categories - use ParseLogCounts as primary source - logCategories, logErrors, logWarnings := ParseLogCounts(stats.LogFilePath, 10) - - // Use parsed counts if available, otherwise fall back to stats + // Issue counts and categories are snapshotted immediately before the + // notification group starts, so all notifiers see the same pre-notification + // totals. Re-parsing the log here per-notifier would over-count warnings + // emitted by earlier notifiers in the dispatch chain. errorCount := stats.ErrorCount warningCount := stats.WarningCount - if logErrors > 0 || logWarnings > 0 { - errorCount = logErrors - warningCount = logWarnings - } - + logCategories := append([]notify.LogCategory(nil), stats.LogCategories...) totalIssues := errorCount + warningCount // Extract filename from full path for email display diff --git a/internal/orchestrator/notification_adapter_test.go b/internal/orchestrator/notification_adapter_test.go index fd648f1c..1a9256f6 100644 --- a/internal/orchestrator/notification_adapter_test.go +++ b/internal/orchestrator/notification_adapter_test.go @@ -312,20 +312,12 @@ func TestFormatHelpers(t *testing.T) { } } -func TestConvertBackupStatsUsesLogCountsAndCompressionFallback(t *testing.T) { +func TestConvertBackupStatsPropagatesIssueSnapshotAndCompressionFallback(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) adapter := NewNotificationAdapter(&stubNotifier{name: "Email", enabled: true}, logger) - // Prepare a temporary log file with known error/warning counts - tempDir := t.TempDir() - logFile := filepath.Join(tempDir, "notif.log") - content := `[2025-11-10 14:30:01] [WARNING] Something minor -[2025-11-10 14:30:02] [ERROR] Something bad -` - if err := os.WriteFile(logFile, []byte(content), 0644); err != nil { - t.Fatalf("failed to write log file: %v", err) - } - + // Issue counts and categories are snapshotted before the notification group; + // conversion must propagate them as-is without re-parsing the log file. stats := &BackupStats{ ExitCode: 0, Hostname: "host", @@ -337,20 +329,29 @@ func TestConvertBackupStatsUsesLogCountsAndCompressionFallback(t *testing.T) { CloudEnabled: false, MaxLocalBackups: 5, LocalRetentionPolicy: "simple", - LogFilePath: logFile, - ErrorCount: 0, - WarningCount: 0, - Compression: types.CompressionZstd, + ErrorCount: 1, + WarningCount: 1, + LogCategories: []notify.LogCategory{ + {Label: "Something minor", Type: "WARNING", Count: 1, Example: "Something minor"}, + {Label: "Something bad", Type: "ERROR", Count: 1, Example: "Something bad"}, + }, + Compression: types.CompressionZstd, } data := adapter.convertBackupStatsToNotificationData(stats) - // Log counts should override stats.ErrorCount/WarningCount if data.ErrorCount != 1 || data.WarningCount != 1 { - t.Fatalf("expected error/warning counts from log = 1/1; got %d/%d", data.ErrorCount, data.WarningCount) + t.Fatalf("expected error/warning counts from stats = 1/1; got %d/%d", data.ErrorCount, data.WarningCount) + } + if data.TotalIssues != 2 { + t.Fatalf("expected TotalIssues = 2; got %d", data.TotalIssues) } - if len(data.LogCategories) == 0 { - t.Fatalf("expected LogCategories to be populated from log") + if len(data.LogCategories) != 2 { + t.Fatalf("expected LogCategories to be propagated from stats, got %d entries", len(data.LogCategories)) + } + data.LogCategories[0].Label = "mutated" + if stats.LogCategories[0].Label == "mutated" { + t.Fatalf("LogCategories should be copied into NotificationData") } // Compression ratio should be computed from sizes when explicit savings is <= 0 @@ -370,6 +371,43 @@ func TestConvertBackupStatsUsesLogCountsAndCompressionFallback(t *testing.T) { } } +func TestConvertBackupStatsDoesNotRereadLogFile(t *testing.T) { + // Regression test: conversion must trust stats.ErrorCount/WarningCount/LogCategories + // and never re-parse stats.LogFilePath, so warnings emitted during the notification + // phase aren't double-counted across notifiers in the dispatch chain. + logger := logging.New(types.LogLevelDebug, false) + adapter := NewNotificationAdapter(&stubNotifier{name: "Email", enabled: true}, logger) + + tempDir := t.TempDir() + logFile := filepath.Join(tempDir, "notif.log") + // Log file contains 5 warnings that are NOT yet snapshotted into stats. + content := `[2025-11-10 14:30:01] [WARNING] one +[2025-11-10 14:30:02] [WARNING] two +[2025-11-10 14:30:03] [WARNING] three +[2025-11-10 14:30:04] [WARNING] four +[2025-11-10 14:30:05] [WARNING] five +` + if err := os.WriteFile(logFile, []byte(content), 0644); err != nil { + t.Fatalf("failed to write log file: %v", err) + } + + stats := &BackupStats{ + ExitCode: 0, + Hostname: "host", + ArchivePath: "/var/tmp/backup.tar", + LogFilePath: logFile, + ErrorCount: 0, + WarningCount: 0, + Compression: types.CompressionZstd, + } + + data := adapter.convertBackupStatsToNotificationData(stats) + + if data.WarningCount != 0 || data.ErrorCount != 0 { + t.Fatalf("conversion must trust stats counts (0/0); got errors=%d warnings=%d", data.ErrorCount, data.WarningCount) + } +} + func TestConvertBackupStatsToNotificationDataWarnsOnInconsistentUsageStats(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) var buf bytes.Buffer diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index cf097c4e..959391f2 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -18,6 +18,7 @@ import ( "github.com/tis24dev/proxsave/internal/environment" "github.com/tis24dev/proxsave/internal/logging" "github.com/tis24dev/proxsave/internal/metrics" + "github.com/tis24dev/proxsave/internal/notify" "github.com/tis24dev/proxsave/internal/storage" "github.com/tis24dev/proxsave/internal/types" ) @@ -151,9 +152,10 @@ type BackupStats struct { CloudGFSCurrentYearly int // Error/warning counts - ErrorCount int - WarningCount int - LogFilePath string + ErrorCount int + WarningCount int + LogFilePath string + LogCategories []notify.LogCategory // Exit code ExitCode int diff --git a/internal/orchestrator/pbs_api_apply_test.go b/internal/orchestrator/pbs_api_apply_test.go index f9b3b20a..a0ce6402 100644 --- a/internal/orchestrator/pbs_api_apply_test.go +++ b/internal/orchestrator/pbs_api_apply_test.go @@ -284,11 +284,11 @@ func TestPBSAPIApply_StrictListCommandErrors(t *testing.T) { logger := logging.New(types.LogLevelDebug, false) tests := []struct { - name string - relPath string - content string - listCmd string - apply func(context.Context, *logging.Logger, string, bool) error + name string + relPath string + content string + listCmd string + apply func(context.Context, *logging.Logger, string, bool) error }{ { name: "remote", @@ -945,7 +945,7 @@ func TestApplyPBSDatastoreCfgViaAPI_CreateAndUpdateFail(t *testing.T) { runner.errs = map[string]error{ "proxmox-backup-manager datastore create ds1 /p1 --comment c1": errors.New("create failed"), - "proxmox-backup-manager datastore update ds1 --comment c1": errors.New("update failed"), + "proxmox-backup-manager datastore update ds1 --comment c1": errors.New("update failed"), } err := applyPBSDatastoreCfgViaAPI(context.Background(), logger, stageRoot, false) @@ -1023,7 +1023,7 @@ func TestApplyPBSSyncCfgViaAPI_StrictCleanupAndFallbackUpdate(t *testing.T) { "proxmox-backup-manager sync-job list --output-format=json": []byte(`{"data":[{"id":"job1"},{"id":"old"}]}`), } runner.errs = map[string]error{ - "proxmox-backup-manager sync-job remove old": errors.New("cannot remove old"), + "proxmox-backup-manager sync-job remove old": errors.New("cannot remove old"), "proxmox-backup-manager sync-job create job1 --remote r1 --store ds1": errors.New("already exists"), } @@ -1078,7 +1078,7 @@ func TestApplyPBSVerificationCfgViaAPI_StrictCleanupAndFallbackUpdate(t *testing "proxmox-backup-manager verify-job list --output-format=json": []byte(`{"data":[{"id":"v1"},{"id":"old"}]}`), } runner.errs = map[string]error{ - "proxmox-backup-manager verify-job remove old": errors.New("cannot remove old"), + "proxmox-backup-manager verify-job remove old": errors.New("cannot remove old"), "proxmox-backup-manager verify-job create v1 --store ds1": errors.New("already exists"), } @@ -1135,7 +1135,7 @@ func TestApplyPBSPruneCfgViaAPI_StrictCleanupAndFallbackUpdate(t *testing.T) { "proxmox-backup-manager prune-job list --output-format=json": []byte(`{"data":[{"id":"old"},{"id":"p1"}]}`), } runner.errs = map[string]error{ - "proxmox-backup-manager prune-job remove old": errors.New("cannot remove old"), + "proxmox-backup-manager prune-job remove old": errors.New("cannot remove old"), "proxmox-backup-manager prune-job create p1 --store ds1 --keep-last 3": errors.New("already exists"), }