Skip to content

fix(extension): auto-start must not kill a running server (restart storm, #112) #113

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.

Sign up for GitHub

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
laulpogan wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −5
Open

fix(extension): auto-start must not kill a running server (restart storm, #112) #113

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions .pi/extensions/pi-web.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ export function openBrowser(pi: ExtensionAPI, url: string): Promise<void> {
async function healthCheck(host: string, port: string): Promise<boolean> {
try {
const res = await fetch(`http://${host}:${port}`, {
signal: AbortSignal.timeout(1000),
// 1s was tight enough that a busy machine reported a healthy server as
// down. A false negative here is expensive: it triggers service restarts.
signal: AbortSignal.timeout(4000),
});
// 401/403 means pi-web is running with auth enabled.
return res.ok || res.status === 401 || res.status === 403;
Expand All @@ -218,7 +220,17 @@ function windowsLauncher(): string {
return join(homedir(), ".config", "pi-web", "pi-web-start.vbs");
}

async function startPiWeb(pi: ExtensionAPI): Promise<void> {
// startPiWeb must only ever *start* the service. It used to run
// `launchctl kickstart -k`, which SIGTERMs a running instance (pi-web exits 0
// on SIGTERM). This function is reached from the opportunistic load-time check
// that every pi process runs, so one transient health-check failure killed a
// healthy server, which then made every other concurrent check fail and kickstart
// too -- a self-sustaining restart storm. Killing is left to /pi-web restart.
async function startPiWeb(
pi: ExtensionAPI,
host = "127.0.0.1",
port = "31415",
): Promise<void> {
if (process.platform === "win32") {
const launcher = windowsLauncher();
if (!existsSync(launcher)) {
Expand All @@ -233,7 +245,11 @@ async function startPiWeb(pi: ExtensionAPI): Promise<void> {
if (process.platform === "darwin") {
await pi.exec("sh", [
"-lc",
`plist="$HOME/Library/LaunchAgents/com.pi-web.plist"; if [ ! -f "$plist" ]; then exit 127; fi; launchctl bootstrap "gui/$(id -u)" "$plist" 2>/dev/null || launchctl load "$plist" 2>/dev/null || true; launchctl kickstart -k "gui/$(id -u)/com.pi-web" 2>/dev/null || launchctl start com.pi-web`,
`plist="$HOME/Library/LaunchAgents/com.pi-web.plist"; if [ ! -f "$plist" ]; then exit 127; fi; ` +
`launchctl bootstrap "gui/$(id -u)" "$plist" 2>/dev/null || launchctl load "$plist" 2>/dev/null || true; ` +
// Something already bound to the port is not "not running"; leave it alone.
`if nc -z '${host}' '${port}' 2>/dev/null; then exit 0; fi; ` +
`launchctl start com.pi-web 2>/dev/null || true`,
]);
return;
}
Expand Down Expand Up @@ -318,8 +334,13 @@ async function ensurePiWebRunning(
): Promise<boolean> {
if (await healthCheck(host, port)) return true;

// Confirm before touching a service other processes may be using: concurrent
// pi starts amplify a single slow response into restarts of a healthy server.
await new Promise((resolve) => setTimeout(resolve, 1500));
if (await healthCheck(host, port)) return true;

try {
await startPiWeb(pi);
await startPiWeb(pi, host, port);
} catch {
return false;
}
Expand Down Expand Up @@ -825,7 +846,7 @@ export default function (pi: ExtensionAPI) {
return;
}
try {
await startPiWeb(pi);
await startPiWeb(pi, host, port);
let started = false;
for (let i = 0; i < 10; i++) {
await new Promise((resolve) => setTimeout(resolve, 300));
Expand Down