fix(scheduler): return a real execution_id, and 409 when nothing started (#1968) by dolho · Pull Request #1976 · Abilityai/trinity

dolho · 2026-08-04T08:42:34Z

Stacked on #1974 (#1970). Base this PR against fix/1970-scheduler-execution-origin; review the own commit only. Merge #1974 first — GitHub will retarget this to dev automatically. See Why stacked below.

Problem

_trigger_handler was fire-and-forget: it spawned the run with asyncio.create_task and responded immediately — before the execution record existed.

asyncio.create_task(self._execute_manual_trigger(schedule_id))

return web.json_response({
    "status": "triggered",
    "schedule_id": schedule_id,
    ...                       # no execution_id — there was nothing to name yet
})

So the backend relayed id-less fields, and the MCP tool interpolated the missing key:

Schedule triggered. Execution started with ID 'undefined'.

on every trigger, while the execution itself ran fine. A caller could not correlate its trigger with a run, poll it, or fetch its result. The workaround was guessing 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. 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, with only a scheduler-side WARNING as evidence.

Fix

Acquire the lock and create the row synchronously in the handler, then hand both to the background task. This makes two facts sayable that did not yet exist at response time:

	Before	After
Which execution is this?	absent → `'undefined'`	real `execution_id`, valid the moment the caller receives it
Did anything start?	always `"triggered"`	`409 already_running` when the lock is denied

Per the pre-work decision, this is the full variant including the 409 — the issue notes the honest-suppression half is where the operator value sits.

Exactly one row per trigger

_execute_schedule_with_lock now accepts the pre-created execution and skips its own create. Two rows would be worse than none: the caller's id would name a row that never runs while a second row did the work. Asserted at both ends — the handler's, and the service's.

Consequences the issue didn't list, handled here

A row can now exist before a gate decides not to run. If the schedule is deleted in the window between the response and the task starting, the row is already created and its id already in the caller's hands. Left running it never terminates — canary E-01's exact signature, and a task the UI shows forever. Abandoned runs now FAIL their pre-created row with a reason. The helper is best-effort and never raises: it runs on abort paths, inside a background task nobody awaits.

The handler now holds a lock across a DB write. That is a new failure window, so every exit from it releases: create_execution raising, create_execution returning None, the run raising, normal completion — exactly once each. Deliberately 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. (I wrote the two-release version first; it is the kind of thing that looks defensive and is not.)

The 404 gate stays ahead of the lock — locking a schedule that does not exist would block nothing and leak a key until its TTL.

Relay through the remaining surfaces

Backend forwards execution_id, records it on the audit row (a trigger and its run are now joinable after the fact, not just correlatable by timestamp), 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.
MCP tool returns a structured already_running instead of throwing, so a calling agent gets a decision it can act on rather than a stack-shaped string, and can choose to poll instead of retrying into the same lock. It also 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. It was typed as a required string while the wire never sent the field at all — which is precisely why the compiler stayed happy through every undefined. Worth calling out: the type asserted a guarantee nothing upheld.
UI reads 409 as "already running — no new run was started" rather than "nothing was changed — try again", and reloads so the user can see the run in question. Retrying would only hit the same lock.
CLI prints the id it was already fetching into data and discarding.

Blast radius of the 409

All three consumers improve; none regress:

Consumer	Before	After
UI	silent no-op, looked successful	explicit "already running", executions reloaded
CLI	printed a false success	non-zero exit with the reason
MCP	`'undefined'` id, looked successful	structured `already_running` + poll hint

Verification

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

$ git stash push -- src/ && pytest unit/test_1968_trigger_execution_id.py -q
17 failed, 5 passed
$ git stash pop && pytest unit/test_1968_trigger_execution_id.py -q
22 passed

One test failure during development was mine, not the code's, and is worth recording: the fixture's schedule_executions table omitted the columns update_execution_status writes, so the abandon path raised OperationalError, its fail-safe swallowed it, and the row stayed running — the test "found" a bug that was the fixture's. The table now carries that column set with a comment tying it to the UPDATE.

Also green: the full scheduler_tests/ suite plus the stacked #1970 tests — 269 passed; tsc --noEmit clean; test_1808 / test_1945 unaffected.

Why stacked

_trigger_handler is where #1970 (PR #1974) parses the caller identity, and this PR moves create_execution into that same function. Built off dev instead, whichever merged second would conflict there and the origin arguments would have to be re-attached to the moved call by hand — silently regressing #1970 to all-NULL if missed. test_precreated_row_still_carries_the_caller_identity pins that they moved together.

Acceptance criteria

Manual trigger returns a real execution_id end to end (scheduler → backend → MCP)
A lock-denied trigger is reported honestly (409 already_running) instead of a false success
One trigger produces exactly one execution row
No pre-created row is left running when the run is abandoned

Siblings

#1974 (#1970) is the parent. #1975 (#1969, lock-denied cron tick audit) is independent and already green — it touches _execute_schedule, the cron entry point, not _execute_manual_trigger.

Related to #1968

🤖 Generated with Claude Code

dolho · 2026-08-04T09:21:32Z

/review — self-review (PR #1976, #1968)

Branch: fix/1968-trigger-execution-id → fix/1970-scheduler-execution-origin
Scope: CLEAN — the diff is the trigger path plus its three relay surfaces; no unrelated files.
Plan completion: 4 AC DONE, 0 partial.

Caveat: this is a self-review, so it is worth less than an independent pass. I went looking for reasons the change is wrong rather than reasons it is right, and found one worth acting on.

[C1] Concurrency: the spawned task was only weakly referenced (Confidence: 8/10) — FIXED in `33e2ce2`

asyncio.create_task(self._execute_manual_trigger(...))   # result discarded

The event loop keeps only a weak reference to a task; the asyncio docs say outright to keep your own or it can be garbage-collected mid-flight.

The bare call predates this PR — but this PR changes what it costs. Before, a collected task meant the run silently didn't happen. Now the lock is acquired and the execution row created before the task is spawned, so a collected task strands a running execution whose id the caller is already holding, and pins the schedule's lock until its Redis TTL. That is the failure mode this PR exists to eliminate, reachable by a different route.

This repo already has the fix pattern — _inflight + add_done_callback(discard) in agent_server/services/result_callback.py:89 (#1083). Now applied here, with test_the_spawned_task_is_strongly_referenced checking the reference is held in the window before the task runs (the only window where collection is possible) and released after.

This is category 4.14: a fix that raises the stakes of an adjacent unguarded sibling.

[I1] A dropped task leaves no scheduler-side reaper (Confidence: 7/10)

Even strongly referenced, a process kill between the 200 response and the task completing leaves the row running. The backend's cleanup_service stale-execution sweep is the backstop, but nothing in the scheduler reconciles it. Pre-existing for cron rows; newly reachable for manual ones because the row now exists earlier. Not worth blocking on — flagging so it is a known gap rather than a surprise.

[I2] `test_every_create_execution_call_site_passes_an_origin` parses with `block.split(")")[0]` (Confidence: 6/10)

Inherited from #1974's file. A ) inside an argument truncates the slice early — but that direction produces a false failure, not a false pass, so it fails safe. Left as is.

Clean categories

SQL safety — no raw SQL added; create_execution uses the existing qmark-parameterised INSERT, PG-safe via _PgCursor.
Auth — no new endpoint; the scheduler trigger route's exposure is unchanged (platform network only).
Credential exposure — the new log line carries source_user_id/source_agent only, deliberately not the email.
Enum completeness — no new status/enum values; already_running is a response field, not a persisted status.
Error handling — every exit from the new lock-held window releases exactly once (4 tests).

Summary

Critical: 1 — fixed in this PR
Informational: 2 — no action needed
Scope: clean

github-actions · 2026-08-05T08:47:44Z

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

`schedule_executions` has carried five origin columns for audit since AUDIT-001, and the backend populates them on every path it owns. The scheduler is a separate service with its own DB module, and its `create_execution()` listed none of them in the INSERT — nor accepted them in its signature, so there was nowhere to put a caller even if one had been forwarded. Every scheduler-created row was written with all five NULL. `triggered_by='manual'` therefore recorded *that* a human ran something and never *who*. The attribution lived only in backend/MCP-server logs, bounded by log retention, so past a few weeks the durable record could not answer "did anyone trigger this run, and who?". The identity was dropped at three points, not one: 1. the backend's delegating POST sent no body at all, so the authenticated caller — in scope right there — never crossed the hop; 2. `_trigger_handler` had no parameter to receive one; 3. `create_execution()` had nowhere to put it. An `ExecutionOrigin` value object is threaded through all three. One object rather than five parallel parameters at four call depths: five positional siblings is how one of them silently stops being forwarded. Two paths the DB fix alone would have left blank are covered too. A retry inherits the original run's origin — it has no caller of its own, but a chain of retries that drops the initiator makes the first attempt the only attributable one; the read is fail-open, since an audit lookup must not be able to stop a retry from running. A reminder inherits the provenance #1296 already persisted. Cron ticks stay NULL. Attributing an autonomous fire to, say, the schedule's owner would make the column actively misleading — a blank reads as "unknown", a wrong name does not. Also hardened while here: - the untrusted trigger body is validated at the scheduler boundary. `source_user_id` is dropped rather than coerced when it is not an int: `bool` IS an `int` in Python, so `True` would have persisted as user 1, a real account attributed to a run it had nothing to do with. Strings are length-capped and blank-to-None, so "" and NULL are not two spellings of "unknown". - the backend prefers the validated `current_user.agent_name` over the raw `X-Source-Agent` header — the reverse of chat.py's precedence, which is fine for a collaboration hint but would let a caller pin its run on a sibling agent in an audit column. - the MCP trigger tool forwards the origin headers `chat()` already sends (Invariant #13). Without it an MCP-triggered run attributes to the key OWNER but not to which key or agent fired it — the part that identifies the actor when one human owns many of both. Not a vulnerability: nothing authorizes on these columns. Backward compatible in both rolling-deploy directions — an old scheduler ignores the new body fields, and a new scheduler treats a bodyless POST as an unattributed manual trigger. tests/unit/test_1970_execution_origin.py — 27 checks, 25 of which fail against the pre-fix tree. Related to #1970 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ted (#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>

…1968) Self-review finding on this PR. The event loop holds only a WEAK reference to a task, so a bare `asyncio.create_task(...)` whose result nobody keeps can be garbage-collected mid-flight — the asyncio docs say so outright. The bare call predates this PR, but this PR changes what it costs. Before, a collected task meant the run silently did not happen. Now the lock is acquired and the execution row created BEFORE the task is spawned, so a collected task strands a `running` execution whose id the caller already holds and pins the schedule's lock until its Redis TTL. Uses the `_inflight` set + `add_done_callback(discard)` shape the #1083 result-callback path already established (`agent_server/services/result_callback.py`), so the set cannot grow without bound. Guarded by `test_the_spawned_task_is_strongly_referenced`, which checks the reference is held in the window BEFORE the task runs — the only window where collection is possible — and released after. Related to #1968 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

dolho · 2026-08-05T09:07:12Z

Rebased onto current dev — the PR was conflicting, so there was no refs/pull/1976/merge for CI to build and no checks were reporting at all. They should run now.

Conflicts were in tests/registry.json (both sides appended entries; unioned, ours last). Resolved by re-serializing from parsed JSON rather than splicing lines, so the separating comma can't be lost.

Local verification after the rebase is in the individual runs; no source conflicts, only the registry.

Ready for review.

…uced My rebase of this branch onto dev left `src/scheduler/models.py` with TWO `class ExecutionOrigin` definitions: this PR's at line 110 and dev's copy (#1974, as merged) at 187. Python keeps the last one, so the second shadowed the first and both of this PR's fixes became dead code — the None-comparing `is_empty` and the SQLite-range guard on `source_user_id`. Three `test_execution_origin_properties` cases went red on head, correctly. Git did not flag it. Both sides added the class at different offsets, so the textual auto-merge took both hunks and reported no conflict — a semantic duplicate that only a reader or an importer would notice. Two things on my side let it through: - I re-read only the files git marked as conflicted, not the whole merged result of a 4-commit rebase. - My post-rebase check was `-k "1968 or ent326 or timeline or scheduler or executions"`, which does not match `test_execution_origin_properties`. The filter was narrower than the blast radius, so 129 tests passed and said nothing about the file I had just broken. Kept the first copy: it is a strict superset (identical fields, `is_empty` comparing against None so `user_id=0` is not reported empty, and the `_SQLITE_INT_MIN/MAX` range check that stops an out-of-range id reaching the INSERT and taking the dispatch down). Deleted dev's older copy. Swept the other six branches I rebased for the same class of damage — duplicate top-level defs in any changed .py — all clean. 350 passed across origin/scheduler/1968/1969/1970/execution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

dolho · 2026-08-05T09:50:52Z

Two separate reds here — one was mine, one is not this PR.

The three new failures were my rebase (fixed)

src/scheduler/models.py ended up with two class ExecutionOrigin definitions: this PR's at line 110 and dev's copy (#1974, as merged) at 187. Python keeps the last one, so the second shadowed the first and both of this PR's fixes became dead code — the None-comparing is_empty and the SQLite-range guard on source_user_id. test_execution_origin_properties went red on exactly those three range cases, correctly.

Git never flagged it: both sides added the class at different offsets, so the textual auto-merge took both hunks with no conflict. A semantic duplicate only a reader or an importer would catch.

Two failures on my side let it through, and both are worth naming:

I re-read only the files git marked conflicted, not the merged result of a 4-commit rebase.
My post-rebase check was -k "1968 or ent326 or timeline or scheduler or executions" — which does not match test_execution_origin_properties. 129 tests passed and told me nothing about the file I had just broken. The filter was narrower than the blast radius.

Kept the first copy (strict superset: same fields, is_empty comparing against None so user_id=0 isn't reported empty, plus the _SQLITE_INT_MIN/MAX guard that stops an out-of-range id reaching the INSERT and taking the dispatch with it). Deleted dev's older copy. 350 passed across origin/scheduler/1968/1969/1970/execution.

I also swept the other six branches I rebased today for the same class of damage — duplicate top-level defs in any changed .py — all clean.

The base-side red is not this PR

pytest (base, seed 67890) is the base side (plain dev) and it was cancelled at the 25-minute job timeout, not failed. Its JUnit artifact came back empty (junit-base-67890.xml — 0 tests), which is why regression diff also went red: it is deliberately fail-closed on a missing/empty XML.

Same shard has now done this on #1952 too, so it is a specific ordering under seed 67890 on dev, not a random flake. Filed as #2019: the workflow installs pytest-timeout but never passes --timeout, so one hanging test consumes the whole budget and dies without naming itself.

dolho · 2026-08-05T09:53:05Z

Correction to my previous comment on the base-side red: I wrote that the twice-hit base, seed 67890 shard meant "a specific ordering under seed 67890 on dev, not a random flake." That was wrong.

Since then it has hit head, seed 12345 (#2018) and head, seed 99999 (#2010) as well — all three seeds, both sides — and re-running the exact seed-67890 base job passed with no code change (#1952 is green now). Same ordering, different outcome, so ordering is not the deciding factor.

The evidence points at something timing-dependent that usually returns fast and occasionally blocks: the #1952 log shows a 3m37s gap with zero output between two progress lines, which is a stall rather than slowness. Details and the corrected table are on #2019.

Nothing changes for this PR — the base-side red still isn't yours, and the fix (a per-test --timeout, so a stall names itself instead of killing the shard anonymously) is unaffected.

dolho · 2026-08-05T12:04:36Z

Correction to my earlier comment on the duplicate ExecutionOrigin, found while edge-case-testing dev.

I wrote that the duplicate was "this PR's at line 110 and dev's copy (#1974, as merged) at 187", and that the rebase had shadowed this PR's fixes. That attribution is backwards.

git log -S"_SQLITE_INT_MAX" -- src/scheduler/models.py points at 4e7372ba — #1974 itself. Both the SQLite range guard and the None-comparing is_empty shipped with that feature and are on dev today. This branch was cut before it merged, so the branch's own ExecutionOrigin is the OLDER one. My rebase replayed it on top of dev's, git took both hunks, and the stale branch copy at 187 shadowed dev's good copy at 110.

So: same defect, same fix, wrong story. What I deleted was the branch's own outdated copy, and what I kept is dev's — which is the right outcome either way, but "this PR's improvements became dead code" was wrong. They were never this PR's improvements.

Nothing to change in the code. Correcting it because the comment is the record, and the next person reading it would otherwise credit the guard to the wrong PR.

dolho mentioned this pull request Aug 4, 2026

bug: manual schedule trigger always returns execution_id 'undefined' #1968

Open

This was referenced Aug 4, 2026

fix(scheduler): audit a tick suppressed by the distributed lock (#1969) #1975

Merged

fix(scheduler): record who initiated an execution (#1970) #1974

Merged

vybe changed the base branch from fix/1970-scheduler-execution-origin to dev August 4, 2026 17:57

dolho and others added 4 commits August 5, 2026 12:04

docs(learnings): record the create_task-owns-state class surfaced by #…

27f4632

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

dolho force-pushed the fix/1968-trigger-execution-id branch from db46a5a to 27f4632 Compare August 5, 2026 09:06

dolho mentioned this pull request Aug 5, 2026

bug(ci): a single hanging test burns the 25-minute unit-suite budget — no --timeout despite pytest-timeout being installed #2019

Open

4 tasks

dolho mentioned this pull request Aug 5, 2026

fix(agent-server): rebuild the execution env per spawn so a removed .env key stops applying (#1999) #2010

Open

4 tasks

Navigation Menu

Uh oh!

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

fix(scheduler): return a real execution_id, and 409 when nothing started (#1968)#1976
dolho wants to merge 5 commits into
devfrom
fix/1968-trigger-execution-id

dolho commented Aug 4, 2026

Uh oh!

dolho commented Aug 4, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

dolho commented Aug 4, 2026

Problem

Fix

Exactly one row per trigger

Consequences the issue didn't list, handled here

Relay through the remaining surfaces

Blast radius of the 409

Verification

Why stacked

Acceptance criteria

Siblings

Uh oh!

dolho commented Aug 4, 2026

/review — self-review (PR #1976, #1968)

[C1] Concurrency: the spawned task was only weakly referenced (Confidence: 8/10) — FIXED in 33e2ce2

[I1] A dropped task leaves no scheduler-side reaper (Confidence: 7/10)

[I2] test_every_create_execution_call_site_passes_an_origin parses with block.split(")")[0] (Confidence: 6/10)

Clean categories

Summary

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

The three new failures were my rebase (fixed)

The base-side red is not this PR

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

dolho commented Aug 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

[C1] Concurrency: the spawned task was only weakly referenced (Confidence: 8/10) — FIXED in `33e2ce2`

[I2] `test_every_create_execution_call_site_passes_an_origin` parses with `block.split(")")[0]` (Confidence: 6/10)