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: do not wait for on-event script to finish when triggering them #3075

Merged
merged 1 commit into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix: do not wait for on-event script to finish when triggering them
on-event scripts don't have output so waiting for them to finish is not
necessary
Also modifying the max duration timeout to 5mins for on-event script
  • Loading branch information
TBonnin committed Nov 27, 2024
commit f83317f27f6018574f3f4f1bc9e07683c8675986
2 changes: 1 addition & 1 deletion packages/jobs/lib/execution/onEvent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export async function startOnEvent(task: TaskOnEvent): Promise<Result<void>> {
}

export async function handleOnEventSuccess({ nangoProps }: { nangoProps: NangoProps }): Promise<void> {
const content = `Webhook "${nangoProps.syncConfig.sync_name}" has been run successfully.`;
const content = `Script "${nangoProps.syncConfig.sync_name}" has been run successfully.`;
void bigQueryClient.insert({
executionType: 'on-event',
connectionId: nangoProps.connectionId,
Expand Down
15 changes: 13 additions & 2 deletions packages/orchestrator/lib/clients/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,17 +254,28 @@ export class OrchestratorClient {
return this.execute(schedulingProps);
}

public async executeOnEvent(props: ExecuteOnEventProps): Promise<ExecuteReturn> {
public async executeOnEvent(props: ExecuteOnEventProps): Promise<VoidReturn> {
const { args, ...rest } = props;
const schedulingProps = {
retry: { count: 0, max: 0 },
timeoutSettingsInSecs: {
createdToStarted: 30,
startedToCompleted: 5 * 60,
Copy link
Collaborator Author

@TBonnin TBonnin Nov 27, 2024

Choose a reason for hiding this comment

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

like all our task timeouts, this value is arbitrary. 5 mins is very generous imho

heartbeat: 99999
},
...rest,
args: {
...args,
type: 'on-event' as const
}
};
return this.execute(schedulingProps);
const res = await this.immediate(schedulingProps);
if (res.isErr()) {
return Err(res.error);
}
return Ok(undefined);
}

public async searchTasks({
ids,
groupKey,
Expand Down
2 changes: 0 additions & 2 deletions packages/server/lib/hooks/connection/on/connection-created.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,6 @@ export async function postConnectionCreation(
});
if (res.isErr()) {
await logCtx.failed();
} else {
await logCtx.success();
}
}
}
2 changes: 0 additions & 2 deletions packages/server/lib/hooks/connection/on/connection-deleted.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ export async function preConnectionDeletion({
});
if (res.isErr()) {
await logCtx.failed();
} else {
await logCtx.success();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see where you success() now?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's in that handleOnEventSuccess function, I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

indeed, might be that I must have missed it 👍🏻

}
}
}
2 changes: 1 addition & 1 deletion packages/shared/lib/clients/orchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export interface OrchestratorClientInterface {
recurring(props: RecurringProps): Promise<Result<{ scheduleId: string }>>;
executeAction(props: ExecuteActionProps): Promise<ExecuteReturn>;
executeWebhook(props: ExecuteWebhookProps): Promise<ExecuteReturn>;
executeOnEvent(props: ExecuteOnEventProps): Promise<ExecuteReturn>;
executeOnEvent(props: ExecuteOnEventProps): Promise<VoidReturn>;
executeSync(props: ExecuteSyncProps): Promise<VoidReturn>;
pauseSync({ scheduleName }: { scheduleName: string }): Promise<VoidReturn>;
unpauseSync({ scheduleName }: { scheduleName: string }): Promise<VoidReturn>;
Expand Down
Loading