Skip to content

Commit 81d950d

Browse files
committed
feat(stovepipe): reschedule process when concurrency gate is closed
Ack the current delivery and PublishAfter the same ProcessRequest when the latest head cannot claim a build slot, so gate waits do not burn MaxAttempts or block the partition.
1 parent f48bbb2 commit 81d950d

2 files changed

Lines changed: 113 additions & 29 deletions

File tree

stovepipe/controller/process/process.go

Lines changed: 47 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"context"
2323
"errors"
2424
"fmt"
25+
"time"
2526

2627
"github.com/uber-go/tally"
2728
entityqueue "github.com/uber/submitqueue/platform/base/messagequeue"
@@ -153,7 +154,7 @@ func (c *Controller) processAccepted(ctx context.Context, request entity.Request
153154
return fmt.Errorf("ProcessController failed to load queue config for %s: %w", request.Queue, err)
154155
}
155156

156-
return c.admitLatestHead(ctx, request, queueRow, cfg.MaxConcurrent)
157+
return c.admitLatestHead(ctx, request, queueRow, cfg)
157158
}
158159

159160
// coalesce supersedes request when a newer head exists (RFC process step 5), returning
@@ -183,23 +184,16 @@ func (c *Controller) coalesce(ctx context.Context, request entity.Request, lates
183184
// admitLatestHead runs the gate-then-admit workflow for the latest head: claim a build
184185
// slot, mark the request processing, and publish it to build. Every queue-row reload
185186
// re-runs coalesce-then-gate, so a slot is never spent on a now-stale head; a closed gate
186-
// defers (acks) rather than failing.
187-
func (c *Controller) admitLatestHead(ctx context.Context, request entity.Request, queueRow entity.Queue, maxConcurrent int32) error {
187+
// defers by rescheduling the request (ack after re-enqueue) rather than failing.
188+
func (c *Controller) admitLatestHead(ctx context.Context, request entity.Request, queueRow entity.Queue, cfg entity.QueueConfig) error {
188189
var sc sourcecontrol.SourceControl
189190
var strategy entity.BuildStrategy
190191
var baseURI string
191192
var err error
192193

193194
for {
194-
if queueRow.InFlightCount >= maxConcurrent {
195-
// TODO: re-enqueue the request via PublishAfter on the process topic with GateWaitDelayMs.
196-
c.logger.Infow("latest head awaiting build slot",
197-
"request_id", request.ID,
198-
"queue", request.Queue,
199-
"uri", request.URI,
200-
"in_flight_count", queueRow.InFlightCount,
201-
)
202-
return nil
195+
if queueRow.InFlightCount >= cfg.MaxConcurrent {
196+
return c.rescheduleProcess(ctx, request, queueRow.InFlightCount, cfg.GateWaitDelayMs)
203197
}
204198

205199
if queueRow.LastGreenURI != "" && sc == nil {
@@ -418,6 +412,47 @@ func (c *Controller) supersedeRequest(ctx context.Context, request entity.Reques
418412
}
419413
}
420414

415+
// rescheduleProcess re-enqueues the same ProcessRequest after a delay so the gate can be
416+
// re-checked without burning MaxAttempts. delayMs must be positive.
417+
func (c *Controller) rescheduleProcess(ctx context.Context, request entity.Request, inFlightCount int32, delayMs int64) error {
418+
if delayMs <= 0 {
419+
metrics.NamedCounter(c.metricsScope, _opName, "config_errors", 1)
420+
return fmt.Errorf("ProcessController requires a positive gate wait delay for queue %s, got %dms", request.Queue, delayMs)
421+
}
422+
423+
payload, err := stovepipemq.Marshal(&stovepipemq.ProcessRequest{Id: request.ID})
424+
if err != nil {
425+
return fmt.Errorf("ProcessController failed to serialize process request %s: %w", request.ID, err)
426+
}
427+
428+
// Suffix the message id with the publish time so the reschedule can't collide with
429+
// the in-flight delivery's still-present message-store row.
430+
msgID := fmt.Sprintf("%s/reschedule/%d", request.ID, time.Now().UnixMilli())
431+
msg := entityqueue.NewMessage(msgID, payload, request.Queue, nil)
432+
433+
q, ok := c.registry.Queue(c.topicKey)
434+
if !ok {
435+
return fmt.Errorf("no queue registered for topic key %s", c.topicKey)
436+
}
437+
topicName, ok := c.registry.TopicName(c.topicKey)
438+
if !ok {
439+
return fmt.Errorf("no topic name registered for topic key %s", c.topicKey)
440+
}
441+
442+
if err := q.Publisher().PublishAfter(ctx, topicName, msg, delayMs); err != nil {
443+
metrics.NamedCounter(c.metricsScope, _opName, "publish_errors", 1)
444+
return fmt.Errorf("ProcessController failed to reschedule process request %s: %w", request.ID, err)
445+
}
446+
c.logger.Infow("rescheduled latest head awaiting build slot",
447+
"request_id", request.ID,
448+
"queue", request.Queue,
449+
"uri", request.URI,
450+
"in_flight_count", inFlightCount,
451+
"delay_ms", delayMs,
452+
)
453+
return nil
454+
}
455+
421456
// loadRequest returns the request for id. A not-yet-visible row is retryable.
422457
func (c *Controller) loadRequest(ctx context.Context, id string) (entity.Request, error) {
423458
got, err := c.store.GetRequestStore().Get(ctx, id)

stovepipe/controller/process/process_test.go

Lines changed: 66 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,14 @@ const (
4444
testURI = "git://repo/monorepo/main/abc123"
4545
)
4646

47+
// rescheduledMsg matches a gate-wait re-publish: same partition, but a fresh message id —
48+
// re-publishing under the in-flight delivery's id would be silently deduped against its
49+
// still-present message-store row and lost on ack.
50+
func rescheduledMsg(msg entityqueue.Message) bool {
51+
// Fresh non-empty id, same queue.
52+
return msg.ID != testID && msg.ID != "" && msg.PartitionKey == testQueue
53+
}
54+
4755
type processMocks struct {
4856
reqStore *storagemock.MockRequestStore
4957
queueStore *storagemock.MockQueueStore
@@ -74,6 +82,7 @@ func newControllerWithScope(t *testing.T, ctrl *gomock.Controller, scope tally.S
7482
queue := mqmock.NewMockQueue(ctrl)
7583
queue.EXPECT().Publisher().Return(m.publisher).AnyTimes()
7684
registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{
85+
{Key: stovepipemq.TopicKeyProcess, Name: "process", Queue: queue},
7786
{Key: stovepipemq.TopicKeyBuild, Name: "build", Queue: queue},
7887
})
7988
require.NoError(t, err)
@@ -499,7 +508,7 @@ func TestProcess(t *testing.T) {
499508
},
500509
},
501510
{
502-
name: "latest accepted head awaits slot when gate closed",
511+
name: "latest accepted head reschedules when gate closed",
503512
setup: func(m processMocks) {
504513
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
505514
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
@@ -509,6 +518,52 @@ func TestProcess(t *testing.T) {
509518
LastGreenURI: "git://repo/monorepo/main/green",
510519
Version: 1,
511520
}, nil)
521+
m.publisher.EXPECT().
522+
PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)).
523+
Return(nil)
524+
},
525+
},
526+
{
527+
name: "gate reschedule publish error surfaces",
528+
wantErr: true,
529+
wantRetry: false,
530+
setup: func(m processMocks) {
531+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
532+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
533+
Name: testQueue,
534+
LatestRequestID: testID,
535+
InFlightCount: 1,
536+
Version: 1,
537+
}, nil)
538+
m.publisher.EXPECT().
539+
PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)).
540+
Return(errors.New("queue down"))
541+
},
542+
},
543+
{
544+
name: "gate closed after slot claim race reschedules",
545+
setup: func(m processMocks) {
546+
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
547+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
548+
Name: testQueue,
549+
LatestRequestID: testID,
550+
Version: 1,
551+
}, nil)
552+
m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{
553+
Name: testQueue,
554+
LatestRequestID: testID,
555+
InFlightCount: 1,
556+
Version: 1,
557+
}, int32(1), int32(2)).Return(storage.ErrVersionMismatch)
558+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
559+
Name: testQueue,
560+
LatestRequestID: testID,
561+
InFlightCount: 1,
562+
Version: 2,
563+
}, nil)
564+
m.publisher.EXPECT().
565+
PublishAfter(gomock.Any(), "process", gomock.Cond(rescheduledMsg), int64(5000)).
566+
Return(nil)
512567
},
513568
},
514569
{
@@ -537,22 +592,6 @@ func TestProcess(t *testing.T) {
537592
expectBuildPublish(t, m, testID)
538593
},
539594
},
540-
{
541-
name: "gate closed after reload acks without failing",
542-
setup: func(m processMocks) {
543-
m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(acceptedRequest(testID), nil)
544-
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
545-
Name: testQueue, LatestRequestID: testID, Version: 1,
546-
}, nil)
547-
m.queueStore.EXPECT().Update(gomock.Any(), entity.Queue{
548-
Name: testQueue, LatestRequestID: testID, InFlightCount: 1, Version: 1,
549-
}, int32(1), int32(2)).Return(storage.ErrVersionMismatch)
550-
// Reload: another admit took the last slot — gate now closed, defer (ack).
551-
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
552-
Name: testQueue, LatestRequestID: testID, InFlightCount: 1, Version: 2,
553-
}, nil)
554-
},
555-
},
556595
{
557596
name: "reload after claim mismatch supersedes a now-stale head",
558597
setup: func(m processMocks) {
@@ -765,3 +804,13 @@ func TestProcess(t *testing.T) {
765804
})
766805
}
767806
}
807+
808+
func TestRescheduleProcessRequiresPositiveDelay(t *testing.T) {
809+
ctrl := gomock.NewController(t)
810+
c, _ := newController(t, ctrl)
811+
812+
err := c.rescheduleProcess(context.Background(), acceptedRequest(testID), 1, 0)
813+
814+
require.Error(t, err)
815+
assert.False(t, errs.IsRetryable(err))
816+
}

0 commit comments

Comments
 (0)