Skip to content
Merged
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
25 changes: 25 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,41 @@ gem but are what an operator runs against their own image.

## next / unreleased

### Upgrading

Some actions that application developers should consider taking when upgrading from an earlier version:

* Replace `rescue HotCell::Verdict` with `rescue HotCell::Failure::Exception`, and `HotCell::Client::Legacy::Verdict` with `HotCell::Client::Legacy::Failure::Exception`. Only the name changed: the exception still carries `hot_cell_failure`.
* In your own `perform.hot_cell` subscribers, test for success with `payload[:code] == "ok"` instead of `payload[:code].nil?`, and expect `interrupted`. The event now sets `code` on every call; see [Per-call notification](docs/observability.md#per-call-notification).
* Where a test instruments `perform.hot_cell` by hand to stand in for a successful call, pass `code: "ok"`. `HotCell::LogSubscriber` and `yabeda-hotcell` now record `code` as the event gives it, so an event without one records no code.
* Count `code="interrupted"` as a failed call wherever a dashboard or alert computes an error rate from `hotcell_requests`. `yabeda-hotcell` counted these calls as `ok` before.

### HotCell::Client

#### Breaking

* The `perform.hot_cell` event sets `code` on every call: `ok` on success, the failure's code on failure, and `interrupted` when an exception interrupts the call, such as the application's own request timeout. Previously, `code` was `nil` on success and absent from an interrupted call, so each subscriber had to work out the call's result. (#93)
* `HotCell::Verdict` is renamed `HotCell::Failure::Exception`.

#### Changed

* `HotCell::LogSubscriber` writes `"code":"interrupted"` for a call that an exception interrupted, beside the exception's class. Previously, the line had no code.

#### Fixed

* On macOS, a call to a full cell reports `capacity` instead of occasionally reporting `unavailable`. A full cell answers and closes the connection without reading the request, so the client's send fails. The client already ignored that failure as `EPIPE` or `ECONNRESET` and read the answer, but macOS sometimes raises `ENOTCONN` instead. (#110)

### HotCell::Client::Legacy

#### Breaking

* `HotCell::Client::Legacy::Verdict` is renamed `HotCell::Client::Legacy::Failure::Exception`.

### Yabeda::HotCell

#### Fixed

* A call that an exception interrupted, such as the application's own request timeout, is counted in `requests` under `code="interrupted"` and records no `perform` observation. Previously, it was counted as `code="ok"` with a `perform` of 0 seconds. (#93)
* `perform` records nothing for a call whose response has no `perform_ms`: a call that the cell refused with `capacity`, or that the client failed itself with `unavailable` or `timeout`. Previously, it recorded 0 seconds, which pulled down the low percentiles. (#112)

## v1.1.0 / 2026-10-08
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ module HotCell
module Client
# These four jobs declare `retry_on ActiveStorage::IntegrityError` and nothing else, and ActiveJob does
# not retry by default. They should retry the transient class too: `capacity` most obviously, and every
# other transient verdict. The policy matches the one they already declare, so a cell failure and an
# other transient failure. The policy matches the one they already declare, so a cell failure and an
# integrity failure back off the same way.
#
# Which of these classes exists depends on the Rails version, so a name that is not loaded is skipped.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module Analyzers
# **Which way `metadata` fails is the whole design of the method.** The built-in vips analyzer rescues
# every Vips::Error and returns an empty hash, which Rails then merges with `analyzed: true` — so an
# undecodable image is recorded as successfully analyzed, forever, and nothing ever re-enqueues
# AnalyzeJob. That is right for a permanent verdict and catastrophic for a transient one.
# AnalyzeJob. That is right for a permanent failure and catastrophic for a transient one.
#
# So a permanent failure follows the built-in behaviour and lets the blob be marked analyzed, with the
# reason written to the log so it can be re-decided later against a newer library. A transient failure
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def test_it_reports_the_dimensions_exif_asks_for

# The built-in vips analyzer rescues every Vips::Error and returns an empty hash, which Rails merges with
# `analyzed: true` — so an undecodable image is recorded as successfully analyzed, forever, and nothing ever
# re-enqueues AnalyzeJob. That is the right answer for a permanent verdict, so it is copied deliberately.
# re-enqueues AnalyzeJob. That is the right answer for a permanent failure, so it is copied deliberately.
def test_a_permanently_undecodable_image_is_marked_analyzed_the_way_rails_marks_it
with_cell do
assert_empty analyze("broken.png")
Expand Down
2 changes: 1 addition & 1 deletion activestorage-hotcell-client/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def with_cell
end
end

# For the cases about classification, where booting a cell to produce one verdict would be theatre.
# For the cases about classification, where booting a cell to produce one failure would be theatre.
def with_canned_response(response)
HotCell.root = "/nowhere"
transport = CannedTransport.new(response)
Expand Down
6 changes: 3 additions & 3 deletions activestorage-hotcell-client/test/transformers/vips_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,9 @@ def test_a_loader_reaches_the_cell_and_keeps_every_frame
# declares Vips::Error `unreadable`, so it lands permanent.
#
# KNOWN DEVIATION (resolved by the planned ImageMagick operations): this is a caller bug, not a bad
# document, and Rails treats it as an unclassified job failure rather than a verdict on the blob. The
# document, and Rails treats it as an unclassified job failure rather than a permanent failure of the blob. The
# `unreadable Vips::Error` rule was written for the load phase, where a Vips::Error means bad pixels; a
# bad operation name raising the same class slips through as a false permanent verdict. The real fix is an
# bad operation name raising the same class slips through as a false permanent failure. The real fix is an
# ImageMagick transformer where `coalesce` is a real operation — until then an application moving from
# mini_magick to vips rewrites these URLs at its own boundary. This test pins the current behavior so the
# deviation is visible, not hidden.
Expand All @@ -139,7 +139,7 @@ def test_an_undecodable_image_raises_the_applications_permanent_class
end
end

# A caller bug, not a verdict on the document: ImageProcessing refuses the name, the failure is
# A caller bug, not a permanent failure of the document: ImageProcessing refuses the name, the failure is
# unclassified and therefore transient, and it stays distinguishable from an undecodable image — which
# raises the permanent class and gets a placeholder.
def test_an_unknown_transformation_raises_the_transient_class
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module Image
# deliberately does not copy: it rescues every Vips::Error and returns an empty hash, which is then
# merged with `analyzed: true`. An undecodable image is recorded as successfully analyzed, forever,
# and nothing re-enqueues AnalyzeJob. Here an undecodable input raises Vips::Error, the cell answers
# `unreadable`, and the client decides — because only the client knows whether that verdict is safe
# `unreadable`, and the client decides — because only the client knows whether that failure is safe
# to write down.
class Vips < VipsOperation
operation "active_storage.analyzers.image.vips"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def initialize

# MiniMagick::Error is how mini_magick reports a `magick` that exited non-zero — the common shape of an
# input it cannot decode. MiniMagick::Invalid is an input `identify` rejects outright.
# ImageProcessing::Error is the pipeline's own verdict on the input against the requested
# ImageProcessing::Error is the pipeline's own rejection of the input against the requested
# transform — a multi-layer source into a single-layer destination, for example. All three are
# the input's fault rather than the operation's; unclassified, each was a transient `failed`
# that never marked the blob, so the same file spent a full conversion on every request.
Expand All @@ -69,7 +69,7 @@ def initialize
# **Accepted risk.** A tool's output is not bounded on this path. `Operation#run_tool` caps what it
# reads at 64KB and drops the rest as it arrives, because an input that makes a tool print gigabytes
# of diagnostics costs this worker gigabytes of address space, takes RLIMIT_DATA with it, and arrives
# as a `memory` verdict — which is permanent, for a document whose only crime was being noisy.
# as a `memory` failure — which is permanent, for a document whose only crime was being noisy.
# mini_magick reads both streams to EOF in a thread apiece and has no setting that bounds either;
# `graphicsmagick`, `cli_prefix`, `cli_env`, `restricted_env`, `timeout`, `logger`, `tmpdir`,
# `errors` and `warnings` are the whole list. The premise is that patching the library is worse than
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class VipsOperation < Operation

# Vips::Error is how libvips reports a file it cannot decode, which is common rather than exceptional: it
# covers truncated uploads, formats this build was not compiled with, and formats deliberately refused by
# block_untrusted. ImageProcessing::Error is the pipeline's own verdict on the input against the
# block_untrusted. ImageProcessing::Error is the pipeline's own rejection of the input against the
# requested transform, classified the same way. All of those are the input's fault and none is
# the operation's.
unreadable Vips::Error, ImageProcessing::Error
Expand Down
2 changes: 1 addition & 1 deletion activestorage-hotcell-server/test/analyze_image_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def test_analysis_says_how_many_frames_there_are

# The built-in vips analyzer rescues every Vips::Error and returns an empty hash, which is then merged with
# `analyzed: true` — so an undecodable image is recorded as successfully analyzed, forever, and nothing
# re-enqueues AnalyzeJob. This deliberately does not copy that: the cell reports the verdict and the client
# re-enqueues AnalyzeJob. This deliberately does not copy that: the cell reports the failure and the client
# decides, because only the client knows whether it is safe to write down.
# Rails imposes no size limit on analysis, and neither may the cell. The input is read through its
# descriptor rather than copied onto scratch, so a file far larger than the operation's file_size — which
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def test_an_imagemagick_only_operation_runs

# The production shape that surfaced this: a multi-layer source, its frames kept by
# `loader: { page: nil }`, into a single-layer destination. ImageProcessing refuses the
# combination, which is a verdict on the input — unclassified it was a transient `failed`
# combination, which is a permanent failure of the input — unclassified it was a transient `failed`
# that never marked the blob, so the same file spent a full conversion on every request.
def test_a_multi_layer_source_into_a_single_layer_format_is_unreadable
Cell.boot do |cell|
Expand Down
2 changes: 1 addition & 1 deletion activestorage-hotcell-server/test/transform_image_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ def test_a_format_no_saver_answers_to_is_a_vips_error
# A name that is neither an ImageProcessing operation nor a Vips::Image method is refused by
# ImageProcessing itself, exactly as it is under Rails on vips. It arrives `failed` — transient — so a
# caller bug is never written down against the document.
def test_an_unknown_transformation_is_not_a_verdict_on_the_document
def test_an_unknown_transformation_is_not_a_permanent_failure_of_the_document
Cell.boot do |cell|
with_output do |destination|
failure = assert_failed "failed", cell.call("active_storage.transformers.image.vips",
Expand Down
2 changes: 1 addition & 1 deletion bin/conformance
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ docker volume create "$VOLUME" >/dev/null
# The security flags the accessory sets, added one at a time so a negative job can leave exactly one out.
# `keep NAME arg...` appends the flag unless it is the one named in DROP. No --ulimit stack: docs/container.md
# forbids it — a lowered stack turns an overflow into a SIGSEGV the cell reports as killed/crashed, a transient
# verdict the caller retries against a limit that will fail it again, so the helper would measure a shape
# failure the caller retries against a limit that will fail it again, so the helper would measure a shape
# operators are told not to deploy.
run=(docker run --detach --name "$CELL")

Expand Down
2 changes: 1 addition & 1 deletion docs/cell-settings.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ limit.
| --- | --- | --- |
| `concurrency` | `4` | Workers that run at once, and the number of slots. Start at twice `cpus`. See [Settings that trade isolation](#settings-that-trade-isolation). |
| `queue_size` | `8` | Connections that can wait for a worker. When `running + queued` reaches `concurrency + queue_size`, the cell answers `capacity`. To refuse instead of queueing, use `0`. |
| `queue_wait` | `10` | Seconds that a queued connection can wait before the cell answers `capacity`. A saturated cell then answers with a verdict instead of holding the caller until the caller's own timeout. |
| `queue_wait` | `10` | Seconds that a queued connection can wait before the cell answers `capacity`. A saturated cell then answers with a failure instead of holding the caller until the caller's own timeout. |
| `max_requests_per_worker` | `1` | Requests that one worker serves before the cell discards it. `1` forks for each request. `:unlimited` keeps a worker for the life of the cell. A reused worker skips the copy-on-write cost of a new fork, which is a performance setting with an isolation cost. See [Settings that trade isolation](#settings-that-trade-isolation). |
| `control_deadline` | `5` | Seconds that a control connection can take to send its request. |
| `sweep_interval` | `10` | How often, in seconds, the supervisor looks for the directories that killed requests left behind. When it finds one, it forks a sweeper process to delete them. The sweeper runs under `deadline` like a worker, so the supervisor and the requests in flight never wait on the deletion. |
Expand Down
2 changes: 1 addition & 1 deletion docs/client-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ publishes a `perform.hot_cell` notification. See
[Observability](observability.md#per-call-notification).

The exception carries the failure. Call `hot_cell_failure` on it to read the failure's `code` and `cause`,
or rescue `HotCell::Verdict` to catch it whatever classes you registered. See
or rescue `HotCell::Failure::Exception` to catch it whatever classes you registered. See
[Response codes](codes.md#exception-classes).

### Errors that the client raises for a bad call
Expand Down
2 changes: 1 addition & 1 deletion docs/client-legacy.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ end
| `HotCell::Client::Legacy::PermanentFailure` | The cell marks the failure permanent. A retry fails the same way. |
| `HotCell::Client::Legacy::TransientFailure` | Any other failure. A retry might succeed. |

Both classes include `HotCell::Client::Legacy::Verdict`, so you can rescue `Verdict` to catch either one.
Both classes include `HotCell::Client::Legacy::Failure::Exception`, so you can rescue `Failure::Exception` to catch either one.
Call `hot_cell_failure` on the exception to read the failure, as with `hotcell-client`. See
[Exception classes](codes.md#exception-classes). The failure has these attributes:

Expand Down
7 changes: 4 additions & 3 deletions docs/codes.md
Original file line number Diff line number Diff line change
Expand Up @@ -147,18 +147,19 @@ the `stderr` tail in parentheses when it exists. For example, `killed: crashed (
To read the failure without parsing the message, call `hot_cell_failure` on the exception. It returns the
`HotCell::Failure`, which has `code`, `cause`, `error_class`, `message`, `signal`, `stderr`, and
`permanent?`. The client extends every exception that it raises for a failed call with the
`HotCell::Verdict` module, so you can also rescue `HotCell::Verdict` whatever classes you registered:
`HotCell::Failure::Exception` module, so you can also rescue `HotCell::Failure::Exception` whatever
classes you registered:

```ruby
rescue HotCell::Verdict => error
rescue HotCell::Failure::Exception => error
raise unless error.hot_cell_failure.cause == HotCell::Codes::PROTECTED

lock_placeholder
```

## What Active Storage records

A permanent verdict is irreversible only if the application records it. In the shipped Active Storage
A permanent failure is irreversible only if the application records it. In the shipped Active Storage
integration, analysis records it and nothing else does.

Rails persists a blob's analysis like this:
Expand Down
2 changes: 1 addition & 1 deletion docs/concepts.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,5 +119,5 @@ See [Inputs and outputs](operation-api.md#inputs-and-outputs).
## Code

Every failure carries a code, and each code is permanent or transient. A permanent failure means that the
input fails this way every time, so the caller can record that verdict against the input. Every uncertain
input fails this way every time, so the caller can record that failure against the input. Every uncertain
failure is transient, which means that it might succeed on a retry. See [Response codes](codes.md).
2 changes: 1 addition & 1 deletion docs/conformance.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ bin/conformance my-cell:test
1. Boots the image with the flags in [Container flags](container.md#container-flags).
2. Mounts the example operations over `/hotcell/operations`.
3. Runs a battery of checks from a second container over a shared volume: descriptor round trips, each
kill verdict, refusal at capacity, and the isolation flags.
cause of `killed`, refusal at capacity, and the isolation flags.
4. Runs the same battery again against the image booted without one security flag at a time, and requires
each run to fail at that flag's own check.

Expand Down
2 changes: 1 addition & 1 deletion docs/container.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ Leave `ulimit: stack` unset. A container inherits the Docker daemon's value, nor
Lowering it to 2MB buys a worker about 24MB more room, and nothing for an operation that shells out. It
costs far more than that: a thread that overflows the smaller stack dies on `SIGSEGV`, and the cell
reports that as `killed` with cause `crashed`, which is transient. The caller's job retries the request
against a limit that fails it again, for as long as the job keeps trying, and nothing in the verdict
against a limit that fails it again, for as long as the job keeps trying, and nothing in the failure
points at the setting. Raise the cell's `memory` instead.

The stack limit is a container flag rather than a `config.rb` setting because glibc reads it at exec,
Expand Down
Loading
Loading