diff --git a/internal/link/window.go b/internal/link/window.go new file mode 100644 index 0000000..2b89a15 --- /dev/null +++ b/internal/link/window.go @@ -0,0 +1,106 @@ +package link + +import ( + "errors" + "time" + + "github.com/novox/mesh-controller/internal/inventory" +) + +// The store window, as a decision rather than a mechanism. +// +// The guarantee (novox/hq ADR 0083): a push the controller cannot record because its store is +// restarting is **held and retried**, not dropped and not falsely acknowledged. On the bus the +// mesh runs on today that is done by keeping the delivery unacknowledged in memory and settling +// it later. On the bus being built it is a `nak` with a delay: the server holds it and redelivers, +// so the controller keeps no list of parked messages and a controller that restarts mid-window +// loses nothing it was holding. +// +// **The decision is the same either way, and the mechanism is not the interesting part.** What is +// interesting is that moving the holding into the server introduces a problem the in-memory +// version did not have, and the answer was already in the message. + +// Verdict is what to do with one control message. +type Verdict int + +const ( + // Take it: apply, then acknowledge. + Take Verdict = iota + // Hold it: the store cannot record this yet. Nak with a delay and let the server redeliver. + Hold + // Stale: this is about a declaration the node has already moved past, and applying it would + // undo what came after. Acknowledge without acting — redelivering forever is worse. + Stale + // GiveUp: the store has not come back in time. Settle it and say so, loudly. + GiveUp +) + +// StoreWindow decides. Pure, so the guarantee is testable without a bus, a store or a clock. +type StoreWindow struct { + // GiveUpAfter is how long one message may be held before it is let go with a line saying so. + GiveUpAfter time.Duration +} + +// Decide answers for one delivery. +// +// - err is what the store said, or nil. +// - declaredIn is the digest of the declaration this message is about, empty when it is not +// about one (an enrolment, a build result). +// - outstanding is the digest the mesh last sent that node, empty when it has sent none. +// - heldFor is how long this message has already been held; zero on first delivery. +func (w StoreWindow) Decide(err error, declaredIn, outstanding string, heldFor time.Duration) Verdict { + // **Staleness is checked before the store, not after.** A redelivery that lost its race is + // not worth waiting on a store for, and asking the store first would mean a message about a + // superseded declaration holding a slot in the window that a current one needs. + if declaredIn != "" && outstanding != "" && declaredIn != outstanding { + return Stale + } + if err == nil { + return Take + } + if !errors.Is(err, ErrTryAgain) && !inventory.Unreachable(err) { + // Not the store being away: a refusal is an answer, and holding it would turn a message + // the mesh understood into one it retries forever. + return Take + } + if heldFor >= w.GiveUpAfter { + return GiveUp + } + return Hold +} + +// RedeliverAfter is how long the server should hold a naked message before trying again. +// +// Backed off, and bounded. A store restarting is back in seconds; a store that is gone is not +// helped by being asked every second, and the delay is what keeps a window of held messages from +// becoming a spin. +func RedeliverAfter(heldFor time.Duration) time.Duration { + switch { + case heldFor < 5*time.Second: + return time.Second + case heldFor < 30*time.Second: + return 5 * time.Second + default: + return 15 * time.Second + } +} + +// The problem holding-in-the-server introduces, and why the answer was already in the message. +// +// Holding a delivery in memory let the controller do something a server cannot: when a newer +// report for the same node arrived, it dropped the older one, "because acting on it after the +// newer would undo the newer". A `nak`ed message is the server's, and the server will redeliver +// it whatever else has happened in the meantime — so the older report comes back *after* the +// newer was applied, and applying it would undo exactly what that comment describes. +// +// **A report already says which declaration it is about.** `Declared` is the digest of the exact +// bytes the mesh sent, and it exists because an earlier attempt to order reports by time lost the +// race it invited — an apply that started under the previous declaration finishes after the next +// is sent, and the report reads as newer than the send. Clocks cannot answer *which*; the digest +// is the answer itself. +// +// So supersession stops being a thing the controller remembers and becomes a thing it checks: a +// report whose digest is not the one outstanding for that node is stale, and is acknowledged +// without being acted on. Which is the same shape as a node refusing a superseded declaration by +// sequence (novox/hq issue 107) — ordering settled by what the message says, not by when it +// happened to arrive. diff --git a/internal/link/window_test.go b/internal/link/window_test.go new file mode 100644 index 0000000..531efea --- /dev/null +++ b/internal/link/window_test.go @@ -0,0 +1,84 @@ +package link + +import ( + "errors" + "testing" + "time" +) + +func window() StoreWindow { return StoreWindow{GiveUpAfter: 2 * time.Minute} } + +// The guarantee itself (novox/hq ADR 0083): a push the store cannot record is held, not dropped +// and not falsely acknowledged. +func TestAMessageTheStoreCannotTakeYetIsHeld(t *testing.T) { + if got := window().Decide(ErrTryAgain, "", "", 0); got != Hold { + t.Fatalf("got %v; a push the store could not record was not held", got) + } +} + +// A refusal is an answer. Holding it would turn a message the mesh understood into one it +// retries forever. +func TestARefusalIsNotHeld(t *testing.T) { + if got := window().Decide(errors.New("that node does not exist"), "", "", 0); got != Take { + t.Fatalf("got %v; a refusal was mistaken for the store being away", got) + } +} + +// Held has a limit, and past it the message is settled rather than held for ever. +func TestAStoreThatNeverComesBackEndsTheHold(t *testing.T) { + if got := window().Decide(ErrTryAgain, "", "", 3*time.Minute); got != GiveUp { + t.Fatalf("got %v; the window has no end", got) + } + if got := window().Decide(ErrTryAgain, "", "", time.Minute); got != Hold { + t.Fatalf("got %v; the window ended early", got) + } +} + +// **The problem that holding in the server introduces.** A naked message is redelivered whatever +// else happened meanwhile, so a report about a superseded declaration comes back after the newer +// one was applied — and applying it would undo the newer. +func TestAReportAboutASupersededDeclarationIsNotApplied(t *testing.T) { + got := window().Decide(nil, "digest-of-the-old-one", "digest-of-the-current-one", 0) + if got != Stale { + t.Fatalf("got %v; a redelivery that lost its race would have undone what came after", got) + } +} + +func TestAReportAboutTheOutstandingDeclarationIsApplied(t *testing.T) { + if got := window().Decide(nil, "same", "same", 0); got != Take { + t.Fatalf("got %v; a current report was discarded", got) + } +} + +// A message that is about no declaration — an enrolment, a build result — is never stale: there +// is nothing for it to be out of date with. +func TestAMessageAboutNoDeclarationIsNeverStale(t *testing.T) { + if got := window().Decide(nil, "", "whatever-is-outstanding", 0); got != Take { + t.Fatalf("got %v; an enrolment was treated as a stale report", got) + } + if got := window().Decide(nil, "a-digest", "", 0); got != Take { + t.Fatalf("got %v; a report was called stale against a node that was sent nothing", got) + } +} + +// Staleness is decided before the store is waited on: a redelivery that lost its race must not +// hold a slot in the window that a current message needs. +func TestAStaleMessageIsNotHeldForTheStore(t *testing.T) { + if got := window().Decide(ErrTryAgain, "old", "current", 0); got != Stale { + t.Fatalf("got %v; a superseded message was held for a store it would never be applied to", got) + } +} + +// The delay backs off: a store that is gone is not helped by being asked every second, and the +// delay is what keeps a window of held messages from becoming a spin. +func TestRedeliveryBacksOff(t *testing.T) { + first := RedeliverAfter(0) + later := RedeliverAfter(10 * time.Second) + last := RedeliverAfter(time.Minute) + if !(first < later && later < last) { + t.Fatalf("delays do not back off: %v %v %v", first, later, last) + } + if last > 30*time.Second { + t.Fatalf("a held message waits %v between attempts, which is longer than a store restart", last) + } +}