Skip to content

fix: log hook and dispatch errors instead of exiting - #15

Closed
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/hook-errors
Closed

fix: log hook and dispatch errors instead of exiting#15
usefulish wants to merge 1 commit into
desbma:masterfrom
usefulish:pr/hook-errors

Conversation

@usefulish

Copy link
Copy Markdown
Contributor

Every hook::run(...)? and folder try_into()? in main's dispatch propagates to main() and exits the daemon: a typo in a hook command path kills stfed on the first triggered event, and a Syncthing folder whose path doesn't resolve locally (e.g. Syncthing running in Docker reporting container paths) kills it on the first event for that folder — even if no hook references it.

Log hook run errors and keep dispatching; folder resolution errors are logged by the caller and no longer stop the event loop. Dispatch is extracted from main() into dispatch_event so the behavior is testable.

Red-green test: failing_hook_does_not_interrupt_dispatch (a failing hook must not prevent the following hook from running — on master the equivalent flow returns an error that exits the daemon), plus unresolvable_folder_path_is_an_error documenting that a bad folder path is a recoverable error.

Follow-up ready once this lands: warn once per unresolvable folder instead of logging an error on every event for it.

🤖 Generated with Claude Code

Any hook run error (typo in command path, resource exhaustion...) or
failure to canonicalize an event's folder path propagated to main and
killed the daemon on the first triggered event. Log these errors and
keep the event loop running. Event dispatch is extracted into a
function so the behavior is testable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
desbma added a commit that referenced this pull request Jul 15, 2026
@desbma

desbma commented Jul 15, 2026

Copy link
Copy Markdown
Owner

Good catch, on both counts: the unresolvable folder path is the worst one, since with the provided systemd unit it becomes a crash loop that also drops the in-memory event cursor, silently losing events for healthy folders on each restart. I fixed it differently in 3adfbec, on top of the reworks that conflicted with this PR: spawn failures are logged inside hook::run instead of wrapping every caller, and the folder path is resolved once before dispatch, logging and skipping the event when it fails.

One deliberate difference: a reaper channel send failure stays fatal, where run_hook here logged it and kept going. A dead reaper means children are never waited on, so continuing would just accumulate zombies while logging on every hook run; there is now a test pinning that this remains an error. Closing in favor of 3adfbec, thanks for the detailed report.

If you have more fixes coming please say so, otherwise I will tag a new release in a few days.

@desbma desbma closed this Jul 15, 2026
@usefulish

Copy link
Copy Markdown
Contributor Author

Nothing blocking a release from my side — the reworks look good, and the crash-loop-drops-the-cursor consequence you spotted is nastier than what I reported. Two minor notes, take them or leave them:

  • A folder whose path never resolves locally (e.g. Syncthing in Docker reporting container paths, with no matching host path) now logs an error on every event for it. A once-per-folder dedup would quiet that; happy to send a small PR after the release if you want it.
  • Observation, not a bug in stfed: at debug log level, ureq logs full request preludes, which includes the X-API-Key header. Might be worth a note in the README for anyone sharing debug logs.

Thanks for the quick turnaround on all of these.

🤖 Generated with Claude Code

@desbma

desbma commented Jul 15, 2026

Copy link
Copy Markdown
Owner

On the repeated error: I'd rather not special-case this. A Syncthing instance reporting paths that don't exist on the host stfed runs on is a Docker deployment mismatch, to be fixed on that side. A once-per-folder dedup would also quiet the case where a hooked folder loses its mount mid-run, which is exactly when a repeating error is the signal you want.

On the API key: with the pinned ureq it is not logged at debug level — ureq logs headers through an allowlist, so X-API-Key gets redacted (the N HEADERS ARE REDACTED notice). What you saw was the trace-level raw prelude dump from ureq-proto (x-api-key: in the hex rows). Trace now requires an explicit RUST_LOG=trace: a9173f2 sets default levels (debug for debug builds, info for release) — not prompted by your comment, it's a default I usually use, but it does mean the dump no longer shows up unasked. Raw wire traffic is expected at trace level, so I'll leave that as is too.

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