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

[dolho]

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

Fixes #1968

[dolho]

/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]

⚠️ 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.

[dolho]

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.

[dolho]

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]

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]

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.

[vybe]

The stack is resolved correctly — merge-base is e6df5bf8, the branch carries #1974's own commit un-squashed while dev has the squashed version, and the three-dot diff (what GitHub shows and what merges) contains zero #1974 content. The remaining conflicts are learnings.md and tests/registry.json only, from newer dev commits, so a plain rebase clears them.

The core work is good: exactly-one-row is asserted at both ends, lock release is exactly-once across all five exits (the early returns sit before the try, so lock.release() is never reached with lock=None), 404-before-lock ordering is right, and the strong task reference is genuinely load-bearing given the reordering. The regression tests are real — reverting src/scheduler/ to dev fails 12 of 23.

One thing blocks.

The webhook consumer was missed, and it regresses.

src/backend/routers/webhooks.py is the fourth caller of the scheduler /trigger endpoint, and it wasn't updated. The scheduler now returns 409 when a webhook fires against a schedule that is already running, and this gate catches it:

if response.status_code not in (200, 202):
    logger.error(f"Webhook trigger: scheduler error {response.status_code} …")
    raise HTTPException(status_code=503, detail="Trigger failed — try again later")

So a healthy, busy schedule now produces an ERROR log line and a 503 "try again later" to an unauthenticated public caller — advice that only hits the same lock — and the except HTTPException: arm below calls idempotency_service.fail(idem), releasing the #525 dedup claim for something that is not a failure. Before this PR that delivery returned 202.

The blast-radius table lists three consumers (UI, CLI, MCP) and asserts none regress; the omitted fourth is the one that does. And tests/test_webhook_triggers.py asserts status_code in (202, 503) throughout, so CI accepts the regression silently — which is why it slipped.

Non-blocking:

• _abandon_precreated_execution calls db.update_execution_status, which in the standalone scheduler is an unconditional UPDATE … WHERE id = ? with no status precondition — a new non-CAS status writer, the set architecture.md already names as the open #1082 follow-up. Safe today (the abandon fires only on gates that run before dispatch, so no competing writer exists), but AND status = 'running' is one clause and makes it safe by construction rather than by argument.
• Error handling is asymmetric: try_acquire_schedule_lock is unwrapped at the original call site and wrapped in try/except at the new fallback. A Redis outage at the first raises → 500. Honest (never a false 409), but one of the two is wrong.
• stop() never drains _inflight_triggers — the strong-ref set defeats GC, not shutdown. The exact failure the new comment describes stays reachable on SIGTERM, now with a row that didn't previously exist.
• The backend's timeout=10.0 wasn't revisited, though the handler now does a Redis SET plus a synchronous SQLite INSERT before responding. Under write contention a >10s handler yields 504 while the run is genuinely in flight — a fresh instance of "it's running and you have no id", which is the situation this PR exists to eliminate.
• Lock-denied manual triggers now produce no audit row at all (the 409 raises before platform_audit_service.log). Previously they produced a misleading one; sibling #1975 added the honest lock-denied audit for cron ticks, and the manual path arguably wants the same rather than nothing.
• schedules.ts: the console.log(...execution_id: ...) sits before the !result.execution_id guard, so it still prints undefined — the same string the issue is named after.
• Six tests are source-regex assertions over the backend/MCP/Vue surfaces rather than behavioural — acceptable for cross-language seams, but they'd pass against a renamed-yet-broken implementation, and there is no webhook-consumer test at all, which is the gap that let the finding above through.

Timestamps are correct throughout (utc_now_iso / to_utc_iso from src/scheduler/utils.py), so Invariant #16 / #1713 parity is preserved.

[vybe]

Re-validated after the 2026-08-08 push. The rebase is clean and the non-blocking items I raised are partly addressed — but the one blocking item is still open, so I'm holding this rather than merging it with today's batch.

src/backend/routers/webhooks.py is still not in the diff. Re-verified against the current head rather than assuming:

• src/scheduler/main.py now answers 409 {"status": "already_running"} on a denied lock (diff L232–237), and routers/schedules.py maps it correctly (L23–30).
• routers/webhooks.py:292 is a separate consumer of the same scheduler endpoint and still reads if response.status_code not in (200, 202) → logger.error(...) + 503 "Trigger failed — try again later".
• webhooks.py:300-305 then catches its own HTTPException and calls idempotency_service.fail(idem), releasing the feat: idempotency keys at all execution trigger boundaries (RELIABILITY-006) #525 dedup claim.

So after this merges, a webhook firing at a healthy, busy schedule returns 503 to an unauthenticated public caller where it previously returned 202, writes an ERROR line for a non-error, and drops the idempotency claim for something that did not fail. The advice "try again later" only re-hits the same lock.

