added goroutine to wait on systray process so it does not zombie when dies, now only spin up systray where uid less than 501, call to systray.Quit on exit

[James-Pickett]

No description provided.

[directionless]

Given your comment -- that the parent need to stick around for some process cleanup -- this makes sense. But I'm a little leery about some fragility.

The biggest (potential) issue I see, is that a misbehaving process, will cause the parent to block on exiting. I feel like that sort of thing needs some protections. Which probably means something hooked to the runner's interrupt maybe? Not sure?

The other thing I see, which is more of a feature than bug, is that this adds a go routine to the parent that's watching this child. That would be an obvious place to do signaling. If we want to exit, send a STOP, and if it doesn't die, send a KILL. Etc.

[directionless]

I think this for loop feel a little weird, but I can't quite say why. I think it's probably fine to merge, and at worst will be noisy and we'll iterate.

[James-Pickett]

yea, it's weird ....

how about ...

if !processExists(proc.Pid) {
	continue
} else if err := proc.Signal(syscall.SIGTERM); err != nil {
	level.Error(r.logger).Log(
		"msg", "error sending SIGTERM to systray process after interrupt",
		"err", err,
	)
} else if err := proc.Kill(); err != nil {
	level.Error(r.logger).Log(
		"msg", "error killing systray process after interrupt",
		"err", err,
	)
}

could use a switch statement, but then you can't capture the err in a variable for logging

switch {
case !processExists(proc.Pid):
	continue
case proc.Signal(syscall.SIGTERM) == nil:
	continue
case proc.Kill() == nil:
	continue
default:
	level.Error(r.logger).Log(
		"msg", "sigterm and kill failed for process",
		"pid", proc.Pid,
	)
}

[directionless]

Yeah, these are both getting at what I mean... Though Ideally you want a small sleep between the kills.

I almost want something like:

for _, proc := range r.uidProcs {
   proc.Signal(syscall.SIGTERM)
}

select {
case <-wgDone:
    return
case <-time.NewTimer(time.Second * 1).C:
    for _, proc := range r.uidProcs {
        proc.Kill()
    }
case <-time.NewTimer(time.Second * 5).C:
    level.Error(r.logger).Log("msg", "timeout waiting for systray processes to finish")
}

can't quite tell if that's correct, or too complicated, or what

[directionless]

Suggested change

	executablePath := filepath.Join(t.TempDir(), "systray")
	executablePath := filepath.Join(t.TempDir(), "systray.exe")

Somewhat optional, but if/when windows, we'll need .exe