Skip to content

Commit 1b36b3b

Browse files
schicklingclaude
andcommitted
fix(codex): stop-aware startup, session-boundary takeover, and machine-readable ask kinds
SIGTERM is honored through connect/initialize/binding (graceful, before the TUI exists no record is left behind), the restarted pump opens a fresh transition instead of claiming continuity across its own gap, and the human-blocking holds name their ask kind (review/permission/question). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent d23816c commit 1b36b3b

1 file changed

Lines changed: 66 additions & 21 deletions

File tree

src/codex_app_server.rs

Lines changed: 66 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ impl CodexObservedState {
218218
/// ones setting the blocked axis), while holds that only mean "st2 cannot currently prove
219219
/// anything" project to `None`, the indeterminate observation that writes nothing.
220220
pub fn harness_observation(&self) -> Option<harness_state::Observation> {
221-
use crate::harness_state::{Activity, BlockedOn, InputBuffer, Observation};
221+
use crate::harness_state::{Activity, Ask, BlockedOn, InputBuffer, Observation};
222222
let observation = |state, blocked_on| {
223223
// This producer reads the app-server control stream and cannot see the composer.
224224
Observation::new(state, blocked_on, InputBuffer::Unknown)
@@ -233,15 +233,19 @@ impl CodexObservedState {
233233
Some(observation(Activity::Active, BlockedOn::None))
234234
}
235235
CodexObservedState::Held { reason, .. } => match reason {
236-
CodexHoldReason::Review => {
237-
Some(observation(Activity::Active, BlockedOn::Human).with_reason("review"))
238-
}
236+
CodexHoldReason::Review => Some(
237+
observation(Activity::Active, BlockedOn::Human)
238+
.with_ask(Ask::Review)
239+
.with_reason("review"),
240+
),
239241
CodexHoldReason::WaitingOnApproval => Some(
240242
observation(Activity::Active, BlockedOn::Human)
243+
.with_ask(Ask::Permission)
241244
.with_reason("waitingOnApproval"),
242245
),
243246
CodexHoldReason::WaitingOnUserInput => Some(
244247
observation(Activity::Active, BlockedOn::Human)
248+
.with_ask(Ask::Question)
245249
.with_reason("waitingOnUserInput"),
246250
),
247251
CodexHoldReason::Compaction => {
@@ -407,12 +411,16 @@ impl CodexInboxDelivery {
407411
// The pty session whose liveness vouches for the record is the wrapper's task: the
408412
// runtime ID names the pty registry entry, and only aliases the identity on
409413
// driver-expanded seats — a hand-authored seat may declare a different task ID.
410-
let harness_writer = harness_state::Writer::new(
414+
let mut harness_writer = harness_state::Writer::new(
411415
&config.agent_dir,
412416
config.identity.clone(),
413417
"codex",
414418
Some(runtime.runtime_id().to_string()),
415419
);
420+
// A restarted pump is a new session: even a first observation that matches a fresh
421+
// predecessor record must open a new transition rather than claim continuity across an
422+
// interval nothing observed.
423+
harness_writer.interrupt();
416424
Ok(Self {
417425
config,
418426
state_path,
@@ -1304,9 +1312,20 @@ fn run_connected(
13041312
let expected_resume =
13051313
expected_resume_thread(&codex_argv[1..], resume_thread)?.map(str::to_owned);
13061314
diagnostics.record("waitingForControlSocket", json!({ "pid": server.id() }))?;
1307-
let control = connect_control(server, socket_path, STARTUP_TIMEOUT)?;
1315+
// A stop during startup ends the launch before anything was observed: no TUI exists, the
1316+
// caller reaps the app-server, and this session leaves no record — its predecessor's ages
1317+
// out on its own.
1318+
let Some(control) = connect_control(server, socket_path, STARTUP_TIMEOUT)? else {
1319+
diagnostics.record("stoppedDuringStartup", json!({ "phase": "connect" }))?;
1320+
return Ok(());
1321+
};
13081322
diagnostics.record("controlSocketConnected", json!({}))?;
13091323
let shutdown = control.try_clone()?;
1324+
if crate::provider_session::STOP.load(std::sync::atomic::Ordering::SeqCst) {
1325+
diagnostics.record("stoppedDuringStartup", json!({ "phase": "initialize" }))?;
1326+
let _ = shutdown.shutdown(Shutdown::Both);
1327+
return Ok(());
1328+
}
13101329
let websocket = initialize_control(control)?;
13111330
diagnostics.record("controlInitialized", json!({}))?;
13121331
let (events_tx, events_rx) = mpsc::channel();
@@ -1371,10 +1390,16 @@ fn run_connected(
13711390
.context("starting Codex control resume after the TUI launched")?;
13721391
}
13731392
diagnostics.record("waitingForThreadBinding", json!({ "pid": tui.id() }))?;
1374-
wait_for_binding(&mut tui, &events_rx, STARTUP_TIMEOUT, diagnostics).and_then(|_| {
1375-
diagnostics.record("threadBound", json!({ "pid": tui.id() }))?;
1376-
monitor_bound_tui(&mut tui, &events_rx)
1377-
})
1393+
match wait_for_binding(&mut tui, &events_rx, STARTUP_TIMEOUT, diagnostics)? {
1394+
BindingWait::Bound => {
1395+
diagnostics.record("threadBound", json!({ "pid": tui.id() }))?;
1396+
monitor_bound_tui(&mut tui, &events_rx)
1397+
}
1398+
BindingWait::Stopped => {
1399+
terminate_child(&mut tui);
1400+
Ok(TuiEnd::Stopped(tui.try_wait().ok().flatten()))
1401+
}
1402+
}
13781403
})();
13791404
if result.is_err() {
13801405
terminate_child(&mut tui);
@@ -1593,7 +1618,11 @@ fn preflight_hook_trust(
15931618
let result = diagnostics
15941619
.record("hookTrustPreflightStarted", json!({ "pid": server.id() }))
15951620
.and_then(|_| {
1596-
let control = connect_control(&mut server, socket_path, STARTUP_TIMEOUT)?;
1621+
let Some(control) = connect_control(&mut server, socket_path, STARTUP_TIMEOUT)? else {
1622+
// Stop requested mid-preflight: skip the projection — the launch proceeds to the
1623+
// connect stage, whose own stop check exits gracefully before the TUI starts.
1624+
return Ok(None);
1625+
};
15971626
let mut websocket = initialize_control(control)?;
15981627
query_hook_trust_projection(&mut websocket, cwd)
15991628
});
@@ -1887,11 +1916,16 @@ fn connect_control(
18871916
server: &mut Child,
18881917
socket_path: &Path,
18891918
timeout: Duration,
1890-
) -> Result<UnixStream> {
1919+
) -> Result<Option<UnixStream>> {
18911920
let deadline = Instant::now() + timeout;
18921921
loop {
1922+
// st2's stop path may fire before the control socket ever connects; without this check
1923+
// the wrapper would sit out the whole startup timeout with SIGTERM already delivered.
1924+
if crate::provider_session::STOP.load(std::sync::atomic::Ordering::SeqCst) {
1925+
return Ok(None);
1926+
}
18931927
match UnixStream::connect(socket_path) {
1894-
Ok(stream) => return Ok(stream),
1928+
Ok(stream) => return Ok(Some(stream)),
18951929
Err(error) if Instant::now() < deadline => {
18961930
if let Some(status) = server.try_wait()? {
18971931
anyhow::bail!("Codex app-server exited before control connected: {status}");
@@ -2320,14 +2354,23 @@ fn binding_candidate(message: &Value) -> Result<Option<&str>> {
23202354
}
23212355
}
23222356

2357+
/// How the binding wait ended: the thread bound, or st2's stop flag ended the session first.
2358+
enum BindingWait {
2359+
Bound,
2360+
Stopped,
2361+
}
2362+
23232363
fn wait_for_binding(
23242364
tui: &mut Child,
23252365
events: &Receiver<ControlEvent>,
23262366
timeout: Duration,
23272367
diagnostics: &mut WrapperDiagnostics,
2328-
) -> Result<()> {
2368+
) -> Result<BindingWait> {
23292369
let deadline = Instant::now() + timeout;
23302370
loop {
2371+
if crate::provider_session::STOP.load(std::sync::atomic::Ordering::SeqCst) {
2372+
return Ok(BindingWait::Stopped);
2373+
}
23312374
if let Some(status) = tui.try_wait()? {
23322375
anyhow::bail!("controlled Codex TUI exited before thread binding: {status}");
23332376
}
@@ -2345,7 +2388,7 @@ fn wait_for_binding(
23452388
diagnostics.record("tuiThreadLoaded", json!({ "pid": tui.id() }))?;
23462389
let _ = acknowledge.send(());
23472390
}
2348-
Ok(ControlEvent::Bound) => return Ok(()),
2391+
Ok(ControlEvent::Bound) => return Ok(BindingWait::Bound),
23492392
Ok(ControlEvent::Observed) => {}
23502393
Ok(ControlEvent::Closed) => {
23512394
anyhow::bail!("Codex control connection closed before thread binding")
@@ -2852,7 +2895,7 @@ mod tests {
28522895
/// emitting row is asserted positively.
28532896
#[test]
28542897
fn harness_projection_is_faithful_and_withholds_only_unprovable_rows() {
2855-
use crate::harness_state::{Activity, BlockedOn, InputBuffer};
2898+
use crate::harness_state::{Activity, Ask, BlockedOn, InputBuffer};
28562899
let held = |reason| CodexObservedState::Held {
28572900
reason,
28582901
turn_id: None,
@@ -2886,17 +2929,19 @@ mod tests {
28862929
assert_eq!(observation.input_buffer, InputBuffer::Unknown, "{state:?}");
28872930
}
28882931

2889-
// The holds a human resolves set the blocked axis instead of disappearing into active.
2890-
for reason in [
2891-
CodexHoldReason::Review,
2892-
CodexHoldReason::WaitingOnApproval,
2893-
CodexHoldReason::WaitingOnUserInput,
2932+
// The holds a human resolves set the blocked axis instead of disappearing into active,
2933+
// and each names its machine-readable ask kind so consumers never branch on `reason`.
2934+
for (reason, ask) in [
2935+
(CodexHoldReason::Review, Ask::Review),
2936+
(CodexHoldReason::WaitingOnApproval, Ask::Permission),
2937+
(CodexHoldReason::WaitingOnUserInput, Ask::Question),
28942938
] {
28952939
let observation = held(reason)
28962940
.harness_observation()
28972941
.unwrap_or_else(|| panic!("{reason:?} must emit"));
28982942
assert_eq!(observation.state, Activity::Active, "{reason:?}");
28992943
assert_eq!(observation.blocked_on, BlockedOn::Human, "{reason:?}");
2944+
assert_eq!(observation.ask, ask, "{reason:?}");
29002945
}
29012946

29022947
let idle = CodexObservedState::Idle.harness_observation().unwrap();

0 commit comments

Comments
 (0)