fix(webui): add send-message idempotency replay guards

[think-in-universe]

Summary

• scope WebUI send-message idempotency replays across the authenticated caller context instead of per-thread keys
• reject reuse of the same client_action_id on another thread with a stable 409 Conflict
• reconcile concurrent duplicate submissions after turn-coordinator dedupe so duplicate browser sends preserve the accepted message outcome
• add contract tests for cross-thread duplicate rejection and concurrent duplicate behavior

Testing

• cargo test -p ironclaw_product_workflow --test reborn_services_contract -- --nocapture

Issue

• Closes [Reborn WebUI Beta] Add WebUI idempotency and accepted-message ledger #3625

[Copilot]

Pull request overview

This PR fixes WebUI send-message idempotency so duplicate browser submissions are deduped per authenticated caller (project/agent/actor) rather than per thread, and so concurrent duplicate sends still resolve to the same accepted message. It also adds a stable 409 Conflict rejection when the same client_action_id is reused on a different thread, plus reconciliation logic to recover from races between two mark_message_* writers after the turn coordinator has already deduped.

Changes:

• Drop thread_id from webui_source_binding_id and add project_id, so the (source_binding_id, external_event_id) idempotency bucket spans threads within a caller context.
• In submit_turn, detect cross-thread reuse of a previously-accepted client_action_id and return Conflict/409; replace direct mark_message_submitted / mark_message_deferred_busy calls with helpers that fall back to a replay lookup when the write loses a race.
• Add contract tests for cross-thread duplicate rejection and concurrent duplicate submit collapsing to a single message/run.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
crates/ironclaw_product_workflow/src/reborn_services.rs	Re-scopes WebUI source binding key, adds cross-thread conflict guard, and introduces mark_message_*_or_replay + reconcile_terminal_duplicate helpers for concurrent duplicate handling.
crates/ironclaw_product_workflow/tests/reborn_services_contract.rs	Adds tests for cross-thread client_action_id reuse rejection and for concurrent duplicate sends using DefaultTurnCoordinator + InMemoryTurnStateStore.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gemini-code-assist]

Code Review

This pull request implements a cross-thread guard to prevent idempotency key reuse across different threads and shifts the idempotency scope to the project level. It also introduces helper functions to reconcile terminal duplicate submissions, ensuring consistent outcomes during concurrent race conditions. The reviewer validated these changes and provided architectural references for locking strategies and state management.

I am having trouble creating individual review comments. Click here to see my feedback.

crates/ironclaw_product_workflow/src/reborn_services.rs (134-140)

This cross-thread guard is a critical addition for preventing client_action_id reuse across different threads within the same authenticated context. Returning a 409 Conflict here correctly signals to the client that the idempotency key is already bound to another resource (the other thread).

References

1. To avoid performance bottlenecks, use a granular, keyed locking strategy (e.g., by tenant, user, or agent) instead of a single global lock for a data store, allowing for concurrent operations on different resources.

crates/ironclaw_product_workflow/src/reborn_services.rs (463-488)

The implementation of reconcile_terminal_duplicate correctly handles concurrent race conditions where the turn-coordinator has already de-duplicated the submission but the local thread-service state update hasn't been observed yet. By re-fetching the replay and verifying the thread_id, message_id, and terminal status (including the run_id for submissions), it ensures that duplicate browser sends preserve the accepted message outcome without returning a conflict error to the user.

References

1. When syncing job states, JobState::Accepted can be treated as a terminal success state, but JobState::Submitted should be treated as an in-progress state as it can still transition to Failed.

crates/ironclaw_product_workflow/src/reborn_services.rs (621-624)

Removing the thread segment and adding the project segment to the source_binding_id effectively shifts the idempotency scope from per-thread to the broader authenticated caller context (tenant/agent/project/actor). This is the key change that enables the cross-thread rejection logic in submit_turn.

References

1. To avoid performance bottlenecks, use a granular, keyed locking strategy (e.g., by tenant, user, or agent) instead of a single global lock for a data store, allowing for concurrent operations on different resources.

[think-in-universe]

