fix(secrets): synchronize SecretsHubSyncStatus getters under the write lock

Review finding (Fix round 1, Task 1): the property getters read their
backing fields without synchronization while RecordSuccess/RecordFailure
write them under _gate, so a concurrent read could observe a torn
DateTimeOffset or a mix of fields from two different writes. Converted the
four auto-properties to explicit properties over private fields, each
getter taking the same lock the writers use. Public interface unchanged.
This commit is contained in:
Joseph Doherty
2026-09-28 17:30:33 -04:00
parent b392b84f21
commit adbf2eb182
@@ -36,21 +36,46 @@ public interface ISecretsHubSyncStatus
}
/// <summary>The mutable implementation the sync service writes to; consumers see only the interface.</summary>
/// <remarks>
/// Writes (<see cref="RecordSuccess"/>/<see cref="RecordFailure"/>) run under <see cref="_gate"/>,
/// and every getter takes the same lock rather than reading its backing field bare — a plain field
/// read races the writer with no synchronization otherwise, and a torn/half-written
/// <see cref="DateTimeOffset"/> (a multi-field struct) or an interleaving that pairs one write's
/// <c>LastSweepUtc</c> with another write's <c>LastError</c> is exactly the inconsistency a status
/// consumer must not see.
/// </remarks>
internal sealed class SecretsHubSyncStatus : ISecretsHubSyncStatus
{
private readonly object _gate = new();
/// <inheritdoc/>
public DateTimeOffset? LastSweepUtc { get; private set; }
private DateTimeOffset? _lastSweepUtc;
private DateTimeOffset? _lastSuccessUtc;
private int _consecutiveFailures;
private string? _lastError;
/// <inheritdoc/>
public DateTimeOffset? LastSuccessUtc { get; private set; }
public DateTimeOffset? LastSweepUtc
{
get { lock (_gate) { return _lastSweepUtc; } }
}
/// <inheritdoc/>
public int ConsecutiveFailures { get; private set; }
public DateTimeOffset? LastSuccessUtc
{
get { lock (_gate) { return _lastSuccessUtc; } }
}
/// <inheritdoc/>
public string? LastError { get; private set; }
public int ConsecutiveFailures
{
get { lock (_gate) { return _consecutiveFailures; } }
}
/// <inheritdoc/>
public string? LastError
{
get { lock (_gate) { return _lastError; } }
}
/// <summary>
/// Records a successful sweep, resetting the failure streak.
@@ -64,10 +89,10 @@ internal sealed class SecretsHubSyncStatus : ISecretsHubSyncStatus
{
lock (_gate)
{
int prior = ConsecutiveFailures;
LastSweepUtc = LastSuccessUtc = now;
ConsecutiveFailures = 0;
LastError = null;
int prior = _consecutiveFailures;
_lastSweepUtc = _lastSuccessUtc = now;
_consecutiveFailures = 0;
_lastError = null;
return prior;
}
}
@@ -79,9 +104,9 @@ internal sealed class SecretsHubSyncStatus : ISecretsHubSyncStatus
{
lock (_gate)
{
LastSweepUtc = now;
ConsecutiveFailures++;
LastError = ex.GetType().Name + ": " + ex.Message;
_lastSweepUtc = now;
_consecutiveFailures++;
_lastError = ex.GetType().Name + ": " + ex.Message;
}
}
}