From 8adcd85b98e5e0c5dcaf5c352c4dd2ea322e026a Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:49:47 -0700 Subject: [PATCH 1/2] Ignore context-canceled errors when logging drift detector shutdown failures Motivation: When a piped agent shuts down, its drift-detector context is canceled while a check cycle may still be in flight. This causes in-flight operations to fail (e.g. kustomize being killed with "signal: killed", or a git command returning "context canceled"), and those expected, benign failures were logged at Error level as "failed to check application: ..." and "failed to clean partially cloned repository", producing log spam / false alarms at shutdown time. Approach: In the kubernetes drift detector's check method, both the checkApplication call and the gitRepo.CleanPath cleanup call still run unconditionally as before, but the resulting error is now only logged at Error level when ctx.Err() == nil. ctx.Err() is checked directly (rather than errors.Is(err, context.Canceled)) because a process killed via exec.CommandContext on context cancellation (e.g. kustomize) returns "signal: killed", which does not wrap context.Canceled in Go's os/exec implementation, so checking the context directly reliably covers both error shapes reported in the issue. Validation: Added TestCheck_ContextCanceled in detector_test.go, which drives detector.check with a mocked git.Repo whose CleanPath call fails and whose repository path lacks an application config file (making checkApplication fail quickly, independent of context state). With a non-canceled context both failures are logged as Error (2 entries); with an already-canceled context, zero Error logs are produced, while the mocked CleanPath/Pull/GetLatestCommit calls are still asserted to run. Verified this test fails (2 unwanted Error logs) against the pre-fix code and passes after the fix. Ran and confirmed passing: - go test ./pkg/app/piped/driftdetector/kubernetes/... -run TestCheck_ContextCanceled -v - go test ./pkg/app/piped/driftdetector/... -v - go build ./pkg/app/piped/driftdetector/... - go vet ./pkg/app/piped/driftdetector/... - golangci-lint run --config .golangci.yml ./pkg/app/piped/driftdetector/... This is a logging-only behavior change: cleanup and manifest-check operations still execute exactly as before, only the two spurious Error-level log lines during graceful shutdown are removed. Scope is limited to the kubernetes drift detector, the exact file the issue and a maintainer's comment point to, and the only drift detector with the CleanPath cleanup call. The terraform/ecs/lambda/cloudrun detectors have a similar checkApplication error-log line but weren't reported in this issue and are left untouched. Report: https://github.com/pipe-cd/pipecd/issues/5338 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- .../driftdetector/kubernetes/detector.go | 20 ++- .../driftdetector/kubernetes/detector_test.go | 137 ++++++++++++++++++ 2 files changed, 151 insertions(+), 6 deletions(-) create mode 100644 pkg/app/piped/driftdetector/kubernetes/detector_test.go diff --git a/pkg/app/piped/driftdetector/kubernetes/detector.go b/pkg/app/piped/driftdetector/kubernetes/detector.go index b8ebd7c8d4..5906b67649 100644 --- a/pkg/app/piped/driftdetector/kubernetes/detector.go +++ b/pkg/app/piped/driftdetector/kubernetes/detector.go @@ -167,7 +167,11 @@ func (d *detector) check(ctx context.Context) { // Start checking all applications in this repository. for _, app := range apps { if err := d.checkApplication(ctx, app, gitRepo, headCommit); err != nil { - d.logger.Error(fmt.Sprintf("failed to check application: %s", app.Id), zap.Error(err)) + // Ignore the error caused by the context being canceled (e.g. piped is + // shutting down); it's an expected cancellation, not a real failure. + if ctx.Err() == nil { + d.logger.Error(fmt.Sprintf("failed to check application: %s", app.Id), zap.Error(err)) + } } // Reset the app dir to the head commit. @@ -180,11 +184,15 @@ func (d *detector) check(ctx context.Context) { zap.String("app-path", app.GitPath.Path), ) if err := gitRepo.CleanPath(ctx, app.GitPath.Path); err != nil { - d.logger.Error("failed to clean partially cloned repository", - zap.String("repo-id", repoID), - zap.String("app-id", app.Id), - zap.String("app-path", app.GitPath.Path), - zap.Error(err)) + // This clean is only partial; the entire cleanup is performed elsewhere, + // so ignore the error when it's caused by the context being canceled. + if ctx.Err() == nil { + d.logger.Error("failed to clean partially cloned repository", + zap.String("repo-id", repoID), + zap.String("app-id", app.Id), + zap.String("app-path", app.GitPath.Path), + zap.Error(err)) + } } } } diff --git a/pkg/app/piped/driftdetector/kubernetes/detector_test.go b/pkg/app/piped/driftdetector/kubernetes/detector_test.go new file mode 100644 index 0000000000..fee0072781 --- /dev/null +++ b/pkg/app/piped/driftdetector/kubernetes/detector_test.go @@ -0,0 +1,137 @@ +// Copyright 2026 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kubernetes + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/pipe-cd/pipecd/pkg/app/piped/livestatestore/kubernetes" + provider "github.com/pipe-cd/pipecd/pkg/app/piped/platformprovider/kubernetes" + "github.com/pipe-cd/pipecd/pkg/cache/memorycache" + "github.com/pipe-cd/pipecd/pkg/config" + "github.com/pipe-cd/pipecd/pkg/git" + "github.com/pipe-cd/pipecd/pkg/git/gittest" + "github.com/pipe-cd/pipecd/pkg/model" +) + +type fakeAppLister struct { + apps []*model.Application +} + +func (f *fakeAppLister) ListByPlatformProvider(name string) []*model.Application { + return f.apps +} + +type fakeStateGetter struct{} + +func (fakeStateGetter) GetKubernetesAppLiveState(appID string) (kubernetes.AppState, bool) { + return kubernetes.AppState{}, false +} + +func (fakeStateGetter) NewEventIterator() kubernetes.EventIterator { + return kubernetes.EventIterator{} +} + +func (fakeStateGetter) GetWatchingResourceKinds() []provider.APIVersionKind { + return nil +} + +func (fakeStateGetter) GetAppLiveManifests(appID string) []provider.Manifest { + return nil +} + +func (fakeStateGetter) WaitForReady(ctx context.Context, timeout time.Duration) error { + return nil +} + +// TestCheck_ContextCanceled verifies that check() does not log the per-application +// failures as errors when the given context has already been canceled, e.g. during +// piped shutdown, while it still logs them as errors otherwise. +func TestCheck_ContextCanceled(t *testing.T) { + testcases := []struct { + name string + cancel bool + wantErrorLogs int + }{ + { + name: "context not canceled: failures are logged as errors", + cancel: false, + wantErrorLogs: 2, + }, + { + name: "context canceled: failures are not logged as errors", + cancel: true, + wantErrorLogs: 0, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + + core, logs := observer.New(zapcore.DebugLevel) + logger := zap.New(core) + + repo := gittest.NewMockRepo(ctrl) + repo.EXPECT().GetClonedBranch().Return("main").AnyTimes() + repo.EXPECT().Pull(gomock.Any(), gomock.Any()).Return(nil) + repo.EXPECT().GetLatestCommit(gomock.Any()).Return(git.Commit{Hash: "abc123"}, nil) + // Point GetPath to an empty directory so loading the application + // configuration fails quickly without touching any real repository. + repo.EXPECT().GetPath().Return(t.TempDir()).AnyTimes() + repo.EXPECT().CleanPath(gomock.Any(), gomock.Any()).Return(errors.New("clean failed")) + + app := &model.Application{ + Id: "app-1", + Kind: model.ApplicationKind_KUBERNETES, + GitPath: &model.ApplicationGitPath{ + Repo: &model.ApplicationGitRepository{Id: "repo-1"}, + Path: "path/to/app", + }, + } + + d := &detector{ + provider: config.PipedPlatformProvider{Name: "kubernetes-default"}, + appLister: &fakeAppLister{apps: []*model.Application{app}}, + stateGetter: fakeStateGetter{}, + appManifestsCache: memorycache.NewCache(), + config: &config.PipedSpec{}, + logger: logger, + gitRepos: map[string]git.Repo{"repo-1": repo}, + syncStates: make(map[string]model.ApplicationSyncState), + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if tc.cancel { + cancel() + } + + d.check(ctx) + + errorLogs := logs.FilterLevelExact(zapcore.ErrorLevel).All() + assert.Len(t, errorLogs, tc.wantErrorLogs) + }) + } +} From 0abd2e4db15b6dd8e1a2ac5682eda50e00278f6c Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 6 Sep 2026 14:32:13 -0700 Subject: [PATCH 2/2] Address review feedback on context-canceled log suppression Rewords the code comments so they no longer imply an error was caused by context cancellation when only ctx.Err() is checked, and wires a no-op fake reporter into TestCheck_ContextCanceled so the test doesn't rely on checkApplication failing early to avoid a nil reporter. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- pkg/app/piped/driftdetector/kubernetes/detector.go | 9 ++++++--- pkg/app/piped/driftdetector/kubernetes/detector_test.go | 7 +++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/app/piped/driftdetector/kubernetes/detector.go b/pkg/app/piped/driftdetector/kubernetes/detector.go index 5906b67649..8e18e5be24 100644 --- a/pkg/app/piped/driftdetector/kubernetes/detector.go +++ b/pkg/app/piped/driftdetector/kubernetes/detector.go @@ -167,8 +167,9 @@ func (d *detector) check(ctx context.Context) { // Start checking all applications in this repository. for _, app := range apps { if err := d.checkApplication(ctx, app, gitRepo, headCommit); err != nil { - // Ignore the error caused by the context being canceled (e.g. piped is - // shutting down); it's an expected cancellation, not a real failure. + // Suppress this error when the context is already canceled (e.g. piped + // is shutting down), since it's expected in that case regardless of + // whether it actually stems from the cancellation. if ctx.Err() == nil { d.logger.Error(fmt.Sprintf("failed to check application: %s", app.Id), zap.Error(err)) } @@ -185,7 +186,9 @@ func (d *detector) check(ctx context.Context) { ) if err := gitRepo.CleanPath(ctx, app.GitPath.Path); err != nil { // This clean is only partial; the entire cleanup is performed elsewhere, - // so ignore the error when it's caused by the context being canceled. + // so suppress the error when the context is already canceled (e.g. piped + // is shutting down), since it's expected in that case regardless of + // whether it actually stems from the cancellation. if ctx.Err() == nil { d.logger.Error("failed to clean partially cloned repository", zap.String("repo-id", repoID), diff --git a/pkg/app/piped/driftdetector/kubernetes/detector_test.go b/pkg/app/piped/driftdetector/kubernetes/detector_test.go index fee0072781..55ff83264a 100644 --- a/pkg/app/piped/driftdetector/kubernetes/detector_test.go +++ b/pkg/app/piped/driftdetector/kubernetes/detector_test.go @@ -65,6 +65,12 @@ func (fakeStateGetter) WaitForReady(ctx context.Context, timeout time.Duration) return nil } +type fakeReporter struct{} + +func (fakeReporter) ReportApplicationSyncState(ctx context.Context, appID string, state *model.ApplicationSyncState) error { + return nil +} + // TestCheck_ContextCanceled verifies that check() does not log the per-application // failures as errors when the given context has already been canceled, e.g. during // piped shutdown, while it still logs them as errors otherwise. @@ -115,6 +121,7 @@ func TestCheck_ContextCanceled(t *testing.T) { provider: config.PipedPlatformProvider{Name: "kubernetes-default"}, appLister: &fakeAppLister{apps: []*model.Application{app}}, stateGetter: fakeStateGetter{}, + reporter: fakeReporter{}, appManifestsCache: memorycache.NewCache(), config: &config.PipedSpec{}, logger: logger,