diff --git a/internals/overlord/cmdstate/export_test.go b/internals/overlord/cmdstate/export_test.go new file mode 100644 index 000000000..4aab805d1 --- /dev/null +++ b/internals/overlord/cmdstate/export_test.go @@ -0,0 +1,24 @@ +// Copyright (c) 2026 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package cmdstate + +// AddTestExecution inserts a fake execution into the manager's map and +// broadcasts the condition variable. +func (m *CommandManager) AddTestExecution(taskID string) { + m.executionsCond.L.Lock() + m.executions[taskID] = &execution{} + m.executionsCond.Broadcast() + m.executionsCond.L.Unlock() +} diff --git a/internals/overlord/cmdstate/manager.go b/internals/overlord/cmdstate/manager.go index 1daf1728b..6b08d11ef 100644 --- a/internals/overlord/cmdstate/manager.go +++ b/internals/overlord/cmdstate/manager.go @@ -68,10 +68,12 @@ func (m *CommandManager) Connect(r *http.Request, w http.ResponseWriter, task *s m.executionsCond.L.Unlock() }() - executionCh := make(chan *execution) + executionCh := make(chan *execution, 1) go func() { e := m.waitExecution(task.ID(), stopWait) if e != nil { + // executionCh is buffered to avoid a goroutine leak when this is + // blocked indefinitely. executionCh <- e } }() diff --git a/internals/overlord/cmdstate/manager_test.go b/internals/overlord/cmdstate/manager_test.go new file mode 100644 index 000000000..b5dc84070 --- /dev/null +++ b/internals/overlord/cmdstate/manager_test.go @@ -0,0 +1,60 @@ +// Copyright (c) 2026 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package cmdstate_test + +import ( + "context" + "net/http" + "net/http/httptest" + + . "gopkg.in/check.v1" + + "github.com/canonical/pebble/internals/overlord/cmdstate" + "github.com/canonical/pebble/internals/overlord/state" +) + +type managerSuite struct{} + +var _ = Suite(&managerSuite{}) + +// TestConnectContextCancelledGoroutineLeak demonstrates the goroutine leak in +// Connect. executionCh is unbuffered: if Connect exits via r.Context().Done() +// after waitExecution has already found a non-nil execution, the goroutine +// blocks forever trying to send on executionCh with no receiver. +// +// The test is probabilistic. +func (s *managerSuite) TestConnectContextCancelledGoroutineLeak(c *C) { + st := state.New(nil) + runner := state.NewTaskRunner(st) + mgr := cmdstate.NewManager(runner) + + st.Lock() + task := st.NewTask("exec", "test cmd") + chg := st.NewChange("exec", "test change") + chg.AddTask(task) + st.Unlock() + + // Pre-register the execution so waitExecution returns immediately + // with a non-nil value, before stopWait can be closed. + mgr.AddTestExecution(task.ID()) + + // Pre-cancel the context so Connect can exit via r.Context().Done() + // while the goroutine is still trying to send on executionCh. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + r := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) + _ = mgr.Connect(r, httptest.NewRecorder(), task, "stdio") +} diff --git a/internals/overlord/cmdstate/package_test.go b/internals/overlord/cmdstate/package_test.go new file mode 100644 index 000000000..a4ea8d462 --- /dev/null +++ b/internals/overlord/cmdstate/package_test.go @@ -0,0 +1,26 @@ +// Copyright (c) 2026 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package cmdstate_test + +import ( + "testing" + + "github.com/canonical/pebble/internals/testutil" + . "gopkg.in/check.v1" +) + +func Test(t *testing.T) { + testutil.PrintGoroutineLeaks(t, TestingT) +}