Skip to content

Commit 5cea770

Browse files
fix(webhook): reject new syncs during shutdown
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
1 parent 153e5e6 commit 5cea770

4 files changed

Lines changed: 118 additions & 16 deletions

File tree

internal/webhook/handler.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,10 @@ func (h *WebhookHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
161161
http.Error(w, "sync queue full", http.StatusTooManyRequests)
162162
return
163163
}
164+
if err == ErrSyncQueueShuttingDown {
165+
http.Error(w, "sync queue shutting down", http.StatusServiceUnavailable)
166+
return
167+
}
164168
http.Error(w, "sync dispatch failed", http.StatusInternalServerError)
165169
return
166170
}

internal/webhook/handler_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,31 @@ func TestWebhookHandler_ReturnsTooManyRequestsWhenSyncQueueFull(t *testing.T) {
517517
}
518518
}
519519

520+
func TestWebhookHandler_ReturnsServiceUnavailableWhenSyncQueueShuttingDown(t *testing.T) {
521+
secret := []byte("test-secret")
522+
al := NewRepoFilter([]string{"org/*"})
523+
h := NewWebhookHandlerWithConfig(WebhookHandlerConfig{
524+
Secret: secret,
525+
Filter: al,
526+
CloneBaseURLs: []string{"https://github.com"},
527+
OnSync: func(context.Context, string, string, string) error {
528+
return ErrSyncQueueShuttingDown
529+
},
530+
})
531+
532+
payload := makePushEvent("refs/heads/main", "org/svc", "https://github.com/org/svc.git")
533+
req := httptest.NewRequest(http.MethodPost, "/webhook", bytes.NewReader(payload))
534+
req.Header.Set("X-Hub-Signature-256", signPayload(secret, payload))
535+
req.Header.Set("X-GitHub-Event", "push")
536+
537+
rr := httptest.NewRecorder()
538+
h.ServeHTTP(rr, req)
539+
540+
if rr.Code != http.StatusServiceUnavailable {
541+
t.Fatalf("status = %d, want %d", rr.Code, http.StatusServiceUnavailable)
542+
}
543+
}
544+
520545
func TestWebhookHandler_PropagatesTraceparentToSyncContext(t *testing.T) {
521546
secret := []byte("test-secret")
522547
al := NewRepoFilter([]string{"org/*"})

internal/webhook/syncqueue.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ import (
1515
"github.com/tae2089/code-context-graph/internal/obs"
1616
)
1717

18-
var ErrSyncQueueFull = errors.New("sync queue full")
18+
var (
19+
ErrSyncQueueFull = errors.New("sync queue full")
20+
ErrSyncQueueShuttingDown = errors.New("sync queue shutting down")
21+
)
1922

2023
// @intent mark sync failures that should stop retry backoff immediately.
2124
type nonRetryableError struct {
@@ -98,6 +101,7 @@ type SyncQueue struct {
98101
ctx context.Context
99102
handler SyncHandlerFunc
100103
retryConfig RetryConfig
104+
shutdownTimeout time.Duration
101105
maxTrackedRepos int
102106
mu sync.Mutex
103107
queue []string
@@ -164,6 +168,7 @@ func NewSyncQueueWithContext(ctx context.Context, workers int, handler SyncHandl
164168
// @intent configure queue retry policy and memory bounds when constructing a SyncQueue.
165169
type QueueConfig struct {
166170
RetryConfig
171+
ShutdownTimeout time.Duration
167172
MaxTrackedRepos int
168173
}
169174

@@ -179,21 +184,25 @@ func NewSyncQueueWithOptions(ctx context.Context, workers int, handler SyncHandl
179184
// @intent coalesce bursty webhook pushes per repository while still allowing different repos to sync concurrently.
180185
// @param cfg controls retry behavior and how many repositories may be tracked at once.
181186
// @sideEffect starts worker goroutines and allocates queue state maps.
182-
// @ensures non-positive MaxAttempts becomes 1 and non-positive MaxTrackedRepos becomes 1024.
187+
// @ensures non-positive MaxAttempts becomes 1, non-positive ShutdownTimeout becomes 30s, and non-positive MaxTrackedRepos becomes 1024.
183188
func NewSyncQueueWithConfig(ctx context.Context, workers int, handler SyncHandlerFunc, cfg QueueConfig) *SyncQueue {
184189
if ctx == nil {
185190
ctx = context.Background()
186191
}
187192
if cfg.MaxAttempts <= 0 {
188193
cfg.MaxAttempts = 1
189194
}
195+
if cfg.ShutdownTimeout <= 0 {
196+
cfg.ShutdownTimeout = 30 * time.Second
197+
}
190198
if cfg.MaxTrackedRepos <= 0 {
191199
cfg.MaxTrackedRepos = 1024
192200
}
193201
q := &SyncQueue{
194202
ctx: ctx,
195203
handler: handler,
196204
retryConfig: cfg.RetryConfig,
205+
shutdownTimeout: cfg.ShutdownTimeout,
197206
maxTrackedRepos: cfg.MaxTrackedRepos,
198207
dirty: make(map[string]bool),
199208
processing: make(map[string]bool),
@@ -216,7 +225,7 @@ func NewSyncQueueWithConfig(ctx context.Context, workers int, handler SyncHandle
216225
// @intent collapse repeated push events into one queued sync while preserving the newest branch and clone data.
217226
// @mutates SyncQueue.queue, SyncQueue.dirty, SyncQueue.payloads.
218227
// @param repoFullName identifies the deduplication key for queueing.
219-
// @return returns ErrSyncQueueFull when a new repository would exceed the bounded tracked-repo set.
228+
// @return returns ErrSyncQueueFull when a new repository would exceed the bounded tracked-repo set and ErrSyncQueueShuttingDown after graceful shutdown begins.
220229
// @domainRule repeated events for the same repository replace payload data instead of enqueueing duplicate work.
221230
func (q *SyncQueue) Add(ctx context.Context, repoFullName, cloneURL, branch string) error {
222231
if ctx != nil {
@@ -230,7 +239,7 @@ func (q *SyncQueue) Add(ctx context.Context, repoFullName, cloneURL, branch stri
230239
defer q.mu.Unlock()
231240

232241
if q.shutdown {
233-
return nil
242+
return ErrSyncQueueShuttingDown
234243
}
235244

236245
if ctx == nil {
@@ -240,7 +249,7 @@ func (q *SyncQueue) Add(ctx context.Context, repoFullName, cloneURL, branch stri
240249
q.queueFullTotal++
241250
return ErrSyncQueueFull
242251
}
243-
q.payloads[repoFullName] = syncPayload{ctx: ctx, repoFullName: repoFullName, cloneURL: cloneURL, branch: branch}
252+
q.payloads[repoFullName] = syncPayload{ctx: context.WithoutCancel(ctx), repoFullName: repoFullName, cloneURL: cloneURL, branch: branch}
244253

245254
now := time.Now()
246255
if q.dirty[repoFullName] {
@@ -279,9 +288,10 @@ func (q *SyncQueue) Shutdown() {
279288

280289
select {
281290
case <-done:
282-
case <-time.After(30 * time.Second):
283-
slog.Error("sync queue shutdown timed out after 30s, abandoning workers")
284-
q.recordFailure("shutdown", "", errors.New("sync queue shutdown timed out after 30s"))
291+
case <-time.After(q.shutdownTimeout):
292+
err := fmt.Errorf("sync queue shutdown timed out after %s", q.shutdownTimeout)
293+
slog.Error(err.Error())
294+
q.recordFailure("shutdown", "", err)
285295
}
286296
}
287297

internal/webhook/syncqueue_test.go

Lines changed: 71 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -405,28 +405,42 @@ func TestSyncQueue_AddHonorsPerCallContextCancel(t *testing.T) {
405405
}
406406
}
407407

408-
func TestSyncQueue_AddPerCallTimeoutPropagates(t *testing.T) {
408+
func TestSyncQueue_AddDetachesPerCallTimeoutAfterEnqueue(t *testing.T) {
409+
handlerStarted := make(chan struct{}, 1)
409410
handlerDone := make(chan error, 1)
411+
release := make(chan struct{})
410412
handler := func(ctx context.Context, repoFullName, cloneURL, branch string) error {
411-
<-ctx.Done()
413+
handlerStarted <- struct{}{}
414+
<-release
412415
handlerDone <- ctx.Err()
413-
return ctx.Err()
416+
return nil
414417
}
415418

416419
q := NewSyncQueue(1, handler)
417420
defer q.Shutdown()
418421

419422
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
420423
defer cancel()
421-
q.Add(ctx, "org/svc", "url", "main")
424+
if err := q.Add(ctx, "org/svc", "url", "main"); err != nil {
425+
t.Fatalf("Add returned error: %v", err)
426+
}
427+
428+
select {
429+
case <-handlerStarted:
430+
case <-time.After(2 * time.Second):
431+
t.Fatal("timed out waiting for handler start")
432+
}
433+
434+
time.Sleep(100 * time.Millisecond)
435+
close(release)
422436

423437
select {
424438
case err := <-handlerDone:
425-
if !errors.Is(err, context.DeadlineExceeded) {
426-
t.Fatalf("expected deadline exceeded, got %v", err)
439+
if err != nil {
440+
t.Fatalf("expected detached handler context, got %v", err)
427441
}
428442
case <-time.After(2 * time.Second):
429-
t.Fatal("timed out waiting for handler")
443+
t.Fatal("timed out waiting for handler completion")
430444
}
431445
}
432446

@@ -546,6 +560,54 @@ func TestSyncQueue_AddAllowsExistingRepoUpdateWhenAtCapacity(t *testing.T) {
546560
}
547561
}
548562

563+
func TestSyncQueue_AddRejectsAfterShutdownStarts(t *testing.T) {
564+
q := NewSyncQueueWithConfig(context.Background(), 0, func(context.Context, string, string, string) error {
565+
return nil
566+
}, QueueConfig{
567+
RetryConfig: RetryConfig{MaxAttempts: 1, BaseDelay: time.Millisecond, MaxDelay: time.Millisecond},
568+
ShutdownTimeout: time.Second,
569+
MaxTrackedRepos: 1,
570+
})
571+
defer q.Shutdown()
572+
573+
q.Shutdown()
574+
575+
err := q.Add(context.Background(), "org/svc", "url", "main")
576+
if !errors.Is(err, ErrSyncQueueShuttingDown) {
577+
t.Fatalf("Add error = %v, want %v", err, ErrSyncQueueShuttingDown)
578+
}
579+
}
580+
581+
func TestSyncQueue_ShutdownUsesConfiguredTimeout(t *testing.T) {
582+
release := make(chan struct{})
583+
q := NewSyncQueueWithConfig(context.Background(), 1, func(context.Context, string, string, string) error {
584+
<-release
585+
return nil
586+
}, QueueConfig{
587+
RetryConfig: RetryConfig{MaxAttempts: 1, BaseDelay: time.Millisecond, MaxDelay: time.Millisecond},
588+
ShutdownTimeout: 30 * time.Millisecond,
589+
MaxTrackedRepos: 1,
590+
})
591+
592+
if err := q.Add(context.Background(), "org/svc", "url", "main"); err != nil {
593+
t.Fatalf("Add returned error: %v", err)
594+
}
595+
time.Sleep(10 * time.Millisecond)
596+
597+
start := time.Now()
598+
q.Shutdown()
599+
elapsed := time.Since(start)
600+
close(release)
601+
602+
if elapsed > 250*time.Millisecond {
603+
t.Fatalf("Shutdown took %s, want close to configured timeout", elapsed)
604+
}
605+
stats := q.Stats()
606+
if stats.FailureTotal == 0 {
607+
t.Fatalf("expected shutdown timeout failure to be recorded, got %+v", stats)
608+
}
609+
}
610+
549611
func TestSyncQueueStats_ReportsQueueFull(t *testing.T) {
550612
q := NewSyncQueueWithConfig(context.Background(), 0, func(context.Context, string, string, string) error {
551613
return nil
@@ -899,8 +961,9 @@ func TestSyncQueueStats_RecentRepos_IncludesQueuedAndProcessingBeforeFirstOutcom
899961
func TestSyncQueueStats_ReportsAgesAndLastSuccess(t *testing.T) {
900962
release := make(chan struct{})
901963
started := make(chan struct{})
964+
var once sync.Once
902965
q := NewSyncQueueWithConfig(context.Background(), 1, func(context.Context, string, string, string) error {
903-
close(started)
966+
once.Do(func() { close(started) })
904967
<-release
905968
return nil
906969
}, QueueConfig{

0 commit comments

Comments
 (0)