From eb97a3d9a2d7a3ee0580870059b0a1e7fd15c25a Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 15 Oct 2026 15:40:16 +0200 Subject: [PATCH] mesh-mail: refuse a send whose undecided index cannot be written, and never overwrite a corrupt index (issue 811, review round 3) --- modules/mesh-mail/cmd/mesh-mail/send.go | 30 +++++++----- modules/mesh-mail/cmd/mesh-mail/send_test.go | 51 ++++++++++++++++++++ 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/modules/mesh-mail/cmd/mesh-mail/send.go b/modules/mesh-mail/cmd/mesh-mail/send.go index 46e402fe..f05f828a 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send.go +++ b/modules/mesh-mail/cmd/mesh-mail/send.go @@ -63,9 +63,9 @@ type undecidedIndex struct { IDs []string `json:"ids"` } -// addUndecided adds id to the undecided index, retrying its compare-and-set write on a race. Best-effort: the send -// itself is already kept by the time this is called, so a failure here costs the sweep sight of it, never the ask -// (mail_send_status and a direct Heard still decide it). +// addUndecided adds id to the undecided index, retrying its compare-and-set write on a race. A stored index that +// cannot be parsed is never overwritten (which would silently drop every id it already named): it is reported as +// an error naming the key instead (novox/hq issue 811, review round 3). func addUndecided(store SendStore, id string) error { for { raw, rev, ok, err := store.Get(undecidedIndexKey) @@ -74,8 +74,8 @@ func addUndecided(store SendStore, id string) error { } var idx undecidedIndex if ok { - if json.Unmarshal(raw, &idx) != nil { - idx = undecidedIndex{} + if err := json.Unmarshal(raw, &idx); err != nil { + return fmt.Errorf("the undecided index %q cannot be parsed: %w", undecidedIndexKey, err) } for _, existing := range idx.IDs { if existing == id { @@ -104,7 +104,8 @@ func addUndecided(store SendStore, id string) error { } // removeUndecided drops id from the undecided index, retrying its compare-and-set write on a race. A send never -// read by the index (already absent) is left as it is. +// read by the index (already absent) is left as it is. A stored index that cannot be parsed is never overwritten: +// it is reported as an error naming the key instead (novox/hq issue 811, review round 3). func removeUndecided(store SendStore, id string) error { for { raw, rev, ok, err := store.Get(undecidedIndexKey) @@ -112,8 +113,8 @@ func removeUndecided(store SendStore, id string) error { return err } var idx undecidedIndex - if json.Unmarshal(raw, &idx) != nil { - return nil + if err := json.Unmarshal(raw, &idx); err != nil { + return fmt.Errorf("the undecided index %q cannot be parsed: %w", undecidedIndexKey, err) } at := -1 for i, existing := range idx.IDs { @@ -136,15 +137,16 @@ func removeUndecided(store SendStore, id string) error { } } -// undecidedIDs is every id the undecided index currently names, for the sweep to read. +// undecidedIDs is every id the undecided index currently names, for the sweep to read. An index that cannot be +// parsed is reported as an error naming the key, not silently read as empty (novox/hq issue 811, review round 3). func undecidedIDs(store SendStore) ([]string, error) { raw, _, ok, err := store.Get(undecidedIndexKey) if err != nil || !ok { return nil, err } var idx undecidedIndex - if json.Unmarshal(raw, &idx) != nil { - return nil, nil + if err := json.Unmarshal(raw, &idx); err != nil { + return nil, fmt.Errorf("the undecided index %q cannot be parsed: %w", undecidedIndexKey, err) } return idx.IDs, nil } @@ -374,7 +376,11 @@ func AskMailSend(in SendInput, store SendStore, bus AskBus, now time.Time) (map[ if _, err := store.Create(req.ID, req); err != nil { return nil, fmt.Errorf("the send could not be kept, so the operator was not asked: %w. Nothing was sent", err) } - _ = addUndecided(store, req.ID) // best-effort (see addUndecided); the send is already kept either way + if err := addUndecided(store, req.ID); err != nil { + // Nothing was published yet, so refusing here costs nothing: an ask the sweep could never see is never + // sent (novox/hq issue 811, review round 3). + return nil, fmt.Errorf("the send could not be made visible to the sweep, so the operator was not asked: %w. Nothing was sent", err) + } if err := bus.Publish(asks.AskSubject(Module), ask, ask.ID); err != nil { return nil, fmt.Errorf("the operator could not be asked (%v). Nothing was sent; mail_send_status %s says so", err, req.ID) } diff --git a/modules/mesh-mail/cmd/mesh-mail/send_test.go b/modules/mesh-mail/cmd/mesh-mail/send_test.go index 53311284..056400c9 100644 --- a/modules/mesh-mail/cmd/mesh-mail/send_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/send_test.go @@ -28,6 +28,11 @@ type memSends struct { vals map[string]json.RawMessage revs map[string]uint64 n uint64 + + // failKey/failErr: when failKey matches, Create and Update report failErr instead of writing, for a test to + // simulate a store whose index write fails (novox/hq issue 811, review round 3). + failKey string + failErr error } func newMemSends() *memSends { @@ -46,6 +51,9 @@ func (m *memSends) Get(key string) (json.RawMessage, uint64, bool, error) { func (m *memSends) Create(key string, value any) (uint64, error) { m.mu.Lock() defer m.mu.Unlock() + if m.failErr != nil && key == m.failKey { + return 0, m.failErr + } if _, ok := m.vals[key]; ok { return 0, stdio.ErrChanged } @@ -54,6 +62,9 @@ func (m *memSends) Create(key string, value any) (uint64, error) { func (m *memSends) Update(key string, value any, revision uint64) (uint64, error) { m.mu.Lock() defer m.mu.Unlock() + if m.failErr != nil && key == m.failKey { + return 0, m.failErr + } if revision == 0 || m.revs[key] != revision { return 0, stdio.ErrChanged } @@ -692,3 +703,43 @@ func TestSweepReadsOnlyUndecidedSends(t *testing.T) { t.Fatalf("the sweep never read the undecided send's router record") } } + +// (12) When the undecided index cannot be written, mail_send refuses the send and publishes no ask: nothing was +// published yet, so refusing costs nothing, and an ask the sweep could never see is never sent (novox/hq issue +// 811, review round 3). +func TestMailSendRefusesWhenTheUndecidedIndexCannotBeWritten(t *testing.T) { + b, router, sends := sendTestBundle(t, Server{Host: "127.0.0.1", Port: 25, Security: "tls"}) + sends.failKey, sends.failErr = undecidedIndexKey, errors.New("the store is down") + rt := startBundle(t, b.Tools()) + + text, isErr := rt.call(SendTool, "novox.triage", sendArgs) + if !isErr { + t.Fatalf("mail_send did not report the undecided index failing to write: %s", text) + } + if n := router.published1(); n != 0 { + t.Fatalf("an ask was published despite the undecided index failing to write: %d", n) + } +} + +// (13) An undecided index that cannot be parsed is never overwritten: addUndecided and removeUndecided fail, +// naming the key, and leave the stored bytes as they were, instead of resetting the index to empty (novox/hq +// issue 811, review round 3). +func TestAddAndRemoveUndecidedFailOnACorruptIndexRatherThanResettingIt(t *testing.T) { + sends := newMemSends() + sends.vals[undecidedIndexKey] = json.RawMessage(`not json`) + sends.revs[undecidedIndexKey] = 1 + + if err := addUndecided(sends, "send-x"); err == nil || !strings.Contains(err.Error(), undecidedIndexKey) { + t.Fatalf("addUndecided did not fail naming the key on a corrupt index: %v", err) + } + if string(sends.vals[undecidedIndexKey]) != "not json" { + t.Fatalf("addUndecided overwrote the corrupt index instead of leaving it: %s", sends.vals[undecidedIndexKey]) + } + + if err := removeUndecided(sends, "send-x"); err == nil || !strings.Contains(err.Error(), undecidedIndexKey) { + t.Fatalf("removeUndecided did not fail naming the key on a corrupt index: %v", err) + } + if string(sends.vals[undecidedIndexKey]) != "not json" { + t.Fatalf("removeUndecided overwrote the corrupt index instead of leaving it: %s", sends.vals[undecidedIndexKey]) + } +}