From 42c63a3534022452a6988f894cd463ea6820fce2 Mon Sep 17 00:00:00 2001 From: theBlanketBaron Date: Fri, 6 Nov 2026 20:12:33 -0500 Subject: [PATCH] fix(codex): the bridge doesn't re-register a dead thread (MUS-108) A registered thread whose app-server went away never gets thread/closed, so the MUS-76 loop registered it again every 5 s as a ghost row. Only threads the bridge can wake (app-server up, listed by the reconnect) are registered again; a daemon restart leaves the app-server up, so MUS-76's case still recovers. Found in Marshal's port (MAR-7) by wise-leopard. Co-Authored-By: Claude Opus 5.5 --- crates/shim/src/codex_bridge.rs | 71 ++++++++++++++++++++++++++++++--- docs/roadmap.md | 7 ++++ 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/crates/shim/src/codex_bridge.rs b/crates/shim/src/codex_bridge.rs index 572ae95e..8f8fa2d6 100644 --- a/crates/shim/src/codex_bridge.rs +++ b/crates/shim/src/codex_bridge.rs @@ -1260,9 +1260,19 @@ pub async fn run(args: &[String]) -> Result<()> { // restart without Postgres, is registered again, so it can be woken // before its next hook (MUS-76). Every bridge does this for its own // threads, leader or not. + // Only threads the bridge could wake: with the app-server down, or a + // thread the reconnect's `thread/list` didn't show, the thread may be + // dead, and registering it again would make a ghost row (MUS-108). + let wakeable = registered.wakeable_sessions(); let missing = { let registered = registered.lock().unwrap(); - missing_registrations(®istered, &sessions.get(), &reregistered_at, now) + missing_registrations( + ®istered, + &sessions.get(), + &wakeable, + &reregistered_at, + now, + ) }; for registration in missing { reregistered_at.insert(registration.session_id.clone(), now); @@ -1731,11 +1741,13 @@ impl Registered { } } -/// Registered threads whose session isn't on the roster, and that weren't -/// registered again within the last `REREGISTER_COOLDOWN`. +/// Registered threads whose session isn't on the roster, that the bridge can +/// wake (`wakeable`: the app-server is up and listed them, MUS-108), and that +/// weren't registered again within the last `REREGISTER_COOLDOWN`. fn missing_registrations( registered: &HashMap, sessions: &[Arc], + wakeable: &HashSet, last_attempt: &HashMap, now: Instant, ) -> Vec { @@ -1743,6 +1755,7 @@ fn missing_registrations( registered .values() .filter(|r| !on_roster.contains(r.session_id.as_str())) + .filter(|r| wakeable.contains(&r.session_id)) .filter(|r| { last_attempt .get(&r.session_id) @@ -2957,20 +2970,66 @@ mod tests { ]; let now = Instant::now(); let mut attempts = HashMap::new(); + let all: HashSet = ["kept", "lost"].map(String::from).into(); - let missing = missing_registrations(®istered, &roster, &attempts, now); + let missing = missing_registrations(®istered, &roster, &all, &attempts, now); assert_eq!(missing, vec![registration("lost")]); attempts.insert("lost".to_string(), now); let soon = now + REREGISTER_COOLDOWN / 2; - assert!(missing_registrations(®istered, &roster, &attempts, soon).is_empty()); + assert!(missing_registrations(®istered, &roster, &all, &attempts, soon).is_empty()); let later = now + REREGISTER_COOLDOWN; assert_eq!( - missing_registrations(®istered, &roster, &attempts, later), + missing_registrations(®istered, &roster, &all, &attempts, later), vec![registration("lost")] ); } + /// MUS-108 (found in Marshal's port, MAR-7): a thread the bridge can't wake, + /// because the app-server went away or its reconnect didn't list the thread, + /// may be dead, so it isn't registered again as a ghost row. + #[test] + fn a_thread_the_bridge_cant_wake_isnt_registered_again() { + let registered: HashMap = ["live", "dead"] + .into_iter() + .map(|id| (id.to_string(), registration(id))) + .collect(); + let only_live: HashSet = ["live".to_string()].into(); + let missing = missing_registrations( + ®istered, + &[], + &only_live, + &HashMap::new(), + Instant::now(), + ); + assert_eq!(missing, vec![registration("live")]); + assert!( + missing_registrations( + ®istered, + &[], + &HashSet::new(), + &HashMap::new(), + Instant::now() + ) + .is_empty(), + "app-server down: nothing" + ); + } + + #[test] + fn wakeable_needs_the_app_server_up_and_the_thread_listed() { + let threads = Registered::default(); + threads.insert(registration("live")); + threads.insert(registration("dead")); + assert!( + threads.wakeable_sessions().is_empty(), + "the app-server is down" + ); + threads.set_app_server_up(true); + threads.mark_listed(&["thread-live".to_string()].into()); + assert_eq!(threads.wakeable_sessions(), ["live".to_string()].into()); + } + fn session(id: &str, host: &str, hook_owned: bool) -> Arc { Arc::new(Session { id: SessionId(Arc::from(id)), diff --git a/docs/roadmap.md b/docs/roadmap.md index 89b069ee..9b4cd747 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -571,6 +571,13 @@ Merged and needing a live run: MUS-36 and MUS-37. `to_room`, `message`/`text` for `body`, `recipient` for `to`, `current_task`/`status` for `text`, `room` for `join_room`'s `name`, and `ids` or one `message_id` for `message_ids`. The canonical name wins when both are given. The advertised schemas are unchanged. +- [x] **MUS-108: The Codex bridge doesn't re-register a dead thread.** Found by wise-leopard in + Marshal's port of MUS-76 (MAR-7, Castor-o7/marshal#5). The bridge keeps the threads it registered + until `thread/closed`, and re-registers any whose session left the roster (MUS-76). A thread + whose app-server went away never closes, so it was registered again every 5 s for the bridge's + life: a ghost row, like MUS-104. Done: only wakeable threads are registered again, meaning the + app-server is up and the reconnect's `thread/list` showed them (MUS-77's `wakeable_sessions`). + A daemon restart, MUS-76's case, leaves the app-server up, so those threads still come back. - [ ] **MUS-30: Decide the future of the TypeScript plugins.** The operator decides whether to keep, freeze or remove them. Done when: the decision is recorded here, and if they're removed, the repository's `package.json` workspace goes with them.