v0.15.4: ARCH-1 step 4 — extract MountManager (final ARCH-1 step)

[NathanNeurotic]

Summary

Fourth and final ARCH-1 collaborator extraction. Closes the audit's incremental ARCH-1 work. No user-visible changes. Depends on PRs #43 (v0.15.2 SettingsStore) and #44 (v0.15.3 DependencyProbe).

Changed

• New `src/MountManager.cs` owns the rclone mount process lifecycle:
  • `BuildMountArgs(profile, fullCache, rcCommonFlags, effectiveBandwidth)` — pure helper that assembles the full `rclone mount` argv with all the v0.5.x cache-mode / VFS / network-mode / RC / log / bandwidth flags in one place. Unit-testable without disk.
  • `StartMountProcess(rclonePath, args, logJobWarn)` — `Process.Start` plus the v0.11.4 Job Object binding so the rclone child still dies when Pixelpipe dies.
• `TrayContext.Mount.cs` delegates argv assembly and the spawn to `MountManager`. The dialog flow (rclone-missing, WinFsp-missing, remote-not-configured, drive-in-use, post-launch result) stays in `TrayContext` because it's UI-coupled.

ARCH-1 close-out

Across v0.15.1–v0.15.4 we've extracted:

Step	Class	Owns
v0.15.1	`RcloneClient`	rclone process invocation + `ProcessResult`
v0.15.2	`SettingsStore`	`settings.json` read/write/cache/atomic-write/backup
v0.15.3	`DependencyProbe`	cached rclone/WinFsp availability + sync probes
v0.15.4	`MountManager`	mount argv assembly + Process.Start + Job Object binding

The rest of `TrayContext` is UI orchestration (menu, main window, dialogs, balloons, tray icon, the live-state refresh loop). Further extraction past this point either pulls in UI dependencies or has diminishing returns; the audit's "incremental" intent for ARCH-1 is satisfied.

Test plan

• `scripts/run-tests.ps1` — 53/53 green
• FileVersion 0.15.4.0
• Real-world: mount a profile (any provider) — argv built identically, Job Object binding still triggers (kill Pixelpipe.exe in Task Manager and verify rclone.exe dies)

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request refactors the TrayContext class by extracting several responsibilities into dedicated helper classes: SettingsStore for settings management, DependencyProbe for dependency availability checks, and MountManager for rclone mount process management. The review feedback highlights several critical thread-safety issues introduced during this refactoring, including non-thread-safe lazy initialization of the new helper classes, potential torn reads of a 64-bit DateTime field on 32-bit architectures, and unsafe concurrent access to the cached settings dictionary. Implementing the suggested locking and cloning mechanisms will ensure thread safety across the application.

[gemini-code-assist]

The ReadRoot method returns the direct reference to the cached _cache dictionary. Because Dictionary<TKey, TValue> is not thread-safe, concurrent reads and writes from different threads (such as the UI thread and background workers calling SaveSetting or SaveProfiles) can corrupt the dictionary's internal structure or throw InvalidOperationException during serialization.

Returning a new copy of the dictionary in ReadRoot and cloning the dictionary in WriteRoot under the _cacheLock ensures that each caller works on their own private copy, completely eliminating concurrent modification issues.

        public Dictionary<string, object> ReadRoot()
        {
            lock (_cacheLock)
            {
                if (_cache == null)
                {
                    Dictionary<string, object> root;
                    if (TryReadFile(_settingsFile, out root))
                    {
                        _cache = root;
                    }
                    else
                    {
                        string backupFile = _settingsFile + ".bak";
                        if (TryReadFile(backupFile, out root))
                        {
                            if (_logWarn != null) _logWarn("read settings", "loaded backup settings file after primary file could not be read");
                            _cache = root;
                        }
                        else
                        {
                            _cache = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
                        }
                    }
                }
                return new Dictionary<string, object>(_cache, StringComparer.OrdinalIgnoreCase);
            }
        }

        public void WriteRoot(Dictionary<string, object> root)
        {
            try
            {
                Directory.CreateDirectory(_settingsDir);
                string json;
                lock (_cacheLock)
                {
                    _cache = root != null ? new Dictionary<string, object>(root, StringComparer.OrdinalIgnoreCase) : new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
                    JavaScriptSerializer js = new JavaScriptSerializer();
                    json = js.Serialize(_cache);
                }
                TrayContext.WriteAllTextAtomic(_settingsFile, json, Encoding.UTF8);
            }
            catch (Exception ex) { if (_logIssue != null) _logIssue("write settings", ex); }
        }

