feat(Shovels): implement retry + backoff in Shovel destinations #1215

lukas8219 · 2025-08-18T21:47:32Z

WHAT is this pull request doing?

Implements Retrier in HTTPDestination Shovel - configured by parameters

dest-backoff
dest-jitter
dest-timeout
dest-max-retries

Fixes HTTPDestination not taking no_ack mode into consideration - as all HTTPDestinations were defaulted as OnConfirm/OnPublish.

HOW can this pull request be tested?

Specs:

Expose mock server to receive HTTP requests. Fail at least 2 times and accept third time.
Tests focused on the Retrier logic

RFC

Should we actually implement a proper Circuit Breaker instead of just Retrier?
Currently using ?.try is misleading. Is there a better way to handle it?

Closes #1137

lukas8219 · 2025-08-19T12:12:28Z

src/lavinmq/shovel/shovel.cr

+      private def push_with_retry(path, headers, body, max_retries = 3, jitter = 0.5)
+        retries = 0
+        while retries < max_retries
+          response = @client.not_nil!.post(path, headers: headers, body: body)
+          return true if response.success?
+          retries += 1
+          # Exponential backoff: 2^retries seconds with jitter
+          base_delay = (2.0 ** retries).seconds
+          jitter_delay = jitter.seconds * Random.rand(0.0..1.0)
+          sleep base_delay + jitter_delay
+        end
+        false
+      end


Ideally I think this could be a macro? trait? that receives the max_retries and jitter and executes a &block

yes, like this:

Suggested change

private def push_with_retry(path, headers, body, max_retries = 3, jitter = 0.5)

retries = 0

while retries < max_retries

response = @client.not_nil!.post(path, headers: headers, body: body)

return true if response.success?

retries += 1

# Exponential backoff: 2^retries seconds with jitter

base_delay = (2.0 ** retries).seconds

jitter_delay = jitter.seconds * Random.rand(0.0..1.0)

sleep base_delay + jitter_delay

end

false

end

private def push_with_retry(path, headers, body, max_retries = 3, jitter = 0.5.seconds, &) : Bool

retries = 0

while retries < max_retries

return true if yield

retries += 1

# Exponential backoff: 2^retries seconds with jitter

base_delay = (2.0 ** retries).seconds

jitter_delay = jitter * rand(1.0)

sleep base_delay + jitter_delay

end

false

end

kickster97 · 2025-11-11T16:09:32Z

Have not looked closely at the code, but does this adress issue #1357 aswell?

lukas8219 · 2025-11-12T16:06:38Z

@kickster97 Not related. It was meant for only the HTTP retries

carlhoerberg

Looks like this class needs a lot of rework, for instance, what's up with @client not being assigned in the constructor? What would allow us to get rid of the not_nil! anti-pattern.

snichme · 2025-11-26T14:53:51Z

src/lavinmq/shovel/shovel.cr

                 "/"
               end
-        response = c.post(path, headers: headers, body: msg.body_io)
+        success = push_with_retry(path, headers, msg.body_io, max_retries: 3, jitter: 0.5)


push_with_retry makes sense for AckMode::OnConfirm and AckMode::OnPublish but NoAck is more like "fire-and-forget" so there we don't care about the results.

What I mean is that we should only retry on OnConfirm and OnPublish

lukas8219 · 2025-12-06T20:21:11Z

spec/shovel_retrier_spec.cr

+require "./spec_helper"
+require "../src/lavinmq/shovel/shovel_retrier"
+
+describe LavinMQ::Shovel::Retrier do


It got me wondering wether Retrier shouldn't just be some sort of Circuit Breaker...

Could you explain some more what you are thinking here? :)

@snichme If we are doing HTTP Retries, there's a high possibility that blindly retrying is not the best approach. What if the user has N HTTP shovels, and their downstreams start failing?

This behavior of blindly retrying could just make the situation worse...And it's not a good UX to simply have the user deleting/pausing shovels one by one? (on the other hand for that specific scenario, we could make it simple as having a Pause All sorta button)

Aha agree.
I like it. I would prefer to have a checkbox next to each shovel, so you can select some of them and have batch operations on the selected ones, like Pause, Delete etc.
Like we have with Queues and Streams.

But this should be another PR IMO. so this wont run away in size or time

src/lavinmq/shovel/shovel_retrier.cr

lukas8219 · 2025-12-06T20:27:07Z

src/lavinmq/shovel/shovel_store.cr

+            name,
+            uri,
+            Shovel::HTTPDestinationParameters.from_parameters(config),
+            ack_mode


