From 2889a2252a64c793f1252db40a21592f605032f2 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 1 Oct 2026 14:45:16 -0400 Subject: [PATCH] Warn at boot when a cell runs another hotcell release `HotCell.describe_cells` did not compare a cell's `server_version` with the client's version. `docs/DEPLOYMENT.md` and the generated cell `Gemfile` said that a version skew answers `protocol` on every request, which is false. Warn at boot when the versions differ, and remove the false claim. [Fix #21] --- CHANGELOG.md | 1 + README.md | 2 +- docs/DEPLOYMENT.md | 8 ++--- hotcell-client/lib/hot_cell/cell.rb | 11 ++++++- .../lib/hot_cell/install/Gemfile.tt | 3 +- hotcell-client/test/describe_survival_test.rb | 11 +++++++ hotcell-client/test/install_test.rb | 2 -- hotcell-client/test/integration_test.rb | 30 +++++++++++++++++++ 8 files changed, 58 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f67ff80..c28020e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ Some actions that application developers should consider taking when upgrading f #### Added +* `HotCell.describe_cells` warns at boot when a cell's `hotcell-server` version differs from the application's `hotcell-client` version. (#21) * `HotCell::LogSubscriber` writes one `info` line to the Rails log for each call. The line contains the cell, the operation, the outcome and both durations. It also contains the byte counts when the client measures them. The railtie attaches the subscriber. See the README's "Application logs". * `HotCell::HealthController` and `HotCell::DiagnosticsController` check each registered cell. `HotCell::HealthController` uses only the control socket and returns `OK` or `FAIL`. It takes no worker. `HotCell::DiagnosticsController` also sends `health.echo` and `health.reopen` on the work socket. It takes a worker for each round trip. It returns each result as JSON. To use the controllers, add a route for each to the application. Put the diagnostics route behind authentication. To do this, set `HotCell.diagnostics_controller_parent` in an initializer to the name of an authenticated controller class. Alternatively, subclass `HotCell::DiagnosticsController`, authenticate in the subclass, and route to the subclass. See the README's "Rails healthcheck". diff --git a/README.md b/README.md index 4ec51a1..3fbe1b3 100644 --- a/README.md +++ b/README.md @@ -260,7 +260,7 @@ class for each side of the permanent split: HotCell.root = ENV["HOTCELL_ROOT"] # unset means every cell is off HotCell.group = ENV["HOTCELL_GROUP"] # the gid shared between app and cell -# Quick health check at boot. Warns about a cell that is unreachable, slower than this client waits, or in the wrong group. +# Quick health check at boot. Warns about a cell that is unreachable, slower than this client waits, in the wrong group, or on another hotcell version. Rails.application.config.after_initialize { HotCell.describe_cells } ``` diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 247c36d..ddc830c 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -71,9 +71,9 @@ and not only `/run/hotcell`. Docker creates a missing last level as root, and th socket in it. The installed Dockerfile does this. The application's image needs nothing at its own mount point. -**Keep the app's and the cell's lockfiles in step.** They resolve hotcell separately. A skew between the -client the app loads and the server the cell runs answers `protocol` on every request. While either tracks -a branch rather than a released version, assert in a test that both lockfiles name the same revision. +**Keep the app's and the cell's lockfiles in step.** They resolve hotcell separately. +`HotCell.describe_cells` warns at boot when the cell's `hotcell-server` version differs from the app's +`hotcell-client` version. ### Bringing your own container @@ -422,7 +422,7 @@ HotCell.register "images", | `timeout:` | `30` | Seconds this caller waits for an answer to a work request. It must clear the cell's `answer_within`. A positive, finite number: `register` raises `ConfigurationError` on anything else, including `nil`, which stops the boot rather than leaving the wait for a cell to decide. | | `control_timeout:` | `5` | Seconds this caller waits for `describe` or `metrics`. The supervisor answers both inline, with no fork and no queue, so this is short on purpose and must not be raised toward `timeout`. It is what bounds app boot when a cell accepts connections and never answers, and what lets a health check report a wedged cell instead of waiting out a work timeout. Bounded like `timeout:`, and refused the same way. | | `permanent:`, `transient:` | the gem's classes | The exception classes the application raises for each side of the split. `transient` must not descend from `permanent`, and the client refuses to start if it does. | -| `on_contract_skew:` | — | Called when a cell answers `protocol`, so a version mismatch is visible to an application that rescues broadly. | +| `on_contract_skew:` | — | Called when a cell answers `protocol`, so a protocol version mismatch is visible to an application that rescues broadly. | ### The socket's file mode diff --git a/hotcell-client/lib/hot_cell/cell.rb b/hotcell-client/lib/hot_cell/cell.rb index fd87315..65b3648 100644 --- a/hotcell-client/lib/hot_cell/cell.rb +++ b/hotcell-client/lib/hot_cell/cell.rb @@ -74,7 +74,7 @@ def report_contract_skew(error) # deployment rather than a broken one, and an application that refuses to start because its thumbnail # cell is restarting is worse than one that serves placeholders. So this warns and carries on. # - # Nor when a cell answers something this client cannot read. The two warnings below reach into the + # Nor when a cell answers something this client cannot read. The warnings below reach into the # description without checking types, so a cell that sends the wrong ones raises here — and the README # calls `describe_cells` from `after_initialize`, where that is not a failed check but an application # that does not boot. The process that wrote the description is the one that runs untrusted content. @@ -88,6 +88,7 @@ def describe response.result.tap do |described| warn_about_timeout described warn_about_group_skew described + warn_about_version_skew described end rescue StandardError => error HotCell.logger.warn "hotcell #{name}: this cell's description could not be read and is being " \ @@ -222,6 +223,14 @@ def warn_about_group_skew(described) "every operation that gives a tool a filename will fail with EACCES." end + def warn_about_version_skew(described) + reported = described[:server_version] + return if reported == Client::VERSION + + HotCell.logger.warn "hotcell #{name}: this client is #{Client::VERSION} and the cell reports " \ + "hotcell-server #{Failure.one_line reported.inspect}." + end + # A `nil` timeout reaches `Transport::Socket#receive` as `deadline: nil`, and reading with no deadline # blocks: a cell that accepts the connection and then never answers holds this caller for good. At # boot that is an application that never finishes starting, with no exception and nothing to rescue. diff --git a/hotcell-client/lib/hot_cell/install/Gemfile.tt b/hotcell-client/lib/hot_cell/install/Gemfile.tt index 2c9125b..4e82d60 100644 --- a/hotcell-client/lib/hot_cell/install/Gemfile.tt +++ b/hotcell-client/lib/hot_cell/install/Gemfile.tt @@ -5,8 +5,7 @@ source "https://rubygems.org" -# Pinned to the client that installed it: the two are halves of one wire contract, and a skew between -# them answers `protocol` on every request. +# Keep this at the application's hotcell-client version; HotCell.describe_cells warns at boot otherwise. gem "hotcell-server", "<%= HotCell::Client::VERSION %>" # Gems your operations need go here. For example: diff --git a/hotcell-client/test/describe_survival_test.rb b/hotcell-client/test/describe_survival_test.rb index 100027e..25d72e1 100644 --- a/hotcell-client/test/describe_survival_test.rb +++ b/hotcell-client/test/describe_survival_test.rb @@ -78,6 +78,17 @@ def test_a_cell_cannot_forge_a_log_line end end + def test_a_cell_cannot_forge_a_log_line_through_its_version + forged = %(ERROR -- : hotcell test: everything is fine) + + with_cell_describing(%({"server_version":"0.0.1\\n#{forged}"})) do |cell| + warnings = capturing_warnings { cell.describe } + + assert_match "0.0.1", warnings + assert_equal 1, warnings.lines.grep(/hotcell test/).size, "the cell wrote a line of its own: #{warnings}" + end + end + def test_one_unusable_cell_does_not_stop_the_others_being_read Dir.mktmpdir "hc" do |root| answering root, "hostile", %({"answer_within":"attacker-controlled"}) diff --git a/hotcell-client/test/install_test.rb b/hotcell-client/test/install_test.rb index df639eb..060517d 100644 --- a/hotcell-client/test/install_test.rb +++ b/hotcell-client/test/install_test.rb @@ -41,8 +41,6 @@ def test_every_installer_template_is_packaged assert_empty templates - spec.files, "an installer template the published gem would not carry" end - # The Gemfile template is ERB rather than a copy, because the cell's server and this client are halves of - # one wire contract: a skew between them answers `protocol` on every request. def test_install_pins_the_cell_to_the_installing_clients_version Dir.mktmpdir do |root| HotCell::Install.call(root, out: StringIO.new) diff --git a/hotcell-client/test/integration_test.rb b/hotcell-client/test/integration_test.rb index c6c85bf..a82791c 100644 --- a/hotcell-client/test/integration_test.rb +++ b/hotcell-client/test/integration_test.rb @@ -239,6 +239,30 @@ def test_describe_reports_the_groups_the_cell_runs_in end end + def test_describe_is_quiet_when_the_cell_runs_this_clients_version + with_cell do + refute_match "reports hotcell-server", capturing_warnings { HotCell.describe_cells } + end + end + + def test_describe_warns_when_the_cell_runs_another_version + register_describing server_version: "0.0.1" + + warnings = capturing_warnings { HotCell.describe_cells } + + assert_match %(this client is #{HotCell::Client::VERSION} and the cell reports hotcell-server "0.0.1"), warnings + end + + def test_describe_reads_and_warns_about_a_cell_too_old_to_report_its_version + register_describing operations: [ "test.echo" ] + + described = nil + warnings = capturing_warnings { described = HotCell.describe_cells } + + assert_equal [ "test.echo" ], described["test"][:operations] + assert_match "this client is #{HotCell::Client::VERSION} and the cell reports hotcell-server nil", warnings + end + # Counts lag responses — the worker answers the caller before the supervisor reads its idle report and # increments the counter — so the wait is the assertion, exactly as in the server suite's metrics test. def test_metrics_come_back_through_the_registration @@ -277,6 +301,12 @@ class Blocking < HotCell::Client end private + def register_describing(result) + HotCell.root = "/nowhere" + HotCell.register "test", permanent: Unprocessable, transient: TemporarilyUnavailable, + transport: ->(*) { HotCell::Response.ok(result: result, timing: {}) } + end + # A gid this process cannot be holding, so the check has something unambiguous to fail on. def unheld_group @unheld_group ||= ((1..65_000).to_a - Process.groups - [ Process.gid, Process.egid ]).last