-
Notifications
You must be signed in to change notification settings - Fork 0
Fix audit findings (non-docs): bugs, spec-conformance, security #77
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
Changes from all commits
aa69780
a1bdd14
1efe6fe
bfb3e90
c97fa23
84c7207
f9df876
341c548
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -81,10 +81,35 @@ internal void OnResult(JobResultPayload payload) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| internal void OnError(JobErrorPayload payload) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If the job was rejected before acceptance (e.g. a server session.error for a duplicate | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // key or unavailable agent), the awaiter on Accepted must fault rather than hang forever. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // For a post-acceptance terminal error, Accepted is already resolved so this is a no-op. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _accepted.TrySetException(ToException(payload.Code, payload.Message, payload.Detail)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _terminal.TrySetResult(new JobResult(false, null, payload)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _events.Writer.TryComplete(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary>Map a wire error code to the most specific <see cref="ArcpException"/> subtype so | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// callers can <c>catch</c> on the concrete type (e.g. <see cref="DuplicateKeyException"/>).</summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| internal static ArcpException ToException(string code, string message, string? detail) => code switch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.DuplicateKey => new DuplicateKeyException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.AgentNotAvailable => new AgentNotAvailableException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.AgentVersionNotAvailable => new AgentVersionNotAvailableException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.LeaseSubsetViolation => new LeaseSubsetViolationException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.PermissionDenied => new PermissionDeniedException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.JobNotFound => new JobNotFoundException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.InvalidRequest => new InvalidRequestException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.Unauthenticated => new UnauthenticatedException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.BudgetExhausted => new BudgetExhaustedException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.LeaseExpired => new LeaseExpiredException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.ResumeWindowExpired => new ResumeWindowExpiredException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.HeartbeatLost => new HeartbeatLostException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.Timeout => new Arcp.Core.Errors.TimeoutException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ErrorCode.Cancelled => new CancelledException(message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ => new ArcpException(code, message, detail), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+92
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Map
Suggested fix ErrorCode.ResumeWindowExpired => new ResumeWindowExpiredException(message, detail),
ErrorCode.HeartbeatLost => new HeartbeatLostException(message, detail),
+ ErrorCode.InternalError => new InternalErrorException(message, detail),
ErrorCode.Timeout => new Arcp.Core.Errors.TimeoutException(message, detail),
ErrorCode.Cancelled => new CancelledException(message, detail),
_ => new ArcpException(code, message, detail),📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary>Events.</summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public async IAsyncEnumerable<JobEvent> Events([EnumeratorCancellation] CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Don’t fail every accepted job on a request-level
session.error.Now that
session.erroris the rejection path for pendingjob.submitandsession.list_jobs, this method will also callOnError(...)on every already-accepted handle in_handles. One bad submit or list request can therefore terminally fail unrelated running jobs even if the server keeps streaming their events/results. Split request-scoped rejection handling from truly session-fatal errors, or mark the session broken and stop further dispatch before you fail accepted handles.🤖 Prompt for AI Agents