[gemini-code-assist]

The _cachedStampUtc field is a 64-bit DateTime value that is read and written across different threads (the UI thread and the background refresh worker thread) without synchronization. On 32-bit architectures, 64-bit reads and writes are not atomic, which can lead to torn reads. Additionally, without memory barriers, there is no guarantee of visibility or ordering.

Using a lock to synchronize access to all cached fields (_cachedRcloneAvailable, _cachedWinfspInstalled, and _cachedStampUtc) ensures thread safety and correctness across all platforms.

        private readonly object _lock = new object();
        private bool _cachedRcloneAvailable;
        private bool _cachedWinfspInstalled;
        private DateTime _cachedStampUtc = DateTime.MinValue;

        public DependencyProbe(Func<string> rclonePathProvider, Action<string, Exception> logIssue)
        {
            _rclonePathProvider = rclonePathProvider;
            _logIssue = logIssue;
        }

        public bool RcloneAvailable { get { lock (_lock) return _cachedRcloneAvailable; } }
        public bool WinFspInstalled { get { lock (_lock) return _cachedWinfspInstalled; } }
        public DateTime LastProbeUtc { get { lock (_lock) return _cachedStampUtc; } }
        public bool IsStale { get { lock (_lock) return (DateTime.UtcNow - _cachedStampUtc).TotalSeconds >= CacheTtlSeconds; } }

        // Called by the async refresh worker after the slow probes complete.
        public void PublishProbeResults(bool rcloneAvailable, bool winfspInstalled)
        {
            lock (_lock)
            {
                _cachedRcloneAvailable = rcloneAvailable;
                _cachedWinfspInstalled = winfspInstalled;
                _cachedStampUtc = DateTime.UtcNow;
            }
        }

[gemini-code-assist]

The Deps property lazily instantiates _depProbe without synchronization. Since Deps is accessed from both the UI thread and background worker threads (e.g., in MountProfile), a race condition could result in multiple instances of DependencyProbe being created concurrently.

Adding a lock ensures thread-safe lazy initialization.

        private readonly object _depProbeLock = new object();
        private DependencyProbe _depProbe;
        private DependencyProbe Deps
        {
            get
            {
                lock (_depProbeLock)
                {
                    if (_depProbe == null)
                    {
                        _depProbe = new DependencyProbe(
                            delegate { rclonePath = FindRclonePath(); return rclonePath; },
                            LogUiIssue);
                    }
                    return _depProbe;
                }
            }
        }

[gemini-code-assist]

The SettingsBackend property lazily instantiates _settingsStore without synchronization. Since SettingsBackend is accessed from both the UI thread and background worker threads (e.g., in SaveSetting), a race condition could result in multiple instances of SettingsStore being created concurrently.

Adding a lock ensures thread-safe lazy initialization.

        private readonly object _settingsStoreLock = new object();
        private SettingsStore _settingsStore;
        private SettingsStore SettingsBackend
        {
            get
            {
                lock (_settingsStoreLock)
                {
                    if (_settingsStore == null)
                    {
                        _settingsStore = new SettingsStore(settingsFile, LogUiIssue,
                            delegate(string area, string msg) { LogUiWarn(area, msg); });
                    }
                    return _settingsStore;
                }
            }
        }