fix: atomic claim prevents scheduled tasks from executing twice (#657)

* fix: atomic claim prevents scheduled tasks from executing twice (#138)

Replace the two-phase getDueTasks() + deferred updateTaskAfterRun() with
an atomic SQLite transaction (claimDueTasks) that advances next_run
BEFORE dispatching tasks to the queue. This eliminates the race window
where subsequent scheduler polls re-discover in-progress tasks.

Key changes:
- claimDueTasks(): SELECT + UPDATE in a single db.transaction(), so no
  poll can read stale next_run values. Once-tasks get next_run=NULL;
  recurring tasks get next_run advanced to the future.
- computeNextRun(): anchors interval tasks to the scheduled time (not
  Date.now()) to prevent cumulative drift. Includes a while-loop to
  skip missed intervals and a guard against invalid interval values.
- updateTaskAfterRun(): simplified to only record last_run/last_result
  since next_run is already handled by the claim.

Closes #138, #211, #300, #578

Co-authored-by: @taslim (PR #601)
Co-authored-by: @baijunjie (Issue #138)
Co-authored-by: @Michaelliv (Issue #300)
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* style: apply prettier formatting

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: track running task ID in GroupQueue to prevent duplicate execution (#138)

Previous commits implemented an "atomic claim" approach (claimDueTasks)
that advanced next_run before execution. Per Gavriel's review, this
solved the symptom at the wrong layer and introduced crash-recovery
risks for once-tasks.

This commit reverts claimDueTasks and instead fixes the actual bug:
GroupQueue.enqueueTask() only checked pendingTasks for duplicates, but
running tasks had already been shifted out. Adding runningTaskId to
GroupState closes that gap with a 3-line fix at the correct layer.

The computeNextRun() drift fix is retained, applied post-execution
where it belongs.

Closes #138, #211, #300, #578

Co-authored-by: @taslim (PR #601)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add changelog entry for scheduler duplicate fix

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: add contributors for scheduler race condition fix

Co-Authored-By: Taslim <9999802+taslim@users.noreply.github.com>
Co-Authored-By: BaiJunjie <7956480+baijunjie@users.noreply.github.com>
Co-Authored-By: Michael <13676242+Michaelliv@users.noreply.github.com>
Co-Authored-By: Kyle Zhike Chen <3477852+kk17@users.noreply.github.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: gavrielc <gabicohen22@yahoo.com>
Co-authored-by: Taslim <9999802+taslim@users.noreply.github.com>
Co-authored-by: BaiJunjie <7956480+baijunjie@users.noreply.github.com>
Co-authored-by: Michael <13676242+Michaelliv@users.noreply.github.com>
Co-authored-by: Kyle Zhike Chen <3477852+kk17@users.noreply.github.com>
This commit is contained in:
Gabi Simons
2026-03-04 16:23:29 +02:00
committed by GitHub
parent 03f792bfce
commit f794185c21
6 changed files with 167 additions and 13 deletions

View File

@@ -3,6 +3,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { _initTestDatabase, createTask, getTaskById } from './db.js';
import {
_resetSchedulerLoopForTests,
computeNextRun,
startSchedulerLoop,
} from './task-scheduler.js';
@@ -50,4 +51,79 @@ describe('task scheduler', () => {
const task = getTaskById('task-invalid-folder');
expect(task?.status).toBe('paused');
});
it('computeNextRun anchors interval tasks to scheduled time to prevent drift', () => {
const scheduledTime = new Date(Date.now() - 2000).toISOString(); // 2s ago
const task = {
id: 'drift-test',
group_folder: 'test',
chat_jid: 'test@g.us',
prompt: 'test',
schedule_type: 'interval' as const,
schedule_value: '60000', // 1 minute
context_mode: 'isolated' as const,
next_run: scheduledTime,
last_run: null,
last_result: null,
status: 'active' as const,
created_at: '2026-01-01T00:00:00.000Z',
};
const nextRun = computeNextRun(task);
expect(nextRun).not.toBeNull();
// Should be anchored to scheduledTime + 60s, NOT Date.now() + 60s
const expected = new Date(scheduledTime).getTime() + 60000;
expect(new Date(nextRun!).getTime()).toBe(expected);
});
it('computeNextRun returns null for once-tasks', () => {
const task = {
id: 'once-test',
group_folder: 'test',
chat_jid: 'test@g.us',
prompt: 'test',
schedule_type: 'once' as const,
schedule_value: '2026-01-01T00:00:00.000Z',
context_mode: 'isolated' as const,
next_run: new Date(Date.now() - 1000).toISOString(),
last_run: null,
last_result: null,
status: 'active' as const,
created_at: '2026-01-01T00:00:00.000Z',
};
expect(computeNextRun(task)).toBeNull();
});
it('computeNextRun skips missed intervals without infinite loop', () => {
// Task was due 10 intervals ago (missed)
const ms = 60000;
const missedBy = ms * 10;
const scheduledTime = new Date(Date.now() - missedBy).toISOString();
const task = {
id: 'skip-test',
group_folder: 'test',
chat_jid: 'test@g.us',
prompt: 'test',
schedule_type: 'interval' as const,
schedule_value: String(ms),
context_mode: 'isolated' as const,
next_run: scheduledTime,
last_run: null,
last_result: null,
status: 'active' as const,
created_at: '2026-01-01T00:00:00.000Z',
};
const nextRun = computeNextRun(task);
expect(nextRun).not.toBeNull();
// Must be in the future
expect(new Date(nextRun!).getTime()).toBeGreaterThan(Date.now());
// Must be aligned to the original schedule grid
const offset =
(new Date(nextRun!).getTime() - new Date(scheduledTime).getTime()) % ms;
expect(offset).toBe(0);
});
});