⚠️ 🔥 This might be a breaking change for some customers. I need to check deployed LMQ definitions and adding in release notes a comment about this. ACK mode was not being considered in HTTPDest

lukas8219 · 2025-12-06T20:30:46Z

src/lavinmq/shovel/shovel.cr

+          jitter: parameters["dest-jitter"]?.try &.as_f? || 1.0,
+          backoff: parameters["dest-backoff"]?.try &.as_f? || 2.0,
+          timeout: parameters["dest-timeout"]?.try &.as_f? || 10.0,
+          max_retries: parameters["dest-max-retries"]?.try &.as_i? || 1


💡 ⚠️ ?.try may fail to transform user input like 1.0 or 1 between Float and Integer. Not sure if ?.try is the best idea here.

Meaning, it could be misleading for the user if the Shovel Parameter accepts their input but behind the scenes, it uses the default values. Notice it via Spec

This is a reoccurring thing, how and when to validate input data and also handle it when it's invalid.
What you have here is common pattern that we try to parse the input and if it fails we use a default value, not saying it's the best since the shovel might end up being configured in a wrong way. But trying to find a common solution to all input is also difficult. @spuun did some refactoring on validating input for a queue here: https://github.com/cloudamqp/lavinmq/blob/main/src/lavinmq/amqp/queue/queue.cr#L27 might give some inspiration.

snichme · 2026-01-08T13:19:34Z

Draft PR but still something that will impact this PR if merged before: #1591 I can help rebase if needed.

lukas8219 · 2026-01-16T16:31:12Z

@snichme Ok. I am still waiting on that PR to be merged so I can push this further.

lukas8219 added 2 commits August 18, 2025 18:47

wip: naive implementation of push_with_retry

2d8c204

lint: removed c unused variable

986a277

lukas8219 changed the title ~~wip: naive implementation of push_with_retry~~ feat(Shovels): implement retry + backoff in Shovel destinations Aug 19, 2025

lukas8219 commented Aug 19, 2025

View reviewed changes

Merge branch 'cloudamqp:main' into 1137

2e69319

Merge branch 'cloudamqp:main' into 1137

318bd34

carlhoerberg approved these changes Nov 20, 2025

View reviewed changes

carlhoerberg marked this pull request as ready for review November 20, 2025 05:58

carlhoerberg requested a review from a team as a code owner November 20, 2025 05:58

snichme reviewed Nov 26, 2025

View reviewed changes

lukas8219 and others added 2 commits November 28, 2025 13:40

feat(Shovel::Retrier): added module only for retries

c611a52

Merge branch 'cloudamqp:main' into 1137

fbf7d85

lukas8219 marked this pull request as draft November 28, 2025 16:43

lukas8219 and others added 4 commits November 28, 2025 13:48

Fixed max_retries variable naming

8dc104f

Merge branch 'cloudamqp:main' into 1137

4734ce3

specs: added spec scenarios for shovel

235f2e3

added spec for shovel_retrier

dbaaf31

lukas8219 commented Dec 6, 2025

View reviewed changes

applied linting and formatting

be49fa6

lukas8219 commented Dec 6, 2025

View reviewed changes

src/lavinmq/shovel/shovel_retrier.cr Outdated Show resolved Hide resolved

lukas8219 commented Dec 6, 2025

View reviewed changes

adjustments: typing in some parameters and removes Either

ef20949

lukas8219 marked this pull request as ready for review December 6, 2025 20:41

lukas8219 requested a review from snichme December 6, 2025 20:42

kickster97 linked an issue Jan 19, 2026 that may be closed by this pull request

HTTP shovel/webhook acks and removes message on HTTP 404; message does not return to source queue; pausing shovel does not stop in-flight retries #1612

Open

feat(Shovels): implement retry + backoff in Shovel destinations #1215

Are you sure you want to change the base?

feat(Shovels): implement retry + backoff in Shovel destinations #1215

Conversation

lukas8219 commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHAT is this pull request doing?

HOW can this pull request be tested?

RFC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlhoerberg Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kickster97 commented Nov 11, 2025

Uh oh!

lukas8219 commented Nov 12, 2025

Uh oh!

carlhoerberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lukas8219 Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snichme commented Jan 8, 2026

Uh oh!

lukas8219 commented Jan 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukas8219 commented Aug 18, 2025 •

edited

Loading

carlhoerberg Nov 20, 2025 •

edited

Loading

lukas8219 Dec 6, 2025 •

edited

Loading