From cc3b6c25aa60e1ea6bee1cc20a456e581d944ee4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Wed, 27 Oct 2021 19:08:59 +0200 Subject: [PATCH] make timeout and frequency mandatory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .../src/tasks/PluginTaskManagerImpl.test.ts | 10 +++- .../src/tasks/PluginTaskManagerImpl.ts | 4 +- .../src/tasks/TaskManager.test.ts | 1 + .../src/tasks/TaskWorker.test.ts | 52 +++++++------------ .../backend-tasks/src/tasks/TaskWorker.ts | 14 ----- packages/backend-tasks/src/tasks/types.ts | 6 +-- 6 files changed, 34 insertions(+), 53 deletions(-) diff --git a/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.test.ts b/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.test.ts index 1d1c89cb6b..193dd2a95d 100644 --- a/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.test.ts +++ b/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.test.ts @@ -16,6 +16,7 @@ import { getVoidLogger } from '@backstage/backend-common'; import { TestDatabaseId, TestDatabases } from '@backstage/backend-test-utils'; +import { Duration } from 'luxon'; import waitForExpect from 'wait-for-expect'; import { migrateBackendTasks } from '../database/migrateBackendTasks'; import { PluginTaskManagerImpl } from './PluginTaskManagerImpl'; @@ -44,7 +45,14 @@ describe('PluginTaskManagerImpl', () => { const { manager } = await init(databaseId); const fn = jest.fn(); - const { unschedule } = await manager.scheduleTask('task1', {}, fn); + const { unschedule } = await manager.scheduleTask( + 'task1', + { + timeout: Duration.fromMillis(5000), + frequency: Duration.fromMillis(5000), + }, + fn, + ); await waitForExpect(() => { expect(fn).toBeCalled(); diff --git a/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.ts b/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.ts index 7859f4ee23..1b890a69c8 100644 --- a/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.ts +++ b/packages/backend-tasks/src/tasks/PluginTaskManagerImpl.ts @@ -42,8 +42,8 @@ export class PluginTaskManagerImpl implements PluginTaskManager { await task.start({ version: 1, initialDelayDuration: options.initialDelay?.toISO(), - recurringAtMostEveryDuration: options.frequency?.toISO(), - timeoutAfterDuration: options.timeout?.toISO(), + recurringAtMostEveryDuration: options.frequency.toISO(), + timeoutAfterDuration: options.timeout.toISO(), }); return { diff --git a/packages/backend-tasks/src/tasks/TaskManager.test.ts b/packages/backend-tasks/src/tasks/TaskManager.test.ts index cf6e808ee3..002db0b3de 100644 --- a/packages/backend-tasks/src/tasks/TaskManager.test.ts +++ b/packages/backend-tasks/src/tasks/TaskManager.test.ts @@ -47,6 +47,7 @@ describe('TaskManager', () => { 'task1', { timeout: Duration.fromMillis(5000), + frequency: Duration.fromMillis(5000), }, () => {}, ); diff --git a/packages/backend-tasks/src/tasks/TaskWorker.test.ts b/packages/backend-tasks/src/tasks/TaskWorker.test.ts index 5e0152f776..2f977ca639 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.test.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.test.ts @@ -34,7 +34,7 @@ describe('TaskWorker', () => { }); it.each(databases.eachSupportedId())( - 'can run a single task to completion, %p', + 'goes through the expected states, %p', async databaseId => { const knex = await databases.init(databaseId); await migrateBackendTasks(knex); @@ -44,32 +44,9 @@ describe('TaskWorker', () => { ); const settings: TaskSettingsV1 = { version: 1, - }; - - const worker = new TaskWorker('task1', fn, knex, logger); - await worker.start(settings); - - waitForExpect(() => { - expect(fn).toBeCalledTimes(1); - }); - }, - 60_000, - ); - - it.each(databases.eachSupportedId())( - 'goes through the expected states for a single run, %p', - async databaseId => { - const knex = await databases.init(databaseId); - await migrateBackendTasks(knex); - - const fn = jest.fn( - async () => new Promise(resolve => setTimeout(resolve, 50)), - ); - const settings: TaskSettingsV1 = { - version: 1, - initialDelayDuration: Duration.fromObject({ seconds: 1 }).toISO(), - recurringAtMostEveryDuration: undefined, - timeoutAfterDuration: undefined, + initialDelayDuration: Duration.fromMillis(1000).toISO(), + recurringAtMostEveryDuration: Duration.fromMillis(2000).toISO(), + timeoutAfterDuration: Duration.fromMillis(60000).toISO(), }; const worker = new TaskWorker('task1', fn, knex, logger); @@ -87,6 +64,8 @@ describe('TaskWorker', () => { expect(JSON.parse(row.settings_json)).toEqual({ version: 1, initialDelayDuration: 'PT1S', + recurringAtMostEveryDuration: 'PT2S', + timeoutAfterDuration: 'PT60S', }); await expect(worker.findReadyTask()).resolves.toEqual({ @@ -117,7 +96,7 @@ describe('TaskWorker', () => { id: 'task1', current_run_ticket: 'ticket', current_run_started_at: expect.anything(), - current_run_expires_at: null, + current_run_expires_at: expect.anything(), }), ); @@ -126,7 +105,14 @@ describe('TaskWorker', () => { ); row = (await knex(DB_TASKS_TABLE))[0]; - expect(row).toBeUndefined(); + expect(row).toEqual( + expect.objectContaining({ + id: 'task1', + current_run_ticket: null, + current_run_started_at: null, + current_run_expires_at: null, + }), + ); }, 60_000, ); @@ -142,7 +128,7 @@ describe('TaskWorker', () => { version: 1, initialDelayDuration: undefined, recurringAtMostEveryDuration: Duration.fromMillis(0).toISO(), - timeoutAfterDuration: undefined, + timeoutAfterDuration: Duration.fromMillis(60000).toISO(), }; const worker = new TaskWorker('task1', fn, knex, logger); @@ -167,6 +153,7 @@ describe('TaskWorker', () => { const settings: TaskSettingsV1 = { version: 1, recurringAtMostEveryDuration: Duration.fromMillis(0).toISO(), + timeoutAfterDuration: Duration.fromMillis(60000).toISO(), }; const worker = new TaskWorker('task1', fn, knex, logger); @@ -183,7 +170,7 @@ describe('TaskWorker', () => { id: 'task1', current_run_ticket: 'ticket', current_run_started_at: expect.anything(), - current_run_expires_at: null, + current_run_expires_at: expect.anything(), }), ); @@ -201,7 +188,7 @@ describe('TaskWorker', () => { id: 'task1', current_run_ticket: 'stolen', current_run_started_at: expect.anything(), - current_run_expires_at: null, + current_run_expires_at: expect.anything(), }), ); }, @@ -218,6 +205,7 @@ describe('TaskWorker', () => { const settings: TaskSettingsV1 = { version: 1, recurringAtMostEveryDuration: Duration.fromMillis(0).toISO(), + timeoutAfterDuration: Duration.fromMillis(60000).toISO(), }; const worker1 = new TaskWorker('task1', fn, knex, logger); diff --git a/packages/backend-tasks/src/tasks/TaskWorker.ts b/packages/backend-tasks/src/tasks/TaskWorker.ts index dc48f843aa..881a013360 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.ts @@ -68,9 +68,6 @@ export class TaskWorker { if (runResult.result === 'abort') { break; } - if (!settings.recurringAtMostEveryDuration) { - break; - } await this.sleep(WORK_CHECK_FREQUENCY); } @@ -240,17 +237,6 @@ export class TaskWorker { ): Promise { const { recurringAtMostEveryDuration } = settings; - // If this is not a recurring task, and we still have the current run - // ticket, delete it from the table - if (recurringAtMostEveryDuration === undefined) { - const rows = await this.knex(DB_TASKS_TABLE) - .where('id', '=', this.taskId) - .where('current_run_ticket', '=', ticket) - .delete(); - - return rows === 1; - } - // We make an effort to keep the datetime calculations in the database // layer, making sure to not have to perform conversions back and forth and // leaning on the database as a central clock source diff --git a/packages/backend-tasks/src/tasks/types.ts b/packages/backend-tasks/src/tasks/types.ts index eed652093d..989414ec12 100644 --- a/packages/backend-tasks/src/tasks/types.ts +++ b/packages/backend-tasks/src/tasks/types.ts @@ -31,7 +31,7 @@ export interface TaskOptions { * If no value is given for this field then there is no timeout. This is * potentially dangerous. */ - timeout?: Duration; + timeout: Duration; /** * The amount of time that should pass between task invocation starts. @@ -48,7 +48,7 @@ export interface TaskOptions { * If no value is given for this field then the task will only be invoked * once (on any worker) and then unscheduled automatically. */ - frequency?: Duration; + frequency: Duration; /** * The amount of time that should pass before the first invocation happens. @@ -107,11 +107,9 @@ export const taskSettingsV1Schema = z.object({ .refine(isValidOptionalDurationString, { message: 'Invalid duration' }), recurringAtMostEveryDuration: z .string() - .optional() .refine(isValidOptionalDurationString, { message: 'Invalid duration' }), timeoutAfterDuration: z .string() - .optional() .refine(isValidOptionalDurationString, { message: 'Invalid duration' }), });