Skip to content

Version aware daemon socket#10782

Merged
kevinyang372 merged 2 commits into
masterfrom
kevin/version-aware-daemon-socket
May 13, 2026
Merged

Version aware daemon socket#10782
kevinyang372 merged 2 commits into
masterfrom
kevin/version-aware-daemon-socket

Conversation

@kevinyang372
Copy link
Copy Markdown
Member

@kevinyang372 kevinyang372 commented May 13, 2026

Description

This creates a version aware daemon socket. There are two main components:

  1. Now socket connections are versioned. So given proxy version X, we will connect to server-X.pid and server-X.socket. This allows proxy to enforce it's connecting to a daemon with the SAME version
  2. After connection, proxy also cleans up any old socket and pid file that doesn't match with its version

Testing

I tested the versioned socket creation and clean up separately

Versioned socket:

  1. Deploy the server with dev channel and rename it to a versioned binary v0.2026.05.12.09.10.dev_00
  2. Run a local binary with GIT_RELEASE_TAG=v0.2026.05.12.09.10.dev_00 cargo run --bin dev
  3. Confirm the connection is successful and the right socket / pid file is created
Screenshot 2026-05-13 at 11 02 59 AM

Clean up:

  1. Rename the existing socket/pid file from the last step to server-old-X.pid and server-old-X.socket
  2. Run a local binary with GIT_RELEASE_TAG=v0.2026.05.12.09.10.dev_00 cargo run --bin dev
  3. Confirm the connection is successful, old socket/pid is removed, new socket/pid file is created

@cla-bot cla-bot Bot added the cla-signed label May 13, 2026
@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 13, 2026

@kevinyang372

I ran into an unexpected error while working on this.

Powered by Oz

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kevinyang372 kevinyang372 requested a review from alokedesai May 13, 2026 15:16
@kevinyang372
Copy link
Copy Markdown
Member Author

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 13, 2026

@kevinyang372

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

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

Overview

This PR versions the remote-server daemon socket and PID filenames by release version and adds proxy-side cleanup for non-current daemon artifacts.

Concerns

  • The cleanup path treats every non-current server*.pid as stale and sends SIGTERM, which can terminate an active daemon still serving another running Warp/proxy version.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread app/src/remote_server/unix/proxy.rs Outdated
// SAFETY: sending SIGTERM is safe — it requests graceful
// shutdown. If the process doesn't exist, kill returns -1
// with ESRCH which we ignore.
let ret = unsafe { libc::kill(pid, libc::SIGTERM) };
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.

⚠️ [IMPORTANT] Versioned sockets let different client versions run side-by-side, but this kills any daemon whose PID file does not match the current version. If an older Warp/proxy is still connected, starting a newer proxy can terminate its active daemon and break that session; only reap daemons proven inactive or use an age/self-shutdown based cleanup instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is good feedback. I think we could avoid eagerly SIGTERMing the connection and just remove the socket / pid file to be safe. This will prevent new connections to that daemon but not interrupt its active connections. The daemon will self-shutdown when its connection drops

Copy link
Copy Markdown
Member

@alokedesai alokedesai left a comment

Choose a reason for hiding this comment

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

noice

Comment thread crates/remote_server/src/setup.rs Outdated
/// - Local / Oss: `server.sock`
pub fn daemon_socket_name() -> String {
match ChannelState::channel() {
Channel::Local | Channel::Oss => "server.sock".to_string(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If the daemon has a version set for Local / Oss, I think we should use it. Otherwise you can't really test the versioned flow locally

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah agreed. Done

@kevinyang372 kevinyang372 merged commit cde06f9 into master May 13, 2026
40 of 44 checks passed
@kevinyang372 kevinyang372 deleted the kevin/version-aware-daemon-socket branch May 13, 2026 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants