Fix/audit reliability hardening

[eakkawut]

Summary

Fixes a set of defects found by an audit of the code outside the affinity/topology/rules core:
four correctness bugs that are visible to users (one of them a data-loss path), five lifecycle
bugs, and a group of threading problems around the tray, notifications and performance
monitoring.

No behaviour in the elevation model, the process-control guardrails or the affinity apply
pipeline is changed. See Security and Risk Notes below.

Branch: fix/audit-reliability-hardening, seven commits split by concern:

Commit	Scope
3a8e74f	fix: data loss and correctness — rules store, settings edits, quiet hours, hotkey defaults
7d47802	fix: disposal — service container, ProcessMonitorService, view models
1488cdb	fix: main window close handling and tray thread affinity
42b8f8a	fix: performance monitoring and CPU Set probing
300fe4a	fix: autostart ordering
d647aad	docs: changelog
b7d31ea	chore: ignore the local tmp working directory

Reviewing commit by commit works; each one is a single concern and the tip is what was built and tested.

The changed code carries no explanatory comments. The rationale for each change lives in the commit
message and in docs/CHANGELOG.md instead, so it stays in one place rather than being duplicated
into the source. Existing comments in the touched files are untouched.

Type of Change

• Bug fix
• Feature
• Refactor
• Documentation
• Security hardening
• Performance improvement

What changed

Data safety and correctness

Defect	Fix
A transient read failure on persistent_rules.json (antivirus/backup/sync lock) cached an empty rule set for the session; the next save then overwrote every saved rule	PersistentProcessRuleJsonStore no longer caches a failed read, and preserves an unreadable file as *.unreadable before any later save can replace it
Any background settings write (for example the startup update check recording LastUpdateCheckUtc) overwrote the user's unsaved Settings edits and cleared the dirty flag	SettingsViewModel re-bases only the saved snapshot while edits are pending
The quiet-hours comparison was inverted relative to its own comments: the shipped 22:00–08:00 default never suppressed anything, and a daytime window suppressed everything around the clock. An explicit Do Not Disturb request was also overridden by the schedule	Extracted IsWithinQuietHours with correct wrap-around handling; an explicit request now wins over the schedule; auto-expiry raises DoNotDisturbChanged
Default hotkeys were behind settings.KeyboardShortcuts != null, which is never false because the model initialises an empty list and CopyFrom keeps it non-null, so a fresh install registered no global hotkeys	Condition is now Count > 0; registration is refused (and logged) when no window handle exists yet; SetWindowHandle is idempotent

Lifecycle

• The service container is now disposed on exit. That is what flushes buffered structured logs, stops the WMI watchers and releases native counters — previously the last few seconds of diagnostics before every exit were lost.
• ProcessMonitorService.Dispose set the disposed flag before calling StopMonitoringAsync, which short-circuits on that flag, so nothing was stopped. The idempotency flag is now separate from the disposed flag.
• MainWindow.OnClosing cancels the close and then runs the shutdown chain. A fault there left the window permanently unclosable with no visible error; it now runs through TaskSafety with a fallback Shutdown().
• Every ViewModel the main window owns is disposed on close. PowerPlanViewModel releases its 10-second timer; LogViewerViewModel, ProcessPowerPlanAssociationViewModel and SettingsViewModel detach from the singleton services they subscribed to.
• KeyboardShortcutService.Dispose no longer blocks on a Task, and a failed UnregisterHotKey no longer leaves a stale entry that makes "clear all" report success.

Threading

• The tray context-menu rebuild mutates Windows Forms ToolStrip state from a powercfg continuation, i.e. a thread-pool thread. UpdateContextMenuAsync now takes the dispatcher and marshals every tray call; monitoring-status updates raised from WMI watcher and polling-timer threads are dispatched too.
• Throttle history in SmartNotificationService is a List<DateTime> inside a ConcurrentDictionary, written by caller threads, the 2-second processing timer and the hourly cleanup timer. It is now lock-guarded and uses GetOrAdd instead of check-then-act.
• PerformanceMonitoringService: historicalData is lock-guarded, duplicate timers are prevented with Interlocked, the core-counter list is snapshotted before iteration, and disposal marks state before releasing counters so an in-flight tick cannot resurrect them.

Other

• Per-core counters use the group-aware Processor Information category above 64 logical processors, and tolerate individual missing instances instead of clearing the whole per-core view.
• The notification retry ladder is reachable again (it sat behind a hardcoded success = true).
• WMI result collections and objects are disposed.
• A failed CPU Set topology probe is retried after 30 seconds instead of disabling CPU Sets for the process lifetime.
• EnhancedLoggingService caches the debug-logging flag instead of deep-cloning the ~60-property settings model on every structured log call.
• Autostart checks elevation before removing the legacy registry entry, and UpdateAutostartAsync no longer disables before enabling.

Validation

• Built Debug and Release successfully
• Ran automated tests
• Verified impacted UI flows — not done, see below
• Verified no secrets/credentials introduced

Run locally on SDK 8.0.423 (pinned by global.json at 8.0.400 / latestFeature):

dotnet build ThreadPilot_1.sln --configuration Debug   --no-restore   → 0 errors, 0 warnings
dotnet build ThreadPilot_1.sln --configuration Release --no-restore   → 0 errors, 0 warnings
dotnet test  Tests/ThreadPilot.Core.Tests --configuration Release     → 668 passed, 0 failed, 0 skipped

The first Debug build reported one warning, SA1623 on the new IsDebugLoggingEnabled summary; that
is fixed in 2dc793a and both configurations are now warning-free.

Coverage gate from ci-devsecops.yml reproduced locally with the same coverlet runsettings:

Metric	Value	Gate
Line coverage	38.44 %	≥ 35.0 % — PASS
Branch coverage	32.75 %	not gated

UI flows are the one box still unchecked. The app ships a requireAdministrator manifest, and
the paths this PR touches most — the tray context menu rebuild, quiet-hours suppression, global
hotkey registration and the shutdown/disposal chain — need an interactive elevated session with real
processes and power plans to exercise meaningfully. They are covered by unit tests at the logic level
(quiet-hours wrap-around, hotkey defaults, dispatcher marshalling, Dispose actually stopping
monitoring) but were not clicked through by hand.

Other checks that passed locally:

• No credential-like patterns in the diff (manual scan; gitleaks is a CI-runtime download and is not installed here).
• Artifact policy from .githooks/pre-commit.ps1 applied to every committed file: no forbidden patterns or directories, nothing over 100 MB. TestResults/, bin/ and obj/ are all confirmed ignored, so the coverage run left nothing behind.

Security and Risk Notes

Required by docs/CONTRIBUTING.md because this touches elevation, process control and power plans.

Elevation — AutostartService.EnableAutostartAsync now checks IsRunningAsAdministrator()
before removing the legacy HKCU\...\Run entry, and only retires that entry once the scheduled
task is confirmed created. The previous order deleted the working mechanism and then bailed out,
leaving a user with no autostart while the UI still showed it enabled. UpdateAutostartAsync calls
enable directly, relying on schtasks /F to overwrite, instead of a disable-then-enable sequence
that had no rollback. The requireAdministrator manifest, the startup elevation bootstrap and the
scheduled-task definitions are unchanged.

Process control — no change to how affinity, priority or memory priority are applied. The
CpuSelectionAffinityApplier order (CPU Sets first, legacy fallback only when representable), the
Realtime priority block, the High priority warning, the protected-process denylist and the
NtQueryInformationProcess check are all untouched. The one behavioural change in this area is
that a failed CPU Set topology probe is retried after 30 seconds instead of being cached as
permanently unavailable; the failure path itself still reports CpuSetsUnavailable and callers
still fall back exactly as before. ProcessMonitorService.Dispose now genuinely stops monitoring,
which is a behaviour change on exit paths that previously leaked the watchers.

Power plans — no new powercfg calls and no change to PowerPlanService,
PowerPlanTransitionGate or the association engine. The tray subscribes to the existing
IPowerPlanService.PowerPlanChanged notification to refresh its menu. The periodic tray timer was
deliberately not re-enabled: it is gated off by AppNavigationOptions.ShowAdvancedDiagnostics,
and that gating is what produced the idle-CPU reduction recorded for v1.4.4, so the fix is
event-driven instead of restoring polling.