PR review (#3694)

Summary

This PR tightens WebUI send-message idempotency by:

1. Caller-scoped binding keys — webui_source_binding_id stops including thread_id and includes project_id (plus existing agent/actor segments), so the same client_action_id is not treated as a fresh idempotency scope per thread.
2. Explicit cross-thread replay rejection — If a replay ledger hit exists but replay.thread_id != scope.thread_id, the API returns 409 Conflict instead of replaying another thread’s outcome.
3. Post–turn-coordinator duplicate reconciliation — After a successful turn handoff, mark_message_submitted / mark_message_deferred_busy failures call reconcile_terminal_duplicate, which replays by (source_binding_id, client_action_id) and only succeeds when thread, message id, and terminal status (including matching run_id for submitted) align—so concurrent duplicate submits can still converge to one accepted outcome.

Contract tests add cross-thread 409 behavior and concurrent duplicate behavior using DefaultTurnCoordinator + InMemoryTurnStateStore, which is stronger than only exercising FakeTurnCoordinator.

What looks good

• Safety: Thread mismatch on replay is the right guard; without it, a reused id could surface the wrong thread’s message/run.
• Race handling: The reconcile path is narrow (replay + structural match + status predicate) and falls back to the original thread error if the replay does not prove a benign duplicate.
• Tests: Cross-thread rejection plus real coordinator concurrency cover the two failure modes called out in the PR body.
• HTTP semantics: 409 for “same idempotency key, different resource expectation” matches common API practice.

Questions and risks

1. project_id optional / empty
   The new binding uses project_id with unwrap_or(""). If any WebUI path omits project_id, distinct logical tenants could share the same binding prefix and see spurious 409s or broader idempotency coupling than intended. Worth confirming that send-message always has a stable, non-empty project scope (or documenting that empty project collapses to a single global bucket for that actor).

2. Intentional breaking change for clients
   Same client_action_id on different threads under the same project/actor will now conflict. That matches the PR summary; release notes or client guidance should say that ids must be unique per caller/project (and agent) scope, not per thread.

3. reconcile_terminal_duplicate and matches_replay
   The submitted branch closes over run_id for equality with replay.turn_run_id. As long as run_id formatting is identical on both sides (same string serialization as stored in the ledger), this is correct; if either side normalizes differently, benign duplicates might still surface as errors.

4. Observability (optional)
   Structured logs or metrics when returning 409 for thread mismatch vs cross-thread reuse could speed up future debugging—not a blocker.

Verdict

The change set is focused, the 409 on wrong-thread replay is important for correctness, and the reconcile-after-handoff logic is a sensible fix for coordinator-level dedupe racing with thread terminalization. The main thing to validate before merge is project_id always populated for WebUI sends (or an explicit product decision for the empty-project case).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: acf7c7d36f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve legacy send idempotency keys

For retries of WebUI send-message requests that were accepted before this change, the persisted inbound idempotency record was written under the previous source binding format that included the thread id. This new key is used for the only replay lookup, so a client retry that crosses a deploy will miss the old record and create a second accepted message/turn instead of replaying the original outcome. Please add a same-thread fallback lookup for the legacy binding or migrate existing records before changing the key format.

Useful? React with 👍 / 👎.

[think-in-universe]

Addressed in 6032dc7a5.

I added a same-thread fallback replay lookup for the legacy WebUI send binding format and now preserve the stored binding id when a retry replays an older accepted message. That keeps deploy-crossing retries on the original accepted message / turn instead of accepting a duplicate under the new caller-scoped key.

Regression coverage:

• same_thread_retry_replays_legacy_submitted_message_after_binding_key_change
• same_thread_retry_reuses_legacy_accepted_message_without_creating_duplicate

[serrrfirat]

Architecture alignment request: this PR has the right WebUI idempotency behavior, but it should remain part of the single WebUI facade path and be reconciled with #3725.

Please avoid introducing or depending on a second route parallel to ProductWorkflow. Proposed split:

• WebUI browser sends: RebornServicesApi / RebornServices, using SessionThreadService accepted-message replay for client_action_id idempotency.
• External product/channel adapters: ProductWorkflow, using the product inbound IdempotencyLedger and canonical conversation binding.

Follow-up PR #3727 prepares the ProductWorkflow side for the adapter/channel slice. If this PR needs shared product-adapter behavior, please point it through that ProductWorkflow surface instead of adding a separate WebUI/product workflow path. The WebUI-specific fixes here should stack onto or merge with #3725 so we end up with one WebUI facade, not multiple parallel ingress implementations.

[think-in-universe]

@copilot resolve the merge conflicts in this pull request

[Copilot]

PR review (#3694)

Summary

...

Covered in 0a5abe5. The WebUI binding no longer collapses a missing project to an empty segment; it encodes project_scope:none, and the contract tests now cover both the no-project cross-thread 409 case and per-project isolation.

Architecture alignment request: this PR has the right WebUI idempotency behavior, but it should remain part of the single WebUI facade path and be reconciled with #3725.

Please avoid introducing or d...

Addressed in 0a5abe5. I merged reborn-integration and kept the idempotency logic on the single WebUI facade path (RebornServices / RebornServicesApi) while leaving ProductWorkflow as the adapter/channel ingress surface.

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts in 0a5abe5.

[serrrfirat]

Reviewed again with the Reborn rollout context in mind. No blocking findings.

The current shape looks right:

• WebUI send idempotency stays inside the RebornServices facade.
• client_action_id replay is scoped to caller/project/agent/actor instead of per thread.
• Same-key reuse across another thread returns 409 before creating the second thread/message.
• The concurrent duplicate test uses the real DefaultTurnCoordinator/InMemoryTurnStateStore, which is the important race path here.

The legacy binding fallback is not a merge blocker given Reborn is not live. It is extra compatibility coverage inside the Reborn path, not a dependency on the old product workflow path.

I did not find a maintainer-level correctness issue in this revision.