Conversation
Updates golang.org/x/crypto, x/term, x/text and the dependency-review/setup-node Actions with regenerated checksums
Use a plain channel receive when draining the tar writer goroutine after compressor start failure
Store exec info cache state behind pointers so tests can override and restore it without copying sync.Once
Call isMounted directly from guardMountPoint and remove the pass-through helper with misleading unmount wording
Keep runtime-derived env key removal only at the final write point in the install config flow
* Fix per-notifier error/warning count drift in dispatch chain Snapshot ErrorCount, WarningCount and LogCategories into BackupStats once at backup completion (finalizeBackupStats / parseFailedBackupLogCounts) and have convertBackupStatsToNotificationData read them as-is. Re-parsing the log file per-notifier was over-counting warnings emitted by earlier notifiers in the dispatch chain, so e.g. Pushover reported 5 warnings while Email reported 0 on the same run. * Place issue snapshot at notification boundary * Align exit code with notification snapshot --------- Co-authored-by: tis24dev <github@tis24.it>
…dates group (#214) ci: bump github/codeql-action in the actions-updates group Bumps the actions-updates group with 1 update: [github/codeql-action](https://github.com/github/codeql-action). Updates `github/codeql-action` from 4.35.4 to 4.35.5 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@68bde55...9e0d7b8) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.35.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions-updates ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR introduces a structured pre-notification snapshot system for backup statistics, capturing error/warning counts and log categories at a well-defined point before notification dispatch. This prevents inconsistent re-parsing of log files across multiple notifiers. Supporting changes include exec-info caching refactoring, config wizard early-exit logic, and routine dependency updates. ChangesBackup Stats Log Issue Snapshot System
Exec Info Pointer-Based Caching
Config Wizard Early Exit
Minor Refactoring and Documentation
Dependency Version Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency ReviewThe following issues were found:
License Issuesgo.mod
OpenSSF Scorecard
Scanned Files
|
Review Summary by QodoStabilize notifier dispatch with pre-notification issue snapshot WalkthroughsDescription• Fix per-notifier error/warning count drift by snapshotting stats before notification dispatch • Store exec info behind pointers to enable test overrides without copying sync.Once • Simplify archiver drain and mount guard logic, remove redundant code • Update Go module dependencies and GitHub Actions versions • Add comprehensive test coverage for notification snapshot boundary Diagramflowchart LR
A["Backup Completion"] --> B["finalizeBackupStats"]
B --> C["Parse Log File"]
C --> D["Snapshot ErrorCount<br/>WarningCount<br/>LogCategories"]
D --> E["startNotificationGroup"]
E --> F["snapshotPreNotificationIssues"]
F --> G["applyIssueExitCode"]
G --> H["dispatchNotifications"]
H --> I["Notifier 1"]
H --> J["Notifier 2"]
I --> K["Read Snapshotted Stats"]
J --> K
K --> L["Consistent Counts<br/>Across Notifiers"]
File Changes1. internal/orchestrator/extensions.go
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Keep common backup finalization separate from dry-run issue parsing.
Remove dead success handling and cover issue promotion rules.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
New Features
proxmox,proxmox-notifications,proxmox-mail-forward) for PMF delivery.Bug Fixes
Tests
Chores
Greptile Summary
This PR fixes per-notifier error/warning count drift in the dispatch chain by introducing a pre-notification snapshot (
startNotificationGroup→snapshotPreNotificationIssues) taken once before all notifiers run, and removing the per-call log-file re-parsing fromconvertBackupStatsToNotificationData. It also cleans up dead code, deduplicates template sanitization, and bumps CI actions and Go module dependencies.extensions.go,notification_adapter.go): Issue counts and log categories are now snapshotted immediately before dispatch viarefreshLogIssuesFromFile(true)and stored onBackupStats.LogCategories; each notifier reads the frozen snapshot instead of re-parsing the log, so warnings emitted by an earlier notifier in the chain cannot inflate the counts seen by later ones.applyIssueExitCode): Extracted into a standalone function that preserves domain-specific failure codes (e.g.ExitStorageError) and only promotesExitSuccess/ExitGenericError— a deliberate improvement over the old unconditional switch.backup_run_phases.go):finalizeBackupStatsnow delegates issue-count finalization exclusively tofinalizeDryRunIssueStatsfor dry-run; the non-dry-run path relies entirely on the notification-boundary snapshot, eliminating the double-parse that existed before.Confidence Score: 5/5
Safe to merge; all changed paths are well-tested and the notification snapshot design is consistent end-to-end.
The core bug fix (count drift across notifiers) is correctly implemented: a single snapshot replaces per-notifier log re-parsing, and the new tests cover both the snapshot invariant and the applyIssueExitCode logic exhaustively. The dry-run and failure paths each still set their own exit codes before the notification phase, so no execution path is left with an uninitialized ExitCode. The remaining changes (dead-code removal, duplicate sanitization call, switch refactors, dependency bumps) are straightforward and low-risk.
No files require special attention; extensions.go and notification_adapter.go carry the heaviest logic change but are covered by targeted regression tests.
Important Files Changed
Sequence Diagram
sequenceDiagram participant O as Orchestrator participant FAS as finalizeBackupStats participant SNG as startNotificationGroup participant SPNI as snapshotPreNotificationIssues participant AEC as applyIssueExitCode participant DN as dispatchNotifications participant NA as NotificationAdapter participant N1 as Notifier 1 participant N2 as Notifier 2 O->>FAS: finalizeBackupStats(run) alt dry-run FAS->>FAS: finalizeDryRunIssueStats(stats) else non-dry-run FAS->>FAS: set Duration only end O->>SNG: startNotificationGroup(ctx, stats) SNG->>SPNI: snapshotPreNotificationIssues(stats) SPNI->>SPNI: "refreshLogIssuesFromFile(includeCategories=true)" SPNI-->>SNG: snapshot frozen on stats SNG->>AEC: applyIssueExitCode(stats) SNG->>DN: dispatchNotifications(ctx, stats) DN->>N1: Notify(ctx, stats) N1->>NA: convertBackupStatsToNotificationData(stats) NA->>NA: reads frozen counts, copies LogCategories NA-->>N1: NotificationData N1-->>DN: done DN->>N2: Notify(ctx, stats) N2->>NA: convertBackupStatsToNotificationData(stats) NA->>NA: same frozen counts NA-->>N2: NotificationData N2-->>DN: doneReviews (2): Last reviewed commit: "Clarify issue exit code policy" | Re-trigger Greptile