Ap 527 healthcheck #11

steve-sullivan · 2026-01-22T22:05:26Z

No description provided.

anarchivist

generally looks good; i have a couple of questions. thanks for tackling this!

config/initializers/okcomputer.rb

awilfox

Thank you for working on this! This is a good first pass.

I did find some issues that need to be worked out.

awilfox · 2026-01-24T17:19:59Z

app/lib/health_checks/iiif_server_check.rb

+    def check
+      result = validate_iiif_server
+      mark_message result[:message]
+      mark_failure if result[:warning]


Why is the key called warning if it is marking a failure? This seems unclear.

Good call - originally I had them as "warnings", then changed failed checks to "failures", but didn't think about the keys.

awilfox · 2026-01-24T17:21:58Z

app/lib/health_checks/iiif_server_check.rb

+      mark_failure if result[:warning]
+    rescue StandardError => e
+      logger.error(e)
+      mark_message "#{e.class}: #{e.message}"


We probably don't want to expose the full exception message to the public Internet as it may have sensitive info. (For example, consider PG::ConnectionBad: role "foo" does not exist - it has the database username in the short message.)

We can probably just settle for e.class.

awilfox · 2026-01-24T17:22:58Z

app/lib/health_checks/test_item_exists.rb

+  class TestItemExists < OkComputer::Check
+    include BerkeleyLibrary::Logging
+
+    ERR_NO_COMPLETE_ITEM = 'Unable to locate complete item'.freeze


Is this a constant only for the later reference in the test? Is it important for another reason?

I'm not necessarily saying it shouldn't be, but this doesn't match the way we do this anywhere else.

Yeah, it was referenced in the spec, but to stay more consistent I've removed it and updated the spec

awilfox · 2026-01-24T17:23:30Z

app/lib/health_checks/lending_root_path.rb

+      mark_failure if result[:warning]
+    rescue StandardError => e
+      logger.error(e)
+      mark_message "#{e.class}: #{e.message}"


As above, this should probably just be something like "Error: #{e.class}".

awilfox · 2026-01-24T17:23:48Z

app/lib/health_checks/test_item_exists.rb

+        mark_failure
+      end
+    rescue StandardError => e
+      mark_message "Failed to check item: #{e.message}"


As above:

Suggested change

mark_message "Failed to check item: #{e.message}"

mark_message "Failed to check item: #{e.class}"

Also, we aren't logging this one?

Good catch, I've added logging

awilfox · 2026-01-24T17:27:40Z

app/lib/health_checks.rb

@@ -0,0 +1,3 @@
+module HealthChecks
+  Dir[File.join(__dir__, 'health_checks', '*.rb')].each { |f| require f }


While I understand the concept of making this less of a maintenance hassle, requiring a glob like this feels dangerous to me. It's easy code injection for any malicious actor with minimal ability to even be detected (just drop into app/lib/health_checks; this could even target developer workstations).

We could do something like a frozen array of checks and then iterate over them, calling require on each one.

awilfox · 2026-01-24T17:29:39Z

spec/system/health_system_spec.rb

-      visit health_path
+      begin
+        # Register a check that always fails
+        OkComputer::Registry.register 'failing-check', OkComputer::HttpCheck.new('http://localhost:9999/nonexistent')


This should probably use example.test or such as we did above (which is guaranteed to never exist). Otherwise, if another service is running locally on a dev machine, it could hit that (and if it's a "magic" service that returns a response for every URL, it might even pass).

awilfox · 2026-01-24T17:30:28Z

CHANGES.md

@@ -1,3 +1,7 @@
+# 1.7.6 (2026-01-16)


This date is already wrong. It should not be there until the release is actually made and tagged (in its own commit).

awilfox · 2026-01-24T17:31:49Z

spec/lib/lending/config_spec.rb

+        Config.instance_variable_set(:@lending_root_path, nil)
+      end
+
+      it 'raises ConfigException when ENV IIIF base URL is invalid (covers line 70)' do


In general, it's a bad idea to reference specific line numbers. If the source file is changed, these test descriptions will quickly stop having any meaning and become outdated. Keeping them updated is an unexpected maintenance burden.

The test as you have written it is descriptive enough without the line numbers.

UGH... bad me... letting AI do a bunch of the heavy lifting for test coverage and made the mistake of not proof reading all of the spec. Fixed those

steve-sullivan added 3 commits January 22, 2026 10:45

AP-527 Move healthchecks to OkComputer

b7d5c15

Renaming class to fit with zeitwerk

0f0f4cd

Tweak collector spec having ordering issues

78c9593

steve-sullivan requested review from anarchivist and awilfox January 22, 2026 22:05

anarchivist reviewed Jan 22, 2026

View reviewed changes

config/initializers/okcomputer.rb Outdated Show resolved Hide resolved

config/initializers/okcomputer.rb Outdated Show resolved Hide resolved

Fixed request spec

9e6262a

awilfox requested changes Jan 24, 2026

View reviewed changes

	mark_message "Failed to check item: #{e.message}"
	mark_message "Failed to check item: #{e.class}"

		@@ -0,0 +1,3 @@
		module HealthChecks
		Dir[File.join(__dir__, 'health_checks', '*.rb')].each { \|f\| require f }

Ap 527 healthcheck #11

Are you sure you want to change the base?

Ap 527 healthcheck #11

Conversation

steve-sullivan commented Jan 22, 2026

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

awilfox 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants