fix(mqtt): review minors - QoS 0 is not answer evidence, caller cancels stay cancels, rebirth wait cap, bound/trie pins (R06 G2 MQTT review minors 2 3 4 6 7 8)
2: only a PUBACKed (QoS >= 1) publish advances LastSuccessfulRead; a QoS 0 Sparkplug command completing means only that the bytes left the local socket. 3: a no-verdict SUBSCRIBE caused by the caller's own cancellation now throws OperationCanceledException (refs still Bad + pending), so the breaker no longer counts a pipeline timeout as a failure. 4: the rebirth wait is the window capped at 15 s, below the AdminUI 20 s per-call timeout. 6: a 10-topic test pins the 8-in-flight publish bound. 7: partial-level wildcards and $share filters are pinned never to confirm. 8: the intended init-vs-shutdown supersede is commented at the re-check. Refs #744 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
@@ -43,6 +43,21 @@ public sealed class MqttBirthObservationWindowTests
|
||||
public void BirthWindow_IsClamped(int configured, int expectedSeconds)
|
||||
=> MqttDriverBrowser.BirthWindow(Sparkplug(configured)).ShouldBe(TimeSpan.FromSeconds(expectedSeconds));
|
||||
|
||||
/// <summary>
|
||||
/// Review minor 4: the rebirth wait is capped below the AdminUI's 20 s per-call timeout, so a long
|
||||
/// configured window cannot hold the button past it (and a short one still governs).
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData(120, 15)]
|
||||
[InlineData(15, 15)]
|
||||
[InlineData(5, 5)]
|
||||
public async Task RebirthWait_IsTheWindowCappedAt15Seconds(int windowSeconds, int expectedSeconds)
|
||||
{
|
||||
await using var s = new MqttBrowseSession(
|
||||
MqttMode.SparkplugB, client: null, logger: null, birthWindow: TimeSpan.FromSeconds(windowSeconds));
|
||||
s.RebirthWait.ShouldBe(TimeSpan.FromSeconds(expectedSeconds));
|
||||
}
|
||||
|
||||
/// <summary>An idle plant costs one quiet interval, not the whole window.</summary>
|
||||
[Fact]
|
||||
public async Task Settle_WithNoBirths_ReturnsAfterOneQuietInterval()
|
||||
|
||||
@@ -361,6 +361,79 @@ public sealed class MqttDriverSessionLifecycleTests
|
||||
driver.GetHealth().LastSuccessfulRead.ShouldNotBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review minor 6: the bound itself. Ten topics against a broker that never PUBACKs: exactly
|
||||
/// <see cref="MqttDriver.MaxConcurrentPublishes"/> PUBLISHes go out and the count then holds — a
|
||||
/// regression to unbounded concurrency sends all ten. The stability check re-reads the count across
|
||||
/// several condition polls rather than sleeping once.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task AWriteBatch_NeverHasMoreThanTheBoundInFlight()
|
||||
{
|
||||
using var broker = new MiniBroker(); // never PUBACKs
|
||||
var options = BrokerOptions(broker.Port) with
|
||||
{
|
||||
ConnectTimeoutSeconds = 30,
|
||||
RawTags = [.. Enumerable.Range(0, 10).Select(i => WritableTag($"Plant/Mqtt/dev1/Sp{i}", $"cmd/sp{i}"))],
|
||||
};
|
||||
await using var driver = new MqttDriver(options, "d", null);
|
||||
await driver.InitializeAsync(Json(options), TestContext.Current.CancellationToken);
|
||||
|
||||
using var cts = CancellationTokenSource.CreateLinkedTokenSource(TestContext.Current.CancellationToken);
|
||||
var write = driver.WriteAsync(
|
||||
[.. Enumerable.Range(0, 10).Select(i => new WriteRequest($"Plant/Mqtt/dev1/Sp{i}", (double)i))],
|
||||
cts.Token);
|
||||
|
||||
await WaitForAsync(() => broker.Published.Count >= MqttDriver.MaxConcurrentPublishes, "the bound to fill");
|
||||
for (var i = 0; i < 10; i++)
|
||||
{
|
||||
await Task.Delay(50, TestContext.Current.CancellationToken);
|
||||
broker.Published.Count.ShouldBe(MqttDriver.MaxConcurrentPublishes, "a slot freed with no PUBACK received");
|
||||
}
|
||||
|
||||
await cts.CancelAsync();
|
||||
var results = await write.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken);
|
||||
results.Count.ShouldBe(10);
|
||||
results.ShouldAllBe(r => r.StatusCode == 0x80050000u);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review minor 2: a QoS 0 publish (a Sparkplug command) completing only means the bytes reached the
|
||||
/// local socket buffer; the broker has not answered, so it must not advance <c>LastSuccessfulRead</c>.
|
||||
/// The positive control is <see cref="AWriteBatch_KeepsSameTopicWritesInRequestOrder"/>, where a QoS 1
|
||||
/// PUBACK does advance it.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ASparkplugQos0Command_DoesNotAdvanceLastSuccessfulRead()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var options = BrokerOptions(broker.Port) with
|
||||
{
|
||||
Mode = MqttMode.SparkplugB,
|
||||
Sparkplug = new MqttSparkplugOptions { GroupId = "G", RequestRebirthOnGap = false },
|
||||
RawTags =
|
||||
[
|
||||
new RawTagEntry(
|
||||
"Plant/Mqtt/spb/Setpoint",
|
||||
"""{"groupId":"G","edgeNodeId":"E","metricName":"Setpoint","dataType":"Float64"}""",
|
||||
WriteIdempotent: false),
|
||||
],
|
||||
};
|
||||
await using var driver = new MqttDriver(options, "d", null);
|
||||
await driver.InitializeAsync(Json(options), TestContext.Current.CancellationToken);
|
||||
var before = driver.GetHealth().LastSuccessfulRead;
|
||||
var publishedBefore = broker.Published.Count;
|
||||
|
||||
var results = await driver.WriteAsync(
|
||||
[new WriteRequest("Plant/Mqtt/spb/Setpoint", 4.5)], TestContext.Current.CancellationToken);
|
||||
|
||||
results.ShouldHaveSingleItem().StatusCode.ShouldBe(0u);
|
||||
await WaitForAsync(
|
||||
() => broker.Published.Skip(publishedBefore).Any(p => p.Topic == "spBv1.0/G/NCMD/E"),
|
||||
"the NCMD to reach the broker — the control that the write really went out");
|
||||
driver.GetHealth().LastSuccessfulRead.ShouldBe(before);
|
||||
}
|
||||
|
||||
private static async Task WaitForAsync(Func<bool> condition, string what)
|
||||
{
|
||||
var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10);
|
||||
|
||||
@@ -553,7 +553,7 @@ public sealed class MqttSubscriptionManagerTests
|
||||
var raised = new List<DataChangeEventArgs>();
|
||||
mgr.OnDataChange += (_, e) => raised.Add(e);
|
||||
|
||||
await Should.ThrowAsync<Exception>(() =>
|
||||
await Should.ThrowAsync<InvalidOperationException>(() =>
|
||||
mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken));
|
||||
|
||||
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(BadCommunicationError);
|
||||
@@ -561,6 +561,32 @@ public sealed class MqttSubscriptionManagerTests
|
||||
raised.ShouldContain(e => e.FullReference == OvenTempPath && e.Snapshot.StatusCode == BadCommunicationError);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review minor 3: a cancel of the CALLER's token (a pipeline timeout, host shutdown) is a cancellation,
|
||||
/// not a broker fault — it must surface as OperationCanceledException, or the resilience breaker counts
|
||||
/// it as a failure. The refs are still Bad and pending, so the next reconnect retries them (the positive
|
||||
/// control). The uncancelled no-verdict case throws InvalidOperationException
|
||||
/// (<see cref="SubscribeAsync_TransportThrows_DegradesRefsToBad_AndThrows"/>).
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task SubscribeAsync_CallerCancelled_ThrowsOperationCanceled_AndTheTopicStaysPending()
|
||||
{
|
||||
var transport = new FakeTransport { Throw = new OperationCanceledException() };
|
||||
var mgr = NewManager(transport);
|
||||
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
|
||||
using var cts = new CancellationTokenSource();
|
||||
await cts.CancelAsync();
|
||||
|
||||
var ex = await Should.ThrowAsync<OperationCanceledException>(() =>
|
||||
mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), cts.Token));
|
||||
ex.ShouldNotBeOfType<InvalidOperationException>();
|
||||
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(BadCommunicationError);
|
||||
|
||||
transport.Throw = null;
|
||||
await mgr.OnReconnectedAsync(TestContext.Current.CancellationToken);
|
||||
transport.Calls.Last().Select(f => f.Topic).ShouldBe([OvenTopic]);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The reconcile path: once the actor re-subscribes after the throw, the topic IS re-SUBSCRIBEd and
|
||||
/// delivery resumes on the new handle — the positive control for the throw above.
|
||||
|
||||
@@ -69,6 +69,27 @@ public sealed class TopicFilterTrieTests
|
||||
collect.Seen.Contains(1).ShouldBe(expected);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review minor 7: partial-level wildcards and <c>$share/…</c> filters are outside the trie's level rules.
|
||||
/// Whatever it yields, confirmation by the broker's comparer must produce no match — pinned so a later
|
||||
/// "smarter" trie cannot start delivering them.
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData("a+", "ab")]
|
||||
[InlineData("a/b#", "a/bc")]
|
||||
[InlineData("a/b#", "a/b")]
|
||||
[InlineData("$share/g/a/b", "a/b")]
|
||||
[InlineData("$share/g/#", "a/b")]
|
||||
public void Match_partialLevelWildcardsAndSharedFilters_neverConfirm(string filter, string topic)
|
||||
{
|
||||
var trie = TopicFilterTrie<int>.Build([(filter, 1)]);
|
||||
var collect = new Collect();
|
||||
trie.Match(topic, ref collect);
|
||||
|
||||
collect.Seen.Where(_ => MqttTopicFilterComparer.Compare(topic, filter) == MqttTopicFilterCompareResult.IsMatch)
|
||||
.ShouldBeEmpty();
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Empty_MatchesNothing()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user