From 0a7669b48f83ef466ac2701d30087501cd38f6a8 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 10:31:39 +0200 Subject: [PATCH] mesh-mail: a send still approved (the tools' own publish-then-record race) is redelivered later, never refused or told (review round 1 finding 2; issue 1196) --- modules/mesh-mail/cmd/mesh-mail/claim_test.go | 36 +++++++++++++++++++ modules/mesh-mail/cmd/mesh-mail/send.go | 13 +++++++ modules/mesh-mail/cmd/mesh-mail/sender.go | 7 ++++ 3 files changed, 56 insertions(+) diff --git a/modules/mesh-mail/cmd/mesh-mail/claim_test.go b/modules/mesh-mail/cmd/mesh-mail/claim_test.go index 7d3fa295..e12ea0b5 100644 --- a/modules/mesh-mail/cmd/mesh-mail/claim_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/claim_test.go @@ -192,6 +192,42 @@ func TestClaimQueuedSendOnALostRaceSendsNothingAndLeavesTheMessageToTheBus(t *te } } +// 1196/AC5: given a send still approved (publishQueued published to mail-sends before it recorded queued, +// and a message was delivered inside that window), when taken, then it is delivered again later and +// nothing is recorded or told (novox/hq review round 1 finding 2). +func TestClaimQueuedSendOnASendStillApprovedAsksRedeliveryAndRecordsNothing(t *testing.T) { + sends := newMemSends() + req := rawQueuedSend(t, sends) + current, rev, ok, err := getSend(sends, req.ID) + if err != nil || !ok { + t.Fatalf("reading it back: %v %v", err, ok) + } + current.Decided.Outcome = "approved" // not yet advanced to "queued" by the tools' own second write + if _, err := sends.Update(req.ID, current, rev); err != nil { + t.Fatalf("leaving it approved: %v", err) + } + req = current + + _, d, _, outcome, err := claimQueuedSend(sends, newSendRouter().Record, req.ID, judgedAt.Add(3*time.Minute)) + if err != nil { + t.Fatalf("claiming: %v", err) + } + if outcome != claimNotYetQueued { + t.Fatalf("outcome: %v, want claimNotYetQueued", outcome) + } + if d.Outcome != "approved" { + t.Fatalf("decision: %+v, want outcome still approved (unchanged)", d) + } + kept, _, ok, err := getSend(sends, req.ID) + if err != nil || !ok || kept.Decided == nil || kept.Decided.Outcome != "approved" || kept.Decided.Tell != "" { + t.Fatalf("something was recorded or told: %v %v %+v", err, ok, kept.Decided) + } + ids, err := tellPendingIDs(sends) + if err != nil || len(ids) != 0 { + t.Fatalf("a tell was queued despite nothing being told: %v %v", err, ids) + } +} + // raceOnFirstUpdate performs one competing write the first time Update is called — simulating another // delivery's claim landing first — so the caller's own compare-and-set loses with stdio.ErrChanged // (novox/hq issue 1196, AC4). diff --git a/modules/mesh-mail/cmd/mesh-mail/send.go b/modules/mesh-mail/cmd/mesh-mail/send.go index 7fadd3f4..977dfc02 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send.go +++ b/modules/mesh-mail/cmd/mesh-mail/send.go @@ -737,8 +737,17 @@ const ( // message, racing this one); nothing is recorded here and nothing sent; the caller leaves the message to // the bus entirely — no Ack, no Nak, no Term. claimLostRace + // claimNotYetQueued: publishQueued (above) publishes to mail-sends before it records queued, so a + // message delivered in that narrow window finds the record still "approved" — the tools really did + // approve it and are about to record queued, so this is not a refusal: nothing is recorded and nobody + // is told; the caller asks the bus to redeliver shortly (novox/hq review round 1 finding 2). + claimNotYetQueued ) +// NotYetQueuedDelay is how long the sender asks the bus to wait before redelivering a message whose record +// is still "approved" (claimNotYetQueued): long enough for the tools' own second write (queued) to land. +const NotYetQueuedDelay = 5 * time.Second + // claimQueuedSend verifies the operator's warrant for id (mesh-mail's own ask) and claims the send by // compare-and-set from queued or retrying to sending, with the attempt and time, before any connection // (novox/hq ADR 0436 clauses 9, 10). It is never trusted from the queue's message alone: the ask is read @@ -776,6 +785,10 @@ func claimQueuedSend(store SendStore, record RouterRecord, id string, now time.T _ = removeSenderPending(store, req.ID) _ = addTellPending(store, req.ID) return req, d, 0, claimNotProven, err + case req.Decided.Outcome == "approved": + // Not yet visible as queued (publishQueued publishes before it records queued): the tools did + // approve it, so this is not a refusal (novox/hq review round 1 finding 2). + return req, *req.Decided, 0, claimNotYetQueued, nil case req.Decided.Outcome != "queued" && req.Decided.Outcome != "retrying": return refuse(fmt.Sprintf("the send is %q, neither queued nor retrying", req.Decided.Outcome)) } diff --git a/modules/mesh-mail/cmd/mesh-mail/sender.go b/modules/mesh-mail/cmd/mesh-mail/sender.go index 219b6939..1c70983e 100644 --- a/modules/mesh-mail/cmd/mesh-mail/sender.go +++ b/modules/mesh-mail/cmd/mesh-mail/sender.go @@ -225,6 +225,13 @@ func (h *senderHandler) handle(msg *process.Message) { switch outcome { case claimLostRace: return // another delivery of the same message already owns it. + case claimNotYetQueued: + // The tools' own second write (queued) has not landed yet: not a refusal (novox/hq review round 1 + // finding 2). Nothing recorded, nobody told; ask the bus to redeliver shortly. + if err := msg.NakWithDelay(NotYetQueuedDelay); err != nil { + h.logf("[mesh-mail] send: asking %s redelivered: %v", q.Send, err) + } + return case claimRefused, claimNotProven: if err := msg.Term(); err != nil { h.logf("[mesh-mail] send: terminating %s: %v", q.Send, err)