From acb82992eec252c39ee33a827c394411682d0d92 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 10:38:07 +0200 Subject: [PATCH] mesh-mail: index a send for a tell or mail_health's overdue check before its record carries the outcome, drop the old index entry only after, and report an index write that fails instead of dropping it silently (review round 1 finding 3; issues 1194, 1199) --- modules/mesh-mail/cmd/mesh-mail/claim_test.go | 75 ++++++++++ .../mesh-mail/cmd/mesh-mail/outcome_test.go | 48 +++++- modules/mesh-mail/cmd/mesh-mail/send.go | 139 ++++++++++++------ modules/mesh-mail/cmd/mesh-mail/sender.go | 10 +- modules/mesh-mail/cmd/mesh-mail/tell_test.go | 28 ++++ 5 files changed, 246 insertions(+), 54 deletions(-) diff --git a/modules/mesh-mail/cmd/mesh-mail/claim_test.go b/modules/mesh-mail/cmd/mesh-mail/claim_test.go index e12ea0b5..da27507f 100644 --- a/modules/mesh-mail/cmd/mesh-mail/claim_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/claim_test.go @@ -8,6 +8,7 @@ package main import ( "encoding/json" + "errors" "strings" "testing" "time" @@ -228,6 +229,80 @@ func TestClaimQueuedSendOnASendStillApprovedAsksRedeliveryAndRecordsNothing(t *t } } +// review round 1 finding 3: a failed write of the tells-pending index does not stop the refusal itself +// from being recorded — the record write and the message's own fate are never undone by an index that +// could not be kept — but the index failure is still reported, never silently dropped. +func TestClaimQueuedSendStillRefusesWhenTheTellsPendingIndexCannotBeWritten(t *testing.T) { + sends, router := newMemSends(), newSendRouter() + req := rawQueuedSend(t, sends) + sends.failKey, sends.failErr = tellsPendingKey, errors.New("the store is down") + + _, d, _, outcome, err := claimQueuedSend(sends, router.Record, req.ID, judgedAt.Add(3*time.Minute)) + if outcome != claimRefused || !strings.HasPrefix(d.Outcome, "refused: ") { + t.Fatalf("outcome %v, decision %+v", outcome, d) + } + if err == nil { + t.Fatalf("the index failure was not reported") + } + kept, _, ok, kerr := getSend(sends, req.ID) + if kerr != nil || !ok || kept.Decided == nil || kept.Decided.Outcome != d.Outcome { + t.Fatalf("the refusal itself was not kept despite the index failing: %v %v %+v", kerr, ok, kept.Decided) + } +} + +// Same finding, for the redelivery-finds-sending path (not proven). +func TestClaimQueuedSendStillRecordsNotProvenWhenTheTellsPendingIndexCannotBeWritten(t *testing.T) { + sends, router := newMemSends(), newSendRouter() + 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 = "sending" + if _, err := sends.Update(req.ID, current, rev); err != nil { + t.Fatalf("leaving it sending: %v", err) + } + sends.failKey, sends.failErr = tellsPendingKey, errors.New("the store is down") + + _, d, _, outcome, err := claimQueuedSend(sends, router.Record, req.ID, judgedAt.Add(4*time.Minute)) + if outcome != claimNotProven || d.Outcome != "not proven" { + t.Fatalf("outcome %v, decision %+v", outcome, d) + } + if err == nil { + t.Fatalf("the index failure was not reported") + } + kept, _, ok2, kerr := getSend(sends, req.ID) + if kerr != nil || !ok2 || kept.Decided == nil || kept.Decided.Outcome != "not proven" { + t.Fatalf("not proven was not kept despite the index failing: %v %v %+v", kerr, ok2, kept.Decided) + } +} + +// Same finding, for publishQueued: a failed sender-pending index write does not stop "queued" from being +// recorded, since the bus already took the message once. +func TestPublishQueuedStillRecordsQueuedWhenTheSenderPendingIndexCannotBeWritten(t *testing.T) { + sends := newMemSends() + req := SendRequest{ID: newSendID(), Account: "mesh", To: []string{"ops@example.org"}, Subject: "s", Body: "body", At: judgedAt} + claim := SendDecision{At: judgedAt, Outcome: "approved"} + req.Decided = &claim + rev, err := sends.Create(req.ID, req) + if err != nil { + t.Fatal(err) + } + sends.failKey, sends.failErr = senderPendingKey, errors.New("the store is down") + queue := newMemQueue() + + d, err := publishQueued(sends, req, rev, claim, queue.publish, judgedAt.Add(time.Minute)) + if d.Outcome != "queued" { + t.Fatalf("decision: %+v, want queued despite the index failing", d) + } + if err == nil { + t.Fatalf("the index failure was not reported") + } + if n := queue.count(); n != 1 { + t.Fatalf("published %d time(s)", n) + } +} + // 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/outcome_test.go b/modules/mesh-mail/cmd/mesh-mail/outcome_test.go index 816447f9..367d4dca 100644 --- a/modules/mesh-mail/cmd/mesh-mail/outcome_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/outcome_test.go @@ -46,7 +46,10 @@ func outcomeTestClaim(t *testing.T, attempt int) (SendRequest, SendDecision, uin func TestRecordOutcomeOnSuccessRecordsSentAndEmitsIt(t *testing.T) { req, claimed, rev, sends := outcomeTestClaim(t, 1) emit := &memEmit{} - d, action, _ := recordOutcome(sends, emit.emit, req, claimed, rev, nil, judgedAt.Add(2*time.Minute)) + d, action, _, err := recordOutcome(sends, emit.emit, req, claimed, rev, nil, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("recordOutcome: %v", err) + } if d.Outcome != "sent" { t.Fatalf("outcome: %+v", d) } @@ -83,7 +86,10 @@ func TestRecordOutcomeOnNotTakenRetriesUpToTheFourthAttempt(t *testing.T) { req, claimed, rev, sends := outcomeTestClaim(t, n) emit := &memEmit{} now := judgedAt.Add(2 * time.Minute) - d, action, delay := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: NotTaken, Err: errors.New("no connection")}, now) + d, action, delay, err := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: NotTaken, Err: errors.New("no connection")}, now) + if err != nil { + t.Fatalf("attempt %d: recordOutcome: %v", n, err) + } if d.Outcome != "retrying" || d.Attempt != n { t.Fatalf("attempt %d: outcome %+v", n, d) } @@ -102,8 +108,11 @@ func TestRecordOutcomeOnNotTakenRetriesUpToTheFourthAttempt(t *testing.T) { func TestRecordOutcomeOnNotTakenFailsOnTheFifthAttempt(t *testing.T) { req, claimed, rev, sends := outcomeTestClaim(t, MostAttempts) emit := &memEmit{} - d, action, _ := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: NotTaken, Err: errors.New("no connection")}, + d, action, _, err := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: NotTaken, Err: errors.New("no connection")}, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("recordOutcome: %v", err) + } if !strings.HasPrefix(d.Outcome, "failed: ") || d.Attempt != MostAttempts { t.Fatalf("outcome: %+v", d) } @@ -120,8 +129,11 @@ func TestRecordOutcomeOnNotTakenFailsOnTheFifthAttempt(t *testing.T) { func TestRecordOutcomeOnRefusedRecordsFailedAndTerminates(t *testing.T) { req, claimed, rev, sends := outcomeTestClaim(t, 1) emit := &memEmit{} - d, action, _ := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: Refused, Err: errors.New("550 no such mailbox")}, + d, action, _, err := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: Refused, Err: errors.New("550 no such mailbox")}, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("recordOutcome: %v", err) + } if !strings.HasPrefix(d.Outcome, "failed: ") || !strings.Contains(d.Outcome, "550 no such mailbox") { t.Fatalf("outcome: %+v", d) } @@ -136,8 +148,11 @@ func TestRecordOutcomeOnRefusedRecordsFailedAndTerminates(t *testing.T) { func TestRecordOutcomeOnNotProvenRecordsNotProvenAndTerminates(t *testing.T) { req, claimed, rev, sends := outcomeTestClaim(t, 1) emit := &memEmit{} - d, action, _ := recordOutcome(sends, emit.emit, req, claimed, rev, + d, action, _, err := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: NotProven, Err: errors.New("connection lost after the data was written")}, judgedAt.Add(2*time.Minute)) + if err != nil { + t.Fatalf("recordOutcome: %v", err) + } if d.Outcome != "not proven" { t.Fatalf("outcome: %+v", d) } @@ -148,3 +163,26 @@ func TestRecordOutcomeOnNotProvenRecordsNotProvenAndTerminates(t *testing.T) { t.Fatalf("emitted: %+v", emit.calls) } } + +// review round 1 finding 3: a failed write of the tells-pending index does not stop the failed decision +// itself from being recorded and emitted — only the index write is reported as failing. +func TestRecordOutcomeStillRecordsFailedWhenTheTellsPendingIndexCannotBeWritten(t *testing.T) { + req, claimed, rev, sends := outcomeTestClaim(t, 1) + sends.failKey, sends.failErr = tellsPendingKey, errors.New("the store is down") + emit := &memEmit{} + d, action, _, err := recordOutcome(sends, emit.emit, req, claimed, rev, &SendError{Outcome: Refused, Err: errors.New("550 no such mailbox")}, + judgedAt.Add(2*time.Minute)) + if err == nil { + t.Fatalf("the index failure was not reported") + } + if !strings.HasPrefix(d.Outcome, "failed: ") || action != actionTerm { + t.Fatalf("outcome %+v, action %v", d, action) + } + kept, _, ok, kerr := getSend(sends, req.ID) + if kerr != nil || !ok || kept.Decided == nil || kept.Decided.Outcome != d.Outcome { + t.Fatalf("the decision itself was not kept despite the index failing: %v %v %+v", kerr, ok, kept.Decided) + } + if len(emit.calls) != 1 { + t.Fatalf("emitted %d time(s) despite the index failing", len(emit.calls)) + } +} diff --git a/modules/mesh-mail/cmd/mesh-mail/send.go b/modules/mesh-mail/cmd/mesh-mail/send.go index 977dfc02..f8dcc328 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send.go +++ b/modules/mesh-mail/cmd/mesh-mail/send.go @@ -695,21 +695,28 @@ func publishQueued(store SendStore, req SendRequest, rev uint64, approved SendDe if err := publish(req.ID, QueuedSend{Send: req.ID, Account: req.Account, Ask: req.ID}); err != nil { return approved, nil } + // mail_health's own overdue check (Overdue, issue 1199) needs to find every send the sender now owns, + // the same way the sweep needs the undecided index: a reserved key, since neither side's own state over + // mesh-sdk go/process lists its keys whole. Indexed BEFORE the record carries "queued" (novox/hq review + // round 1 finding 3): a crash between this and the write below must still leave it findable once + // "queued" does land — Overdue itself only ever matches "queued" or "retrying", so an entry that still + // reads "approved" in the meantime is simply not yet counted overdue. + indexErr := addSenderPending(store, req.ID) queued := SendDecision{At: now, Outcome: "queued", Warrant: approved.Warrant} final, newRev, err := updateDecision(store, req, rev, queued) if err != nil { // The publish stands either way: the bus already took the message once. The caller logs this; it is // not retried (a retry here would publish a second message under a new id). - return approved, fmt.Errorf("the send %s was queued, and that could not be kept (%w)", req.ID, err) + return approved, errOr(indexErr, fmt.Errorf("the send %s was queued, and that could not be kept (%w)", req.ID, err)) } if newRev != 0 { - _ = removeUndecided(store, req.ID) // from here the sender owns it; the tools' sweep is done with it - // mail_health's own overdue check (issue 1199) needs to find every send the sender now owns, the - // same way the sweep needed the undecided index: a reserved key, since neither side's own state - // over mesh-sdk go/process lists its keys whole. - _ = addSenderPending(store, req.ID) + // Only once "queued" is durably recorded is the undecided index's entry dropped: removing it first, + // then crashing before the write, would have left the send unreachable by the sweep too. + if rmErr := removeUndecided(store, req.ID); rmErr != nil { + indexErr = errOr(indexErr, fmt.Errorf("dropping %s from the undecided index: %w", req.ID, rmErr)) + } } - return final, nil + return final, indexErr } // ---- the sender's own claim (novox/hq ADR 0436 clauses 9, 10, issue 1196) -------------------------------------- @@ -772,19 +779,33 @@ func claimQueuedSend(store SendStore, record RouterRecord, id string, now time.T // Every other refusal below follows a real Approve (the send reached at least queued): clause 9's own // refusal owes the operator a tell, same as clause 10's (novox/hq ADR 0436 clause 11, issue 1199). refuse := func(reason string) (SendRequest, SendDecision, uint64, claimOutcome, error) { + // Indexed for a tell BEFORE the record carries "refused" (novox/hq review round 1 finding 3): a + // crash between this and the write below must still leave it findable once refused does land; + // tellSend itself tolerates finding it still sending, queued or retrying in the meantime. + indexErr := addTellPending(store, req.ID) d, _, err := settleSend(store, req, rev, SendDecision{At: now, Outcome: "refused: " + reason}) - _ = removeSenderPending(store, req.ID) - _ = addTellPending(store, req.ID) - return req, d, 0, claimRefused, err + if err != nil { + return req, d, 0, claimRefused, errOr(indexErr, err) + } + // Only once refused is durably recorded is the sender-pending index's entry dropped. + if rmErr := removeSenderPending(store, req.ID); rmErr != nil { + indexErr = errOr(indexErr, fmt.Errorf("dropping %s from the sender-pending index: %w", req.ID, rmErr)) + } + return req, d, 0, claimRefused, indexErr } switch { case req.Decided.Outcome == "sending": // A previous delivery may have handed it to the server already; never sent twice, and never - // retried automatically either way (ADR 0436 clause 10). + // retried automatically either way (ADR 0436 clause 10). Same ordering as refuse() above. + indexErr := addTellPending(store, req.ID) d, _, err := settleSend(store, req, rev, SendDecision{At: now, Outcome: "not proven", Attempt: req.Decided.Attempt}) - _ = removeSenderPending(store, req.ID) - _ = addTellPending(store, req.ID) - return req, d, 0, claimNotProven, err + if err != nil { + return req, d, 0, claimNotProven, errOr(indexErr, err) + } + if rmErr := removeSenderPending(store, req.ID); rmErr != nil { + indexErr = errOr(indexErr, fmt.Errorf("dropping %s from the sender-pending index: %w", req.ID, rmErr)) + } + return req, d, 0, claimNotProven, indexErr 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). @@ -882,31 +903,34 @@ const ( // *SendError, exactly as a real SendMail failure would be: safe to retry), and says what the caller does // with the message next. Attempt (req's own, from the claim) decides retrying from failed on a not-taken // failure: the fifth attempt always fails rather than retries again (novox/hq ADR 0436 clauses 10, 11). -func recordOutcome(store SendStore, emit Emit, req SendRequest, claimed SendDecision, rev uint64, sendErr error, now time.Time) (SendDecision, msgAction, time.Duration) { - write := func(d SendDecision, final bool) SendDecision { - result, _, err := settleSend(store, req, rev, d) - if final { - // From here the sender is done with it, one way or another (novox/hq issue 1199): mail_health's - // overdue check no longer needs to find it. - _ = removeSenderPending(store, req.ID) +func recordOutcome(store SendStore, emit Emit, req SendRequest, claimed SendDecision, rev uint64, sendErr error, now time.Time) (SendDecision, msgAction, time.Duration, error) { + write := func(d SendDecision, final, tellWorthy bool) (SendDecision, error) { + var indexErr error + if tellWorthy { + // Indexed for a tell BEFORE the record carries the outcome that owes it (novox/hq review round + // 1 finding 3): a crash between this and the write below must still leave it findable once the + // outcome does land; tellSend itself tolerates finding it still sending, queued or retrying. + indexErr = addTellPending(store, req.ID) } + result, _, err := settleSend(store, req, rev, d) if err != nil { // The attempt (and its SMTP outcome) stand either way; the caller logs this. A redelivery (or the // sweep, for a send stuck mid-write) finds whatever the real kept state is and reconciles. - return d + return d, errOr(indexErr, err) } - return result - } - tell := func(outcome string) { - // Owed a tell (failed, refused or not proven, novox/hq ADR 0436 clause 11): the index the sender's - // own look reads, kept to once by tell: telling then told (issue 1199). - _ = addTellPending(store, req.ID) - _ = emit(MailSendFailed, req.ID, OutcomeEvent{Send: req.ID, Account: req.Account, Outcome: outcome}) + if final { + // Only once the outcome is durably recorded is the sender-pending index's entry dropped: from + // here the sender is done with it, one way or another (novox/hq issue 1199). + if rmErr := removeSenderPending(store, req.ID); rmErr != nil { + indexErr = errOr(indexErr, fmt.Errorf("dropping %s from the sender-pending index: %w", req.ID, rmErr)) + } + } + return result, indexErr } if sendErr == nil { - d := write(SendDecision{At: now, Outcome: "sent", Warrant: claimed.Warrant, Attempt: claimed.Attempt}, true) + d, err := write(SendDecision{At: now, Outcome: "sent", Warrant: claimed.Warrant, Attempt: claimed.Attempt}, true, false) _ = emit(MailSendSent, req.ID, OutcomeEvent{Send: req.ID, Account: req.Account, Outcome: d.Outcome}) - return d, actionAck, 0 + return d, actionAck, 0, err } var se *SendError if !errors.As(sendErr, &se) { @@ -917,26 +941,26 @@ func recordOutcome(store SendStore, emit Emit, req SendRequest, claimed SendDeci } switch se.Outcome { case Refused: - d := write(SendDecision{At: now, Outcome: "failed: " + se.Err.Error(), Warrant: claimed.Warrant, - Attempt: claimed.Attempt, Error: se.Err.Error()}, true) - tell(d.Outcome) - return d, actionTerm, 0 + d, err := write(SendDecision{At: now, Outcome: "failed: " + se.Err.Error(), Warrant: claimed.Warrant, + Attempt: claimed.Attempt, Error: se.Err.Error()}, true, true) + _ = emit(MailSendFailed, req.ID, OutcomeEvent{Send: req.ID, Account: req.Account, Outcome: d.Outcome}) + return d, actionTerm, 0, err case NotProven: - d := write(SendDecision{At: now, Outcome: "not proven", Warrant: claimed.Warrant, Attempt: claimed.Attempt, - Error: se.Err.Error()}, true) - tell(d.Outcome) - return d, actionTerm, 0 + d, err := write(SendDecision{At: now, Outcome: "not proven", Warrant: claimed.Warrant, Attempt: claimed.Attempt, + Error: se.Err.Error()}, true, true) + _ = emit(MailSendFailed, req.ID, OutcomeEvent{Send: req.ID, Account: req.Account, Outcome: d.Outcome}) + return d, actionTerm, 0, err default: // NotTaken if claimed.Attempt >= MostAttempts { - d := write(SendDecision{At: now, Outcome: "failed: " + se.Err.Error(), Warrant: claimed.Warrant, - Attempt: claimed.Attempt, Error: se.Err.Error()}, true) - tell(d.Outcome) - return d, actionTerm, 0 + d, err := write(SendDecision{At: now, Outcome: "failed: " + se.Err.Error(), Warrant: claimed.Warrant, + Attempt: claimed.Attempt, Error: se.Err.Error()}, true, true) + _ = emit(MailSendFailed, req.ID, OutcomeEvent{Send: req.ID, Account: req.Account, Outcome: d.Outcome}) + return d, actionTerm, 0, err } delay := RetryDelays[claimed.Attempt-1] - d := write(SendDecision{At: now, Outcome: "retrying", Warrant: claimed.Warrant, Attempt: claimed.Attempt, - Error: se.Err.Error(), NextAttempt: now.Add(delay)}, false) - return d, actionRetry, delay // not a final outcome: no emit (clause 11 names only sent and failed) + d, err := write(SendDecision{At: now, Outcome: "retrying", Warrant: claimed.Warrant, Attempt: claimed.Attempt, + Error: se.Err.Error(), NextAttempt: now.Add(delay)}, false, false) + return d, actionRetry, delay, err // not a final outcome: no emit (clause 11 names only sent and failed) } } @@ -947,6 +971,16 @@ func recordOutcome(store SendStore, emit Emit, req SendRequest, claimed SendDeci // send ever sent, not with the few still live): a reserved key stands in for a list, exactly as the // undecided index already does. +// errOr is a, or b when a is nil: the first of two errors a caller that can only report one keeps (novox/hq +// review round 1 finding 3), so an index-bookkeeping failure is never silently dropped once there is also a +// record-write failure to report, and vice versa. +func errOr(a, b error) error { + if a != nil { + return a + } + return b +} + // idIndex is the shape a send-id index keeps: generic across all three (undecided, sender-pending, // tells-pending). type idIndex struct { @@ -1135,7 +1169,18 @@ func tellSend(store SendStore, tell TellOperator, id string, now time.Time) erro if err != nil { return err } - if !ok || req.Decided == nil || !needsTell(req.Decided.Outcome) || req.Decided.Tell == "told" { + if !ok || req.Decided == nil { + _ = removeTellPending(store, id) + return nil + } + switch req.Decided.Outcome { + case "sending", "queued", "retrying": + // Indexed for a tell before the outcome that owes it had actually landed (addTellPending now runs + // before the write, novox/hq review round 1 finding 3): not decided yet whether a tell is owed. + // Left in the index; the next look finds it once the write lands. + return nil + } + if !needsTell(req.Decided.Outcome) || req.Decided.Tell == "told" { _ = removeTellPending(store, id) return nil } diff --git a/modules/mesh-mail/cmd/mesh-mail/sender.go b/modules/mesh-mail/cmd/mesh-mail/sender.go index 1c70983e..75a35316 100644 --- a/modules/mesh-mail/cmd/mesh-mail/sender.go +++ b/modules/mesh-mail/cmd/mesh-mail/sender.go @@ -219,8 +219,11 @@ func (h *senderHandler) handle(msg *process.Message) { } req, claimed, rev, outcome, err := claimQueuedSend(h.Store, h.Record, q.Send, h.now()) if err != nil { + // Logged, never returned on alone: an index-bookkeeping error (novox/hq review round 1 finding 3) + // can stand beside a real decision (claimRefused, claimNotProven) that the switch below still + // must act on; a fatal read or claim failure, with no decision made, always reads claimLostRace, + // which the switch already leaves to the bus. h.logf("[mesh-mail] send: claiming %s: %v", q.Send, err) - return // left to the bus: a transient read or write failure, never a verified refusal. } switch outcome { case claimLostRace: @@ -239,7 +242,10 @@ func (h *senderHandler) handle(msg *process.Message) { return } sendErr := h.attempt(req) - _, action, delay := recordOutcome(h.Store, h.Emit, req, claimed, rev, sendErr, h.now()) + _, action, delay, err := recordOutcome(h.Store, h.Emit, req, claimed, rev, sendErr, h.now()) + if err != nil { + h.logf("[mesh-mail] send: recording the outcome of %s: %v", q.Send, err) + } switch action { case actionAck: if err := msg.Ack(); err != nil { diff --git a/modules/mesh-mail/cmd/mesh-mail/tell_test.go b/modules/mesh-mail/cmd/mesh-mail/tell_test.go index adec5175..c6f237aa 100644 --- a/modules/mesh-mail/cmd/mesh-mail/tell_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/tell_test.go @@ -125,6 +125,34 @@ func TestTellSendNeverTellsForASentSend(t *testing.T) { } } +// review round 1 finding 3: a send indexed for a tell before its own outcome write has landed (recordOutcome +// and claimQueuedSend now index first) must not be dropped by a look that finds it still sending, queued or +// retrying — it is left in the index for the next look, once the real outcome lands. +func TestTellSendKeepsAnEntryWhoseRecordIsStillInFlight(t *testing.T) { + for _, outcome := range []string{"sending", "queued", "retrying"} { + sends := newMemSends() + req := SendRequest{ID: newSendID(), Account: "mesh", At: judgedAt} + req.Decided = &SendDecision{At: judgedAt, Outcome: outcome} + if _, err := sends.Create(req.ID, req); err != nil { + t.Fatal(err) + } + if err := addTellPending(sends, req.ID); err != nil { + t.Fatal(err) + } + tell := &memTell{} + if err := tellSend(sends, tell.tell, req.ID, judgedAt.Add(time.Minute)); err != nil { + t.Fatalf("%s: telling: %v", outcome, err) + } + if len(tell.headlines) != 0 { + t.Fatalf("%s: told before its outcome landed: %v", outcome, tell.headlines) + } + ids, err := tellPendingIDs(sends) + if err != nil || len(ids) != 1 { + t.Fatalf("%s: dropped from the index while still in flight: %v %v", outcome, err, ids) + } + } +} + // 1199/AC2: given a send queued more than 10 minutes ago, or retrying 10 minutes past its next attempt, // when mail_health runs (Overdue), then it names the send; a send waiting for its next attempt is never // overdue.