Skip to content

Carry a failure's stderr on the perform.hot_cell event - #60

Merged
flavorjones merged 1 commit into
masterfrom
improved-logging-5270
Sep 8, 2026
Merged

flavorjones merged 1 commit into
masterfrom
improved-logging-5270

Conversation

@flavorjones

Copy link
Copy Markdown
Member

The HotCell (…) line each application logs from the perform.hot_cell event reads "cause":"crashed" with no diagnosis. The cell sends the worker's stderr on the wire and Failure#to_s renders it into the exception's message, but only the Retrying … line logs that message, and only when a retry follows. A failure that was discarded lost the one line that explained it, libgomp: Thread creation failed: Resource temporarily unavailable, and reading it meant querying the cell's own Loki stream. Post-mortem: card 10261975276; this is the "Log the cell's stderr and signal Rails-side" action item, card 10270096231.

Client#publish now sets event[:stderr] = failure&.stderr beside signal. The field is already bounded by Failure.sanitize on the way in, tail-kept, so a hostile cell cannot grow it past MAX_MESSAGE_BYTES. It is text a tool wrote while processing a hostile file, and the comment on publish says a subscriber should write it to a log field and interpolate it nowhere else.

The test is a crashed failure with stderr publishing an event that carries it, beside the existing cause and signal cases; the success case asserts the field is nil. Changelog under HotCell::Client / Added. VERSION is already 0.3.2.dev, so no bump.

The application half is separate: once this ships, Yabeda::HotCell.log_perform in bc3, haystack and fizzy logs signal and stderr on the HotCell (…) line.

The `HotCell (…)` log line an application writes from the event read
`"cause":"crashed"` with no diagnosis. The tool's stderr survived only
in the exception's message, and only a retry logs that, so a discarded
failure lost it.

Publish `stderr` beside `signal`. The field is bounded by
`Failure.sanitize` on the way in; the comment on `publish` says what a
subscriber may do with it.

ref: https://app.basecamp.com/2914079/buckets/1666/card_tables/cards/10270096231
Copilot AI balanced review requested due to automatic review settings September 8, 2026 13:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is small, consistent with existing Failure sanitization/bounding behavior, and is covered by targeted tests and changelog documentation.

Pull request overview

This PR improves observability of HotCell failures by including the cell/worker’s captured stderr on the perform.hot_cell event payload, so subscribers can log the crash diagnosis even when a failure is discarded (i.e., not retried) and its exception message isn’t otherwise emitted.

Changes:

  • Add event[:stderr] = failure&.stderr to HotCell::Client#publish alongside existing code/cause/signal/permanent fields.
  • Extend classification tests to assert stderr is present for a crashed failure and nil on success.
  • Document the new event field in CHANGELOG.md under HotCell::Client / Added.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries
File Description
hotcell-client/lib/hot_cell/client.rb Publishes stderr on perform.hot_cell events so subscribers can log crash diagnostics.
hotcell-client/test/classification_test.rb Adds assertions covering stderr propagation for crash failures and nil on success.
CHANGELOG.md Documents the addition of stderr on the perform.hot_cell event.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@flavorjones
flavorjones merged commit 460a8e1 into master Sep 8, 2026
17 checks passed
@flavorjones
flavorjones deleted the improved-logging-5270 branch September 8, 2026 13:56
flavorjones added a commit to basecamp/fizzy that referenced this pull request Sep 8, 2026
* Upgrade hotcell to 0.4.0

A killed worker left its tool's scratch files at the top of the cell's
`/tmp`, outside the slot tree that is removed when a request ends, so
the scratch volume filled over time. A worker now points `TMPDIR` at
the request's home, and the supervisor empties the scratch at boot.

That boot sweep is why the dev cell in `saas/Procfile.dev` now gets a
`TMPDIR` of its own: unset, it would sweep the developer's `/tmp`.

The release's ImageMagick changes do not reach this cell, which loads
only the vips, ffprobe, mutool and ffmpeg operations and installs no
ImageMagick, so the `MAGICK_*_LIMIT` variables stay unset.

ref: https://github.com/basecamp/hotcell/blob/v0.4.0/CHANGELOG.md

* Log what a failed cell tool wrote to stderr

A crash's diagnosis — `libgomp: Thread creation failed` — survived only
in the exception's message, so a failure that was discarded rather than
retried lost it. hotcell 0.4.0 carries the captured stream on the
`perform.hot_cell` event; write it as a field on the existing log line.

ref: basecamp/hotcell#60

* Upgrade hotcell to 0.4.1 and boot the dev cell with --development

The 0.4.0 dev cell needed a `TMPDIR` of its own, made by hand, or its
boot sweep emptied the developer's `/tmp`. hotcell 0.4.1 adds
`hotcell --development`, which keeps the scratch in a directory of the
cell's own under the system temporary directory and never sweeps the
directory it was given. Use it in `saas/Procfile.dev` instead.

ref: basecamp/hotcell#61

* Pin the hotcell gems to exact versions

A `~>` pin let a patch release move the app's client and the cell's
server independently, and one version apart is a `protocol` failure on
every request. Pin both Gemfiles to `0.4.1`.
rhysb27 pushed a commit to rhysb27/fizzy that referenced this pull request Sep 16, 2026
* Upgrade hotcell to 0.4.0

A killed worker left its tool's scratch files at the top of the cell's
`/tmp`, outside the slot tree that is removed when a request ends, so
the scratch volume filled over time. A worker now points `TMPDIR` at
the request's home, and the supervisor empties the scratch at boot.

That boot sweep is why the dev cell in `saas/Procfile.dev` now gets a
`TMPDIR` of its own: unset, it would sweep the developer's `/tmp`.

The release's ImageMagick changes do not reach this cell, which loads
only the vips, ffprobe, mutool and ffmpeg operations and installs no
ImageMagick, so the `MAGICK_*_LIMIT` variables stay unset.

ref: https://github.com/basecamp/hotcell/blob/v0.4.0/CHANGELOG.md

* Log what a failed cell tool wrote to stderr

A crash's diagnosis — `libgomp: Thread creation failed` — survived only
in the exception's message, so a failure that was discarded rather than
retried lost it. hotcell 0.4.0 carries the captured stream on the
`perform.hot_cell` event; write it as a field on the existing log line.

ref: basecamp/hotcell#60

* Upgrade hotcell to 0.4.1 and boot the dev cell with --development

The 0.4.0 dev cell needed a `TMPDIR` of its own, made by hand, or its
boot sweep emptied the developer's `/tmp`. hotcell 0.4.1 adds
`hotcell --development`, which keeps the scratch in a directory of the
cell's own under the system temporary directory and never sweeps the
directory it was given. Use it in `saas/Procfile.dev` instead.

ref: basecamp/hotcell#61

* Pin the hotcell gems to exact versions

A `~>` pin let a patch release move the app's client and the cell's
server independently, and one version apart is a `protocol` failure on
every request. Pin both Gemfiles to `0.4.1`.
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.

2 participants