Persisted configuration — no schema or format change. persistent_rules.json,
settings.json, core_masks.json and saved profiles are read and written exactly as before. The
only new file the app can create is a persistent_rules.json.unreadable copy, written once, only
when the real file cannot be parsed, and never overwritten.

Regressions to watch during review

1. Disposing the container at exit now runs every singleton's Dispose. ProcessMonitorManagerService.Dispose blocks on StopAsync behind a semaphore; on the normal path PerformGracefulShutdownAsync has already stopped it so this returns immediately, but a shutdown that skips the graceful path will now do that work at exit.
2. Disposing the ViewModels in OnClosed means their handlers stop firing earlier than before. ProcessViewModel and MasksViewModel are singletons and are intentionally left to the container.
3. UpdateContextMenuAsync gained a required parameter. There is a single production call site and the test was updated.

Checklist

• Updated docs where relevant — docs/CHANGELOG.md gains an Unreleased section. The repo has no prior Unreleased convention, so please rename it to the target version at release time.
• Added or updated tests where relevant — see below
• Kept changes backwards compatible — no persisted-format or public-contract changes beyond the one interface parameter noted above

Tests added

Test	Covers
SmartNotificationQuietHoursTests (15 cases)	Overnight and same-day quiet-hours windows, and the zero-length window
KeyboardShortcutDefaultsTests	Empty list falls back to defaults; a configured list does not; defaults are enabled and global
PersistentProcessRuleJsonStoreTests (+2)	A failed read is retried rather than cached; an unreadable file is preserved
ProcessMonitorServiceSettingsTests (+2)	Dispose actually stops monitoring, and is idempotent
SystemTrayStatusUpdaterTests (+1)	Every tray mutation goes through the dispatcher

Note on scope

docs/CONTRIBUTING.md asks for pull requests scoped to a single concern, and this one is not: it
carries 17 defect groups across 13 production files. They were found and fixed as one audit pass.
If you would prefer smaller reviews, a clean split would be:

1. Data loss and correctness — PersistentProcessRuleJsonStore, SettingsViewModel, SmartNotificationService (quiet hours), KeyboardShortcutService (defaults) + their tests
2. Shutdown and disposal — App.xaml.cs, ProcessMonitorService, MainWindow (OnClosing/OnClosed), the ViewModel disposal overrides + their tests
3. Tray threading — SystemTrayStatusUpdater, MainWindow tray paths + its test
4. Performance monitoring — PerformanceMonitoringService, ProcessCpuSetHandler, EnhancedLoggingService
5. Autostart — AutostartService

Happy to reopen it as that sequence if you want it reviewed in pieces.

[PrimeBuild-pc]

Thank you very much for taking the time to audit the project and submit such a detailed contribution. I appreciate the clear commit structure, the changelog update, the risk notes, the added tests, and the fact that you explicitly documented the UI flows that were not manually validated. A substantial part of this PR is heading in the right direction, and I did not find suspicious dependencies, unrelated workflow changes, weakened security controls, or other signs of malicious behavior.

After reviewing the full diff, however, I found a few correctness issues that I need addressed before I can approve the PR.

1. PersistentProcessRuleJsonStore can still overwrite an unpreserved rules file

The new behavior is intended to protect persistent_rules.json after a failed read, but the protection is not guaranteed in the transient-lock scenario described in the PR.

The current sequence is:

1. LoadAsync fails and sets loadFailed = true.
2. TryPreserveUnreadableFile attempts to copy the original file to .unreadable.
3. If that copy also fails, the exception is only logged at debug level.
4. A later SaveAsync is still allowed to replace the original file.

If antivirus, backup, or synchronization software temporarily locks the file, both the read and the backup copy can fail for the same reason. Once the lock is released, a subsequent save can replace the original file even though no recovery copy was successfully created.

There is a second edge case: if an older .unreadable file already exists, the method returns without preserving the current content. A newer damaged or temporarily unreadable rules file could therefore still be lost.

Please change this so that a save after a failed read cannot overwrite the existing file unless the existing content has actually been preserved. A timestamped or otherwise unique recovery filename would also avoid reusing a stale backup.

Please add tests covering at least:

• preservation failure followed by a save attempt;
• an existing .unreadable recovery file;
• retrying after a transient read/copy failure;
• confirmation that the original file is not overwritten when preservation was unsuccessful.

The current corrupt-JSON test validates a readable but invalid file; it does not cover the file-lock/preservation-failure path described by the PR.

2. The Settings rebase is not a true merge and can overwrite background updates

The new OnSettingsServiceSettingsChanged behavior correctly avoids immediately replacing the user's visible unsaved edits. However, while edits are pending, it updates only savedSettingsSnapshot and leaves the complete Settings object unchanged.

SaveSettingsAsync later writes the entire Settings object back through UpdateSettingsAsync, rather than applying only the properties changed by the user.

For example:

1. The user changes a UI setting but does not save it.
2. A background update changes LastUpdateCheckUtc.
3. The persisted snapshot is updated, but the UI model still contains the old LastUpdateCheckUtc value.
4. The user saves the unrelated UI change.
5. The old timestamp is written back, undoing the background update.

This prevents background writes from deleting the user's edits, but it creates the inverse problem: saving the user's edits can delete background changes.

Please implement a real three-way merge using:

• the previous saved snapshot;
• the newly persisted settings;
• the locally edited settings;

Only properties actually changed locally should override the newly persisted model. Tracking a local dirty-property set would also be a reasonable approach.

Please add a regression test where a user-facing property is edited locally, an unrelated property such as LastUpdateCheckUtc changes externally, and saving preserves both changes.

3. Skipping an unavailable CPU counter loses the logical processor identity

The per-core counter initialization now skips individual unavailable instances, which is preferable to dropping the entire per-core view. However, the resulting counters are stored in a compact list, and GetCpuCoreUsageAsync uses the list index as the logical CoreId and as the input for topology lookups.

If logical processor 2 is unavailable but processor 3 is available, the counter for processor 3 becomes list element 2 and is reported and classified as core 2. This can produce incorrect core names, core types, physical-core mappings, and hyper-threading information.

Please preserve the original logical processor index together with each counter, for example with a small record/tuple containing (LogicalProcessorIndex, PerformanceCounter), and use that index when producing CpuCoreUsage and querying topology.

Please add a test or an injectable abstraction that verifies correct IDs when an intermediate counter instance is unavailable.

4. Please verify the performance-monitor stop/start race

StopMonitoringAsync resets isMonitoringTickInProgress immediately after disposing the timer, but disposing a System.Threading.Timer does not necessarily wait for an already-running async callback to finish. A rapid stop/start can therefore allow the old callback and the new timer callback to overlap while sharing counters and historical state.

Please either harden this lifecycle so an old generation cannot overlap a restarted monitor, or add a deterministic test demonstrating that the current implementation is safe. A generation token, cancellation token, or awaited shutdown mechanism would make the intended lifecycle clearer.

5. Additional validation required

The PR currently reports local build/test results, while the repository CI and CodeQL runs are still waiting for authorization and have not produced jobs. Before approval, I need the repository checks to run and pass, including build, tests, coverage, dependency audit, secret scanning, and CodeQL.

Please also manually validate the elevated Windows UI flows that the PR itself identifies as not yet tested:

• tray menu rebuilding and power-plan checkmark refresh;
• monitoring-status updates from background threads;
• default and configured global hotkey registration;
• quiet-hours and explicit/timed Do Not Disturb behavior;
• normal shutdown and fallback shutdown after an exception;
• autostart enable, update, and disable behavior.

I would also appreciate focused regression coverage for the autostart ordering, notification retry path, debug-logging cache updates, and complete shutdown/disposal chain, because these are meaningful behavior changes that currently rely mostly on inspection.

Scope

This PR is considerably broader than the repository's single-concern guideline. I am not requiring a split if the issues above are corrected cleanly and the validation remains manageable, but I am open to reviewing the smaller sequence proposed in the PR description if that is easier.

Again, thank you for the contribution. There are several worthwhile fixes here, and I would be happy to re-review the PR after these points are addressed and the checks are green.