fix(mqtt): AcceptBirth is NBIRTH-only -- a DBIRTH routed there wipes bdSeq
Self-review of the commit before this one caught a defect in the contract it published rather than in the code it ran. AcceptBirth's doc said it records "a birth (NBIRTH/DBIRTH)". That is wrong on both halves of this type, and the wrong version plants exactly the defect the bdSeq tie exists to prevent. Only the NBIRTH restarts the Sparkplug sequence; a DBIRTH carries the next number in the edge node's ongoing stream, so it belongs to Accept. And only the NBIRTH and NDEATH carry bdSeq at all -- a DBIRTH has none to pass. So a Task 21 that followed the old doc and routed a DBIRTH to AcceptBirth would do two wrong things at once: rebase the sequence mid-stream, and wipe the node's session token to null. A stale Last Will arriving afterwards would then find nothing to compare against, fall through the deliberate fail-toward-stale rule in IsDeathForCurrentSession, and kill the live node. Renamed AcceptBirth -> AcceptNodeBirth so the DBIRTH call site reads wrong at a glance rather than only in prose, documented the hazard on both methods, and added DeviceBirth_ContinuesTheNodeSequence_AndLeavesTheSessionTokenAlone to pin the shape at this type's own boundary -- the ingest state machine now has something to fail against if it wires the call sites the other way round. Falsifiability: mutating Accept to clear _birthBdSeq reddens exactly the new test (1 RED), reverted. 29 tests, 438/438 for the MQTT suite. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
@@ -88,8 +88,8 @@ public sealed class SequenceTracker
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Records a birth (NBIRTH/DBIRTH): restarts the sequence at the birth's <c>seq</c> and adopts
|
/// Records an <b>NBIRTH</b>: restarts the sequence at the birth's <c>seq</c> and adopts its
|
||||||
/// its <c>bdSeq</c> as the current session token.
|
/// <c>bdSeq</c> as the current session token.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="seq">
|
/// <param name="seq">
|
||||||
/// The birth payload's <c>seq</c> — <see cref="SparkplugPayload.Seq"/>, which is
|
/// The birth payload's <c>seq</c> — <see cref="SparkplugPayload.Seq"/>, which is
|
||||||
@@ -105,11 +105,22 @@ public sealed class SequenceTracker
|
|||||||
/// </returns>
|
/// </returns>
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// <para>
|
/// <para>
|
||||||
/// <b>A birth is never a gap.</b> It restarts the sequence by definition, so it must not
|
/// <b>An NBIRTH is never a gap.</b> It restarts the sequence by definition, so it must not
|
||||||
/// be run through <see cref="Accept"/> — doing so would flag a gap on the very message
|
/// be run through <see cref="Accept"/> — doing so would flag a gap on the very message
|
||||||
/// that just resynchronized everything, and answer a birth with a request for another one.
|
/// that just resynchronized everything, and answer a birth with a request for another one.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// <para>
|
/// <para>
|
||||||
|
/// <b>A DBIRTH must NOT be offered here — it belongs to <see cref="Accept"/>.</b> Only the
|
||||||
|
/// NBIRTH restarts the sequence; a DBIRTH carries the next number in the edge node's
|
||||||
|
/// ongoing stream, and only the NBIRTH (with the NDEATH) carries <c>bdSeq</c> at all.
|
||||||
|
/// Routing a DBIRTH here would do two wrong things at once: silently rebase the sequence
|
||||||
|
/// mid-stream, and — because the DBIRTH has no <c>bdSeq</c> to pass — <b>wipe the node's
|
||||||
|
/// session token to null</b>. A stale Last Will arriving afterwards would then find
|
||||||
|
/// nothing to be compared against, fall through the fail-toward-stale rule in
|
||||||
|
/// <see cref="IsDeathForCurrentSession"/>, and kill the live node this type exists to
|
||||||
|
/// protect. The method is named for the node deliberately, so that call site reads wrong.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
/// <b>An in-range <c>seq</c> other than the spec-required 0 is adopted, not refused.</b>
|
/// <b>An in-range <c>seq</c> other than the spec-required 0 is adopted, not refused.</b>
|
||||||
/// Refusing it would leave the tracker unsynchronized against a publisher whose stream is
|
/// Refusing it would leave the tracker unsynchronized against a publisher whose stream is
|
||||||
/// otherwise perfectly followable, and every message that followed would demand a fresh
|
/// otherwise perfectly followable, and every message that followed would demand a fresh
|
||||||
@@ -122,7 +133,7 @@ public sealed class SequenceTracker
|
|||||||
/// thing that lets a stale will kill a live node.
|
/// thing that lets a stale will kill a live node.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
public bool AcceptBirth(ulong? seq, ulong? bdSeq = null)
|
public bool AcceptNodeBirth(ulong? seq, ulong? bdSeq = null)
|
||||||
{
|
{
|
||||||
lock (_gate)
|
lock (_gate)
|
||||||
{
|
{
|
||||||
@@ -143,8 +154,8 @@ public sealed class SequenceTracker
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Offers the next sequenced message's <c>seq</c> (NDATA/DDATA/DDEATH) and reports whether the
|
/// Offers the next sequenced message's <c>seq</c> — <b>DBIRTH</b>, NDATA, DDATA or DDEATH —
|
||||||
/// stream is still contiguous.
|
/// and reports whether the stream is still contiguous.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
/// <param name="seq">
|
/// <param name="seq">
|
||||||
/// The payload's <c>seq</c> — <see cref="SparkplugPayload.Seq"/>, which is
|
/// The payload's <c>seq</c> — <see cref="SparkplugPayload.Seq"/>, which is
|
||||||
@@ -252,7 +263,7 @@ public sealed class SequenceTracker
|
|||||||
/// <para>
|
/// <para>
|
||||||
/// <b>Pure.</b> It answers a question and changes nothing, so the caller can ask it while
|
/// <b>Pure.</b> It answers a question and changes nothing, so the caller can ask it while
|
||||||
/// logging or deciding. A death that <i>is</i> acted on needs no reset here: the node is
|
/// logging or deciding. A death that <i>is</i> acted on needs no reset here: the node is
|
||||||
/// offline, and the next birth calls <see cref="AcceptBirth"/> which restarts the sequence
|
/// offline, and the next birth calls <see cref="AcceptNodeBirth"/> which restarts the sequence
|
||||||
/// anyway.
|
/// anyway.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
|
|||||||
@@ -63,7 +63,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Sequence_FullWrapCycles_NeverReportAGap()
|
public void Sequence_FullWrapCycles_NeverReportAGap()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0).ShouldBeTrue();
|
s.AcceptNodeBirth(0).ShouldBeTrue();
|
||||||
|
|
||||||
for (var i = 1; i <= 600; i++)
|
for (var i = 1; i <= 600; i++)
|
||||||
{
|
{
|
||||||
@@ -200,7 +200,7 @@ public sealed class SequenceTrackerTests
|
|||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.Accept(200).ShouldBeTrue();
|
s.Accept(200).ShouldBeTrue();
|
||||||
|
|
||||||
s.AcceptBirth(0).ShouldBeTrue();
|
s.AcceptNodeBirth(0).ShouldBeTrue();
|
||||||
|
|
||||||
s.LastSeq.ShouldBe((byte)0);
|
s.LastSeq.ShouldBe((byte)0);
|
||||||
s.IsBirthSynchronized.ShouldBeTrue();
|
s.IsBirthSynchronized.ShouldBeTrue();
|
||||||
@@ -216,7 +216,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Birth_ThenSkippedValue_IsStillAGap()
|
public void Birth_ThenSkippedValue_IsStillAGap()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0).ShouldBeTrue();
|
s.AcceptNodeBirth(0).ShouldBeTrue();
|
||||||
|
|
||||||
s.Accept(1).ShouldBeTrue();
|
s.Accept(1).ShouldBeTrue();
|
||||||
s.Accept(3).ShouldBeFalse();
|
s.Accept(3).ShouldBeFalse();
|
||||||
@@ -233,7 +233,7 @@ public sealed class SequenceTrackerTests
|
|||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
|
|
||||||
s.AcceptBirth(7).ShouldBeTrue();
|
s.AcceptNodeBirth(7).ShouldBeTrue();
|
||||||
|
|
||||||
s.LastSeq.ShouldBe((byte)7);
|
s.LastSeq.ShouldBe((byte)7);
|
||||||
s.IsBirthSynchronized.ShouldBeTrue();
|
s.IsBirthSynchronized.ShouldBeTrue();
|
||||||
@@ -249,12 +249,35 @@ public sealed class SequenceTrackerTests
|
|||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
|
|
||||||
s.AcceptBirth(256).ShouldBeFalse();
|
s.AcceptNodeBirth(256).ShouldBeFalse();
|
||||||
|
|
||||||
s.LastSeq.ShouldBeNull();
|
s.LastSeq.ShouldBeNull();
|
||||||
s.IsBirthSynchronized.ShouldBeFalse();
|
s.IsBirthSynchronized.ShouldBeFalse();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// <b>A DBIRTH continues the edge node's sequence — it does not restart it, and it must not be
|
||||||
|
/// routed through <see cref="SequenceTracker.AcceptNodeBirth"/>.</b> Only the NBIRTH resets
|
||||||
|
/// <c>seq</c>, and only the NBIRTH/NDEATH carry <c>bdSeq</c>. Sending a DBIRTH to the node-birth
|
||||||
|
/// entry point would rebase the sequence mid-stream <i>and</i> — having no <c>bdSeq</c> to pass
|
||||||
|
/// — wipe the session token, at which point a stale Last Will finds nothing to be compared
|
||||||
|
/// against and kills the live node. This test pins the shape at the tracker's own boundary so
|
||||||
|
/// the ingest state machine has something to fail against if it wires the call sites the other
|
||||||
|
/// way round.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public void DeviceBirth_ContinuesTheNodeSequence_AndLeavesTheSessionTokenAlone()
|
||||||
|
{
|
||||||
|
var s = new SequenceTracker();
|
||||||
|
s.AcceptNodeBirth(0, bdSeq: 7).ShouldBeTrue();
|
||||||
|
|
||||||
|
s.Accept(1).ShouldBeTrue(); // the DBIRTH — next in the node's stream, not a restart
|
||||||
|
s.Accept(2).ShouldBeTrue(); // first DDATA
|
||||||
|
|
||||||
|
s.BirthBdSeq.ShouldBe(7UL, "a device birth must not disturb the node's session token");
|
||||||
|
s.IsDeathForCurrentSession(6).ShouldBeFalse("the stale-will guard must still have a token to compare");
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// <see cref="SequenceTracker.Reset"/> returns the tracker to its virgin state. This is the
|
/// <see cref="SequenceTracker.Reset"/> returns the tracker to its virgin state. This is the
|
||||||
/// reconnect seam: after a dropped connection the driver has missed an unknown number of
|
/// reconnect seam: after a dropped connection the driver has missed an unknown number of
|
||||||
@@ -264,7 +287,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Reset_ClearsTheBaselineAndTheBirthSynchronizedFlag()
|
public void Reset_ClearsTheBaselineAndTheBirthSynchronizedFlag()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0, bdSeq: 4).ShouldBeTrue();
|
s.AcceptNodeBirth(0, bdSeq: 4).ShouldBeTrue();
|
||||||
s.Accept(1).ShouldBeTrue();
|
s.Accept(1).ShouldBeTrue();
|
||||||
|
|
||||||
s.Reset();
|
s.Reset();
|
||||||
@@ -289,8 +312,8 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Death_WithStaleBdSeqArrivingAfterARebirth_DoesNotApply()
|
public void Death_WithStaleBdSeqArrivingAfterARebirth_DoesNotApply()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0, bdSeq: 7); // session 7 — the one that is about to die
|
s.AcceptNodeBirth(0, bdSeq: 7); // session 7 — the one that is about to die
|
||||||
s.AcceptBirth(0, bdSeq: 8); // session 8 — the node is back, and alive
|
s.AcceptNodeBirth(0, bdSeq: 8); // session 8 — the node is back, and alive
|
||||||
|
|
||||||
s.IsDeathForCurrentSession(7).ShouldBeFalse("session 7's will must not kill session 8");
|
s.IsDeathForCurrentSession(7).ShouldBeFalse("session 7's will must not kill session 8");
|
||||||
}
|
}
|
||||||
@@ -300,7 +323,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Death_WithMatchingBdSeq_Applies()
|
public void Death_WithMatchingBdSeq_Applies()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0, bdSeq: 8);
|
s.AcceptNodeBirth(0, bdSeq: 8);
|
||||||
|
|
||||||
s.IsDeathForCurrentSession(8).ShouldBeTrue();
|
s.IsDeathForCurrentSession(8).ShouldBeTrue();
|
||||||
}
|
}
|
||||||
@@ -327,7 +350,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Death_WithNoBdSeq_Applies()
|
public void Death_WithNoBdSeq_Applies()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0, bdSeq: 8);
|
s.AcceptNodeBirth(0, bdSeq: 8);
|
||||||
|
|
||||||
s.IsDeathForCurrentSession(null).ShouldBeTrue();
|
s.IsDeathForCurrentSession(null).ShouldBeTrue();
|
||||||
}
|
}
|
||||||
@@ -337,7 +360,7 @@ public sealed class SequenceTrackerTests
|
|||||||
public void Death_WhenTheBirthCarriedNoBdSeq_Applies()
|
public void Death_WhenTheBirthCarriedNoBdSeq_Applies()
|
||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
s.AcceptBirth(0, bdSeq: null);
|
s.AcceptNodeBirth(0, bdSeq: null);
|
||||||
|
|
||||||
s.IsDeathForCurrentSession(7).ShouldBeTrue();
|
s.IsDeathForCurrentSession(7).ShouldBeTrue();
|
||||||
}
|
}
|
||||||
@@ -352,7 +375,7 @@ public sealed class SequenceTrackerTests
|
|||||||
{
|
{
|
||||||
var s = new SequenceTracker();
|
var s = new SequenceTracker();
|
||||||
|
|
||||||
s.AcceptBirth(256, bdSeq: 8).ShouldBeFalse();
|
s.AcceptNodeBirth(256, bdSeq: 8).ShouldBeFalse();
|
||||||
|
|
||||||
s.BirthBdSeq.ShouldBe(8UL);
|
s.BirthBdSeq.ShouldBe(8UL);
|
||||||
s.IsDeathForCurrentSession(7).ShouldBeFalse();
|
s.IsDeathForCurrentSession(7).ShouldBeFalse();
|
||||||
|
|||||||
Reference in New Issue
Block a user