Skip to content

Use long polling in sqs CLI find_all so dump/mv don't stop early#1032

Open
mensfeld wants to merge 3 commits into
mainfrom
fix/b8-cli-find-all-short-polling
Open

Use long polling in sqs CLI find_all so dump/mv don't stop early#1032
mensfeld wants to merge 3 commits into
mainfrom
fix/b8-cli-find-all-short-polling

Conversation

@mensfeld

@mensfeld mensfeld commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

find_all (used by sqs dump and sqs mv, both defaulting --number to Float::INFINITY) terminated on the first empty receive_message response. With the default short polling, SQS samples only a subset of its hosts and routinely returns an empty batch while the queue still has messages, so on real (distributed) SQS dump/mv quietly processed only a fraction of the queue (invisible on single-node LocalStack).

Long poll (wait_time_seconds) and only stop after several consecutive empty batches, so the queue is actually drained.

(No spec: exercising bin/cli in-process defines Shoryuken::CLI, and Shoryuken#server? is defined?(Shoryuken::CLI), which would flip server? globally and break other specs - same constraint as the CLI fix in the message_size PR.)

Summary by CodeRabbit

  • Bug Fixes

    • Fixed queue-draining behavior for sqs dump and mv so they continue through intermittent empty SQS responses and finish only after the queue is truly drained.
    • Improved reliability when working with queues that can briefly appear empty even though messages remain.
  • Tests

    • Added coverage for draining queues with a false-empty batch to ensure all messages are collected.

find_all (used by sqs dump and sqs mv, both defaulting --number to
Float::INFINITY) terminated on the first empty receive_message response.
With the default short polling, SQS samples only a subset of its hosts and
routinely returns an empty batch while the queue still has messages, so on
real (distributed) SQS dump/mv quietly processed only a fraction of the
queue (invisible on single-node LocalStack).

Long poll (wait_time_seconds) and only stop after several consecutive
empty batches, so the queue is actually drained.

(No spec: exercising bin/cli in-process defines Shoryuken::CLI, and
Shoryuken#server? is defined?(Shoryuken::CLI), which would flip server?
globally and break other specs - same constraint as the CLI fix in the
message_size PR.)
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3506249e-f57b-4d0a-931d-9ed6c98c694f

📥 Commits

Reviewing files that changed from the base of the PR and between 37acaf8 and d7ce4fa.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • bin/cli/sqs.rb
  • spec/integration/cli/find_all_spec.rb

📝 Walkthrough

Walkthrough

find_all in bin/cli/sqs.rb is updated to use SQS long polling (wait_time_seconds) and to tolerate false-empty batches by counting consecutive empty responses and only terminating after FIND_ALL_MAX_EMPTY_BATCHES. An integration spec and changelog entry accompany the change.

Changes

find_all long-polling drain fix

Layer / File(s) Summary
Long-polling constants and find_all loop logic
bin/cli/sqs.rb
Adds FIND_ALL_WAIT_SECONDS = 1 and FIND_ALL_MAX_EMPTY_BATCHES = 3 constants, initializes an empty_batches counter, passes wait_time_seconds to receive_message, and replaces break-on-first-empty with break-after-N-consecutive-empty logic.
Integration spec
spec/integration/cli/find_all_spec.rb
Introduces ScriptedSqsClient with a mid-stream false-empty batch, injects it into CLI::SQS, and asserts all messages are collected and all wait_time_seconds values are positive.
Changelog
CHANGELOG.md
Adds an [Unreleased] entry documenting the fix for premature drain termination.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐇 Hoppin' through the queue with care,
No more stopping when the batch looks bare!
I count the empties—one, two, three—
Then declare the queue is finally free.
Long-polling now, no tricks from SQS!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: switching SQS CLI find_all to long polling so dump/mv do not stop early.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/b8-cli-find-all-short-polling

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mensfeld mensfeld self-assigned this Jun 30, 2026
Integration spec (separate process, so loading bin/cli doesn't flip
Shoryuken#server? for the unit suite) drives CLI::SQS#find_all with a
scripted client containing a false-empty batch, and asserts it drains
everything and long-polls - deterministic, since ElasticMQ never produces
the false-empties this guards against. Plus a CHANGELOG entry.
@mensfeld mensfeld marked this pull request as ready for review June 30, 2026 15:05
@mensfeld mensfeld added the bug label Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant