[AI-3436] Deliver flow results through a server restart and a stale token

[alexeyzimarev]

AI-3436 (no GitHub issue; the server half is kurrent-io/kcap-server#2233)

What & why

A hosted reviewer's submit_review_result was lost across a server restart in two ways. A 503 from the ingress went straight back to the reviewer as an error. A retry after a 401 then resent a stored token that differed from the rejected one but had itself expired, because forced rotation only refreshed a token equal to the rejected one. The flow-result channel now retries 502/503/504 and transport failures for up to 240 s. Forced rotation refreshes any expired stored token. Codex gets tool_timeout_sec=300 on that channel so it does not abandon the call first.

Where to look

TokenStore forced rotation now refreshes a different-but-expired stored token under the profile lock, so the single-use WorkOS refresh token spent is the one the store holds. The retry deadline is checked before each wait plus attempt, so a call ends within 240 s. Other vendors have no per-server tool timeout.

Verification

Reproduced the incident sequence in tests: a memoized token, a peer-written expired token, then a 401. The resend now carries a freshly refreshed token, and that test fails without the fix. Submit and send ride out 503s, 504s and connection resets, keep their message id, and give up with the retry hint after the window. A coded 503 and stale_round_token are not retried. A release AOT publish of the CLI shows no IL or AOT warnings.

🤖 Generated with Claude Code

[linear-code]

AI-3436

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-03T09:31:44.710402Z	7f2da5c	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[qodo-code-review]

PR Summary by Qodo

Keep flow results deliverable through restarts and expired tokens

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Retry flow-result delivery through server restarts and transport failures within a bounded window.
• Refresh expired stored credentials after a 401 instead of resending them.
• Give Codex enough time to complete delivery and test the failure paths.

Diagram

graph TD
  Codex["Codex reviewer"] --> MCP["Flow-result channel"] --> Retry{"Retry policy"} --> API["Flow API"]
  API -->|"uncoded gateway failure"| Retry
  API -->|"401"| Recovery["Token recovery"] --> Store["Token store"]
  Store -->|"refreshed credential"| MCP

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Leave retries to the reviewer

• ➕ Avoids retry logic in the result channel.
• ➖ A reviewer may stop after a transient failure, losing the result.
• ➖ Cannot reliably distinguish ingress failures from terminal server responses.

2. Persist results in a durable outbox

• ➕ Could continue delivery after the tool process exits.
• ➖ Requires a larger persistence and delivery lifecycle.
• ➖ Adds complexity to the result channel and borrowed-reviewer path.

Recommendation: Keep the PR's bounded in-channel retries: they address server restarts without adding durable infrastructure, while preserving terminal server responses and staying below Codex's tool timeout.

Files changed (9) +385 / -87

Bug fix (2) +123 / -84

TokenStore.csRefresh expired stored tokens during 401 recovery +14/-11

Refresh expired stored tokens during 401 recovery

• Forced rotation now refreshes a stored token that has expired even when it differs from the rejected token. The raw-read fallback withholds that differing expired token if rotation cannot complete, while retaining the existing server-binding check.

src/Capacitor.Cli.Core/Auth/TokenStore.cs

McpFlowResultServer.csShare bounded restart retries across result and message delivery +109/-73

Share bounded restart retries across result and message delivery

• Submit and send now use a shared retry path for transport failures and uncoded 502, 503, and 504 responses, with attempt timeouts and a delivery deadline. Coded server responses retain their distinct handling, and message retries reuse the same message ID.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs

Tests (5) +255 / -3

RefreshEndpointStub.csAdd a recording refresh-endpoint stub +24/-0

Add a recording refresh-endpoint stub

• Provides a refresh response for auth tests and records the access token presented, allowing tests to verify which stored credential was refreshed.

test/Capacitor.Cli.Core.Tests.Unit/Auth/RefreshEndpointStub.cs

TokenServerBindingTests.csCover expired peer-token rotation and lock contention +46/-0

Cover expired peer-token rotation and lock contention

• Tests that forced refresh uses a differing expired stored token and persists its replacement. Also verifies that neither rotation nor raw recovery returns that expired token when the profile lock is unavailable.

test/Capacitor.Cli.Core.Tests.Unit/Auth/TokenServerBindingTests.cs

UnauthorizedRecoveryHandlerTests.csVerify the 401 resend uses a freshly rotated token +21/-2

Verify the 401 resend uses a freshly rotated token

• Reproduces a 401 after the stored token has changed and expired, then verifies the resend carries a token refreshed from the stored credential. The test client accepts an injectable refresh endpoint.

test/Capacitor.Cli.Core.Tests.Unit/Auth/UnauthorizedRecoveryHandlerTests.cs

CodexLauncherTests.csAssert Codex's result-channel timeout override +5/-1

Assert Codex's result-channel timeout override

• Checks the generated Codex arguments include the 300-second timeout and updates the exact expected launch arguments.

test/Capacitor.Cli.Daemon.Tests.Unit/Harness/Codex/CodexLauncherTests.cs

McpFlowResultServerTests.csCover restart retries and terminal delivery responses +159/-0

Cover restart retries and terminal delivery responses

• Adds submit and send tests for gateway failures, connection errors, retry exhaustion, and stable message IDs. Tests also confirm coded 503 responses and stale round tokens remain terminal, and that the delivery deadline is below the tool timeout.

test/Capacitor.Cli.Tests.Unit/Commands/McpFlowResultServerTests.cs

Other (2) +7 / -0

KcapMcpRegistry.csDefine the result-channel tool timeout +5/-0

Define the result-channel tool timeout

• Adds a shared 300-second timeout value for harnesses that support per-server tool timeouts, leaving room for the channel's 240-second delivery deadline.

src/Capacitor.Cli.Core/KcapMcpRegistry.cs

CodexLauncher.csExtend Codex's flow-result tool timeout +2/-0

Extend Codex's flow-result tool timeout

• Adds a timeout override only for the reserved flow-result MCP server, so Codex does not abandon a delivery still riding out a restart.

src/Capacitor.Cli.Daemon/Harness/Codex/CodexLauncher.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Another server’s credential gets rotated ✓ Resolved

Description

ForceRefreshForProfileAsync now refreshes a different expired stored token before checking whether
it is bound to the server that rejected the request. If a peer has written an expired token for
another server, recovery rotates that credential and only then discards it as wrong-server.

Code

src/Capacitor.Cli.Core/Auth/TokenStore.cs[R532-533]

+            needsRefresh: t => string.Equals(t.AccessToken, rejectedAccessToken, StringComparison.Ordinal)
+                               || t.IsExpiredAt(time.GetUtcNow()),

Relevance

●●● Strong

Accepted binding precedents prioritize validating the exact token returned; rotating before server
validation can mutate unrelated credentials.

PR-#387
PR-#257

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The lock helper refreshes the latest stored token when the new expiry predicate matches; the
expected-server binding check occurs only after that refresh returns.

src/Capacitor.Cli.Core/Auth/TokenStore.cs[521-537]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[678-705]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[382-390]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Forced recovery can refresh a different expired token bound to another server, spending its credential before rejecting the result.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[516-537]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[678-705]
## Recommended Fix
Check the re-read token’s binding to the expected server under the profile lock before invoking the refresh delegate. Return without refreshing when the binding differs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Reviewers misread committed results ✗ Dismissed

Description

PostRetryingAsync resends a reviewer result after a connection failure or timeout, but kcap-server
rejects a duplicate of an already-recorded result as stale_round_token. If the first POST commits
and its response is lost, the retry tells the reviewer to discard a result the server actually
recorded.

Code

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[R307-310]

+            } catch (HttpRequestException ex) {
+                failure = ex.Message;
+            } catch (OperationCanceledException) when (!ct.IsCancellationRequested) {
+                failure = "the request timed out";

Relevance

●●● Strong

Accepted retry precedents treat uncertain delivery and idempotency as correctness concerns; stale
responses can mislead after committed writes.

PR-#262
PR-#609

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds retries for transport failures and treats a subsequent stale-token response as an error.
The server validates a submission before appending it and explicitly returns stale_round_token when
the matching round already has a recorded result, unlike its idempotent participant-message
endpoint.

kcap-cli -> kcap-server
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[254-275]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[301-310]
External repo: kurrent-io/kcap-server, src/Capacitor.Server/Flows/FlowOrchestratorService.cs [5548-5560]
External repo: kurrent-io/kcap-server, src/Capacitor.Server/Flows/FlowOrchestratorService.cs [5640-5655]
External repo: kurrent-io/kcap-server, src/Capacitor.Server/Flows/FlowOrchestratorService.cs [5694-5703]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The CLI retries reviewer-result POSTs after ambiguous transport failures, but kcap-server rejects an already-committed duplicate as stale. A lost response therefore makes a delivered result appear rejected.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[254-275]
- /cross_repos/kcap-server/src/Capacitor.Server/Flows/FlowOrchestratorService.cs[5639-5655]

## Recommended Fix
Coordinate with kcap-server to recognize an identical, previously committed reviewer result and return success without appending it again. Keep genuinely stale or different submissions terminal, and add a cross-repo test for a commit followed by a lost response.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. A peer login can use the wrong refresh path ✓ Resolved

Description

ForceRefreshForProfileAsync selects a provider-specific refresh delegate from its pre-lock
snapshot, while the new expiry condition permits refreshing a different token loaded under the lock.
If a peer replaces the token with an expired credential from another provider, the selected delegate
receives that credential and can send it to the wrong provider’s refresh endpoint.

Code

src/Capacitor.Cli.Core/Auth/TokenStore.cs[R532-533]

+            needsRefresh: t => string.Equals(t.AccessToken, rejectedAccessToken, StringComparison.Ordinal)
+                               || t.IsExpiredAt(time.GetUtcNow()),

Relevance

●●● Strong

Accepted refresh precedents require stable profile/provider context across locking and refresh,
matching this cross-provider delegate hazard.

PR-#257
PR-#387

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Provider selection precedes the lock; the lock helper then passes the re-read token to that
delegate. GitHub and WorkOS use materially different refresh paths.

src/Capacitor.Cli.Core/Auth/TokenStore.cs[516-533]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[678-700]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[725-761]
src/Capacitor.Cli.Core/Auth/TokenStore.cs[796-817]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A peer can replace the pre-lock token with an expired token from another provider, but forced recovery retains the original provider’s refresh delegate.
## Fix Focus Areas
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[516-533]
- src/Capacitor.Cli.Core/Auth/TokenStore.cs[678-700]
## Recommended Fix
Choose and validate the provider-specific refresh operation using the token re-read under the profile lock, including that provider’s required refresh credentials.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Launch-race retries can overrun the deadline ✓ Resolved

Description

In PostRetryingAsync, the isRace branch retries no_active_flow, no_open_round, and
concurrent_update responses after RetryDelay without applying the DeliveryDeadline check used
for restart-noise retries. If 503 responses consume most of the 240-second window before a restarted
server returns a race code, further retries can each wait 3 seconds and take up to the 20-second
AttemptTimeout, carrying the call past Codex’s 300-second tool timeout.

Code

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[R322-325]

+                } else if (isRace(code) && ++races < MaxAttempts) {
+                    response.Dispose();
                    await delay(RetryDelay);
                    continue;

Relevance

●●● Strong

Recent accepted deadline precedent requires checking remaining budget before each operation,
directly matching the unbounded race branch.

PR-#1269
PR-#1097

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Lines 335–337 compare elapsed time plus the next wait and AttemptTimeout with DeliveryDeadline,
but only on the restart-noise path. The race branch at lines 322–325 checks only `++races <
MaxAttempts` before delaying and continuing, so race responses following a long run of unavailable
responses bypass the 240-second limit; neither the production delay nor Codex’s 300-second tool
timeout enforces that limit within the branch.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[313-340]
src/Capacitor.Cli.Core/KcapMcpRegistry.cs[73-76]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[38-49]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[301-339]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[254-258]
src/Capacitor.Cli.Daemon/Harness/Codex/CodexLauncher.cs[345-349]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`PostRetryingAsync` applies `DeliveryDeadline` to restart-noise retries but not coded-race retries. Interleaved unavailable and race responses can therefore keep the call running past Codex’s tool timeout.

## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[297-340]

## Recommended Fix
Enforce the overall delivery deadline before every retry wait and attempt, including in the `isRace(code)` branch. Before a race retry, require `time.GetElapsedTime(started) + RetryDelay + AttemptTimeout <= DeliveryDeadline`; if it fails, return `(response, code, message, null)` so the caller can show the coded error and retry hint. Bound attempts by the remaining time, and add a fake-time-provider test that spends most of the window on 503 responses, then returns `no_active_flow` and verifies that no attempt starts after the deadline.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Flow drivers can receive duplicate notes ✓ Resolved

Description

SendMessageCoreAsync assigns a new message_id per tool invocation, but its new
restart-exhaustion response tells the caller to retry the tool call. If the server recorded a
message before its response was lost, that instructed second invocation uses a different ID, so
message-ID deduplication cannot prevent the driver receiving it twice.

Code

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[R380-381]

+        if (response is null)
+            return ($"Error: the Capacitor server is unreachable ({reply.Failure}). Retry this tool call.", true);

Relevance

●●● Strong

Accepted replay and idempotency precedents recognize that a new invocation identifier defeats server
deduplication after lost acknowledgements.

PR-#262
PR-#609

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The message ID is generated once per invocation and reused only by attempts inside
PostRetryingAsync; the added exhaustion response requests another invocation.

src/Capacitor.Cli/Commands/McpFlowResultServer.cs[347-375]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[378-381]
src/Capacitor.Cli/Commands/McpFlowResultServer.cs[333-340]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new instruction to retry an exhausted message call can duplicate a message whose first response was lost, because the next invocation generates a new ID.
## Fix Focus Areas
- src/Capacitor.Cli/Commands/McpFlowResultServer.cs[351-381]
## Recommended Fix
Provide a way to reuse the original message ID across separate tool invocations and include it in the retry guidance, or otherwise establish whether the original message was recorded before instructing a new send.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

  Explored: repo: kurrent-io/kcap-deployments (sha: 25961df7) — View relationship

  Explored: repo: kurrent-io/kcap-server (branch: alexeyzimarev/ai-3436-review-flow-loses-a-reviewer-result-acknowledged-just-after, sha: fb9f78a3) — View relationship

Review mode: 🧠 Deep: This is a bug-dense behavioral change spanning authentication/token rotation, retry and timeout semantics across two delivery paths, CLI harness configuration, and numerous independent code/test sites where subtle defects could be missed in one pass.

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f2da5c3da

ℹ️ 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]

Enforce the delivery deadline for race retries

When restart failures have already consumed most of DeliveryDeadline and the recovered server then returns no_active_flow, no_open_round, or concurrent_update, this branch performs up to four additional delays and 20-second attempts without checking the deadline. For example, several slow 503 responses followed by five slow race responses can exceed both the advertised 240-second bound and Codex's 300-second tool timeout, causing the only result-delivery call to be abandoned. Apply the same elapsed-time check before race retries, or enforce one deadline across every retry path.

Useful? React with 👍 / 👎.

[alexeyzimarev]

Fixed in 6097f9b: every retry, race-coded or restart noise, now passes one deadline check (elapsed + next delay + attempt timeout ≤ DeliveryDeadline). A race retry that would not fit returns the coded error with the retry hint. A fake-time test mixes slow 503s with no_open_round and asserts the call ends inside the deadline.