From 436f97f678df6268d3b32c9b4aac575ead049c36 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 5 Jan 2026 14:34:03 +0100 Subject: [PATCH 01/31] add exchange declaration --- internal/queue/consumer.go | 7 +++- internal/queue/consumer_test.go | 3 ++ internal/queue/producer.go | 17 ++++++--- internal/queue/producer_test.go | 66 +++++++++++++++++++-------------- internal/queue/types.go | 55 +++++++++++++++------------ internal/queue/types_test.go | 4 ++ 6 files changed, 94 insertions(+), 58 deletions(-) diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index 5b160e4e..c38e8013 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -106,7 +106,12 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { } if c.client.amqpChannel == nil || c.client.amqpChannel.IsClosed() { - err := c.client.resetChannel(c.queueName, false) + err := c.client.resetChannel() + if err != nil { + return err + } + + err = c.client.declareQueue(c.queueName) if err != nil { return err } diff --git a/internal/queue/consumer_test.go b/internal/queue/consumer_test.go index 627e678f..cd804d9f 100644 --- a/internal/queue/consumer_test.go +++ b/internal/queue/consumer_test.go @@ -14,6 +14,9 @@ import ( "github.com/stretchr/testify/assert" ) +const queueName = "test-queue" + +const queueWithDeclareError = "queue-with-declare-error" const queueWithConsumeError = "queue-with-consume-error" const queueWithQosError = "queue-with-qos-error" diff --git a/internal/queue/producer.go b/internal/queue/producer.go index 078d0058..376ea87d 100644 --- a/internal/queue/producer.go +++ b/internal/queue/producer.go @@ -40,7 +40,12 @@ func (p *AmqpProducer) resetConnectionOrChannelIfNecessary() error { } if p.client.amqpChannel == nil || p.client.amqpChannel.IsClosed() { - err := p.client.resetChannel(p.queueName, false) + err := p.client.resetChannel() + if err != nil { + return err + } + + err = p.client.declareExchange(p.exchangeName) if err != nil { return err } @@ -63,10 +68,10 @@ func waitForMsgConfirmation(ctx context.Context, confirmation confirmation) erro func (p *AmqpProducer) publishMsg(msg []byte, headers map[string]interface{}) (confirmation, error) { confirmation, err := p.client.amqpChannel.PublishWithDeferredConfirm( - "", // exchange - p.queueName, // routing key - false, // mandatory - false, // immediate + "", // exchange + p.exchangeName, // routing key + false, // mandatory + false, // immediate amqp.Publishing{ ContentType: "application/json", Body: msg, @@ -86,7 +91,7 @@ func NewAmqpProducer(uri string, queueName string) *AmqpProducer { uri: uri, connectFunc: amqpConnect, }, - queueName: queueName, + exchangeName: queueName, } } diff --git a/internal/queue/producer_test.go b/internal/queue/producer_test.go index 4339bfb0..55c22390 100644 --- a/internal/queue/producer_test.go +++ b/internal/queue/producer_test.go @@ -14,25 +14,28 @@ import ( "github.com/stretchr/testify/assert" ) -const queueName = "test-queue" -const queueWithPublishNoAck = "queue-with-publish-no-ack" -const queueWithPublishError = "queue-with-publish-error" -const queueWithPublishHangs = "queue-with-publish-hangs" -const queueWithDeclareError = "queue-with-declare-error" +const exchangeName = "test-exchange" +const exchangeWithDeclareError = "exchange-with-declare-error" +const exchangeWithPublishNoAck = "exchange-with-publish-no-ack" +const exchangeWithPublishError = "exchange-with-publish-error" +const exchangeWithPublishHangs = "exchange-with-publish-hangs" + +// TODO : Put this in a helper file because it is also used by consumer tests type MockAmqpChannel struct { msgs [][]byte headers []map[string]interface{} isclosed bool confirmMode bool queueName string + exchangeName string queueDurable bool confirmModeError bool } func (ch *MockAmqpChannel) PublishWithDeferredConfirm(_ string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { - if key == queueWithPublishError { + if key == exchangeWithPublishError { return nil, errors.New("publish error") } ch.msgs = append(ch.msgs, msg.Body) @@ -40,11 +43,11 @@ func (ch *MockAmqpChannel) PublishWithDeferredConfirm(_ string, key string, _, _ done_ch := make(chan struct{}, 1) - if key != queueWithPublishHangs { + if key != exchangeWithPublishHangs { done_ch <- struct{}{} } - ack := key != queueWithPublishNoAck + ack := key != exchangeWithPublishNoAck confirmation := &MockConfirmation{ done: done_ch, ack: ack, @@ -64,6 +67,14 @@ func (ch *MockAmqpChannel) Confirm(_ bool) error { return nil } +func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ bool, _ amqp.Table) error { + if name == exchangeWithDeclareError { + return errors.New("exchange declare error") + } + ch.exchangeName = name + return nil +} + func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ amqp.Table) (amqp.Queue, error) { if name == queueWithDeclareError { return amqp.Queue{}, errors.New("queue declare error") @@ -145,7 +156,7 @@ func TestPush(t *testing.T) { amqpChannel: mockAmqpChannel, }, }, - queueName: queueName, + exchangeName: exchangeName, } headers := map[string]interface{}{"header1": "value1"} @@ -169,7 +180,7 @@ func TestPushFailure(t *testing.T) { }{ { name: "message is not acked", - queueName: queueWithPublishNoAck, + queueName: exchangeWithPublishNoAck, errMsg: "confirmation not acknowledged", context: context.Background(), }, @@ -180,12 +191,12 @@ func TestPushFailure(t *testing.T) { cancel() return ctx }(), - queueName: queueWithPublishHangs, + queueName: exchangeWithPublishHangs, errMsg: "context canceled", }, { name: "publish returns error", - queueName: queueWithPublishError, + queueName: exchangeWithPublishError, errMsg: "publish error", context: context.Background(), }, @@ -201,7 +212,7 @@ func TestPushFailure(t *testing.T) { amqpChannel: mockAmqpChannel, }, }, - queueName: tt.queueName, + exchangeName: tt.queueName, } err := amqpProducer.Push(tt.context, nil, []byte("TestMessage")) @@ -335,11 +346,11 @@ func TestPushNoConnectionFailure(t *testing.T) { assert.ErrorContains(t, err, "connection error") } -func TestPushQueueDeclare(t *testing.T) { +func TestPushExchangeDeclare(t *testing.T) { /* - arrange: create a queue with no amqp channel + arrange: create a producer with no amqp channel act: push a message to the queue - assert: channel with confirm mode is established and queue is declared + assert: channel with confirm mode is established and exchange is declared */ mockAmqpConnection := &MockAmqpConnection{} amqpProducer := &AmqpProducer{ @@ -347,37 +358,36 @@ func TestPushQueueDeclare(t *testing.T) { amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - queueName: queueName, + exchangeName: exchangeName, } amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) assert.Equal(t, mockAmqpConnection.amqpChannel.confirmMode, true, "expected channel to be in confirm mode") - assert.Equal(t, mockAmqpConnection.amqpChannel.queueName, queueName, "expected queue name to be "+queueName) - assert.Equal(t, mockAmqpConnection.amqpChannel.queueDurable, true, "expected queue to be durable") + assert.Equal(t, mockAmqpConnection.amqpChannel.exchangeName, exchangeName, "expected exchange name to be "+exchangeName) } -func TestPushQueueDeclareFailure(t *testing.T) { +func TestPushExchangeDeclareFailure(t *testing.T) { /* - arrange: create a queue with no amqp channel where the queue declare fails + arrange: create a queue with no amqp channel where the exchange declare fails act: push a message to the queue assert: push returns an error */ tests := []struct { name string - queueName string + exchangeName string mockAmqpConnection *MockAmqpConnection errMsg string }{ { - name: "queue declare error", - queueName: queueWithDeclareError, + name: "exchange declare error", + exchangeName: exchangeWithDeclareError, mockAmqpConnection: &MockAmqpConnection{}, - errMsg: "queue declare error", + errMsg: "exchange declare error", }, { - name: "confirm error", - queueName: queueName, + name: "confirm error", + exchangeName: exchangeName, mockAmqpConnection: &MockAmqpConnection{ confirmModeError: true, }, @@ -392,7 +402,7 @@ func TestPushQueueDeclareFailure(t *testing.T) { amqpChannel: nil, amqpConnection: tt.mockAmqpConnection, }, - queueName: tt.queueName, + exchangeName: tt.exchangeName, } err := amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) diff --git a/internal/queue/types.go b/internal/queue/types.go index 63614750..4844be9c 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -34,8 +34,8 @@ func (c *Client) resetConnection() error { return nil } -// resetChannel opens a new AMQP channel and declares the queue. -func (c *Client) resetChannel(queue string, passive bool) error { +// resetChannel opens a new AMQP channel +func (c *Client) resetChannel() error { if c.amqpChannel != nil && !c.amqpChannel.IsClosed() { return nil } @@ -51,29 +51,37 @@ func (c *Client) resetChannel(queue string, passive bool) error { return fmt.Errorf("failed to put channel in confirm mode: %w", err) } - if passive { - _, err = ch.QueueDeclarePassive( - queue, // queueName - true, // durable - false, // delete when unused - false, // exclusive - false, // no-wait - nil, // arguments - ) - } else { - _, err = ch.QueueDeclare( - queue, // queueName - true, // durable - false, // delete when unused - false, // exclusive - false, // no-wait - nil, // arguments - ) + return nil +} + +func (c *Client) declareExchange(exchangeName string) error { + err := c.amqpChannel.ExchangeDeclare( + exchangeName, + "direct", // use direct exchange + true, // durable - don't delete on broker restart + false, // autoDelete - don't delete when no longer in use + false, // internal set to false - be suitable for publishing + false, // wait for confirmation + nil, // no additional arguments + ) + if err != nil { + return fmt.Errorf("failed to declare AMQP exchange: %w", err) } + return nil +} + +func (c *Client) declareQueue(queueName string) error { + _, err := c.amqpChannel.QueueDeclare( + queueName, + true, // durable + false, // delete when unused + false, // exclusive + false, // no-wait + nil, // arguments + ) if err != nil { return fmt.Errorf("failed to declare AMQP queue: %w", err) } - return nil } @@ -100,8 +108,8 @@ func (c *Client) Close() error { } type AmqpProducer struct { - client *Client - queueName string + client *Client + exchangeName string } type Producer interface { @@ -113,6 +121,7 @@ type amqpChannel interface { IsClosed() bool Close() error Confirm(noWait bool) error + ExchangeDeclare(name string, kind string, durable, autoDelete, internal, noWait bool, args amqp.Table) error QueueDeclare(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index c515bf71..2a8dc11f 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -38,6 +38,10 @@ func (f *fakeChannel) Close() error { return f.closeErr } func (f *fakeChannel) Confirm(noWait bool) error { return f.confirmErr } +func (f *fakeChannel) ExchangeDeclare(name string, kind string, durable, autoDelete, internal, noWait bool, args amqp.Table) error { + return nil +} + func (f *fakeChannel) QueueDeclare(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { return amqp.Queue{Name: name}, f.declareErr } From fc43cc68ba552b732f8c4d42372be1c3b2952810 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 5 Jan 2026 14:58:44 +0100 Subject: [PATCH 02/31] add exchange declaration to consumer --- internal/queue/consumer.go | 17 ++++++++++++----- internal/queue/consumer_test.go | 29 ++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index c38e8013..e2cff9be 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -18,15 +18,17 @@ import ( // AmqpConsumer is an AMQP consumer for workflow job events. type AmqpConsumer struct { - client *Client - queueName string - channel <-chan amqp.Delivery + client *Client + exchangeName string + queueName string + channel <-chan amqp.Delivery } // NewAmqpConsumer creates a new AmqpConsumer with the given dependencies. -func NewAmqpConsumer(uri, queueName string) *AmqpConsumer { +func NewAmqpConsumer(uri, exchangeName string, queueName string) *AmqpConsumer { return &AmqpConsumer{ - queueName: queueName, + exchangeName: exchangeName, + queueName: queueName, client: &Client{ uri: uri, connectFunc: amqpConnect, @@ -111,6 +113,11 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } + err = c.client.declareExchange(c.exchangeName) + if err != nil { + return err + } + err = c.client.declareQueue(c.queueName) if err != nil { return err diff --git a/internal/queue/consumer_test.go b/internal/queue/consumer_test.go index cd804d9f..89e91521 100644 --- a/internal/queue/consumer_test.go +++ b/internal/queue/consumer_test.go @@ -343,7 +343,7 @@ func TestPullQueueDeclare(t *testing.T) { /* arrange: create a consumer with no amqp channel act: pull a message from the queue - assert: channel is established and queue is declared + assert: channel is established and exchange and queue is declared */ msgChannel := make(chan amqp.Delivery, 1) msgChannel <- amqp.Delivery{Body: []byte("TestMessage")} @@ -360,38 +360,50 @@ func TestPullQueueDeclare(t *testing.T) { amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - queueName: queueName, + exchangeName: exchangeName, + queueName: queueName, } _, err := amqpConsumer.Pull(context.Background()) assert.NoError(t, err) assert.True(t, mockAmqpChannelConsumer.confirmMode, "expected channel to be in confirm mode") + assert.Equal(t, exchangeName, mockAmqpChannelConsumer.exchangeName, "expected exchange name to be "+exchangeName) assert.Equal(t, queueName, mockAmqpChannelConsumer.queueName, "expected queue name to be "+queueName) assert.True(t, mockAmqpChannelConsumer.queueDurable, "expected queue to be durable") } func TestPullQueueDeclareFailure(t *testing.T) { /* - arrange: create a consumer with no amqp channel where the queue declare fails + arrange: create a consumer with no amqp channel where a declaration fails act: pull a message from the queue assert: pull returns an error */ tests := []struct { name string + exchangeName string queueName string mockAmqpConnection *MockAmqpConnectionConsumer errMsg string }{ + { + name: "exchange declare error", + exchangeName: exchangeWithDeclareError, + queueName: queueName, + mockAmqpConnection: &MockAmqpConnectionConsumer{}, + errMsg: "exchange declare error", + }, { name: "queue declare error", + exchangeName: exchangeName, queueName: queueWithDeclareError, mockAmqpConnection: &MockAmqpConnectionConsumer{}, errMsg: "queue declare error", }, { - name: "confirm error", - queueName: queueName, + name: "confirm error", + exchangeName: exchangeWithDeclareError, + queueName: queueName, mockAmqpConnection: &MockAmqpConnectionConsumer{ confirmModeError: true, }, @@ -406,7 +418,8 @@ func TestPullQueueDeclareFailure(t *testing.T) { amqpChannel: nil, amqpConnection: tt.mockAmqpConnection, }, - queueName: tt.queueName, + exchangeName: tt.exchangeName, + queueName: tt.queueName, } _, err := amqpConsumer.Pull(context.Background()) @@ -477,11 +490,13 @@ func TestNewAmqpConsumer(t *testing.T) { assert: consumer is properly initialized */ uri := "amqp://guest:guest@localhost:5672/" + exchangeName := "test-exchange" queueName := "test-queue" - consumer := NewAmqpConsumer(uri, queueName) + consumer := NewAmqpConsumer(uri, exchangeName, queueName) assert.NotNil(t, consumer) + assert.Equal(t, exchangeName, consumer.exchangeName) assert.Equal(t, queueName, consumer.queueName) assert.NotNil(t, consumer.client) assert.Equal(t, uri, consumer.client.uri) From c658666b00bc9f4ce21e389f6fae0392353eb395 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 10:53:17 +0100 Subject: [PATCH 03/31] add queue bind --- internal/queue/consumer.go | 2 +- internal/queue/types.go | 5 +++++ internal/queue/types_test.go | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index e2cff9be..6b849e28 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -118,7 +118,7 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } - err = c.client.declareQueue(c.queueName) + err = c.client.bindQueue(c.config.QueueName, c.config.RoutingKey, c.config.ExchangeName) if err != nil { return err } diff --git a/internal/queue/types.go b/internal/queue/types.go index 4844be9c..a27643b7 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -124,6 +124,7 @@ type amqpChannel interface { ExchangeDeclare(name string, kind string, durable, autoDelete, internal, noWait bool, args amqp.Table) error QueueDeclare(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) + QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) Qos(prefetchCount, prefetchSize int, global bool) error } @@ -164,6 +165,10 @@ func (ch *amqpChannelWrapper) Qos(prefetchCount, prefetchSize int, global bool) return ch.Channel.Qos(prefetchCount, prefetchSize, global) } +func (ch *amqpChannelWrapper) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { + return ch.Channel.QueueBind(name, key, exchange, noWait, args) +} + type Consumer interface { Pull(ctx context.Context) (amqp.Delivery, error) Close() error diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index 2a8dc11f..73560c0f 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -45,6 +45,9 @@ func (f *fakeChannel) ExchangeDeclare(name string, kind string, durable, autoDel func (f *fakeChannel) QueueDeclare(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { return amqp.Queue{Name: name}, f.declareErr } +func (f *fakeChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { + return nil +} func (f *fakeChannel) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { return amqp.Queue{Name: name}, f.declarePassErr } From 2420d3eefe1b5b95241059b45b449e897c4eef87 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 10:53:43 +0100 Subject: [PATCH 04/31] add DefaultQueueConfig --- cmd/planner/main.go | 4 +- cmd/webhook-gateway/main.go | 3 +- internal/planner/consumer.go | 4 +- internal/queue/config.go | 29 ++++++++ internal/queue/consumer.go | 33 +++++---- internal/queue/consumer_test.go | 104 +++++++++++++-------------- internal/queue/producer.go | 14 ++-- internal/queue/producer_test.go | 123 ++++++++++++++++++-------------- internal/queue/types.go | 18 ++++- internal/queue/types_test.go | 1 - 10 files changed, 197 insertions(+), 136 deletions(-) create mode 100644 internal/queue/config.go diff --git a/cmd/planner/main.go b/cmd/planner/main.go index bcfccb5c..ea8da28f 100644 --- a/cmd/planner/main.go +++ b/cmd/planner/main.go @@ -23,6 +23,7 @@ import ( "github.com/canonical/github-runner-operators/internal/database" "github.com/canonical/github-runner-operators/internal/planner" + "github.com/canonical/github-runner-operators/internal/queue" "github.com/canonical/github-runner-operators/internal/telemetry" "github.com/canonical/github-runner-operators/internal/version" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" @@ -33,7 +34,6 @@ const ( portEnvVar = "APP_PORT" serviceName = "github-runner-planner" rabbitMQUriEnvVar = "RABBITMQ_CONNECT_STRING" - queueName = "webhook-queue" shutdownTimeout = 30 * time.Second ) @@ -71,7 +71,7 @@ func main() { metrics := planner.NewMetrics(db) - consumer := planner.NewJobConsumer(rabbitMQUri, queueName, db, metrics) + consumer := planner.NewJobConsumer(rabbitMQUri, queue.DefaultQueueConfig(), db, metrics) var consumerWg sync.WaitGroup consumerWg.Add(1) diff --git a/cmd/webhook-gateway/main.go b/cmd/webhook-gateway/main.go index 50a6e23c..e378d35a 100644 --- a/cmd/webhook-gateway/main.go +++ b/cmd/webhook-gateway/main.go @@ -19,7 +19,6 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) -const queueName = "webhook-queue" const webhookPath = "/webhook" const portEnvVar = "APP_PORT" const rabbitMQUriEnvVar = "RABBITMQ_CONNECT_STRING" @@ -51,7 +50,7 @@ func main() { log.Fatalln(webhookSecretEnvVar + " environment variable not set") } - p := queue.NewAmqpProducer(uri, queueName) + p := queue.NewAmqpProducer(uri, queue.DefaultQueueConfig()) handler := &webhook.Handler{ WebhookSecret: webhookSecret, Producer: p, diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index af64cccf..38ec97fc 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -32,9 +32,9 @@ const ( maxBackoff = 5 * time.Minute ) -func NewJobConsumer(amqpUri, queueName string, db JobDatabase, metrics *Metrics) *JobConsumer { +func NewJobConsumer(amqpUri string, queueConfig queue.QueueConfig, db JobDatabase, metrics *Metrics) *JobConsumer { return &JobConsumer{ - consumer: queue.NewAmqpConsumer(amqpUri, queueName), + consumer: queue.NewAmqpConsumer(amqpUri, queueConfig), db: db, metrics: metrics, } diff --git a/internal/queue/config.go b/internal/queue/config.go new file mode 100644 index 00000000..da334189 --- /dev/null +++ b/internal/queue/config.go @@ -0,0 +1,29 @@ +/* + * Copyright 2026 Canonical Ltd. + * See LICENSE file for licensing details. + */ + +package queue + +// Default AMQP configuration values. +const ( + defaultExchangeName = "github-workflow-jobs" + defaultQueueName = "github-workflow-jobs" + defaultRoutingKey = "workflow-job" +) + +// QueueConfig holds AMQP exchange, queue, and routing configuration. +type QueueConfig struct { + ExchangeName string + QueueName string + RoutingKey string +} + +// DefaultQueueConfig returns a QueueConfig with default values. +func DefaultQueueConfig() QueueConfig { + return QueueConfig{ + ExchangeName: defaultExchangeName, + QueueName: defaultQueueName, + RoutingKey: defaultRoutingKey, + } +} diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index 6b849e28..e96ab814 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -18,21 +18,19 @@ import ( // AmqpConsumer is an AMQP consumer for workflow job events. type AmqpConsumer struct { - client *Client - exchangeName string - queueName string - channel <-chan amqp.Delivery + client *Client + config QueueConfig + channel <-chan amqp.Delivery } // NewAmqpConsumer creates a new AmqpConsumer with the given dependencies. -func NewAmqpConsumer(uri, exchangeName string, queueName string) *AmqpConsumer { +func NewAmqpConsumer(uri string, config QueueConfig) *AmqpConsumer { return &AmqpConsumer{ - exchangeName: exchangeName, - queueName: queueName, client: &Client{ uri: uri, connectFunc: amqpConnect, }, + config: config, } } @@ -83,13 +81,13 @@ func (c *AmqpConsumer) ensureChannel() error { } channel, err := c.client.amqpChannel.Consume( - c.queueName, // queue - "", // consumer tag, empty string means a unique random tag will be generated - false, // whether rabbitmq auto-acknowledges messages - true, // whether only this consumer can access the queue - false, // no-local - false, // false means wait for server confirmation that consumer is registered - nil, // args + c.config.QueueName, // queue + "", // consumer tag, empty string means a unique random tag will be generated + false, // whether rabbitmq auto-acknowledges messages + true, // whether only this consumer can access the queue + false, // no-local + false, // false means wait for server confirmation that consumer is registered + nil, // args ) if err != nil { return fmt.Errorf("cannot consume amqp messages: %w", err) @@ -113,7 +111,12 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } - err = c.client.declareExchange(c.exchangeName) + err = c.client.declareExchange(c.config.ExchangeName) + if err != nil { + return err + } + + err = c.client.declareQueue(c.config.QueueName) if err != nil { return err } diff --git a/internal/queue/consumer_test.go b/internal/queue/consumer_test.go index 89e91521..a5945c4e 100644 --- a/internal/queue/consumer_test.go +++ b/internal/queue/consumer_test.go @@ -1,5 +1,5 @@ /* - * Copyright 2025 Canonical Ltd. + * Copyright 2026 Canonical Ltd. * See LICENSE file for licensing details. */ @@ -14,12 +14,6 @@ import ( "github.com/stretchr/testify/assert" ) -const queueName = "test-queue" - -const queueWithDeclareError = "queue-with-declare-error" -const queueWithConsumeError = "queue-with-consume-error" -const queueWithQosError = "queue-with-qos-error" - type MockAmqpChannelConsumer struct { MockAmqpChannel msgChannel chan amqp.Delivery @@ -30,10 +24,12 @@ type MockAmqpChannelConsumer struct { qosCalls int consumerTag string consumerAutoAck bool + consumeError bool + qosError bool } func (ch *MockAmqpChannelConsumer) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { - if queue == queueWithConsumeError { + if ch.consumeError { return nil, errors.New("consume error") } ch.consumeCalls++ @@ -46,7 +42,7 @@ func (ch *MockAmqpChannelConsumer) Consume(queue, consumer string, autoAck, excl } func (ch *MockAmqpChannelConsumer) Qos(prefetchCount, prefetchSize int, global bool) error { - if ch.queueName == queueWithQosError { + if ch.qosError { return errors.New("qos error") } ch.qosCalls++ @@ -62,6 +58,9 @@ type MockAmqpConnectionConsumer struct { isclosed bool errMode bool confirmModeError bool + exchangeDeclareError bool + queueDeclareError bool + bindError bool returnChannelOnSetup bool } @@ -75,7 +74,10 @@ func (m *MockAmqpConnectionConsumer) Channel() (amqpChannel, error) { } m.amqpChannelConsumer = &MockAmqpChannelConsumer{ MockAmqpChannel: MockAmqpChannel{ - confirmModeError: m.confirmModeError, + confirmModeError: m.confirmModeError, + exchangeDeclareError: m.exchangeDeclareError, + queueDeclareError: m.queueDeclareError, + bindError: m.bindError, }, } return m.amqpChannelConsumer, nil @@ -115,8 +117,8 @@ func TestPull(t *testing.T) { amqpChannelConsumer: mockAmqpChannel, }, }, - queueName: queueName, - channel: msgChannel, + config: DefaultQueueConfig(), + channel: msgChannel, } msg, err := amqpConsumer.Pull(context.Background()) @@ -146,8 +148,8 @@ func TestPullContextCanceled(t *testing.T) { amqpChannelConsumer: mockAmqpChannel, }, }, - queueName: queueName, - channel: msgChannel, + config: DefaultQueueConfig(), + channel: msgChannel, } _, err := amqpConsumer.Pull(ctx) @@ -175,8 +177,8 @@ func TestPullChannelClosed(t *testing.T) { amqpChannelConsumer: mockAmqpChannel, }, }, - queueName: queueName, - channel: msgChannel, + config: DefaultQueueConfig(), + channel: msgChannel, } _, err := amqpConsumer.Pull(context.Background()) @@ -225,7 +227,7 @@ func TestPullNoChannel(t *testing.T) { amqpChannel: tt.channel, amqpConnection: mockAmqpConnection, }, - queueName: queueName, + config: DefaultQueueConfig(), } msg, err := amqpConsumer.Pull(context.Background()) @@ -255,7 +257,7 @@ func TestPullNoChannelFailure(t *testing.T) { amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - queueName: queueName, + config: DefaultQueueConfig(), } _, err := amqpConsumer.Pull(context.Background()) @@ -304,7 +306,7 @@ func TestPullNoConnection(t *testing.T) { return mockAmqpConnection, nil }, }, - queueName: queueName, + config: DefaultQueueConfig(), } msg, err := amqpConsumer.Pull(context.Background()) @@ -330,7 +332,7 @@ func TestPullNoConnectionFailure(t *testing.T) { return nil, errors.New("connection error") }, }, - queueName: queueName, + config: DefaultQueueConfig(), } _, err := amqpConsumer.Pull(context.Background()) @@ -339,11 +341,11 @@ func TestPullNoConnectionFailure(t *testing.T) { assert.ErrorContains(t, err, "connection error") } -func TestPullQueueDeclare(t *testing.T) { +func TestPullQueueDeclareAndBind(t *testing.T) { /* arrange: create a consumer with no amqp channel act: pull a message from the queue - assert: channel is established and exchange and queue is declared + assert: channel is established, exchange and queue declared, and queue bound */ msgChannel := make(chan amqp.Delivery, 1) msgChannel <- amqp.Delivery{Body: []byte("TestMessage")} @@ -355,25 +357,28 @@ func TestPullQueueDeclare(t *testing.T) { amqpChannelConsumer: mockAmqpChannelConsumer, returnChannelOnSetup: true, } + config := DefaultQueueConfig() amqpConsumer := &AmqpConsumer{ client: &Client{ amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - exchangeName: exchangeName, - queueName: queueName, + config: config, } _, err := amqpConsumer.Pull(context.Background()) assert.NoError(t, err) assert.True(t, mockAmqpChannelConsumer.confirmMode, "expected channel to be in confirm mode") - assert.Equal(t, exchangeName, mockAmqpChannelConsumer.exchangeName, "expected exchange name to be "+exchangeName) - assert.Equal(t, queueName, mockAmqpChannelConsumer.queueName, "expected queue name to be "+queueName) + assert.Equal(t, config.ExchangeName, mockAmqpChannelConsumer.exchangeName, "expected exchange name to be "+config.ExchangeName) + assert.Equal(t, config.QueueName, mockAmqpChannelConsumer.queueName, "expected queue name to be "+config.QueueName) assert.True(t, mockAmqpChannelConsumer.queueDurable, "expected queue to be durable") + assert.Equal(t, config.QueueName, mockAmqpChannelConsumer.boundQueue, "expected queue to be bound") + assert.Equal(t, config.ExchangeName, mockAmqpChannelConsumer.boundExchange, "expected queue to be bound to exchange") + assert.Equal(t, config.RoutingKey, mockAmqpChannelConsumer.boundRoutingKey, "expected routing key to be "+config.RoutingKey) } -func TestPullQueueDeclareFailure(t *testing.T) { +func TestPullDeclarationFailure(t *testing.T) { /* arrange: create a consumer with no amqp channel where a declaration fails act: pull a message from the queue @@ -381,29 +386,26 @@ func TestPullQueueDeclareFailure(t *testing.T) { */ tests := []struct { name string - exchangeName string - queueName string mockAmqpConnection *MockAmqpConnectionConsumer errMsg string }{ { name: "exchange declare error", - exchangeName: exchangeWithDeclareError, - queueName: queueName, - mockAmqpConnection: &MockAmqpConnectionConsumer{}, + mockAmqpConnection: &MockAmqpConnectionConsumer{exchangeDeclareError: true}, errMsg: "exchange declare error", }, { name: "queue declare error", - exchangeName: exchangeName, - queueName: queueWithDeclareError, - mockAmqpConnection: &MockAmqpConnectionConsumer{}, + mockAmqpConnection: &MockAmqpConnectionConsumer{queueDeclareError: true}, errMsg: "queue declare error", }, { - name: "confirm error", - exchangeName: exchangeWithDeclareError, - queueName: queueName, + name: "queue bind error", + mockAmqpConnection: &MockAmqpConnectionConsumer{bindError: true}, + errMsg: "bind error", + }, + { + name: "confirm error", mockAmqpConnection: &MockAmqpConnectionConsumer{ confirmModeError: true, }, @@ -418,13 +420,12 @@ func TestPullQueueDeclareFailure(t *testing.T) { amqpChannel: nil, amqpConnection: tt.mockAmqpConnection, }, - exchangeName: tt.exchangeName, - queueName: tt.queueName, + config: DefaultQueueConfig(), } _, err := amqpConsumer.Pull(context.Background()) - assert.Error(t, err, "expected error when queue declare fails") + assert.Error(t, err, "expected error when declaration fails") assert.ErrorContains(t, err, tt.errMsg) }) } @@ -437,9 +438,7 @@ func TestPullQosFailure(t *testing.T) { assert: pull returns an error */ mockAmqpChannelConsumer := &MockAmqpChannelConsumer{ - MockAmqpChannel: MockAmqpChannel{ - queueName: queueWithQosError, - }, + qosError: true, } mockAmqpConnection := &MockAmqpConnectionConsumer{ amqpChannelConsumer: mockAmqpChannelConsumer, @@ -450,7 +449,7 @@ func TestPullQosFailure(t *testing.T) { amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - queueName: queueWithQosError, + config: DefaultQueueConfig(), } _, err := amqpConsumer.Pull(context.Background()) @@ -465,16 +464,19 @@ func TestPullConsumeFailure(t *testing.T) { act: pull a message from the queue assert: pull returns an error */ + mockAmqpChannelConsumer := &MockAmqpChannelConsumer{ + consumeError: true, + } mockAmqpConnection := &MockAmqpConnectionConsumer{ returnChannelOnSetup: true, - amqpChannelConsumer: &MockAmqpChannelConsumer{}, + amqpChannelConsumer: mockAmqpChannelConsumer, } amqpConsumer := &AmqpConsumer{ client: &Client{ amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - queueName: queueWithConsumeError, + config: DefaultQueueConfig(), } _, err := amqpConsumer.Pull(context.Background()) @@ -490,15 +492,13 @@ func TestNewAmqpConsumer(t *testing.T) { assert: consumer is properly initialized */ uri := "amqp://guest:guest@localhost:5672/" - exchangeName := "test-exchange" - queueName := "test-queue" + config := DefaultQueueConfig() - consumer := NewAmqpConsumer(uri, exchangeName, queueName) + consumer := NewAmqpConsumer(uri, config) assert.NotNil(t, consumer) - assert.Equal(t, exchangeName, consumer.exchangeName) - assert.Equal(t, queueName, consumer.queueName) assert.NotNil(t, consumer.client) assert.Equal(t, uri, consumer.client.uri) assert.NotNil(t, consumer.client.connectFunc) + assert.Equal(t, config, consumer.config) } diff --git a/internal/queue/producer.go b/internal/queue/producer.go index 376ea87d..cfdda8c8 100644 --- a/internal/queue/producer.go +++ b/internal/queue/producer.go @@ -45,7 +45,7 @@ func (p *AmqpProducer) resetConnectionOrChannelIfNecessary() error { return err } - err = p.client.declareExchange(p.exchangeName) + err = p.client.declareExchange(p.config.ExchangeName) if err != nil { return err } @@ -68,10 +68,10 @@ func waitForMsgConfirmation(ctx context.Context, confirmation confirmation) erro func (p *AmqpProducer) publishMsg(msg []byte, headers map[string]interface{}) (confirmation, error) { confirmation, err := p.client.amqpChannel.PublishWithDeferredConfirm( - "", // exchange - p.exchangeName, // routing key - false, // mandatory - false, // immediate + p.config.ExchangeName, // exchange + p.config.RoutingKey, // routing key + false, // mandatory + false, // immediate amqp.Publishing{ ContentType: "application/json", Body: msg, @@ -85,13 +85,13 @@ func (p *AmqpProducer) publishMsg(msg []byte, headers map[string]interface{}) (c return confirmation, nil } -func NewAmqpProducer(uri string, queueName string) *AmqpProducer { +func NewAmqpProducer(uri string, config QueueConfig) *AmqpProducer { return &AmqpProducer{ client: &Client{ uri: uri, connectFunc: amqpConnect, }, - exchangeName: queueName, + config: config, } } diff --git a/internal/queue/producer_test.go b/internal/queue/producer_test.go index 55c22390..51a2ff08 100644 --- a/internal/queue/producer_test.go +++ b/internal/queue/producer_test.go @@ -14,13 +14,6 @@ import ( "github.com/stretchr/testify/assert" ) -const exchangeName = "test-exchange" -const exchangeWithDeclareError = "exchange-with-declare-error" - -const exchangeWithPublishNoAck = "exchange-with-publish-no-ack" -const exchangeWithPublishError = "exchange-with-publish-error" -const exchangeWithPublishHangs = "exchange-with-publish-hangs" - // TODO : Put this in a helper file because it is also used by consumer tests type MockAmqpChannel struct { msgs [][]byte @@ -31,11 +24,22 @@ type MockAmqpChannel struct { exchangeName string queueDurable bool confirmModeError bool + // Publish error modes + publishError bool + publishNoAck bool + publishHangs bool + // Declaration error modes + exchangeDeclareError bool + queueDeclareError bool + // Binding tracking + boundQueue string + boundExchange string + boundRoutingKey string + bindError bool } -func (ch *MockAmqpChannel) PublishWithDeferredConfirm(_ string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { - - if key == exchangeWithPublishError { +func (ch *MockAmqpChannel) PublishWithDeferredConfirm(exchange string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { + if ch.publishError { return nil, errors.New("publish error") } ch.msgs = append(ch.msgs, msg.Body) @@ -43,11 +47,11 @@ func (ch *MockAmqpChannel) PublishWithDeferredConfirm(_ string, key string, _, _ done_ch := make(chan struct{}, 1) - if key != exchangeWithPublishHangs { + if !ch.publishHangs { done_ch <- struct{}{} } - ack := key != exchangeWithPublishNoAck + ack := !ch.publishNoAck confirmation := &MockConfirmation{ done: done_ch, ack: ack, @@ -68,7 +72,7 @@ func (ch *MockAmqpChannel) Confirm(_ bool) error { } func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ bool, _ amqp.Table) error { - if name == exchangeWithDeclareError { + if ch.exchangeDeclareError { return errors.New("exchange declare error") } ch.exchangeName = name @@ -76,7 +80,7 @@ func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ boo } func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ amqp.Table) (amqp.Queue, error) { - if name == queueWithDeclareError { + if ch.queueDeclareError { return amqp.Queue{}, errors.New("queue declare error") } ch.queueName = name @@ -88,6 +92,16 @@ func (ch *MockAmqpChannel) QueueDeclarePassive(name string, durable, autoDelete, return amqp.Queue{}, nil } +func (ch *MockAmqpChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { + if ch.bindError { + return errors.New("bind error") + } + ch.boundQueue = name + ch.boundRoutingKey = key + ch.boundExchange = exchange + return nil +} + func (ch *MockAmqpChannel) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { return nil, nil } @@ -115,11 +129,12 @@ func (c *MockConfirmation) Acked() bool { } type MockAmqpConnection struct { - channelCalls int - amqpChannel *MockAmqpChannel - isclosed bool - errMode bool - confirmModeError bool + channelCalls int + amqpChannel *MockAmqpChannel + isclosed bool + errMode bool + confirmModeError bool + exchangeDeclareError bool } func (m *MockAmqpConnection) Channel() (amqpChannel, error) { @@ -128,7 +143,8 @@ func (m *MockAmqpConnection) Channel() (amqpChannel, error) { } m.channelCalls++ m.amqpChannel = &MockAmqpChannel{ - confirmModeError: m.confirmModeError, + confirmModeError: m.confirmModeError, + exchangeDeclareError: m.exchangeDeclareError, } return m.amqpChannel, nil } @@ -144,7 +160,7 @@ func (m *MockAmqpConnection) Close() error { func TestPush(t *testing.T) { /* - arrange: create a queue with a fake amqp connection + arrange: create a producer with a fake amqp connection act: push a message to the queue assert: message was published to the amqp channel */ @@ -156,7 +172,7 @@ func TestPush(t *testing.T) { amqpChannel: mockAmqpChannel, }, }, - exchangeName: exchangeName, + config: DefaultQueueConfig(), } headers := map[string]interface{}{"header1": "value1"} @@ -168,21 +184,21 @@ func TestPush(t *testing.T) { func TestPushFailure(t *testing.T) { /* - arrange: create a queue with a fake confirm handler that always fails + arrange: create a producer with a fake confirm handler that always fails act: push a message to the queue that will fail to publish assert: the push return an error */ tests := []struct { - name string - queueName string - context context.Context - errMsg string + name string + mockChannel *MockAmqpChannel + context context.Context + errMsg string }{ { - name: "message is not acked", - queueName: exchangeWithPublishNoAck, - errMsg: "confirmation not acknowledged", - context: context.Background(), + name: "message is not acked", + mockChannel: &MockAmqpChannel{publishNoAck: true}, + errMsg: "confirmation not acknowledged", + context: context.Background(), }, { name: "context is done", @@ -191,28 +207,27 @@ func TestPushFailure(t *testing.T) { cancel() return ctx }(), - queueName: exchangeWithPublishHangs, - errMsg: "context canceled", + mockChannel: &MockAmqpChannel{publishHangs: true}, + errMsg: "context canceled", }, { - name: "publish returns error", - queueName: exchangeWithPublishError, - errMsg: "publish error", - context: context.Background(), + name: "publish returns error", + mockChannel: &MockAmqpChannel{publishError: true}, + errMsg: "publish error", + context: context.Background(), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - mockAmqpChannel := &MockAmqpChannel{} amqpProducer := &AmqpProducer{ client: &Client{ - amqpChannel: mockAmqpChannel, + amqpChannel: tt.mockChannel, amqpConnection: &MockAmqpConnection{ - amqpChannel: mockAmqpChannel, + amqpChannel: tt.mockChannel, }, }, - exchangeName: tt.queueName, + config: DefaultQueueConfig(), } err := amqpProducer.Push(tt.context, nil, []byte("TestMessage")) @@ -252,6 +267,7 @@ func TestPushNoChannel(t *testing.T) { amqpChannel: tt.channel, amqpConnection: mockAmqpConnection, }, + config: DefaultQueueConfig(), } amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) @@ -276,6 +292,7 @@ func TestPushNoChannelFailure(t *testing.T) { amqpChannel: nil, amqpConnection: mockAmqpConnection, }, + config: DefaultQueueConfig(), } err := amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) assert.Error(t, err, "expected error when channel fails to open") @@ -316,6 +333,7 @@ func TestPushNoConnection(t *testing.T) { return mockAmqpConnection, nil }, }, + config: DefaultQueueConfig(), } amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) @@ -339,6 +357,7 @@ func TestPushNoConnectionFailure(t *testing.T) { return nil, errors.New("connection error") }, }, + config: DefaultQueueConfig(), } err := amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) @@ -350,44 +369,42 @@ func TestPushExchangeDeclare(t *testing.T) { /* arrange: create a producer with no amqp channel act: push a message to the queue - assert: channel with confirm mode is established and exchange is declared + assert: channel with confirm mode is established and exchange is declared with config name */ mockAmqpConnection := &MockAmqpConnection{} + config := DefaultQueueConfig() amqpProducer := &AmqpProducer{ client: &Client{ amqpChannel: nil, amqpConnection: mockAmqpConnection, }, - exchangeName: exchangeName, + config: config, } amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) - assert.Equal(t, mockAmqpConnection.amqpChannel.confirmMode, true, "expected channel to be in confirm mode") - assert.Equal(t, mockAmqpConnection.amqpChannel.exchangeName, exchangeName, "expected exchange name to be "+exchangeName) + assert.True(t, mockAmqpConnection.amqpChannel.confirmMode, "expected channel to be in confirm mode") + assert.Equal(t, config.ExchangeName, mockAmqpConnection.amqpChannel.exchangeName, "expected exchange name to be "+config.ExchangeName) } func TestPushExchangeDeclareFailure(t *testing.T) { /* - arrange: create a queue with no amqp channel where the exchange declare fails + arrange: create a producer with no amqp channel where the exchange declare fails act: push a message to the queue assert: push returns an error */ tests := []struct { name string - exchangeName string mockAmqpConnection *MockAmqpConnection errMsg string }{ { name: "exchange declare error", - exchangeName: exchangeWithDeclareError, - mockAmqpConnection: &MockAmqpConnection{}, + mockAmqpConnection: &MockAmqpConnection{exchangeDeclareError: true}, errMsg: "exchange declare error", }, { - name: "confirm error", - exchangeName: exchangeName, + name: "confirm error", mockAmqpConnection: &MockAmqpConnection{ confirmModeError: true, }, @@ -402,12 +419,12 @@ func TestPushExchangeDeclareFailure(t *testing.T) { amqpChannel: nil, amqpConnection: tt.mockAmqpConnection, }, - exchangeName: tt.exchangeName, + config: DefaultQueueConfig(), } err := amqpProducer.Push(context.Background(), nil, []byte("TestMessage")) - assert.Error(t, err, "expected error when queue declare fails") + assert.Error(t, err, "expected error when exchange declare fails") assert.ErrorContains(t, err, tt.errMsg) }) } diff --git a/internal/queue/types.go b/internal/queue/types.go index a27643b7..6b52d0d1 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -85,6 +85,20 @@ func (c *Client) declareQueue(queueName string) error { return nil } +func (c *Client) bindQueue(queueName, routingKey, exchangeName string) error { + err := c.amqpChannel.QueueBind( + queueName, + routingKey, + exchangeName, + false, // no-wait + nil, // arguments + ) + if err != nil { + return fmt.Errorf("failed to bind AMQP queue: %w", err) + } + return nil +} + // Close gracefully closes the AMQP channel and connection. func (c *Client) Close() error { c.mu.Lock() @@ -108,8 +122,8 @@ func (c *Client) Close() error { } type AmqpProducer struct { - client *Client - exchangeName string + client *Client + config QueueConfig } type Producer interface { diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index 73560c0f..634e08a6 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -195,7 +195,6 @@ func TestAmqpConsumer_Table(t *testing.T) { conn := &fakeConnection{isClosedRet: false} cons := &AmqpConsumer{ - queueName: "q", client: &Client{ amqpChannel: ch, amqpConnection: conn, From ce15ff785c13f5a4b26bb457009b9f64cdbc4591 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 11:11:07 +0100 Subject: [PATCH 05/31] move MockAmqpChannel to mock_test.go --- internal/queue/mock_test.go | 172 ++++++++++++++++++++++++++++++++ internal/queue/producer_test.go | 145 --------------------------- 2 files changed, 172 insertions(+), 145 deletions(-) create mode 100644 internal/queue/mock_test.go diff --git a/internal/queue/mock_test.go b/internal/queue/mock_test.go new file mode 100644 index 00000000..615bef55 --- /dev/null +++ b/internal/queue/mock_test.go @@ -0,0 +1,172 @@ +/* + * Copyright 2026 Canonical Ltd. + * See LICENSE file for licensing details. + */ + +package queue + +import ( + "errors" + + amqp "github.com/rabbitmq/amqp091-go" +) + +// MockAmqpChannel implements amqpChannel for unit testing. +type MockAmqpChannel struct { + // Message tracking + msgs [][]byte + headers []map[string]interface{} + + // State + isclosed bool + closeCalls int + closeErr error + confirmMode bool + + // Declaration tracking + queueName string + exchangeName string + queueDurable bool + + // Binding tracking + boundQueue string + boundExchange string + boundRoutingKey string + + // Publish behavior + publishError bool + publishNoAck bool + publishHangs bool + + // Error modes + confirmModeError bool + exchangeDeclareError bool + queueDeclareError bool + bindError bool +} + +func (ch *MockAmqpChannel) PublishWithDeferredConfirm(exchange string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { + if ch.publishError { + return nil, errors.New("publish error") + } + ch.msgs = append(ch.msgs, msg.Body) + ch.headers = append(ch.headers, msg.Headers) + + doneCh := make(chan struct{}, 1) + + if !ch.publishHangs { + doneCh <- struct{}{} + } + + ack := !ch.publishNoAck + conf := &MockConfirmation{ + done: doneCh, + ack: ack, + } + return conf, nil +} + +func (ch *MockAmqpChannel) IsClosed() bool { + return ch.isclosed +} + +func (ch *MockAmqpChannel) Confirm(_ bool) error { + if ch.confirmModeError { + return errors.New("confirm error") + } + ch.confirmMode = true + return nil +} + +func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ bool, _ amqp.Table) error { + if ch.exchangeDeclareError { + return errors.New("exchange declare error") + } + ch.exchangeName = name + return nil +} + +func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ amqp.Table) (amqp.Queue, error) { + if ch.queueDeclareError { + return amqp.Queue{}, errors.New("queue declare error") + } + ch.queueName = name + ch.queueDurable = durable + return amqp.Queue{Name: name}, nil +} + +func (ch *MockAmqpChannel) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { + return amqp.Queue{Name: name}, nil +} + +func (ch *MockAmqpChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { + if ch.bindError { + return errors.New("bind error") + } + ch.boundQueue = name + ch.boundRoutingKey = key + ch.boundExchange = exchange + return nil +} + +func (ch *MockAmqpChannel) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { + return nil, nil +} + +func (ch *MockAmqpChannel) Qos(prefetchCount, prefetchSize int, global bool) error { + return nil +} + +func (ch *MockAmqpChannel) Close() error { + ch.closeCalls++ + ch.isclosed = true + return ch.closeErr +} + +// MockConfirmation implements confirmation for unit testing. +type MockConfirmation struct { + done <-chan struct{} + ack bool +} + +func (c *MockConfirmation) Done() <-chan struct{} { + return c.done +} + +func (c *MockConfirmation) Acked() bool { + return c.ack +} + +// MockAmqpConnection implements amqpConnection for unit testing. +type MockAmqpConnection struct { + channelCalls int + amqpChannel *MockAmqpChannel + isclosed bool + closeCalls int + closeErr error + errMode bool + confirmModeError bool + exchangeDeclareError bool +} + +func (m *MockAmqpConnection) Channel() (amqpChannel, error) { + if m.errMode { + return nil, errors.New("failed to open channel") + } + m.channelCalls++ + m.amqpChannel = &MockAmqpChannel{ + confirmModeError: m.confirmModeError, + exchangeDeclareError: m.exchangeDeclareError, + } + return m.amqpChannel, nil +} + +func (m *MockAmqpConnection) IsClosed() bool { + return m.isclosed +} + +func (m *MockAmqpConnection) Close() error { + m.closeCalls++ + m.isclosed = true + return m.closeErr +} diff --git a/internal/queue/producer_test.go b/internal/queue/producer_test.go index 51a2ff08..0f036c77 100644 --- a/internal/queue/producer_test.go +++ b/internal/queue/producer_test.go @@ -10,154 +10,9 @@ import ( "errors" "testing" - amqp "github.com/rabbitmq/amqp091-go" "github.com/stretchr/testify/assert" ) -// TODO : Put this in a helper file because it is also used by consumer tests -type MockAmqpChannel struct { - msgs [][]byte - headers []map[string]interface{} - isclosed bool - confirmMode bool - queueName string - exchangeName string - queueDurable bool - confirmModeError bool - // Publish error modes - publishError bool - publishNoAck bool - publishHangs bool - // Declaration error modes - exchangeDeclareError bool - queueDeclareError bool - // Binding tracking - boundQueue string - boundExchange string - boundRoutingKey string - bindError bool -} - -func (ch *MockAmqpChannel) PublishWithDeferredConfirm(exchange string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { - if ch.publishError { - return nil, errors.New("publish error") - } - ch.msgs = append(ch.msgs, msg.Body) - ch.headers = append(ch.headers, msg.Headers) - - done_ch := make(chan struct{}, 1) - - if !ch.publishHangs { - done_ch <- struct{}{} - } - - ack := !ch.publishNoAck - confirmation := &MockConfirmation{ - done: done_ch, - ack: ack, - } - return confirmation, nil -} - -func (ch *MockAmqpChannel) IsClosed() bool { - return ch.isclosed -} - -func (ch *MockAmqpChannel) Confirm(_ bool) error { - if ch.confirmModeError { - return errors.New("confirm error") - } - ch.confirmMode = true - return nil -} - -func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ bool, _ amqp.Table) error { - if ch.exchangeDeclareError { - return errors.New("exchange declare error") - } - ch.exchangeName = name - return nil -} - -func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ amqp.Table) (amqp.Queue, error) { - if ch.queueDeclareError { - return amqp.Queue{}, errors.New("queue declare error") - } - ch.queueName = name - ch.queueDurable = durable - return amqp.Queue{}, nil -} - -func (ch *MockAmqpChannel) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { - return amqp.Queue{}, nil -} - -func (ch *MockAmqpChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { - if ch.bindError { - return errors.New("bind error") - } - ch.boundQueue = name - ch.boundRoutingKey = key - ch.boundExchange = exchange - return nil -} - -func (ch *MockAmqpChannel) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { - return nil, nil -} - -func (ch *MockAmqpChannel) Qos(prefetchCount, prefetchSize int, global bool) error { - return nil -} - -func (ch *MockAmqpChannel) Close() error { - ch.isclosed = true - return nil -} - -type MockConfirmation struct { - done <-chan struct{} - ack bool -} - -func (c *MockConfirmation) Done() <-chan struct{} { - return c.done -} - -func (c *MockConfirmation) Acked() bool { - return c.ack -} - -type MockAmqpConnection struct { - channelCalls int - amqpChannel *MockAmqpChannel - isclosed bool - errMode bool - confirmModeError bool - exchangeDeclareError bool -} - -func (m *MockAmqpConnection) Channel() (amqpChannel, error) { - if m.errMode { - return nil, errors.New("failed to open channel") - } - m.channelCalls++ - m.amqpChannel = &MockAmqpChannel{ - confirmModeError: m.confirmModeError, - exchangeDeclareError: m.exchangeDeclareError, - } - return m.amqpChannel, nil -} - -func (m *MockAmqpConnection) IsClosed() bool { - return m.isclosed -} - -func (m *MockAmqpConnection) Close() error { - m.isclosed = true - return nil -} - func TestPush(t *testing.T) { /* arrange: create a producer with a fake amqp connection From 51540a39a7abd7e93d6e55eb722ddf3039a9bde1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 11:15:43 +0100 Subject: [PATCH 06/31] remove fakeChannel implementation --- internal/queue/mock_test.go | 28 +++++++++-- internal/queue/types_test.go | 96 ++++++------------------------------ 2 files changed, 39 insertions(+), 85 deletions(-) diff --git a/internal/queue/mock_test.go b/internal/queue/mock_test.go index 615bef55..6b24650e 100644 --- a/internal/queue/mock_test.go +++ b/internal/queue/mock_test.go @@ -19,6 +19,7 @@ type MockAmqpChannel struct { // State isclosed bool + isClosedRet bool closeCalls int closeErr error confirmMode bool @@ -43,6 +44,14 @@ type MockAmqpChannel struct { exchangeDeclareError bool queueDeclareError bool bindError bool + + // Consumer support + consumeCh <-chan amqp.Delivery + consumeErr error + qosErr error + + // Additional error modes for types_test + queueDeclarePassiveErr error } func (ch *MockAmqpChannel) PublishWithDeferredConfirm(exchange string, key string, _, _ bool, msg amqp.Publishing) (confirmation, error) { @@ -67,7 +76,7 @@ func (ch *MockAmqpChannel) PublishWithDeferredConfirm(exchange string, key strin } func (ch *MockAmqpChannel) IsClosed() bool { - return ch.isclosed + return ch.isclosed || ch.isClosedRet } func (ch *MockAmqpChannel) Confirm(_ bool) error { @@ -96,7 +105,7 @@ func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ am } func (ch *MockAmqpChannel) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { - return amqp.Queue{Name: name}, nil + return amqp.Queue{Name: name}, ch.queueDeclarePassiveErr } func (ch *MockAmqpChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { @@ -110,11 +119,11 @@ func (ch *MockAmqpChannel) QueueBind(name, key, exchange string, noWait bool, ar } func (ch *MockAmqpChannel) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { - return nil, nil + return ch.consumeCh, ch.consumeErr } func (ch *MockAmqpChannel) Qos(prefetchCount, prefetchSize int, global bool) error { - return nil + return ch.qosErr } func (ch *MockAmqpChannel) Close() error { @@ -141,7 +150,10 @@ func (c *MockConfirmation) Acked() bool { type MockAmqpConnection struct { channelCalls int amqpChannel *MockAmqpChannel + channel amqpChannel + channelErr error isclosed bool + isClosedRet bool closeCalls int closeErr error errMode bool @@ -153,6 +165,12 @@ func (m *MockAmqpConnection) Channel() (amqpChannel, error) { if m.errMode { return nil, errors.New("failed to open channel") } + if m.channelErr != nil { + return nil, m.channelErr + } + if m.channel != nil { + return m.channel, nil + } m.channelCalls++ m.amqpChannel = &MockAmqpChannel{ confirmModeError: m.confirmModeError, @@ -162,7 +180,7 @@ func (m *MockAmqpConnection) Channel() (amqpChannel, error) { } func (m *MockAmqpConnection) IsClosed() bool { - return m.isclosed + return m.isclosed || m.isClosedRet } func (m *MockAmqpConnection) Close() error { diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index 634e08a6..0cc8abfb 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -11,70 +11,6 @@ import ( "github.com/stretchr/testify/assert" ) -// fakeChannel implements amqpChannel for unit testing without external deps. -type fakeChannel struct { - closed bool - closeCalls int - closeErr error - isClosedRet bool - - publishErr error - confirmErr error - qosErr error - consumeErr error - declareErr error - declarePassErr error - - consumeCh <-chan amqp.Delivery -} - -func (f *fakeChannel) PublishWithDeferredConfirm(exchange string, key string, mandatory, immediate bool, msg amqp.Publishing) (confirmation, error) { - return nil, f.publishErr -} -func (f *fakeChannel) IsClosed() bool { return f.closed || f.isClosedRet } -func (f *fakeChannel) Close() error { - f.closeCalls++ - f.closed = true - return f.closeErr -} -func (f *fakeChannel) Confirm(noWait bool) error { return f.confirmErr } -func (f *fakeChannel) ExchangeDeclare(name string, kind string, durable, autoDelete, internal, noWait bool, args amqp.Table) error { - return nil -} - -func (f *fakeChannel) QueueDeclare(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { - return amqp.Queue{Name: name}, f.declareErr -} -func (f *fakeChannel) QueueBind(name, key, exchange string, noWait bool, args amqp.Table) error { - return nil -} -func (f *fakeChannel) QueueDeclarePassive(name string, durable, autoDelete, exclusive, noWait bool, args amqp.Table) (amqp.Queue, error) { - return amqp.Queue{Name: name}, f.declarePassErr -} -func (f *fakeChannel) Consume(queue, consumer string, autoAck, exclusive, noLocal, noWait bool, args amqp.Table) (<-chan amqp.Delivery, error) { - return f.consumeCh, f.consumeErr -} -func (f *fakeChannel) Qos(prefetchCount, prefetchSize int, global bool) error { return f.qosErr } - -// fakeConnection implements amqpConnection for unit testing. -type fakeConnection struct { - closed bool - closeCalls int - closeErr error - isClosedRet bool - - channel amqpChannel - chErr error -} - -func (f *fakeConnection) Channel() (amqpChannel, error) { return f.channel, f.chErr } -func (f *fakeConnection) IsClosed() bool { return f.closed || f.isClosedRet } -func (f *fakeConnection) Close() error { - f.closeCalls++ - f.closed = true - return f.closeErr -} - // fakeJobDB implements JobDatabase with no-op methods for consumer tests. type fakeJobDB struct{} @@ -107,8 +43,8 @@ func TestClient_Close_Table(t *testing.T) { { name: "both open ok", client: &Client{ - amqpChannel: &fakeChannel{isClosedRet: false}, - amqpConnection: &fakeConnection{isClosedRet: false}, + amqpChannel: &MockAmqpChannel{isClosedRet: false}, + amqpConnection: &MockAmqpConnection{isClosedRet: false}, }, expectErr: nil, expectChCalls: 1, @@ -119,8 +55,8 @@ func TestClient_Close_Table(t *testing.T) { { name: "channel error first", client: &Client{ - amqpChannel: &fakeChannel{isClosedRet: false, closeErr: chErr}, - amqpConnection: &fakeConnection{isClosedRet: false, closeErr: connErr}, + amqpChannel: &MockAmqpChannel{isClosedRet: false, closeErr: chErr}, + amqpConnection: &MockAmqpConnection{isClosedRet: false, closeErr: connErr}, }, expectErr: chErr, expectChCalls: 1, @@ -129,8 +65,8 @@ func TestClient_Close_Table(t *testing.T) { { name: "connection error only", client: &Client{ - amqpChannel: &fakeChannel{isClosedRet: false}, - amqpConnection: &fakeConnection{isClosedRet: false, closeErr: connErr}, + amqpChannel: &MockAmqpChannel{isClosedRet: false}, + amqpConnection: &MockAmqpConnection{isClosedRet: false, closeErr: connErr}, }, expectErr: connErr, expectChCalls: 1, @@ -148,8 +84,8 @@ func TestClient_Close_Table(t *testing.T) { { name: "already closed resources", client: &Client{ - amqpChannel: &fakeChannel{isClosedRet: true}, - amqpConnection: &fakeConnection{isClosedRet: true}, + amqpChannel: &MockAmqpChannel{isClosedRet: true}, + amqpConnection: &MockAmqpConnection{isClosedRet: true}, }, expectErr: nil, expectChCalls: 0, @@ -167,16 +103,16 @@ func TestClient_Close_Table(t *testing.T) { assert.NoError(t, err) } - if fc, ok := tt.client.amqpChannel.(*fakeChannel); ok { + if fc, ok := tt.client.amqpChannel.(*MockAmqpChannel); ok { assert.Equal(t, tt.expectChCalls, fc.closeCalls) if tt.expectChClosed { - assert.True(t, fc.closed) + assert.True(t, fc.isclosed) } } - if fconn, ok := tt.client.amqpConnection.(*fakeConnection); ok { + if fconn, ok := tt.client.amqpConnection.(*MockAmqpConnection); ok { assert.Equal(t, tt.expectConnCalls, fconn.closeCalls) if tt.expectConnClose { - assert.True(t, fconn.closed) + assert.True(t, fconn.isclosed) } } }) @@ -191,8 +127,8 @@ func TestAmqpConsumer_Table(t *testing.T) { act: Close the consumer. assert: Check no error and verify close calls. */ - ch := &fakeChannel{isClosedRet: false} - conn := &fakeConnection{isClosedRet: false} + ch := &MockAmqpChannel{isClosedRet: false} + conn := &MockAmqpConnection{isClosedRet: false} cons := &AmqpConsumer{ client: &Client{ @@ -215,8 +151,8 @@ func TestClient_Close_Idempotent(t *testing.T) { act: Close the client twice. assert: First close should close both, second close should be a no-op. */ - ch := &fakeChannel{isClosedRet: false} - conn := &fakeConnection{isClosedRet: false} + ch := &MockAmqpChannel{isClosedRet: false} + conn := &MockAmqpConnection{isClosedRet: false} c := &Client{ amqpChannel: ch, From 738637379ca51fc63693b937ab57311fd5c15371 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 12:59:08 +0100 Subject: [PATCH 07/31] fix planner main_test --- cmd/planner/main_test.go | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 6e645cc5..f4d990e0 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -19,6 +19,7 @@ import ( "testing" "time" + "github.com/canonical/github-runner-operators/internal/queue" amqp "github.com/rabbitmq/amqp091-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -74,7 +75,7 @@ func TestMain_FlavorPressure(t *testing.T) { assert.Eventually(t, func() bool { press := ctx.getFlavorPressure(flavor) return press[flavor] > pressure - }, 20*time.Minute, 500*time.Millisecond) + }, 2*time.Minute, 100*time.Millisecond, "expected flavor pressure to increase after webhook processing") // Trigger graceful shutdown and verify services stop cleanly ctx.shutdownMain() @@ -112,7 +113,7 @@ func (ctx *testContext) constructWebhookPayload(labels []string) []byte { return body } -// declareQueue declares a RabbitMQ queue for testing purposes. +// declareQueue declares the exchange, queue, and binding for testing. func (ctx *testContext) declareQueue() { ctx.t.Helper() @@ -124,6 +125,21 @@ func (ctx *testContext) declareQueue() { require.NoError(ctx.t, err, "open channel") defer ch.Close() + config := queue.DefaultQueueConfig() + + // Declare exchange + err = ch.ExchangeDeclare( + config.ExchangeName, + "direct", // type + true, // durable + false, // auto-delete + false, // internal + false, // no-wait + nil, + ) + require.NoError(ctx.t, err, "declare exchange") + + // Declare queue _, err = ch.QueueDeclare( ctx.queueName, true, // durable @@ -133,9 +149,19 @@ func (ctx *testContext) declareQueue() { nil, ) require.NoError(ctx.t, err, "declare queue") + + // Bind queue to exchange + err = ch.QueueBind( + ctx.queueName, + config.RoutingKey, + config.ExchangeName, + false, // no-wait + nil, + ) + require.NoError(ctx.t, err, "bind queue") } -// publishAndWaitAck publishes a message to the given RabbitMQ queue and waits for an ack. +// publishAndWaitAck publishes a message to the exchange with routing key and waits for an ack. func (ctx *testContext) publishAndWaitAck(body []byte) { ctx.t.Helper() @@ -152,8 +178,11 @@ func (ctx *testContext) publishAndWaitAck(body []byte) { confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) + config := queue.DefaultQueueConfig() err = ch.Publish( - "", ctx.queueName, false, false, + config.ExchangeName, // publish to exchange instead of directly to queue + config.RoutingKey, // use routing key + false, false, amqp.Publishing{ ContentType: "application/json", Body: body, From 0037c88b96415873b29dc06883be2ac01aec86e1 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 13:06:09 +0100 Subject: [PATCH 08/31] fix webhook_test.go --- cmd/webhook-gateway/webhook_test.go | 64 +++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/cmd/webhook-gateway/webhook_test.go b/cmd/webhook-gateway/webhook_test.go index f3dbb9dd..ce201ed1 100644 --- a/cmd/webhook-gateway/webhook_test.go +++ b/cmd/webhook-gateway/webhook_test.go @@ -19,19 +19,23 @@ import ( "testing" "time" + "github.com/canonical/github-runner-operators/internal/queue" + amqp "github.com/rabbitmq/amqp091-go" "github.com/stretchr/testify/assert" ) func TestHTTPRequestIsForwarded(t *testing.T) { const payload = `{"message":"Hello, Bob!"}` + amqpUri := getAmqpUriFromEnv(t) + + setupQueue(t, amqpUri) go main() secret := getSecretFromEnv(t) sendPayloadToHTTPServer(t, payload, secret) - amqpUri := getAmqpUriFromEnv(t) msg := consumeMessage(t, amqpUri) assert.Equal(t, payload, msg, "expected message body to match") @@ -98,7 +102,7 @@ func createSignature(message string, secret string) string { return "sha256=" + hex.EncodeToString(h.Sum(nil)) } -func consumeMessage(t *testing.T, amqpUri string) string { +func setupQueue(t *testing.T, amqpUri string) { conn, err := amqp.Dial(amqpUri) if err != nil { t.Fatalf("Failed to connect to RabbitMQ: %v", err) @@ -110,20 +114,62 @@ func consumeMessage(t *testing.T, amqpUri string) string { } defer ch.Close() + config := queue.DefaultQueueConfig() + + err = ch.ExchangeDeclare( + config.ExchangeName, // name) + "direct", // type + true, // durable + false, // auto-deleted + false, // internal + false, // no-wait + nil, // arguments + ) + + if err != nil { + t.Fatalf("Failed to declare an exchange: %v", err) + } _, err = ch.QueueDeclare( - "webhook-queue", // name - true, // durable - false, // delete when unused - false, // exclusive - false, // no-wait - nil, // arguments + config.QueueName, // name + true, // durable + false, // delete when unused + false, // exclusive + false, // no-wait + nil, // arguments ) if err != nil { t.Fatalf("Failed to declare a queue: %v", err) } - deliveryChan, err := ch.Consume("webhook-queue", "", true, false, false, false, nil) + err = ch.QueueBind( + config.QueueName, // queue name + config.RoutingKey, // routing key + config.ExchangeName, // exchange + false, + nil, + ) + + if err != nil { + t.Fatalf("Failed to bind a queue: %v", err) + } +} + +func consumeMessage(t *testing.T, amqpUri string) string { + conn, err := amqp.Dial(amqpUri) + if err != nil { + t.Fatalf("Failed to connect to RabbitMQ: %v", err) + } + defer conn.Close() + ch, err := conn.Channel() + if err != nil { + t.Fatalf("Failed to open a channel: %v", err) + } + defer ch.Close() + + config := queue.DefaultQueueConfig() + + deliveryChan, err := ch.Consume(config.QueueName, "", true, false, false, false, nil) if err != nil { t.Fatalf("Failed to register a consumer: %v", err) From 435b180cd831477a2c28f94b0a1c491486be6034 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 13:10:49 +0100 Subject: [PATCH 09/31] refactor webhook_test.go --- cmd/webhook-gateway/webhook_test.go | 36 ++++++++++++++++++----------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/cmd/webhook-gateway/webhook_test.go b/cmd/webhook-gateway/webhook_test.go index ce201ed1..18d462cd 100644 --- a/cmd/webhook-gateway/webhook_test.go +++ b/cmd/webhook-gateway/webhook_test.go @@ -102,21 +102,37 @@ func createSignature(message string, secret string) string { return "sha256=" + hex.EncodeToString(h.Sum(nil)) } -func setupQueue(t *testing.T, amqpUri string) { +// returns an AMQP channel and a cleanup function to close the connection and channel +func setupAMQPChannel(t *testing.T, amqpUri string) (*amqp.Channel, func()) { conn, err := amqp.Dial(amqpUri) if err != nil { t.Fatalf("Failed to connect to RabbitMQ: %v", err) } - defer conn.Close() + ch, err := conn.Channel() if err != nil { + conn.Close() t.Fatalf("Failed to open a channel: %v", err) } - defer ch.Close() + + cleanup := func() { + ch.Close() + conn.Close() + } + + return ch, cleanup +} + +// setupQueue declares the exchange, queue, and binding for the test +// this is required because the webhook-gateway does not create the queue and +// published messages could be discarded if the queue does not exist +func setupQueue(t *testing.T, amqpUri string) { + ch, cleanup := setupAMQPChannel(t, amqpUri) + defer cleanup() config := queue.DefaultQueueConfig() - err = ch.ExchangeDeclare( + err := ch.ExchangeDeclare( config.ExchangeName, // name) "direct", // type true, // durable @@ -156,16 +172,8 @@ func setupQueue(t *testing.T, amqpUri string) { } func consumeMessage(t *testing.T, amqpUri string) string { - conn, err := amqp.Dial(amqpUri) - if err != nil { - t.Fatalf("Failed to connect to RabbitMQ: %v", err) - } - defer conn.Close() - ch, err := conn.Channel() - if err != nil { - t.Fatalf("Failed to open a channel: %v", err) - } - defer ch.Close() + ch, cleanup := setupAMQPChannel(t, amqpUri) + defer cleanup() config := queue.DefaultQueueConfig() From e1de3b1bb26c0ce8677dec663466469bb9493b07 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 13:25:54 +0100 Subject: [PATCH 10/31] remove redundant comments --- cmd/planner/main_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index f4d990e0..57b3d89b 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -127,7 +127,6 @@ func (ctx *testContext) declareQueue() { config := queue.DefaultQueueConfig() - // Declare exchange err = ch.ExchangeDeclare( config.ExchangeName, "direct", // type @@ -139,7 +138,6 @@ func (ctx *testContext) declareQueue() { ) require.NoError(ctx.t, err, "declare exchange") - // Declare queue _, err = ch.QueueDeclare( ctx.queueName, true, // durable @@ -150,7 +148,6 @@ func (ctx *testContext) declareQueue() { ) require.NoError(ctx.t, err, "declare queue") - // Bind queue to exchange err = ch.QueueBind( ctx.queueName, config.RoutingKey, From 0f61ffdede2bda8be7b84a113eab737b49d859ec Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 13:30:49 +0100 Subject: [PATCH 11/31] update CONTRIBUTING.md --- CONTRIBUTING.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f6188b9..de9ea153 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -113,7 +113,7 @@ docker run -d --rm --name postgres -e POSTGRES_PASSWORD=password -p 5432:5432 po Run `webhook-gateway` integration tests using: ```shell -APP_PORT=8080 APP_WEBHOOK_SECRET_VALUE=fake RABBITMQ_CONNECT_STRING="amqp://guest:guest@localhost:5672/" go test -cover -v ./cmd/webhook-gateway -integration +APP_WEBHOOK_SECRET_VALUE=fake APP_PORT=8080 RABBITMQ_CONNECT_STRING="amqp://guest:guest@localhost:5672/" go test -v -cover -tags=integration -race ./cmd/webhook-gateway/... ``` It assumes you have access to a RabbitMQ server running reachable at $RABBITMQ_CONNECT_STRING. @@ -126,11 +126,10 @@ docker run -d --rm --name rabbitmq -p 5672:5672 -p 15672:15672 rabbitmq:4-manag Run `planner` integration tests using: ```shell -POSTGRESQL_DB_CONNECT_STRING="postgres://postgres:postgres@localhost:5432/gh_runner_operators?sslmode=disable" APP_PORT=8080 go test ./cmd/planner -v -count=1 +APP_PORT=8080 POSTGRESQL_DB_CONNECT_STRING="postgres://postgres:password@localhost:5432/postgres?sslmode=disable" RABBITMQ_CONNECT_STRING="amqp://guest:guest@localhost:5672/" go test -v -cover -tags=integration -race ./cmd/planner/.. ``` -It assumes you are connected to a local PostgreSQL database "gh_runner_operators" as user "postgres" on host "localhost" at port "5432". - +It assumes you can reach postgres and rabbitmq servers as described above. ### Test design From ecf7d6c9ebdd5ab3384cf4039e4b1a60f21af8b7 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 6 Jan 2026 13:38:59 +0100 Subject: [PATCH 12/31] Update CONTRIBUTING.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index de9ea153..746972a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -126,7 +126,7 @@ docker run -d --rm --name rabbitmq -p 5672:5672 -p 15672:15672 rabbitmq:4-manag Run `planner` integration tests using: ```shell -APP_PORT=8080 POSTGRESQL_DB_CONNECT_STRING="postgres://postgres:password@localhost:5432/postgres?sslmode=disable" RABBITMQ_CONNECT_STRING="amqp://guest:guest@localhost:5672/" go test -v -cover -tags=integration -race ./cmd/planner/.. +APP_PORT=8080 POSTGRESQL_DB_CONNECT_STRING="postgres://postgres:password@localhost:5432/postgres?sslmode=disable" RABBITMQ_CONNECT_STRING="amqp://guest:guest@localhost:5672/" go test -v -cover -tags=integration -race ./cmd/planner/... ``` It assumes you can reach postgres and rabbitmq servers as described above. From 20be4617088c82370e0d82161b36c98afa88d118 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 08:21:31 +0100 Subject: [PATCH 13/31] add dead-letter queue for non-retryable messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement DLQ mechanism for malformed webhook messages that should not be retried (missing headers, invalid JSON, missing required fields). Unsupported event types are now silently ignored via Ack instead of being sent to DLQ. - Add DeadLetterExchange and DeadLetterQueue to QueueConfig - Create setupDeadLetterQueue() to encapsulate DLQ declaration - Add declareQueueWithDeadLetter() with x-dead-letter-exchange arg - Add ErrUnsupportedEvent sentinel error for ignorable events - Add ignoreMessage() method that uses Ack for expected filtering - Update pullMessage() to distinguish DLQ vs ignore paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- internal/planner/consumer.go | 29 +++++++++++++++++++++++------ internal/planner/consumer_test.go | 20 +++++++++----------- internal/queue/config.go | 24 +++++++++++++++--------- internal/queue/consumer.go | 7 ++++++- internal/queue/deadletter.go | 29 +++++++++++++++++++++++++++++ internal/queue/types.go | 18 ++++++++++++++++++ 6 files changed, 100 insertions(+), 27 deletions(-) create mode 100644 internal/queue/deadletter.go diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index 38ec97fc..52c976ab 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -32,6 +32,9 @@ const ( maxBackoff = 5 * time.Minute ) +// ErrUnsupportedEvent indicates a valid but unsupported webhook event that should be ignored. +var ErrUnsupportedEvent = errors.New("unsupported event") + func NewJobConsumer(amqpUri string, queueConfig queue.QueueConfig, db JobDatabase, metrics *Metrics) *JobConsumer { return &JobConsumer{ consumer: queue.NewAmqpConsumer(amqpUri, queueConfig), @@ -83,14 +86,15 @@ func parseGithubWebhookPayload(ctx context.Context, headers map[string]interface // Check if it's a workflow_job event jobEvent, ok := event.(*github.WorkflowJobEvent) if !ok { - logger.InfoContext(ctx, "event is not a workflow_job, discarding", "event_type", eventType) - return githubWebhookJob{}, fmt.Errorf("unsupported webhook event: %s", eventType) + logger.InfoContext(ctx, "event is not a workflow_job, ignoring", "event_type", eventType) + return githubWebhookJob{}, fmt.Errorf("%w: %s", ErrUnsupportedEvent, eventType) } // Process the workflow job event based on action action := jobEvent.GetAction() if !slices.Contains([]string{"queued", "in_progress", "completed", "waiting"}, action) { - return githubWebhookJob{}, fmt.Errorf("unknown webhook event action: %s", action) + logger.InfoContext(ctx, "unknown webhook event action, ignoring", "action", action) + return githubWebhookJob{}, fmt.Errorf("%w: unknown action %s", ErrUnsupportedEvent, action) } job := jobEvent.GetWorkflowJob() @@ -226,6 +230,11 @@ func (c *JobConsumer) pullMessage(ctx context.Context) error { job, err := parseGithubWebhookPayload(ctx, msg.Headers, msg.Body) if err != nil { + if errors.Is(err, ErrUnsupportedEvent) { + c.metrics.ObserveDiscardedWebhook(ctx, platform) + c.ignoreMessage(ctx, &msg) + return nil + } logger.ErrorContext(ctx, "cannot parse webhook payload", "error", err) span.RecordError(err) c.metrics.ObserveWebhookError(ctx, platform) @@ -236,14 +245,14 @@ func (c *JobConsumer) pullMessage(ctx context.Context) error { if !slices.Contains([]string{"queued", "in_progress", "completed"}, job.action) { logger.InfoContext(ctx, "ignore other action types for GitHub webhook job", "action", job.action) c.metrics.ObserveDiscardedWebhook(ctx, platform) - c.discardMessage(ctx, &msg) + c.ignoreMessage(ctx, &msg) return nil } if !isSelfHosted(&job) { logger.InfoContext(ctx, "ignore non self-hosted job", "repo", job.repo, "labels", strings.Join(job.labels, ",")) c.metrics.ObserveDiscardedWebhook(ctx, platform) - c.discardMessage(ctx, &msg) + c.ignoreMessage(ctx, &msg) return nil } @@ -269,7 +278,15 @@ func (c *JobConsumer) discardMessage(ctx context.Context, delivery *amqp.Deliver oteltrace.SpanFromContext(ctx).RecordError(err) c.metrics.ObserveWebhookError(ctx, platform) } - return +} + +func (c *JobConsumer) ignoreMessage(ctx context.Context, delivery *amqp.Delivery) { + err := delivery.Ack(false) + if err != nil { + logger.ErrorContext(ctx, "cannot ignore queue message", "error", err) + oteltrace.SpanFromContext(ctx).RecordError(err) + c.metrics.ObserveWebhookError(ctx, platform) + } } func (c *JobConsumer) consumedMessage(ctx context.Context, delivery *amqp.Delivery) { diff --git a/internal/planner/consumer_test.go b/internal/planner/consumer_test.go index 2b948c96..8ab2ee11 100644 --- a/internal/planner/consumer_test.go +++ b/internal/planner/consumer_test.go @@ -323,16 +323,15 @@ func TestConsumer(t *testing.T) { }, }), }, - expectErr: true, + expectErr: false, checkDB: func(t *testing.T, db *fakeDB) { assert.Nil(t, db.jobs["github:16"], "job should not be inserted") }, checkMetrics: func(t *testing.T, mr *telemetry.TestMetricReader) { m := mr.Collect(t) - // 2 errors: 1 from processing error, 1 from nack failure - assert.Equal(t, 2.0, m.Counter(t, webhookErrorsMetricName)) + assert.Equal(t, 1.0, m.Counter(t, webhookErrorsMetricName)) // ack failure assert.Equal(t, 0.0, m.Counter(t, consumedWebhooksMetricName)) - assert.Equal(t, 0.0, m.Counter(t, discardedWebhooksMetricName)) + assert.Equal(t, 1.0, m.Counter(t, discardedWebhooksMetricName)) }, }, { name: "requeues when AddJob fails with other error", @@ -482,7 +481,7 @@ func TestConsumer(t *testing.T) { assert.Equal(t, 0.0, m.Counter(t, discardedWebhooksMetricName)) }, }, { - name: "discards non-workflow_job events", + name: "ignores non-workflow_job events", setupDB: func(db *fakeDB) {}, delivery: amqp.Delivery{ Headers: amqp.Table{"X-GitHub-Event": "pull_request"}, @@ -493,16 +492,15 @@ func TestConsumer(t *testing.T) { }, }), }, - expectErr: true, + expectErr: false, checkDB: func(t *testing.T, db *fakeDB) { assert.Empty(t, db.jobs, "no jobs should be inserted for non-workflow_job events") }, checkMetrics: func(t *testing.T, mr *telemetry.TestMetricReader) { m := mr.Collect(t) - // 2 errors: 1 from processing error, 1 from nack failure - assert.Equal(t, 2.0, m.Counter(t, webhookErrorsMetricName)) + assert.Equal(t, 1.0, m.Counter(t, webhookErrorsMetricName)) // ack failure assert.Equal(t, 0.0, m.Counter(t, consumedWebhooksMetricName)) - assert.Equal(t, 0.0, m.Counter(t, discardedWebhooksMetricName)) + assert.Equal(t, 1.0, m.Counter(t, discardedWebhooksMetricName)) }, }, { name: "discards non-self-hosted jobs", @@ -673,7 +671,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { }, }), expectErr: true, - expectErrMsg: "unsupported webhook event", + expectErrMsg: "unsupported event", }, { name: "unknown action", headers: map[string]interface{}{ @@ -691,7 +689,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { }, }), expectErr: true, - expectErrMsg: "unknown webhook event action", + expectErrMsg: "unsupported event: unknown action", }, { name: "missing workflow_job field", headers: map[string]interface{}{ diff --git a/internal/queue/config.go b/internal/queue/config.go index da334189..b1c292ea 100644 --- a/internal/queue/config.go +++ b/internal/queue/config.go @@ -7,23 +7,29 @@ package queue // Default AMQP configuration values. const ( - defaultExchangeName = "github-workflow-jobs" - defaultQueueName = "github-workflow-jobs" - defaultRoutingKey = "workflow-job" + defaultExchangeName = "github-workflow-jobs" + defaultQueueName = "github-workflow-jobs" + defaultRoutingKey = "workflow-job" + defaultDeadLetterExchange = "github-workflow-jobs-dlx" + defaultDeadLetterQueue = "github-workflow-jobs-dlq" ) // QueueConfig holds AMQP exchange, queue, and routing configuration. type QueueConfig struct { - ExchangeName string - QueueName string - RoutingKey string + ExchangeName string + QueueName string + RoutingKey string + DeadLetterExchange string + DeadLetterQueue string } // DefaultQueueConfig returns a QueueConfig with default values. func DefaultQueueConfig() QueueConfig { return QueueConfig{ - ExchangeName: defaultExchangeName, - QueueName: defaultQueueName, - RoutingKey: defaultRoutingKey, + ExchangeName: defaultExchangeName, + QueueName: defaultQueueName, + RoutingKey: defaultRoutingKey, + DeadLetterExchange: defaultDeadLetterExchange, + DeadLetterQueue: defaultDeadLetterQueue, } } diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index e96ab814..51a1be87 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -111,12 +111,17 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } + err = c.client.setupDeadLetterQueue(c.config.DeadLetterExchange, c.config.DeadLetterQueue, c.config.RoutingKey) + if err != nil { + return err + } + err = c.client.declareExchange(c.config.ExchangeName) if err != nil { return err } - err = c.client.declareQueue(c.config.QueueName) + err = c.client.declareQueueWithDeadLetter(c.config.QueueName, c.config.DeadLetterExchange) if err != nil { return err } diff --git a/internal/queue/deadletter.go b/internal/queue/deadletter.go new file mode 100644 index 00000000..6038b89b --- /dev/null +++ b/internal/queue/deadletter.go @@ -0,0 +1,29 @@ +/* + * Copyright 2026 Canonical Ltd. + * See LICENSE file for licensing details. + */ + +package queue + +import "fmt" + +// setupDeadLetterQueue declares the dead-letter exchange and queue, and binds them together. +// This encapsulates the RabbitMQ-specific dead-letter configuration. +func (c *Client) setupDeadLetterQueue(dlxName, dlqName, routingKey string) error { + err := c.declareExchange(dlxName) + if err != nil { + return fmt.Errorf("cannot declare dead-letter exchange: %w", err) + } + + err = c.declareQueue(dlqName) + if err != nil { + return fmt.Errorf("cannot declare dead-letter queue: %w", err) + } + + err = c.bindQueue(dlqName, routingKey, dlxName) + if err != nil { + return fmt.Errorf("cannot bind dead-letter queue: %w", err) + } + + return nil +} diff --git a/internal/queue/types.go b/internal/queue/types.go index 6b52d0d1..af16d8dd 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -85,6 +85,24 @@ func (c *Client) declareQueue(queueName string) error { return nil } +func (c *Client) declareQueueWithDeadLetter(queueName, dlxName string) error { + args := amqp.Table{ + "x-dead-letter-exchange": dlxName, + } + _, err := c.amqpChannel.QueueDeclare( + queueName, + true, // durable + false, // delete when unused + false, // exclusive + false, // no-wait + args, + ) + if err != nil { + return fmt.Errorf("failed to declare AMQP queue with dead-letter: %w", err) + } + return nil +} + func (c *Client) bindQueue(queueName, routingKey, exchangeName string) error { err := c.amqpChannel.QueueBind( queueName, From 11c8473404b29d8c2ffd555e31321580b90984f8 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 09:16:47 +0100 Subject: [PATCH 14/31] add unit tests --- internal/queue/consumer_test.go | 50 ++++++++++++++++++++ internal/queue/deadletter_test.go | 78 +++++++++++++++++++++++++++++++ internal/queue/mock_test.go | 18 +++++-- internal/queue/types_test.go | 32 +++++++++++++ 4 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 internal/queue/deadletter_test.go diff --git a/internal/queue/consumer_test.go b/internal/queue/consumer_test.go index a5945c4e..d08419a1 100644 --- a/internal/queue/consumer_test.go +++ b/internal/queue/consumer_test.go @@ -378,6 +378,56 @@ func TestPullQueueDeclareAndBind(t *testing.T) { assert.Equal(t, config.RoutingKey, mockAmqpChannelConsumer.boundRoutingKey, "expected routing key to be "+config.RoutingKey) } +func TestPullSetsUpDeadLetterQueue(t *testing.T) { + /* + arrange: create a consumer with no amqp channel + act: pull a message from the queue + assert: DLX exchange declared, DLQ queue declared, main queue has DLX argument + */ + msgChannel := make(chan amqp.Delivery, 1) + msgChannel <- amqp.Delivery{Body: []byte("TestMessage")} + + mockAmqpChannelConsumer := &MockAmqpChannelConsumer{ + msgChannel: msgChannel, + } + mockAmqpConnection := &MockAmqpConnectionConsumer{ + amqpChannelConsumer: mockAmqpChannelConsumer, + returnChannelOnSetup: true, + } + config := DefaultQueueConfig() + amqpConsumer := &AmqpConsumer{ + client: &Client{ + amqpChannel: nil, + amqpConnection: mockAmqpConnection, + }, + config: config, + } + + _, err := amqpConsumer.Pull(context.Background()) + + assert.NoError(t, err) + + // Verify DLX exchange is declared + assert.Contains(t, mockAmqpChannelConsumer.MockAmqpChannel.exchangeNames, config.DeadLetterExchange, + "expected DLX exchange to be declared") + + // Verify DLQ queue is declared + assert.Contains(t, mockAmqpChannelConsumer.MockAmqpChannel.queueNames, config.DeadLetterQueue, + "expected DLQ queue to be declared") + + // Verify main queue is declared with x-dead-letter-exchange argument + assert.Equal(t, config.DeadLetterExchange, mockAmqpChannelConsumer.MockAmqpChannel.queueArgs["x-dead-letter-exchange"], + "expected main queue to have x-dead-letter-exchange argument") + + // Verify both exchanges are declared (DLX and main) + assert.Equal(t, 2, mockAmqpChannelConsumer.MockAmqpChannel.exchangeCount, + "expected 2 exchanges to be declared (DLX and main)") + + // Verify both queues are declared (DLQ and main) + assert.Equal(t, 2, mockAmqpChannelConsumer.MockAmqpChannel.declareCount, + "expected 2 queues to be declared (DLQ and main)") +} + func TestPullDeclarationFailure(t *testing.T) { /* arrange: create a consumer with no amqp channel where a declaration fails diff --git a/internal/queue/deadletter_test.go b/internal/queue/deadletter_test.go new file mode 100644 index 00000000..9679f6ed --- /dev/null +++ b/internal/queue/deadletter_test.go @@ -0,0 +1,78 @@ +/* + * Copyright 2026 Canonical Ltd. + * See LICENSE file for licensing details. + */ + +package queue + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSetupDeadLetterQueue(t *testing.T) { + /* + arrange: Create a client with mock channel. + act: Call setupDeadLetterQueue. + assert: DLX exchange declared, DLQ queue declared, and queue bound to exchange. + */ + ch := &MockAmqpChannel{} + client := &Client{amqpChannel: ch} + + err := client.setupDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") + + assert.NoError(t, err) + assert.Contains(t, ch.exchangeNames, "test-dlx", "DLX exchange should be declared") + assert.Contains(t, ch.queueNames, "test-dlq", "DLQ queue should be declared") + assert.Equal(t, "test-dlq", ch.boundQueue) + assert.Equal(t, "test-dlx", ch.boundExchange) + assert.Equal(t, "test-routing-key", ch.boundRoutingKey) +} + +func TestSetupDeadLetterQueueErrors(t *testing.T) { + /* + arrange: Create a client with mock channel configured for each error scenario. + act: Call setupDeadLetterQueue. + assert: Appropriate error returned. + */ + tests := []struct { + name string + exchangeDeclareError bool + queueDeclareError bool + bindError bool + expectErrContains string + }{ + { + name: "returns error when exchange declare fails", + exchangeDeclareError: true, + expectErrContains: "cannot declare dead-letter exchange", + }, + { + name: "returns error when queue declare fails", + queueDeclareError: true, + expectErrContains: "cannot declare dead-letter queue", + }, + { + name: "returns error when queue bind fails", + bindError: true, + expectErrContains: "cannot bind dead-letter queue", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ch := &MockAmqpChannel{ + exchangeDeclareError: tt.exchangeDeclareError, + queueDeclareError: tt.queueDeclareError, + bindError: tt.bindError, + } + client := &Client{amqpChannel: ch} + + err := client.setupDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") + + assert.Error(t, err) + assert.ErrorContains(t, err, tt.expectErrContains) + }) + } +} diff --git a/internal/queue/mock_test.go b/internal/queue/mock_test.go index 6b24650e..6b84e44c 100644 --- a/internal/queue/mock_test.go +++ b/internal/queue/mock_test.go @@ -25,9 +25,14 @@ type MockAmqpChannel struct { confirmMode bool // Declaration tracking - queueName string - exchangeName string - queueDurable bool + queueName string + exchangeName string + queueDurable bool + queueArgs amqp.Table + declareCount int + exchangeNames []string + queueNames []string + exchangeCount int // Binding tracking boundQueue string @@ -92,15 +97,20 @@ func (ch *MockAmqpChannel) ExchangeDeclare(name string, _ string, _, _, _, _ boo return errors.New("exchange declare error") } ch.exchangeName = name + ch.exchangeNames = append(ch.exchangeNames, name) + ch.exchangeCount++ return nil } -func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, _ amqp.Table) (amqp.Queue, error) { +func (ch *MockAmqpChannel) QueueDeclare(name string, durable, _, _, _ bool, args amqp.Table) (amqp.Queue, error) { if ch.queueDeclareError { return amqp.Queue{}, errors.New("queue declare error") } ch.queueName = name ch.queueDurable = durable + ch.queueArgs = args + ch.queueNames = append(ch.queueNames, name) + ch.declareCount++ return amqp.Queue{Name: name}, nil } diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index 0cc8abfb..bcfec3d9 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -169,3 +169,35 @@ func TestClient_Close_Idempotent(t *testing.T) { assert.Equal(t, 1, ch.closeCalls) assert.Equal(t, 1, conn.closeCalls) } + +func TestDeclareQueueWithDeadLetter(t *testing.T) { + /* + arrange: Create a client with mock channel. + act: Call declareQueueWithDeadLetter. + assert: Queue is declared with x-dead-letter-exchange argument. + */ + ch := &MockAmqpChannel{} + client := &Client{amqpChannel: ch} + + err := client.declareQueueWithDeadLetter("test-queue", "test-dlx") + + assert.NoError(t, err) + assert.Equal(t, "test-queue", ch.queueName) + assert.True(t, ch.queueDurable) + assert.Equal(t, "test-dlx", ch.queueArgs["x-dead-letter-exchange"]) +} + +func TestDeclareQueueWithDeadLetterError(t *testing.T) { + /* + arrange: Create a client with mock channel that returns error. + act: Call declareQueueWithDeadLetter. + assert: Error is returned. + */ + ch := &MockAmqpChannel{queueDeclareError: true} + client := &Client{amqpChannel: ch} + + err := client.declareQueueWithDeadLetter("test-queue", "test-dlx") + + assert.Error(t, err) + assert.ErrorContains(t, err, "failed to declare AMQP queue with dead-letter") +} From 9761c6e4c810d5b5fa73a5fbf6826dfd5c336318 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 09:40:22 +0100 Subject: [PATCH 15/31] add integration test --- cmd/planner/main_test.go | 241 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 57b3d89b..a6845e94 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -316,3 +316,244 @@ func randString(n int) string { } return string(b) } + +func TestMain_DeadLetterQueue(t *testing.T) { + /* + arrange: server is listening and DLQ is set up + act: publish malformed message and unsupported event + assert: malformed message appears in DLQ, unsupported event does not + */ + ctx := newTestContext(t) + ctx.declareQueueWithDLQ() + + go main() + ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) + + config := queue.DefaultQueueConfig() + + // First: publish unsupported event (pull_request) - should be ignored, not in DLQ + unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) + ctx.publishWithHeader(unsupportedBody, "pull_request") + + // Wait for message to be processed + time.Sleep(2 * time.Second) + + // Verify DLQ is empty after unsupported event + dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) + assert.Equal(t, 0, dlqDepth, "unsupported events should NOT appear in DLQ") + + // Second: publish malformed message (missing X-GitHub-Event header) - should go to DLQ + malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) + ctx.publishWithoutHeader(malformedBody) + + // Wait for message to be processed and appear in DLQ + dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) + assert.Equal(t, malformedBody, dlqMessage, "malformed message should appear in DLQ") + + ctx.shutdownMain() +} + +// declareQueueWithDLQ declares the exchange, queue, DLQ and bindings for testing. +func (ctx *testContext) declareQueueWithDLQ() { + ctx.t.Helper() + + conn, err := amqp.Dial(ctx.rabbitURI) + require.NoError(ctx.t, err, "connect rabbitmq") + defer conn.Close() + + ch, err := conn.Channel() + require.NoError(ctx.t, err, "open channel") + defer ch.Close() + + config := queue.DefaultQueueConfig() + + // Declare DLX exchange + err = ch.ExchangeDeclare( + config.DeadLetterExchange, + "direct", + true, + false, + false, + false, + nil, + ) + require.NoError(ctx.t, err, "declare DLX exchange") + + // Declare DLQ + _, err = ch.QueueDeclare( + config.DeadLetterQueue, + true, + false, + false, + false, + nil, + ) + require.NoError(ctx.t, err, "declare DLQ") + + // Bind DLQ to DLX + err = ch.QueueBind( + config.DeadLetterQueue, + config.RoutingKey, + config.DeadLetterExchange, + false, + nil, + ) + require.NoError(ctx.t, err, "bind DLQ") + + // Declare main exchange + err = ch.ExchangeDeclare( + config.ExchangeName, + "direct", + true, + false, + false, + false, + nil, + ) + require.NoError(ctx.t, err, "declare exchange") + + // Declare main queue with DLX argument + _, err = ch.QueueDeclare( + ctx.queueName, + true, + false, + false, + false, + amqp.Table{"x-dead-letter-exchange": config.DeadLetterExchange}, + ) + require.NoError(ctx.t, err, "declare queue with DLX") + + // Bind main queue + err = ch.QueueBind( + ctx.queueName, + config.RoutingKey, + config.ExchangeName, + false, + nil, + ) + require.NoError(ctx.t, err, "bind queue") +} + +// publishWithoutHeader publishes a message without X-GitHub-Event header. +func (ctx *testContext) publishWithoutHeader(body []byte) { + ctx.t.Helper() + + conn, err := amqp.Dial(ctx.rabbitURI) + require.NoError(ctx.t, err, "connect rabbitmq") + defer conn.Close() + + ch, err := conn.Channel() + require.NoError(ctx.t, err, "open channel") + defer ch.Close() + + err = ch.Confirm(false) + require.NoError(ctx.t, err, "enable confirms") + + confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) + + config := queue.DefaultQueueConfig() + err = ch.Publish( + config.ExchangeName, + config.RoutingKey, + false, false, + amqp.Publishing{ + ContentType: "application/json", + Body: body, + // No X-GitHub-Event header - this makes it malformed + }, + ) + require.NoError(ctx.t, err, "publish") + + select { + case c := <-confirms: + require.True(ctx.t, c.Ack, "message not acked") + case <-time.After(10 * time.Second): + require.Fail(ctx.t, "timeout waiting for ack") + } +} + +// publishWithHeader publishes a message with the specified X-GitHub-Event header. +func (ctx *testContext) publishWithHeader(body []byte, eventType string) { + ctx.t.Helper() + + conn, err := amqp.Dial(ctx.rabbitURI) + require.NoError(ctx.t, err, "connect rabbitmq") + defer conn.Close() + + ch, err := conn.Channel() + require.NoError(ctx.t, err, "open channel") + defer ch.Close() + + err = ch.Confirm(false) + require.NoError(ctx.t, err, "enable confirms") + + confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) + + config := queue.DefaultQueueConfig() + err = ch.Publish( + config.ExchangeName, + config.RoutingKey, + false, false, + amqp.Publishing{ + ContentType: "application/json", + Body: body, + Headers: amqp.Table{"X-GitHub-Event": eventType}, + }, + ) + require.NoError(ctx.t, err, "publish") + + select { + case c := <-confirms: + require.True(ctx.t, c.Ack, "message not acked") + case <-time.After(10 * time.Second): + require.Fail(ctx.t, "timeout waiting for ack") + } +} + +// consumeFromQueue consumes a single message from the specified queue. +func (ctx *testContext) consumeFromQueue(queueName string, timeout time.Duration) []byte { + ctx.t.Helper() + + conn, err := amqp.Dial(ctx.rabbitURI) + require.NoError(ctx.t, err, "connect rabbitmq") + defer conn.Close() + + ch, err := conn.Channel() + require.NoError(ctx.t, err, "open channel") + defer ch.Close() + + deliveryChan, err := ch.Consume(queueName, "", true, false, false, false, nil) + require.NoError(ctx.t, err, "consume from queue") + + select { + case msg := <-deliveryChan: + return msg.Body + case <-time.After(timeout): + require.Fail(ctx.t, "timeout waiting for message in "+queueName) + return nil + } +} + +// getQueueDepthByName returns the current number of messages in the specified queue. +func (ctx *testContext) getQueueDepthByName(queueName string) int { + ctx.t.Helper() + + conn, err := amqp.Dial(ctx.rabbitURI) + require.NoError(ctx.t, err, "connect rabbitmq") + defer conn.Close() + + ch, err := conn.Channel() + require.NoError(ctx.t, err, "open channel") + defer ch.Close() + + q, err := ch.QueueDeclarePassive( + queueName, + true, + false, + false, + false, + nil, + ) + require.NoError(ctx.t, err, "declare passive queue") + return q.Messages +} From b667590689723b3e465f3f6238783982747fa716 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 10:15:38 +0100 Subject: [PATCH 16/31] add one integration test using multiple scenarios --- cmd/planner/main_test.go | 98 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 95 insertions(+), 3 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index a6845e94..8d623c80 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -38,7 +38,7 @@ func newTestContext(t *testing.T) *testContext { t: t, port: os.Getenv("APP_PORT"), rabbitURI: os.Getenv("RABBITMQ_CONNECT_STRING"), - queueName: "webhook-queue", + queueName: queue.DefaultQueueConfig().QueueName, } } @@ -49,7 +49,7 @@ func TestMain_FlavorPressure(t *testing.T) { assert: 201 Created and 200 OK with expected pressure value updated after webhook processing */ ctx := newTestContext(t) - ctx.declareQueue() + //ctx.declareQueue() go main() ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) @@ -317,6 +317,98 @@ func randString(n int) string { return string(b) } +func TestMain_IntegrationScenarios(t *testing.T) { + /* + arrange: server is listening with full AMQP setup including DLQ + act: Test multiple scenarios in subtests + assert: all scenarios work correctly + */ + ctx := newTestContext(t) + + go main() + ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) + + // Scenario 1: Flavor pressure updates + t.Run("flavor_pressure", func(t *testing.T) { + /* + act: send create flavor request, get flavor pressure request and publish webhook message + assert: 201 Created and 200 OK with expected pressure value updated after webhook processing + */ + + platform := "github" + labels := []string{"self-hosted", "s390x", "medium"} + priority := 100 + flavor := randString(10) + pressure := 0 + + // Test create flavor + resp := ctx.createFlavor(flavor, platform, labels, priority) + require.Equal(t, http.StatusCreated, resp, "unexpected status creating flavor") + + // Test get flavor pressure + pressures := ctx.getFlavorPressure(flavor) + ctx.assertFlavorPressureEquals(pressures, flavor, pressure) + + // Test consume webhook and reflect pressure + body := ctx.constructWebhookPayload(labels) + ctx.publishAndWaitAck(body) + + assert.Eventually(t, func() bool { + press := ctx.getFlavorPressure(flavor) + return press[flavor] > pressure + }, 2*time.Minute, 100*time.Millisecond, "expected flavor pressure to increase after webhook processing") + }) + + // Scenario 2: Dead letter queue behavior + t.Run("dead_letter_queue", func(t *testing.T) { + /* + act: + 1. unsupported event + 2. malformed message + assert: + 1. unsupported event does not appear in DLQ + 2. malformed message appears in DLQ + */ + config := queue.DefaultQueueConfig() + + // Unsupported event should be ignored + unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) + ctx.publishWithHeader(unsupportedBody, "pull_request") + time.Sleep(2 * time.Second) // TODO replace with assert.Eventually + + dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) + assert.Equal(t, 0, dlqDepth) + + // Malformed message should go to DLQ + malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) + ctx.publishWithoutHeader(malformedBody) + + dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) + assert.Equal(t, malformedBody, dlqMessage) + }) + + // Last scenario: Graceful shutdown + t.Run("graceful_shutdown", func(t *testing.T) { + /* + act: trigger graceful shutdown + assert: services stop cleanly and AMQP consumer stops processing messages + */ + labels := []string{"self-hosted", "s390x", "medium"} + + ctx.shutdownMain() + ctx.waitForHTTPDown("http://localhost:"+ctx.port+"/health", 30*time.Second) + + // Verify AMQP consumer is stopped by checking queue depth increases after publish + beforeDepth := ctx.getQueueDepth() + body2 := ctx.constructWebhookPayload(labels) + ctx.publishAndWaitAck(body2) + + assert.Eventually(t, func() bool { + return ctx.getQueueDepth() >= beforeDepth+1 + }, 10*time.Second, 200*time.Millisecond, "expected queue depth to increase after shutdown (consumer stopped)") + }) +} + func TestMain_DeadLetterQueue(t *testing.T) { /* arrange: server is listening and DLQ is set up @@ -324,7 +416,7 @@ func TestMain_DeadLetterQueue(t *testing.T) { assert: malformed message appears in DLQ, unsupported event does not */ ctx := newTestContext(t) - ctx.declareQueueWithDLQ() + //ctx.declareQueueWithDLQ() go main() ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) From c21f3ad39d74704c0f6fd93939f923ab0034b4d7 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 12:11:57 +0100 Subject: [PATCH 17/31] refactor integration test --- cmd/planner/main_test.go | 502 ++++++++------------------------------- 1 file changed, 97 insertions(+), 405 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 8d623c80..4c237629 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -25,6 +25,8 @@ import ( "github.com/stretchr/testify/require" ) +const supportedEventType = "workflow_job" + // testContext holds test configuration and dependencies. type testContext struct { t *testing.T @@ -42,53 +44,98 @@ func newTestContext(t *testing.T) *testContext { } } -func TestMain_FlavorPressure(t *testing.T) { +func TestMain_IntegrationScenarios(t *testing.T) { /* - arrange: server is listening on the configured port and prepare request payload - act: send create flavor request, get flavor pressure request and publish webhook message - assert: 201 Created and 200 OK with expected pressure value updated after webhook processing + arrange: server is listening with full AMQP setup including DLQ + act: Test multiple scenarios in subtests + assert: all scenarios work correctly */ ctx := newTestContext(t) - //ctx.declareQueue() go main() ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) - platform := "github" - labels := []string{"self-hosted", "s390x", "medium"} - priority := 100 - flavor := randString(10) - pressure := 0 - - // Test create flavor - resp := ctx.createFlavor(flavor, platform, labels, priority) - require.Equal(t, http.StatusCreated, resp, "unexpected status creating flavor") - - // Test get flavor pressure - pressures := ctx.getFlavorPressure(flavor) - ctx.assertFlavorPressureEquals(pressures, flavor, pressure) - - // Test consume webhook and reflect pressure - body := ctx.constructWebhookPayload(labels) - ctx.publishAndWaitAck(body) - - assert.Eventually(t, func() bool { - press := ctx.getFlavorPressure(flavor) - return press[flavor] > pressure - }, 2*time.Minute, 100*time.Millisecond, "expected flavor pressure to increase after webhook processing") - - // Trigger graceful shutdown and verify services stop cleanly - ctx.shutdownMain() - ctx.waitForHTTPDown("http://localhost:"+ctx.port+"/health", 30*time.Second) - - // Verify AMQP consumer is stopped by checking queue depth increases after publish - beforeDepth := ctx.getQueueDepth() - body2 := ctx.constructWebhookPayload(labels) - ctx.publishAndWaitAck(body2) - - assert.Eventually(t, func() bool { - return ctx.getQueueDepth() >= beforeDepth+1 - }, 10*time.Second, 200*time.Millisecond, "expected queue depth to increase after shutdown (consumer stopped)") + // Scenario 1: Flavor pressure updates + t.Run("flavor_pressure", func(t *testing.T) { + /* + act: send create flavor request, get flavor pressure request and publish webhook message + assert: 201 Created and 200 OK with expected pressure value updated after webhook processing + */ + + platform := "github" + labels := []string{"self-hosted", "s390x", "medium"} + priority := 100 + flavor := randString(10) + pressure := 0 + + // Test create flavor + resp := ctx.createFlavor(flavor, platform, labels, priority) + require.Equal(t, http.StatusCreated, resp, "unexpected status creating flavor") + + // Test get flavor pressure + pressures := ctx.getFlavorPressure(flavor) + ctx.assertFlavorPressureEquals(pressures, flavor, pressure) + + // Test consume webhook and reflect pressure + body := ctx.constructWebhookPayload(labels) + ctx.publish(body, supportedEventType) + + assert.Eventually(t, func() bool { + press := ctx.getFlavorPressure(flavor) + return press[flavor] > pressure + }, 2*time.Minute, 100*time.Millisecond, "expected flavor pressure to increase after webhook processing") + }) + + // Scenario 2: Dead letter queue behavior + t.Run("dead_letter_queue", func(t *testing.T) { + /* + act: + 1. unsupported event + 2. malformed message + assert: + 1. unsupported event does not appear in DLQ + 2. malformed message appears in DLQ + */ + config := queue.DefaultQueueConfig() + + // Unsupported event should be ignored + unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) + ctx.publish(unsupportedBody, "pull_request") + time.Sleep(2 * time.Second) // TODO replace with assert.Eventually + + dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) + assert.Equal(t, 0, dlqDepth) + + // Malformed message should go to DLQ + malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) + ctx.publish(malformedBody, supportedEventType) + + dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) + assert.Equal(t, malformedBody, dlqMessage) + }) + + // Last scenario: Graceful shutdown + t.Run("graceful_shutdown", func(t *testing.T) { + /* + act: trigger graceful shutdown + assert: services stop cleanly and AMQP consumer stops processing messages + */ + config := queue.DefaultQueueConfig() + + labels := []string{"self-hosted", "s390x", "medium"} + + ctx.shutdownMain() + ctx.waitForHTTPDown("http://localhost:"+ctx.port+"/health", 30*time.Second) + + // Verify AMQP consumer is stopped by checking queue depth increases after publish + beforeDepth := ctx.getQueueDepthByName(config.QueueName) + body2 := ctx.constructWebhookPayload(labels) + ctx.publish(body2, supportedEventType) + + assert.Eventually(t, func() bool { + return ctx.getQueueDepthByName(config.QueueName) >= beforeDepth+1 + }, 10*time.Second, 200*time.Millisecond, "expected queue depth to increase after shutdown (consumer stopped)") + }) } // constructWebhookPayload creates a webhook payload with the given labels. @@ -113,113 +160,6 @@ func (ctx *testContext) constructWebhookPayload(labels []string) []byte { return body } -// declareQueue declares the exchange, queue, and binding for testing. -func (ctx *testContext) declareQueue() { - ctx.t.Helper() - - conn, err := amqp.Dial(ctx.rabbitURI) - require.NoError(ctx.t, err, "connect rabbitmq") - defer conn.Close() - - ch, err := conn.Channel() - require.NoError(ctx.t, err, "open channel") - defer ch.Close() - - config := queue.DefaultQueueConfig() - - err = ch.ExchangeDeclare( - config.ExchangeName, - "direct", // type - true, // durable - false, // auto-delete - false, // internal - false, // no-wait - nil, - ) - require.NoError(ctx.t, err, "declare exchange") - - _, err = ch.QueueDeclare( - ctx.queueName, - true, // durable - false, // auto-delete - false, // exclusive - false, // no-wait - nil, - ) - require.NoError(ctx.t, err, "declare queue") - - err = ch.QueueBind( - ctx.queueName, - config.RoutingKey, - config.ExchangeName, - false, // no-wait - nil, - ) - require.NoError(ctx.t, err, "bind queue") -} - -// publishAndWaitAck publishes a message to the exchange with routing key and waits for an ack. -func (ctx *testContext) publishAndWaitAck(body []byte) { - ctx.t.Helper() - - conn, err := amqp.Dial(ctx.rabbitURI) - require.NoError(ctx.t, err, "connect rabbitmq") - defer conn.Close() - - ch, err := conn.Channel() - require.NoError(ctx.t, err, "open channel") - defer ch.Close() - - err = ch.Confirm(false) - require.NoError(ctx.t, err, "enable confirms") - - confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) - - config := queue.DefaultQueueConfig() - err = ch.Publish( - config.ExchangeName, // publish to exchange instead of directly to queue - config.RoutingKey, // use routing key - false, false, - amqp.Publishing{ - ContentType: "application/json", - Body: body, - Headers: amqp.Table{"X-GitHub-Event": "workflow_job"}, - }, - ) - require.NoError(ctx.t, err, "publish") - - select { - case c := <-confirms: - require.True(ctx.t, c.Ack, "message not acked") - case <-time.After(10 * time.Second): - require.Fail(ctx.t, "timeout waiting for ack") - } -} - -// getQueueDepth returns the current number of messages in the queue via passive declare. -func (ctx *testContext) getQueueDepth() int { - ctx.t.Helper() - - conn, err := amqp.Dial(ctx.rabbitURI) - require.NoError(ctx.t, err, "connect rabbitmq") - defer conn.Close() - - ch, err := conn.Channel() - require.NoError(ctx.t, err, "open channel") - defer ch.Close() - - q, err := ch.QueueDeclarePassive( - ctx.queueName, - true, // durable - false, // auto-delete - false, // exclusive - false, // no-wait - nil, - ) - require.NoError(ctx.t, err, "declare passive queue") - return q.Messages -} - // shutdownMain sends SIGTERM to trigger graceful shutdown in main(). func (ctx *testContext) shutdownMain() { ctx.t.Helper() @@ -317,217 +257,9 @@ func randString(n int) string { return string(b) } -func TestMain_IntegrationScenarios(t *testing.T) { - /* - arrange: server is listening with full AMQP setup including DLQ - act: Test multiple scenarios in subtests - assert: all scenarios work correctly - */ - ctx := newTestContext(t) - - go main() - ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) - - // Scenario 1: Flavor pressure updates - t.Run("flavor_pressure", func(t *testing.T) { - /* - act: send create flavor request, get flavor pressure request and publish webhook message - assert: 201 Created and 200 OK with expected pressure value updated after webhook processing - */ - - platform := "github" - labels := []string{"self-hosted", "s390x", "medium"} - priority := 100 - flavor := randString(10) - pressure := 0 - - // Test create flavor - resp := ctx.createFlavor(flavor, platform, labels, priority) - require.Equal(t, http.StatusCreated, resp, "unexpected status creating flavor") - - // Test get flavor pressure - pressures := ctx.getFlavorPressure(flavor) - ctx.assertFlavorPressureEquals(pressures, flavor, pressure) - - // Test consume webhook and reflect pressure - body := ctx.constructWebhookPayload(labels) - ctx.publishAndWaitAck(body) - - assert.Eventually(t, func() bool { - press := ctx.getFlavorPressure(flavor) - return press[flavor] > pressure - }, 2*time.Minute, 100*time.Millisecond, "expected flavor pressure to increase after webhook processing") - }) - - // Scenario 2: Dead letter queue behavior - t.Run("dead_letter_queue", func(t *testing.T) { - /* - act: - 1. unsupported event - 2. malformed message - assert: - 1. unsupported event does not appear in DLQ - 2. malformed message appears in DLQ - */ - config := queue.DefaultQueueConfig() - - // Unsupported event should be ignored - unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) - ctx.publishWithHeader(unsupportedBody, "pull_request") - time.Sleep(2 * time.Second) // TODO replace with assert.Eventually - - dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) - assert.Equal(t, 0, dlqDepth) - - // Malformed message should go to DLQ - malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) - ctx.publishWithoutHeader(malformedBody) - - dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) - assert.Equal(t, malformedBody, dlqMessage) - }) - - // Last scenario: Graceful shutdown - t.Run("graceful_shutdown", func(t *testing.T) { - /* - act: trigger graceful shutdown - assert: services stop cleanly and AMQP consumer stops processing messages - */ - labels := []string{"self-hosted", "s390x", "medium"} - - ctx.shutdownMain() - ctx.waitForHTTPDown("http://localhost:"+ctx.port+"/health", 30*time.Second) - - // Verify AMQP consumer is stopped by checking queue depth increases after publish - beforeDepth := ctx.getQueueDepth() - body2 := ctx.constructWebhookPayload(labels) - ctx.publishAndWaitAck(body2) - - assert.Eventually(t, func() bool { - return ctx.getQueueDepth() >= beforeDepth+1 - }, 10*time.Second, 200*time.Millisecond, "expected queue depth to increase after shutdown (consumer stopped)") - }) -} - -func TestMain_DeadLetterQueue(t *testing.T) { - /* - arrange: server is listening and DLQ is set up - act: publish malformed message and unsupported event - assert: malformed message appears in DLQ, unsupported event does not - */ - ctx := newTestContext(t) - //ctx.declareQueueWithDLQ() - - go main() - ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) - - config := queue.DefaultQueueConfig() - - // First: publish unsupported event (pull_request) - should be ignored, not in DLQ - unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) - ctx.publishWithHeader(unsupportedBody, "pull_request") - - // Wait for message to be processed - time.Sleep(2 * time.Second) - - // Verify DLQ is empty after unsupported event - dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) - assert.Equal(t, 0, dlqDepth, "unsupported events should NOT appear in DLQ") - - // Second: publish malformed message (missing X-GitHub-Event header) - should go to DLQ - malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) - ctx.publishWithoutHeader(malformedBody) - - // Wait for message to be processed and appear in DLQ - dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) - assert.Equal(t, malformedBody, dlqMessage, "malformed message should appear in DLQ") - - ctx.shutdownMain() -} - -// declareQueueWithDLQ declares the exchange, queue, DLQ and bindings for testing. -func (ctx *testContext) declareQueueWithDLQ() { - ctx.t.Helper() - - conn, err := amqp.Dial(ctx.rabbitURI) - require.NoError(ctx.t, err, "connect rabbitmq") - defer conn.Close() - - ch, err := conn.Channel() - require.NoError(ctx.t, err, "open channel") - defer ch.Close() - - config := queue.DefaultQueueConfig() - - // Declare DLX exchange - err = ch.ExchangeDeclare( - config.DeadLetterExchange, - "direct", - true, - false, - false, - false, - nil, - ) - require.NoError(ctx.t, err, "declare DLX exchange") - - // Declare DLQ - _, err = ch.QueueDeclare( - config.DeadLetterQueue, - true, - false, - false, - false, - nil, - ) - require.NoError(ctx.t, err, "declare DLQ") - - // Bind DLQ to DLX - err = ch.QueueBind( - config.DeadLetterQueue, - config.RoutingKey, - config.DeadLetterExchange, - false, - nil, - ) - require.NoError(ctx.t, err, "bind DLQ") - - // Declare main exchange - err = ch.ExchangeDeclare( - config.ExchangeName, - "direct", - true, - false, - false, - false, - nil, - ) - require.NoError(ctx.t, err, "declare exchange") - - // Declare main queue with DLX argument - _, err = ch.QueueDeclare( - ctx.queueName, - true, - false, - false, - false, - amqp.Table{"x-dead-letter-exchange": config.DeadLetterExchange}, - ) - require.NoError(ctx.t, err, "declare queue with DLX") - - // Bind main queue - err = ch.QueueBind( - ctx.queueName, - config.RoutingKey, - config.ExchangeName, - false, - nil, - ) - require.NoError(ctx.t, err, "bind queue") -} - -// publishWithoutHeader publishes a message without X-GitHub-Event header. -func (ctx *testContext) publishWithoutHeader(body []byte) { +// publish a message to the exchange with body and eventType header +// To omit the header, please pass an empty string as eventType. +func (ctx *testContext) publish(body []byte, eventType string) { ctx.t.Helper() conn, err := amqp.Dial(ctx.rabbitURI) @@ -540,58 +272,18 @@ func (ctx *testContext) publishWithoutHeader(body []byte) { err = ch.Confirm(false) require.NoError(ctx.t, err, "enable confirms") - confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) config := queue.DefaultQueueConfig() - err = ch.Publish( - config.ExchangeName, - config.RoutingKey, - false, false, - amqp.Publishing{ - ContentType: "application/json", - Body: body, - // No X-GitHub-Event header - this makes it malformed - }, - ) - require.NoError(ctx.t, err, "publish") - - select { - case c := <-confirms: - require.True(ctx.t, c.Ack, "message not acked") - case <-time.After(10 * time.Second): - require.Fail(ctx.t, "timeout waiting for ack") + publishing := amqp.Publishing{ + ContentType: "application/json", + Body: body, + } + if eventType != "" { + publishing.Headers = amqp.Table{"X-GitHub-Event": eventType} } -} - -// publishWithHeader publishes a message with the specified X-GitHub-Event header. -func (ctx *testContext) publishWithHeader(body []byte, eventType string) { - ctx.t.Helper() - - conn, err := amqp.Dial(ctx.rabbitURI) - require.NoError(ctx.t, err, "connect rabbitmq") - defer conn.Close() - - ch, err := conn.Channel() - require.NoError(ctx.t, err, "open channel") - defer ch.Close() - - err = ch.Confirm(false) - require.NoError(ctx.t, err, "enable confirms") - - confirms := ch.NotifyPublish(make(chan amqp.Confirmation, 1)) - config := queue.DefaultQueueConfig() - err = ch.Publish( - config.ExchangeName, - config.RoutingKey, - false, false, - amqp.Publishing{ - ContentType: "application/json", - Body: body, - Headers: amqp.Table{"X-GitHub-Event": eventType}, - }, - ) + err = ch.Publish(config.ExchangeName, config.RoutingKey, false, false, publishing) require.NoError(ctx.t, err, "publish") select { From 706dca9dd4f0683b6b6ae553c9568d7218d27434 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 7 Jan 2026 12:16:45 +0100 Subject: [PATCH 18/31] remove sleep --- cmd/planner/main_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 4c237629..20035c7d 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -101,7 +101,6 @@ func TestMain_IntegrationScenarios(t *testing.T) { // Unsupported event should be ignored unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) ctx.publish(unsupportedBody, "pull_request") - time.Sleep(2 * time.Second) // TODO replace with assert.Eventually dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) assert.Equal(t, 0, dlqDepth) @@ -258,6 +257,7 @@ func randString(n int) string { } // publish a message to the exchange with body and eventType header +// publish waits for confirmation that the message was accepted by RabbitMQ. // To omit the header, please pass an empty string as eventType. func (ctx *testContext) publish(body []byte, eventType string) { ctx.t.Helper() From 71942d6667710c8c6fea3e53b0a77096b0024e9c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 8 Jan 2026 07:34:53 +0100 Subject: [PATCH 19/31] remove redundant comments --- CONTRIBUTING.md | 2 +- internal/queue/consumer_test.go | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 746972a3..5e713a1c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -107,7 +107,7 @@ POSTGRESQL_DB_CONNECT_STRING="postgres://postgres:password@localhost:5432/postgr It assumes you have access to a Postgres server running reachable at $POSTGRESQL_DB_CONNECT_STRING. You can use `docker` to run a Postgres server locally: ```shell -docker run -d --rm --name postgres -e POSTGRES_PASSWORD=password -p 5432:5432 postgres:18.1 +docker run -d --rm --name postgres -e POSTGRES_PASSWORD=password -p 5432:5432 postgres:16.11 ``` Run `webhook-gateway` integration tests using: diff --git a/internal/queue/consumer_test.go b/internal/queue/consumer_test.go index d08419a1..b69be6a8 100644 --- a/internal/queue/consumer_test.go +++ b/internal/queue/consumer_test.go @@ -407,23 +407,18 @@ func TestPullSetsUpDeadLetterQueue(t *testing.T) { assert.NoError(t, err) - // Verify DLX exchange is declared assert.Contains(t, mockAmqpChannelConsumer.MockAmqpChannel.exchangeNames, config.DeadLetterExchange, "expected DLX exchange to be declared") - // Verify DLQ queue is declared assert.Contains(t, mockAmqpChannelConsumer.MockAmqpChannel.queueNames, config.DeadLetterQueue, "expected DLQ queue to be declared") - // Verify main queue is declared with x-dead-letter-exchange argument assert.Equal(t, config.DeadLetterExchange, mockAmqpChannelConsumer.MockAmqpChannel.queueArgs["x-dead-letter-exchange"], "expected main queue to have x-dead-letter-exchange argument") - // Verify both exchanges are declared (DLX and main) assert.Equal(t, 2, mockAmqpChannelConsumer.MockAmqpChannel.exchangeCount, "expected 2 exchanges to be declared (DLX and main)") - // Verify both queues are declared (DLQ and main) assert.Equal(t, 2, mockAmqpChannelConsumer.MockAmqpChannel.declareCount, "expected 2 queues to be declared (DLQ and main)") } From 2db4423a5fff0f5021d90a560fa1ecfd7467addc Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 8 Jan 2026 07:44:53 +0100 Subject: [PATCH 20/31] comply with STYLE guide --- internal/queue/types.go | 14 +++++++------- internal/queue/types_test.go | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/queue/types.go b/internal/queue/types.go index af16d8dd..c543ac1a 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -28,7 +28,7 @@ func (c *Client) resetConnection() error { } conn, err := c.connectFunc(c.uri) if err != nil { - return fmt.Errorf("failed to connect to AMQP server: %w", err) + return fmt.Errorf("cannot connect to AMQP server: %w", err) } c.amqpConnection = conn return nil @@ -42,13 +42,13 @@ func (c *Client) resetChannel() error { ch, err := c.amqpConnection.Channel() if err != nil { - return fmt.Errorf("failed to open AMQP channel: %w", err) + return fmt.Errorf("cannot open AMQP channel: %w", err) } c.amqpChannel = ch err = ch.Confirm(false) if err != nil { - return fmt.Errorf("failed to put channel in confirm mode: %w", err) + return fmt.Errorf("cannot put channel in confirm mode: %w", err) } return nil @@ -65,7 +65,7 @@ func (c *Client) declareExchange(exchangeName string) error { nil, // no additional arguments ) if err != nil { - return fmt.Errorf("failed to declare AMQP exchange: %w", err) + return fmt.Errorf("cannot declare AMQP exchange: %w", err) } return nil } @@ -80,7 +80,7 @@ func (c *Client) declareQueue(queueName string) error { nil, // arguments ) if err != nil { - return fmt.Errorf("failed to declare AMQP queue: %w", err) + return fmt.Errorf("cannot declare AMQP queue: %w", err) } return nil } @@ -98,7 +98,7 @@ func (c *Client) declareQueueWithDeadLetter(queueName, dlxName string) error { args, ) if err != nil { - return fmt.Errorf("failed to declare AMQP queue with dead-letter: %w", err) + return fmt.Errorf("cannot declare AMQP queue with dead-letter: %w", err) } return nil } @@ -112,7 +112,7 @@ func (c *Client) bindQueue(queueName, routingKey, exchangeName string) error { nil, // arguments ) if err != nil { - return fmt.Errorf("failed to bind AMQP queue: %w", err) + return fmt.Errorf("cannot bind AMQP queue: %w", err) } return nil } diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index bcfec3d9..c4c0dcb8 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -199,5 +199,5 @@ func TestDeclareQueueWithDeadLetterError(t *testing.T) { err := client.declareQueueWithDeadLetter("test-queue", "test-dlx") assert.Error(t, err) - assert.ErrorContains(t, err, "failed to declare AMQP queue with dead-letter") + assert.ErrorContains(t, err, "cannot declare AMQP queue with dead-letter") } From bad5d8e97a1bcb03701aa6b02a5b8f96be498f5f Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 8 Jan 2026 07:50:24 +0100 Subject: [PATCH 21/31] remove redundant tests --- internal/queue/types_test.go | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/internal/queue/types_test.go b/internal/queue/types_test.go index c4c0dcb8..0cc8abfb 100644 --- a/internal/queue/types_test.go +++ b/internal/queue/types_test.go @@ -169,35 +169,3 @@ func TestClient_Close_Idempotent(t *testing.T) { assert.Equal(t, 1, ch.closeCalls) assert.Equal(t, 1, conn.closeCalls) } - -func TestDeclareQueueWithDeadLetter(t *testing.T) { - /* - arrange: Create a client with mock channel. - act: Call declareQueueWithDeadLetter. - assert: Queue is declared with x-dead-letter-exchange argument. - */ - ch := &MockAmqpChannel{} - client := &Client{amqpChannel: ch} - - err := client.declareQueueWithDeadLetter("test-queue", "test-dlx") - - assert.NoError(t, err) - assert.Equal(t, "test-queue", ch.queueName) - assert.True(t, ch.queueDurable) - assert.Equal(t, "test-dlx", ch.queueArgs["x-dead-letter-exchange"]) -} - -func TestDeclareQueueWithDeadLetterError(t *testing.T) { - /* - arrange: Create a client with mock channel that returns error. - act: Call declareQueueWithDeadLetter. - assert: Error is returned. - */ - ch := &MockAmqpChannel{queueDeclareError: true} - client := &Client{amqpChannel: ch} - - err := client.declareQueueWithDeadLetter("test-queue", "test-dlx") - - assert.Error(t, err) - assert.ErrorContains(t, err, "cannot declare AMQP queue with dead-letter") -} From dc4adf29ebb1769b081db711da32d6a14ee2191c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 8 Jan 2026 08:01:06 +0100 Subject: [PATCH 22/31] pin postgres 16.11 --- .github/workflows/internal_tests.yaml | 2 +- .github/workflows/planner_integration.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/internal_tests.yaml b/.github/workflows/internal_tests.yaml index f565f772..6d1cb967 100644 --- a/.github/workflows/internal_tests.yaml +++ b/.github/workflows/internal_tests.yaml @@ -59,7 +59,7 @@ jobs: services: postgres: - image: postgres:18.1 + image: postgres:16.11 env: POSTGRES_PASSWORD: postgres ports: diff --git a/.github/workflows/planner_integration.yaml b/.github/workflows/planner_integration.yaml index 05b154ea..2500d71d 100644 --- a/.github/workflows/planner_integration.yaml +++ b/.github/workflows/planner_integration.yaml @@ -56,7 +56,7 @@ jobs: services: postgres: - image: postgres:18.1 + image: postgres:16.11 env: POSTGRES_PASSWORD: postgres ports: From 0ce1183ac1bbb5ca57c0e6233723ea5494825572 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Thu, 8 Jan 2026 08:15:07 +0100 Subject: [PATCH 23/31] change to debug log level for ignoring log messages --- internal/planner/consumer.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index 8a815996..08877630 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -86,14 +86,12 @@ func parseGithubWebhookPayload(ctx context.Context, headers map[string]interface // Check if it's a workflow_job event jobEvent, ok := event.(*github.WorkflowJobEvent) if !ok { - logger.InfoContext(ctx, "event is not a workflow_job, ignoring", "event_type", eventType) return githubWebhookJob{}, fmt.Errorf("%w: %s", ErrUnsupportedEvent, eventType) } // Process the workflow job event based on action action := jobEvent.GetAction() if !slices.Contains([]string{"queued", "in_progress", "completed", "waiting"}, action) { - logger.InfoContext(ctx, "unknown webhook event action, ignoring", "action", action) return githubWebhookJob{}, fmt.Errorf("%w: unknown action %s", ErrUnsupportedEvent, action) } @@ -231,6 +229,7 @@ func (c *JobConsumer) pullMessage(ctx context.Context) error { job, err := parseGithubWebhookPayload(ctx, msg.Headers, msg.Body) if err != nil { if errors.Is(err, ErrUnsupportedEvent) { + logger.DebugContext(ctx, "ignoring unsupported event", "error", err) c.metrics.ObserveDiscardedWebhook(ctx, platform) c.ignoreMessage(ctx, &msg) return nil @@ -243,14 +242,14 @@ func (c *JobConsumer) pullMessage(ctx context.Context) error { } if !slices.Contains([]string{"queued", "in_progress", "completed"}, job.action) { - logger.InfoContext(ctx, "ignore other action types for GitHub webhook job", "action", job.action) + logger.DebugContext(ctx, "ignore other action types for GitHub webhook job", "action", job.action) c.metrics.ObserveDiscardedWebhook(ctx, platform) c.ignoreMessage(ctx, &msg) return nil } if !isSelfHosted(&job) { - logger.InfoContext(ctx, "ignore non self-hosted job", "repo", job.repo, "labels", strings.Join(job.labels, ",")) + logger.DebugContext(ctx, "ignore non self-hosted job", "repo", job.repo, "labels", strings.Join(job.labels, ",")) c.metrics.ObserveDiscardedWebhook(ctx, platform) c.ignoreMessage(ctx, &msg) return nil From 7141b9a4566c39bf0d130c22ccef7eb3ec411d18 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 10:20:56 +0100 Subject: [PATCH 24/31] getQueueDepthByName -> getNumMessagesInQueue --- cmd/planner/main_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 20035c7d..dbaf36aa 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -102,7 +102,7 @@ func TestMain_IntegrationScenarios(t *testing.T) { unsupportedBody := []byte(`{"action": "opened", "pull_request": {"id": 123}}`) ctx.publish(unsupportedBody, "pull_request") - dlqDepth := ctx.getQueueDepthByName(config.DeadLetterQueue) + dlqDepth := ctx.getNumMessagesInQueue(config.DeadLetterQueue) assert.Equal(t, 0, dlqDepth) // Malformed message should go to DLQ @@ -127,12 +127,12 @@ func TestMain_IntegrationScenarios(t *testing.T) { ctx.waitForHTTPDown("http://localhost:"+ctx.port+"/health", 30*time.Second) // Verify AMQP consumer is stopped by checking queue depth increases after publish - beforeDepth := ctx.getQueueDepthByName(config.QueueName) + beforeDepth := ctx.getNumMessagesInQueue(config.QueueName) body2 := ctx.constructWebhookPayload(labels) ctx.publish(body2, supportedEventType) assert.Eventually(t, func() bool { - return ctx.getQueueDepthByName(config.QueueName) >= beforeDepth+1 + return ctx.getNumMessagesInQueue(config.QueueName) >= beforeDepth+1 }, 10*time.Second, 200*time.Millisecond, "expected queue depth to increase after shutdown (consumer stopped)") }) } @@ -318,8 +318,8 @@ func (ctx *testContext) consumeFromQueue(queueName string, timeout time.Duration } } -// getQueueDepthByName returns the current number of messages in the specified queue. -func (ctx *testContext) getQueueDepthByName(queueName string) int { +// getNumMessagesInQueue returns the current number of messages in the specified queue. +func (ctx *testContext) getNumMessagesInQueue(queueName string) int { ctx.t.Helper() conn, err := amqp.Dial(ctx.rabbitURI) From ba7a909f0bd32e34f788b28d4b7165a4bd2cf280 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 10:29:38 +0100 Subject: [PATCH 25/31] rename declare* functions to ensure* to clarify they are idempotent --- internal/queue/consumer.go | 4 ++-- internal/queue/deadletter.go | 4 ++-- internal/queue/producer.go | 2 +- internal/queue/types.go | 9 ++++++--- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index 51a1be87..ac229255 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -116,12 +116,12 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } - err = c.client.declareExchange(c.config.ExchangeName) + err = c.client.ensureExchange(c.config.ExchangeName) if err != nil { return err } - err = c.client.declareQueueWithDeadLetter(c.config.QueueName, c.config.DeadLetterExchange) + err = c.client.ensureQueueWithDeadLetter(c.config.QueueName, c.config.DeadLetterExchange) if err != nil { return err } diff --git a/internal/queue/deadletter.go b/internal/queue/deadletter.go index 6038b89b..2dc06cfc 100644 --- a/internal/queue/deadletter.go +++ b/internal/queue/deadletter.go @@ -10,12 +10,12 @@ import "fmt" // setupDeadLetterQueue declares the dead-letter exchange and queue, and binds them together. // This encapsulates the RabbitMQ-specific dead-letter configuration. func (c *Client) setupDeadLetterQueue(dlxName, dlqName, routingKey string) error { - err := c.declareExchange(dlxName) + err := c.ensureExchange(dlxName) if err != nil { return fmt.Errorf("cannot declare dead-letter exchange: %w", err) } - err = c.declareQueue(dlqName) + err = c.ensureQueue(dlqName) if err != nil { return fmt.Errorf("cannot declare dead-letter queue: %w", err) } diff --git a/internal/queue/producer.go b/internal/queue/producer.go index cfdda8c8..536ae43d 100644 --- a/internal/queue/producer.go +++ b/internal/queue/producer.go @@ -45,7 +45,7 @@ func (p *AmqpProducer) resetConnectionOrChannelIfNecessary() error { return err } - err = p.client.declareExchange(p.config.ExchangeName) + err = p.client.ensureExchange(p.config.ExchangeName) if err != nil { return err } diff --git a/internal/queue/types.go b/internal/queue/types.go index c543ac1a..c4bcc403 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -54,7 +54,8 @@ func (c *Client) resetChannel() error { return nil } -func (c *Client) declareExchange(exchangeName string) error { +// ensureExchange declares an exchange with given name - this is an idempotent operation +func (c *Client) ensureExchange(exchangeName string) error { err := c.amqpChannel.ExchangeDeclare( exchangeName, "direct", // use direct exchange @@ -70,7 +71,8 @@ func (c *Client) declareExchange(exchangeName string) error { return nil } -func (c *Client) declareQueue(queueName string) error { +// ensureQueue declares a queue with given name - this is an idempotent operation +func (c *Client) ensureQueue(queueName string) error { _, err := c.amqpChannel.QueueDeclare( queueName, true, // durable @@ -85,7 +87,8 @@ func (c *Client) declareQueue(queueName string) error { return nil } -func (c *Client) declareQueueWithDeadLetter(queueName, dlxName string) error { +// ensureQueueWithDeadLetter declares a queue with given name and dead-letter exchange specified - this is an idempotent operation +func (c *Client) ensureQueueWithDeadLetter(queueName, dlxName string) error { args := amqp.Table{ "x-dead-letter-exchange": dlxName, } From a74ffb00761fcdedbf91f41ae3de5a16c4410713 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 12:15:13 +0100 Subject: [PATCH 26/31] rename other functions to ensure* to clarify they are idempotent --- internal/queue/consumer.go | 4 ++-- internal/queue/deadletter.go | 8 ++++---- internal/queue/deadletter_test.go | 12 ++++++------ internal/queue/types.go | 3 ++- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/internal/queue/consumer.go b/internal/queue/consumer.go index ac229255..3eb5ed2d 100644 --- a/internal/queue/consumer.go +++ b/internal/queue/consumer.go @@ -111,7 +111,7 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } - err = c.client.setupDeadLetterQueue(c.config.DeadLetterExchange, c.config.DeadLetterQueue, c.config.RoutingKey) + err = c.client.ensureDeadLetterQueue(c.config.DeadLetterExchange, c.config.DeadLetterQueue, c.config.RoutingKey) if err != nil { return err } @@ -126,7 +126,7 @@ func (c *AmqpConsumer) ensureAmqpChannel() error { return err } - err = c.client.bindQueue(c.config.QueueName, c.config.RoutingKey, c.config.ExchangeName) + err = c.client.ensureQueueBinding(c.config.QueueName, c.config.RoutingKey, c.config.ExchangeName) if err != nil { return err } diff --git a/internal/queue/deadletter.go b/internal/queue/deadletter.go index 2dc06cfc..66fcddf6 100644 --- a/internal/queue/deadletter.go +++ b/internal/queue/deadletter.go @@ -7,9 +7,9 @@ package queue import "fmt" -// setupDeadLetterQueue declares the dead-letter exchange and queue, and binds them together. -// This encapsulates the RabbitMQ-specific dead-letter configuration. -func (c *Client) setupDeadLetterQueue(dlxName, dlqName, routingKey string) error { +// ensureDeadLetterQueue declares the dead-letter exchange and queue, and binds them together. +// This is an idempotent operation that encapsulates the RabbitMQ-specific dead-letter configuration. +func (c *Client) ensureDeadLetterQueue(dlxName, dlqName, routingKey string) error { err := c.ensureExchange(dlxName) if err != nil { return fmt.Errorf("cannot declare dead-letter exchange: %w", err) @@ -20,7 +20,7 @@ func (c *Client) setupDeadLetterQueue(dlxName, dlqName, routingKey string) error return fmt.Errorf("cannot declare dead-letter queue: %w", err) } - err = c.bindQueue(dlqName, routingKey, dlxName) + err = c.ensureQueueBinding(dlqName, routingKey, dlxName) if err != nil { return fmt.Errorf("cannot bind dead-letter queue: %w", err) } diff --git a/internal/queue/deadletter_test.go b/internal/queue/deadletter_test.go index 9679f6ed..14eaeb1b 100644 --- a/internal/queue/deadletter_test.go +++ b/internal/queue/deadletter_test.go @@ -11,16 +11,16 @@ import ( "github.com/stretchr/testify/assert" ) -func TestSetupDeadLetterQueue(t *testing.T) { +func TestEnsureDeadLetterQueue(t *testing.T) { /* arrange: Create a client with mock channel. - act: Call setupDeadLetterQueue. + act: Call ensureDeadLetterQueue. assert: DLX exchange declared, DLQ queue declared, and queue bound to exchange. */ ch := &MockAmqpChannel{} client := &Client{amqpChannel: ch} - err := client.setupDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") + err := client.ensureDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") assert.NoError(t, err) assert.Contains(t, ch.exchangeNames, "test-dlx", "DLX exchange should be declared") @@ -30,10 +30,10 @@ func TestSetupDeadLetterQueue(t *testing.T) { assert.Equal(t, "test-routing-key", ch.boundRoutingKey) } -func TestSetupDeadLetterQueueErrors(t *testing.T) { +func TestEnsureDeadLetterQueueErrors(t *testing.T) { /* arrange: Create a client with mock channel configured for each error scenario. - act: Call setupDeadLetterQueue. + act: Call ensureDeadLetterQueue. assert: Appropriate error returned. */ tests := []struct { @@ -69,7 +69,7 @@ func TestSetupDeadLetterQueueErrors(t *testing.T) { } client := &Client{amqpChannel: ch} - err := client.setupDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") + err := client.ensureDeadLetterQueue("test-dlx", "test-dlq", "test-routing-key") assert.Error(t, err) assert.ErrorContains(t, err, tt.expectErrContains) diff --git a/internal/queue/types.go b/internal/queue/types.go index c4bcc403..cbf5e845 100644 --- a/internal/queue/types.go +++ b/internal/queue/types.go @@ -106,7 +106,8 @@ func (c *Client) ensureQueueWithDeadLetter(queueName, dlxName string) error { return nil } -func (c *Client) bindQueue(queueName, routingKey, exchangeName string) error { +// ensureQueueBinding binds a queue to an exchange with the given routing key - this is an idempotent operation. +func (c *Client) ensureQueueBinding(queueName, routingKey, exchangeName string) error { err := c.amqpChannel.QueueBind( queueName, routingKey, From 80efcf8cabf7d76874422212234c4723966e7894 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 12:37:44 +0100 Subject: [PATCH 27/31] refactor: introduce getWorkflowJob for more obvious semantics --- internal/planner/consumer.go | 114 +++++++++++++----------------- internal/planner/consumer_test.go | 56 +++++++++------ 2 files changed, 85 insertions(+), 85 deletions(-) diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index 08877630..23cdb678 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -32,9 +32,6 @@ const ( maxBackoff = 5 * time.Minute ) -// ErrUnsupportedEvent indicates a valid but unsupported webhook event that should be ignored. -var ErrUnsupportedEvent = errors.New("unsupported event") - func NewJobConsumer(consumer queue.Consumer, db JobDatabase, metrics *Metrics) *JobConsumer { return &JobConsumer{ consumer: consumer, @@ -63,100 +60,103 @@ func parseOptionalGitHubTime(ts *github.Timestamp) *time.Time { return &t } -func parseGithubWebhookPayload(ctx context.Context, headers map[string]interface{}, body []byte) (githubWebhookJob, error) { - // Extract the event type from headers +// getWorkflowJob extracts and validates a workflow job from a webhook message. +// Returns nil for events that should be ignored (non-workflow_job, unsupported actions, non-self-hosted). +// Returns an error only for malformed data that cannot be processed. +func getWorkflowJob(ctx context.Context, headers map[string]interface{}, body []byte) (*githubWebhookJob, error) { eventTypeHeader, ok := headers[githubEventHeaderKey] if !ok { - logger.WarnContext(ctx, "missing X-GitHub-Event header, discarding message") - return githubWebhookJob{}, fmt.Errorf("missing X-GitHub-Event header") + return nil, fmt.Errorf("missing X-GitHub-Event header") } eventType, ok := eventTypeHeader.(string) if !ok { - logger.WarnContext(ctx, "X-GitHub-Event header is not a string, discarding message") - return githubWebhookJob{}, fmt.Errorf("X-GitHub-Event must be string") + return nil, fmt.Errorf("X-GitHub-Event must be string") } - // Parse the webhook using go-github library event, err := github.ParseWebHook(eventType, body) if err != nil { - return githubWebhookJob{}, fmt.Errorf("unable to parse webhook: %w", err) + return nil, fmt.Errorf("unable to parse webhook: %w", err) } - // Check if it's a workflow_job event jobEvent, ok := event.(*github.WorkflowJobEvent) if !ok { - return githubWebhookJob{}, fmt.Errorf("%w: %s", ErrUnsupportedEvent, eventType) + logger.DebugContext(ctx, "ignoring non-workflow_job event", "event_type", eventType) + return nil, nil } - // Process the workflow job event based on action action := jobEvent.GetAction() - if !slices.Contains([]string{"queued", "in_progress", "completed", "waiting"}, action) { - return githubWebhookJob{}, fmt.Errorf("%w: unknown action %s", ErrUnsupportedEvent, action) + + if !slices.Contains([]string{"queued", "in_progress", "completed"}, action) { + logger.DebugContext(ctx, "ignoring workflow_job action", "action", action) + return nil, nil } job := jobEvent.GetWorkflowJob() if job == nil { - return githubWebhookJob{}, fmt.Errorf("missing workflow_job field in event webhook") + return nil, fmt.Errorf("missing workflow_job field in event webhook") + } + + if !isSelfHosted(job.Labels) { + repo := jobEvent.GetRepo().GetFullName() + logger.DebugContext(ctx, "ignoring non self-hosted job", "repo", repo, "labels", strings.Join(job.Labels, ",")) + return nil, nil } createdAt := job.GetCreatedAt() if createdAt.IsZero() { - return githubWebhookJob{}, fmt.Errorf("missing created_at in queued event webhook") + return nil, fmt.Errorf("missing created_at in queued event webhook") } startedAt := job.GetStartedAt() if startedAt.IsZero() && action == "in_progress" { - return githubWebhookJob{}, fmt.Errorf("missing started_at in in_progress event webhook") + return nil, fmt.Errorf("missing started_at in in_progress event webhook") } completedAt := job.GetCompletedAt() if completedAt.IsZero() && action == "completed" { - return githubWebhookJob{}, fmt.Errorf("missing completed_at in completed event webhook") + return nil, fmt.Errorf("missing completed_at in completed event webhook") } id := strconv.FormatInt(job.GetID(), 10) if id == "0" { - return githubWebhookJob{}, fmt.Errorf("missing job id in event webhook") + return nil, fmt.Errorf("missing job id in event webhook") } var rawJson map[string]interface{} err = json.Unmarshal(body, &rawJson) if err != nil { - return githubWebhookJob{}, fmt.Errorf("invalid github webhook: %v", err) - } - - dbJob := &database.Job{ - ID: id, - Platform: platform, - Labels: job.Labels, - CreatedAt: createdAt.Time, - StartedAt: parseOptionalGitHubTime(job.StartedAt), - CompletedAt: parseOptionalGitHubTime(job.CompletedAt), - Raw: map[string]interface{}{action: rawJson}, + return nil, fmt.Errorf("invalid github webhook: %v", err) } repo := jobEvent.GetRepo().GetFullName() - if repo == "" { - return githubWebhookJob{}, fmt.Errorf("missing repo full name in webhook payload") + return nil, fmt.Errorf("missing repo full name in webhook payload") } - return githubWebhookJob{ - id: id, - repo: jobEvent.GetRepo().GetFullName(), - labels: job.Labels, - action: action, - + webhookJob := &githubWebhookJob{ + id: id, + repo: repo, + labels: job.Labels, + action: action, payload: jobEvent, - - job: dbJob, - }, nil + job: &database.Job{ + ID: id, + Platform: platform, + Labels: job.Labels, + CreatedAt: createdAt.Time, + StartedAt: parseOptionalGitHubTime(job.StartedAt), + CompletedAt: parseOptionalGitHubTime(job.CompletedAt), + Raw: map[string]interface{}{action: rawJson}, + }, + } + + return webhookJob, nil } -// isSelfHosted checks if the job is intended for self-hosted runners. -func isSelfHosted(job *githubWebhookJob) bool { - for _, label := range job.labels { +// isSelfHosted checks if the job labels indicate it's for self-hosted runners. +func isSelfHosted(labels []string) bool { + for _, label := range labels { if strings.Contains(label, "self-hosted") { return true } @@ -226,36 +226,22 @@ func (c *JobConsumer) pullMessage(ctx context.Context) error { ctx, span := trace.Start(ctx, "consume webhook") defer span.End() - job, err := parseGithubWebhookPayload(ctx, msg.Headers, msg.Body) + job, err := getWorkflowJob(ctx, msg.Headers, msg.Body) if err != nil { - if errors.Is(err, ErrUnsupportedEvent) { - logger.DebugContext(ctx, "ignoring unsupported event", "error", err) - c.metrics.ObserveDiscardedWebhook(ctx, platform) - c.ignoreMessage(ctx, &msg) - return nil - } - logger.ErrorContext(ctx, "cannot parse webhook payload", "error", err) + logger.ErrorContext(ctx, "cannot parse webhook payload, discarding to DLQ", "error", err) span.RecordError(err) c.metrics.ObserveWebhookError(ctx, platform) c.discardMessage(ctx, &msg) return err } - if !slices.Contains([]string{"queued", "in_progress", "completed"}, job.action) { - logger.DebugContext(ctx, "ignore other action types for GitHub webhook job", "action", job.action) - c.metrics.ObserveDiscardedWebhook(ctx, platform) - c.ignoreMessage(ctx, &msg) - return nil - } - - if !isSelfHosted(&job) { - logger.DebugContext(ctx, "ignore non self-hosted job", "repo", job.repo, "labels", strings.Join(job.labels, ",")) + if job == nil { c.metrics.ObserveDiscardedWebhook(ctx, platform) c.ignoreMessage(ctx, &msg) return nil } - err = c.handleMessage(ctx, &job) + err = c.handleMessage(ctx, job) if err == nil { c.consumedMessage(ctx, &msg) c.metrics.ObserveConsumedGitHubWebhook(ctx, job.payload) diff --git a/internal/planner/consumer_test.go b/internal/planner/consumer_test.go index 8ab2ee11..f85778f6 100644 --- a/internal/planner/consumer_test.go +++ b/internal/planner/consumer_test.go @@ -602,11 +602,11 @@ func TestConsumer(t *testing.T) { } } -func TestParseGithubWebhookPayload(t *testing.T) { +func TestGetWorkflowJob(t *testing.T) { /* arrange: setup context for parsing webhook payload - act: parse the GitHub webhook payload with provided headers and body - assert: check for expected errors and validate parsed job data + act: extract workflow job from GitHub webhook payload + assert: check for expected errors, nil returns, and validate parsed job data */ mk := func(m map[string]any) []byte { b, _ := json.Marshal(m); return b } @@ -616,7 +616,8 @@ func TestParseGithubWebhookPayload(t *testing.T) { body []byte expectErr bool expectErrMsg string - validateJob func(*testing.T, githubWebhookJob) + expectNil bool + validateJob func(*testing.T, *githubWebhookJob) }{{ name: "successful parsing of queued event", headers: map[string]interface{}{ @@ -634,8 +635,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { "created_at": "2025-01-01T10:00:00Z", }, }), - expectErr: false, - validateJob: func(t *testing.T, job githubWebhookJob) { + validateJob: func(t *testing.T, job *githubWebhookJob) { assert.Equal(t, "100", job.id) assert.Equal(t, "canonical/test-repo", job.repo) assert.Equal(t, "queued", job.action) @@ -670,8 +670,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { "id": 1, }, }), - expectErr: true, - expectErrMsg: "unsupported event", + expectNil: true, }, { name: "unknown action", headers: map[string]interface{}{ @@ -688,8 +687,24 @@ func TestParseGithubWebhookPayload(t *testing.T) { "created_at": "2025-01-01T10:00:00Z", }, }), - expectErr: true, - expectErrMsg: "unsupported event: unknown action", + expectNil: true, + }, { + name: "non-self-hosted job", + headers: map[string]interface{}{ + "X-GitHub-Event": "workflow_job", + }, + body: mk(map[string]any{ + "action": "queued", + "repository": map[string]any{ + "full_name": "canonical/test", + }, + "workflow_job": map[string]any{ + "id": 250, + "labels": []string{"ubuntu-latest"}, // Not self-hosted + "created_at": "2025-01-01T10:00:00Z", + }, + }), + expectNil: true, }, { name: "missing workflow_job field", headers: map[string]interface{}{ @@ -798,7 +813,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { expectErr: true, expectErrMsg: "missing repo full name", }, { - name: "waiting action is accepted", + name: "waiting action is ignored", headers: map[string]interface{}{ "X-GitHub-Event": "workflow_job", }, @@ -813,11 +828,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { "created_at": "2025-01-01T10:00:00Z", }, }), - expectErr: false, - validateJob: func(t *testing.T, job githubWebhookJob) { - assert.Equal(t, "700", job.id) - assert.Equal(t, "waiting", job.action) - }, + expectNil: true, }, { name: "in_progress with all fields", headers: map[string]interface{}{ @@ -836,8 +847,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { "started_at": "2025-01-01T10:05:00Z", }, }), - expectErr: false, - validateJob: func(t *testing.T, job githubWebhookJob) { + validateJob: func(t *testing.T, job *githubWebhookJob) { assert.Equal(t, "800", job.id) assert.Equal(t, "in_progress", job.action) assert.NotNil(t, job.job.StartedAt) @@ -862,8 +872,7 @@ func TestParseGithubWebhookPayload(t *testing.T) { "completed_at": "2025-01-01T10:15:00Z", }, }), - expectErr: false, - validateJob: func(t *testing.T, job githubWebhookJob) { + validateJob: func(t *testing.T, job *githubWebhookJob) { assert.Equal(t, "900", job.id) assert.Equal(t, "completed", job.action) assert.NotNil(t, job.job.CompletedAt) @@ -882,15 +891,20 @@ func TestParseGithubWebhookPayload(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - job, err := parseGithubWebhookPayload(ctx, tt.headers, tt.body) + job, err := getWorkflowJob(ctx, tt.headers, tt.body) if tt.expectErr { assert.Error(t, err) if tt.expectErrMsg != "" { assert.Contains(t, err.Error(), tt.expectErrMsg) } + assert.Nil(t, job) + } else if tt.expectNil { + assert.NoError(t, err) + assert.Nil(t, job) } else { assert.NoError(t, err) + assert.NotNil(t, job) if tt.validateJob != nil { tt.validateJob(t, job) } From 6f2350c226103d4bfa39ce16ab0a7bdbb99ea24c Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 12:45:15 +0100 Subject: [PATCH 28/31] add paranoia check --- internal/planner/consumer.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index 23cdb678..79232c31 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -81,7 +81,11 @@ func getWorkflowJob(ctx context.Context, headers map[string]interface{}, body [] jobEvent, ok := event.(*github.WorkflowJobEvent) if !ok { - logger.DebugContext(ctx, "ignoring non-workflow_job event", "event_type", eventType) + if eventType == "workflow_job" { + logger.WarnContext(ctx, "workflow_job event did not parse to expected type", "event_type", eventType) + } else { + logger.DebugContext(ctx, "ignoring non-workflow_job event", "event_type", eventType) + } return nil, nil } From 9fc75d257c95b46d94118ddbeea2c718ead0c1d6 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Fri, 9 Jan 2026 13:59:56 +0100 Subject: [PATCH 29/31] fix ci --- cmd/planner/main_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index dbaf36aa..98e1bb4e 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -105,8 +105,8 @@ func TestMain_IntegrationScenarios(t *testing.T) { dlqDepth := ctx.getNumMessagesInQueue(config.DeadLetterQueue) assert.Equal(t, 0, dlqDepth) - // Malformed message should go to DLQ - malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999}}`) + // Malformed message should go to DLQ (must have self-hosted label to pass filter) + malformedBody := []byte(`{"action": "queued", "workflow_job": {"id": 999, "labels": ["self-hosted"]}}`) ctx.publish(malformedBody, supportedEventType) dlqMessage := ctx.consumeFromQueue(config.DeadLetterQueue, 10*time.Second) From 716de1e67a273e0d77f56e0c6b7fe65554faddec Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 12 Jan 2026 07:41:26 +0100 Subject: [PATCH 30/31] improve log warning --- internal/planner/consumer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/planner/consumer.go b/internal/planner/consumer.go index 79232c31..4e80ac53 100644 --- a/internal/planner/consumer.go +++ b/internal/planner/consumer.go @@ -82,7 +82,7 @@ func getWorkflowJob(ctx context.Context, headers map[string]interface{}, body [] jobEvent, ok := event.(*github.WorkflowJobEvent) if !ok { if eventType == "workflow_job" { - logger.WarnContext(ctx, "workflow_job event did not parse to expected type", "event_type", eventType) + logger.WarnContext(ctx, "received workflow_job in \"X-GitHub-Event\" header but payload did not parse to expected type; possible GitHub API change or library issue") } else { logger.DebugContext(ctx, "ignoring non-workflow_job event", "event_type", eventType) } From d5aeae25cc92eb4874e59e74e9a0e4baf3c3ccab Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Mon, 12 Jan 2026 11:51:09 +0100 Subject: [PATCH 31/31] wait for queue to avoid race condition where queue is not existent --- cmd/planner/main_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/cmd/planner/main_test.go b/cmd/planner/main_test.go index 19d25ee6..63eeb8e6 100644 --- a/cmd/planner/main_test.go +++ b/cmd/planner/main_test.go @@ -55,6 +55,7 @@ func TestMain_IntegrationScenarios(t *testing.T) { go main() ctx.waitForHTTP("http://localhost:"+ctx.port+"/health", 10*time.Second) + ctx.waitForQueue(queue.DefaultQueueConfig().QueueName, 10*time.Second) // Scenario 1: Flavor pressure updates t.Run("flavor_pressure", func(t *testing.T) { @@ -319,6 +320,33 @@ func (ctx *testContext) consumeFromQueue(queueName string, timeout time.Duration } } +// waitForQueue waits until the specified queue exists in RabbitMQ. +func (ctx *testContext) waitForQueue(queueName string, timeout time.Duration) { + ctx.t.Helper() + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + conn, err := amqp.Dial(ctx.rabbitURI) + if err != nil { + time.Sleep(100 * time.Millisecond) + continue + } + ch, err := conn.Channel() + if err != nil { + conn.Close() + time.Sleep(100 * time.Millisecond) + continue + } + _, err = ch.QueueDeclarePassive(queueName, true, false, false, false, nil) + ch.Close() + conn.Close() + if err == nil { + return + } + time.Sleep(100 * time.Millisecond) + } + require.Fail(ctx.t, "queue did not appear", "queue %s did not exist within %s", queueName, timeout) +} + // getNumMessagesInQueue returns the current number of messages in the specified queue. func (ctx *testContext) getNumMessagesInQueue(queueName string) int { ctx.t.Helper()