From 9a79f3fa64921888e4761d7bd5a3499496578e73 Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Sat, 20 Jun 2026 15:20:52 +1000 Subject: [PATCH 1/6] fix: svcinit tolerates service crash in ibazel reload mode - Replace blocking `for range servicesErrCh` with non-blocking drain so reload proceeds even when no crash errors are pending - Stop() returns nil for already-dead processes instead of propagating ESRCH from killGroup - Hot-reload stdin.Write failure falls back to full restart instead of panicking Fixes https://github.com/hermeticbuild/rules_itest/issues/72 --- cmd/svcinit/main.go | 44 ++++++++++++++++++++++++++++++++++++++ runner/runner.go | 9 +++++++- runner/service_instance.go | 5 +++-- 3 files changed, 55 insertions(+), 3 deletions(-) diff --git a/cmd/svcinit/main.go b/cmd/svcinit/main.go index cc40ec1..7976750 100644 --- a/cmd/svcinit/main.go +++ b/cmd/svcinit/main.go @@ -351,6 +351,50 @@ func main() { if isOneShot { break } + + if shouldHotReload && !enablePerServiceReload { + fmt.Println() + fmt.Println() + fmt.Println("###########################################################################################") + fmt.Println(" Detected that you are running under ibazel, but do not have per-service-reload enabled.") + fmt.Println(" In this configuration, services will not be restarted when their code changes.") + fmt.Println(" If this was unintentional, you can retry with per-service-reload enabled:") + fmt.Println("") + fmt.Printf(" `bazel run --@rules_itest//:enable_per_service_reload %s`\n", testLabel) + fmt.Println("###########################################################################################") + fmt.Println() + fmt.Println() + } + + select { + case <-ctx.Done(): + log.Println("Shutting down services.") + _, err := r.StopAll() + must(err) + log.Println("Cleaning up.") + return + case ibazelCmd := <-interactiveCh: + log.Println(ibazelCmd) + + // Restart any services as needed. + unversionedSpecs, err := readServiceSpecs(serviceSpecsPath) + must(err) + + serviceSpecs, err := augmentServiceSpecs(unversionedSpecs, ports, svcctlPortStr) + must(err) + + // Non-blocking drain of any pending service crash errors before restarting. + for { + select { + case <-servicesErrCh: + default: + goto drained + } + } + drained: + criticalPath, err = r.UpdateSpecsAndRestart(serviceSpecs, servicesErrCh, []byte(ibazelCmd)) + must(err) + } } } diff --git a/runner/runner.go b/runner/runner.go index 5fd3cf7..4027f50 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -200,7 +200,14 @@ func (r *Runner) UpdateSpecs(serviceSpecs ServiceSpecs, ibazelCmd []byte) error for _, label := range updateActions.toReloadLabels { _, err := r.serviceInstances[label].stdin.Write(ibazelCmd) if err != nil { - return err + // Service likely crashed — fall back to a full restart. + log.Printf(colorize(r.serviceInstances[label].VersionedServiceSpec) + " hot-reload stdin write failed, falling back to restart: " + err.Error()) + r.serviceInstances[label].Stop(syscall.SIGKILL) + delete(r.serviceInstances, label) + r.serviceInstances[label], err = prepareServiceInstance(r.ctx, serviceSpecs[label]) + if err != nil { + return err + } } } diff --git a/runner/service_instance.go b/runner/service_instance.go index 99b8cf6..b6715aa 100644 --- a/runner/service_instance.go +++ b/runner/service_instance.go @@ -249,8 +249,9 @@ func (s *ServiceInstance) StopWithSignal(signal syscall.Signal) error { s.killed = true }() - if err != nil { - return err + // If process already exited, nothing to kill. + if s.isDone() { + return nil } if signal == syscall.SIGKILL { From a8eaabfbe764c5e7f8c92cc4b31eb0ecd5850922 Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Sat, 20 Jun 2026 16:02:55 +1000 Subject: [PATCH 2/6] fix: attempt process group kill even when group leader has already exited A crashed service may leave child processes alive in the same process group (holding a port or stdout). Previously we returned nil immediately when isDone() was true, skipping the killGroup call. Now we always attempt the group kill and discard the error (ESRCH means the group is already gone, which is fine). Addresses review comment on #73. --- runner/runner.go | 3 ++- runner/service_instance.go | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/runner/runner.go b/runner/runner.go index 4027f50..8b71826 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -9,6 +9,7 @@ import ( "reflect" "runtime" "sync" + "syscall" "time" "rules_itest/logger" @@ -202,7 +203,7 @@ func (r *Runner) UpdateSpecs(serviceSpecs ServiceSpecs, ibazelCmd []byte) error if err != nil { // Service likely crashed — fall back to a full restart. log.Printf(colorize(r.serviceInstances[label].VersionedServiceSpec) + " hot-reload stdin write failed, falling back to restart: " + err.Error()) - r.serviceInstances[label].Stop(syscall.SIGKILL) + r.serviceInstances[label].StopWithSignal(syscall.SIGKILL) delete(r.serviceInstances, label) r.serviceInstances[label], err = prepareServiceInstance(r.ctx, serviceSpecs[label]) if err != nil { diff --git a/runner/service_instance.go b/runner/service_instance.go index b6715aa..774384a 100644 --- a/runner/service_instance.go +++ b/runner/service_instance.go @@ -249,8 +249,10 @@ func (s *ServiceInstance) StopWithSignal(signal syscall.Signal) error { s.killed = true }() - // If process already exited, nothing to kill. + // If process already exited, still attempt to clean up any remaining + // process group members (children may outlive the group leader). if s.isDone() { + _ = killGroup(s.cmd, signal) return nil } From 9bbc0343d220578fdb003655db28726ef25749a0 Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Sat, 20 Jun 2026 16:36:45 +1000 Subject: [PATCH 3/6] fix: log drained errors and close stdin on crash fallback - Log each discarded service error during pre-reload drain instead of silently dropping them - Explicitly close old stdin pipe before stopping crashed service in hot-reload fallback - Log StopWithSignal error in crash fallback instead of silently ignoring --- cmd/svcinit/main.go | 17 +++++++++-------- runner/runner.go | 8 ++++++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/cmd/svcinit/main.go b/cmd/svcinit/main.go index 7976750..e1d5fd6 100644 --- a/cmd/svcinit/main.go +++ b/cmd/svcinit/main.go @@ -383,15 +383,16 @@ func main() { serviceSpecs, err := augmentServiceSpecs(unversionedSpecs, ports, svcctlPortStr) must(err) - // Non-blocking drain of any pending service crash errors before restarting. - for { - select { - case <-servicesErrCh: - default: - goto drained - } + // Non-blocking drain of any pending service crash errors before restarting. + for { + select { + case crashErr := <-servicesErrCh: + log.Printf("Discarding pending service error before reload: %v", crashErr) + default: + goto drained } - drained: + } + drained: criticalPath, err = r.UpdateSpecsAndRestart(serviceSpecs, servicesErrCh, []byte(ibazelCmd)) must(err) } diff --git a/runner/runner.go b/runner/runner.go index 8b71826..66fb4e7 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -202,8 +202,12 @@ func (r *Runner) UpdateSpecs(serviceSpecs ServiceSpecs, ibazelCmd []byte) error _, err := r.serviceInstances[label].stdin.Write(ibazelCmd) if err != nil { // Service likely crashed — fall back to a full restart. - log.Printf(colorize(r.serviceInstances[label].VersionedServiceSpec) + " hot-reload stdin write failed, falling back to restart: " + err.Error()) - r.serviceInstances[label].StopWithSignal(syscall.SIGKILL) + old := r.serviceInstances[label] + log.Printf("%s hot-reload stdin write failed, falling back to restart: %v", colorize(old.VersionedServiceSpec), err) + old.stdin.Close() + if stopErr := old.StopWithSignal(syscall.SIGKILL); stopErr != nil { + log.Printf("%s stop during crash fallback failed: %v", colorize(old.VersionedServiceSpec), stopErr) + } delete(r.serviceInstances, label) r.serviceInstances[label], err = prepareServiceInstance(r.ctx, serviceSpecs[label]) if err != nil { From fdab41a1fddd1af1d3d9a118986bc1f4b5d9b678 Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Sat, 20 Jun 2026 16:43:27 +1000 Subject: [PATCH 4/6] fix: remove duplicate reload block from bad rebase; log drained errors in upstream Drain block --- cmd/svcinit/main.go | 53 ++++----------------------------------------- 1 file changed, 4 insertions(+), 49 deletions(-) diff --git a/cmd/svcinit/main.go b/cmd/svcinit/main.go index e1d5fd6..4c10ee5 100644 --- a/cmd/svcinit/main.go +++ b/cmd/svcinit/main.go @@ -285,17 +285,17 @@ func main() { testCancel() - // TODO(zbarsky): what is the right behavior here when services are crashing in ibazel mode? - // This is a brittle way of draining a channel in a nonblocking way, // consider instead signalling cancellation of the services with a // context, letting them close the channel, and using a waitgroup to // wait for them to exit. + // Non-blocking drain of any pending service crash errors before restarting. + // See: https://github.com/hermeticbuild/rules_itest/issues/72 Drain: for { select { - case <-servicesErrCh: - // nothing + case crashErr := <-servicesErrCh: + log.Printf("Discarding pending service error before reload: %v", crashErr) default: break Drain } @@ -351,51 +351,6 @@ func main() { if isOneShot { break } - - if shouldHotReload && !enablePerServiceReload { - fmt.Println() - fmt.Println() - fmt.Println("###########################################################################################") - fmt.Println(" Detected that you are running under ibazel, but do not have per-service-reload enabled.") - fmt.Println(" In this configuration, services will not be restarted when their code changes.") - fmt.Println(" If this was unintentional, you can retry with per-service-reload enabled:") - fmt.Println("") - fmt.Printf(" `bazel run --@rules_itest//:enable_per_service_reload %s`\n", testLabel) - fmt.Println("###########################################################################################") - fmt.Println() - fmt.Println() - } - - select { - case <-ctx.Done(): - log.Println("Shutting down services.") - _, err := r.StopAll() - must(err) - log.Println("Cleaning up.") - return - case ibazelCmd := <-interactiveCh: - log.Println(ibazelCmd) - - // Restart any services as needed. - unversionedSpecs, err := readServiceSpecs(serviceSpecsPath) - must(err) - - serviceSpecs, err := augmentServiceSpecs(unversionedSpecs, ports, svcctlPortStr) - must(err) - - // Non-blocking drain of any pending service crash errors before restarting. - for { - select { - case crashErr := <-servicesErrCh: - log.Printf("Discarding pending service error before reload: %v", crashErr) - default: - goto drained - } - } - drained: - criticalPath, err = r.UpdateSpecsAndRestart(serviceSpecs, servicesErrCh, []byte(ibazelCmd)) - must(err) - } } } From 4edb50ce9f919591c4effcba362382b3e0e3438f Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Sat, 20 Jun 2026 16:49:19 +1000 Subject: [PATCH 5/6] fix: clean up comments; document intentional error discard in StopWithSignal --- cmd/svcinit/main.go | 11 +++++------ runner/service_instance.go | 2 ++ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cmd/svcinit/main.go b/cmd/svcinit/main.go index 4c10ee5..faf40de 100644 --- a/cmd/svcinit/main.go +++ b/cmd/svcinit/main.go @@ -285,12 +285,11 @@ func main() { testCancel() - // This is a brittle way of draining a channel in a nonblocking way, - // consider instead signalling cancellation of the services with a - // context, letting them close the channel, and using a waitgroup to - // wait for them to exit. - // Non-blocking drain of any pending service crash errors before restarting. - // See: https://github.com/hermeticbuild/rules_itest/issues/72 + // This is a brittle way of draining a channel in a nonblocking way, + // consider instead signalling cancellation of the services with a + // context, letting them close the channel, and using a waitgroup to + // wait for them to exit. + // See: https://github.com/hermeticbuild/rules_itest/issues/72 Drain: for { select { diff --git a/runner/service_instance.go b/runner/service_instance.go index 774384a..2222673 100644 --- a/runner/service_instance.go +++ b/runner/service_instance.go @@ -251,6 +251,8 @@ func (s *ServiceInstance) StopWithSignal(signal syscall.Signal) error { // If process already exited, still attempt to clean up any remaining // process group members (children may outlive the group leader). + // Any error (ESRCH, EPERM) is intentionally discarded — the group leader + // is already done, so cleanup is best-effort. if s.isDone() { _ = killGroup(s.cmd, signal) return nil From 1c338ea622f4e9a9821fbd6948b400b7bf59bd74 Mon Sep 17 00:00:00 2001 From: jaqx0r Date: Thu, 25 Jun 2026 09:48:49 +1000 Subject: [PATCH 6/6] fix: address dzbarsky review comments - Move old instance capture above stdin.Write in hot-reload fallback - Remove redundant killGroup call in isDone() fast-path; first killGroup call already handled group cleanup --- runner/runner.go | 4 ++-- runner/service_instance.go | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/runner/runner.go b/runner/runner.go index 66fb4e7..d9f57c7 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -199,10 +199,10 @@ func (r *Runner) UpdateSpecs(serviceSpecs ServiceSpecs, ibazelCmd []byte) error } for _, label := range updateActions.toReloadLabels { - _, err := r.serviceInstances[label].stdin.Write(ibazelCmd) + old := r.serviceInstances[label] + _, err := old.stdin.Write(ibazelCmd) if err != nil { // Service likely crashed — fall back to a full restart. - old := r.serviceInstances[label] log.Printf("%s hot-reload stdin write failed, falling back to restart: %v", colorize(old.VersionedServiceSpec), err) old.stdin.Close() if stopErr := old.StopWithSignal(syscall.SIGKILL); stopErr != nil { diff --git a/runner/service_instance.go b/runner/service_instance.go index 2222673..e645d96 100644 --- a/runner/service_instance.go +++ b/runner/service_instance.go @@ -249,12 +249,9 @@ func (s *ServiceInstance) StopWithSignal(signal syscall.Signal) error { s.killed = true }() - // If process already exited, still attempt to clean up any remaining - // process group members (children may outlive the group leader). - // Any error (ESRCH, EPERM) is intentionally discarded — the group leader - // is already done, so cleanup is best-effort. + // If the process has already exited (raced between killGroup above and here), + // return nil — the killGroup call above already attempted group cleanup. if s.isDone() { - _ = killGroup(s.cmd, signal) return nil }