Four API follow-ups from the codebase review (#197)

[richardmhope]

The API section of #197. The core/connectors and web/deploy sections follow in their own PRs; #197 stays open until all three land.

1. A duplicate name raced to a 500

Sources, notification channels, hand-over targets, owner groups and routing rules all answer "is this name free?" with a SELECT and then an INSERT. That guards an operator retyping a name already in use, and nothing else — two concurrent creates both find nothing, both insert, and the loser gets an unhandled IntegrityError for a condition the route already has a 409 and a message for.

commit_or_conflict is register_engine's existing catch-rollback-409, factored out and applied at ten call sites (five creates, five renames). Two deliberate details:

• Only a unique violation converts. A foreign key or a check constraint is a bug, and answering "that name already exists" would send an operator looking in entirely the wrong place. Read from SQLSTATE 23505 where the driver has one, falling back to the message only on SQLite — so a Postgres deployment never depends on matching English.
• The rollback is as load-bearing as the status. A session holding a failed transaction fails every later statement on it, including the ones a dependency runs on the way out of the request. There's a test for exactly that.

All five models carry precisely one unique constraint (name), so the message cannot misattribute.

2. A lock-order inversion between the two engine writers

Both handlers take the same two row locks — the scan FOR UPDATE, and the task through a conditional UPDATE:

	first	second
submit_progress	scan	task (advance_checkpoint)
submit_results (before)	task (claim_result)	scan

Textbook AB/BA, reachable when a retried progress races the same task's final results. Postgres aborts one side and the engine's retry ladder recovers, so nothing was ever lost — it was simply free to make impossible. Results now takes the scan first.

Pinned by reading the source with ast, because SQLite has no FOR UPDATE at all: no test that runs these routes could observe the ordering. Confirmed to fail against the old order (assert 13 < 12).

3. Cancelling a scan threw away the coverage its tasks had reported

cancel_scan set coverage = failure_report(...) in a bulk UPDATE, replacing a running task's accumulated per-batch report with zeros. So a cancelled scan's manifest said a task that had demonstrably ingested findings scanned nothing — the opposite of what a manifest is for.

The cancellation gap is merged onto the stored report instead (coverage.cancelled_report). The bulk statement becomes one conditional UPDATE per row, which keeps the property that mattered: a task reaching a terminal state mid-cancellation still keeps what it reported.

4. Ingest was an N+1 on the busiest transaction in the API

One point select per finding payload — two during a pepper-rotation window — inside the transaction holding the scan row locked, so a 500-finding batch paid it in lock time as well as round trips. Both identities now load in one pass.

The map is kept live, not treated as a snapshot, and this is the subtle part: the per-payload select was seeing pending inserts through autoflush, so the loop now registers what it creates and re-keys what it moves. Without that, a duplicate fingerprint inside one batch turns from a harmless repeat into an insert the unique constraint refuses.

Measured: 20 payloads went from 20 selects to 2, and the test asserts the count doesn't move with batch size rather than pinning a magic number.

Validation

Every fix has a test confirmed to fail against the code before it — I stashed each change and re-ran. New: test_conflicts.py, test_ingest_queries.py, plus tests in test_coverage_manifest.py and test_scan_progress.py.

make check green: ruff, mypy, docs check, 1972 passed / 2 skipped.

---

Generated by Claude Code

[icebergai-review-bot]

Verdict

CHANGES_REQUESTED

Completed bounded review across 1 immutable scope(s). 1 high-severity correctness regression found.

Scope health

Convergence: healthy. Review mode: initial.
Recommended action: CONTINUE_INCREMENTAL.

• No escalation signals.

Prior findings

Finding	Status
—	No prior finding state

New findings

Root cause: The merge is computed before the conditional UPDATE, but the UPDATE protects only terminal-state transitions, not intervening coverage changes.

• BLOCKER · high: Cancellation can still overwrite concurrently reported coverage — apps/api/src/iceberg_api/scans/service.py
  Status: NEW. Attribution: new_in_scope.
  cancel_scan reads each nonterminal task and derives cancelled_report(task) from that in-memory coverage, then later conditionally updates only on task status. A concurrent progress submission can commit newer coverage while the task remains nonterminal; the cancellation UPDATE still matches and writes the stale merged report.
  Invariant: Cancellation must preserve all coverage successfully reported before it takes effect.
  Ownership: Scan cancellation and engine progress submission transactions.. Behaviour: Concurrent cancellation of a scan with an actively reporting task..
  Evidence: Reproduction: cancellation reads a running task with scanned=7; before its per-task UPDATE, a progress request records scanned=10 and leaves the task running; cancellation's status NOT IN terminal predicate remains true and overwrites coverage with a cancellation report merged from 7. The new loop has no task/scan lock or coverage-version predicate to prevent that stale write.
  Independent assessment: The new select-then-per-row update derives coverage from a stale task snapshot while its UPDATE predicate guards only status. A concurrent nonterminal progress write can update coverage after the select and before the cancellation UPDATE, which still matches and overwrites the newer coverage.

Fix-induced regressions

• None evidenced.

Uncertainty

• No material uncertainty recorded.

Validation

• Reviewed the supplied immutable diff; exact-head CI was reported as passed.

Residual risks

• None identified.

[icebergai-review-bot]

Verdict

APPROVE

Completed bounded review across 1 immutable scope(s). No findings. The follow-up serializes cancellation with engine coverage writers using the shared scan-row lock before cancellation reads task coverage.

Scope health

Convergence: healthy. Review mode: delta.
Recommended action: CONTINUE_INCREMENTAL.

• No escalation signals.

Prior findings

Finding	Status
F-3684AFBC53B3	Resolved

New findings

No new findings.

Fix-induced regressions

• None evidenced.

Uncertainty

• No material uncertainty recorded.

Validation

• Prior blocker F-3684AFBC53B3 resolved: cancel_scan executes select(Scan)...with_for_update() before iterating tasks and invoking cancelled_report; the added AST test verifies that ordering.
• Reviewed the supplied immutable diff at head 004c52f.
• Exact-head CI is reported as passed.

Residual risks

• None identified.