Skip to content

Commit c863edd

Browse files
authored
feat(platform): hold/postpone — a fourth delivery outcome for backing off (#487)
## Summary ### Why? Queue controllers have no way to say "this message is fine, but it must wait." The outcome model is ack/nack/reject, so waiting stages ack and republish fresh copies of their own messages via PublishAfter, with message-id minting to dodge the publish dedup, retry accounting reset every cycle, and loop liveness hanging on a publish succeeding. Designed in doc/rfc/consumer-hold.md (previous commit). ### What? The messagequeue extension Delivery gains Postpone(delayMs): the delivery finalizes, the message becomes invisible for the delay, and it acts as a partition barrier — the mysql poll loop stops scanning the partition at a postponed row instead of skipping past it (nacked rows keep skip-and-continue, so failures never halt a partition). A new `postponed` flag on queue_delivery_state makes the post-postpone redelivery exempt from the retry_count increment and resets the count, so deliberate waits never burn the DLQ budget while real failures still dead-letter. The consumer framework's Delivery view gains Hold(delayMs): an intent-recording call with no I/O. On a nil return from Process the framework postpones instead of acking (metric op `postpone`); an error return wins over a recorded hold (`hold_ignored` counter). A failed postpone write is abandoned like a failed ack — the visibility timeout lapses into a normal redelivery, so hold-loop liveness is framework-owned. Controller unit tests across submitqueue/stovepipe/runway previously passed the extension mock as consumer.Delivery, which only worked structurally; they now use the consumer-facing mock (which has Hold). ## Test Plan ✅ `make test` (83 targets) — includes new consumer hold outcome tests, mysql MarkPostponed/GetDeliveryState store tests, and poll-loop barrier tests. ✅ `bazel test //test/integration/extension/messagequeue/...` — new end-to-end tests: postpone blocks the partition until due then redelivers in order as attempt 1; a postpone resets the budget but subsequent real failures still dead-letter. ✅ `make fmt`, `make gazelle`, `make mocks`. ## Issues ## Stack 1. #486 1. @ #487 1. #488 1. #489 1. #490 1. #491 1. #492
1 parent 568e9d5 commit c863edd

58 files changed

Lines changed: 834 additions & 94 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

platform/consumer/README.md

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ type Controller interface {
6262

6363
### Delivery
6464

65-
A restricted view of a queue delivery exposed to controllers. Hides Ack/Nack/Reject (handled automatically by Consumer) while exposing message data and `ExtendVisibilityTimeout`.
65+
A restricted view of a queue delivery exposed to controllers. Hides Ack/Nack/Reject (handled automatically by Consumer) while exposing message data, `ExtendVisibilityTimeout`, and `Hold`.
6666

6767
## TopicRegistry
6868

@@ -92,6 +92,7 @@ registry, _ := consumer.NewTopicRegistry([]consumer.TopicConfig{
9292
The consumer passes every non-nil controller error through the configured `errs.ErrorProcessor` once and then uses `errs.IsRetryable` to decide the transport action:
9393

9494
- **`return nil`** — success, message is acked.
95+
- **`delivery.Hold(delayMs)` then `return nil`** — success that chose to wait: the message is postponed instead of acked. It redelivers after the delay as a barrier its partition waits behind, and the redelivery does not count toward the retry limit (`Attempt()` restarts at 1). A hold is only honored on success — if `Process` returns an error, the failure outcome below wins and the recorded hold is discarded (logged, `hold_ignored` counter). Use hold for backoff loops (waiting for a budget slot, polling an external status) instead of acking and republishing to your own topic.
9596
- **non-nil, retryable after processing** — message is nacked for redelivery (visibility timeout drives the retry delay).
9697
- **non-nil, non-retryable after processing** — message is rejected, which moves it to the DLQ if one is configured for the subscription, or simply acks-and-drops if not.
9798

@@ -120,7 +121,21 @@ When the consumer is wired with `errs.AlwaysRetryableProcessor` (DLQ reconciliat
120121

121122
The consumer records controller operations with `process.start` and `process.finish`. The finish histogram records both latency and completion count with `result=success|error|cancel`; error and cancellation series also include `origin=infra|infra_retryable|user` and `dependency=yes|no`. These dimensions are added after error processing, so they describe the classified error that drives ack, nack, or reject behavior rather than the controller's raw return value. The lifecycle histogram count replaces separate received, processed, and controller-error counters.
122123

123-
The consumer also owns lifecycle metrics for the resulting `ack`, `nack`, or `reject` transport operation. Queue controllers should emit only domain-specific event counters; they must not duplicate the consumer-owned `process` lifecycle metrics.
124+
The consumer also owns lifecycle metrics for the resulting `ack`, `nack`, `postpone`, or `reject` transport operation. Queue controllers should emit only domain-specific event counters; they must not duplicate the consumer-owned `process` lifecycle metrics.
125+
126+
## Which wait do I want?
127+
128+
Several mechanisms can delay work; they mean different things. Pick by what you're trying to say:
129+
130+
| You want to say | Use | Partition while waiting |
131+
|---|---|---|
132+
| "This delivery failed — retry it" | return a retryable error (framework nacks) | keeps flowing — a failure never halts its partition |
133+
| "I'm still working — keep my lease" | `delivery.ExtendVisibilityTimeout(...)` | blocked behind the in-flight delivery |
134+
| "Done for now — wake this partition in N ms" | `delivery.Hold(N)` then `return nil` | paused behind the postponed message (barrier), redelivers first in order |
135+
| "Stop this controller/partition from outside" (tests, operators) | consumer gate (`platform/extension/consumergate`) | parked in flight until the gate opens |
136+
| "Defer *other* work" — a delayed message to another topic or key | `Publisher.PublishAfter` | not involved — it's a fresh publish |
137+
138+
Gate vs hold, since both pause a partition: the **gate** is an external, event-ended stop — someone stops the controller at the door, before `Process` ever sees the message. **Hold** is a controller-chosen, timer-ended wait — the controller saw the work and decided to come back later. Business logic never closes or opens gates; a controller that needs to back off uses hold.
124139

125140
## Lifecycle
126141

platform/consumer/consumer.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,18 @@ func (m *consumer) processDelivery(ctx context.Context, controller Controller, d
411411
op.Complete(err, completionTags...)
412412

413413
if err != nil {
414+
// A failure outcome wins over a recorded hold — a hold is only honored
415+
// on success, so retry accounting and dead-lettering stay meaningful.
416+
if wrapped.held {
417+
metrics.NamedCounter(controllerScope, opName, "hold_ignored", 1)
418+
m.logger.Warnw("hold recorded but controller returned error, failure outcome wins",
419+
"controller", controller.Name(),
420+
"topic_key", topicKey,
421+
"message_id", msg.ID,
422+
"partition_key", msg.PartitionKey,
423+
)
424+
}
425+
414426
// By convention, Controller can only return context.Canceled if it is
415427
// cancelled by the processing context during shutdown.
416428
isCanceled := errors.Is(err, context.Canceled)
@@ -474,6 +486,36 @@ func (m *consumer) processDelivery(ctx context.Context, controller Controller, d
474486
return
475487
}
476488

489+
// Controller succeeded with a recorded hold - postpone instead of acking.
490+
// The message redelivers after the delay as a partition barrier, without
491+
// consuming retry budget. A failed postpone is abandoned like a failed ack:
492+
// the visibility timeout lapses into a normal redelivery, so the hold
493+
// loop's liveness never depends on this write succeeding.
494+
if wrapped.held {
495+
postponeOp := metrics.Begin(controllerScope, "postpone", metrics.StorageLatencyBuckets)
496+
postponeErr := delivery.Postpone(ctx, wrapped.holdDelayMs)
497+
postponeOp.Complete(postponeErr)
498+
if postponeErr != nil {
499+
m.logger.Errorw("failed to postpone held message",
500+
"controller", controller.Name(),
501+
"topic_key", topicKey,
502+
"message_id", msg.ID,
503+
"error", postponeErr,
504+
)
505+
return
506+
}
507+
508+
m.logger.Debugw("message held, postponed for redelivery",
509+
"controller", controller.Name(),
510+
"topic_key", topicKey,
511+
"message_id", msg.ID,
512+
"partition_key", msg.PartitionKey,
513+
"delay_ms", wrapped.holdDelayMs,
514+
"elapsed_ms", elapsed.Milliseconds(),
515+
)
516+
return
517+
}
518+
477519
// Controller succeeded - ack message
478520
ackOp := metrics.Begin(controllerScope, "ack", metrics.StorageLatencyBuckets)
479521
ackErr := delivery.Ack(ctx)

platform/consumer/consumer_test.go

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,125 @@ func TestConsumer_ProcessDelivery_Error(t *testing.T) {
357357
require.NoError(t, err)
358358
}
359359

360+
func TestConsumer_ProcessDelivery_Hold(t *testing.T) {
361+
tests := []struct {
362+
name string
363+
processFunc func(ctx context.Context, delivery Delivery) error
364+
postponeErr error
365+
// wantOutcome is the delivery method the framework must call: "postpone" or "nack".
366+
wantOutcome string
367+
wantDelayMs int64
368+
}{
369+
{
370+
name: "hold postpones instead of acking",
371+
processFunc: func(ctx context.Context, delivery Delivery) error {
372+
delivery.Hold(5000)
373+
return nil
374+
},
375+
wantOutcome: "postpone",
376+
wantDelayMs: 5000,
377+
},
378+
{
379+
name: "last hold wins",
380+
processFunc: func(ctx context.Context, delivery Delivery) error {
381+
delivery.Hold(1000)
382+
delivery.Hold(2500)
383+
return nil
384+
},
385+
wantOutcome: "postpone",
386+
wantDelayMs: 2500,
387+
},
388+
{
389+
name: "negative delay clamps to zero",
390+
processFunc: func(ctx context.Context, delivery Delivery) error {
391+
delivery.Hold(-5)
392+
return nil
393+
},
394+
wantOutcome: "postpone",
395+
wantDelayMs: 0,
396+
},
397+
{
398+
name: "error outcome wins over hold",
399+
processFunc: func(ctx context.Context, delivery Delivery) error {
400+
delivery.Hold(5000)
401+
return errs.NewRetryableError(fmt.Errorf("processing failed"))
402+
},
403+
wantOutcome: "nack",
404+
},
405+
{
406+
name: "postpone failure leaves delivery in flight",
407+
processFunc: func(ctx context.Context, delivery Delivery) error {
408+
delivery.Hold(3000)
409+
return nil
410+
},
411+
postponeErr: fmt.Errorf("db error"),
412+
wantOutcome: "postpone",
413+
wantDelayMs: 3000,
414+
},
415+
}
416+
417+
for _, tt := range tests {
418+
t.Run(tt.name, func(t *testing.T) {
419+
ctrl := gomock.NewController(t)
420+
logger := zaptest.NewLogger(t).Sugar()
421+
422+
deliveryChan := make(chan extqueue.Delivery, 1)
423+
mockSub := queuemock.NewMockSubscriber(ctrl)
424+
mockSub.EXPECT().Subscribe(gomock.Any(), gomock.Any(), gomock.Any()).Return(deliveryChan, nil)
425+
426+
mockQ := queuemock.NewMockQueue(ctrl)
427+
mockQ.EXPECT().Subscriber().Return(mockSub)
428+
429+
reg := newRegistry(t, mockQ, testTopicKeyStart, "test-group")
430+
431+
c := New(logger, tally.NoopScope, reg, errs.NewClassifierProcessor(), consumergatenoop.New())
432+
433+
handler := &testController{}
434+
setupController(handler, "test-handler", testTopicKeyStart, "test-group", tt.processFunc)
435+
436+
require.NoError(t, c.Register(handler))
437+
438+
ctx, cancel := context.WithCancel(context.Background())
439+
defer cancel()
440+
441+
require.NoError(t, c.Start(ctx))
442+
443+
msg := entityqueue.NewMessage("held-msg", []byte("payload"), "partition1", nil)
444+
done := make(chan struct{})
445+
var gotDelayMs int64
446+
mockDel := queuemock.NewMockDelivery(ctrl)
447+
mockDel.EXPECT().Message().Return(msg).AnyTimes()
448+
mockDel.EXPECT().Attempt().Return(1).AnyTimes()
449+
mockDel.EXPECT().ReceivedAt().Return(time.Now().UnixMilli()).AnyTimes()
450+
mockDel.EXPECT().Metadata().Return(nil).AnyTimes()
451+
mockDel.EXPECT().DeliveryID().Return(msg.ID).AnyTimes()
452+
// No Ack expectation: an Ack call on a held delivery fails the test.
453+
switch tt.wantOutcome {
454+
case "postpone":
455+
mockDel.EXPECT().Postpone(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, delayMs int64) error {
456+
gotDelayMs = delayMs
457+
close(done)
458+
return tt.postponeErr
459+
})
460+
case "nack":
461+
mockDel.EXPECT().Nack(gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, requeueAfterMillis int64) error {
462+
close(done)
463+
return nil
464+
})
465+
}
466+
467+
deliveryChan <- mockDel
468+
<-done
469+
470+
if tt.wantOutcome == "postpone" {
471+
assert.Equal(t, tt.wantDelayMs, gotDelayMs)
472+
}
473+
474+
require.NoError(t, c.Stop(30000))
475+
})
476+
}
477+
}
478+
360479
func TestConsumer_ProcessDelivery_NonRetryableError(t *testing.T) {
361480
ctrl := gomock.NewController(t)
362481
logger := zaptest.NewLogger(t).Sugar()

platform/consumer/controller.go

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,14 @@ import (
2626
// Delivery is the consumer package's view of a queue delivery.
2727
// It exists to hide Ack/Nack from controllers — the Consumer framework handles those
2828
// automatically based on the error returned from Process(). Controllers only see
29-
// message data, metadata, and ExtendVisibilityTimeout (a business-level concern for
30-
// long-running processing).
29+
// message data, metadata, ExtendVisibilityTimeout (a business-level concern for
30+
// long-running processing), and Hold (a business-level concern for backing off).
3131
//
3232
// To signal outcome from Process():
3333
// - Return nil to ack the message (success).
3434
// - Return an error to nack the message for retry.
3535
// - Return a non-retryable error to reject a poison pill message (removes it from the queue).
36+
// - Call Hold(delayMs) and return nil to postpone the message (redeliver later, partition waits).
3637
type Delivery interface {
3738
// Message returns the delivered message.
3839
Message() entityqueue.Message
@@ -41,11 +42,21 @@ type Delivery interface {
4142
// visible to other consumers. Use when processing takes longer than expected.
4243
ExtendVisibilityTimeout(ctx context.Context, durationMillis int64) error
4344

45+
// Hold records intent to postpone this delivery: when Process then returns
46+
// nil, the framework postpones the message for delayMs instead of acking.
47+
// The postponed message is a barrier — its partition is not consumed past
48+
// it until it redelivers, in order — and the redelivery does not count
49+
// toward the retry limit. Recording has no side effects; the last call
50+
// wins; a negative delay is clamped to 0. If Process returns an error, the
51+
// failure outcome wins and the recorded hold is discarded. Must be called
52+
// from the Process goroutine before returning.
53+
Hold(delayMs int64)
54+
4455
// DeliveryID returns a backend-specific identifier for this delivery.
4556
DeliveryID() string
4657

4758
// Attempt returns how many times this message has been delivered.
48-
// Starts at 1 for first delivery.
59+
// Starts at 1 for first delivery. A postponed redelivery restarts at 1.
4960
Attempt() int
5061

5162
// ReceivedAt returns when this delivery was received (Unix milliseconds).
@@ -59,6 +70,11 @@ type Delivery interface {
5970
// Hides Ack/Nack from controllers - Consumer handles those automatically.
6071
type deliveryWrapper struct {
6172
delivery extqueue.Delivery
73+
74+
// held and holdDelayMs record Hold intent. Written from the Process
75+
// goroutine, read by the framework after Process returns.
76+
held bool
77+
holdDelayMs int64
6278
}
6379

6480
func (d *deliveryWrapper) Message() entityqueue.Message {
@@ -69,6 +85,14 @@ func (d *deliveryWrapper) ExtendVisibilityTimeout(ctx context.Context, durationM
6985
return d.delivery.ExtendVisibilityTimeout(ctx, durationMillis)
7086
}
7187

88+
func (d *deliveryWrapper) Hold(delayMs int64) {
89+
if delayMs < 0 {
90+
delayMs = 0
91+
}
92+
d.held = true
93+
d.holdDelayMs = delayMs
94+
}
95+
7296
func (d *deliveryWrapper) DeliveryID() string {
7397
return d.delivery.DeliveryID()
7498
}
@@ -96,6 +120,7 @@ type Controller interface {
96120
// Process processes a delivery. Controller receives consumer.Delivery (not extension/entityqueue.Delivery)
97121
// which prevents direct Ack/Nack calls - Consumer handles those automatically.
98122
// Return nil to ack the message (success), error to nack and retry, or NonRetryableError to ack a poison pill message.
123+
// Call delivery.Hold(delayMs) and return nil to postpone the message instead of acking it.
99124
// Context controls the lifecycle of the service. It is cancelled when the consumer is stopped. The implementation should process it gracefully:
100125
// - Pass the context to the underlying services and wait for them to complete their operations.
101126
// - Proceed to the nearest safe state.

platform/consumer/mock/controller_mock.go

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

platform/extension/messagequeue/README.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ type Publisher interface {
2525
- `Nack` is "this delivery failed, try again" — it bumps `retry_count` and eventually trips DLQ.
2626
- `PublishAfter` is "postpone this work" — `retry_count` resets to 0, DLQ stays available for true failures.
2727

28-
Use `PublishAfter` for self-driven poll loops (e.g. the orchestrator's `buildsignal` consumer re-publishing itself between `Status` calls). Use `Nack` for processing failures.
28+
For a consumer deferring its *own current delivery* ("check back in N ms"), prefer `Delivery.Postpone` (below) over ack-plus-`PublishAfter`: it needs no publisher, no fresh message id, and keeps the same log row. `PublishAfter` remains the tool for deferring *other* work — publishing a delayed message to a different topic or key. Use `Nack` for processing failures.
2929

3030
### Subscriber
3131
Consumes messages from topics with per-subscription configuration.
@@ -45,6 +45,7 @@ type Delivery interface {
4545
Message() entityqueue.Message
4646
Ack(ctx context.Context) error
4747
Nack(ctx context.Context, requeueAfterMillis int64) error
48+
Postpone(ctx context.Context, delayMs int64) error
4849
Reject(ctx context.Context, reason string) error
4950
ExtendVisibilityTimeout(ctx context.Context, durationMillis int64) error
5051
DeliveryID() string
@@ -56,9 +57,12 @@ type Delivery interface {
5657

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

64+
**`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.
65+
6266
### SubscriptionConfig
6367

6468
Per-subscription configuration for polling, batching, leasing, retries, and DLQ:

platform/extension/messagequeue/delivery.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,14 @@ type Delivery interface {
4040
// If requeueAfterMillis is 0, the message is requeued immediately.
4141
Nack(ctx context.Context, requeueAfterMillis int64) error
4242

43+
// Postpone finishes this delivery as "processed successfully, redeliver
44+
// later": the message becomes invisible for delayMs and acts as a barrier —
45+
// its partition is not consumed past it until it redelivers, in order.
46+
// Unlike Nack, the redelivery does not count against the failure budget
47+
// (retry limit / DLQ); postponing resets the failure streak.
48+
// Postpone is terminal for this delivery, like Ack/Nack/Reject.
49+
Postpone(ctx context.Context, delayMs int64) error
50+
4351
// Reject moves the message to the dead letter entityqueue.
4452
// Use for poison pill messages that should never be retried.
4553
// reason is stored as last_error in the DLQ for debugging.

platform/extension/messagequeue/mock/delivery_mock.go

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

0 commit comments

Comments
 (0)