The single webhook test added (test_webhook_trigger_still_records_its_trigger_type, L1100) exercises the scheduler handler with triggered_by: "webhook". It does not touch the backend relay, so the regression stays invisible to CI — and tests/test_webhook_triggers.py asserts status_code in (202, 503) throughout, which accepts it silently. That is the same gap that let this through the first time.

I'm deliberately not fixing this for you, because the correct behaviour is a product decision I shouldn't make unilaterally. The public webhook contract today is 202-accepted; the options are at least:

1. Relay the 409 — honest, consistent with the other three consumers, but changes a public contract for unauthenticated senders (CI hooks, monitors, IFTTT) that mostly cannot act on it.
2. Keep 202 and treat lock-denied as satisfied delivery — the schedule is running, which is what the sender wanted; arguably the most correct for at-least-once senders, and it preserves the idempotency claim.
3. 202 with a distinguishing body field — compatible, and observable for callers that look.

My inclination is (2), since the caller's intent ("make sure this runs") is already satisfied and the #525 claim should stand — but it's your call and the issue doesn't settle it. Whichever you pick, it needs a test at the backend layer, and tests/test_webhook_triggers.py's in (202, 503) assertions want tightening so they can't absorb the next one.

Everything else I checked is in good shape: exactly-one-row asserted at both ends, lock release exactly-once across all five exits, 404-before-lock ordering, the strong task reference, and utc_now_iso/to_utc_iso throughout (Invariant #16 / #1713 parity intact). Regression tests are genuine — 17 of 22 fail against the pre-fix tree.

Re-request review once the webhook consumer is settled and I'll merge it straight away.

[dolho]

@vybe — blocking item fixed: the fourth consumer is handled, and it was regressing exactly as you described.

routers/webhooks.py now treats 409 as its own outcome. A busy schedule is healthy, not an error:

	before	after
log	ERROR Webhook trigger: scheduler error 409	INFO … already executing — delivery coalesced
response	503 "Trigger failed — try again later" (only hits the same lock)	202 {"status": "already_running", …}
#525 claim	idempotency_service.fail(idem) — released for something that never failed	complete(...) with a snapshot recording the coalesced outcome

The response message says the delivery was folded into the run in flight rather than claiming a fresh execution started, so the 202 isn't a second lie in place of the first. Genuine scheduler errors (500/502/400) still surface as 503 — the carve-out is on 409 alone, and a parametrized test pins that it doesn't widen into "any non-2xx is fine".

On the test gap you identified — tests/test_webhook_triggers.py asserts status_code in (202, 503) throughout, which is precisely why CI accepted this silently. The new tests name the status rather than tolerating a set, and run in-process (TestClient + faked scheduler, #1422's harness) so they don't need a live instance:

409 carve-out removed:  3 failed, 3 passed
as shipped:             6 passed
webhook/scheduler unit selection: 75 passed, 5 skipped

Two of the non-blocking notes, both one-liners:

• _abandon_precreated_execution now passes expected_status=RUNNING to a new optional CAS precondition on the scheduler's update_execution_status. You're right that it's safe today — every abandon gate runs before dispatch — but refactor: status-as-projection — schedule_executions.status never read as authoritative for "is running" #1082 exists to retire "safe by argument", and the clause makes it safe by construction. expected_status=None default keeps every other caller byte-identical.
• schedules.ts logged execution_id: undefined above the !result.execution_id guard — the exact string the issue is named after. Moved below it.

Left alone deliberately, and worth a ruling if you disagree: the try_acquire_schedule_lock error-handling asymmetry, stop() not draining _inflight_triggers, the backend's timeout=10.0, and the missing lock-denied audit row for manual triggers. Each is a behaviour decision rather than a defect in this diff, and folding them in would widen a PR that already spans the scheduler, backend, MCP and Vue surfaces.

Merged latest dev.

[vybe]

Re-reviewed. The blocking item is fixed and the fix is scoped correctly.

routers/webhooks.py 409 carve-out — the fourth consumer now treats a busy schedule as its own outcome: 202 {"status":"already_running"} with an INFO log and idempotency_service.complete(...) carrying a snapshot, instead of 503 + fail(idem) releasing a key for something that never failed. Genuine scheduler errors (400/500/502) still surface as 503, and the parametrized test pins that the carve-out doesn't widen into "any non-2xx is fine" — which was my actual worry.

The response wording matters and you got it right: saying the delivery was coalesced into the run in flight, rather than claiming a fresh execution started, avoids replacing one lie with another.

On the test gap — assert status_code in (202, 503) is exactly why CI accepted the regression silently, and naming the status in the new tests is the durable fix. Running them in-process on #1422's harness rather than against a live instance is the right call.

The two extras are good: expected_status=RUNNING on _abandon_precreated_execution makes it safe by construction rather than by argument (#1082's whole point), with expected_status=None keeping every other caller byte-identical; and moving the schedules.ts log below the guard fixes the literal string the issue is named after.

Deferred items accepted — each is a behaviour decision, and this PR already spans scheduler, backend, MCP and Vue.