Repository navigation
feat(id): coordinate concurrent world sessions (#3845) - #3896
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 53 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1549d0903
ℹ️ 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".
| if !self.roots.contains_key(&key.0) { | ||
| let root = WorldClient::connect(&self.endpoint, addr).await?; | ||
| self.roots.insert(key.0.clone(), root); |
There was a problem hiding this comment.
After a server disconnects or restarts, its closed connection remains in roots. Even if the caller removes the failed session with close_session and calls open_session again, this membership check skips reconnecting, so WorldClient::open_session keeps attempting open_bi on the permanently closed connection. The only available recovery is close_all, which also disconnects healthy sessions on unrelated servers. Detect and replace closed roots, invalidating their associated sessions, so one endpoint can recover independently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec840e9 (pushed to this branch): WorldSessions::open_session now evicts a closed root — detected via a new WorldClient::is_closed (conn.close_reason().is_some(), the idiom already used in repl.rs) — together with every session keyed to that endpoint, before the membership checks, so the endpoint redials independently instead of wedging until close_all. Regression test coordinator_replaces_a_closed_root_connection_on_reopen covers exactly your scenario (root dies → close_session → open_session succeeds on a fresh root).
Verification caveat, stated plainly: the targeted tests cannot run on the current branch base yet — the base does not compile pkgs/id (references.rs E0277/E0597, the breakage PR #4477 restores; an E0599 on addr.id() also fires on this PR's pre-existing Self::key at the same base). Task 5831 is parked to rebase onto post-#4477 main and run cargo test -p id --lib world_net::tests::coordinator before merge; the PR will not merge unverified.
f1549d0 to
da895b5
Compare
) Codex P2 on PR #3896: a root connection that closed (server restart) stayed in WorldSessions.roots forever; after close_session, open_session skipped redialing on the membership check and kept attempting open_bi on the dead connection, so one endpoint could only recover via close_all, which also drops healthy sessions on other servers. WorldSessions::open_session now evicts a closed root (detected via WorldClient::is_closed, the close_reason idiom from repl.rs) together with every session keyed to its endpoint before dialing, so the endpoint redials independently. Regression test coordinator_replaces_a_closed_root_connection_on_reopen covers close_session + reopen after the root dies. Verification status: targeted tests cannot run yet — the branch base does not compile pkgs/id (references.rs E0277/E0597, the breakage PR #4477 restores; world_net.rs E0599 addr.id() also fires on the PRs pre-existing Self::key at the same base). Rebase onto post-#4477 main and rerun cargo test -p id --lib world_net::tests::coordinator before merge (task 5831 park note).
…rrent-world-sessions
Summary
Refs #3845
Draft: implementation and targeted verification are still in progress.
Need help on this PR? Tag
@codesmith-botwith what you need.