Skip to content

Commit d2165ac

Browse files
authored
fix(capture): Fix Windows capture stop ctx. (#2385)
# Description Fix Windows capture ctx. Stop Network capture parent ctx is expired by the time `stopNetworkCapture` is called. Please provide a brief description of the changes made in this pull request. ## Related Issue #2298 ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/Contributing/overview). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. ## Screenshots (if applicable) or Testing Completed <img width="1247" height="692" alt="image" src="https://github.com/user-attachments/assets/751f4df6-08c8-46ac-b169-df3094994642" /> <img width="1885" height="407" alt="image" src="https://github.com/user-attachments/assets/3470c845-0364-4bc5-b121-3abd720009b4" /> ## Additional Notes Issue introduced: #2322 https://github.com/microsoft/retina/pull/2322/changes#diff-bd1c3510276019b2f8f85384d4963e4fa366a428acad36280432a841079a9bf2 --------- Signed-off-by: mereta <mereta.degutyte@hotmail.co.uk>
1 parent 27f1a7f commit d2165ac

2 files changed

Lines changed: 50 additions & 4 deletions

File tree

pkg/capture/provider/network_capture_win.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func (ncp *NetworkCaptureProvider) CaptureNetworkPacket(ctx context.Context, fil
8484
}
8585
if stopTrace {
8686
ncp.l.Info("Stopping netsh trace session before starting a new one")
87-
_ = ncp.stopNetworkCapture(ctx)
87+
_ = ncp.stopNetworkCapture() //nolint:contextcheck // stopNetworkCapture creates its own context
8888
}
8989

9090
captureFileName := ncp.Filename.String() + ".etl"
@@ -164,7 +164,8 @@ func (ncp *NetworkCaptureProvider) CaptureNetworkPacket(ctx context.Context, fil
164164
}
165165

166166
ncp.l.Info("Stop netsh")
167-
if err := ncp.stopNetworkCapture(ctx); err != nil {
167+
// stopNetworkCapture creates its own context; parent ctx is expired here
168+
if err := ncp.stopNetworkCapture(); err != nil { //nolint:contextcheck // stopNetworkCapture creates its own context
168169
ncp.l.Error("Failed to stop netsh trace by 'netsh trace stop', will kill the process", zap.Error(err))
169170
_ = captureStartCmd.Process.Kill()
170171
return fmt.Errorf("netsh stop failed: Output: %s", err)
@@ -204,10 +205,15 @@ func (ncp *NetworkCaptureProvider) needToStopTraceSession(ctx context.Context) (
204205
return false, fmt.Errorf("cannot stop trace session because it's not created by Retina capture")
205206
}
206207

207-
func (ncp *NetworkCaptureProvider) stopNetworkCapture(ctx context.Context) error {
208+
func (ncp *NetworkCaptureProvider) stopNetworkCapture() error {
208209
ncp.l.Info("Stopping netsh trace session")
209210

210-
command := exec.CommandContext(ctx, "netsh", "trace", "stop")
211+
// Create independent context for cleanup.
212+
// netsh trace stop completes in ~1s; 30s timeout provides ample safety margin.
213+
stopCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
214+
defer cancel()
215+
216+
command := exec.CommandContext(stopCtx, "netsh", "trace", "stop")
211217
output, err := command.CombinedOutput()
212218
// ignore the error when stop the trace when no live trace session exists.
213219
if strings.Contains(string(output), "There is no trace session currently in progress") {

pkg/capture/provider/network_capture_win_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,13 @@
66
package provider
77

88
import (
9+
"context"
910
"testing"
11+
"time"
12+
13+
"github.com/microsoft/retina/pkg/capture/file"
14+
"github.com/microsoft/retina/pkg/log"
15+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1016
)
1117

1218
func TestValidateNetshFilter(t *testing.T) {
@@ -114,3 +120,37 @@ func TestValidateNetshFilter(t *testing.T) {
114120
})
115121
}
116122
}
123+
124+
// TestStopNetworkCapture_ContextIndependence verifies stopNetworkCapture creates its own context
125+
func TestStopNetworkCapture_ContextIndependence(t *testing.T) {
126+
now := metav1.Now()
127+
ncp := &NetworkCaptureProvider{
128+
NetworkCaptureProviderCommon: NetworkCaptureProviderCommon{
129+
TmpCaptureDir: t.TempDir(),
130+
l: log.Logger().Named("test-capture"),
131+
},
132+
Filename: file.CaptureFilename{
133+
CaptureName: "test-capture",
134+
NodeHostname: "test-node",
135+
StartTimestamp: &now,
136+
},
137+
}
138+
139+
// Create an expired context (simulating capture duration ending)
140+
parentCtx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond)
141+
time.Sleep(10 * time.Millisecond)
142+
defer cancel()
143+
144+
if parentCtx.Err() == nil {
145+
t.Fatal("Setup error: parent context should be expired")
146+
}
147+
148+
// Call StopNetworkCapture - should NOT return "context deadline exceeded"
149+
err := ncp.stopNetworkCapture()
150+
151+
if err != nil && err.Error() == "context deadline exceeded" {
152+
t.Fatal("StopNetworkCapture returned 'context deadline exceeded' - bug reintroduced")
153+
}
154+
155+
t.Logf("StopNetworkCapture uses independent context (netsh error expected: %v)", err)
156+
}

0 commit comments

Comments
 (0)