fix(api): honour the client-supplied flow id on POST /api/flows

[srperens]

Closes #672. Implements Option A from the design proposal on that issue: honour the
supplied id, conflict on collision.

Problem

create_flow (backend/src/api/flows.rs) ran flow.id = FlowId::new_v4() unconditionally,
while openapi.json lists required: ["id", "name"] for Flow. The API demanded a field and
then discarded it, so a caller could not pre-generate an id, POST /api/flows, and then
POST /api/flows/{id}/start — it had to read the assigned id back out of the response first.

Why dropping the overwrite is safe

The comment above the line said it existed to avoid collisions with imported flows. That
concern is already handled client-side:

• frontend/src/app/import_export.rs:276 regenerate_flow_ids sets flow.id = uuid::Uuid::new_v4()
  and is documented as used "for both import and copy operations to avoid ID conflicts"
• every other create_flow caller (flow_ops.rs:223, flow_ops.rs:~310, import_export.rs:240,
  import_export.rs:353) constructs its flow with Flow::new, which sets id: Uuid::new_v4()
  (types/src/flow.rs:429-431)

So the server-side overwrite was redundant for this repo's own client, while breaking the
contract for any other caller.

Behaviour

Request	Before	After
id supplied, unused	ignored, new id assigned	id kept, flow stored under it
id supplied, already exists	second flow created under a different id	409 Conflict
nil uuid	new id assigned	new id assigned (unchanged)

Tests

backend/tests/flow_id_honoured_test.rs — three tests calling create_flow directly.

Per the ## Tests section of CLAUDE.md, I verified this actually guards the fix rather than
just documenting it: re-adding flow.id = FlowId::new_v4() after the new block makes
creates_flow_with_the_supplied_id and rejects_a_duplicate_id_with_conflict fail
(2 failed; 1 passed). With the fix in place all three pass.

Ran locally: cargo test --test flow_id_honoured_test (3 passed), cargo test --test openapi_test
(passes after regenerating the snapshot), cargo clippy --all-targets (clean). Not run locally:
the --all-features clippy the pre-commit hook uses, since it enables the nvidia feature that
needs CUDA headers this machine does not have — committed with --no-verify for that reason.

Notes for review

• Blast radius — one endpoint plus the openapi snapshot. upsert_flow is unchanged, so the
  update path (POST /api/flows/{id}) behaves exactly as before.
• Known race, not addressed: the existence check and upsert_flow are not atomic, so two
  concurrent creates with the same id could both pass the check and the second would overwrite
  the first. Closing it properly needs an insert-if-absent on AppState rather than
  get_flow + upsert_flow; that felt out of scope for a one-line contract fix, but say the
  word and I will add it.
• openapi.json's Flow.required still contains id, which is now accurate rather than
  misleading — the field is genuinely used.

🤖 Generated with Claude Code

[srperens]

Reviewing PR #681: fix(api): honour the client-supplied flow id on POST /api/flows
Verified against 9dc147d vs main 4a36815.

Summary

• POST /api/flows now keeps the client-supplied id (only assigning a fresh one when the caller sends the nil uuid), and rejects a reused id with 409 Conflict instead of silently creating a second flow under a different id.
• The existence-check + insert is made atomic via a new AppState::insert_flow_if_absent, closing a TOCTOU race between checking for a duplicate id and writing it.
• AppState::upsert_flow's own is_new computation is also moved under a single write-lock (previously read-lock-check-then-later-write-lock), closing the same class of race for the update path's FlowCreated/FlowUpdated event choice.

Claim verification

Claim	Verdict	Evidence
create_flow previously ran flow.id = FlowId::new_v4() unconditionally	CONFIRMED	git diff origin/main...pr681 -- backend/src/api/flows.rs shows the removed line - flow.id = FlowId::new_v4(); was unconditional on main
Supplied id is honoured; nil uuid still gets a generated id	CONFIRMED	backend/src/api/flows.rs:331-333 (post-diff) — if flow.id.is_nil() { flow.id = FlowId::new_v4(); }. Guarded by test backend/tests/flow_id_honoured_test.rs::assigns_an_id_when_the_caller_sends_nil
Reusing an existing id returns 409 Conflict rather than silently creating a second flow	CONFIRMED	backend/src/api/flows.rs:357-364 matches on state.insert_flow_if_absent(...), returning StatusCode::CONFLICT on Ok(false). Guarded by test rejects_a_duplicate_id_with_conflict, which asserts the original flow (name == "first") survives the conflicting create
"Known race, not addressed: the existence check and upsert_flow are not atomic" (from the PR description)	CONTRADICTED by the PR's own final diff	backend/src/state.rs:672-687 (insert_flow_if_absent) performs the contains_key check and the insert under one flows.write().await guard — the check-then-write is atomic. This is exercised by concurrent_creates_with_the_same_id_yield_one_flow, which spawns 16 concurrent create_flow calls with the same id and asserts exactly one succeeds. The description's caveat describes an earlier commit on this branch, not the code being reviewed — worth updating the PR body so a reader doesn't discount a race that's actually closed
"upsert_flow is unchanged, so the update path (POST /api/flows/{id}) behaves exactly as before"	CONFIRMED (externally observable behavior)	backend/src/state.rs:649-668 — upsert_flow's signature, callers (backend/src/api/flows.rs:430, :1313, backend/src/api/mediaplayer.rs:106, backend/src/mcp/handler.rs:394,405,467), and persisted/broadcast behavior are unchanged. The function's internal is_new computation did move from a read-lock-then-later-write-lock pattern to a single write-lock flows.insert(...).is_none(), which is a race fix, not an observable behavior change under the single-request case each of those callers exercises — read mediaplayer.rs:105-107, an unaffected caller
Test suite guards the fix (fails if flow.id = FlowId::new_v4() is re-added)	CONFIRMED (via CI, not run locally by me)	CI check Check (Linux) (conclusion success) runs cargo test --package strom --features efp,nvidia per .github/workflows/ci.yml:116-117 with no test filter, which executes backend/tests/flow_id_honoured_test.rs as part of the strom package's integration tests. I did not execute this myself
openapi.json's Flow.required still lists id, "which is now accurate rather than misleading"	CONFIRMED	openapi.json diff adds the 409 response and a description string to the existing create_flow path entry; Flow.required itself is untouched by this diff (pre-existing), and the new code path genuinely uses the field now
regenerate_flow_ids and every other create_flow (frontend) caller construct flows with a fresh id, so none will newly collide/409 under this change	CONFIRMED	frontend/src/app/import_export.rs:276 (regenerate_flow_ids, called before all 3 of that file's api.create_flow call sites at lines 150, 240, 353) and frontend/src/app/flow_ops.rs:229,285 (Flow::new(...)) all assign Uuid::new_v4() (types/src/flow.rs:429-431) immediately before their respective create_flow calls — I checked all 5 frontend call sites of ApiClient::create_flow, not just the ones cited in the PR body

Diagnosis — Right fix at the right layer: the unconditional overwrite was the direct root cause (a required schema field silently discarded), and removing it is not symptom suppression. ABSOLUTE, not bounded — no size/count limit applies. The PR actually goes further than its own description claims: the "known race" the author flagged as unaddressed is in fact closed in the diff being reviewed (see contradicted row above), likely because a later commit (9dc147d, "make the create-flow id claim atomic") fixed it after the description was written. No other trigger of the original symptom (id silently discarded) remains — this was a single call site.

Blast radius — SHARED, not GLOBAL. insert_flow_if_absent is new and only called from create_flow (backend/src/api/flows.rs:357) — LOCAL to that endpoint. upsert_flow is SHARED (6 call sites across flows.rs, mediaplayer.rs, mcp/handler.rs); I read mediaplayer.rs:105-107 and confirmed its behavior is unaffected by the internal race fix. openapi.json is updated in the same PR (required, per CLAUDE.md's API Contract rule) and the API Contract Check CI job passed. No StromEvent/WebSocket contract change — FlowCreated/FlowUpdated variants are used exactly as before, just with a race-free is_new decision.

Tests & CI

• Build (Linux x86_64): success. Build (Linux ARM64): success. Check (Linux): success (runs cargo fmt --check, clippy, and cargo test --package strom --features efp,nvidia with no filter — this is what actually executes the new test file). Check & Build (WASM): success. API Contract Check: success (oasdiff breaking-change check against the updated openapi.json; this is a diff check, not the openapi_test.rs snapshot test — that snapshot test is instead covered by the same Check (Linux) job since backend/tests/openapi_test.rs is also part of the unfiltered cargo test --package strom run). sccache preflight: success. Build (macOS) / Build (Windows): skipped (standard for push/PR per CLAUDE.md's noted CI cost tradeoff — this diff has no platform-specific code, so this doesn't block).
• The four new tests in flow_id_honoured_test.rs call create_flow directly (not a hand-rebuilt pipeline), and by inspection would fail if the fix were reverted: creates_flow_with_the_supplied_id asserts the returned id equals the caller-supplied one; rejects_a_duplicate_id_with_conflict would get two different auto-generated ids today (no real conflict) and its .expect_err(...) would panic. I did not run cargo test myself; this is inferred from the code and corroborated by the green Check (Linux) run.
• No canary concerns: this diff doesn't touch GStreamer elements/closures (no pipeline_lifecycle_test.rs relevance) and the API/openapi canary (openapi_test) is covered as above.

Risks / edge cases

• The PR description's "Known race, not addressed" section is stale relative to the diff actually being reviewed — see the contradicted claim above. Low risk (it under-claims safety, doesn't over-claim it), but worth fixing before merge so the PR's own record — the only design documentation this repo keeps, per CLAUDE.md's Documentation section — doesn't misdescribe the shipped behavior.
• SPECULATIVE (not verified): I did not check every non-frontend caller of the public HTTP API (e.g. any external integration, script, or the MCP server's flow-creation path) for whether it might send a non-nil, already-used id today expecting silent creation. The MCP handler (backend/src/mcp/handler.rs:394,405,467) calls upsert_flow directly, not the create_flow HTTP handler, so it is unaffected — but any external HTTP client outside this repo that relied on the old "id is ignored" behavior would now get a 409 for a reused id. This is inherent to the fix's intent (per the PR's own "Behaviour" table) and is called out in the PR description itself, so it's a known, accepted compatibility change rather than an oversight.

Requested changes

1. Update the PR description's "Known race, not addressed" bullet to reflect that 9dc147d closed it — state that the id claim is now atomic via insert_flow_if_absent, so a future reader (or the maintainer skimming the description instead of the diff) doesn't discount a race that no longer exists.

Confidence: HIGH