Skip to content

Commit 3f68b02

Browse files
committed
feat(messagequeue)!: drop the requeue delay parameter from Nack
## Summary ### Why? Nack's delay parameter was vestigial. The consumer framework — the only production caller — always passed 0 and let the visibility timeout space retries, and the delay's other conceivable use ("check back later" pacing) is exactly what Postpone now expresses with correct retry accounting. Keeping the parameter left two overlapping delay knobs on the delivery contract and invited nack-as-backoff, which burns the DLQ budget. ### What? Delivery.Nack becomes Nack(ctx): the message is immediately eligible for redelivery, retries are spaced by the visibility timeout, and the redelivery counts toward the failure budget as before. MarkNacked drops its delay accordingly (invisible_until = now). Integration tests: TestNackWithDelay is deleted (the delay was the feature; redelivery-after-lapse is already covered by TestVisibilityTimeoutAndRetry); TestNackDoesNotBlockOtherMessages becomes TestInFlightMessageDoesNotBlockOtherMessages, proving the skip property with an un-finalized in-flight head instead of a 30s nack (arrange-first, mirroring the postpone barrier test it contrasts with); other call sites are mechanical. Docs updated (READMEs, sql-queue RFC nack semantics). ## Test Plan ✅ `make test` (83 targets). ✅ `bazel test //test/integration/extension/messagequeue/...` — DLQ at MaxAttempts via immediate nacks, non-blocking in-flight head, multi-consumer-group independence. ✅ `make fmt`, `make mocks`.
1 parent 797ee0b commit 3f68b02

15 files changed

Lines changed: 64 additions & 98 deletions

File tree

doc/rfc/sql-queue-rfc.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ DLQ messages are stored in the same `queue_messages` table under a different top
245245

246246
**6. Ack** — Set `acked = TRUE` in delivery state. Watermark advancement is deferred to the poll loop for reduced per-ack latency. All operations are idempotent.
247247

248-
**7. Nack** — Set `invisible_until = now + delay` for retry after backoff
248+
**7. Nack** — Set `invisible_until = now`; the message is immediately eligible for redelivery (the visibility timeout is what spaces retries)
249249

250250
**8. DLQ** — If `retry_count >= MaxAttempts`: atomically move message to DLQ topic (INSERT with DLQ topic + DELETE from original topic in transaction). MoveToDLQ must succeed before marking acked — otherwise the message would be lost from both main queue and DLQ.
251251

@@ -271,13 +271,13 @@ By default, the poll loop fetches a batch of messages (`BatchSize`, default 10)
271271

272272
### Non-Blocking Nack
273273

274-
When a message is nacked, its `invisible_until` is set to a future timestamp. On the next poll, the nacked message is skipped (not deliverable) while subsequent messages are still delivered normally. A nacked message does not block, starve, or delay any other message in the partition.
274+
A nacked message becomes immediately eligible for redelivery; while it is invisible (in flight, or waiting out a visibility lapse), subsequent messages are still delivered normally. A nacked message does not block, starve, or delay any other message in the partition. (A *postponed* message is deliberately the opposite: it acts as a barrier its partition waits behind — see the messagequeue README.)
275275

276276
Example with 5 messages at offsets 1-5, all delivered:
277-
- Message 3 is nacked with 30s delay
277+
- Message 3 is nacked
278278
- Messages 1, 2, 4, 5 can be acked independently
279279
- Watermark advances to 2 (contiguous from head), stops at 3 (not acked)
280-
- After 30s, message 3 becomes deliverable again, is redelivered
280+
- Message 3 is redelivered on a subsequent poll
281281
- Once message 3 is acked, watermark jumps from 2 to 5
282282

283283
### Strict Serialization (Opt-In)

platform/consumer/consumer.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -471,9 +471,9 @@ func (m *consumer) processDelivery(ctx context.Context, controller Controller, d
471471
"elapsed_ms", elapsed.Milliseconds(),
472472
)
473473

474-
// Nack with no delay - let visibility timeout handle retry delay
474+
// Nack requeues immediately - the visibility timeout spaces retries
475475
nackOp := metrics.Begin(controllerScope, "nack", metrics.StorageLatencyBuckets)
476-
nackErr := delivery.Nack(ctx, 0)
476+
nackErr := delivery.Nack(ctx)
477477
nackOp.Complete(nackErr)
478478
if nackErr != nil {
479479
m.logger.Errorw("failed to nack message",

platform/consumer/consumer_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func setupDelivery(del *queuemock.MockDelivery, msg entityqueue.Message, ackErr,
115115
close(done)
116116
return ackErr
117117
}).MaxTimes(1)
118-
del.EXPECT().Nack(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, requeueAfterMillis int64) error {
118+
del.EXPECT().Nack(gomock.Any()).DoAndReturn(func(ctx context.Context) error {
119119
close(done)
120120
return nackErr
121121
}).MaxTimes(1)
@@ -458,7 +458,7 @@ func TestConsumer_ProcessDelivery_Hold(t *testing.T) {
458458
return tt.postponeErr
459459
})
460460
case "nack":
461-
mockDel.EXPECT().Nack(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, requeueAfterMillis int64) error {
461+
mockDel.EXPECT().Nack(gomock.Any()).DoAndReturn(func(ctx context.Context) error {
462462
close(done)
463463
return nil
464464
})
@@ -922,7 +922,7 @@ func TestConsumer_PerPartitionProcessing(t *testing.T) {
922922
mockDelA.EXPECT().Metadata().Return(nil).AnyTimes()
923923
mockDelA.EXPECT().DeliveryID().Return(msgA.ID).AnyTimes()
924924
mockDelA.EXPECT().Ack(gomock.Any()).Return(nil).MaxTimes(1)
925-
mockDelA.EXPECT().Nack(gomock.Any(), gomock.Any()).Return(nil).MaxTimes(1)
925+
mockDelA.EXPECT().Nack(gomock.Any()).Return(nil).MaxTimes(1)
926926

927927
deliveryChan <- mockDelA
928928

platform/extension/messagequeue/README.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ Message with acknowledgment operations.
3636
type Delivery interface {
3737
Message() entityqueue.Message
3838
Ack(ctx context.Context) error
39-
Nack(ctx context.Context, requeueAfterMillis int64) error
39+
Nack(ctx context.Context) error
4040
Postpone(ctx context.Context, delayMs int64) error
4141
Reject(ctx context.Context, reason string) error
4242
ExtendVisibilityTimeout(ctx context.Context, durationMillis int64) error
@@ -48,12 +48,12 @@ type Delivery interface {
4848
```
4949

5050
- **Ack** — message processed successfully, remove from queue
51-
- **Nack** — processing failed, requeue for retry after delay
51+
- **Nack** — processing failed, requeue for immediate retry
5252
- **Postpone** — processed successfully but must wait: redeliver after delay, without consuming retry budget; the message is a barrier its partition waits behind
5353
- **Reject** — poison pill, move to DLQ (or ack if DLQ disabled)
5454
- **ExtendVisibilityTimeout** — extend processing window for long-running work
5555

56-
**`Postpone` vs `Nack` vs `ExtendVisibilityTimeout`:** all three can produce "next delivery happens at T+delay", but they mean different things. `Nack` is a failure — it counts toward `Retry.MaxAttempts` and eventually trips the DLQ, and later offsets in the partition keep flowing past the nacked message (a failed message must not halt its partition). `Postpone` is a deliberate wait — it resets the failure streak (the redelivery restarts at attempt 1) and blocks the partition behind it until it redelivers, in order. `ExtendVisibilityTimeout` is neither: the delivery is still being processed and stays in flight.
56+
**`Postpone` vs `Nack` vs `ExtendVisibilityTimeout`:** `Nack` is a failure — the message is immediately eligible again, the redelivery counts toward `Retry.MaxAttempts` and eventually trips the DLQ, and later offsets in the partition keep flowing past the nacked message (a failed message must not halt its partition). `Postpone` is a deliberate wait — the redelivery happens after the chosen delay, resets the failure streak (it restarts at attempt 1), and blocks the partition behind it until it redelivers, in order. `ExtendVisibilityTimeout` is neither: the delivery is still being processed and stays in flight.
5757

5858
### SubscriptionConfig
5959

@@ -89,7 +89,7 @@ cfg := extqueue.DefaultSubscriptionConfig("worker-1", "consumer-group")
8989
deliveries, _ := sub.Subscribe(ctx, "topic", cfg)
9090
for delivery := range deliveries {
9191
if err := process(delivery.Message().Payload); err != nil {
92-
delivery.Nack(ctx, 0) // Retry
92+
delivery.Nack(ctx) // Retry
9393
continue
9494
}
9595
delivery.Ack(ctx)

platform/extension/messagequeue/delivery.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,10 @@ type Delivery interface {
3636
Ack(ctx context.Context) error
3737

3838
// Nack negatively acknowledges the message, indicating processing failure.
39-
// The message will be requeued for redelivery after requeueAfterMillis.
40-
// If requeueAfterMillis is 0, the message is requeued immediately.
41-
Nack(ctx context.Context, requeueAfterMillis int64) error
39+
// The message is requeued for redelivery immediately; the visibility
40+
// timeout is what spaces retries (a crash or missed ack redelivers on the
41+
// same schedule). The redelivery counts toward the failure budget.
42+
Nack(ctx context.Context) error
4243

4344
// Postpone finishes this delivery as "processed successfully, redeliver
4445
// later": the message becomes invisible for delayMs and acts as a barrier —

platform/extension/messagequeue/mock/delivery_mock.go

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

platform/extension/messagequeue/mysql/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ subConfig := extqueue.DefaultSubscriptionConfig("worker-1", "orchestrator")
3333
deliveryCh, _ := q.Subscriber().Subscribe(ctx, "merge_events", subConfig)
3434
for delivery := range deliveryCh {
3535
if err := process(delivery.Message()); err != nil {
36-
delivery.Nack(ctx, 0) // Retry
36+
delivery.Nack(ctx) // Retry
3737
continue
3838
}
3939
delivery.Ack(ctx)
@@ -194,4 +194,4 @@ Requires Docker running:
194194
bazel test //test/integration/extension/messagequeue/... --test_output=streamed
195195
```
196196

197-
Integration tests cover: publish/subscribe, partition isolation, ordering, visibility timeout, nack with delay, idempotent publish, concurrent publishers, crash recovery, multiple consumer groups, rebalancing, DLQ, graceful shutdown, non-blocking nack, strict serialization (`BatchSize=1`), and independent consumer group state.
197+
Integration tests cover: publish/subscribe, partition isolation, ordering, visibility timeout, idempotent publish, concurrent publishers, crash recovery, multiple consumer groups, rebalancing, DLQ, graceful shutdown, non-blocking in-flight messages, the postpone barrier, strict serialization (`BatchSize=1`), and independent consumer group state.

platform/extension/messagequeue/mysql/delivery_state_store.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,17 +149,17 @@ func (s *sqldeliveryStateStore) MarkAcked(ctx context.Context, consumerGroup, to
149149
return nil
150150
}
151151

152-
// MarkNacked sets invisible_until = now + delay to schedule redelivery.
152+
// MarkNacked sets invisible_until = now, making the message immediately
153+
// eligible for redelivery on the next poll.
153154
// retry_count is NOT incremented here — it is incremented by MarkDelivered on redelivery.
154-
func (s *sqldeliveryStateStore) MarkNacked(ctx context.Context, consumerGroup, topic, partitionKey string, offset int64, delayMs int64) (retErr error) {
155+
func (s *sqldeliveryStateStore) MarkNacked(ctx context.Context, consumerGroup, topic, partitionKey string, offset int64) (retErr error) {
155156
op := metrics.Begin(s.scope, "mark_nacked", metrics.StorageLatencyBuckets,
156157
metrics.NewTag("topic", topic),
157158
metrics.NewTag("consumer_group", consumerGroup),
158159
metrics.NewTag("partition_key", partitionKey))
159160
defer func() { op.Complete(retErr) }()
160161

161-
now := time.Now().UnixMilli()
162-
invisibleUntil := now + delayMs
162+
invisibleUntil := time.Now().UnixMilli()
163163

164164
_, err := s.db.ExecContext(ctx, fmt.Sprintf(`
165165
INSERT INTO %s (consumer_group, topic, partition_key, message_offset, acked, invisible_until, retry_count)

platform/extension/messagequeue/mysql/delivery_state_store_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ func TestDeliveryStateStore_MarkNacked(t *testing.T) {
212212
WillReturnResult(sqlmock.NewResult(1, 1))
213213
}
214214

215-
err := store.MarkNacked(context.Background(), "group-1", "orders", "part-1", 5, 5000)
215+
err := store.MarkNacked(context.Background(), "group-1", "orders", "part-1", 5)
216216

217217
if tt.wantErr {
218218
require.Error(t, err)

platform/extension/messagequeue/mysql/mock_stores.go

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)