fix(scheduled-tasks): prevent dual-server double dispatch of daily tasks (#2713)

* fix(scheduled-tasks): claim schedule occurrences across server instances Two OpenChamber servers sharing project config each armed timers and both dispatched the same daily/weekly/cron/once slot (#2710). Claim the occurrence in shared config under a cross-process write lock before creating a session. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com> * fix(scheduled-tasks): harden occurrence claim failure and lock ownership Address PR review blockers: release running-slot bookkeeping when claim throws, avoid silently dropping an armed occurrence after a due-slack sync, verify lock-file ownership on release, and cover real on-disk lock behavior. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com> * fix(scheduled-tasks): always release running slot on state-write failures Wrap runTask bookkeeping in finally so claim, manual-start, and completion lock timeouts cannot stuck-run a task; drop the diskNext claim guard that suppressed later occurrences; recover unparseable locks via mtime age. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com> * fix(scheduled-tasks): stop re-arming past nextRunAt and clear stuck running Only schedule future nextRunAt values so once-task losers and claim-failed paths cannot spin delay-0 retries. Clear past once nextRunAt on claim, and on completion-write failure retry terminal status so manual runNow still returns the session instead of a hard 500 with lastStatus stuck running. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com> * fix(scheduled-tasks): release write chain on lock acquire timeout withProjectWriteLock left the in-process promise chain pending when acquireProjectFileLock timed out, wedging every later project write and stranding runTask before finally. Always release the chain; surface persistError on run; record once claim failures in task state. Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com> --------- Co-authored-by: Serhii Dziupin <makeittech@users.noreply.github.com>
2026-08-13 16:14:42 +03:00
parent 86e6a2ae76
commit e99f6560be
20 changed files with 1436 additions and 117 deletions
@@ -254,6 +254,12 @@ const normalizeState = (value, fallback) => {
  const nextRunAt = typeof source.nextRunAt === 'number' && Number.isFinite(source.nextRunAt)
    ? Math.max(0, Math.round(source.nextRunAt))
    : undefined;
+  // Absolute ms of the schedule occurrence last claimed for dispatch. Used so
+  // two OpenChamber server instances sharing this config cannot both start a
+  // run for the same daily/weekly/cron/once slot (see issue #2710).
+  const lastScheduledFor = typeof source.lastScheduledFor === 'number' && Number.isFinite(source.lastScheduledFor)
+    ? Math.max(0, Math.round(source.lastScheduledFor))
+    : undefined;
  const lastSessionId = asNonEmptyString(source.lastSessionId);
  const lastErrorRaw = asNonEmptyString(source.lastError);
  const lastError = lastErrorRaw ? clampLength(lastErrorRaw, MAX_LAST_ERROR_LENGTH) : undefined;
@@ -269,6 +275,7 @@ const normalizeState = (value, fallback) => {
    ...(typeof lastRunAt === 'number' ? { lastRunAt } : {}),
    ...(typeof lastDurationMs === 'number' ? { lastDurationMs } : {}),
    ...(typeof nextRunAt === 'number' ? { nextRunAt } : {}),
+    ...(typeof lastScheduledFor === 'number' ? { lastScheduledFor } : {}),
    ...(lastSessionId ? { lastSessionId } : {}),
    ...(lastError ? { lastError } : {}),
  };
@@ -360,6 +367,9 @@ export const createProjectConfigRuntime = (deps) => {
    });

  const writeLocks = new Map();
+  const PROJECT_FILE_LOCK_WAIT_MS = 10_000;
+  const PROJECT_FILE_LOCK_STALE_MS = 60_000;
+  const PROJECT_FILE_LOCK_RETRY_MS = 20;

  const sanitizeProjectID = (projectID) => {
    const value = asNonEmptyString(projectID);
@@ -377,6 +387,104 @@ export const createProjectConfigRuntime = (deps) => {
    return path.join(projectsDirPath, `${safeProjectID}.json`);
  };

+  const isProcessAlive = (pid) => {
+    if (!Number.isInteger(pid) || pid <= 0) {
+      return false;
+    }
+    try {
+      process.kill(pid, 0);
+      return true;
+    } catch (error) {
+      // EPERM: process exists but belongs to another user — treat as alive.
+      return error?.code === 'EPERM';
+    }
+  };
+
+  /**
+   * Cross-process exclusive lock for a project config file.
+   * In-process chaining alone cannot serialize Electron (port 57123) and CLI
+   * serve (port 3000) writers that share the same on-disk projects dir.
+   */
+  const acquireProjectFileLock = async (projectID) => {
+    const configPath = resolveProjectConfigPath(projectID);
+    const lockPath = `${configPath}.lock`;
+    const startedAt = Date.now();
+
+    await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+
+    while (Date.now() - startedAt < PROJECT_FILE_LOCK_WAIT_MS) {
+      let handle;
+      try {
+        handle = await fsPromises.open(lockPath, 'wx');
+        const lockPayload = {
+          pid: process.pid,
+          at: Date.now(),
+        };
+        await handle.writeFile(JSON.stringify(lockPayload));
+        return {
+          release: async () => {
+            try {
+              await handle.close();
+            } catch {
+            }
+            // Only unlink if we still own the lock. A stale-recovery steal can
+            // replace the file; unlinking blindly would drop the new owner's lock.
+            try {
+              const raw = await fsPromises.readFile(lockPath, 'utf8');
+              const parsed = JSON.parse(raw);
+              if (Number(parsed?.pid) !== process.pid) {
+                return;
+              }
+              await fsPromises.unlink(lockPath);
+            } catch {
+            }
+          },
+        };
+      } catch (error) {
+        if (handle) {
+          try {
+            await handle.close();
+          } catch {
+          }
+        }
+        if (error?.code !== 'EEXIST') {
+          throw error;
+        }
+
+        try {
+          const raw = await fsPromises.readFile(lockPath, 'utf8');
+          const parsed = JSON.parse(raw);
+          const lockPid = Number(parsed?.pid);
+          const lockAt = Number(parsed?.at);
+          const staleByPid = Number.isInteger(lockPid) && lockPid > 0 && !isProcessAlive(lockPid);
+          const staleByAge = Number.isFinite(lockAt) && (Date.now() - lockAt) > PROJECT_FILE_LOCK_STALE_MS;
+          if (staleByPid || staleByAge || !Number.isInteger(lockPid)) {
+            await fsPromises.unlink(lockPath).catch(() => {});
+            continue;
+          }
+        } catch {
+          // Crash between open(wx) and writeFile (or a partial write) leaves an
+          // unparseable lock. Fall back to mtime age so recovery is not wedged.
+          try {
+            const stat = await fsPromises.stat(lockPath);
+            const mtimeMs = Number(stat?.mtimeMs);
+            if (Number.isFinite(mtimeMs) && (Date.now() - mtimeMs) > PROJECT_FILE_LOCK_STALE_MS) {
+              await fsPromises.unlink(lockPath).catch(() => {});
+              continue;
+            }
+          } catch {
+          }
+        }
+
+        await new Promise((resolve) => {
+          setTimeout(resolve, PROJECT_FILE_LOCK_RETRY_MS);
+        });
+      }
+    }
+
+    throw new Error(`timeout acquiring project config lock for ${projectID}`);
+  };
+
  const readRawProjectConfigFromDisk = async (projectID) => {
    const filePath = resolveProjectConfigPath(projectID);
    try {
@@ -443,8 +551,16 @@ export const createProjectConfigRuntime = (deps) => {
    writeLocks.set(key, chained);

    await previous;
+    // Acquire sits outside the mutate try — if it throws (10s lock timeout),
+    // we must still release the in-process chain or every later write for this
+    // project hangs forever on await previous.
    try {
-      return await mutate();
+      const fileLock = await acquireProjectFileLock(projectID);
+      try {
+        return await mutate();
+      } finally {
+        await fileLock.release();
+      }
    } finally {
      release();
      const current = writeLocks.get(key);
@@ -533,7 +649,7 @@ export const createProjectConfigRuntime = (deps) => {
      const current = await readProjectConfigFromDisk(projectID);
      const taskIndex = current.scheduledTasks.findIndex((task) => task.id === normalizedTaskID);
      if (taskIndex === -1) {
-        return { task: null, tasks: current.scheduledTasks };
+        return { task: null, tasks: current.scheduledTasks, updated: false };
      }

      const currentTask = current.scheduledTasks[taskIndex];
@@ -561,6 +677,68 @@ export const createProjectConfigRuntime = (deps) => {
      return {
        task: nextTask,
        tasks: nextTasks,
+        updated: true,
+      };
+    });
+  };
+
+  /**
+   * Conditionally patch task runtime state under the project write lock.
+   * `predicate(currentTask)` is evaluated after the latest on-disk read; when
+   * it returns false the write is skipped and `{ updated: false }` is returned.
+   * Used by the scheduled-tasks runtime to claim a single schedule occurrence
+   * across concurrent OpenChamber server instances.
+   */
+  const updateScheduledTaskStateIf = async (projectID, taskID, predicate, statePatch) => {
+    return withProjectWriteLock(projectID, async () => {
+      const normalizedTaskID = asNonEmptyString(taskID);
+      if (!normalizedTaskID) {
+        throw new Error('taskId is required');
+      }
+      if (typeof predicate !== 'function') {
+        throw new Error('predicate is required');
+      }
+
+      const current = await readProjectConfigFromDisk(projectID);
+      const taskIndex = current.scheduledTasks.findIndex((task) => task.id === normalizedTaskID);
+      if (taskIndex === -1) {
+        return { task: null, tasks: current.scheduledTasks, updated: false };
+      }
+
+      const currentTask = current.scheduledTasks[taskIndex];
+      if (!predicate(currentTask)) {
+        return {
+          task: currentTask,
+          tasks: current.scheduledTasks,
+          updated: false,
+        };
+      }
+
+      const patchObject = statePatch && typeof statePatch === 'object' ? statePatch : {};
+      const nextTask = {
+        ...currentTask,
+        state: normalizeState(
+          {
+            ...currentTask.state,
+            ...patchObject,
+            updatedAt: Date.now(),
+          },
+          currentTask.state,
+        ),
+      };
+
+      const nextTasks = current.scheduledTasks.slice();
+      nextTasks[taskIndex] = nextTask;
+
+      await writeProjectConfigToDisk(projectID, {
+        version: PROJECT_CONFIG_VERSION,
+        scheduledTasks: nextTasks,
+      });
+
+      return {
+        task: nextTask,
+        tasks: nextTasks,
+        updated: true,
      };
    });
  };
@@ -699,6 +877,7 @@ export const createProjectConfigRuntime = (deps) => {
    upsertScheduledTask,
    deleteScheduledTask,
    updateScheduledTaskState,
+    updateScheduledTaskStateIf,
    reconcileLoopTasks,
    resolveProjectConfigPath,
  };
@@ -471,4 +471,299 @@ describe('project-config loop reconciliation', () => {
      await cleanup();
    }
  });
+
+  it('conditionally updates state only when the predicate passes (occurrence claim)', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    try {
+      const created = await runtime.upsertScheduledTask('project-test', {
+        name: 'claim-me',
+        enabled: true,
+        schedule: { kind: 'daily', time: '15:00', timezone: 'UTC' },
+        execution: { prompt: 'Run once', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+
+      const scheduledFor = Date.UTC(2026, 0, 1, 15, 0, 0);
+      const first = await runtime.updateScheduledTaskStateIf(
+        'project-test',
+        created.task.id,
+        (task) => !Number.isFinite(task.state?.lastScheduledFor),
+        {
+          lastScheduledFor: scheduledFor,
+          lastStatus: 'running',
+          nextRunAt: scheduledFor + 86_400_000,
+        },
+      );
+      expect(first.updated).toBe(true);
+      expect(first.task.state.lastScheduledFor).toBe(scheduledFor);
+
+      const second = await runtime.updateScheduledTaskStateIf(
+        'project-test',
+        created.task.id,
+        (task) => task.state?.lastScheduledFor !== scheduledFor,
+        {
+          lastScheduledFor: scheduledFor,
+          lastStatus: 'running',
+        },
+      );
+      expect(second.updated).toBe(false);
+      expect(second.task.state.lastScheduledFor).toBe(scheduledFor);
+
+      const reloaded = await runtime.listScheduledTasks('project-test');
+      expect(reloaded[0].state.lastScheduledFor).toBe(scheduledFor);
+    } finally {
+      await cleanup();
+    }
+  });
+
+  it('serializes concurrent writes across two runtimes sharing a projects dir', async () => {
+    const tempRoot = await mkdtemp(path.join(os.tmpdir(), 'oc-project-lock-contention-'));
+    const fsPromises = await import('fs/promises');
+    try {
+      const runtimeA = createProjectConfigRuntime({
+        fsPromises,
+        path,
+        projectsDirPath: tempRoot,
+        createTaskID: () => 'shared-task',
+      });
+      const runtimeB = createProjectConfigRuntime({
+        fsPromises,
+        path,
+        projectsDirPath: tempRoot,
+        createTaskID: () => 'shared-task',
+      });
+
+      await runtimeA.upsertScheduledTask('project-lock', {
+        name: 'contended',
+        enabled: true,
+        schedule: { kind: 'daily', time: '15:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+
+      await Promise.all([
+        runtimeA.updateScheduledTaskState('project-lock', 'shared-task', {
+          lastStatus: 'success',
+          lastRunAt: 100,
+        }),
+        runtimeB.updateScheduledTaskState('project-lock', 'shared-task', {
+          lastStatus: 'error',
+          lastRunAt: 200,
+        }),
+      ]);
+
+      const tasks = await runtimeA.listScheduledTasks('project-lock');
+      expect(tasks).toHaveLength(1);
+      expect(['success', 'error']).toContain(tasks[0].state.lastStatus);
+      expect([100, 200]).toContain(tasks[0].state.lastRunAt);
+
+      const lockPath = `${runtimeA.resolveProjectConfigPath('project-lock')}.lock`;
+      await expect(fsPromises.access(lockPath)).rejects.toMatchObject({ code: 'ENOENT' });
+    } finally {
+      await rm(tempRoot, { recursive: true, force: true });
+    }
+  });
+
+  it('recovers from a stale-by-age project config lock and cleans it up', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    const fsPromises = await import('fs/promises');
+    try {
+      const projectID = 'stale-age';
+      const configPath = runtime.resolveProjectConfigPath(projectID);
+      const lockPath = `${configPath}.lock`;
+      await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+      await writeFile(lockPath, JSON.stringify({
+        pid: process.pid,
+        at: Date.now() - 120_000,
+      }));
+
+      const created = await runtime.upsertScheduledTask(projectID, {
+        name: 'after-stale',
+        enabled: true,
+        schedule: { kind: 'daily', time: '09:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+      expect(created.created).toBe(true);
+      await expect(fsPromises.access(lockPath)).rejects.toMatchObject({ code: 'ENOENT' });
+    } finally {
+      await cleanup();
+    }
+  });
+
+  it('recovers from a stale-by-dead-pid project config lock', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    const fsPromises = await import('fs/promises');
+    try {
+      const projectID = 'stale-pid';
+      const configPath = runtime.resolveProjectConfigPath(projectID);
+      const lockPath = `${configPath}.lock`;
+      await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+      // PID unlikely to exist; kill(pid, 0) should fail with ESRCH.
+      await writeFile(lockPath, JSON.stringify({
+        pid: 2_147_483_647,
+        at: Date.now(),
+      }));
+
+      const created = await runtime.upsertScheduledTask(projectID, {
+        name: 'after-dead-pid',
+        enabled: true,
+        schedule: { kind: 'daily', time: '09:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+      expect(created.created).toBe(true);
+      await expect(fsPromises.access(lockPath)).rejects.toMatchObject({ code: 'ENOENT' });
+    } finally {
+      await cleanup();
+    }
+  });
+
+  it('release does not unlink a lock stolen by another holder', async () => {
+    const tempRoot = await mkdtemp(path.join(os.tmpdir(), 'oc-project-lock-own-'));
+    const realFs = await import('fs/promises');
+    let enteredCritical;
+    const entered = new Promise((resolve) => {
+      enteredCritical = resolve;
+    });
+    let resumeCritical;
+    const hold = new Promise((resolve) => {
+      resumeCritical = resolve;
+    });
+
+    const fsPromises = {
+      ...realFs,
+      rename: async (from, to) => {
+        if (String(from).includes('.tmp-')) {
+          enteredCritical();
+          await hold;
+        }
+        return realFs.rename(from, to);
+      },
+    };
+
+    try {
+      const runtime = createProjectConfigRuntime({
+        fsPromises,
+        path,
+        projectsDirPath: tempRoot,
+        createTaskID: () => 'owned-task',
+      });
+      const projectID = 'lock-own';
+      const lockPath = `${runtime.resolveProjectConfigPath(projectID)}.lock`;
+      const stolenPid = process.pid + 1;
+
+      const upsertPromise = runtime.upsertScheduledTask(projectID, {
+        name: 'ownership',
+        enabled: true,
+        schedule: { kind: 'daily', time: '10:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+
+      await entered;
+      await realFs.writeFile(lockPath, JSON.stringify({
+        pid: stolenPid,
+        at: Date.now(),
+      }));
+      resumeCritical();
+      await upsertPromise;
+
+      const raw = await realFs.readFile(lockPath, 'utf8');
+      expect(JSON.parse(raw).pid).toBe(stolenPid);
+    } finally {
+      await rm(tempRoot, { recursive: true, force: true });
+    }
+  });
+
+  it('times out when a live lock holder never releases', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    const fsPromises = await import('fs/promises');
+    try {
+      const projectID = 'lock-timeout';
+      const configPath = runtime.resolveProjectConfigPath(projectID);
+      const lockPath = `${configPath}.lock`;
+      await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+      // Current process is alive — acquire must wait then throw.
+      await writeFile(lockPath, JSON.stringify({
+        pid: process.pid,
+        at: Date.now(),
+      }));
+
+      await expect(runtime.upsertScheduledTask(projectID, {
+        name: 'blocked',
+        enabled: true,
+        schedule: { kind: 'daily', time: '11:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      })).rejects.toThrow(/timeout acquiring project config lock/);
+    } finally {
+      await cleanup();
+    }
+  }, 15_000);
+
+  it('releases the write chain after a lock timeout so a later write can complete', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    const fsPromises = await import('fs/promises');
+    try {
+      const projectID = 'lock-timeout-recover';
+      const configPath = runtime.resolveProjectConfigPath(projectID);
+      const lockPath = `${configPath}.lock`;
+      await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+      await writeFile(lockPath, JSON.stringify({
+        pid: process.pid,
+        at: Date.now(),
+      }));
+
+      await expect(runtime.upsertScheduledTask(projectID, {
+        name: 'blocked-then-recover',
+        enabled: true,
+        schedule: { kind: 'daily', time: '11:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      })).rejects.toThrow(/timeout acquiring project config lock/);
+
+      // Remove the hostile lock. A wedged in-process chain would hang forever here.
+      await fsPromises.unlink(lockPath);
+
+      const created = await Promise.race([
+        runtime.upsertScheduledTask(projectID, {
+          name: 'after-timeout',
+          enabled: true,
+          schedule: { kind: 'daily', time: '12:00', timezone: 'UTC' },
+          execution: { prompt: 'Run after timeout', providerID: 'openai', modelID: 'gpt-4.1' },
+        }),
+        new Promise((_, reject) => {
+          setTimeout(() => reject(new Error('second write hung after lock timeout')), 3_000);
+        }),
+      ]);
+
+      expect(created.created).toBe(true);
+      expect(created.task.name).toBe('after-timeout');
+      const listed = await runtime.listScheduledTasks(projectID);
+      expect(listed).toHaveLength(1);
+      expect(listed[0].name).toBe('after-timeout');
+    } finally {
+      await cleanup();
+    }
+  }, 20_000);
+
+  it('recovers from an unparseable lock using mtime age', async () => {
+    const { runtime, cleanup } = await createRuntime();
+    const fsPromises = await import('fs/promises');
+    try {
+      const projectID = 'stale-unparseable';
+      const configPath = runtime.resolveProjectConfigPath(projectID);
+      const lockPath = `${configPath}.lock`;
+      await fsPromises.mkdir(path.dirname(configPath), { recursive: true });
+      // Simulate crash between open(wx) and writeFile / partial payload.
+      await writeFile(lockPath, '{not-json');
+      const staleMtime = new Date(Date.now() - 120_000);
+      await fsPromises.utimes(lockPath, staleMtime, staleMtime);
+
+      const created = await runtime.upsertScheduledTask(projectID, {
+        name: 'after-unparseable',
+        enabled: true,
+        schedule: { kind: 'daily', time: '09:00', timezone: 'UTC' },
+        execution: { prompt: 'Run', providerID: 'openai', modelID: 'gpt-4.1' },
+      });
+      expect(created.created).toBe(true);
+      await expect(fsPromises.access(lockPath)).rejects.toMatchObject({ code: 'ENOENT' });
+    } finally {
+      await cleanup();
+    }
+  });
 });