feat(DIARCHERS-1521): add MCP write endpoints + fix log/artifacts/trigger

[Jiachen0715]

Summary

Fixes three defects in the MCP backend surfaced during test-env
integration (build_3201) and adds the 5 MCP write endpoints the
feat/mcp-restart-rerun branch name promised.

Branch base: feat/mcp-restart-rerun. This PR therefore carries
the 8 upstream commits from #617-#625 in addition to the single
commit authored here. Once #620 (traffic isolation) and #621
(restart/rerun endpoints) land on master, the base can be switched
or this PR can be rebased.

DIARCHERS-1521 — Backend fixes

1. get_job_log silent empty response: the MCP endpoint queried
   only the console streaming table. The scheduler archives job
   logs to job.console on termination and DELETEs the streaming
   rows, so every finished job returned HTTP 200 with an empty body.
   Fixed with a two-stage lookup: job.console archive column
   first, fall back to console for jobs still running.

2. list_job_artifacts returns 500 unconditionally: the MCP
   endpoint queried a non-existent archive table. archive is a
   jsonb[] column on the job table (migration 00009.sql).
   Rewrote to SELECT archive FROM job and unpack the array,
   matching the legacy endpoint behavior.

3. POST /trigger creates zombie builds: the MCP endpoint only
   inserted a build row without the seed create_job_matrix job
   the scheduler polls for, so every trigger returned HTTP 200 +
   build_id but the build never ran. Also hardcoded
   restart_counter=0, diverging from schema default 1 and hiding
   the build from legacy UI filters. Fixed by resolving
   source_upload_id per project type (upload projects reuse the
   most recent source_upload; test projects allow NULL), inserting
   the Create Jobs seed in the same transaction, moving the success
   audit entry to after the seed job commits, and removing the
   restart_counter=0 override.

5 new MCP write endpoints

All require allow_trigger=true on the MCP token, share the
trigger_build rate-limit bucket (5 RPM), and are fully audited.

Method	Path	Handler	Semantics
POST	/builds/<bid>/restart	MCPBuildRestart	New build, cloned Create Jobs seed
DELETE	/builds/<bid>/abort	MCPBuildAbort	Insert abort rows for all jobs in build
POST	/jobs/<jid>/restart	MCPJobRestart	Restart job + DAG downstream
POST	/jobs/<jid>/rerun	MCPJobRerun	Rerun single job only
DELETE	/jobs/<jid>/abort	MCPJobAbort	Insert abort row for single job

Handlers do not delegate to the legacy /api/v1/*
implementations, which hard-depend on
g.token['type'] in ('user', 'project'). MCP handlers use
get_mcp_user_id() and reimplement the SQL directly. Shared helper
_restart_or_rerun_job() covers both restart_job (DAG-propagating)
and rerun_job (single-job) paths.

Dashboard UI

src/dashboard-client/src/components/user/UserGlobalTokens.vue:
moved the MCP Tokens section to the top of the page, ahead of Global
Viewer Tokens and Project Tokens. Rationale: MCP Tokens is the
newest feature and the highest-traffic section for users onboarding
to the MCP integration.

Concurrency hardening (review feedback)

Following review, the write paths were hardened against race conditions
under concurrent requests:

• MCPBuildRestart — atomic restart_counter: LOCK TABLE build IN EXCLUSIVE MODE is now taken before SELECT max(restart_counter), so
  two concurrent restarts of the same build cannot read the same max and
  insert duplicate rows. Matches the pattern MCPTrigger already uses;
  EXCLUSIVE MODE does not block reads.

• restart_job / rerun_job — atomic claim of the target job: the
  read-then-check on the restarted flag was replaced by a compare-and-set
  UPDATE job SET restarted=true WHERE id=%s AND restarted=false RETURNING id;
  a concurrent loser matches zero rows and gets a clean 409.

• _restart_or_rerun_job — per-build serialization: a
  SELECT id FROM build WHERE id=%s FOR UPDATE row lock serializes
  concurrent restarts of the same build, so overlapping downstream
  dependents cannot be cloned twice.

• audit_mcp — failure audits guarded with rollback(): audit
  writes share the request's g.db connection. A failed SQL statement
  (write OR read) leaves the connection in InFailedSqlTransaction, so a
  later failure-audit INSERT on the same connection would itself fail and
  be swallowed — losing exactly the failure/error events that matter most.
  Every failure/partial audit inside an except block now calls
  g.db.rollback() first (17 sites, across both write and read endpoints),
  which clears the aborted state so the audit INSERT succeeds and also
  discards any stray uncommitted write (e.g. a restarted=true claim on a
  job-restart path that aborted before commit) so it is not committed by
  the audit. Pre-write "not found"/"forbidden" audits and the seed-missing
  path (which already rolled back) are unchanged. The audit_mcp docstring
  documents the ordering invariants callers must uphold (attempt-before-
  write, success-after-commit, failure-rolls-back-first, no-audit-in-lock).

[liuwei08]

Two high-severity concurrency bugs found during review. Both involve race conditions on write paths that could produce duplicate data or conflicting DB state under concurrent requests. See inline comments for details and suggested fixes.

[liuwei08]

Race condition: restart_counter is not computed atomically

Two concurrent restart requests for the same build_number will both read the same MAX(restart_counter), increment it independently to the same value, and then both attempt to insert a row with that value — causing either a unique-constraint violation or duplicate restart_counter entries.

The POST /trigger handler avoids this correctly by calling LOCK TABLE build IN EXCLUSIVE MODE before computing the next build_number. The same pattern must be applied here before the SELECT max(restart_counter).

Suggested fix:

g.db.execute('LOCK TABLE build IN EXCLUSIVE MODE')

row = g.db.execute_one_dict('''
    SELECT max(restart_counter) AS restart_counter
    FROM build
    WHERE build_number = %s AND project_id = %s
''', [build['build_number'], project_id])
restart_counter = row['restart_counter'] + 1

The lock must be acquired before the SELECT max(...), not after, so that no concurrent transaction can read the same max between the SELECT and the INSERT.

[Jiachen0715]

Done — see 4d18d36. Added LOCK TABLE build IN EXCLUSIVE MODE before the SELECT max(restart_counter), held until commit (also fixed audit_mcp so it no longer commits/releases the caller's transaction, which would otherwise defeat this lock).

[liuwei08]

TOCTOU race: restarted flag check is not atomic

The guard if job['restarted']: abort(400, ...) (a few lines above) and the UPDATE job SET restarted = true here are two separate operations with no lock between them. Two concurrent requests for the same job_id can both pass the check (both read restarted = false), and then both proceed to clone the job, resulting in duplicate cloned rows in the build.

Suggested fix — use an atomic CAS update instead:

# Replace the SELECT + guard + UPDATE pattern with a single atomic operation.
updated = g.db.execute_one_dict('''
    UPDATE job SET restarted = true
    WHERE id = %s AND restarted = false
    RETURNING id
''', [j['id']])
if not updated:
    abort(409, 'Job %s has already been restarted by a concurrent request' % j['id'])

This ensures only one request can claim the job: the second concurrent request will find no rows returned by the RETURNING clause and abort cleanly.

[Jiachen0715]

Done — see 4d18d36. Replaced the read-then-check guard with an atomic CAS: UPDATE job SET restarted=true WHERE id=%s AND restarted=false RETURNING id; 0 rows → 409 so only one concurrent request wins.

[Jiachen0715]

Thanks @liuwei08 — both race conditions confirmed and fixed. While fixing them I audited the rest of the MCP write paths and found a root-cause transaction bug that would have made the fix for (1) ineffective, so I fixed that too. Summary (final state as of d511f8be):

Your findings

1. MCPBuildRestart restart_counter race — fixed. Added LOCK TABLE build IN EXCLUSIVE MODE before SELECT max(restart_counter), matching the pattern MCPTrigger already uses. Held until commit. Also null-guarded max(restart_counter) ((row['restart_counter'] or 0) + 1) so an empty result can't crash with None + 1.

2. restarted flag TOCTOU — fixed with the atomic CAS you suggested: UPDATE job SET restarted=true WHERE id=%s AND project_id=%s AND restarted=false RETURNING id; 0 rows → 409 so only one concurrent request wins.

Additional issues found & fixed

3. audit_mcp's g.db.commit() was committing the caller's transaction (root cause). Because the audit shares the request's g.db connection, its commit() ended the handler's transaction mid-flight — releasing any LOCK TABLE it held and flushing partial rows. This would have defeated fix (1). Final approach: audit_mcp stays on g.db (no extra connection per call), and every failure path now calls g.db.rollback() before the failure audit. This guarantees three things:

   • the failure audit runs on a clean connection instead of an aborted one (a failed SQL statement leaves the connection in InFailedSqlTransaction, so without the rollback the audit INSERT itself fails and the failure log is lost);
   • no stray uncommitted write (e.g. the orphan build on the seed-missing path, or a restarted=true claim on a path that then aborts) gets committed by the audit;
   • the ordering invariants are documented in the audit_mcp docstring — attempt-before-write, success-after-commit, failure-rolls-back-first, no-audit-inside-a-lock.

   A failed SELECT poisons the connection the same way a failed write does, so this guard is applied to the read handlers too, not just the write endpoints — 17 sites in total.

4. Concurrent restarts shared an unlocked build_jobs snapshot — two restarts on the same build could clone overlapping downstream dependents twice. Added SELECT id FROM build WHERE id=%s FOR UPDATE at the top of _restart_or_rerun_job, serializing restarts per build.

End-to-end verification — all 19 MCP tools re-tested on the test env (build_3216) through the real MCP protocol, on a multi-stage DAG build (build → test1/test2 → deploy):

• restart_job cascades to downstream jobs (cloned_count=2: the job + deploy), while rerun_job reruns only the single job (cloned_count=1); the sibling test2 and upstream build are correctly left untouched.
• The CAS de-dup works: a second rerun of the same job row → 400 "already restarted"; rerunning the newly-cloned job still succeeds (the flag is per job row, not per logical step).
• abort_job kills one job (downstream skipped); abort_build kills the whole build (Aborted 5 job(s)), each with Aborted by <user> recorded.
• allow_trigger gate verified: with it revoked, all write tools return 403; read tools are unaffected; re-granting restores writes immediately.
• The two previously-500 read tools (get_job_stats, list_job_artifacts) now return correctly.

[liuwei08]

One deterministic crash found: MAX(restart_counter) returns NULL when no build row matches the build_number, causing None + 1 TypeError on every such request. See inline comment.

[liuwei08]

Crash bug: TypeError: unsupported operand type(s) for +: 'NoneType' and 'int'

MAX() returns NULL (mapped to Python None by psycopg2) when no rows match the WHERE clause — which happens whenever build_number does not yet exist in the build table for this project (e.g. the very first restart attempt for a freshly-inserted build, or any edge case where build_number is unique so far).

None + 1 raises TypeError, the except block catches it and re-raises, Flask returns HTTP 500. This is a deterministic crash, not a race.

Fix:

restart_counter = (row['restart_counter'] or 0) + 1

Note: row itself will never be None here because MAX() always returns a row (with a NULL value), so the row is None case does not apply — only the value inside the row needs the null guard.

[Jiachen0715]

Good catch — fixed in 64be938: restart_counter = (row['restart_counter'] or 0) + 1, matching the COALESCE pattern MCPTrigger already uses for build_number. Agreed the row itself is never None (MAX always returns a row), only the value needed the guard.

[Jiachen0715]

Update on the audit fix (commit d511f8b).

I've kept audit_mcp on the shared g.db + commit form and guarded every failure path with an explicit g.db.rollback(), rather than moving audit onto a separate connection. This avoids opening a fresh connection per audit call (2-3 per request) while still fully fixing the transaction bugs.

What the rollback approach fixes, and why it's applied everywhere:

• The failure audit shares g.db. After any failed SQL statement the connection is in InFailedSqlTransaction, so the audit INSERT on that same connection would itself fail and be swallowed — losing the failure/error log. A g.db.rollback() before the audit clears the aborted state so the INSERT succeeds.
• A failed SELECT poisons the connection just like a failed write, so this isn't only about the write endpoints — the read handlers (list/get builds, jobs, log, artifacts, stats, testruns, manifest, projects) need the same guard, or their failure audits are lost too. That's why the rollback is added at 17 sites, not just the 6 write endpoints.
• On the restart_job/rerun_job paths that abort after the restarted=true claim (connection not poisoned, but a stray uncommitted write pending), the rollback also discards that stray write so the audit's commit doesn't persist it.

Pre-write "not found"/"forbidden" audits and the seed-missing path (which already rolled back) are unchanged. I've documented the ordering invariants this relies on in the audit_mcp docstring (attempt-before-write, success-after-commit, failure-rolls-back-first, no-audit-inside-a-lock).

[liuwei08]

LGTM