Keep a question's slot until it expires while the router has not recorded it (issue 589, review 107)

Freeing the slot two minutes after asking could let a backlogged router still show
the question after the session read it as ended, and let a third question in. The
seat's queue offers work again after an error and the router drops a cancel of an
ask it has not recorded, so a cancel cannot take it back; once expired, the router
refuses it if taken late. The slot test now uses a stale revision, so a plain
write in place of the compare-and-set fails it.

modules/claude-code/cmd/claude-code/questions.go

@@ -43,10 +43,11 @@ const QuestionPrefix = "q-"
 // create or by compare-and-set, so two nodes asking at once cannot both take the last one.
 const MostQuestions = asks.MostOpen - 1
 
-// RouterHoldsWithin is how long after a question is asked the router has recorded it: it records an ask before it
-// takes its work. A question it holds no record of after that was refused (and the refusal not heard here) or never
-// taken: it no longer holds a slot, and reads as ended unless a record of it appears after all.
-var RouterHoldsWithin = 2 * time.Minute
+// A question the router holds no record of, and whose refusal was not heard here, keeps its slot until it expires:
+// the router may still have it in its queue (down, or backlogged), and neither the queue's order nor a cancel can
+// take it back — the queue offers a piece of work again after an error, so a cancel may be taken before the ask it
+// cancels, and a cancel of an ask not yet recorded is dropped. Once the question has expired the router refuses it
+// when it is taken (asks.Ask.Check), so the slot is free and the question has ended.
 
 // slotKey is the key of one of the MostQuestions slots: not a question's key, so no question is ever read from it.
 func slotKey(n int) string { return fmt.Sprintf("slot-%d", n) }
@@ -381,8 +382,8 @@ func labelsOf(q Question) string {
 	return strings.Join(out, ", ")
 }
 
