Skip to content

app-server: support daemon-safe restart handling#21831

Merged
euroelessar merged 1 commit into
mainfrom
ruslan/app-server-daemon-prereqs
May 8, 2026
Merged

app-server: support daemon-safe restart handling#21831
euroelessar merged 1 commit into
mainfrom
ruslan/app-server-daemon-prereqs

Conversation

@euroelessar
Copy link
Copy Markdown
Collaborator

Why

The app-server daemon work needs two app-server behaviors to be safe when lifecycle management is driven by a helper process:

  • a readiness probe must not become the process-wide client identity just because it connects first
  • a graceful reload signal needs to keep draining active turns even if it is delivered more than once

What changed

  • Treat codex_app_server_daemon initialization as a probe-only client for process-global originator and user-agent suffix state.
  • Distinguish forceable shutdown signals from graceful-only ones, and treat Unix SIGHUP as graceful-only while leaving SIGTERM and Ctrl-C forceable.
  • Add regression coverage for daemon probe initialization and repeated SIGHUP delivery while a turn is still running.

Testing

  • cargo test -p codex-app-server
    • The new daemon-probe and repeated-SIGHUP coverage passed.
    • The run still failed in the existing suite::conversation_summary::get_conversation_summary_by_relative_rollout_path_resolves_from_codex_home and suite::conversation_summary::get_conversation_summary_by_thread_id_reads_rollout tests because their initialize handshake timed out.
  • cargo test -p codex-app-server --test all suite::conversation_summary::
    • Reproduced the same two existing initialize-timeout failures in isolation.

@euroelessar euroelessar marked this pull request as ready for review May 8, 2026 22:34
Copy link
Copy Markdown
Collaborator

@owenlin0 owenlin0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of the things we'll have to figure out with the app-server singleton is to not make originator process-global 😅

Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8620c103e

ℹ️ 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".

}
let originator = name.clone();
let user_agent_suffix = format!("{name}; {version}");
let mutates_global_identity = name != DAEMON_PROBE_CLIENT_NAME;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not trust clientInfo to identify daemon probes

Any client controls clientInfo.name, so a websocket/unix client can initialize as codex_app_server_daemon and avoid setting the process originator/user-agent suffix. If no real client has initialized yet, later backend calls use the default codex_cli_rs identity instead of the actual client identity, defeating the new attribution behavior for non-probe clients.

Useful? React with 👍 / 👎.

@euroelessar euroelessar merged commit 1b86906 into main May 8, 2026
26 checks passed
@euroelessar euroelessar deleted the ruslan/app-server-daemon-prereqs branch May 8, 2026 22:47
@github-actions github-actions Bot locked and limited conversation to collaborators May 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants