Explain why dlq reconciliation frees slots only for processing requests

Review question on the buildsignal reconciler: failRequest releases the queue
slot only for a request in processing, so does it need to widen that?

It does not. Processing is the only non-terminal state that can own a slot:
process claims the slot and CAS-marks accepted->processing, compensating its own
claim when that CAS does not land, and processing exits only to a terminal
outcome, which releases the slot itself. Releasing for accepted would decrement
for the common request that never claimed one, over-admitting against
MaxConcurrent. Say that where failRequest gates on the state, note at the
buildsignal call site that a build row implies processing-or-terminal, and pin
the intent on the accepted test case, which passes no queue expectations.

Author: mnoah1

stovepipe/controller/dlq/buildsignal.go

@@ -138,6 +138,10 @@ func (c *BuildSignalController) Process(ctx context.Context, delivery consumer.D
 		return nil
 	}
 
+	// Every request reachable from a build row is either still processing, and holding
+	// the slot failRequest releases, or already terminal, and past releasing it: build
+	// triggers only once process has written the strategy, which lands in the same CAS
+	// as accepted→processing, and processing exits only to a terminal outcome.
 	if err := failRequest(ctx, store, c.logger, build.RequestID); err != nil {
 		metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "reconcile_errors", 1)
 		return err

stovepipe/controller/dlq/dlq.go

@@ -62,7 +62,19 @@ func TopicKey(main consumer.TopicKey) consumer.TopicKey {
 // in a terminal state. If the request had reached RequestStateProcessing — meaning process's
 // admit step already CAS-incremented the queue's in_flight_count for it and no terminal
 // outcome has released it yet — the queue's
-// slot is released first. Queue and Request are separate entities with no cross-entity
+// slot is released first.
+//
+// Processing is the only non-terminal state that can own a slot, so the condition is not a
+// narrowing of some broader set: process claims the slot and CAS-marks accepted→processing,
+// releasing its own claim if that CAS never lands, and the exits from processing are the
+// terminal outcomes, which release the slot themselves. Widening the release to accepted
+// would decrement for the far more common request that never claimed a slot, over-admitting
+// against MaxConcurrent. The one case that escapes both this reconciler and process's
+// compensation is a hard crash between the two admit writes, which leaves an accepted
+// request holding a slot that nothing here can tell apart from a request that never
+// claimed one; distinguishing them needs per-request slot ownership on the row.
+//
+// Queue and Request are separate entities with no cross-entity
 // transaction, so the two writes cannot be atomic and the ordering picks which crash
 // failure mode we accept: a crash between the writes leaves the request non-terminal,
 // redelivery re-runs reconciliation, and releaseSlot (which tracks no per-request slot

stovepipe/controller/dlq/dlq_test.go

@@ -101,7 +101,9 @@ func TestProcess(t *testing.T) {
 		wantErr bool
 	}{
 		{
-			name: "accepted request is marked failed",
+			// No queue expectations: an accepted request never claimed a slot,
+			// so releasing one here would over-admit against MaxConcurrent.
+			name: "accepted request is marked failed without releasing a slot",
 			setup: func(m dlqMocks) {
 				m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateAccepted), nil)
 				updated := requestWithState(entity.RequestStateAccepted)