From 33ecd0d945d253dcb5a516fd7a2e57fadfe27c38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 7 Aug 2026 21:01:47 +0100 Subject: [PATCH] fix(agentctl): log canceled git status refreshes at debug (#2398) --- .../server/process/workspace_git_status.go | 25 ++++++- .../process/workspace_git_status_test.go | 65 +++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status.go b/apps/backend/internal/agentctl/server/process/workspace_git_status.go index 5276113a03..301b8c20ef 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status.go @@ -2,6 +2,7 @@ package process import ( "context" + "errors" "fmt" "path/filepath" "regexp" @@ -36,10 +37,19 @@ func (wt *WorkspaceTracker) updateGitStatus(ctx context.Context) bool { func (wt *WorkspaceTracker) updateGitStatusClass(ctx context.Context, class subproc.GitWorkClass) bool { status, err := wt.getGitStatusClass(ctx, class) if err != nil { - wt.logger.Warn("updateGitStatus: getGitStatus failed", zap.Error(err)) + // A cancellation is the expected outcome when the tracker is being + // torn down (Stop cancels cancelCtx, which kills the in-flight git + // command) or when the poll's admission slot is revoked. That is not a + // failure worth a warning — it mirrors handleGitPollFailure, which also + // downgrades cancellation-class errors to Debug to keep shutdown quiet. + if wt.isGitStatusCancellation(err) { + wt.logger.Debug("updateGitStatus: getGitStatus canceled", zap.Error(err)) + } else { + wt.logger.Warn("updateGitStatus: getGitStatus failed", zap.Error(err)) + } return false } - if ctx.Err() != nil || wt.cancelCtx.Err() != nil { + if ctx.Err() != nil || (wt.cancelCtx != nil && wt.cancelCtx.Err() != nil) { return false } @@ -52,6 +62,17 @@ func (wt *WorkspaceTracker) updateGitStatusClass(ctx context.Context, class subp return true } +// isGitStatusCancellation reports whether a getGitStatus error is a benign +// cancellation rather than a real git failure: the passed context or the +// tracker's own shutdown context was canceled, or the background admission slot +// was revoked. These are all expected during teardown and don't warrant a warn. +func (wt *WorkspaceTracker) isGitStatusCancellation(err error) bool { + if errors.Is(err, context.Canceled) || errors.Is(err, subproc.ErrAdmissionCanceled) { + return true + } + return wt.cancelCtx != nil && wt.cancelCtx.Err() != nil +} + // tryUpdateGitStatus attempts a non-blocking git status update. If another // update is already in progress (from the other polling loop or an explicit // refresh), the call is skipped — the running update will produce the same diff --git a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go index 62856bbb31..2d3ad81854 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go +++ b/apps/backend/internal/agentctl/server/process/workspace_git_status_test.go @@ -2,14 +2,79 @@ package process import ( "context" + "errors" "os" "path/filepath" "testing" "github.com/kandev/kandev/internal/agentctl/types" "github.com/kandev/kandev/internal/common/subproc" + "go.uber.org/zap/zapcore" ) +// TestUpdateGitStatus_CancellationLogsDebugNotWarn verifies that a canceled +// observation (the expected outcome during tracker teardown) is logged at +// Debug, while a genuine failure still warns. This keeps shutdown from +// emitting a burst of "getGitStatus failed" warnings for context.Canceled. +func TestUpdateGitStatus_CancellationLogsDebugNotWarn(t *testing.T) { + tests := []struct { + name string + observerErr error + cancelWT bool + wantWarn bool + }{ + {"context canceled", context.Canceled, false, false}, + {"admission canceled", subproc.ErrAdmissionCanceled, false, false}, + {"tracker context canceled with opaque error", errors.New("exit status 1"), true, false}, + {"real failure", errors.New("git exploded"), false, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + log, observed := newObservedTestLogger(t) + wt := NewWorkspaceTracker(t.TempDir(), log) + t.Cleanup(wt.Stop) + wt.gitStatusObserver = func(context.Context) (types.GitStatusUpdate, error) { + return types.GitStatusUpdate{}, tt.observerErr + } + if tt.cancelWT { + wt.cancelFunc() + } + + if wt.updateGitStatus(context.Background()) { + t.Fatal("updateGitStatus returned true on error") + } + + warns := observed.FilterMessage("updateGitStatus: getGitStatus failed"). + FilterLevelExact(zapcore.WarnLevel).Len() + debugs := observed.FilterMessage("updateGitStatus: getGitStatus canceled"). + FilterLevelExact(zapcore.DebugLevel).Len() + + if tt.wantWarn { + if warns != 1 { + t.Fatalf("warn count = %d, want 1", warns) + } + if debugs != 0 { + t.Fatalf("debug count = %d, want 0", debugs) + } + return + } + if warns != 0 { + t.Fatalf("warn count = %d, want 0 for cancellation", warns) + } + if debugs != 1 { + t.Fatalf("debug count = %d, want 1 for cancellation", debugs) + } + }) + } +} + +func TestWorkspaceTrackerIsGitStatusCancellation_NilCancelCtx(t *testing.T) { + wt := &WorkspaceTracker{} + if wt.isGitStatusCancellation(errors.New("git exploded")) { + t.Fatal("isGitStatusCancellation returned true with nil cancelCtx") + } +} + func TestUnquoteGitPath(t *testing.T) { tests := []struct { name string