Skip to content

Commit 28591bd

Browse files
committed
fix(update): require MainPID ownership before treating a daemon as systemd-managed (issue #92)
$INVOCATION_ID and a .service cgroup leaf are inherited by every descendant of any systemd service — e.g. all processes inside a GitHub Actions runner (hosted-compute-agent.service) — so unit membership alone misidentified daemons as supervised and update install skipped restarting them (failing master CI on ubuntu since PR #89). detectSelfSupervisor and the legacy cgroup fallback in resolveDaemonSupervisor now also require the unit's MainPID (via systemctl show) to be the daemon itself or its direct parent (covering ExecStart shell wrappers). When MainPID can't be read the daemon is treated as unsupervised, restoring the pre-#82 direct-restart path.
1 parent 1849ff8 commit 28591bd

2 files changed

Lines changed: 128 additions & 18 deletions

File tree

src/common-utils/daemon-state.ts

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -68,32 +68,80 @@ export async function readSystemdUnit(pid: number | "self"): Promise<string | un
6868
}
6969
}
7070

71+
/**
72+
* MainPID of a systemd unit as reported by the system manager, or undefined when it cannot be
73+
* determined (systemctl missing, unit unknown, MainPID=0). `systemctl --user` units are invisible
74+
* to the system manager and resolve to undefined — the daemon is then treated as unsupervised,
75+
* matching the restart path, which also only drives the system manager.
76+
*/
77+
export async function readUnitMainPid(unit: string): Promise<number | undefined> {
78+
try {
79+
const { stdout } = await execFileAsync("systemctl", ["show", "--property=MainPID", "--value", unit]);
80+
const mainPid = Number(String(stdout).trim());
81+
return Number.isInteger(mainPid) && mainPid > 0 ? mainPid : undefined;
82+
} catch {
83+
return undefined;
84+
}
85+
}
86+
87+
/** Parent PID of `pid` from /proc/<pid>/stat, or undefined. */
88+
export async function readParentPid(pid: number): Promise<number | undefined> {
89+
try {
90+
const stat = await fs.readFile(`/proc/${pid}/stat`, "utf-8");
91+
// Format: `pid (comm) state ppid …` — comm may itself contain spaces or parens, so the
92+
// fields are only parseable after the LAST closing paren.
93+
const afterComm = stat.slice(stat.lastIndexOf(")") + 2);
94+
const ppid = Number(afterComm.split(" ")[1]);
95+
return Number.isInteger(ppid) && ppid > 0 ? ppid : undefined;
96+
} catch {
97+
return undefined;
98+
}
99+
}
100+
71101
/**
72102
* Detect whether THIS process was started by systemd, and under which unit. systemd sets
73-
* $INVOCATION_ID for every service it spawns; the unit name comes from this process's own cgroup.
74-
* `env`/`readUnit` are injectable for testing. Returns undefined when not systemd-supervised.
103+
* $INVOCATION_ID for every service it spawns and the unit name comes from this process's own
104+
* cgroup — but BOTH are inherited by every descendant of any service (e.g. all processes inside a
105+
* CI runner's agent service, issue #92), so unit membership alone is not ownership. The daemon is
106+
* only systemd-supervised when it is the unit's main process (or its direct child, covering
107+
* `ExecStart=/bin/sh -c …` wrappers); otherwise restarting the unit would bounce an unrelated
108+
* service. `env`/`readUnit`/`getMainPid`/`self` are injectable for testing. Returns undefined when
109+
* not systemd-supervised.
75110
*/
76111
export async function detectSelfSupervisor(
77112
env: NodeJS.ProcessEnv = process.env,
78-
readUnit: (pid: number | "self") => Promise<string | undefined> = readSystemdUnit
113+
readUnit: (pid: number | "self") => Promise<string | undefined> = readSystemdUnit,
114+
getMainPid: (unit: string) => Promise<number | undefined> = readUnitMainPid,
115+
self: { pid: number; ppid: number } = { pid: process.pid, ppid: process.ppid }
79116
): Promise<DaemonSupervisor | undefined> {
80117
if (!env.INVOCATION_ID) return undefined;
81118
const unit = await readUnit("self");
82-
return unit ? { type: "systemd", unit } : undefined;
119+
if (!unit) return undefined;
120+
const mainPid = await getMainPid(unit);
121+
if (mainPid === undefined || (mainPid !== self.pid && mainPid !== self.ppid)) return undefined;
122+
return { type: "systemd", unit };
83123
}
84124

85125
/**
86126
* Resolve the supervisor for a daemon described by `state`. Prefers the `supervisor` it recorded
87-
* at startup; for legacy daemons that predate that field, falls back to inferring the unit from the
88-
* live process's cgroup. `readUnit` is injectable for testing.
127+
* at startup (already ownership-verified by detectSelfSupervisor); for legacy daemons that predate
128+
* that field, falls back to inferring the unit from the live process's cgroup, with the same
129+
* MainPID ownership check as detectSelfSupervisor (issue #92).
130+
* `readUnit`/`getMainPid`/`getParentPid` are injectable for testing.
89131
*/
90132
export async function resolveDaemonSupervisor(
91133
state: DaemonState,
92-
readUnit: (pid: number | "self") => Promise<string | undefined> = readSystemdUnit
134+
readUnit: (pid: number | "self") => Promise<string | undefined> = readSystemdUnit,
135+
getMainPid: (unit: string) => Promise<number | undefined> = readUnitMainPid,
136+
getParentPid: (pid: number) => Promise<number | undefined> = readParentPid
93137
): Promise<DaemonSupervisor | undefined> {
94138
if (state.supervisor) return state.supervisor;
95139
const unit = await readUnit(state.pid);
96-
return unit ? { type: "systemd", unit } : undefined;
140+
if (!unit) return undefined;
141+
const mainPid = await getMainPid(unit);
142+
if (mainPid === undefined) return undefined;
143+
if (mainPid !== state.pid && mainPid !== (await getParentPid(state.pid))) return undefined;
144+
return { type: "systemd", unit };
97145
}
98146

99147
function stateFilePath(pid: number): string {

test/common-utils/daemon-supervisor.test.ts

Lines changed: 72 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,18 +44,37 @@ describe("parseSystemdUnitFromCgroup", () => {
4444
});
4545

4646
describe("detectSelfSupervisor", () => {
47+
const self = { pid: 5678, ppid: 5600 };
48+
4749
it("returns undefined when INVOCATION_ID is absent (not started by systemd)", async () => {
48-
const sup = await detectSelfSupervisor({}, async () => "bitsocial.service");
50+
const sup = await detectSelfSupervisor({}, async () => "bitsocial.service", async () => self.pid, self);
4951
expect(sup).toBeUndefined();
5052
});
5153

52-
it("returns the systemd unit when INVOCATION_ID is set and the cgroup is a service", async () => {
53-
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => "bitsocial.service");
54+
it("returns the systemd unit when this process is the unit's MainPID", async () => {
55+
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => "bitsocial.service", async () => self.pid, self);
56+
expect(sup).toEqual({ type: "systemd", unit: "bitsocial.service" });
57+
});
58+
59+
it("returns the systemd unit when the parent is the MainPID (ExecStart shell wrapper)", async () => {
60+
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => "bitsocial.service", async () => self.ppid, self);
5461
expect(sup).toEqual({ type: "systemd", unit: "bitsocial.service" });
5562
});
5663

5764
it("returns undefined when INVOCATION_ID is set but the cgroup is not a service", async () => {
58-
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => undefined);
65+
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => undefined, async () => self.pid, self);
66+
expect(sup).toBeUndefined();
67+
});
68+
69+
it("returns undefined when the unit's MainPID is an unrelated ancestor (issue #92: CI runner agent)", async () => {
70+
// On GitHub Actions every process inherits $INVOCATION_ID and lives in the runner agent's
71+
// service cgroup; the agent's MainPID is neither this process nor its parent.
72+
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => "hosted-compute-agent.service", async () => 1234, self);
73+
expect(sup).toBeUndefined();
74+
});
75+
76+
it("returns undefined when the unit's MainPID cannot be read", async () => {
77+
const sup = await detectSelfSupervisor({ INVOCATION_ID: "abc" }, async () => "bitsocial.service", async () => undefined, self);
5978
expect(sup).toBeUndefined();
6079
});
6180
});
@@ -71,16 +90,59 @@ describe("resolveDaemonSupervisor", () => {
7190
expect(sup).toEqual(recorded);
7291
});
7392

74-
it("falls back to the process cgroup for a legacy daemon (no supervisor field)", async () => {
75-
const sup = await resolveDaemonSupervisor(base, async (pid) => {
76-
expect(pid).toBe(base.pid);
77-
return "bitsocial.service";
78-
});
93+
it("falls back to the process cgroup for a legacy daemon that is the unit's MainPID", async () => {
94+
const sup = await resolveDaemonSupervisor(
95+
base,
96+
async (pid) => {
97+
expect(pid).toBe(base.pid);
98+
return "bitsocial.service";
99+
},
100+
async () => base.pid,
101+
async () => undefined
102+
);
79103
expect(sup).toEqual({ type: "systemd", unit: "bitsocial.service" });
80104
});
81105

106+
it("accepts a legacy daemon whose parent is the unit's MainPID (shell wrapper)", async () => {
107+
const sup = await resolveDaemonSupervisor(
108+
base,
109+
async () => "bitsocial.service",
110+
async () => 4000,
111+
async (pid) => {
112+
expect(pid).toBe(base.pid);
113+
return 4000;
114+
}
115+
);
116+
expect(sup).toEqual({ type: "systemd", unit: "bitsocial.service" });
117+
});
118+
119+
it("returns undefined for a legacy daemon inside an unrelated ancestor service (issue #92)", async () => {
120+
const sup = await resolveDaemonSupervisor(
121+
base,
122+
async () => "hosted-compute-agent.service",
123+
async () => 1,
124+
async () => 999
125+
);
126+
expect(sup).toBeUndefined();
127+
});
128+
129+
it("returns undefined for a legacy daemon when the unit's MainPID cannot be read", async () => {
130+
const sup = await resolveDaemonSupervisor(
131+
base,
132+
async () => "bitsocial.service",
133+
async () => undefined,
134+
async () => undefined
135+
);
136+
expect(sup).toBeUndefined();
137+
});
138+
82139
it("returns undefined for a legacy daemon whose cgroup is not a service", async () => {
83-
const sup = await resolveDaemonSupervisor(base, async () => undefined);
140+
const sup = await resolveDaemonSupervisor(
141+
base,
142+
async () => undefined,
143+
async () => base.pid,
144+
async () => undefined
145+
);
84146
expect(sup).toBeUndefined();
85147
});
86148
});

0 commit comments

Comments
 (0)