diff --git a/cmd/toolsrv/main.go b/cmd/toolsrv/main.go index 271ef11..b1cc766 100644 --- a/cmd/toolsrv/main.go +++ b/cmd/toolsrv/main.go @@ -22,6 +22,7 @@ import ( "os/user" "path/filepath" "strings" + "sync" "sync/atomic" "syscall" "time" @@ -54,6 +55,9 @@ var activeConns atomic.Int64 // hadConnection is set to true once the first client connects. var hadConnection atomic.Bool +// procInterruptWG tracks completion notifications during shutdown. +var procInterruptWG sync.WaitGroup + func main() { if len(os.Args) < 2 || os.Args[1] != "serve" { fmt.Fprintln(os.Stderr, "usage: toolsrv serve --cwd --listen [--yolo]") @@ -91,32 +95,37 @@ func main() { srv.SetRegistry(toolReg) } + // Set up signal handling + runCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + sigCh := make(chan os.Signal, 1) + signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigCh) + go func() { + <-sigCh + cancel() + }() + // Set up callback to push completed proc output to agent prompt srv.Fs.OnProcExit = func(proc *server.Proc) { if proc.SessionID == "" || proc.AgentID == "" { return } - go pushProcInterrupt(proc) + procInterruptWG.Add(1) + go func() { + defer procInterruptWG.Done() + pushProcInterrupt(runCtx, proc) + }() } - // Build the filesystem tree tree := srv.BuildTree() if tree == nil { logger.Error("failed to build filesystem tree") os.Exit(1) } - // Set up signal handling - runCtx, cancel := context.WithCancel(context.Background()) - defer cancel() - sigCh := make(chan os.Signal, 1) - signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) - go func() { - <-sigCh - cancel() - }() - // Kill all procs on shutdown + defer procInterruptWG.Wait() defer srv.Fs.KillAll() // Start idle timeout monitor @@ -218,7 +227,7 @@ monitor: } // pushProcInterrupt pushes the completed proc output to the agent's prompt. -func pushProcInterrupt(proc *server.Proc) { +func pushProcInterrupt(ctx context.Context, proc *server.Proc) { // Build the interrupt message proc.Lock() output := proc.OutputString() @@ -244,12 +253,21 @@ func pushProcInterrupt(proc *server.Proc) { } sockPath := fmt.Sprintf("/tmp/ns.%s.%s/ollie", u.Username, display) - conn, err := p9client.Dial("unix", sockPath) + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + conn, err := dialUnixContext(ctx, sockPath) if err != nil { logger.Warn("pushProcInterrupt: failed to connect to olliesrv: %v", err) return } - fsys, err := conn.Attach(nil, u.Username, "") + p9conn, err := p9client.NewConn(conn) + if err != nil { + conn.Close() + logger.Warn("pushProcInterrupt: failed to initialize 9P connection: %v", err) + return + } + defer p9conn.Close() + fsys, err := p9conn.Attach(nil, u.Username, "") if err != nil { conn.Close() logger.Warn("pushProcInterrupt: failed to attach to olliesrv: %v", err) @@ -277,7 +295,11 @@ func pushProcInterrupt(proc *server.Proc) { logger.Info("pushed proc %d interrupt to %s", pid, promptPath) } -// escapeAttr escapes a string for use in an XML-like attribute. +func dialUnixContext(ctx context.Context, path string) (net.Conn, error) { + var d net.Dialer + return d.DialContext(ctx, "unix", path) +} + func escapeAttr(s string) string { s = strings.ReplaceAll(s, "&", "&") s = strings.ReplaceAll(s, "\"", """) diff --git a/doc/concurrency-resource-audit.md b/doc/concurrency-resource-audit.md index 0c7bb4f..9574dff 100644 --- a/doc/concurrency-resource-audit.md +++ b/doc/concurrency-resource-audit.md @@ -274,13 +274,9 @@ Timeout and send-error paths delete the request channel from `pending`. Late res ### M12. `pushProcInterrupt` has weak connection ownership -**Evidence:** `cmd/toolsrv/main.go:247-258`. +**Status:** Fixed in the working tree. -The function explicitly closes the filesystem but not the underlying connection. It is launched once per completed background process without a bound or timeout. - -**Impact:** potential socket retention and completion-goroutine pressure. - -**Remediation:** close both filesystem and connection, use a timeout, and track completion work under toolsrv shutdown. +Completion notifications now use the toolsrv run context with a ten-second timeout, close both the 9P filesystem and connection, and are tracked by a wait group during shutdown. ### M13. Repeated process-GC startup creates duplicate goroutines