-// questionOpen says whether a question still waits: kept, asked, not ended, not expired, and held by the router or
-// asked less than RouterHoldsWithin ago. An unreadable record is open: a slot is never freed on a guess.
+// questionOpen says whether a question still waits: kept, asked, not ended, not expired, and open in the router's
+// record or not recorded yet. An unreadable record is open: a slot is never freed on a guess.
 func questionOpen(store QuestionStore, bus AskBus, id string, now time.Time) (bool, error) {
 	q, ok, err := readQuestionErr(store, id)
 	if err != nil {
@@ -396,7 +397,7 @@ func questionOpen(store QuestionStore, bus AskBus, id string, now time.Time) (bo
 		return true, err
 	}
 	if entry == nil {
-		return now.Sub(q.At) < RouterHoldsWithin, nil
+		return true, nil
 	}
 	var rec routerRecord
 	if json.Unmarshal(entry.Value, &rec) != nil {
@@ -559,9 +560,15 @@ func ReadAnswer(store QuestionStore, bus AskBus, key string, now time.Time) (map
 	switch {
 	case entry == nil && q.Ended != "":
 		out["state"], out["words"] = AnswerEnded, q.Ended
-	case entry == nil && now.Sub(q.At) >= RouterHoldsWithin:
-		out["state"], out["words"] = AnswerEnded, "the router holds no record of it: refused (its refusal was not heard "+
-			"here) or never taken; nothing waits for the operator's answer"
+	case entry == nil && !now.Before(q.Expires):
+		out["state"], out["words"] = AnswerEnded, "expired without the router recording it: refused (its refusal was not "+
+			"heard here) or never taken; the router refuses it now if it is taken late"
+		out["now"] = "do what you said: " + q.OnExpiry
+	case entry == nil:
+		out["state"] = AnswerWaiting
+		out["words"] = "not yet recorded by the router: it may still be in the router's queue, or was refused without the " +
+			"refusal being heard here. It holds its slot, and the operator may still be asked, until it expires at " +
+			q.Expires.Format(time.RFC3339)
 	case entry == nil, rec.State == "open", rec.Warrant == nil:
 		out["state"] = AnswerWaiting
 		if q.Ended != "" {

modules/claude-code/cmd/claude-code/questions_test.go

@@ -314,6 +314,8 @@ type memQuestions struct {
 	n   uint64
 	// failGet fails every read of a key with this prefix.
 	failGet string
+	// afterGet runs after a read, outside the lock: another node writing between a read and a write.
+	afterGet func(key string)
 }
 
 func newMemQuestions() *memQuestions {
@@ -322,12 +324,17 @@ func newMemQuestions() *memQuestions {
 
 func (s *memQuestions) Get(key string) (json.RawMessage, uint64, bool, error) {
 	s.mu.Lock()
-	defer s.mu.Unlock()
 	if s.failGet != "" && strings.HasPrefix(key, s.failGet) {
+		s.mu.Unlock()
 		return nil, 0, false, errors.New("the bus did not answer")
 	}
 	raw, ok := s.m[key]
-	return raw, s.rev[key], ok, nil
+	rev, after := s.rev[key], s.afterGet
+	s.mu.Unlock()
+	if after != nil {
+		after(key)
+	}
+	return raw, rev, ok, nil
 }
 func (s *memQuestions) put(key string, value any) error {
 	raw, err := json.Marshal(value)
@@ -431,22 +438,60 @@ func TestTheCapFailsClosed(t *testing.T) {
 	}
 }
 
-// A question whose refusal was not heard here, and of which the router holds no record, frees its slot once the
-// router has had time to record it, and reads as ended — not open until it expires.
-func TestAQuestionTheRouterNeverHeldFreesItsSlot(t *testing.T) {
+// A question whose refusal was not heard here, and of which the router holds no record, keeps its slot and reads as
+// waiting until it expires — the router may still take it from its queue — and only then frees its slot and ends.
+func TestAQuestionTheRouterHasNotRecordedHoldsItsSlotUntilItExpires(t *testing.T) {
 	p, _ := node(t, "g14")
 	store, r := newMemQuestions(), newRouter()
 	first := askMergeTonight(t, p, store, r, t0)
 	askMergeTonight(t, p, store, r, t0)
-	delete(r.records, first) // refused, and the refusal never heard here
-	if _, err := AskQuestion(p, mergeTonight(), store, r, t0.Add(time.Minute)); err == nil {
-		t.Fatal("a slot was freed before the router had time to record the question")
+	delete(r.records, first) // the router down or backlogged, or its refusal never heard here
+	later := t0.Add(QuestionDefault - time.Minute)
+	if _, err := AskQuestion(p, mergeTonight(), store, r, later); err == nil {
+		t.Fatal("the slot of a question the router may still show was freed before it expired")
 	}
-	if out, _ := ReadAnswer(store, r, first, t0.Add(RouterHoldsWithin)); out["state"] != AnswerEnded {
-		t.Errorf("read as %v", out)
+	out, _ := ReadAnswer(store, r, first, later)
+	if out["state"] != AnswerWaiting || !strings.Contains(out["words"].(string), "not yet recorded") {
+		t.Errorf("before expiry read as %v", out)
 	}
-	if _, err := AskQuestion(p, mergeTonight(), store, r, t0.Add(RouterHoldsWithin)); err != nil {
-		t.Errorf("the slot of a question the router never held was not freed: %v", err)
+	expired := t0.Add(QuestionDefault)
+	if out, _ := ReadAnswer(store, r, first, expired); out["state"] != AnswerEnded ||
+		out["now"] != "do what you said: I leave the pull request open" {
+		t.Errorf("at expiry read as %v", out)
+	}
+	if _, err := AskQuestion(p, mergeTonight(), store, r, expired); err != nil {
+		t.Errorf("the slot of an expired question was not freed: %v", err)
+	}
+}
+
+// A slot is taken only at the revision read: when another node takes both slots between this node's read and its
+// write, this node's writes are refused and the question is refused — a plain write would take a slot it lost.
+func TestASlotIsTakenOnlyAtTheRevisionRead(t *testing.T) {
+	p, _ := node(t, "g14")
+	store, r := newMemQuestions(), newRouter()
+	// Two questions that ended: their slots are free to take again.
+	for _, id := range []string{askMergeTonight(t, p, store, r, t0), askMergeTonight(t, p, store, r, t0)} {
+		r.close(asks.Warrant{Ask: id, Outcome: asks.OutcomeExpired, At: t0.Add(time.Minute)})
+	}
+	shanks, _ := node(t, "shanks")
+	other := newRouter()
+	store.afterGet = func(key string) {
+		if !strings.HasPrefix(key, "slot-") {
+			return
+		}
+		// Another node asks between this node's read of the slot and its write: its question waits.
+		q := Question{ID: newQuestionID(), Headline: "Another", Context: "c", Why: "w", OnExpiry: "e", By: "other",
+			From: shanks.Node, At: t0, Expires: t0.Add(time.Hour),
+			Options: []QuestionOption{{ID: "o1", Label: "A", Means: "a", Recommended: true}, {ID: "o2", Label: "B", Means: "b"}}}
+		_ = store.Put(q.ID, q)
+		_ = other.Publish(asks.AskSubject(Module), q.Ask(), q.ID)
+		r.records[q.ID] = other.records[q.ID]
+		_ = store.Put(key, slot{Question: q.ID, At: t0})
+	}
+	_, err := AskQuestion(p, mergeTonight(), store, r, t0.Add(2*time.Minute))
+	store.afterGet = nil
+	if err == nil || !strings.Contains(err.Error(), "questions already wait") {
+		t.Fatalf("a slot lost to another node was taken: %v", err)
 	}
 }