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

@@ -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)
 	}

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])
+	}
+}