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

fix(orchestrator): return 408 on timeout #3537

Merged
merged 4 commits into from
Feb 24, 2025

Conversation

bodinsamuel
Copy link
Collaborator

Changes

  • return 408 on timeout instead of 500
    To reduce the number of identified error in the logs

@bodinsamuel bodinsamuel self-assigned this Feb 18, 2025
@bodinsamuel bodinsamuel requested review from khaliqgant and a team February 18, 2025 14:55
@bodinsamuel bodinsamuel requested a review from TBonnin February 24, 2025 10:53
@@ -51,7 +51,7 @@ const handler = (scheduler: Scheduler, eventEmitter: EventEmitter) => {
cleanupAndRespond((res) => res.status(200).json({ state: completedTask.state, output: completedTask.output }));
};
const timeout = setTimeout(() => {
cleanupAndRespond((res) => res.status(500).send({ error: { code: 'timeout', message: 'Long polling timeout' } }));
cleanupAndRespond((res) => res.status(408).send({ error: { code: 'timeout', message: 'Long polling timeout' } }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine, the status code is not used. The client logic relies on the error field in the content.

const getOutput = await this.routeFetch(getOutputRoute, {
retryConfig: {
maxAttempts: 1000,
delayMs: 100,
retryIf: (res) => 'error' in res && Date.now() < retryUntil
}
})({ params: { taskId }, query: { longPolling: 30_000 } });
if ('error' in getOutput) {

@bodinsamuel bodinsamuel enabled auto-merge (squash) February 24, 2025 15:06
@bodinsamuel bodinsamuel merged commit d9c84bf into master Feb 24, 2025
16 checks passed
@bodinsamuel bodinsamuel deleted the sam/25_02_18/fix/orch-timeout branch February 24, 2025 15:11
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.

3 participants