Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.markdown
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#### Unreleased
* ActionMailerCheck: Support :sendmail and :test
> awilfox: https://github.com/okcomputer-ruby/okcomputer/pull/21
* Add SolidQueue checks: `SolidQueueCheck` (liveness + job stats),
`SolidQueueBackedUpCheck` (per-queue backlog), `SolidQueueFailedJobsCheck`
(total failed jobs), `SolidQueueFailedJobsRateCheck` (rapid increase in
Expand Down
33 changes: 28 additions & 5 deletions lib/ok_computer/built_in_checks/action_mailer_check.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,34 @@ def initialize(klass = ActionMailer::Base, timeout = 5)

# Public: Return the status of the check
def check
tcp_socket_request
mark_message "#{klass} check to #{host}:#{port} successful"
rescue => e
mark_message "#{klass} at #{host}:#{port} is not accepting connections: '#{e}'"
mark_failure
case klass.delivery_method
when :smtp
begin
tcp_socket_request
mark_message "#{klass} check to #{host}:#{port} successful"
rescue => e
mark_message "#{klass} at #{host}:#{port} is not accepting connections: '#{e}'"
mark_failure
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The :sendmail branch has no exception handling, unlike :smtp which keeps the original begin/rescue. If sendmail_settings[:location] is nil (a realistic misconfiguration — e.g. an app sets sendmail_settings = { arguments: [...] } without :location), File.executable?(nil) raises a TypeError. I reproduced this live, and it propagates unrescued through Check#run and CheckCollection (both check_in_sequence and check_in_parallel, since Thread#join re-raises), crashing the health-check endpoint instead of reporting a graceful failure. Worth wrapping this in a begin/rescue like the :smtp branch.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Exceptions are handled in the latest revision.

when :sendmail
begin
location = klass.sendmail_settings[:location]
if File.executable?(location)
mark_message "#{klass} sendmail executable #{location} can be executed"
else
mark_message "#{klass} sendmail executable #{location} is not executable"
mark_failure
end
rescue => e
mark_message "#{klass} error checking sendmail executable: '#{e}'"
mark_failure
end
when :test
mark_message "#{klass} is in test mode"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The case klass.delivery_method has no else/default branch. Any delivery method other than :smtp, :sendmail, or :test (e.g. :file, or third-party adapters like Postmark/SES/Resend) falls through and silently reports success? == true with message == nil — verified this live. For a health-check gem, silently reporting "healthy" without actually checking anything seems like a real problem. Worth adding an else branch that fails (or explicitly documents unsupported delivery methods as unsupported) rather than defaulting to success.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed and changed in latest revision.

else
mark_message "unknown delivery method #{klass.delivery_method}"
mark_failure
end
end
end
end
31 changes: 31 additions & 0 deletions spec/ok_computer/built_in_checks/action_mailer_check_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class ActionMailerSubclass < ActionMailer::Base
describe '#check' do
context "when mailer is accepting connections" do
before do
ActionMailer::Base.delivery_method = :smtp
ActionMailer::Base.smtp_settings[:address] = 'localhost'
ActionMailer::Base.smtp_settings[:port] = 25
expect(TCPSocket).to receive(:new).with('localhost', 25).and_return(double(:socket, :close => true))
Expand All @@ -64,6 +65,7 @@ class ActionMailerSubclass < ActionMailer::Base
end

context "when mailer does not accept connection" do
before { ActionMailer::Base.delivery_method = :smtp }
let(:amcheck) { described_class.new(ActionMailerSubclass) }
it 'is not successful' do
expect(amcheck).to receive(:tcp_socket_request).and_raise(Errno::ECONNREFUSED)
Expand All @@ -74,6 +76,35 @@ class ActionMailerSubclass < ActionMailer::Base
expect(amcheck).to have_message "OkComputer::ActionMailerSubclass at mail.example.com:666 is not accepting connections: 'Connection refused'"
end
end

context "when mailer is in sendmail mode" do
before do
ActionMailer::Base.delivery_method = :sendmail
ActionMailer::Base.sendmail_settings[:location] = '/usr/sbin/sendmail'
expect(File).to receive(:executable?).with('/usr/sbin/sendmail').and_return(true)
end

it { is_expected.to be_successful_check }
it { is_expected.to have_message "ActionMailer::Base sendmail executable /usr/sbin/sendmail can be executed" }
end

context "when sendmail is not installed" do
before do
ActionMailer::Base.delivery_method = :sendmail
ActionMailer::Base.sendmail_settings[:location] = '/usr/sbin/sendmail'
expect(File).to receive(:executable?).with('/usr/sbin/sendmail').and_return(false)
end

it { is_expected.not_to be_successful_check }
it { is_expected.to have_message "ActionMailer::Base sendmail executable /usr/sbin/sendmail is not executable" }
end

context "when mailer is in test mode" do
before { ActionMailer::Base.delivery_method = :test }

it { is_expected.to be_successful_check }
it { is_expected.to have_message "ActionMailer::Base is in test mode" }
end
end
end
end