Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to rename session #57

Open
mrzv opened this issue Jun 18, 2024 · 9 comments
Open

Add ability to rename session #57

mrzv opened this issue Jun 18, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@mrzv
Copy link

mrzv commented Jun 18, 2024

I ran into a need to rename a (long-running) session. It would be great if there was a shpool rename old new command.

@ethanpailes ethanpailes added enhancement New feature or request good first issue Good for newcomers and removed good first issue Good for newcomers labels Jun 24, 2024
@ethanpailes
Copy link
Contributor

I think there are a few different issues we need to address to do this well

What do we do if the target session name already exists

I think by default, we should exit with an error, but let people override the behavior with -f/--force to just clobber the old session or -s/--swap to make the two sessions swap names, retaining both.

What do we do about SHPOOL_SESSION_NAME

The SHPOOL_SESSION_NAME env var is used by the shpool detach and shpool kill subcommands, and is shown in the prompt (though we currently fully replace $SHPOOL_SESSION_NAME with the session name in the prompt prefix. To deal with this correctly, we probably need to change the way the prompt code works so it really is dynamically looking up the session name each time, and also somehow update the session name env var. There is no good way to do this that I'm aware of.

We could inject a export SHPOOL_SESSION_NAME=<new session name> command into the target session, but this will only work if the session is parked at a blank prompt. We can't be sure of this and we don't have any 100% effective way to check. We could also just tell the user they need to update the session name env var themselves, but that feels clunky.

One option that has been in the back of my mind for a while, is making an LD_PRELOAD shim that we can inject into child shells to let us change environment variables. I believe that as long as we call setenv from inside the same address, the environment variable will get updated. As long as we can somehow inject some code that runs on startup into a child process, we can have the code open up a unix domain socket, then listen for connections and pull setenv calls off the incoming connections. We can have a magic env var that holds a PID and this shim will only accept connections from that exact PID as a security measure to prevent other programs from being able to inject env vars into the target shell. This would potentially allow us to avoid the symlink trick we use for SSH_AUTH_SOCK as well. This will be pretty involved and probably deserves its own crate the same way that motd got its own crate. I doubt it is possible to accomplish this portably, so features that depend on it might be Linux specific once we get the Mac port working.

@mrzv
Copy link
Author

mrzv commented Jun 25, 2024

These are complicated issues I hadn't thought of. I don't fully understand your last paragraph, but what if the mechanism to get the session name was changed? Something like running shpool name would return the name of the current session. Then instead of the SHPOOL_SESSION_NAME environment variable, one could get the name that way, and use it for whatever one wants (e.g., inside the prompt). Would something like that be simpler?

@ethanpailes
Copy link
Contributor

In terms of my last paragraph, I just wrote a bit more about what I mean in the chook placeholder crate that I just made. For now, I don't have any code, but I think this should be possible through a combination of defining a custom _init and setting LD_PRELOAD.

I think shpool name is a pretty good idea. We could implement shpool name by adding a new RPC to the daemon where the caller presents a PID and gets the session name associated with that PID back.

There are two issues I see with the shpool name approach though:

  1. It would break backwards compatibility, which I'm a bit worried about since I think having some custom prompt code based on $SHPOOL_SESSION_NAME is probably one of the most common customizations.
  2. Assuming we change the prompt prefix to be shpool:$(shpool name), which I think we would need to if we want the prompt hooks to pick up name changes, we would be grabbing a lock on the global session table every time a new prompt is drawn. This would mean that two different side-by-side shells are contending with one another if they are both drawing new prompts. Even without the locking considerations (which we might be able to deal with via a lock free data structure of some sort), there would still be an RPC every time a new prompt is drawn.

Potentially we could do something like cache the name in a file, but it's not clear to me that that would be much better. It would still be IO on the same host.

I think I'm going to play with chook to see the degree of gross systems shenanigans that it requires to get done and then decide on the way to go based on that.

@ethanpailes
Copy link
Contributor

I took a swing at chook, and I don't know that it can be reasonably implemented as a rust crate. The problem is that rust doesn't really want any code running before main, so you can't really use the stdlib. This is a pain for both the little RPC code I wrote and also for the user provided function. This was my first swing at it, but I never really got it working. Also, I didn't get to the point of trying to use it before deciding that this is probably untenable, but rust-ctor looks like a useful crate for this if I ever decide to pick it back up.

I'm now thinking that making a more focused inject-env crate that uses a hardcoded C file that can only tweak environment variables is probably a better way to go. It will mean doing some manual encoding rather than getting to just lean on serde, but it hopefully won't be too bad. Figuring out how to build a c .so with a custom _init routine will probably be the most annoying part.

@ethanpailes
Copy link
Contributor

I just posted a working inject-env crate: https://github.com/shell-pool/subprocess-inject-env

Fortunately it was a lot easier to get working when the shim is written in C rather than rust (I could probably have managed with rust, but C seems a bit simpler since I don't need a seperate shim crate and it doesn't seem worth it if users can't write their own custom stub handler to be injected in the target).

Once I get it reviewed and the various CI toil done I'll cut a release and then I can integrate the crate with shpool proper and use it to take on this actual request.

Mac does not support LD_PRELOAD natively, but it looks like there might be an equivilent we can use, so hopefully it will be possible to port to mac as well since the goal is to get shpool working on mac at some point.

@ethanpailes
Copy link
Contributor

Seems like zsh and fish cache their environment variables, which is pretty annoying. We might not be able to use the inject-env crate

@mrzv
Copy link
Author

mrzv commented Jul 17, 2024

Oof, that's a pity. I'm a zsh user.

@ethanpailes
Copy link
Contributor

I think it just means we need to consider the id and subcommand based approach to session name resolution since that will definitely work regardless of shell.

@ethanpailes
Copy link
Contributor

As much as I like doing cursed systems things, it seems like env var injection is not gonna work because zsh and fish want to be efficient or something annoying like that, so we have to do the id based approach.

I think this is a general sketch of what we want to do

  • add a new SHPOOL_SESSION_ID env var that uses a numeric id peeled off an atomic counter stored in the daemon. This never changes, even after renames.
  • keep using SHPOOL_SESSION_NAME for backwards compatibility, but mark it deprecated
  • tell people to use $(shpool session-name) instead of using the env var directly. This command will ping the daemon to resolve SHPOOL_SESSION_ID to the name.

possibly (not sure if this will work, but it could help avoid a bunch of RPCs if it does):

  • add a new SHPOOL_SESSION_NAME_CACHE_MODE=on env var set by the prompt hook code
  • make the prompt hook code inject a trap routine on SIGWINCH or some other signal that pings the daemon to resolve id id into a name and updates SHPOOL_SESSION_NAME. This push bashed approach will allow shpool session-name to just resolve based on the env var if not called with a -f/--force flag or something.
  • add a shpool setup <bash|zsh|fish> subcommand which injects the prompt hook setup code so that people who set prompt_prefix = "" can join in on the fun, this should probably accept a prompt prefix option as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants