Skip to content

fix(cron): media-send failure logs an empty reason when the cause is a timeout - #87965

Closed
AiwendilInTheWoods wants to merge 1 commit into
NousResearch:mainfrom
AiwendilInTheWoods:fix/cron-media-send-empty-timeout-reason
Closed

fix(cron): media-send failure logs an empty reason when the cause is a timeout#87965
AiwendilInTheWoods wants to merge 1 commit into
NousResearch:mainfrom
AiwendilInTheWoods:fix/cron-media-send-empty-timeout-reason

Conversation

@AiwendilInTheWoods

Copy link
Copy Markdown
Contributor

Problem

When a cron job's media delivery fails on a timeout, the log records no cause at all:

WARNING cron.scheduler: Job 'abc123': failed to send media /path/to/file.mp3:

Note the trailing colon with nothing after it. The operator cannot tell whether this was a timeout, an auth failure, a size rejection, or a network error — and no traceback is emitted on this path either.

Root cause

cron/scheduler.py bounds the send with future.result(timeout=30) and re-raises TimeoutError into the generic handler, which formats the exception with %s:

try:
    result = future.result(timeout=30)
except TimeoutError:
    future.cancel()
    raise                                   # -> outer handler
...
except Exception as e:
    logger.warning("Job '%s': failed to send media %s: %s", job.get("id", "?"), media_path, e)

TimeoutError is raised with no arguments, and str(TimeoutError()) is the empty string. So the single most likely failure on this path is precisely the one that erases itself from the log.

>>> import concurrent.futures
>>> repr(str(concurrent.futures.TimeoutError()))
"''"
>>> "failed to send media %s: %s" % ("/path/file.mp3", concurrent.futures.TimeoutError())
'failed to send media /path/file.mp3: '

Fix

Fall back to the exception class name when str(e) is empty, so an argument-less exception still identifies itself. Messages that do exist are unchanged.

Related

Same defect class as #63357 (computer_use: empty TimeoutError surfaces as blank "capture failed: "), which is still open — this is the cron/scheduler.py instance of the same pattern.

Scope

Logging only. No behaviour change, no new configuration.

How it was found

A ~7.4 MB audio attachment built correctly and then failed to deliver in production. The log line was empty by construction, so identifying the cause required reading the source rather than the journal.

TimeoutError carries no message and str(TimeoutError()) is the empty
string, so the media-send warning rendered with nothing after the colon.
Fall back to the exception class name when str(e) is empty.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 16, 2026
teknium1 added a commit that referenced this pull request Aug 18, 2026
Follow-up on the salvaged commits from PRs #87965 and #87967
(@AiwendilInTheWoods):

- Promote the media-send timeout to the standard resolution pattern:
  HERMES_CRON_MEDIA_SEND_TIMEOUT env var, then
  cron.media_send_timeout_seconds in config.yaml, then 300s default
  (mirrors script_timeout_seconds; .env stays secrets-only).
- Register the config key in DEFAULT_CONFIG and document both surfaces
  (environment-variables reference + cron user guide).
- Fold the empty-str() exception fallback into the error string recorded
  in delivery_errors (post-#88631 the reason reaches the run status, not
  just the log line).
- Tests: timeout resolution precedence + TimeoutError reason fallback.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #88787 (merge commit bc76f62) — your commit landed with authorship preserved, together with your #87967 timeout work. A follow-up commit folds the class-name fallback into the delivery error recorded on the run status as well (post-#88631 those reasons surface in last_error, not just the log line), with regression tests for the empty-str(TimeoutError) case. Thanks for catching the trailing-colon-nothing log!

@teknium1 teknium1 closed this Aug 18, 2026
lisajlau pushed a commit to lisajlau/hermes-agent that referenced this pull request Aug 20, 2026
Follow-up on the salvaged commits from PRs NousResearch#87965 and NousResearch#87967
(@AiwendilInTheWoods):

- Promote the media-send timeout to the standard resolution pattern:
  HERMES_CRON_MEDIA_SEND_TIMEOUT env var, then
  cron.media_send_timeout_seconds in config.yaml, then 300s default
  (mirrors script_timeout_seconds; .env stays secrets-only).
- Register the config key in DEFAULT_CONFIG and document both surfaces
  (environment-variables reference + cron user guide).
- Fold the empty-str() exception fallback into the error string recorded
  in delivery_errors (post-NousResearch#88631 the reason reaches the run status, not
  just the log line).
- Tests: timeout resolution precedence + TimeoutError reason fallback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants