-
Notifications
You must be signed in to change notification settings - Fork 219
chore: improve error handling for templates #1687
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
Conversation
1f2deba to
f54100c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54100c1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/orchestrator/internal/template/server/create_template.go
Outdated
Show resolved
Hide resolved
| // If it's a timeout context, wrap it as a user error | ||
| if errors.Is(err, context.DeadlineExceeded) { | ||
| return phases.NewPhaseBuildError(phases.PhaseMeta{}, ErrTimeout) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal errors masked when context is also canceled
Medium Severity
When an internal error occurs and the context is also canceled, the defer in builder.go joins them with errors.Join(e, ctx.Err()). Then WrapContextAsUserError checks errors.Is(err, context.Canceled) which finds the context error in the joined error and returns a new PhaseBuildError wrapping ErrCanceled, discarding the original internal error. This causes internal errors to be miscategorized as BuildResultUserError in metrics instead of BuildResultInternalError, undermining the PR's goal of improving error tracking and regression monitoring.
Improves the template building error handling by not exposing internal server errors, simplifying user errors and adding tracking for the error types. This should help us improve tracking, alerting and regression monitoring.
Note
Improves reliability and observability of template builds with structured error handling and metrics.
builderrorspackage: classifies errors as user vs internal, wraps context cancellations/timeouts as user errors, and returns generic internal messages viaUnwrapUserErrorRecordBuildResultnow tracksresultassuccess|user_error|internal_error; build flow updated to set proper result typeNewPhaseBuildErrornow acceptsPhaseMeta; propagate metadata through base/steps/finalize and rootfs creation; error wrapping standardizedErrCanceledmessage; panic handling avoids leaking internals; removed cache-level cancelled reason constantWritten by Cursor Bugbot for commit 9583660. This will update automatically on new commits. Configure here.