v0.15.3: ARCH-1 step 3 — extract DependencyProbe#46
Conversation
Third collaborator extracted from TrayContext. No user-visible changes. - New src/DependencyProbe.cs owns cached RcloneAvailable / WinFspInstalled state, TTL stamp, and the slow synchronous probes (File.Exists, spawn rclone version, registry lookups). Constructor takes Func<string> rclone path provider + log callback. - TrayContext.Setup.cs delegates RcloneAvailable, WinFspInstalled, ProbeRcloneAvailableSync, ProbeWinFspInstalledSync. The async refresh worker stays in TrayContext (composes results into the setup-status line) and calls Deps.PublishProbeResults at the end. 53/53 tests green, FileVersion 0.15.3.0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request extracts the cached dependency state and synchronous probe logic from the TrayContext partial into a new, dedicated DependencyProbe class, updating Pixelpipe.Setup.cs to delegate to this new class. The review feedback highlights two critical concurrency issues: first, the lazy initialization of the Deps property in Pixelpipe.Setup.cs lacks thread-safety guards, which can lead to race conditions; second, the volatile keyword cannot be applied to the DateTime struct in DependencyProbe, making the caching mechanism prone to thread-safety issues. Implementing thread-safe lazy initialization and a lock-based synchronization mechanism for the cached fields is highly recommended.
| private DependencyProbe _depProbe; | ||
| private DependencyProbe Deps | ||
| { | ||
| EnsureDependencyCacheFresh(); | ||
| return cachedRcloneAvailable; | ||
| get | ||
| { | ||
| if (_depProbe == null) | ||
| { | ||
| _depProbe = new DependencyProbe( | ||
| delegate { rclonePath = FindRclonePath(); return rclonePath; }, | ||
| LogUiIssue); | ||
| } | ||
| return _depProbe; | ||
| } | ||
| } |
There was a problem hiding this comment.
The Deps property lazily initializes _depProbe without any thread-safety guards. Since Deps is accessed concurrently from both the UI thread (e.g., via RcloneAvailable()) and background threads (via ProbeRcloneAvailableSync() on a ThreadPool worker), there is a race condition where multiple instances of DependencyProbe could be created, or a thread could read a partially-constructed object.
To prevent this, we should use a lock or double-check locking to ensure thread-safe lazy initialization.
private readonly object _depProbeLock = new object();
private DependencyProbe _depProbe;
private DependencyProbe Deps
{
get
{
if (_depProbe == null)
{
lock (_depProbeLock)
{
if (_depProbe == null)
{
_depProbe = new DependencyProbe(
delegate { rclonePath = FindRclonePath(); return rclonePath; },
LogUiIssue);
}
}
}
return _depProbe;
}
}| private volatile bool _cachedRcloneAvailable; | ||
| private volatile bool _cachedWinfspInstalled; | ||
| private DateTime _cachedStampUtc = DateTime.MinValue; | ||
|
|
||
| public DependencyProbe(Func<string> rclonePathProvider, Action<string, Exception> logIssue) | ||
| { | ||
| _rclonePathProvider = rclonePathProvider; | ||
| _logIssue = logIssue; | ||
| } | ||
|
|
||
| public bool RcloneAvailable { get { return _cachedRcloneAvailable; } } | ||
| public bool WinFspInstalled { get { return _cachedWinfspInstalled; } } | ||
| public DateTime LastProbeUtc { get { return _cachedStampUtc; } } | ||
| public bool IsStale { get { return (DateTime.UtcNow - _cachedStampUtc).TotalSeconds >= CacheTtlSeconds; } } | ||
|
|
||
| // Called by the async refresh worker after the slow probes complete. | ||
| public void PublishProbeResults(bool rcloneAvailable, bool winfspInstalled) | ||
| { | ||
| _cachedRcloneAvailable = rcloneAvailable; | ||
| _cachedWinfspInstalled = winfspInstalled; | ||
| _cachedStampUtc = DateTime.UtcNow; | ||
| } |
There was a problem hiding this comment.
The _cachedStampUtc field is a 64-bit DateTime struct, and volatile cannot be applied to it in C#. Since IsStale and PublishProbeResults are accessed concurrently across the UI thread and background threads, there is a risk of memory reordering or stale reads.
To ensure complete thread safety, atomicity, and proper memory barriers without relying on complex volatile ordering, we can use a simple lock to protect the cached fields and properties. Since these are read infrequently (on UI refresh ticks) and written even less frequently (every 30 seconds), lock contention will be virtually zero.
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;
}
}
Summary
Third collaborator extracted from `TrayContext`. No user-visible changes. Rebased onto main after v0.15.2 (#43) merged (supersedes #44, which was stacked on the pre-squash branch).
Test plan
🤖 Generated with Claude Code