Skip to content

Raise when sendmsg fails for every resolved address#23

Merged
oskgu360 merged 1 commit into
mainfrom
raise-on-all-addrs-fail
May 6, 2026
Merged

Raise when sendmsg fails for every resolved address#23
oskgu360 merged 1 commit into
mainfrom
raise-on-all-addrs-fail

Conversation

@oskgu360
Copy link
Copy Markdown
Contributor

@oskgu360 oskgu360 commented May 6, 2026

Summary

  • Drop the per-packet warn \"Sparoid error: ...\" in sendmsg — hosts with both v4 and v6 addresses on v4-only networks were spamming No route to host even when the other path worked.
  • auth now rescues SystemCallError per address, collects the failed-addr error messages, and raises Sparoid::Error only when every address failed.
  • auth returns the list of successful addresses so fdpass doesn't try unreachable peers.

Test plan

  • bundle exec rake test (23 runs, 42 assertions, 0 failures)
  • bundle exec rubocop (clean)
  • New tests cover the all-fail (raises) and partial-fail (returns successful addrs, no stderr noise) cases

Previously sendmsg warned on each failed UDP send and auth always
returned the full address list. Hosts with both IPv4 and IPv6 records
on networks lacking v6 routing produced noisy 'No route to host'
warnings even when the v4 path succeeded.

Drop the per-packet warn, rescue SystemCallError per address in auth,
and raise Sparoid::Error only when no address accepted the send.
Return only the successful addresses so fdpass skips unreachable
peers.
@oskgu360 oskgu360 marked this pull request as ready for review May 6, 2026 08:29
@oskgu360 oskgu360 requested a review from a team as a code owner May 6, 2026 08:29
@oskgu360
Copy link
Copy Markdown
Contributor Author

oskgu360 commented May 6, 2026

Lazy me didn't wanna go the Bring your own logger path. Just silence the error logs for now :) Tested locally with ssh-monitor

 bundle exec rake dev
bundle exec guard --plugin=foreman --no-interactions --notify false
10:29:08 - INFO - Foreman started.
10:29:08 - INFO - Guard is now watching at '/Users/oskarwestberg/84codes/ssh-monitor'
10:29:08 worker.1 | started with pid 47481
10:29:09 worker.1 | at=info known_hosts written: 139 bytes
10:29:10 worker.1 | at=info HEROKU_API_KEY not set, assuming 1 dyno formation
10:29:10 worker.1 | at=info Test cluster, not posting to PagerDuty: dev-crisp-pink-snake-slow-maintenance Maintenance on dev-crisp-pink-snake is running outside its window
10:29:10 worker.1 | at=info servers#count=2
10:29:10 worker.1 | at=info thread#count=46 alive#count=46 working#count=1 queue#length=1 servers#offline=0
10:29:10 worker.1 | at=info DB store listening to servers.update, servers.insert, clusters.update, accounts.update
10:29:30 worker.1 | at=info servers#count=2

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts SPA packet sending behavior to avoid noisy per-packet warnings on partial connectivity (e.g., dual-stack hosts on v4-only networks), and instead only raises an error when all resolved destination addresses fail. It also returns only the addresses that successfully received packets so downstream connection logic can skip unreachable peers.

Changes:

  • Update Sparoid.auth to rescue SystemCallError per resolved address, collecting failures and raising Sparoid::Error only if every address fails.
  • Change Sparoid.auth to return only the subset of addresses that were successfully sent to.
  • Remove sendmsg per-packet warning behavior and make socket cleanup resilient when socket creation fails.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/sparoid.rb Implements per-address failure handling in auth, returns only successful addrs, and removes sendmsg warning behavior while hardening socket close.
test/sparoid_test.rb Adds tests for “all addresses fail => raises” and “partial failure => returns only successful addrs with no stderr noise”.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Contributor

@walro walro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@oskgu360 oskgu360 merged commit f3c72e4 into main May 6, 2026
12 checks passed
@oskgu360 oskgu360 deleted the raise-on-all-addrs-fail branch May 6, 2026 10:50
@oskgu360 oskgu360 mentioned this pull request May 6, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants