fix(scheduler): return a real execution_id, and 409 when nothing started (#1968)

`_trigger_handler` was fire-and-forget: it spawned the run with
`asyncio.create_task` and responded immediately, *before* the execution
record existed. So it had no id to return. The backend relayed the same
id-less fields, and the MCP tool interpolated the missing key — telling
every agent `Execution started with ID 'undefined'`, on every trigger,
while the execution ran fine. Callers could not correlate a trigger with
its run, poll it, or fetch its result; the workaround was to guess from
`list_recent_executions` by timestamp.

The same ordering hid a second problem. The response was emitted before
`_execute_manual_trigger` had even attempted the distributed lock, so a
trigger suppressed because the schedule was already running still
answered `"status": "triggered"`. A suppressed trigger and a real one
were byte-identical to the caller.

The handler now acquires the lock and creates the row synchronously,
then hands both to the background task. That makes two facts sayable
that simply did not exist yet at response time: which execution this is,
and whether one was started at all.

  * 200 carries a real `execution_id`, valid the moment the caller
    receives it — a fast poller must not 404.
  * 409 `already_running` replaces the false "triggered", with no id and
    no row, because nothing ran.

Exactly one row per trigger: `_execute_schedule_with_lock` takes the
pre-created execution and skips its own create. Two rows would hand the
caller an id naming a row that never runs while a second did the work.

Because a row can now exist before a gate decides not to run, an
abandoned run FAILs its pre-created row rather than leaving it `running`
forever — canary E-01's exact signature, and a task the UI would show
indefinitely.

The handler also now holds the lock across a DB write, which is new, so
every exit from that window releases it: creation raising, creation
returning None, the run raising, and normal completion — exactly once
each. A second release is the dangerous one, since a lock re-acquired by
the next run in between would be freed out from under it.

Relayed through the remaining surfaces:

  * the backend forwards `execution_id` (and records it on the audit row,
    so a trigger and its run are joinable after the fact) and maps 409
    rather than flattening it into "Failed to trigger schedule" — a
    worse lie than the original, since it claims failure where the
    schedule is healthily busy;
  * the MCP tool returns a structured `already_running` instead of
    throwing, so an agent gets a decision it can act on, and GUARDS the
    id instead of interpolating it — an older backend still omits the
    field, and swapping one confident lie for another is not a fix;
  * `ScheduleTriggerResult.execution_id` becomes optional. Typing it as
    a required `string` while the wire never sent it is precisely why
    the compiler stayed happy through every `undefined`;
  * the UI reads 409 as "already running" rather than "nothing was
    changed — try again", and the CLI prints the id it was already
    fetching and discarding.

tests/unit/test_1968_trigger_execution_id.py — 22 checks, 17 of which
fail against the pre-fix tree.

Related to #1968

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: dolho

src/backend/routers/schedules.py

@@ -483,6 +483,16 @@ async def trigger_schedule(
                     status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
                     detail="Scheduler service unavailable"
                 )
+            elif response.status_code == 409:
+                # #1968: the scheduler declined because this schedule is
+                # already running. Relayed as a 409 rather than flattened into
+                # the 500 below, because it is not a failure — it is the answer
+                # to the caller's question, and the old handler's silent
+                # `"status": "triggered"` for this case is the bug.
+                raise HTTPException(
+                    status_code=status.HTTP_409_CONFLICT,
+                    detail="Schedule is already executing"
+                )
             elif response.status_code != 200:
                 logger.error(f"Scheduler trigger failed: {response.status_code} - {response.text}")
                 raise HTTPException(
@@ -505,12 +515,20 @@ async def trigger_schedule(
                     "schedule_id": schedule_id,
                     "schedule_name": result.get("schedule_name"),
                     "triggered_by": "manual",
+                    # #1968: the audit row can now name the execution it
+                    # started, so a trigger and its run are joinable after the
+                    # fact rather than only correlatable by timestamp.
+                    "execution_id": result.get("execution_id"),
                 },
             )
 
             return {
                 "status": "triggered",
                 "schedule_id": schedule_id,
+                # #1968: the field the MCP tool has always read and never
+                # found. It was absent here because the scheduler responded
+                # before the row existed; it now creates the row first.
+                "execution_id": result.get("execution_id"),
                 "schedule_name": result.get("schedule_name"),
                 "agent_name": result.get("agent_name"),
                 "message": result.get("message", "Execution started")

src/cli/trinity_cli/commands/schedules.py

@@ -41,4 +41,13 @@ def trigger_schedule(agent, schedule_id):
     """Trigger a schedule immediately."""
     client = TrinityClient()
     data = client.post(f"/api/agents/{agent}/schedules/{schedule_id}/trigger")
-    click.echo(f"Triggered schedule {schedule_id} on '{agent}'")
+    # #1968: `data` was fetched and thrown away, so the command could not tell
+    # the user which run it had just started. The response now carries a real
+    # execution_id; print it, guarded, since an older backend still omits it.
+    execution_id = (data or {}).get("execution_id")
+    if execution_id:
+        click.echo(
+            f"Triggered schedule {schedule_id} on '{agent}' (execution {execution_id})"
+        )
+    else:
+        click.echo(f"Triggered schedule {schedule_id} on '{agent}'")

src/frontend/src/components/SchedulesPanel.vue

@@ -1337,7 +1337,20 @@ async function triggerSchedule(schedule) {
       await loadExecutions(schedule.id)
     }
   } catch (error) {
-    reportActionFailure(error, `run the schedule "${schedule.name}" now`)
+    // #1968: the backend now answers 409 when the schedule is already running,
+    // instead of reporting a success that started nothing. That is not the
+    // "nothing was changed — try again" case reportActionFailure describes: a
+    // run IS in flight, and retrying only hits the same lock. Say what is
+    // actually true, and reload so the user can see the run in question.
+    if (error?.response?.status === 409) {
+      actionError.value = `"${schedule.name}" is already running — no new run was started.`
+      actionErrorDetail.value = ''
+      if (expandedSchedule.value === schedule.id) {
+        await loadExecutions(schedule.id)
+      }
+    } else {
+      reportActionFailure(error, `run the schedule "${schedule.name}" now`)
+    }
   } finally {
     triggerLoading.value = null
   }

src/mcp-server/src/tools/schedules.ts

@@ -5,7 +5,7 @@
  */
 
 import { z } from "zod";
-import { TrinityClient } from "../client.js";
+import { ApiError, TrinityClient } from "../client.js";
 import type { McpAuthContext } from "../types.js";
 
 /**
@@ -536,17 +536,52 @@ export function createScheduleTools(
 
         // #1970: carry the caller identity through so the resulting execution
         // row is attributable to this key/agent, not just to the key owner.
-        const result = await apiClient.triggerAgentSchedule(
-          agent_name,
-          schedule_id,
-          authContext?.agentName,
-          authContext
-            ? { keyId: authContext.keyId, keyName: authContext.keyName }
-            : undefined
-        );
+        let result;
+        try {
+          result = await apiClient.triggerAgentSchedule(
+            agent_name,
+            schedule_id,
+            authContext?.agentName,
+            authContext
+              ? { keyId: authContext.keyId, keyName: authContext.keyName }
+              : undefined
+          );
+        } catch (error) {
+          // #1968: "already running" is an ANSWER, not a failure. Surfacing the
+          // raw ApiError would hand the calling agent a stack-shaped string it
+          // has to parse; a structured status lets it decide to wait and poll
+          // instead of retrying into the same lock.
+          if (error instanceof ApiError && error.status === 409) {
+            console.log(`[trigger_agent_schedule] Schedule '${schedule_id}' is already executing`);
+            return JSON.stringify({
+              status: "already_running",
+              schedule_id,
+              agent_name,
+              message:
+                "This schedule is already executing, so no new run was started. " +
+                "Poll list_recent_executions for the run in flight.",
+            }, null, 2);
+          }
+          throw error;
+        }
 
         console.log(`[trigger_agent_schedule] Triggered schedule '${schedule_id}' for agent '${agent_name}', execution_id: ${result.execution_id}`);
 
+        // #1968: `execution_id` used to be absent from the response, so this
+        // string always read "...with ID 'undefined'". Guard the wording rather
+        // than assume — an old scheduler behind a new MCP server still omits it,
+        // and a confident lie about the id is what this issue is about.
+        if (!result.execution_id) {
+          return JSON.stringify({
+            status: "triggered",
+            schedule_id,
+            execution_id: null,
+            message:
+              "Schedule triggered, but the backend returned no execution id. " +
+              "Find the run via list_recent_executions.",
+          }, null, 2);
+        }
+
         return JSON.stringify({
           status: "triggered",
           schedule_id,

src/mcp-server/src/types.ts

@@ -270,7 +270,12 @@ export interface ScheduleToggleResult {
 export interface ScheduleTriggerResult {
   status: "triggered";
   schedule_id: string;
-  execution_id: string;
+  // #1968: optional, and that is the honest declaration. This was typed as a
+  // required `string` while the backend never sent the field at all — which is
+  // precisely why the compiler stayed happy while every trigger interpolated
+  // `undefined` into its success message. A current backend fills it in; an
+  // older one still omits it, so callers must check rather than trust the type.
+  execution_id?: string;
   message?: string;
 }
 

src/scheduler/main.py

@@ -210,17 +210,76 @@ async def _trigger_handler(self, request: web.Request) -> web.Response:
             f"source_user_id={origin.user_id} source_agent={origin.agent_name}"
         )
 
-        # Execute in background (fire-and-forget)
+        # #1968: acquire the lock and create the execution row HERE, before
+        # responding. Both facts the caller needs — whether the trigger took
+        # effect, and which execution it produced — only exist after these two
+        # steps, and the old handler answered before either had happened.
+        #
+        # It reported `"status": "triggered"` with no id even when the lock was
+        # denied and nothing ran, so a suppressed trigger and a real one were
+        # byte-identical to the caller. The MCP tool then interpolated the
+        # missing key and told every agent `Execution started with ID
+        # 'undefined'`.
+        lock = self.scheduler_service.lock_manager.try_acquire_schedule_lock(schedule_id)
+        if not lock:
+            logger.warning(
+                f"Trigger for {schedule_id} (triggered_by={triggered_by}): "
+                "schedule already executing"
+            )
+            return web.json_response({
+                "status": "already_running",
+                "schedule_id": schedule_id,
+                "schedule_name": schedule.name,
+                "agent_name": schedule.agent_name,
+                "message": "Schedule is already executing",
+            }, status=409)
+
+        # From here the lock is HELD. Every path below must either hand it to
+        # the background task or release it — a leak would wedge the schedule
+        # until the Redis TTL expires.
+        try:
+            execution = self.scheduler_service.db.create_execution(
+                schedule_id=schedule.id,
+                agent_name=schedule.agent_name,
+                message=schedule.message,
+                triggered_by=triggered_by,
+                model_used=schedule.model,
+                source_user_id=origin.user_id,
+                source_user_email=origin.user_email,
+                source_agent_name=origin.agent_name,
+                source_mcp_key_id=origin.mcp_key_id,
+                source_mcp_key_name=origin.mcp_key_name,
+            )
+        except Exception as exc:
+            lock.release()
+            logger.error(f"Trigger for {schedule_id}: could not create execution: {exc}")
+            return web.json_response(
+                {"error": "Failed to create execution record"}, status=500
+            )
+
+        if not execution:
+            lock.release()
+            logger.error(f"Trigger for {schedule_id}: could not create execution record")
+            return web.json_response(
+                {"error": "Failed to create execution record"}, status=500
+            )
+
+        # Execute in background, handing over the lock we already hold and the
+        # row we already created.
         asyncio.create_task(
             self._execute_manual_trigger(
-                schedule_id, triggered_by=triggered_by, origin=origin
+                schedule_id,
+                triggered_by=triggered_by,
+                origin=origin,
+                lock=lock,
+                execution=execution,
             )
         )
 
-        # Return immediately; execution creates its own record asynchronously
         return web.json_response({
             "status": "triggered",
             "schedule_id": schedule_id,
+            "execution_id": execution.id,
             "schedule_name": schedule.name,
             "agent_name": schedule.agent_name,
             "triggered_by": triggered_by,
@@ -232,31 +291,55 @@ async def _execute_manual_trigger(
         schedule_id: str,
         triggered_by: str = "manual",
         origin: Optional[ExecutionOrigin] = None,
+        lock=None,
+        execution=None,
     ):
-        """Execute a manually or webhook-triggered schedule."""
-        try:
-            # Acquire lock (prevents concurrent execution)
-            lock = self.scheduler_service.lock_manager.try_acquire_schedule_lock(schedule_id)
+        """Execute a manually or webhook-triggered schedule.
+
+        #1968: `lock` and `execution` are acquired/created by `_trigger_handler`
+        BEFORE it responds, so the response can carry a real `execution_id` and
+        a denied lock can be reported as 409 instead of a false "triggered".
+        Both are passed in rather than taken here; this coroutine's job is to
+        run the schedule and, whatever happens, release the lock.
+
+        They stay optional so the method keeps working if called without them
+        (it then acquires its own lock, as before, and lets the service create
+        the row) — a caller that forgets is degraded, not broken.
+
+        Whichever way the lock arrived, this coroutine owns it from here and
+        releases it exactly once. Structured as acquire-then-single-`finally`
+        rather than a release in both the `finally` and an error branch: the
+        second release is the dangerous one, since a lock re-acquired by the
+        next run in between would be freed out from under it.
+        """
+        if lock is None:
+            try:
+                lock = self.scheduler_service.lock_manager.try_acquire_schedule_lock(
+                    schedule_id
+                )
+            except Exception as exc:
+                logger.error(f"Trigger for {schedule_id}: lock acquisition failed: {exc}")
+                return
             if not lock:
                 logger.warning(
                     f"Trigger for {schedule_id} (triggered_by={triggered_by}): "
                     "schedule already executing"
                 )
                 return
 
-            try:
-                await self.scheduler_service._execute_schedule_with_lock(
-                    schedule_id,
-                    triggered_by=triggered_by,
-                    origin=origin
-                )
-            finally:
-                lock.release()
-
+        try:
+            await self.scheduler_service._execute_schedule_with_lock(
+                schedule_id,
+                triggered_by=triggered_by,
+                origin=origin,
+                execution=execution
+            )
         except Exception as e:
             logger.error(
                 f"Trigger execution failed for {schedule_id} (triggered_by={triggered_by}): {e}"
             )
+        finally:
+            lock.release()
 
     async def _run_until_shutdown(self):
         """Run the scheduler until shutdown signal."""