Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If a component errors after starting the response, show that error #3041

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itowlson
Copy link
Collaborator

@itowlson itowlson commented Mar 10, 2025

Fixes #3035.

This makes little or no odds in non-streaming scenarios, where no guest code executes after the guest returns a response object (although SDK code may execute), and so any errors are likely to occur before the response is constructed let alone commenced.

However, in streaming scenarios, a guest may error (e.g. Rust panic) after the ResponseOutparam is set and its body taken, but before the response body is finished (or even started). The default Spin logging settings turn this into a silent failure, which makes it hard to know where to start. (In some cases a guest may be doing interesting things even after the response has completed, e.g. a Slack command handler doing work after the ack timeout, with intent to call back.)

This PR therefore bumps post-response guest errors to trace at Error level instead of Warning.

@itowlson itowlson requested review from lann and rylev March 10, 2025 00:12
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I'm unsure how I feel about this. Is guest code erroring an error from the perspective of the runtime? If we come from the perspective that the runtime is running untrusted code, an error in the guest is not really an error from the perspective of the runtime time.

That being said I understand the very practical need of showing errors when a guest errors.

Warning feels semantically like the right option here. Should we instead be showing warnings by default?

@lann
Copy link
Collaborator

lann commented Mar 10, 2025

I think error is appropriate - the guest code could be failing to do something semantically important like marking a message as "delivered" once the response is complete.

@rylev
Copy link
Collaborator

rylev commented Mar 10, 2025

I think I agree. We're always making the fundamental assumption in the trigger-http crate that the administrator of Spin cares about what the guest is actually doing. This isn't always the case in a spin compliant runtime, but I think it is the case here.

@itowlson
Copy link
Collaborator Author

@rylev It's a good point. The PR inadvertently takes a developer-centric point of view: I am building an app, it is not responding, but I am getting no feedback as to why. I guess maybe a better solution is to tell me to improve the way I structure their streaming code and my logging habits. But we report the error if a guest panics before starting the response (or at least that's my impression), so it seemed odd to have a different experience for an app that goes down before the body is complete.

I did consider "downgrade to a warning once the response body is complete" but 1. I couldn't find a way to detect that at the point the warning/error is reported and 2. if the guest is still doing work after completing the body then maybe the guest cares.

But you're right about the issue of guest errors cluttering up admin logs in other hosts, and the position of this code makes it high-effort for hosts such as SpinKube to replace. So maybe it's best to back this out, review the error-before-response behaviour, and document better guidance.

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.

HTTP streaming guest panic is not surfaced
3 participants