[SYMPP-5702] Rotate backend ownership without restarting followers - #624
Conversation
Summary: - re-enter the existing cold election when a managed backend disappears - rebind surviving STDIO adapters to the replacement backend epoch - return an indeterminate error instead of replaying ambiguous tool calls - extend the canonical launcher lifecycle matrix with recovery coverage Rationale: - long-lived adapters previously froze one backend URL and HTTP session - shared cold locks and bridge leases already own singleton startup and exit - conservative replay keeps mutating tool outcomes safe after transport loss Tests: - pwsh -NoProfile -ExecutionPolicy Bypass -File plugins/symphony-plus-plus-mcp/tests/launcher/run-launcher-tests.ps1 - make -C elixir all Co-authored-by: Codex <codex@openai.com>
Summary: - Reject truncated backend responses as requests that may have reached the old backend. - Detach and fail closed when replacement cleanup-source validation fails. - Extend the canonical recovery matrix for both lifecycle failures. Rationale: - Ambiguous mutating calls must return one indeterminate result instead of hanging or replaying. - A rejected replacement must retain a trusted path to zero-process cleanup. Tests: - focused six-mode rotating-owner recovery matrix - state-identity-tests.js - bridge-response-forwarding-tests.js - node-bridge-burst.js (200 clients) Co-authored-by: Codex <codex@openai.com>
Summary: - re-enter the existing cold-start election from surviving fallback bridges - rebind sessions and leases while keeping follower STDIO processes alive - cover concurrent and delayed followers on PowerShell 5.1 Rationale: - the Node bridge already recovered after owner and backend loss, but the shipped PowerShell fallback stayed bound to the dead epoch - only provably unsent requests retry; ambiguous tool calls fail indeterminate Tests: - focused launcher state, response, burst, leader-death, and recovery matrix - pwsh -File plugins/symphony-plus-plus-mcp/tests/launcher/run-launcher-tests.ps1 - make -C elixir all Co-authored-by: Codex <codex@openai.com>
Cancel a blocked PowerShell fallback preparation when STDIO closes and clean up an unleased replacement if post-start validation fails. Extend the canonical owner/backend-loss case through final-close cancellation and zero-resource cleanup. Co-authored-by: Codex <codex@openai.com>
Track ordered PowerShell STDIO read-ahead while a fallback election runs so EOF behind a buffered request still cancels final shutdown. Cover the pipelined close in the canonical fallback recovery case. Co-authored-by: Codex <codex@openai.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4862881edc
ℹ️ 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".
Summary: - Reuse a provably unsent caller initialize after fallback recovery. - Add deterministic fallback coverage and lower launcher-test priority. Rationale: - The caller initialize can safely establish the replacement session when the preflight proves that its POST never reached the failed backend. - BelowNormal test priority prevents the launcher matrix from monopolizing the Windows workstation while preserving live runtime priority. Tests: - Focused fallback initialize retry under PowerShell 5.1 and PowerShell 7. - Focused fallback owner recovery and ambiguous mutation coverage. - make -C elixir all (1694/1696 passed; unrelated dashboard migration assertion remains red and one timeout cleared on focused rerun). Co-authored-by: Codex <codex@openai.com>
Summary: - Count the new initialize-retry recovery case in the launcher matrix. - Leave its mode-specific result shape to the dedicated assertion. Rationale: - The canonical wrapper otherwise rejects a successful eight-case smoke run before it can evaluate the initialize-retry contract. Tests: - PowerShell parser and focused aggregate-predicate check. Co-authored-by: Codex <codex@openai.com>
Summary: - Exclude the initialize-only fallback case from the tools-list aggregate. Rationale: - That focused mode proves retransmission and lifecycle cleanup without issuing tools/list, so its dedicated assertion owns the contract. Tests: - PowerShell parser and focused tools-list aggregate check. Co-authored-by: Codex <codex@openai.com>
Summary: - Trigger Node recovery only after transport loss, not live HTTP errors. - Replay recovered requests only when the failed send is provably unsent. - Add focused predicate assertions to the response-forwarding suite. Rationale: - Live application errors must remain responses from the current backend. - Ambiguous delivery cannot be retried safely across a replacement epoch. Tests: - Node bridge response forwarding and recovery-policy assertions. - Focused PowerShell initialize retry and Node ambiguous-tool recovery. Co-authored-by: Codex <codex@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
89f33d4
into
integration/mcp-first-caller-lifecycle
Context
PowerShell fallback followers lost their MCP session when the native owner Job and backend exited,
even though follower STDIO stayed open.
TL;DR
Keep follower STDIO alive while one fallback bridge elects and binds a replacement backend.
Summary
Alternatives
Test Plan
make -C elixir allmake -C elixir ci-fullis not required; GitHub CI is the remote full gate.pwshran once at BelowNormal; the unrelated unmanaged200-client burst had one missing initialize result, so the heavyweight local run was not repeated.
git diff --checkpassed.rvw_2b37f7e3.