Bound MarkAbandoned's compare-and-set retries, giving up after 10 tries (issue 711)

internal/link/calls_kept.go

@@ -87,6 +87,10 @@ func (b *BusCalls) Kept(ctx context.Context, id string) (Call, bool, error) {
 	return c, true, nil
 }
 
+// markAbandonedTries bounds the compare-and-set loop below (novox/hq issue 711 review round 1): a
+// record that keeps changing on every read is not this sweep's to keep retrying forever over.
+const markAbandonedTries = 10
+
 // MarkAbandoned marks id abandoned unless its record already says it finished (novox/hq issue 711):
 // a controller starting finds a call running under a holder that stopped, but that holder may have
 // written the finish — with its kept answer — around the same moment. The record's own
@@ -94,9 +98,11 @@ func (b *BusCalls) Kept(ctx context.Context, id string) (Call, bool, error) {
 // reads the record, and if it still says running, writes abandoned at that revision; a finish landing
 // in between fails the write with a revision mismatch, and this rereads and finds the finish, which
 // then stands. A record already resolved, or gone, is returned as found, so the caller never writes
-// abandoned over it.
+// abandoned over it. Past markAbandonedTries attempts, it gives up and returns an error naming the
+// call instead of looping forever (novox/hq issue 711 review round 1); Durably reports that as it
+// reports any other keeper error.
 func (b *BusCalls) MarkAbandoned(ctx context.Context, id string) (Call, error) {
-	for {
+	for try := 0; try < markAbandonedTries; try++ {
 		entry, err := b.kv.Get(ctx, id)
 		if errors.Is(err, jetstream.ErrKeyNotFound) || errors.Is(err, jetstream.ErrInvalidKey) {
 			return Call{}, nil
@@ -125,6 +131,7 @@ func (b *BusCalls) MarkAbandoned(ctx context.Context, id string) (Call, error) {
 		}
 		return c, nil
 	}
+	return Call{}, fmt.Errorf("%s: its record kept changing; it is left as it stands", id)
 }
 
 // Recent is every call the bucket holds, newest first, without answers: read once through a watch

internal/link/calls_kept_test.go

@@ -230,6 +230,40 @@ func TestNatsAFinishRacingTheSweepsWriteWins(t *testing.T) {
 	}
 }
 
+// alwaysMismatchKV wraps a jetstream.KeyValue whose Update always fails with a revision mismatch, as
+// if some other writer kept winning the race forever (novox/hq issue 711 review round 1).
+type alwaysMismatchKV struct {
+	jetstream.KeyValue
+	tries *int
+}
+
+func (a alwaysMismatchKV) Update(ctx context.Context, key string, value []byte, revision uint64) (uint64, error) {
+	*a.tries++
+	return 0, jetstream.ErrKeyRevisionMismatch
+}
+
+// **MarkAbandoned gives up after markAbandonedTries rather than loop forever** (novox/hq issue 711
+// review round 1): a record whose compare-and-set keeps losing is left as it stands, said in an
+// error naming the call, not retried without end.
+func TestNatsAMarkAbandonedGivesUpAfterTenTries(t *testing.T) {
+	keeper := callsBucket(t)
+	running := Call{ID: "call-711-4", Seat: "mesh-controller", Verb: "push", State: CallRunning, Holder: "controller@one"}
+	if err := keeper.Keep(t.Context(), running); err != nil {
+		t.Fatal(err)
+	}
+	waitKept(t, keeper, running.ID, CallRunning)
+
+	tries := 0
+	stuck := &BusCalls{kv: alwaysMismatchKV{KeyValue: keeper.kv, tries: &tries}}
+	_, err := stuck.MarkAbandoned(t.Context(), running.ID)
+	if err == nil || !strings.Contains(err.Error(), running.ID) {
+		t.Fatalf("expected an error naming %s, got %v", running.ID, err)
+	}
+	if tries != markAbandonedTries {
+		t.Fatalf("tried %d times, want %d", tries, markAbandonedTries)
+	}
+}
+
 // The bucket holds the last thousand calls or fourteen days: a call is two keys, so the stream under
 // it holds twice as many messages, and asserting it again keeps the count.
 func TestNatsTheCallsBucketIsBounded(t *testing.T) {