From 6d99b16b287a14c3122f2b6594ef2f4204a1c78a Mon Sep 17 00:00:00 2001 From: Yobannys Cabrera Gonzalez Date: Tue, 5 Sep 2023 15:29:57 -0400 Subject: [PATCH 1/3] Adding `next_run_start_at` to the merge conflict, when scheduler updates Signed-off-by: Yobannys Cabrera Gonzalez --- .changeset/real-turkeys-report.md | 5 +++++ packages/backend-tasks/src/tasks/TaskWorker.ts | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/real-turkeys-report.md diff --git a/.changeset/real-turkeys-report.md b/.changeset/real-turkeys-report.md new file mode 100644 index 0000000000..7e13f3a937 --- /dev/null +++ b/.changeset/real-turkeys-report.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-tasks': patch +--- + +Fixed bug in backend TaskWorker, adding `next_run_start_at` as a merge conflict when scheduler changes diff --git a/packages/backend-tasks/src/tasks/TaskWorker.ts b/packages/backend-tasks/src/tasks/TaskWorker.ts index a03d1ec058..f1acb4ed19 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.ts @@ -199,7 +199,7 @@ export class TaskWorker { next_run_start_at: startAt, }) .onConflict('id') - .merge(['settings_json']); + .merge(['settings_json', 'next_run_start_at']); } /** From 8fd91547cd0b9545c2de2552f753fc9846ea01c6 Mon Sep 17 00:00:00 2001 From: Yobannys Cabrera Gonzalez Date: Tue, 12 Sep 2023 09:27:05 -0400 Subject: [PATCH 2/3] Adding the least between current and previous `next_run_start_at` to merge conflict when scheduler updates Signed-off-by: Yobannys Cabrera Gonzalez --- .changeset/dry-yaks-type.md | 5 +++ .../src/tasks/TaskWorker.test.ts | 37 +++++++++++++++++++ .../backend-tasks/src/tasks/TaskWorker.ts | 15 +++++++- 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 .changeset/dry-yaks-type.md diff --git a/.changeset/dry-yaks-type.md b/.changeset/dry-yaks-type.md new file mode 100644 index 0000000000..33d053a154 --- /dev/null +++ b/.changeset/dry-yaks-type.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-tasks': patch +--- + +Fixed bug in backend TaskWorker, 'next_run_start_at' will be always the least between schedule changes. diff --git a/packages/backend-tasks/src/tasks/TaskWorker.test.ts b/packages/backend-tasks/src/tasks/TaskWorker.test.ts index 70be2bf415..38bb2f7e99 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.test.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.test.ts @@ -332,4 +332,41 @@ describe('TaskWorker', () => { expect(fn1.mock.calls.length).toBeGreaterThan(before); }, ); + + it.each(databases.eachSupportedId())( + 'next_run_start_at is always the min between schedule changes, %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: TaskSettingsV2 = { + version: 2, + cadence: '*/15 * * * *', + initialDelayDuration: 'PT2M', + timeoutAfterDuration: 'PT1M', + }; + + const worker = new TaskWorker('task99', fn, knex, logger); + await worker.persistTask(settings); + + const settings2 = { + ...settings, + cadence: '*/2 * * * *', + initialDelayDuration: 'PT1M', + }; + await worker.persistTask(settings2); + + const row2 = (await knex(DB_TASKS_TABLE))[0]; + + const settings3 = { ...settings }; + await worker.persistTask(settings3); + + const row3 = (await knex(DB_TASKS_TABLE))[0]; + + expect(row3.next_run_start_at).toBe(row2.next_run_start_at); + }, + ); }); diff --git a/packages/backend-tasks/src/tasks/TaskWorker.ts b/packages/backend-tasks/src/tasks/TaskWorker.ts index f1acb4ed19..6ede00c590 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.ts @@ -190,6 +190,11 @@ export class TaskWorker { this.logger.debug(`task: ${this.taskId} configured to run at: ${startAt}`); + const start_at_previuos = this.knex(DB_TASKS_TABLE) + .where('id', '=', this.taskId) + .select('next_run_start_at') + .first(); + // It's OK if the task already exists; if it does, just replace its // settings with the new value and start the loop as usual. await this.knex(DB_TASKS_TABLE) @@ -199,7 +204,15 @@ export class TaskWorker { next_run_start_at: startAt, }) .onConflict('id') - .merge(['settings_json', 'next_run_start_at']); + .merge({ + settings_json: JSON.stringify(settings), + next_run_start_at: this.knex.raw('CASE WHEN ? < ? THEN ? ELSE ? END', [ + start_at_previuos, + startAt, + start_at_previuos, + startAt, + ]), + }); } /** From bf431f81c42f3e424f8bf09ea04d05c51ca82756 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Thu, 14 Sep 2023 09:23:19 +0200 Subject: [PATCH 3/3] fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/dry-yaks-type.md | 5 ++- .changeset/real-turkeys-report.md | 5 --- .../src/tasks/TaskWorker.test.ts | 11 ++++-- .../backend-tasks/src/tasks/TaskWorker.ts | 39 ++++++++++++------- 4 files changed, 36 insertions(+), 24 deletions(-) delete mode 100644 .changeset/real-turkeys-report.md diff --git a/.changeset/dry-yaks-type.md b/.changeset/dry-yaks-type.md index 33d053a154..3e9cdef474 100644 --- a/.changeset/dry-yaks-type.md +++ b/.changeset/dry-yaks-type.md @@ -2,4 +2,7 @@ '@backstage/backend-tasks': patch --- -Fixed bug in backend TaskWorker, 'next_run_start_at' will be always the least between schedule changes. +When starting a task that existed before, with a faster schedule than it +previously had, the task will now correctly obey the faster schedule +immediately. Before this fix, the new schedule was only obeyed after the next +pending (according to the old schedule) run had completed. diff --git a/.changeset/real-turkeys-report.md b/.changeset/real-turkeys-report.md deleted file mode 100644 index 7e13f3a937..0000000000 --- a/.changeset/real-turkeys-report.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@backstage/backend-tasks': patch ---- - -Fixed bug in backend TaskWorker, adding `next_run_start_at` as a merge conflict when scheduler changes diff --git a/packages/backend-tasks/src/tasks/TaskWorker.test.ts b/packages/backend-tasks/src/tasks/TaskWorker.test.ts index 38bb2f7e99..71d02274e2 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.test.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.test.ts @@ -330,6 +330,8 @@ describe('TaskWorker', () => { const before = fn1.mock.calls.length; await promise2; expect(fn1.mock.calls.length).toBeGreaterThan(before); + + await knex.destroy(); }, ); @@ -351,6 +353,7 @@ describe('TaskWorker', () => { const worker = new TaskWorker('task99', fn, knex, logger); await worker.persistTask(settings); + const row1 = (await knex(DB_TASKS_TABLE))[0]; const settings2 = { ...settings, @@ -358,15 +361,17 @@ describe('TaskWorker', () => { initialDelayDuration: 'PT1M', }; await worker.persistTask(settings2); - const row2 = (await knex(DB_TASKS_TABLE))[0]; + expect(row2.next_run_start_at).not.toStrictEqual(row1.next_run_start_at); + const settings3 = { ...settings }; await worker.persistTask(settings3); - const row3 = (await knex(DB_TASKS_TABLE))[0]; - expect(row3.next_run_start_at).toBe(row2.next_run_start_at); + expect(row3.next_run_start_at).toStrictEqual(row2.next_run_start_at); + + await knex.destroy(); }, ); }); diff --git a/packages/backend-tasks/src/tasks/TaskWorker.ts b/packages/backend-tasks/src/tasks/TaskWorker.ts index 6ede00c590..c1007b4964 100644 --- a/packages/backend-tasks/src/tasks/TaskWorker.ts +++ b/packages/backend-tasks/src/tasks/TaskWorker.ts @@ -190,29 +190,38 @@ export class TaskWorker { this.logger.debug(`task: ${this.taskId} configured to run at: ${startAt}`); - const start_at_previuos = this.knex(DB_TASKS_TABLE) - .where('id', '=', this.taskId) - .select('next_run_start_at') - .first(); - // It's OK if the task already exists; if it does, just replace its // settings with the new value and start the loop as usual. + const settingsJson = JSON.stringify(settings); await this.knex(DB_TASKS_TABLE) .insert({ id: this.taskId, - settings_json: JSON.stringify(settings), + settings_json: settingsJson, next_run_start_at: startAt, }) .onConflict('id') - .merge({ - settings_json: JSON.stringify(settings), - next_run_start_at: this.knex.raw('CASE WHEN ? < ? THEN ? ELSE ? END', [ - start_at_previuos, - startAt, - start_at_previuos, - startAt, - ]), - }); + .merge( + this.knex.client.config.client.includes('mysql') + ? { + settings_json: settingsJson, + next_run_start_at: this.knex.raw( + `CASE WHEN ?? < ?? THEN ?? ELSE ?? END`, + [startAt, 'next_run_start_at', startAt, 'next_run_start_at'], + ), + } + : { + settings_json: this.knex.ref('excluded.settings_json'), + next_run_start_at: this.knex.raw( + `CASE WHEN ?? < ?? THEN ?? ELSE ?? END`, + [ + 'excluded.next_run_start_at', + `${DB_TASKS_TABLE}.next_run_start_at`, + 'excluded.next_run_start_at', + `${DB_TASKS_TABLE}.next_run_start_at`, + ], + ), + }, + ); } /**