Skip to content

Restart on Manual Build #758

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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

Chaitanya-Keyal
Copy link

@Chaitanya-Keyal Chaitanya-Keyal commented May 22, 2025

Description

Currently, for manual builds, the "Restart" option in the UI behaves as follows:

  • The Python code only kills the running SeedSigner process.
  • The process only restarts if the manual build is being run via systemd and the service is configured with Restart=always. However, for local development, Restart=no is preferred to retain manual control over the process state.

What this PR changes:

  • Enhances the restart logic:
    The Python code now kills the running process and also directly starts the SeedSigner main script. replaces the running process with a new process, at the same process id.

  • Updates the documentation to always recommend Restart=no.
    This prevents the risk of unintentionally running multiple instances.

Why this is better:

  • Improved developer experience:
    Local developers can now click "Restart" in the UI without needing to manually kill and relaunch the process.
  • Safer default:
    Prevents accidental duplication of running processes due to systemd auto-restarting.

This pull request is categorized as a:

  • Other

Checklist

  • I’ve run pytest and made sure all unit tests pass before sumbitting the PR

If you modified or added functionality/workflow, did you add new unit tests?

  • N/A

I have tested this PR on the following platforms/os:

@jdlcdl
Copy link

jdlcdl commented May 22, 2025

ACK tested on pi0 seedsigner-os

Still to test on manual install and will review docs for this dev process (since it's been a while that I've done so).

Note: I had a question about the single & (in the background instead of && as in AND) but I confirm that putting kill in the background works (and not doing that kills itself before restarting).

@alvroble
Copy link
Contributor

ACK tested on Raspberry Pi OS manual build. Verified both with the systemctl service and manual execution from the command line.

# `.*` is a wildcard to detect either `python`` or `python3`.
kill_cmd = "kill $(pidof python*)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is slightly too aggressive / non-specific.

I can't really think of a reason why someone might be simultaneously running other python processes (other than possibly someone running via the emulator?), but I think we should aim to "play nice" and be sure that we're only killing our own process.

Though to be fair, the prior kill wasn't as specific as it could have been.

Copy link
Contributor

Choose a reason for hiding this comment

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

else:
call("kill $(ps aux | grep '[p]ython.*main.py' | awk '{print $2}')", shell=True)
python_exec = sys.executable # Current Python interpreter path
script_path = os.path.abspath(sys.argv[0]) # Absolute path to current script
Copy link
Contributor

Choose a reason for hiding this comment

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

See: https://docs.python.org/3/library/sys.html#sys.argv

"it is operating system dependent whether this is a full pathname or not"

So I think os.path.abspath() will fail(?) if sys.argv[0] just returns "main.py".

Copy link
Author

Choose a reason for hiding this comment

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

You're right that sys.argv[0] is not guaranteed to be a full path. It is simply the first argument passed to the Python interpreter. So in a typical case like:

python3 main.py

sys.argv[0] would just be "main.py", which is a relative path.

But this is not a problem for os.path.abspath(). It does not require sys.argv[0] to be absolute. Instead, it combines the current working directory with the relative path to produce an absolute one. So if the working directory is .../seedsigner/src, then:

os.path.abspath(sys.argv[0])

will correctly resolve to .../seedsigner/src/main.py.

@Chaitanya-Keyal
Copy link
Author

Chaitanya-Keyal commented May 23, 2025

Thanks for the feedback @kdmukai. I originally tried to mimic the logic written for seedsigner-os by killing the process and restarting it, but after looking deeper into the logs, I realized that:

kill $(pidof python*) actually does not even work on manual builds (I can't confirm for seedsigner-os, but it's likely that it doesn't work there either). The following output is shown in the logs:

/bin/sh: 1: kill: Usage: kill [-s sigspec | -signum | -sigspec] [pid | job]... or

My testing, and I suppose others' as well, missed this because on the device it seemed like the "restart" worked. However, all that really happened was that the kill command failed silently, and a new instance of SeedSigner was launched.

I've now changed the approach entirely. Instead of killing and restarting the process, I'm using:

os.execv(sys.executable, [sys.executable] + sys.argv)

This replaces the current Python process with a fresh one using the same interpreter and arguments. It avoids brittle process-killing logic, works when run via systemd or terminal, and is overall a cleaner and safer solution.

Essentially, this does the same thing as earlier, except instead of killing a process, it simply replaces it with a new one based on the arguments passed to os.execv(). By passing [sys.executable] + sys.argv, it recreates the original run command, thereby restarting the process.

Official docs: https://docs.python.org/3/library/os.html#os.execv

Let me know if any concerns remain with this new approach.

Also, @jdlcdl and @alvroble, thanks again for testing the previous version. I'd really appreciate it if you could test this one too, when you get the time.

@Chaitanya-Keyal Chaitanya-Keyal force-pushed the fix-manual-build-restart branch from 4aec431 to 643b367 Compare May 23, 2025 21:33
@alvroble
Copy link
Contributor

Thanks @Chaitanya-Keyal. Tested again in Raspberry Pi OS manual build and LGTM. ACK as of 643b367

@newtonick
Copy link
Collaborator

ACK, I want to test this one before merging.

@newtonick newtonick added the enhancement New feature or request label May 25, 2025
@Chaitanya-Keyal
Copy link
Author

Just realised the screenshot generator is going to fail after this new approach. Will make necessary changes and update PR.

@Chaitanya-Keyal
Copy link
Author

Updated PR. CI should pass now.

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

Successfully merging this pull request may close these issues.

5 participants