Run /api/analyze synchronously: single-transaction job persistence

[01painadam]

What & why

Addresses the review concern on #762 about the BackgroundTasks.add_task() lifecycle, and — following the follow-up design question in this PR's review — removes the Job/JobResource abstraction entirely rather than keeping it as a synchronous shell.

The reviewer's observation held up: JobType had exactly one value, /api/analyze was the only job creator, GET /api/jobs/{id} had no frontend caller, and every job produced exactly one resource pointing at exactly one insight. With the analysis now synchronous, the layer was pure indirection.

POST /api/analyze now mirrors the agent/chat path:

POST /api/analyze
  ├─ 1. Validate dataset_id                        (422 if unknown — unchanged)
  ├─ 2. Pull data from analytics API + generate charts    (nothing persisted yet)
  ├─ 3. ONE transaction via persist_insight: InsightOrm + InsightChartOrm
  └─ 4. Return 200 InsightResponse — same shape as GET /api/insights/{id}

Either the insight exists or nothing does. A failed analysis persists nothing and surfaces as a plain HTTP error — the same philosophy as the analyst subagent's chat path, where failed chart generation persists nothing either.

Design decisions

• Response is the insight itself (InsightResponse): saves the client the resource_url follow-up round-trip; the insight remains re-fetchable via GET /api/insights/{id} and listable via GET /api/insights.
• Failure → HTTP error, not a persisted failed row: 502 when the analytics pull fails or errors, 504 when it exceeds the in-request budget (asyncio.timeout(50s), kept below a typical 60s gateway idle timeout). Failures stay observable through structured logs (analysis_failed / analysis_timed_out, severity=high) rather than DB rows nothing reads.
• persist_insight is again the single write entry point: the session-accepting add_insight split existed only for the job transaction, so it's folded back.
• Migration drops jobs/job_resources (d4f8a2b6c1e3), including the updated_at trigger + function. This discards existing job rows — they were only ever written by this endpoint's previous incarnations and nothing reads them; the insights they pointed at are untouched. Downgrade recreates the tables and trigger.

Breaking API changes

• POST /api/analyze returns InsightResponse instead of JobResponse.
• GET /api/jobs/{id} is removed. No frontend caller exists in this repo; if the FE poll loop shipped anywhere, it must switch to reading the analyze response directly.

Commits

1. feat: add single-transaction job persistence to the repo layer
2. feat: run /api/analyze synchronously, persist the job in one transaction
3. refactor: drop the background-task job lifecycle methods
4. refactor: drop the Job/JobResource layer; /api/analyze returns the insight — the removal, per review

Tests

• tests/unit/api/services/test_analysis_runner.py (replaces the job-runner tests): insight id return, charts-only insight, thread_id pass-through, upstream failure / exception / timeout each raise and persist nothing, persistence-error propagation.
• tests/api/test_analyze.py (rewritten): synchronous insight response, insight retrievable via GET /api/insights/{id}, upstream failure → 502 with zero rows persisted.
• tests/api/test_job_repository.py removed with the layer.
• Migration verified both directions against a scratch DB (upgrade drops tables + trigger function; downgrade recreates them).
• Full unit/api tiers green locally (the 4 pre-existing test_aois.py failures are local-env only: no PostGIS; CI uses postgis/postgis). pre-commit (ruff + mypy) clean.

Non-goals

The long-term scaling concern (many users × slow analytics pulls holding open connections) is unchanged. If a queue lands later, a job/polling contract can be reintroduced then — designed against a real second consumer instead of speculatively.

Stacking

Based on feat/curated-insights-registry (#762). No file overlap with #763–#766.

[yellowcap]

Question on the design here rather than the implementation: now that /api/analyze runs synchronously and returns a terminal job in one response, is the Job/JobResource abstraction (JobOrm, JobResourceOrm, JobRepository, the jobs router) still pulling its weight?

A few things that made me question it:

• JobType has exactly one value (ANALYSIS) and analyze.py is the only caller of JobRepository.create_* — it was introduced in Analyze endpoint #704 as a generic long-running-operation pattern, but a second job type/consumer never materialized.
• GET /api/jobs/{id} has no list endpoint and (in this repo at least) no frontend caller — its only consumers are its own docstring and this PR's tests.
• ResourceStatus also has exactly one value (COMPLETED); every job produces exactly one resource pointing at exactly one insight, so the "job → multiple resources" generality has never been exercised either.
• The one thing JobOrm gives us that InsightOrm doesn't is a persisted failed row. But the analyst subagent's chat path (src/agent/subagents/analyst/tool.py) does the same chart-generation work in-loop and, on failure, just returns an error ToolMessage — nothing is persisted. So we already have two different philosophies for the same failure case, and /api/analyze is the outlier that writes a DB row for it.

Now that the original justification for Job (long-running + pollable) is gone, would it be simpler to drop JobOrm/JobResourceOrm/the jobs router/JobRepository entirely and have /api/analyze mirror the chat path — call persist_insight/add_insight directly, return the insight resource on success, and surface failure as a normal HTTP error rather than a persisted failed row? Happy to be told there's a product reason (e.g. wanting a durable, queryable audit trail of failed analyze calls) that I'm missing — just doesn't seem to be exercised anywhere today.

[yellowcap]

Let's remove the JobOrm, see comment I left with the 🤖

[01painadam]

Agreed on all points — there's no product requirement for a durable audit trail of failed analyze calls, so the failed-row behaviour was incidental, not intentional. Done in 3fbb1f8:

• JobOrm/JobResourceOrm, JobRepository, the jobs router and the Job schemas are gone; migration d4f8a2b6c1e3 drops the jobs/job_resources tables (and the updated_at trigger function). Existing job rows are discarded — nothing reads them, and the insights they pointed at are untouched.
• POST /api/analyze now mirrors the chat path exactly as you suggested: success goes through persist_insight (the add_insight split is folded back too, since the job transaction was its only reason to exist) and returns the insight itself — same shape as GET /api/insights/{id}, saving the resource_url round-trip.
• Failure persists nothing and surfaces as 502 (upstream failure) or 504 (in-request timeout); observability moves to the structured logs (analysis_failed/analysis_timed_out, severity=high), which is where the chat path's failures already live.

One deliberate loose end: if the queue-backed future ever materialises, a job/polling contract can be reintroduced then, designed against a real second consumer. PR description updated to match.