openvibers/openvibe.link Commits

plugins: a Windows kill takes every process the plugin started (Job Object) (#92)

OP OpenVibers committed 11 days ago · af4b66bc10 · 8 files, +207 −11

TerminateProcess ends only the direct child, so a plugin's helper (a stdio relay's own process, speech, a
camera helper) survived the plugin and could keep a robot's Bluetooth link open while the Node stopped or
restarted the plugin. Linux and macOS already kill the process group. Each plugin run now gets a Windows Job
Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE: killing the plugin terminates the job and closes its handle,
and the Node exiting closes the handle too. Probe mode gets the same job. Unix behaviour is unchanged.

The new Windows-only test starts a grandchild from the fake plugin and has it ignore stdin, then checks the
grandchild is gone within 2 s of the kill; CI runs it verbosely on windows-latest.

Changes

ddiff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
iindex c7c3c46..16447cd 100644
---- a/.github/workflows/ci.yml
++++ b/.github/workflows/ci.yml
@@@ -98,6 +98,11 @@ jobs:
# Python SDK's own suite, verbose so the log shows each ran on this OS (and its clock readings), never skipped.
- name: deadlines across the pipe (verbose)
run: go test -count=1 -v -run "TestQPC|TestTheClock|TestUntilAndSince|TestPython|TestHelloNamesTheSharedClock|TestPipeDelay|TestWriterQueue|TestStrictTiming" ./internal/mono ./internal/plugins
++ # A plugin's own children must not outlive it on Windows (a Job Object, as the process group does on macOS):
++ # verbose so the log shows this ran there rather than being skipped.
++ - name: plugin process tree (verbose)
++ if: runner.os == 'Windows'
++ run: go test -count=1 -v -run TestPluginKillEndsTheWholeTree ./internal/plugins
- run: go test -count=1 ./...
python:
ddiff --git a/docs/hardware-looi.md b/docs/hardware-looi.md
iindex 81674fc..5be2535 100644
---- a/docs/hardware-looi.md
++++ b/docs/hardware-looi.md
@@@ -99,7 +99,9 @@ openvibe-node install --user-session --home "%OVHOME%"
`install --user-session` starts the Node when you sign in, in a console window that shows its log (Windows keeps no
service log yet). Closing the window stops the Node and every actuator; nothing restarts it until the next sign-in.
--`openvibe-node uninstall --user-session` removes the sign-in start.
++`openvibe-node uninstall --user-session` removes the sign-in start. On Windows a killed plugin takes the processes it
++started with it (a Job Object), as it does on Linux and macOS, so a bridge helper cannot keep the Bluetooth link open
++after the Node stops or restarts the plugin.
Try the driver without the robot first: `openvibe-node plugins` asks each plugin to describe itself without touching
hardware, and `openvibe-node run --dry-run` drives only the simulated plugin.
ddiff --git a/docs/plugins.md b/docs/plugins.md
iindex 127d90b..b493289 100644
---- a/docs/plugins.md
++++ b/docs/plugins.md
@@@ -35,6 +35,8 @@ computer plugin's shared desks) reads the same config as the Node that started i
4. The core sends `heartbeat` every 250 ms and commands as they arrive; the plugin answers each command with `ack` or
`nack` and sends `telemetry`, `event` and `video` messages whenever it has them.
5. On shutdown the core sends `stop` and closes stdin; the plugin stops everything and exits. After 2 s it is killed.
++ On Windows the kill covers every process the plugin started (a Job Object), as killing the process group does on
++ Linux and macOS.
A plugin that exits is restarted with exponential backoff (1 s doubling to 30 s, reset after a minute of uptime). It
starts stopped; if the e-stop or the local kill switch is latched, the core sends `estop` right after `hello`.
ddiff --git a/internal/plugins/plugin.go b/internal/plugins/plugin.go
iindex 5d1311c..7f4b288 100644
---- a/internal/plugins/plugin.go
++++ b/internal/plugins/plugin.go
@@@ -559,9 +559,14 @@ func (p *Plugin) runOnce(ctx context.Context) error {
if err := cmd.Start(); err != nil {
return fmt.Errorf("start %s: %w", p.spec.Argv[0], err)
}
++ tree, treeErr := startProcessTree(cmd)
++ if treeErr != nil {
++ p.log.Warn("plugin is not in a job object; processes it starts may outlive it", "err", errString(treeErr))
++ }
++ defer tree.close()
queue := make(chan line, 256)
var killOnce sync.Once
-- kill := func() { killOnce.Do(func() { killProcess(cmd) }) }
++ kill := func() { killOnce.Do(func() { tree.kill() }) }
p.mu.Lock()
p.queue, p.kill, p.pid, p.fault, p.desc = queue, kill, cmd.Process.Pid, nil, nil
estopped, estopCause, estopReason, haltEpoch := p.estopped, p.estopCause, p.estopReason, p.haltEpoch
@@@ -633,8 +638,9 @@ func (p *Plugin) runOnce(ctx context.Context) error {
}
// Watch the plugin process itself, not its stdout: a child it started (speech, a camera helper) can hold stdout
-- // open after the plugin died. When the plugin exits, its whole process group is killed, which also frees any
-- // motor bus or serial port a child still holds, and ends the stdout reader.
++ // open after the plugin died. When the plugin exits, its whole process tree is killed (the process group on Unix,
++ // the Job Object on Windows), which also frees any motor bus or serial port a child still holds, and ends the
++ // stdout reader.
exited := make(chan error, 1)
go func() {
ps, err := cmd.Process.Wait()
@@@ -819,6 +825,9 @@ func Probe(ctx context.Context, spec Spec) (*Describe, error) {
return nil, err
}
defer func() { _ = cmd.Wait() }()
++ // The probe must not leave children behind either: CommandContext kills only the direct process.
++ tree, _ := startProcessTree(cmd)
++ defer tree.close()
defer stdin.Close()
b, _ := json.Marshal(map[string]any{"op": "hello", "config": nonNil(spec.Config), "probe": true})
if _, err := stdin.Write(append(b, '\n')); err != nil {
ddiff --git a/internal/plugins/plugins_test.go b/internal/plugins/plugins_test.go
iindex 3e15eac..a686e88 100644
---- a/internal/plugins/plugins_test.go
++++ b/internal/plugins/plugins_test.go
@@@ -8,7 +8,9 @@ import (
"io"
"log/slog"
"os"
++ "os/exec"
"path/filepath"
++ "strconv"
"strings"
"sync"
"syscall"
@@@ -22,6 +24,13 @@ import (
// The test binary doubles as a fake plugin: with OPENVIBE_FAKE_PLUGIN set it speaks the plugin protocol and appends
// everything it receives to the file named by that variable.
func TestMain(m *testing.M) {
++ // A grandchild of a fake plugin (mode "jobtree"): write its pid where the test reads it, then wait to be killed
++ // with the plugin's process tree. Checked before OPENVIBE_FAKE_PLUGIN, which it inherits.
++ if path := os.Getenv("OPENVIBE_FAKE_GRANDCHILD"); path != "" {
++ _ = os.WriteFile(path, []byte(strconv.Itoa(os.Getpid())), 0o600)
++ time.Sleep(20 * time.Second)
++ os.Exit(0)
++ }
if path := os.Getenv("OPENVIBE_FAKE_PLUGIN"); path != "" {
fakePlugin(path, os.Stdin, os.Stdout)
os.Exit(0)
@@@ -78,6 +87,19 @@ func fakePlugin(logPath string, in io.Reader, out io.Writer) {
if m["probe"] == true {
continue
}
++ if mode == "jobtree" {
++ // A grandchild of its own, then ignore stdin as a plugin stuck in a handler would: the core has to
++ // kill the whole tree rather than see the plugin exit by itself.
++ exe, _ := os.Executable()
++ child := exec.Command(exe)
++ child.Env = append(os.Environ(), "OPENVIBE_FAKE_GRANDCHILD="+os.Getenv("OPENVIBE_FAKE_GRANDCHILD_PIDFILE"))
++ if err := child.Start(); err != nil {
++ rec("GRANDCHILD-ERROR " + err.Error())
++ }
++ w.Encode(map[string]any{"op": "ready"})
++ time.Sleep(20 * time.Second)
++ continue
++ }
if mode == "fault" {
w.Encode(map[string]any{"op": "fault", "fault_code": "firmware_unsupported", "message": "v1 not v2"})
continue
@@@ -124,12 +146,13 @@ type fixture struct {
seen []Output
}
--func start(t *testing.T, mode string) *fixture {
++func start(t *testing.T, mode string, extraEnv ...string) *fixture {
t.Helper()
logPath := filepath.Join(t.TempDir(), "plugin.log")
exe, _ := os.Executable()
-- spec := Spec{Name: "fake", Argv: []string{exe}, Env: []string{"OPENVIBE_FAKE_PLUGIN=" + logPath, "OPENVIBE_FAKE_MODE=" + mode},
-- Config: map[string]any{"k": "v"}}
++ env := []string{"OPENVIBE_FAKE_PLUGIN=" + logPath, "OPENVIBE_FAKE_MODE=" + mode}
++ env = append(env, extraEnv...)
++ spec := Spec{Name: "fake", Argv: []string{exe}, Env: env, Config: map[string]any{"k": "v"}}
m := NewManager([]Spec{spec}, slog.New(slog.NewTextHandler(io.Discard, nil)))
ctx, cancel := context.WithCancel(context.Background())
m.Start(ctx)
ddiff --git a/internal/plugins/plugins_windows_test.go b/internal/plugins/plugins_windows_test.go
nnew file mode 100644
iindex 0000000..88d54e0
---- /dev/null
++++ b/internal/plugins/plugins_windows_test.go
@@@ -0,0 +1,61 @@
++//go:build windows
++
++package plugins
++
++import (
++ "os"
++ "path/filepath"
++ "strconv"
++ "strings"
++ "testing"
++ "time"
++
++ "golang.org/x/sys/windows"
++)
++
++// A plugin's own children must not outlive it on Windows: TerminateProcess reaches the direct child only, so the
++// core puts the plugin in a Job Object and kills the whole tree. The fake plugin (the test binary, mode "jobtree")
++// starts a grandchild and then ignores stdin, so the core has to kill it after StopGrace instead of the plugin
++// exiting by itself on EOF.
++func TestPluginKillEndsTheWholeTree(t *testing.T) {
++ pidFile := filepath.Join(t.TempDir(), "grandchild.pid")
++ fx := start(t, "jobtree", "OPENVIBE_FAKE_GRANDCHILD_PIDFILE="+pidFile)
++ fx.waitState(StateReady)
++ pid := waitGrandchildPID(t, pidFile)
++ fx.stop()
++ deadline := time.Now().Add(2 * time.Second)
++ for !processGone(uint32(pid)) {
++ if time.Now().After(deadline) {
++ t.Fatalf("grandchild %d is still running 2 s after the plugin was killed", pid)
++ }
++ time.Sleep(20 * time.Millisecond)
++ }
++}
++
++func waitGrandchildPID(t *testing.T, path string) int {
++ t.Helper()
++ deadline := time.Now().Add(5 * time.Second)
++ for time.Now().Before(deadline) {
++ if b, err := os.ReadFile(path); err == nil {
++ if pid, err := strconv.Atoi(strings.TrimSpace(string(b))); err == nil && pid > 0 {
++ return pid
++ }
++ }
++ time.Sleep(10 * time.Millisecond)
++ }
++ t.Fatalf("the plugin never wrote the grandchild's pid to %s", path)
++ return 0
++}
++
++// processGone reports whether the process has exited (or its process object is gone). os.FindProcess always
++// succeeds on Windows, so the test opens the pid: a terminated process whose handles are closed is no longer
++// openable, and one that still exists is waited on without blocking.
++func processGone(pid uint32) bool {
++ h, err := windows.OpenProcess(windows.SYNCHRONIZE, false, pid)
++ if err != nil {
++ return true
++ }
++ defer windows.CloseHandle(h)
++ event, err := windows.WaitForSingleObject(h, 0)
++ return err == nil && event == windows.WAIT_OBJECT_0
++}
ddiff --git a/internal/plugins/proc_tree_other.go b/internal/plugins/proc_tree_other.go
nnew file mode 100644
iindex 0000000..81d0f83
---- /dev/null
++++ b/internal/plugins/proc_tree_other.go
@@@ -0,0 +1,16 @@
++//go:build !windows
++
++package plugins
++
++import "os/exec"
++
++// processTree is the plugin's own process group on Unix, set up by setProcAttr before Start. Killing the group is
++// killProcess, so there is nothing more to hold.
++type processTree struct{ cmd *exec.Cmd }
++
++func startProcessTree(cmd *exec.Cmd) (*processTree, error) { return &processTree{cmd: cmd}, nil }
++
++func (t *processTree) kill() { killProcess(t.cmd) }
++
++// close is empty: the process group goes away with the processes in it.
++func (t *processTree) close() {}
ddiff --git a/internal/plugins/proc_windows.go b/internal/plugins/proc_windows.go
iindex 834cdb6..de99000 100644
---- a/internal/plugins/proc_windows.go
++++ b/internal/plugins/proc_windows.go
@@@ -2,12 +2,90 @@
package plugins
--import "os/exec"
++import (
++ "os/exec"
++ "sync"
++ "unsafe"
++ "golang.org/x/sys/windows"
++)
++
++// setProcAttr is empty on Windows: a process can join a Job Object only once it is running, so the tree is set up
++// after Start (startProcessTree).
func setProcAttr(cmd *exec.Cmd) {}
--func killProcess(cmd *exec.Cmd) {
-- if cmd.Process != nil {
-- _ = cmd.Process.Kill()
++// processTree is the Job Object that holds one plugin process and every process it starts. Windows has no process
++// group for this: TerminateProcess ends only the direct child, so a plugin that starts its own helper (a stdio
++// relay's child, speech, a camera helper) would leave it running when the Node kills or restarts the plugin. The
++// job carries JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so closing the last handle also ends everything in it when the
++// Node exits or crashes and Windows closes its handles.
++type processTree struct {
++ cmd *exec.Cmd
++ mu sync.Mutex
++ job windows.Handle // 0 once kill/close took it, or when no job could be made
++}
++
++// startProcessTree creates the job and puts the running plugin in it. A process the plugin starts before this call
++// would be outside the job; the call follows Start at once and the plugin is not sent hello until after it, so the
++// gap is the few instructions between the two.
++func startProcessTree(cmd *exec.Cmd) (*processTree, error) {
++ t := &processTree{cmd: cmd}
++ job, err := windows.CreateJobObject(nil, nil)
++ if err != nil {
++ return t, err
++ }
++ info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{
++ BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{
++ LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE,
++ },
++ }
++ if _, err := windows.SetInformationJobObject(job, windows.JobObjectExtendedLimitInformation,
++ uintptr(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil {
++ _ = windows.CloseHandle(job)
++ return t, err
++ }
++ // Assigning needs PROCESS_SET_QUOTA and PROCESS_TERMINATE on the process, not just its pid.
++ proc, err := windows.OpenProcess(windows.PROCESS_SET_QUOTA|windows.PROCESS_TERMINATE, false, uint32(cmd.Process.Pid))
++ if err != nil {
++ _ = windows.CloseHandle(job)
++ return t, err
++ }
++ err = windows.AssignProcessToJobObject(job, proc)
++ _ = windows.CloseHandle(proc)
++ if err != nil {
++ _ = windows.CloseHandle(job)
++ return t, err
++ }
++ t.mu.Lock()
++ t.job = job
++ t.mu.Unlock()
++ return t, nil
++}
++
++// kill ends the whole tree: TerminateJobObject for every process in the job, then the plugin itself in case no job
++// could be made.
++func (t *processTree) kill() {
++ if job := t.takeJob(); job != 0 {
++ _ = windows.TerminateJobObject(job, 1)
++ _ = windows.CloseHandle(job)
++ }
++ if t.cmd.Process != nil {
++ _ = t.cmd.Process.Kill()
++ }
++}
++
++// close releases the job once the plugin has exited. KILL_ON_JOB_CLOSE ends anything the plugin left behind; a
++// restart starts a new process and so a new job.
++func (t *processTree) close() {
++ if job := t.takeJob(); job != 0 {
++ _ = windows.CloseHandle(job)
}
}
++
++func (t *processTree) takeJob() windows.Handle {
++ t.mu.Lock()
++ defer t.mu.Unlock()
++ job := t.job
++ t.job = 0
++ return job
++}