From cc6d51cb19fd3bd7a77aa7d950f224d9c857859a Mon Sep 17 00:00:00 2001 From: rvasikarla Date: Sun, 5 Apr 2026 17:15:49 -0500 Subject: [PATCH 1/3] fix: noWatch sync no longer prevents SSH injection When a sync config has noWatch: true, the sync goroutine completes after initial sync and defer-cancels its context. This triggers the RestartOnError handler which calls parent.Kill(nil), putting the tomb in a dying state. Since SSH starts after sync completes (it waits on <-syncDone), the killed tomb prevents SSH from starting. Fix: set RestartOnError to false when NoWatch is true. A one-shot sync has no need for restart-on-error handling, and this prevents the handler from killing the parent tomb on context cancellation. Fixes #3005 Signed-off-by: rvasikarla --- pkg/devspace/services/sync/sync.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/devspace/services/sync/sync.go b/pkg/devspace/services/sync/sync.go index e2dcb88c28..e1c740b138 100644 --- a/pkg/devspace/services/sync/sync.go +++ b/pkg/devspace/services/sync/sync.go @@ -124,7 +124,7 @@ func startSync(ctx devspacecontext.Context, name, arch string, syncConfig *lates Arch: arch, Starter: starter, - RestartOnError: true, + RestartOnError: !syncConfig.NoWatch, Verbose: ctx.Log().GetLevel() == logrus.DebugLevel, } From 65c4fa09e844523935f46f5da86a9d7383eb1c6d Mon Sep 17 00:00:00 2001 From: rvasikarla Date: Fri, 1 May 2026 15:44:06 -0500 Subject: [PATCH 2/3] fix: handle onError in non-restart path for noWatch sync Address review feedback: when RestartOnError is false (noWatch), add an else block to listen for onError and onDone channels. Previously, these channels went unlistened when noWatch was true, which could cause goroutine leaks. Signed-off-by: rvasikarla --- pkg/devspace/services/sync/controller.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/devspace/services/sync/controller.go b/pkg/devspace/services/sync/controller.go index 12ab892802..2dafc779d5 100644 --- a/pkg/devspace/services/sync/controller.go +++ b/pkg/devspace/services/sync/controller.go @@ -226,6 +226,23 @@ func (c *controller) startWithWait(ctx devspacecontext.Context, options *Options } return nil }) + } else { + parent.Go(func() error { + select { + case <-ctx.Context().Done(): + syncStop(ctx, client, options, parent) + case err := <-onError: + hook.LogExecuteHooks(ctx.WithLogger(options.SyncLog), map[string]interface{}{ + "sync_config": options.SyncConfig, + "ERROR": err, + }, hook.EventsForSingle("error:sync", options.Name).With("sync.error")...) + ctx.Log().Errorf("Sync error: %v", err) + syncStop(ctx, client, options, parent) + case <-onDone: + syncDone(ctx, options, parent) + } + return nil + }) } return nil From b4b26e92ac6d89f6f155837365410b707e592555 Mon Sep 17 00:00:00 2001 From: Raman Vasikarla Date: Mon, 11 May 2026 16:28:04 +0000 Subject: [PATCH 3/3] fix: noWatch onDone no longer kills parent tomb; add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When noWatch sync completes (onDone), the previous code called syncDone() which internally called parent.Kill(nil). This killed the parent tomb, causing SSH injection and port-forwarding — which start after sync in the same tomb — to be terminated immediately. Fix: in the noWatch (else) branch, handle onDone by stopping the sync client and emitting the stop:sync lifecycle event directly, without calling parent.Kill. Context-cancellation and error paths still call syncStop → parent.Kill as before, because those represent actual shutdown or failure conditions. Adds three unit tests: - TestNoWatchRestartOnError: verifies RestartOnError=false when NoWatch=true - TestNoWatchOnDoneDoesNotKillParent: verifies parent stays alive after noWatch completion - TestNoWatchOnDoneWatchModeKillsParent: documents that watch-mode onDone does kill parent Addresses review feedback from @zerbitx. Signed-off-by: Raman Vasikarla --- pkg/devspace/services/sync/controller.go | 10 +- pkg/devspace/services/sync/controller_test.go | 93 ++++++++++++++++++- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/pkg/devspace/services/sync/controller.go b/pkg/devspace/services/sync/controller.go index 2dafc779d5..a7c7974ce4 100644 --- a/pkg/devspace/services/sync/controller.go +++ b/pkg/devspace/services/sync/controller.go @@ -239,7 +239,15 @@ func (c *controller) startWithWait(ctx devspacecontext.Context, options *Options ctx.Log().Errorf("Sync error: %v", err) syncStop(ctx, client, options, parent) case <-onDone: - syncDone(ctx, options, parent) + // noWatch: initial sync completed normally. + // Stop the client and emit the stop:sync lifecycle event, + // but do NOT kill the parent tomb — SSH, port-forwarding and + // other services registered in the same parent must continue. + client.Stop(nil) + hook.LogExecuteHooks(ctx.WithLogger(options.SyncLog), map[string]interface{}{ + "sync_config": options.SyncConfig, + }, hook.EventsForSingle("stop:sync", options.Name).With("sync.stop")...) + ctx.Log().Debugf("Stopped sync %s", options.SyncConfig.Path) } return nil }) diff --git a/pkg/devspace/services/sync/controller_test.go b/pkg/devspace/services/sync/controller_test.go index d6b5091f9d..40ebc1c6ed 100644 --- a/pkg/devspace/services/sync/controller_test.go +++ b/pkg/devspace/services/sync/controller_test.go @@ -1,8 +1,12 @@ package sync import ( - "gotest.tools/assert" "testing" + "time" + + "github.com/loft-sh/devspace/pkg/devspace/config/versions/latest" + "github.com/loft-sh/devspace/pkg/util/tomb" + "gotest.tools/assert" ) type parseSyncPathTestCase struct { @@ -30,3 +34,90 @@ func TestParseSyncPath(t *testing.T) { assert.Equal(t, remote, testCase.expectedRemote, "Expect remote path in "+testCase.name) } } + +// TestNoWatchRestartOnError verifies that RestartOnError is disabled when NoWatch is true. +// This mirrors the logic in startSync(): RestartOnError: !syncConfig.NoWatch +func TestNoWatchRestartOnError(t *testing.T) { + testCases := []struct { + name string + noWatch bool + wantRestart bool + }{ + {"watch mode enables restart-on-error", false, true}, + {"noWatch mode disables restart-on-error", true, false}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cfg := &latest.SyncConfig{NoWatch: tc.noWatch} + restartOnError := !cfg.NoWatch + assert.Equal(t, restartOnError, tc.wantRestart, + "RestartOnError mismatch for NoWatch=%v", tc.noWatch) + }) + } +} + +// TestNoWatchOnDoneDoesNotKillParent verifies that when a noWatch sync completes +// (onDone fires), the parent tomb is NOT killed. This ensures other services such +// as SSH and port-forwarding, which are registered in the same parent, continue +// running after the one-shot sync finishes. +func TestNoWatchOnDoneDoesNotKillParent(t *testing.T) { + var parent tomb.Tomb + onDone := make(chan struct{}) + handlerExited := make(chan struct{}) + + // Simulate the noWatch (else) branch goroutine from startWithWait. + // The correct behaviour is: on onDone, do NOT call parent.Kill. + parent.Go(func() error { + defer close(handlerExited) + select { + case <-onDone: + // noWatch sync complete — intentionally no parent.Kill here + } + return nil + }) + + // Signal that the one-shot sync has finished. + close(onDone) + + // Wait for the handler goroutine to exit. + select { + case <-handlerExited: + case <-time.After(2 * time.Second): + t.Fatal("noWatch handler goroutine did not finish in time") + } + + // Parent must still be alive: Kill was never called, so other services continue. + assert.Assert(t, parent.Alive(), + "parent tomb was killed on noWatch sync completion; SSH/port-forwarding would be terminated") +} + +// TestNoWatchOnDoneWatchModeKillsParent documents the intentional contrast: +// in watch mode (RestartOnError=true) the onDone path DOES call syncDone → parent.Kill, +// terminating the whole dev session when the sync unexpectedly stops. +func TestNoWatchOnDoneWatchModeKillsParent(t *testing.T) { + var parent tomb.Tomb + onDone := make(chan struct{}) + handlerExited := make(chan struct{}) + + // Simulate the RestartOnError (if) branch: onDone → Kill parent. + parent.Go(func() error { + defer close(handlerExited) + select { + case <-onDone: + parent.Kill(nil) // watch mode: unexpected stop kills everything + } + return nil + }) + + close(onDone) + + select { + case <-handlerExited: + case <-time.After(2 * time.Second): + t.Fatal("watch-mode handler goroutine did not finish in time") + } + + // Parent should be dying/dead because Kill was called. + assert.Assert(t, !parent.Alive(), + "expected parent tomb to be killed in watch mode when sync stops") +}