[client, proxy] Add packet capture to debug bundle and CLI#5891
Open
[client, proxy] Add packet capture to debug bundle and CLI#5891
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an end-to-end packet-capture feature: new capture package (filtering, session, pcap/text writers, AF_PACKET), plumbing into engine/device/forwarder, daemon RPC/HTTP and CLI/WASM controls to start/stop captures, debug-bundle capture support, and container env vars defaulting capture off. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(240,240,255,0.5)
actor CLI
participant Daemon as Daemon Server
participant Engine as Engine
participant Session as Capture Session
participant Sink as Writer (pcap/text)
end
CLI->>Daemon: StartCapture(options+filter)
Daemon->>Engine: SetCapture(session)
Engine->>Session: register session
Daemon-->>CLI: initial ack packet
Engine->>Session: Offer(packet, outbound)
Session->>Session: apply matcher, enqueue
Session->>Sink: write packet (pcap/text)
CLI->>Daemon: cancel / disconnect
Daemon->>Engine: SetCapture(nil)
Session->>Session: Stop() -> flush remaining -> update stats
Daemon-->>CLI: stream closed
sequenceDiagram
rect rgba(240,255,240,0.5)
actor UI
participant Daemon as Daemon Server
participant Engine as Engine
participant Session as Capture Session
participant File as Temp PCAP File
end
UI->>Daemon: StartBundleCapture(timeout)
Daemon->>File: create temp pcap file
Daemon->>Session: NewSession(write->file)
Daemon->>Engine: SetCapture(session)
Engine->>Session: Offer(packets...)
Session->>File: WritePacket (pcap)
Note over Daemon,Session: timeout or StopBundleCapture triggers cleanup
UI->>Daemon: StopBundleCapture
Daemon->>Engine: SetCapture(nil)
Session->>Daemon: Stop() and finalize file
Daemon->>UI: include temp file in debug bundle
Daemon->>File: remove temp file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/firewall/uspfilter/forwarder/icmp.go (1)
267-275:⚠️ Potential issue | 🟠 MajorOffer the ICMP reply to capture only after the peer send succeeds.
Right now the capture hook records
fullPacketbeforeCreateOutboundPacketruns. If the peer send fails, the PCAP still shows an outbound ICMP reply that never actually left the forwarder, which makes transport debugging misleading.Suggested reorder
- if pc := f.endpoint.capture.Load(); pc != nil { - (*pc).Offer(fullPacket, true) - } - // Bypass netstack and send directly to peer to avoid looping through our ICMP handler if err := f.endpoint.device.CreateOutboundPacket(fullPacket, id.RemoteAddress.AsSlice()); err != nil { f.logger.Error1("forwarder: Failed to send ICMP reply to peer: %v", err) return 0 } + + if pc := f.endpoint.capture.Load(); pc != nil { + (*pc).Offer(fullPacket, true) + } return len(fullPacket) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/icmp.go` around lines 267 - 275, The capture hook currently records the ICMP reply before sending to the peer, causing false-positive PCAP entries if send fails; change the order so that f.endpoint.device.CreateOutboundPacket(fullPacket, id.RemoteAddress.AsSlice()) is called first and only if it returns nil then call the capture Offer via f.endpoint.capture.Load() -> (*pc).Offer(fullPacket, true); keep the existing error handling using f.logger.Error1 to return on send failure so failed sends are not captured.client/Dockerfile (1)
16-21:⚠️ Potential issue | 🟠 MajorKeep packet capture opt-in in the shipped container image.
Setting
NB_ENABLE_CAPTURE="true"here enables decrypted-traffic capture for every container deployment by default. That is a meaningful expansion of the debug surface for a production image; safer is to leave it off by default and document opting in when troubleshooting. The same concern applies toclient/Dockerfile-rootless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/Dockerfile` around lines 16 - 21, The Dockerfile currently enables decrypted-traffic capture by default via NB_ENABLE_CAPTURE="true"; change the default to NB_ENABLE_CAPTURE="false" in client/Dockerfile and repeat the same change in client/Dockerfile-rootless so packet capture is opt-in, leaving the other ENV settings (NETBIRD_BIN, NB_LOG_FILE, NB_DAEMON_ADDR, NB_ENTRYPOINT_SERVICE_TIMEOUT) unchanged and update any README or runtime docs to document how to opt-in when troubleshooting.client/ui/debug.go (1)
229-236:⚠️ Potential issue | 🟠 MajorPass
params.capturethrough the direct bundle path.When "Run with trace logs before creating bundle" is unchecked, this branch bypasses
collectDebugData, so the new checkbox becomes a no-op and the generated bundle never includes a packet capture.handleDebugCreationneeds the full params object and should wrap bundle creation withStartBundleCapture/StopBundleCapturewhenparams.captureis enabled.Also applies to: 262-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` around lines 229 - 236, The direct bundle creation branch currently builds a minimal params object and skips capture handling, so ensure you pass the full debugCollectionParams (including params.capture) into the direct-bundle path and into handleDebugCreation instead of a reduced struct; if params.capture is true, wrap the bundle creation logic with StartBundleCapture(...) and StopBundleCapture(...) calls around the bundle creation (same pattern as when collectDebugData is used) so packet captures are included in the generated bundle; apply the same change for the other similar branch mentioned (the branch referenced around the second occurrence).
🧹 Nitpick comments (5)
util/capture/afpacket_stub.go (1)
18-22: Consider adding inline comments to satisfy static analysis.SonarCloud flags the empty function bodies. While the docstrings explain the intent, adding brief inline comments would silence the warnings and be more explicit:
💡 Optional: Add inline comments
// Stop is a no-op on non-Linux platforms. -func (c *AFPacketCapture) Stop() {} +func (c *AFPacketCapture) Stop() { + // No-op: AF_PACKET is Linux-only +} // Offer is a no-op on non-Linux platforms. -func (c *AFPacketCapture) Offer([]byte, bool) {} +func (c *AFPacketCapture) Offer([]byte, bool) { + // No-op: AF_PACKET is Linux-only +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/afpacket_stub.go` around lines 18 - 22, The empty implementations of AFPacketCapture.Stop and AFPacketCapture.Offer are intentional no-ops on non-Linux, but add a short inline comment inside each function body (e.g., "// no-op on non-Linux; stubbed for build compatibility" or similar) to satisfy static analysis and make the intent explicit; update the bodies of the methods Stop and Offer on type AFPacketCapture to contain those concise comments while leaving behavior unchanged.util/capture/session.go (1)
198-201: Add comment explaining intentionally empty no-op flush function.The static analysis tool flags this empty function. Adding a brief comment clarifies intent.
📝 Suggested fix
switch len(fns) { case 0: - return func() {} + return func() { /* no flushable writers */ } case 1:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/session.go` around lines 198 - 201, Add a brief comment clarifying that the empty returned closure is an intentional no-op flush handler to satisfy the API when no flush functions are provided: annotate the case 0 branch in the switch inside util/capture/session.go (the return func() {}) with a one-line comment like "no-op flush: nothing to flush when no fns are provided" so static analysis understands it's deliberate.util/capture/afpacket_linux.go (1)
163-166: Add explanatory comment for intentionally emptyOffermethod.The static analysis tool flags this empty function. Since
AFPacketCapturedrives its own read loop rather than being called externally, the comment in the docstring is good, but adding an inline comment satisfies the linter and makes the intent clearer to future readers.📝 Suggested fix
// Offer satisfies device.PacketCapture but is unused: the AFPacketCapture // drives its own read loop. This exists only so the type signature is // compatible if someone tries to set it as a PacketCapture. -func (c *AFPacketCapture) Offer([]byte, bool) {} +func (c *AFPacketCapture) Offer([]byte, bool) { + // No-op: AFPacketCapture uses its own readLoop instead of being called externally. +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/afpacket_linux.go` around lines 163 - 166, The empty Offer method on type AFPacketCapture is intentional but needs an inline comment to satisfy the linter; update the Offer([]byte, bool) method implementation on AFPacketCapture to include a short inline comment (e.g., "// intentionally no-op: AFPacketCapture drives its own read loop so Offer is unused") explaining why the body is empty and referencing that AFPacketCapture has its own read loop so the method is a no-op to satisfy the device.PacketCapture signature.client/server/capture.go (1)
162-173: Consider handling stream.Send errors more gracefully.When
stream.Sendfails at line 166, the function returnsnilrather than propagating the error. While this avoids logging errors for normal client disconnections, it also silently swallows unexpected errors. Consider logging at debug level for observability.📝 Suggested improvement
buf := make([]byte, 32*1024) for { n, readErr := pr.Read(buf) if n > 0 { if err := stream.Send(&proto.CapturePacket{Data: buf[:n]}); err != nil { + log.Debugf("capture stream send: %v", err) return nil } } if readErr != nil { return nil } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 162 - 173, The loop reading from pr and sending via stream.Send currently returns nil on send/read errors, which silently swallows unexpected failures; update the handler in the function (the goroutine using pr.Read and stream.Send with proto.CapturePacket) to check the error from stream.Send and either return that error (or log it at debug level before returning) instead of returning nil, and likewise distinguish EOF/normal client disconnects from other errors so you only log unexpected errors at debug and propagate them by returning the actual error value.util/capture/text.go (1)
15-22: Consider bounded flow map to prevent memory growth.The
flowsmap tracks TCP initial sequence numbers per direction but has no eviction policy. For long-running captures with many distinct connections, this could grow unbounded. Consider adding a maximum size with LRU eviction or time-based expiry.For a debug capture tool, this may be acceptable if captures are typically short-lived, but worth noting for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/text.go` around lines 15 - 22, The flows map on TextWriter (field flows, keyed by dirKey) is unbounded and may grow indefinitely; update TextWriter to use a bounded cache with eviction (e.g., an LRU or time-expiring store) for tracking initial sequence numbers: replace or wrap flows with a size-limited structure (or store timestamps alongside sequence numbers and run eviction when inserting) and enforce a configurable maxEntries; adjust methods that read/write flows (references to TextWriter.flows and dirKey) to use the new cache API so entries are evicted when capacity or age limits are exceeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/capture.go`:
- Around line 103-114: The code creates the output file and defers the cleanup
(which logs "Wrote <path>") before the first stream.Recv() acceptance check, so
a rejected capture leaves an empty file with a false success message; to fix,
delay creating/logging the output until after the server acceptance: either call
captureOutput() only after the first successful stream.Recv() (or modify
captureOutput/its cleanup to accept a "committed" flag and only log "Wrote
<path>" when the stream was accepted). Update the call sites around
captureOutput, stream.Recv, handleCaptureError and the deferred cleanup that
emits "Wrote <path>" (also fix the similar pattern at the second occurrence
around lines 152-157) so cleanup/logging happens only after a successful Recv.
In `@client/firewall/uspfilter/filter.go`:
- Around line 356-359: SetPacketCapture currently only applies pc if
m.forwarder.Load() is non-nil, so captures requested before initForwarder() are
lost; persist the desired sink on Manager and apply it when a forwarder is
created. Add a Manager field (e.g., packetCapture forwarder.PacketCapture) or an
atomic.Value to store the most recent capture sink; in SetPacketCapture always
save pc to that field and then, if fwder := m.forwarder.Load(); fwder != nil
call fwder.SetCapture(pc). In initForwarder (or wherever a new forwarder is
created/installed), after creating the forwarder call
forwarder.SetCapture(m.packetCapture) (or load the atomic.Value) so the
persisted capture is applied to newly-initialized forwarders; ensure any access
is concurrency-safe (mutex or atomic.Value) as used elsewhere in Manager.
In `@proxy/cmd/proxy/cmd/debug.go`:
- Line 138: The help text for the duration flag
(debugCaptureCmd.Flags().DurationP("duration", "d", 0, ...)) is wrong:
duration==0 currently omits the query parameter and the server uses its
30-minute default, so update the flag usage/help to accurately describe the
behavior (e.g., "Capture duration (0 = use server default 30m; max 30m)") and
adjust any other identical help strings (the instances referenced around lines
237-245) to match; alternatively, if you want true "until interrupted"
semantics, implement explicit handling in the capture command to interpret a
sentinel value (e.g., -1) and send an explicit parameter or loop until SIGINT,
but at minimum change the help text to reflect the current behavior and mention
the server default of 30 minutes.
In `@proxy/internal/debug/client.go`:
- Around line 327-374: The Capture method should validate required inputs before
making the HTTP request: check that opts.Output is non-nil and opts.AccountID is
non-empty (or otherwise valid) and return a descriptive error if not; update
Client.Capture (and reference CaptureOptions) to perform these guards at the top
of the function so the method never panics on io.Copy, and preserve the existing
streaming/error behavior after the validation.
In `@proxy/internal/debug/handler.go`:
- Around line 684-699: The Flush call currently happens before setting up the
capture and can commit a 200 response prematurely; move the http.Flusher check
and f.Flush() so it runs only after capture.NewSession succeeds and
client.SetCapture(sess) returns nil (i.e., after sess is created, SetCapture
succeeds, and you have deferred sess.Stop()). Locate the http.Flusher usage on
the response writer (w) and the capture.NewSession / client.SetCapture calls,
remove the early Flush, and perform the Flush (or call f.Flush()) after
successful SetCapture (ensuring the Flusher type assertion is re-checked there)
so that http.Error can still send proper 5xx responses on failure.
In `@util/capture/pcap.go`:
- Around line 18-21: The PcapWriter currently only writes the PCAP global header
inside WritePacket, causing zero-byte output if no packets are written; add a
public Close (or Flush) method on PcapWriter that ensures the global header is
emitted even if no packets matched by calling the same internal routine that
WritePacket uses to lazily write the header (extract that logic into a helper
like writeGlobalHeaderIfNeeded used by both WritePacket and Close), implement
io.Closer on the type, and ensure Close is idempotent so repeated Close/Flush
calls are safe; update any callers/tests that should close the writer so empty
captures produce a valid pcap file.
In `@util/capture/session_test.go`:
- Around line 27-31: The tests use fixed time.Sleep(50 * time.Millisecond) which
makes them flaky; replace those sleeps with deterministic synchronization by
waiting for the session's lifecycle/writer completion signal instead (e.g. use a
sess.Done() / sess.Stopped channel or a sync.WaitGroup that the session writer
closes/done-ops on) after calling sess.Offer and before asserting, and ensure
sess.Stop() is still called; update all occurrences (the sleeps after sess.Offer
and the later sleeps) to block on that explicit done signal so the writer
goroutine has completed deterministically.
---
Outside diff comments:
In `@client/Dockerfile`:
- Around line 16-21: The Dockerfile currently enables decrypted-traffic capture
by default via NB_ENABLE_CAPTURE="true"; change the default to
NB_ENABLE_CAPTURE="false" in client/Dockerfile and repeat the same change in
client/Dockerfile-rootless so packet capture is opt-in, leaving the other ENV
settings (NETBIRD_BIN, NB_LOG_FILE, NB_DAEMON_ADDR,
NB_ENTRYPOINT_SERVICE_TIMEOUT) unchanged and update any README or runtime docs
to document how to opt-in when troubleshooting.
In `@client/firewall/uspfilter/forwarder/icmp.go`:
- Around line 267-275: The capture hook currently records the ICMP reply before
sending to the peer, causing false-positive PCAP entries if send fails; change
the order so that f.endpoint.device.CreateOutboundPacket(fullPacket,
id.RemoteAddress.AsSlice()) is called first and only if it returns nil then call
the capture Offer via f.endpoint.capture.Load() -> (*pc).Offer(fullPacket,
true); keep the existing error handling using f.logger.Error1 to return on send
failure so failed sends are not captured.
In `@client/ui/debug.go`:
- Around line 229-236: The direct bundle creation branch currently builds a
minimal params object and skips capture handling, so ensure you pass the full
debugCollectionParams (including params.capture) into the direct-bundle path and
into handleDebugCreation instead of a reduced struct; if params.capture is true,
wrap the bundle creation logic with StartBundleCapture(...) and
StopBundleCapture(...) calls around the bundle creation (same pattern as when
collectDebugData is used) so packet captures are included in the generated
bundle; apply the same change for the other similar branch mentioned (the branch
referenced around the second occurrence).
---
Nitpick comments:
In `@client/server/capture.go`:
- Around line 162-173: The loop reading from pr and sending via stream.Send
currently returns nil on send/read errors, which silently swallows unexpected
failures; update the handler in the function (the goroutine using pr.Read and
stream.Send with proto.CapturePacket) to check the error from stream.Send and
either return that error (or log it at debug level before returning) instead of
returning nil, and likewise distinguish EOF/normal client disconnects from other
errors so you only log unexpected errors at debug and propagate them by
returning the actual error value.
In `@util/capture/afpacket_linux.go`:
- Around line 163-166: The empty Offer method on type AFPacketCapture is
intentional but needs an inline comment to satisfy the linter; update the
Offer([]byte, bool) method implementation on AFPacketCapture to include a short
inline comment (e.g., "// intentionally no-op: AFPacketCapture drives its own
read loop so Offer is unused") explaining why the body is empty and referencing
that AFPacketCapture has its own read loop so the method is a no-op to satisfy
the device.PacketCapture signature.
In `@util/capture/afpacket_stub.go`:
- Around line 18-22: The empty implementations of AFPacketCapture.Stop and
AFPacketCapture.Offer are intentional no-ops on non-Linux, but add a short
inline comment inside each function body (e.g., "// no-op on non-Linux; stubbed
for build compatibility" or similar) to satisfy static analysis and make the
intent explicit; update the bodies of the methods Stop and Offer on type
AFPacketCapture to contain those concise comments while leaving behavior
unchanged.
In `@util/capture/session.go`:
- Around line 198-201: Add a brief comment clarifying that the empty returned
closure is an intentional no-op flush handler to satisfy the API when no flush
functions are provided: annotate the case 0 branch in the switch inside
util/capture/session.go (the return func() {}) with a one-line comment like
"no-op flush: nothing to flush when no fns are provided" so static analysis
understands it's deliberate.
In `@util/capture/text.go`:
- Around line 15-22: The flows map on TextWriter (field flows, keyed by dirKey)
is unbounded and may grow indefinitely; update TextWriter to use a bounded cache
with eviction (e.g., an LRU or time-expiring store) for tracking initial
sequence numbers: replace or wrap flows with a size-limited structure (or store
timestamps alongside sequence numbers and run eviction when inserting) and
enforce a configurable maxEntries; adjust methods that read/write flows
(references to TextWriter.flows and dirKey) to use the new cache API so entries
are evicted when capacity or age limits are exceeded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98db96dc-9752-4e9e-9fcd-5c7c0469c9db
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
lixmal
force-pushed
the
add-packet-capture
branch
2 times, most recently
from
e7ba72b to
a806591
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ui/debug.go (1)
237-246:⚠️ Potential issue | 🟠 MajorDon't leave
Include packet captureas a silent no-op in the non-trace path.When
runForDurationCheckis off, this branch skips bundle capture entirely and also dropscaptureCheck.Checked. The checkbox stays visible, so users can request a capture and still get a bundle without one. Either disable/hide the checkbox when trace collection is off, or honor it here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` around lines 237 - 246, The UI branch that starts the non-trace debug bundle (where statusLabel is set and s.handleDebugCreation is invoked) currently ignores captureCheck.Checked when runForDurationCheck is off, causing the visible "Include packet capture" checkbox to be a silent no-op; either honor the checkbox by passing captureCheck.Checked into s.handleDebugCreation (add a capture boolean parameter and thread it through the handler) or disable/hide captureCheck when runForDurationCheck is false so users cannot request a capture they won't get; update the call site and s.handleDebugCreation signature (and any downstream methods) to accept and act on the capture flag, or toggle captureCheck.Visible/Enabled alongside runForDurationCheck state.
♻️ Duplicate comments (3)
proxy/internal/debug/handler.go (1)
684-699:⚠️ Potential issue | 🟠 MajorDefer
Flush()until capture setup succeeds.The
Flush()call commits the 200 response beforecapture.NewSessionandclient.SetCapturecan fail. If either returns an error,http.Errorcan no longer send the intended status code because headers are already written.Suggested fix: move Flush after successful setup
- // Flush headers so the client sees data immediately. - if f, ok := w.(http.Flusher); ok { - f.Flush() - } - sess, err := capture.NewSession(opts) if err != nil { http.Error(w, "create capture session: "+err.Error(), http.StatusInternalServerError) return } defer sess.Stop() if err := client.SetCapture(sess); err != nil { http.Error(w, "set capture: "+err.Error(), http.StatusServiceUnavailable) return } defer client.SetCapture(nil) //nolint:errcheck + // Flush headers only after the stream is fully initialized. + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + timer := time.NewTimer(duration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/debug/handler.go` around lines 684 - 699, The Flush() call is happening before capture.NewSession and client.SetCapture succeed, which commits the 200 response prematurely; move the Flush so it runs only after sess is created and SetCapture returns nil (i.e., after capture.NewSession(...) succeeds and after client.SetCapture(sess) succeeds). Concretely, remove the early Flush block that checks "if f, ok := w.(http.Flusher); ok { f.Flush() }" and instead perform that same Flusher check and call immediately after the SetCapture(sess) success path (after defer sess.Stop() and after the SetCapture error check), so headers are only flushed when the capture session and client setup have succeeded.proxy/cmd/proxy/cmd/debug.go (1)
138-138:⚠️ Potential issue | 🟡 MinorDuration flag help text is inaccurate.
The help text states
"0 = until interrupted, max 5m"but:
- The server's
maxCaptureDurationis 30 minutes, not 5 minutes- When
duration=0, the query parameter is omitted and the server falls back to its 30-minute default—not "until interrupted"Suggested fix
- debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = until interrupted, max 5m)") + debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = server default 30m, max 30m)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` at line 138, Update the duration flag help text for debugCaptureCmd to accurately reflect server behavior: change the message passed to debugCaptureCmd.Flags().DurationP("duration", "d", 0, ...) to state that 0 omits the duration so the server uses its default and that the server's maxCaptureDuration is 30m (e.g., "Capture duration (0 = omit and use server default, max 30m)"). Ensure the new text replaces the inaccurate "0 = until interrupted, max 5m" wording.client/cmd/capture.go (1)
78-89:⚠️ Potential issue | 🟡 MinorDelay file creation until after the server accepts the capture.
The output file is created before the first
Recv()readiness check, and the deferred cleanup always printsWrote .... A rejected capture still leaves an empty file with a false success message.💡 Proposed fix
- out, cleanup, err := captureOutput(cmd) - if err != nil { - return err - } - defer cleanup() - // First Recv is the empty acceptance message from the server. If the // device is unavailable (kernel WG, not connected, capture disabled), // the server returns an error instead. if _, err := stream.Recv(); err != nil { return handleCaptureError(err) } + + out, cleanup, err := captureOutput(cmd) + if err != nil { + return err + } + defer cleanup()Also applies to: 156-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/capture.go` around lines 78 - 89, The code currently calls captureOutput (which creates the output file and returns cleanup) before verifying the server accepted the capture via stream.Recv, causing empty files and misleading "Wrote ..." messages; fix by calling stream.Recv() first and only after it succeeds call captureOutput(cmd) and install the deferred cleanup, and update the other similar block (the one around lines 156-165) the same way so file creation and the "Wrote ..." cleanup run only after stream.Recv succeeds; keep handleCaptureError(err) for non-accept cases.
🧹 Nitpick comments (1)
client/firewall/uspfilter/forwarder/endpoint.go (1)
15-19: ClarifyOfferbuffer lifetime in the interface contract.At Line 15–19, consider documenting that
datais ephemeral and must be copied if retained asynchronously. This prevents accidental future misuse by otherPacketCaptureimplementations.Suggested doc tweak
// PacketCapture captures raw packets for debugging. Implementations must be -// safe for concurrent use and must not block. +// safe for concurrent use and must not block. +// The data slice is only guaranteed to be valid for the duration of Offer; +// implementations must copy it if they retain or process it asynchronously. type PacketCapture interface { Offer(data []byte, outbound bool) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/endpoint.go` around lines 15 - 19, The PacketCapture interface's Offer method lacks a contract about the lifetime of its data buffer; update the comment for PacketCapture (and the Offer method) to state that the provided data slice is ephemeral and callers MUST copy it if they retain it beyond the call (e.g., for async processing or storage), and reiterate that implementations must be concurrency-safe and non-blocking; reference the PacketCapture type and its Offer(data []byte, outbound bool) method so reviewers can locate and apply the documentation change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/server/capture.go`:
- Around line 84-163: StartCapture (and StartBundleCapture) currently call
engine.SetCapture(sess) directly which overwrites any existing capture because
the engine only has a single slot; add a server-side single-owner guard so
concurrent captures return a clear error instead of tearing each other down.
Specifically, add a capture ownership check around SetCapture: before
creating/setting sess call a shared method (e.g., Server.acquireCapture or check
a Server.captureActive bool protected by a mutex) that returns an error if a
capture is already active, and release that ownership in all cleanup paths
(including the ctx.Done goroutine and error returns) and in StartBundleCapture
similarly; ensure engine.SetCapture is only called after ownership is acquired
and that ownership is cleared before/after calling engine.SetCapture(nil).
- Around line 120-133: The initial empty acceptance packet is sent before the
capture is actually installed; move the stream.Send(&proto.CapturePacket{}) call
to after engine.SetCapture(sess) succeeds so the client only receives readiness
once SetCapture has completed. Keep the existing cleanup (sess.Stop(),
pw.Close()) on errors: if engine.SetCapture(sess) fails, return the error
without sending anything; if the send after SetCapture fails, perform the same
cleanup and return a send error. Update logic around stream.Send,
engine.SetCapture, sess.Stop and pw.Close to reflect this ordering and error
handling so runCapture's first Recv() remains a correct readiness gate.
In `@util/capture/afpacket_linux.go`:
- Around line 50-75: The Stop() deadlock comes from waiting on c.stopped which
is only closed by readLoop; ensure c.stopped is non-nil and guaranteed to be
closed regardless of Start() success by initializing c.stopped when the
AFPacketCapture is created (or at the top of Start()), and if Start() returns
early on error close c.stopped (or mark stopped) so Stop() will not block; also
ensure readLoop does not attempt to close an already closed channel. Update
Start(), Stop(), and readLoop to use the shared c.stopped channel safely (create
it before any early returns in Start(), close it on Start() failure, and have
readLoop close it on normal exit only if still open).
- Around line 38-44: NewAFPacketCapture currently accepts a possibly-nil sess
which leads recvAndOffer to call c.sess.Offer(...) and panic; add a validation
to prevent that by either (A) making NewAFPacketCapture reject or return a
clearly invalid value when sess is nil (so callers must provide a Session), or
(B) add a defensive nil-check inside recvAndOffer (and any other places that
call c.sess.Offer) to skip offering when c.sess == nil; update references to
AFPacketCapture, NewAFPacketCapture, recvAndOffer and the sess.Offer call (also
noted around the code near line ~170) so the capture loop never invokes Offer on
a nil session.
---
Outside diff comments:
In `@client/ui/debug.go`:
- Around line 237-246: The UI branch that starts the non-trace debug bundle
(where statusLabel is set and s.handleDebugCreation is invoked) currently
ignores captureCheck.Checked when runForDurationCheck is off, causing the
visible "Include packet capture" checkbox to be a silent no-op; either honor the
checkbox by passing captureCheck.Checked into s.handleDebugCreation (add a
capture boolean parameter and thread it through the handler) or disable/hide
captureCheck when runForDurationCheck is false so users cannot request a capture
they won't get; update the call site and s.handleDebugCreation signature (and
any downstream methods) to accept and act on the capture flag, or toggle
captureCheck.Visible/Enabled alongside runForDurationCheck state.
---
Duplicate comments:
In `@client/cmd/capture.go`:
- Around line 78-89: The code currently calls captureOutput (which creates the
output file and returns cleanup) before verifying the server accepted the
capture via stream.Recv, causing empty files and misleading "Wrote ..."
messages; fix by calling stream.Recv() first and only after it succeeds call
captureOutput(cmd) and install the deferred cleanup, and update the other
similar block (the one around lines 156-165) the same way so file creation and
the "Wrote ..." cleanup run only after stream.Recv succeeds; keep
handleCaptureError(err) for non-accept cases.
In `@proxy/cmd/proxy/cmd/debug.go`:
- Line 138: Update the duration flag help text for debugCaptureCmd to accurately
reflect server behavior: change the message passed to
debugCaptureCmd.Flags().DurationP("duration", "d", 0, ...) to state that 0 omits
the duration so the server uses its default and that the server's
maxCaptureDuration is 30m (e.g., "Capture duration (0 = omit and use server
default, max 30m)"). Ensure the new text replaces the inaccurate "0 = until
interrupted, max 5m" wording.
In `@proxy/internal/debug/handler.go`:
- Around line 684-699: The Flush() call is happening before capture.NewSession
and client.SetCapture succeed, which commits the 200 response prematurely; move
the Flush so it runs only after sess is created and SetCapture returns nil
(i.e., after capture.NewSession(...) succeeds and after client.SetCapture(sess)
succeeds). Concretely, remove the early Flush block that checks "if f, ok :=
w.(http.Flusher); ok { f.Flush() }" and instead perform that same Flusher check
and call immediately after the SetCapture(sess) success path (after defer
sess.Stop() and after the SetCapture error check), so headers are only flushed
when the capture session and client setup have succeeded.
---
Nitpick comments:
In `@client/firewall/uspfilter/forwarder/endpoint.go`:
- Around line 15-19: The PacketCapture interface's Offer method lacks a contract
about the lifetime of its data buffer; update the comment for PacketCapture (and
the Offer method) to state that the provided data slice is ephemeral and callers
MUST copy it if they retain it beyond the call (e.g., for async processing or
storage), and reiterate that implementations must be concurrency-safe and
non-blocking; reference the PacketCapture type and its Offer(data []byte,
outbound bool) method so reviewers can locate and apply the documentation
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66f69bd6-2f53-447c-9030-3c0661e07c8e
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (8)
- client/Dockerfile-rootless
- client/cmd/root.go
- client/Dockerfile
- util/capture/pcap_test.go
- util/capture/afpacket_stub.go
- util/capture/capture.go
- util/capture/pcap.go
- proxy/internal/debug/client.go
🚧 Files skipped from review as they are similar to previous changes (17)
- client/firewall/uspfilter/forwarder/icmp.go
- client/cmd/testutil_test.go
- client/cmd/service_controller.go
- client/firewall/uspfilter/filter.go
- client/cmd/debug.go
- client/cmd/service_installer.go
- client/iface/device/device_filter.go
- client/embed/embed.go
- util/capture/filter_test.go
- client/internal/debug/debug.go
- util/capture/session_test.go
- client/proto/daemon.proto
- util/capture/filter.go
- util/capture/session.go
- client/internal/engine.go
- client/wasm/internal/capture/capture.go
- util/capture/text.go
lixmal
force-pushed
the
add-packet-capture
branch
from
a806591 to
42178b9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
proxy/internal/debug/handler.go (1)
684-700:⚠️ Potential issue | 🟠 MajorFlush only after the capture session is fully initialized.
Flush()commits the 200 response beforecapture.NewSession()andclient.SetCapture(sess)can fail. If either setup step errors, the client gets a partial text/pcap response instead of the intended 5xx.Suggested fix
- // Flush headers so the client sees data immediately. - if f, ok := w.(http.Flusher); ok { - f.Flush() - } - sess, err := capture.NewSession(opts) if err != nil { http.Error(w, "create capture session: "+err.Error(), http.StatusInternalServerError) @@ if err := client.SetCapture(sess); err != nil { http.Error(w, "set capture: "+err.Error(), http.StatusServiceUnavailable) return } defer client.SetCapture(nil) //nolint:errcheck + + // Flush headers only after the stream is ready. + if f, ok := w.(http.Flusher); ok { + f.Flush() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/debug/handler.go` around lines 684 - 700, The response is being flushed before the capture session is fully initialized, which can send a 200 start to the client even if capture.NewSession or client.SetCapture fails; to fix it, move the http.Flusher flush (the f.Flush() call on w when w.(http.Flusher) succeeds) so it is invoked only after capture.NewSession returns successfully and after client.SetCapture(sess) has succeeded (i.e., after sess is created and SetCapture returns nil), keeping the existing defer sess.Stop() and defer client.SetCapture(nil) semantics.util/capture/session_test.go (1)
27-31:⚠️ Potential issue | 🟡 MinorReplace the fixed sleeps with deterministic synchronization.
These
50msdelays make the tests timing-dependent. Stop the session and wait onsess.Done()(or another explicit writer-completion signal) before asserting, so CI does not race the writer goroutine.Also applies to: 64-66, 93-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/session_test.go` around lines 27 - 31, The tests use fixed time.Sleep to wait for the writer goroutine after calling sess.Offer and sess.Stop which is flaky; replace those sleeps by stopping the session and then waiting for the deterministic completion signal (e.g. call sess.Stop() followed by <-sess.Done() or sess.Done() wait method) so the test blocks until the writer finishes; update every occurrence (including the blocks around sess.Offer, sess.Stop and the sleep at the other occurrences) to use sess.Done() (or an explicit writer-completion channel) instead of time.Sleep.proxy/cmd/proxy/cmd/debug.go (1)
138-142:⚠️ Potential issue | 🟡 MinorFix the
--durationhelp text.
0does not mean “until interrupted” here, and the max is not 5 minutes. When the query param is omitted, the handler falls back to its 30-minute default and caps at 30 minutes.Suggested fix
- debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = until interrupted, max 5m)") + debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = use server default 30m; max 30m)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` around lines 138 - 142, The help text for the "duration" flag on debugCaptureCmd is incorrect: update the Flags().DurationP("duration", "d", 0, ...) help string to state that omitting the flag uses the handler's 30-minute default and that the maximum allowed duration is 30 minutes (do not claim 0 means "until interrupted"); keep the flag name "duration" and shorthand "d" unchanged and ensure the new message clearly reflects "default 30m, max 30m" or similar phrasing.client/cmd/capture.go (1)
78-89:⚠️ Potential issue | 🟡 MinorOnly report
Wrote ...after the stream is actually accepted.The file is created before the first
Recv()confirms capture startup, and the deferred cleanup always logsWrote <path>. If the daemon rejects capture immediately, users still get an empty file and a false success message.Suggested fix
- out, cleanup, err := captureOutput(cmd) + out, cleanup, markCommitted, err := captureOutput(cmd) if err != nil { return err } defer cleanup() @@ if _, err := stream.Recv(); err != nil { return handleCaptureError(err) } + markCommitted()-// captureOutput returns the writer for capture data and a cleanup function. -func captureOutput(cmd *cobra.Command) (io.Writer, func(), error) { +// captureOutput returns the writer for capture data, a cleanup function, and a commit hook. +func captureOutput(cmd *cobra.Command) (io.Writer, func(), func(), error) { outPath, _ := cmd.Flags().GetString("output") if outPath == "" { return os.Stdout, func() { // no cleanup needed for stdout - }, nil + }, func() {}, nil } f, err := os.Create(outPath) if err != nil { - return nil, nil, fmt.Errorf("create output file: %w", err) + return nil, nil, nil, fmt.Errorf("create output file: %w", err) } + committed := false return f, func() { if err := f.Close(); err != nil { cmd.PrintErrf("close output file: %v\n", err) } - cmd.PrintErrf("Wrote %s\n", outPath) - }, nil + if committed { + cmd.PrintErrf("Wrote %s\n", outPath) + } + }, func() { + committed = true + }, nil }Also applies to: 147-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/capture.go` around lines 78 - 89, The code creates the output file (via captureOutput) and defers cleanup—which logs "Wrote <path>"—before the server confirms capture with stream.Recv(), causing false success messages when the daemon rejects the capture; fix by delaying file creation/logging until after the capture is accepted: either call stream.Recv() first and only call captureOutput() and defer cleanup() after the first successful Recv(), or change cleanup() to accept an "accepted" flag and only emit the "Wrote ..." log when the flag is true (update both occurrences around captureOutput/stream.Recv in capture.go, including the earlier block and the similar block at lines ~147-165). Ensure you reference captureOutput, stream.Recv, and cleanup when making the change.client/server/capture.go (1)
120-133:⚠️ Potential issue | 🟠 MajorSend the acceptance frame only after
SetCapturesucceeds.The current code sends the empty acceptance packet (line 123) before
engine.SetCapture(sess)(line 129). IfSetCapturefails, the client already thinks capture started. Reorder to send acceptance only after the engine has installed the session.🔧 Proposed fix
- // Send an empty initial message to signal that the capture was accepted. - // The client waits for this before printing the banner, so it must arrive - // before any packet data. - if err := stream.Send(&proto.CapturePacket{}); err != nil { - sess.Stop() - pw.Close() - return status.Errorf(codes.Internal, "send initial message: %v", err) - } - if err := engine.SetCapture(sess); err != nil { sess.Stop() pw.Close() return status.Errorf(codes.Internal, "set capture: %v", err) } + + // Send an empty initial message to signal that the capture was accepted. + // The client waits for this before printing the banner, so it must arrive + // only after the engine has installed the session. + if err := stream.Send(&proto.CapturePacket{}); err != nil { + if clearErr := engine.SetCapture(nil); clearErr != nil { + log.Debugf("clear capture after failed accept send: %v", clearErr) + } + sess.Stop() + pw.Close() + return status.Errorf(codes.Internal, "send initial message: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 120 - 133, The initial acceptance packet is sent before the engine successfully installs the session; move the call to stream.Send(&proto.CapturePacket{}) to after engine.SetCapture(sess) so the client only receives acceptance once SetCapture succeeds. Specifically, call engine.SetCapture(sess) first, handle its error by calling sess.Stop() and pw.Close() and returning the status error, and only after that call stream.Send(...) and handle any send error similarly (sess.Stop(), pw.Close(), return formatted status error). Ensure you keep the same cleanup calls (sess.Stop, pw.Close) in both failure paths.
🧹 Nitpick comments (1)
client/ui/debug.go (1)
531-544: Consider increasing the capture window in non-duration mode.The 2-second sleep before creating the debug bundle may not capture meaningful traffic in the non-duration mode. Consider either:
- Increasing the sleep to allow more packets to be captured
- Adding a note in the UI that non-duration mode captures fewer packets
This is a minor UX consideration—the current behavior is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` around lines 531 - 544, The 2-second fixed capture window when params.capture is true (see conn.StartBundleCapture, the deferred conn.StopBundleCapture, and time.Sleep(2 * time.Second)) is likely too short for non-duration captures; increase the sleep to a longer default (e.g., 5–10s) or make the wait configurable via a new captureTimeout parameter/flag and use that instead of the hardcoded 2s, and/or add a short UI message indicating that non-duration captures are brief so users can opt into a longer window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/debug.go`:
- Around line 245-247: The StartBundleCapture call builds a request with
Timeout: durationpb.New(duration + 30*time.Second) but the daemon silently caps
captures at 10 minutes; update the call to either clamp the requested timeout to
the daemon cap (10*time.Minute) before creating proto.StartBundleCaptureRequest
(compute requested := duration + 30*time.Second and use timeout :=
min(requested, 10*time.Minute)) and set Timeout to durationpb.New(timeout), or
if you prefer not to clamp automatically, print a clear warning to the user (via
the CLI output such as cmd.Println or similar) that the requested capture
exceeds the daemon 10m cap and will be truncated; change the code around
client.StartBundleCapture(...) to implement one of these behaviors and include
the cap value (10 minutes) in the message.
In `@client/proto/daemon.proto`:
- Around line 875-879: Update the documentation for the
StartBundleCaptureRequest.timeout field to reflect actual server behavior: note
that the server accepts unset or non-positive values and will normalize/clamp
them to the configured maximum timeout rather than treating the field as
strictly required; reference the protobuf message StartBundleCaptureRequest and
the timeout field, and describe the defaulting/clamping semantics (e.g., "if
unset or <= 0, the server will use the maximum allowed timeout") so the comment
matches the real API contract.
In `@proxy/cmd/proxy/cmd/debug.go`:
- Around line 222-226: The deferred cleanup currently prints "Wrote <path>" even
when the server rejects the capture; update the flow around
captureOutputWriter(cmd, outPath) and the subsequent cleanup() so the success
message is emitted only after Capture() returns without error (or use a
committed boolean set to true after successful Capture()); specifically, remove
or gate the success log inside the deferred cleanup and instead log the "Wrote
..." message immediately after a successful call to the capture/commit function
(the Capture() or equivalent commit method), ensuring any error path leaves no
misleading success output.
---
Duplicate comments:
In `@client/cmd/capture.go`:
- Around line 78-89: The code creates the output file (via captureOutput) and
defers cleanup—which logs "Wrote <path>"—before the server confirms capture with
stream.Recv(), causing false success messages when the daemon rejects the
capture; fix by delaying file creation/logging until after the capture is
accepted: either call stream.Recv() first and only call captureOutput() and
defer cleanup() after the first successful Recv(), or change cleanup() to accept
an "accepted" flag and only emit the "Wrote ..." log when the flag is true
(update both occurrences around captureOutput/stream.Recv in capture.go,
including the earlier block and the similar block at lines ~147-165). Ensure you
reference captureOutput, stream.Recv, and cleanup when making the change.
In `@client/server/capture.go`:
- Around line 120-133: The initial acceptance packet is sent before the engine
successfully installs the session; move the call to
stream.Send(&proto.CapturePacket{}) to after engine.SetCapture(sess) so the
client only receives acceptance once SetCapture succeeds. Specifically, call
engine.SetCapture(sess) first, handle its error by calling sess.Stop() and
pw.Close() and returning the status error, and only after that call
stream.Send(...) and handle any send error similarly (sess.Stop(), pw.Close(),
return formatted status error). Ensure you keep the same cleanup calls
(sess.Stop, pw.Close) in both failure paths.
In `@proxy/cmd/proxy/cmd/debug.go`:
- Around line 138-142: The help text for the "duration" flag on debugCaptureCmd
is incorrect: update the Flags().DurationP("duration", "d", 0, ...) help string
to state that omitting the flag uses the handler's 30-minute default and that
the maximum allowed duration is 30 minutes (do not claim 0 means "until
interrupted"); keep the flag name "duration" and shorthand "d" unchanged and
ensure the new message clearly reflects "default 30m, max 30m" or similar
phrasing.
In `@proxy/internal/debug/handler.go`:
- Around line 684-700: The response is being flushed before the capture session
is fully initialized, which can send a 200 start to the client even if
capture.NewSession or client.SetCapture fails; to fix it, move the http.Flusher
flush (the f.Flush() call on w when w.(http.Flusher) succeeds) so it is invoked
only after capture.NewSession returns successfully and after
client.SetCapture(sess) has succeeded (i.e., after sess is created and
SetCapture returns nil), keeping the existing defer sess.Stop() and defer
client.SetCapture(nil) semantics.
In `@util/capture/session_test.go`:
- Around line 27-31: The tests use fixed time.Sleep to wait for the writer
goroutine after calling sess.Offer and sess.Stop which is flaky; replace those
sleeps by stopping the session and then waiting for the deterministic completion
signal (e.g. call sess.Stop() followed by <-sess.Done() or sess.Done() wait
method) so the test blocks until the writer finishes; update every occurrence
(including the blocks around sess.Offer, sess.Stop and the sleep at the other
occurrences) to use sess.Done() (or an explicit writer-completion channel)
instead of time.Sleep.
---
Nitpick comments:
In `@client/ui/debug.go`:
- Around line 531-544: The 2-second fixed capture window when params.capture is
true (see conn.StartBundleCapture, the deferred conn.StopBundleCapture, and
time.Sleep(2 * time.Second)) is likely too short for non-duration captures;
increase the sleep to a longer default (e.g., 5–10s) or make the wait
configurable via a new captureTimeout parameter/flag and use that instead of the
hardcoded 2s, and/or add a short UI message indicating that non-duration
captures are brief so users can opt into a longer window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4035e811-1469-476c-a3fb-abc21b04909e
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (7)
- client/Dockerfile
- client/cmd/root.go
- client/cmd/service.go
- util/capture/pcap_test.go
- util/capture/afpacket_stub.go
- util/capture/capture.go
- util/capture/filter.go
🚧 Files skipped from review as they are similar to previous changes (17)
- client/cmd/testutil_test.go
- client/cmd/service_params.go
- client/firewall/uspfilter/forwarder/forwarder.go
- client/cmd/service_controller.go
- client/embed/embed.go
- client/Dockerfile-rootless
- client/firewall/uspfilter/forwarder/icmp.go
- client/firewall/uspfilter/filter.go
- client/server/debug.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/iface/device/device_filter.go
- client/internal/debug/debug.go
- util/capture/pcap.go
- proxy/internal/debug/client.go
- client/server/server_test.go
- util/capture/afpacket_linux.go
- client/server/server.go
lixmal
force-pushed
the
add-packet-capture
branch
from
42178b9 to
30b9818
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
client/proto/daemon.proto (1)
875-879:⚠️ Potential issue | 🟡 MinorTimeout field documentation should reflect actual server behavior.
The comment states the field is "Required" but based on typical server patterns (and the past review), the server likely accepts unset or non-positive values and normalizes them to the maximum. Update the comment to describe the defaulting/clamping semantics.
Suggested fix
message StartBundleCaptureRequest { - // timeout auto-stops the capture after this duration as a safety net. - // Required; the server enforces a maximum. + // timeout auto-stops the capture after this duration as a safety net. + // If unset or non-positive, defaults to the server's maximum (typically 30 minutes). google.protobuf.Duration timeout = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/proto/daemon.proto` around lines 875 - 879, The comment for StartBundleCaptureRequest.timeout should be updated to reflect server defaulting/clamping semantics instead of saying "Required": note that the field is optional and if unset or set to a non-positive duration the server will normalize it to a configured maximum/ clamp it to allowed bounds; keep mention that the server enforces a maximum and explain that the timeout acts as a safety-net and will be defaulted/clamped by the server when necessary (referencing the StartBundleCaptureRequest message and the timeout field).proxy/cmd/proxy/cmd/debug.go (2)
266-271:⚠️ Potential issue | 🟡 Minor
Wrote ...printed even when capture is rejected by server.The cleanup function unconditionally prints "Wrote %s" on file close, even if the server immediately rejects the capture request. This leaves an empty file with a misleading success message.
Suggested fix using a committed flag
-func captureOutputWriter(cmd *cobra.Command, outPath string) (out *os.File, cleanup func(), err error) { +func captureOutputWriter(cmd *cobra.Command, outPath string) (out *os.File, cleanup func(), markSuccess func(), err error) { if outPath != "" { f, err := os.Create(outPath) if err != nil { - return nil, nil, fmt.Errorf("create output file: %w", err) + return nil, nil, nil, fmt.Errorf("create output file: %w", err) } + success := false return f, func() { if err := f.Close(); err != nil { cmd.PrintErrf("close output file: %v\n", err) } - cmd.PrintErrf("Wrote %s\n", outPath) - }, nil + if success { + cmd.PrintErrf("Wrote %s\n", outPath) + } + }, func() { success = true }, nil } return os.Stdout, func() { // no cleanup needed for stdout - }, nil + }, func() {}, nil }Then call
markSuccess()afterCapture()returns successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` around lines 266 - 271, The cleanup closure currently always prints "Wrote %s" after closing the output file (f.Close()), which misleads when the server rejects the capture; modify the flow to track success using a committed flag (e.g., a local boolean or a markSuccess() function) set only after Capture() completes successfully, and change the returned cleanup closure to print the "Wrote %s" message only when that flag is true (still close the file and log any Close() error unconditionally). Update references around the Capture() call and the returned closure so the success flag is set after Capture() returns without error and checked inside the cleanup closure before printing outPath.
138-138:⚠️ Potential issue | 🟡 MinorFlag help text inconsistent with actual server behavior.
The help text states "max 5m" but the server's
maxCaptureDurationinhandler.gois 30 minutes. Additionally, "server default" is vague when the default equals the max.Suggested fix
- debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = server default, max 5m)") + debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = server max 30m)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` at line 138, The flag help for debugCaptureCmd.Flags().DurationP currently says "max 5m" and "server default" which contradicts handler.go's maxCaptureDuration (30m); update the flag help text to reflect the actual server behavior by stating the server default/max is 30m (e.g., "Capture duration (0 = server default 30m, max 30m)") so it matches the maxCaptureDuration constant and clarifies that 0 equals the server default.
🧹 Nitpick comments (2)
client/firewall/uspfilter/forwarder/endpoint.go (1)
15-19: ClarifyOfferdata ownership contract.Line 64 passes a slice view into packet-backed memory. Please explicitly document that implementations must copy
databefore retaining/async processing it.✏️ Suggested doc update
-// PacketCapture captures raw packets for debugging. Implementations must be -// safe for concurrent use and must not block. +// PacketCapture captures raw packets for debugging. +// Implementations must be safe for concurrent use and must not block. +// The provided data may reference transient packet memory; copy it before +// retaining it beyond the Offer call. type PacketCapture interface { Offer(data []byte, outbound bool) }Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/forwarder/endpoint.go` around lines 15 - 19, The PacketCapture interface's Offer(data []byte, outbound bool) method currently lacks a data ownership contract; update the PacketCapture documentation (above the interface) to state explicitly that the incoming data slice may be a view into packet-backed memory and is only valid for the duration of the call, so implementations of PacketCapture.Offer must copy the contents of data if they retain it, store it asynchronously, or perform any work after Offer returns; include a short example sentence like "callers may reuse/modify the underlying buffer after return — copy data before retaining it" and reference the Offer method name to make the requirement clear.client/server/capture.go (1)
267-277: Doc comment is misleading.The comment says it "stops any running capture, and returns ''" but the function actually returns
bc.path()(which is non-empty when a capture file exists). Consider clarifying:-// bundleCapturePath returns the temp file path if a capture has been taken, -// stops any running capture, and returns "". Called from DebugBundle. +// bundleCapturePath stops any running capture and returns the temp file path, +// or "" if no capture has been taken. Called from DebugBundle. // Must hold s.mutex.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 267 - 277, The doc comment for Server.bundleCapturePath is inaccurate: it says it "stops any running capture, and returns ''" but the function returns the capture file path when a capture exists. Update the comment for bundleCapturePath to accurately describe behavior: when s.bundleCapture is nil it returns "", otherwise it stops the running capture (s.bundleCapture.stop()) and returns the capture path (s.bundleCapture.path()); keep the note that the caller must hold s.mutex and mention that it is called from DebugBundle to help locate usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/debug.go`:
- Around line 254-257: The deferred stop uses the potentially cancelled
cmd.Context(), causing StopBundleCapture to fail on cancellation; change the
defer in the capture routine to call client.StopBundleCapture with a fresh
non-cancelled context (e.g., context.Background() or
context.WithTimeout(context.Background(), <reasonableTimeout>)) when
captureStarted is true so the stop RPC has a live context; update both
occurrences that call client.StopBundleCapture (the defer block around
captureStarted and the later call at lines 269-271) to use the new context and
ensure any created context is cancelled appropriately if you used WithTimeout.
---
Duplicate comments:
In `@client/proto/daemon.proto`:
- Around line 875-879: The comment for StartBundleCaptureRequest.timeout should
be updated to reflect server defaulting/clamping semantics instead of saying
"Required": note that the field is optional and if unset or set to a
non-positive duration the server will normalize it to a configured maximum/
clamp it to allowed bounds; keep mention that the server enforces a maximum and
explain that the timeout acts as a safety-net and will be defaulted/clamped by
the server when necessary (referencing the StartBundleCaptureRequest message and
the timeout field).
In `@proxy/cmd/proxy/cmd/debug.go`:
- Around line 266-271: The cleanup closure currently always prints "Wrote %s"
after closing the output file (f.Close()), which misleads when the server
rejects the capture; modify the flow to track success using a committed flag
(e.g., a local boolean or a markSuccess() function) set only after Capture()
completes successfully, and change the returned cleanup closure to print the
"Wrote %s" message only when that flag is true (still close the file and log any
Close() error unconditionally). Update references around the Capture() call and
the returned closure so the success flag is set after Capture() returns without
error and checked inside the cleanup closure before printing outPath.
- Line 138: The flag help for debugCaptureCmd.Flags().DurationP currently says
"max 5m" and "server default" which contradicts handler.go's maxCaptureDuration
(30m); update the flag help text to reflect the actual server behavior by
stating the server default/max is 30m (e.g., "Capture duration (0 = server
default 30m, max 30m)") so it matches the maxCaptureDuration constant and
clarifies that 0 equals the server default.
---
Nitpick comments:
In `@client/firewall/uspfilter/forwarder/endpoint.go`:
- Around line 15-19: The PacketCapture interface's Offer(data []byte, outbound
bool) method currently lacks a data ownership contract; update the PacketCapture
documentation (above the interface) to state explicitly that the incoming data
slice may be a view into packet-backed memory and is only valid for the duration
of the call, so implementations of PacketCapture.Offer must copy the contents of
data if they retain it, store it asynchronously, or perform any work after Offer
returns; include a short example sentence like "callers may reuse/modify the
underlying buffer after return — copy data before retaining it" and reference
the Offer method name to make the requirement clear.
In `@client/server/capture.go`:
- Around line 267-277: The doc comment for Server.bundleCapturePath is
inaccurate: it says it "stops any running capture, and returns ''" but the
function returns the capture file path when a capture exists. Update the comment
for bundleCapturePath to accurately describe behavior: when s.bundleCapture is
nil it returns "", otherwise it stops the running capture
(s.bundleCapture.stop()) and returns the capture path (s.bundleCapture.path());
keep the note that the caller must hold s.mutex and mention that it is called
from DebugBundle to help locate usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f9d50f0-85e8-42da-9703-ef46c218ec91
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (9)
- client/cmd/root.go
- client/Dockerfile
- util/capture/pcap_test.go
- util/capture/session_test.go
- util/capture/capture.go
- util/capture/afpacket_stub.go
- util/capture/filter_test.go
- util/capture/afpacket_linux.go
- client/internal/debug/debug.go
🚧 Files skipped from review as they are similar to previous changes (13)
- client/Dockerfile-rootless
- client/cmd/service_params.go
- client/firewall/uspfilter/forwarder/forwarder.go
- client/firewall/uspfilter/forwarder/icmp.go
- client/cmd/testutil_test.go
- client/firewall/uspfilter/filter.go
- client/embed/embed.go
- client/iface/device/device_filter.go
- client/cmd/service_controller.go
- proxy/internal/debug/client.go
- util/capture/pcap.go
- client/ui/debug.go
- util/capture/text.go
lixmal
force-pushed
the
add-packet-capture
branch
2 times, most recently
from
d8bb22c to
2243f82
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
proxy/cmd/proxy/cmd/debug.go (1)
138-138:⚠️ Potential issue | 🟡 MinorDuration flag help text is misleading.
The help text states "max 5m" but according to the PR description, the server defaults to 30 minutes when duration is omitted (duration=0). The text should accurately reflect the server's behavior.
Suggested fix
- debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = server default, max 5m)") + debugCaptureCmd.Flags().DurationP("duration", "d", 0, "Capture duration (0 = server default 30m)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` at line 138, Update the misleading flag help for debugCaptureCmd.Flags().DurationP: change the duration flag description from "Capture duration (0 = server default, max 5m)" to accurately state the server default (e.g. "Capture duration (0 = server default, 30m)") and remove or correct the incorrect "max 5m" text so the help matches the server behavior described in the PR.
🧹 Nitpick comments (2)
client/server/capture.go (1)
84-166: Consider adding explicit capture slot ownership.Both streaming capture (
StartCapture) and bundle capture (StartBundleCapture) set the engine's capture viaengine.SetCapture(sess). While bundle-vs-bundle conflicts are handled bystopBundleCaptureLocked(), there's no guard preventing a streaming capture from being overwritten by a bundle capture (or vice versa).Given these are debug features with limited concurrent use in practice, this may be acceptable. If stricter isolation is desired, consider adding a server-level "capture active" flag that both code paths check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 84 - 166, Add explicit capture ownership to prevent stream vs bundle races by introducing a Server-level capture owner flag plus mutex (e.g., add fields on Server: captureOwner string and captureMu sync.Mutex). In StartCapture (and similarly in StartBundleCapture/stopBundleCaptureLocked) acquire captureMu, check that captureOwner is empty (or matches the same type if reentrant allowed), set captureOwner="stream" before calling engine.SetCapture(sess), and clear captureOwner when clearing the capture (in the ctx.Done goroutine, defer cleanup, and on all error paths where sess.Stop()/pw.Close() and engine.SetCapture(nil) are called). Ensure all places that call engine.SetCapture(nil) also clear captureOwner under captureMu to maintain consistent ownership.util/capture/session.go (1)
111-117: Consider clarifying whatbytescounter represents.Line 114 uses
len(data)(original packet length) rather thancaptureLen(truncated length). This meansStats.Bytestracks total bytes offered to the session, not bytes actually captured to disk. Both interpretations are valid, but consider either:
- Renaming to
BytesOfferedfor clarity, or- Adding a comment in the
Statsstruct documenting this semantic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/session.go` around lines 111 - 117, The bytes counter currently uses len(data) (the original packet length) in the select block (s.bytes.Add(int64(len(data)))) which makes Stats.Bytes represent "bytes offered" not "bytes captured"; update this by either (A) renaming Stats.Bytes to BytesOffered (and update all references to s.bytes) to make intent explicit, or (B) keep the name but add a clear comment on the Stats struct documenting that Bytes counts offered bytes, and if you want it to measure actually captured bytes change the increment here to use captureLen (the truncated length) instead of len(data) and document that behavior; adjust references to s.bytes and any metric consumers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@proxy/cmd/proxy/cmd/debug.go`:
- Line 138: Update the misleading flag help for
debugCaptureCmd.Flags().DurationP: change the duration flag description from
"Capture duration (0 = server default, max 5m)" to accurately state the server
default (e.g. "Capture duration (0 = server default, 30m)") and remove or
correct the incorrect "max 5m" text so the help matches the server behavior
described in the PR.
---
Nitpick comments:
In `@client/server/capture.go`:
- Around line 84-166: Add explicit capture ownership to prevent stream vs bundle
races by introducing a Server-level capture owner flag plus mutex (e.g., add
fields on Server: captureOwner string and captureMu sync.Mutex). In StartCapture
(and similarly in StartBundleCapture/stopBundleCaptureLocked) acquire captureMu,
check that captureOwner is empty (or matches the same type if reentrant
allowed), set captureOwner="stream" before calling engine.SetCapture(sess), and
clear captureOwner when clearing the capture (in the ctx.Done goroutine, defer
cleanup, and on all error paths where sess.Stop()/pw.Close() and
engine.SetCapture(nil) are called). Ensure all places that call
engine.SetCapture(nil) also clear captureOwner under captureMu to maintain
consistent ownership.
In `@util/capture/session.go`:
- Around line 111-117: The bytes counter currently uses len(data) (the original
packet length) in the select block (s.bytes.Add(int64(len(data)))) which makes
Stats.Bytes represent "bytes offered" not "bytes captured"; update this by
either (A) renaming Stats.Bytes to BytesOffered (and update all references to
s.bytes) to make intent explicit, or (B) keep the name but add a clear comment
on the Stats struct documenting that Bytes counts offered bytes, and if you want
it to measure actually captured bytes change the increment here to use
captureLen (the truncated length) instead of len(data) and document that
behavior; adjust references to s.bytes and any metric consumers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62cffb58-29a0-4a50-9dc7-2d2a89810c4b
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (10)
- client/cmd/root.go
- client/Dockerfile-rootless
- client/Dockerfile
- client/server/setconfig_test.go
- client/cmd/service_installer.go
- util/capture/capture.go
- util/capture/afpacket_stub.go
- util/capture/pcap.go
- client/cmd/service.go
- util/capture/afpacket_linux.go
🚧 Files skipped from review as they are similar to previous changes (10)
- client/cmd/debug.go
- client/cmd/service_params.go
- client/cmd/service_controller.go
- client/server/server.go
- client/server/debug.go
- client/firewall/uspfilter/filter.go
- client/internal/debug/debug.go
- util/capture/pcap_test.go
- proxy/internal/debug/client.go
- client/ui/debug.go
lixmal
force-pushed
the
add-packet-capture
branch
10 times, most recently
from
e5de280 to
4e749b6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/capture.go`:
- Around line 147-169: captureOutput currently opens outPath directly which
truncates any existing file; instead create a new temporary file in the same
directory (e.g., using os.CreateTemp with the directory derived from outPath)
and return that writer; on cleanup, if the temp file has size > 0 rename
(os.Rename) the temp file to outPath so the original file is not lost, otherwise
remove the temp file; keep the stdout path unchanged and preserve the existing
cmd.PrintErrf messages and error handling in captureOutput and reference the
functions/variables captureOutput, outPath, f (temp file) and the cleanup
closure when implementing this.
In `@client/ui/debug.go`:
- Around line 401-408: The StartBundleCapture error is only being logged
(log.Warnf) so the UI continues as if capture started; modify the bundle-start
flow in client/ui/debug.go where StartBundleCapture is called (the block using
params.capture, conn.StartBundleCapture, s.ctx and
proto.StartBundleCaptureRequest) to propagate the failure to the caller/UI
instead of silently logging: either return an error from the function or set a
visible UI status/alert indicating capture failed (so the overall action fails
or shows a clear warning), and apply the same change to the other identical
block around the 533-547 region.
In `@client/wasm/internal/capture/capture.go`:
- Around line 167-193: The Write method on jsCallbackWriter currently invokes
the JS callback while holding w.mu which can deadlock; change Write
(jsCallbackWriter.Write) to collect complete lines into a local slice while
holding w.mu (read/reset w.buf and append each extracted line to a []string),
release the lock, then iterate that slice and perform the JS checks
(w.iface.IsUndefined, cb := w.iface.Get("onpacket"), cb.IsUndefined/IsNull) and
cb.Invoke(js.ValueOf(line)) outside the mutex. Ensure buffer mutation
(w.buf.Reset and write remainder) remains under the lock and that you preserve
the same skip behavior when iface or callback are undefined.
- Around line 25-30: Stop() unconditionally clears the client's global capture
which can detach a newer capture started after this handle; change Stop() to
only call client.SetCapture(nil) when the client's current capture matches this
handle's session (e.g., compare h.client.GetCapture() or the capture ID to
h.sess or h.sess.ID()), otherwise skip clearing; update Handle.Stop() to check
ownership before calling SetCapture(nil) (or alternatively make start/stop
return a unique capture token and validate that token on Stop to reject
overlapping starts).
In `@proxy/cmd/proxy/cmd/debug.go`:
- Around line 260-275: captureOutputWriter currently calls os.Create(outPath)
which truncates any existing file; instead create a temp file (e.g. via
os.CreateTemp) in the same directory and return that file handle to the caller,
and change the cleanup closure so that on successful capture (temp file size >
0) it closes and atomically renames the temp into outPath (preserving original
permissions if needed), while on failure it removes the temp and leaves any
existing outPath untouched; update error handling in captureOutputWriter to
propagate failures from temp creation and renaming and to log errors with
cmd.PrintErrf in the cleanup.
In `@proxy/internal/debug/handler.go`:
- Around line 654-657: The current duration parsing silently falls back to
maxCaptureDuration on parse errors or negative values; change the logic in the
handler where duration is set (the block using time.ParseDuration,
maxCaptureDuration, min, and r.URL.Query().Get("duration")) to validate the
query value: if time.ParseDuration returns an error or the parsed duration is <=
0, respond with HTTP 400 (use http.Error or write a 400 response) and stop
processing; only if parsing succeeds and d > 0 set duration = min(d,
maxCaptureDuration).
In `@util/capture/filter.go`:
- Around line 164-183: The parseIPv4Info function extracts L4 ports for any IPv4
packet with len(data) >= ihl+4, but it must ignore ports for non-initial
fragments; update parseIPv4Info to read the IPv4 fragment offset from bytes 6-7
(masking with 0x1FFF) and only populate srcPort/dstPort when the fragment offset
is zero and the protocol is TCP/UDP and len(data) >= ihl+4; ensure you still
respect ihl and proto checks before reading ports and set srcPort/dstPort only
in that guarded branch.
In `@util/capture/session.go`:
- Around line 89-117: Offer can race with Stop: a packet can pass the s.closed
check and be sent into s.ch after run() drained and exited. Add an in-flight
synchronizer (e.g., a sync.WaitGroup or atomic counter on Session) and update
Offer and Stop accordingly: in Offer increment the in-flight before proceeding,
re-check s.closed immediately after increment (if closed, decrement and return),
then perform the copy/enqueue and decrement when done; in Stop set s.closed,
wait for the in-flight counter/waitgroup to reach zero, then close or signal the
writer goroutine (run) to exit so no packets can be enqueued after run drains.
Update Session methods Offer, Stop and run to use this in-flight mechanism and
reference s.ch, s.closed, s.packets, s.dropped to ensure accurate stats and no
lost packets.
In `@util/capture/text.go`:
- Around line 421-423: The ServerHello branch is using the legacy version field
(tlsVersionName(data)) which reports 0x0303 as TLS1.2 even for TLS1.3; update
the ServerHello handling (case 0x02) to inspect the ServerHello extensions for
the supported_versions extension (extension type 0x2b / 43) and, if present,
read the version value there (0x0304 == TLS1.3) to produce the correct label; if
the extension is absent, fall back to the legacy field or return a non-precise
label (e.g. "TLS ServerHello (unknown version)") to avoid mislabeling. Make the
change where tlsVersionName is used for ServerHello (and similarly in the other
ServerHello handling around the 438-455 range) — either extend tlsVersionName to
parse supported_versions from the provided data or add a helper like
parseServerHelloVersion(data) that performs the extension lookup and returns the
accurate version string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d6367f4-5213-40b6-b449-2964bd2f7f71
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (9)
- client/Dockerfile
- client/Dockerfile-rootless
- client/cmd/testutil_test.go
- client/cmd/service_controller.go
- util/capture/capture.go
- util/capture/pcap_test.go
- util/capture/filter_test.go
- util/capture/afpacket_linux.go
- util/capture/pcap.go
🚧 Files skipped from review as they are similar to previous changes (9)
- client/cmd/service_params.go
- client/server/setconfig_test.go
- client/cmd/debug.go
- client/cmd/service_installer.go
- client/internal/debug/debug.go
- util/capture/afpacket_stub.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/server/server.go
- client/server/capture.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
proxy/cmd/proxy/cmd/debug.go (1)
260-275:⚠️ Potential issue | 🟠 MajorPreserve an existing
--outputfile on empty or failed captures.Line 262 truncates
outPathup front, and Lines 270-274 delete it again if the capture produces no bytes. With-o existing.pcap, an early server rejection or empty capture destroys the pre-existing file.💡 Safer pattern
Add a
path/filepathimport and write to a temp file in the same directory, then rename it into place only when it has data:func captureOutputWriter(cmd *cobra.Command, outPath string) (out *os.File, cleanup func(), err error) { if outPath != "" { - f, err := os.Create(outPath) + f, err := os.CreateTemp(filepath.Dir(outPath), filepath.Base(outPath)+".*.tmp") if err != nil { return nil, nil, fmt.Errorf("create output file: %w", err) } + tmpPath := f.Name() return f, func() { if err := f.Close(); err != nil { cmd.PrintErrf("close output file: %v\n", err) } - if fi, err := os.Stat(outPath); err == nil && fi.Size() > 0 { + if fi, err := os.Stat(tmpPath); err == nil && fi.Size() > 0 { + if err := os.Rename(tmpPath, outPath); err != nil { + cmd.PrintErrf("rename output file: %v\n", err) + _ = os.Remove(tmpPath) + return + } cmd.PrintErrf("Wrote %s\n", outPath) } else { - os.Remove(outPath) + _ = os.Remove(tmpPath) } }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/cmd/proxy/cmd/debug.go` around lines 260 - 275, The current captureOutputWriter function truncates the target outPath immediately and may delete a pre-existing file on empty/failed captures; change it to create a temporary file in the same directory (use path/filepath to derive dir and os.CreateTemp) and return that temp file for writing, then in the cleanup close the temp, stat it and only if it has data rename it into place with os.Rename(outTemp, outPath); on error or zero-size remove the temp and leave the original outPath untouched; update cleanup messages printed by cmd.PrintErrf accordingly and reference captureOutputWriter, the temp file variable, and the rename/remove logic.client/cmd/capture.go (1)
147-169:⚠️ Potential issue | 🟠 MajorDon't truncate an existing capture file before you know you have output.
Line 156 opens
outPathwithos.Create, so any existing file is truncated immediately. If the accepted stream finishes before the first packet arrives, Lines 164-168 remove that now-empty file, turning an empty capture into data loss.💡 Safer pattern
Add a
path/filepathimport and stage output in a temp file in the same directory:func captureOutput(cmd *cobra.Command) (io.Writer, func(), error) { outPath, _ := cmd.Flags().GetString("output") if outPath == "" { return os.Stdout, func() { // no cleanup needed for stdout }, nil } - f, err := os.Create(outPath) + f, err := os.CreateTemp(filepath.Dir(outPath), filepath.Base(outPath)+".*.tmp") if err != nil { return nil, nil, fmt.Errorf("create output file: %w", err) } + tmpPath := f.Name() return f, func() { if err := f.Close(); err != nil { cmd.PrintErrf("close output file: %v\n", err) } - if fi, err := os.Stat(outPath); err == nil && fi.Size() > 0 { + if fi, err := os.Stat(tmpPath); err == nil && fi.Size() > 0 { + if err := os.Rename(tmpPath, outPath); err != nil { + cmd.PrintErrf("rename output file: %v\n", err) + _ = os.Remove(tmpPath) + return + } cmd.PrintErrf("Wrote %s\n", outPath) } else { - os.Remove(outPath) + _ = os.Remove(tmpPath) } }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/capture.go` around lines 147 - 169, captureOutput currently uses os.Create(outPath) which truncates existing files; instead, write to a temp file in the same directory and only move it to outPath after successful non-empty output. In captureOutput, use filepath.Dir(outPath) and os.CreateTemp (or ioutil.TempFile) to create a staged temp file, return that writer and a cleanup that closes the temp file, checks its size (fi.Size() > 0), and only then atomically renames the temp file to outPath (os.Rename); if the temp is empty or an error occurred, remove the temp file and do not touch the original outPath. Ensure error messages still use cmd.PrintErrf and that stdout behavior (when outPath=="") is unchanged.client/wasm/internal/capture/capture.go (2)
167-193:⚠️ Potential issue | 🟠 MajorInvoke
onpacketafter releasingw.mu.
cb.Invoke(...)runs while the writer mutex is held. If the callback callsstop()or otherwise re-enters capture state,Handle.Stop()waits for the session goroutine whileWrite()is waiting for JS to return, which deadlocks the capture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/capture/capture.go` around lines 167 - 193, In jsCallbackWriter.Write, avoid calling cb.Invoke while holding w.mu; instead, extract the callback and the line to a local variable (e.g., cb := w.iface.Get("onpacket") and line := ...), reset/write the buffer while under the lock, then release the lock and only then call cb.Invoke(js.ValueOf(line)); ensure you still skip nil/undefined cb values and preserve the existing loop semantics by checking/obtaining cb and line inside the locked section but performing the Invoke outside the lock to prevent deadlock with Handle.Stop().
25-30:⚠️ Potential issue | 🟠 MajorMake
Handle.Stop()ownership-aware.
Stop()always clears the client's global capture. If JS starts a second capture before the first handle is stopped, stopping the older handle detaches the newer session and capture silently stops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/capture/capture.go` around lines 25 - 30, Handle.Stop currently unconditionally clears the global capture which can detach a newer capture; modify Stop (in the Handle type) so it only calls h.client.SetCapture(nil) when the client's active capture belongs to this handle (e.g., compare the client's current capture/session or owner ID with h.sess or a handle ID on h before clearing). Keep calling h.sess.Stop() and returning h.sess.Stats(); if the active capture differs, skip SetCapture(nil) to avoid detaching a newer session. Ensure you reference the existing symbols Handle.Stop, h.client.SetCapture, and h.sess when implementing the ownership check.util/capture/session.go (1)
89-128:⚠️ Potential issue | 🟠 MajorSynchronize
Stop()with in-flightOffer()calls.A packet can pass the first
s.closed.Load()check just before Line 125, enqueue afterrun()has drained, and then be lost whileStats()still counts it.Stop()needs an in-flight counter orWaitGroupso the writer goroutine exits only after all acceptedOffer()calls have finished.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/session.go` around lines 89 - 128, Session.Stop can race with concurrent Session.Offer calls: an Offer may pass s.closed.Load(), start copying/enqueuing, but Stop may close s.done and drain the channel before that Offer finishes, causing lost packets and mismatched Stats. Fix by adding an in-flight counter (or sync.WaitGroup) on Session (e.g., s.inflight / s.wg), incrementing it at the start of Offer and decrementing it just before Offer returns (ensure decrement happens even on early returns), and having Stop wait for the in-flight count to reach zero (or wg.Wait()) before closing/closing drains and waiting on s.stopped; update Offer and Stop to use these symbols (Session.Offer, Session.Stop, s.closed, s.done, s.stopped, s.ch) so the writer goroutine only exits after all accepted Offer calls complete.proxy/internal/debug/handler.go (1)
654-657:⚠️ Potential issue | 🟡 MinorReject malformed or negative
durationvalues instead of silently using 30m.
duration=bogusorduration=-1scurrently falls back tomaxCaptureDuration, so an operator typo starts a long capture instead of failing fast. Keep""/0as “use max”, but return400for parse errors and negative durations.🛠️ Proposed fix
duration := maxCaptureDuration - if d, err := time.ParseDuration(r.URL.Query().Get("duration")); err == nil && d > 0 { - duration = min(d, maxCaptureDuration) + if raw := r.URL.Query().Get("duration"); raw != "" { + d, err := time.ParseDuration(raw) + if err != nil || d < 0 { + http.Error(w, "invalid duration", http.StatusBadRequest) + return + } + if d > 0 { + duration = min(d, maxCaptureDuration) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/debug/handler.go` around lines 654 - 657, The handler currently sets duration := maxCaptureDuration and silently falls back on max when time.ParseDuration fails or returns a negative value; change this so that an empty string or explicit "0" continues to mean use maxCaptureDuration, but any parse error or negative parsed duration causes the handler to return HTTP 400. Specifically, use time.ParseDuration on r.URL.Query().Get("duration") and if err != nil (and the raw value is non-empty) respond with 400, and if d < 0 respond with 400; only when the value is "" or d == 0 set duration = maxCaptureDuration, otherwise set duration = min(d, maxCaptureDuration). Ensure the response uses the same handler logic and variable names (duration, maxCaptureDuration) and stops processing after writing the 400.client/server/capture.go (1)
120-155:⚠️ Potential issue | 🟠 MajorSerialize streaming and bundle capture against the single engine capture slot.
StartCapture()andStartBundleCapture()both callengine.SetCapture(sess)independently, but the engine only holds one capture sink. Starting one mode while the other is active overwrites its session, and the older cleanup path later callsengine.SetCapture(nil), tearing down the newer capture.Also applies to: 221-245
util/capture/filter.go (1)
179-182:⚠️ Potential issue | 🟡 MinorIgnore TCP/UDP ports on non-initial IPv4 fragments.
Later IPv4 fragments reuse the bytes at
ihl:as payload, soport/src port/dst portfilters can match unrelated traffic by accident. Gate port extraction onfragment offset == 0.🛠️ Proposed fix
info := packetInfo{ family: 4, srcIP: netip.AddrFrom4([4]byte{data[12], data[13], data[14], data[15]}), dstIP: netip.AddrFrom4([4]byte{data[16], data[17], data[18], data[19]}), proto: data[9], hdrLen: ihl, } - if (info.proto == protoTCP || info.proto == protoUDP) && len(data) >= ihl+4 { + fragOff := binary.BigEndian.Uint16(data[6:8]) & 0x1fff + if fragOff == 0 && (info.proto == protoTCP || info.proto == protoUDP) && len(data) >= ihl+4 { info.srcPort = binary.BigEndian.Uint16(data[ihl:]) info.dstPort = binary.BigEndian.Uint16(data[ihl+2:]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/filter.go` around lines 179 - 182, The port extraction currently reads bytes at ihl for all IPv4 packets; change it to only extract src/dst ports when this is the initial fragment by checking the IPv4 fragment offset == 0. Concretely, before the block that sets info.srcPort/info.dstPort (the if using protoTCP/protoUDP, ihl, data), parse the IPv4 flags+fragment offset from data[6:8] (mask with 0x1FFF) and require that value == 0, then proceed to read ports into info.srcPort and info.dstPort.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/forwarder/endpoint.go`:
- Around line 64-72: The packet capture is happening before the device
injection, causing false positives when CreateOutboundPacket fails; move the
capture Offer call to after a successful e.device.CreateOutboundPacket call and
only invoke it if CreateOutboundPacket returns nil. Locate the block using
e.device.CreateOutboundPacket(...) and e.capture.Load()/(*pc).Offer(...), call
CreateOutboundPacket first, check err == nil, then load e.capture and call
(*pc).Offer(pktBytes, true); do not change Offer semantics or swallow
CreateOutboundPacket errors.
In `@client/server/capture.go`:
- Around line 194-245: s.bundleCapture's previous temp file is left open and its
path lost when assigning a new bundleCapture; before overwriting s.bundleCapture
(just prior to "s.bundleCapture = bc"), if s.bundleCapture != nil then close and
remove its file (call Close() on s.bundleCapture.file and
os.Remove(s.bundleCapture.file.Name())), and clear the reference (set
s.bundleCapture = nil) so the old file is not leaked; perform this cleanup while
holding the same lock/context as
s.stopBundleCaptureLocked()/GetCaptureEngineLocked to avoid races and ensure
sessions are stopped (use existing bundleCapture.stop() if needed).
In `@util/capture/session.go`:
- Around line 146-151: The run() goroutine currently forces a PCAP global header
by calling s.pcapW.WriteHeader() at start (in Session.run), which can
prematurely commit responses or block pipes; remove that immediate call and
instead ensure the PCAP header is written lazily on the first packet write path
(e.g., in the method that writes packets to s.pcapW such as the packet
write/Flush routine) so the header is emitted only when the first packet is
actually written; update Session.run to omit the WriteHeader call and rely on
the existing lazy header logic in util/capture/pcap.go (or add a one-time header
write in the WritePacket function) to preserve unbuffered writer safety.
---
Duplicate comments:
In `@client/cmd/capture.go`:
- Around line 147-169: captureOutput currently uses os.Create(outPath) which
truncates existing files; instead, write to a temp file in the same directory
and only move it to outPath after successful non-empty output. In captureOutput,
use filepath.Dir(outPath) and os.CreateTemp (or ioutil.TempFile) to create a
staged temp file, return that writer and a cleanup that closes the temp file,
checks its size (fi.Size() > 0), and only then atomically renames the temp file
to outPath (os.Rename); if the temp is empty or an error occurred, remove the
temp file and do not touch the original outPath. Ensure error messages still use
cmd.PrintErrf and that stdout behavior (when outPath=="") is unchanged.
In `@client/wasm/internal/capture/capture.go`:
- Around line 167-193: In jsCallbackWriter.Write, avoid calling cb.Invoke while
holding w.mu; instead, extract the callback and the line to a local variable
(e.g., cb := w.iface.Get("onpacket") and line := ...), reset/write the buffer
while under the lock, then release the lock and only then call
cb.Invoke(js.ValueOf(line)); ensure you still skip nil/undefined cb values and
preserve the existing loop semantics by checking/obtaining cb and line inside
the locked section but performing the Invoke outside the lock to prevent
deadlock with Handle.Stop().
- Around line 25-30: Handle.Stop currently unconditionally clears the global
capture which can detach a newer capture; modify Stop (in the Handle type) so it
only calls h.client.SetCapture(nil) when the client's active capture belongs to
this handle (e.g., compare the client's current capture/session or owner ID with
h.sess or a handle ID on h before clearing). Keep calling h.sess.Stop() and
returning h.sess.Stats(); if the active capture differs, skip SetCapture(nil) to
avoid detaching a newer session. Ensure you reference the existing symbols
Handle.Stop, h.client.SetCapture, and h.sess when implementing the ownership
check.
In `@proxy/cmd/proxy/cmd/debug.go`:
- Around line 260-275: The current captureOutputWriter function truncates the
target outPath immediately and may delete a pre-existing file on empty/failed
captures; change it to create a temporary file in the same directory (use
path/filepath to derive dir and os.CreateTemp) and return that temp file for
writing, then in the cleanup close the temp, stat it and only if it has data
rename it into place with os.Rename(outTemp, outPath); on error or zero-size
remove the temp and leave the original outPath untouched; update cleanup
messages printed by cmd.PrintErrf accordingly and reference captureOutputWriter,
the temp file variable, and the rename/remove logic.
In `@proxy/internal/debug/handler.go`:
- Around line 654-657: The handler currently sets duration := maxCaptureDuration
and silently falls back on max when time.ParseDuration fails or returns a
negative value; change this so that an empty string or explicit "0" continues to
mean use maxCaptureDuration, but any parse error or negative parsed duration
causes the handler to return HTTP 400. Specifically, use time.ParseDuration on
r.URL.Query().Get("duration") and if err != nil (and the raw value is non-empty)
respond with 400, and if d < 0 respond with 400; only when the value is "" or d
== 0 set duration = maxCaptureDuration, otherwise set duration = min(d,
maxCaptureDuration). Ensure the response uses the same handler logic and
variable names (duration, maxCaptureDuration) and stops processing after writing
the 400.
In `@util/capture/filter.go`:
- Around line 179-182: The port extraction currently reads bytes at ihl for all
IPv4 packets; change it to only extract src/dst ports when this is the initial
fragment by checking the IPv4 fragment offset == 0. Concretely, before the block
that sets info.srcPort/info.dstPort (the if using protoTCP/protoUDP, ihl, data),
parse the IPv4 flags+fragment offset from data[6:8] (mask with 0x1FFF) and
require that value == 0, then proceed to read ports into info.srcPort and
info.dstPort.
In `@util/capture/session.go`:
- Around line 89-128: Session.Stop can race with concurrent Session.Offer calls:
an Offer may pass s.closed.Load(), start copying/enqueuing, but Stop may close
s.done and drain the channel before that Offer finishes, causing lost packets
and mismatched Stats. Fix by adding an in-flight counter (or sync.WaitGroup) on
Session (e.g., s.inflight / s.wg), incrementing it at the start of Offer and
decrementing it just before Offer returns (ensure decrement happens even on
early returns), and having Stop wait for the in-flight count to reach zero (or
wg.Wait()) before closing/closing drains and waiting on s.stopped; update Offer
and Stop to use these symbols (Session.Offer, Session.Stop, s.closed, s.done,
s.stopped, s.ch) so the writer goroutine only exits after all accepted Offer
calls complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 020a5e31-8185-4ff3-9114-32687fa01cfb
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (41)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (10)
- client/Dockerfile
- client/cmd/root.go
- client/Dockerfile-rootless
- util/capture/session_test.go
- util/capture/afpacket_stub.go
- util/capture/pcap_test.go
- util/capture/capture.go
- util/capture/pcap.go
- client/ui/debug.go
- util/capture/filter_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- client/cmd/service_params.go
- client/cmd/testutil_test.go
- client/cmd/service_controller.go
- client/cmd/service.go
- client/cmd/debug.go
- client/iface/device/device_filter.go
- client/internal/debug/debug.go
- util/capture/afpacket_linux.go
- proxy/internal/debug/client.go
- util/capture/text.go
lixmal
commented
Apr 15, 2026
lixmal
force-pushed
the
add-packet-capture
branch
from
4e749b6 to
81b63cd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
client/wasm/internal/capture/capture.go (2)
25-30:⚠️ Potential issue | 🟠 MajorMake
Stop()ownership-aware.
Handle.Stop()still clears whatever capture is currently installed on the client. If a second capture starts before the first handle is stopped, stopping the stale handle tears down the newer session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/capture/capture.go` around lines 25 - 30, Handle.Stop currently unconditionally clears the client's capture which may tear down a newer session; change Stop to be ownership-aware by checking the client's current capture before clearing it: call the client's accessor (e.g., GetCapture or equivalent) and only call h.client.SetCapture(nil) if the returned capture equals this handle's capture (or this handle) and is non-nil, then proceed to call h.sess.Stop() and return h.sess.Stats(); keep using h.client.SetCapture, h.sess.Stop, and h.sess.Stats to locate the relevant code.
167-193:⚠️ Potential issue | 🟠 MajorMove JS callback invocation out of the writer mutex.
cb.Invoke(...)still runs whilew.muis held. If the callback callsstop()or otherwise re-enters capture state, the capture path can deadlock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/internal/capture/capture.go` around lines 167 - 193, In jsCallbackWriter.Write, the code is invoking the JavaScript callback (cb.Invoke) while holding w.mu which can deadlock if the callback re-enters capture; modify Write so that inside the loop you check w.iface.IsUndefined() and retrieve cb := w.iface.Get("onpacket") while holding the lock, then store the valid cb and the line in local variables (or append to a local slice) and release the lock before calling cb.Invoke(js.ValueOf(line)); ensure you never call cb.Invoke while w.mu is held and keep references to w.iface/onpacket only briefly under the lock.
🧹 Nitpick comments (1)
client/ui/debug.go (1)
525-551: Reuse the existing daemon client instead of dialing twice.
handleDebugCreationalready hasconnat Line 525, but Line 550 calls into a helper that gets another client. Passingconnthrough reduces duplicate connection logic and keeps one error path.♻️ Proposed refactor
- resp, err := s.createDebugBundle(params.anonymize, params.systemInfo, params.uploadURL) + resp, err := s.createDebugBundle(conn, params.anonymize, params.systemInfo, params.uploadURL)-func (s *serviceClient) createDebugBundle(anonymize bool, systemInfo bool, uploadURL string) (*proto.DebugBundleResponse, error) { - conn, err := s.getSrvClient(failFastTimeout) - if err != nil { - return nil, fmt.Errorf("get client: %v", err) - } - +func (s *serviceClient) createDebugBundle(conn proto.DaemonServiceClient, anonymize bool, systemInfo bool, uploadURL string) (*proto.DebugBundleResponse, error) { request := &proto.DebugBundleRequest{ Anonymize: anonymize, SystemInfo: systemInfo, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` around lines 525 - 551, The code opens a second daemon client by calling s.createDebugBundle which internally dials again; instead, reuse the existing conn obtained by getSrvClient to avoid duplicate dialing and unify error handling: change createDebugBundle (or add a new helper createDebugBundleWithConn) to accept the existing grpc client connection (conn) and use that connection for all RPCs (e.g., Start/StopBundleCapture and bundle creation/upload), then update the caller (the code that currently has conn and calls s.createDebugBundle) to pass conn through; ensure you remove or refactor any internal getSrvClient calls and keep the single connection lifecycle (do not open a new connection or duplicate close logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/capture.go`:
- Around line 115-117: The code currently ignores negative durations because the
check uses d > 0 before setting req.Duration; instead validate the parsed
duration from cmd.Flags().GetDuration("duration") and reject negative values
(and zero if intended) by returning or printing an error for the "--duration"
flag; if valid (d > 0) set req.Duration = durationpb.New(d). Update the logic
around cmd.Flags().GetDuration, req.Duration and durationpb.New to perform the
negative-value check and surface a clear error instead of silently leaving
req.Duration nil.
In `@client/internal/debug/debug.go`:
- Around line 685-700: The addCaptureFile method currently zips the raw capture
at g.capturePath regardless of anonymization; update
BundleGenerator.addCaptureFile to check the bundle config anonymization flag
(BundleConfig.Anonymize / g.cfg.Anonymize) and if anonymization is enabled do
not add the raw capture.pcap directly—instead open the capture, run it through
an anonymization step (e.g., anonymizeCapture or a new helper like
g.anonymizeAndAddCapture that strips payloads/headers/tokens) and pass the
anonymized stream to g.addFileToZip, or if anonymization cannot be applied omit
the capture entirely; keep use of g.capturePath, addFileToZip and add a clear
branch for the anonymize condition.
In `@util/capture/afpacket_linux.go`:
- Around line 14-18: The htons helper currently always swaps bytes which breaks
on big-endian Linux arches; update the htons function to be endian-aware by
detecting native endianness once (e.g., via a runtime.GOARCH switch or a small
init-time check using encoding/binary to inspect a multi-byte value) and then
returning v unchanged on big-endian hosts and performing the byte swap only on
little-endian hosts; update the htons implementation used by Socket/Bind in
afpacket_linux.go (function htons) so protocols are correct on mips, mips64,
ppc64, s390x, sparc64.
---
Duplicate comments:
In `@client/wasm/internal/capture/capture.go`:
- Around line 25-30: Handle.Stop currently unconditionally clears the client's
capture which may tear down a newer session; change Stop to be ownership-aware
by checking the client's current capture before clearing it: call the client's
accessor (e.g., GetCapture or equivalent) and only call h.client.SetCapture(nil)
if the returned capture equals this handle's capture (or this handle) and is
non-nil, then proceed to call h.sess.Stop() and return h.sess.Stats(); keep
using h.client.SetCapture, h.sess.Stop, and h.sess.Stats to locate the relevant
code.
- Around line 167-193: In jsCallbackWriter.Write, the code is invoking the
JavaScript callback (cb.Invoke) while holding w.mu which can deadlock if the
callback re-enters capture; modify Write so that inside the loop you check
w.iface.IsUndefined() and retrieve cb := w.iface.Get("onpacket") while holding
the lock, then store the valid cb and the line in local variables (or append to
a local slice) and release the lock before calling cb.Invoke(js.ValueOf(line));
ensure you never call cb.Invoke while w.mu is held and keep references to
w.iface/onpacket only briefly under the lock.
---
Nitpick comments:
In `@client/ui/debug.go`:
- Around line 525-551: The code opens a second daemon client by calling
s.createDebugBundle which internally dials again; instead, reuse the existing
conn obtained by getSrvClient to avoid duplicate dialing and unify error
handling: change createDebugBundle (or add a new helper
createDebugBundleWithConn) to accept the existing grpc client connection (conn)
and use that connection for all RPCs (e.g., Start/StopBundleCapture and bundle
creation/upload), then update the caller (the code that currently has conn and
calls s.createDebugBundle) to pass conn through; ensure you remove or refactor
any internal getSrvClient calls and keep the single connection lifecycle (do not
open a new connection or duplicate close logic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eec1a0a-a559-480b-93bf-feaa78bda29e
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (41)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (8)
- client/Dockerfile
- client/Dockerfile-rootless
- client/cmd/service_controller.go
- util/capture/afpacket_stub.go
- util/capture/pcap_test.go
- util/capture/capture.go
- util/capture/filter_test.go
- client/server/capture.go
🚧 Files skipped from review as they are similar to previous changes (12)
- client/cmd/root.go
- client/firewall/uspfilter/forwarder/icmp.go
- client/cmd/testutil_test.go
- client/server/setconfig_test.go
- client/cmd/service_params.go
- client/firewall/uspfilter/forwarder/forwarder.go
- client/cmd/service_installer.go
- util/capture/session_test.go
- proxy/internal/debug/client.go
- proxy/cmd/proxy/cmd/debug.go
- client/internal/engine.go
- util/capture/text.go
lixmal
force-pushed
the
add-packet-capture
branch
from
81b63cd to
a4c891c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
client/server/capture.go (1)
120-155:⚠️ Potential issue | 🟠 MajorStreaming and bundle captures can still evict each other.
Engine.SetCaptureonly has one active slot (client/internal/engine.go:2174-2210), but these two RPC paths still install separate sessions without a shared owner/active-capture guard. If one starts while the other is running, it replaces the existing capture, and the older cleanup path later callingSetCapture(nil)tears the newer one down. Please serialize both RPCs behind the same server-side ownership check, or reject overlaps explicitly.Also applies to: 190-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 120 - 155, The two RPC paths install captures that race because engine.SetCapture maintains a single global slot; fix by adding a server-side ownership guard so captures are serialized or overlaps rejected: introduce an owner token/ID field on the server (or on the session struct) and require SetCapture to take/return that owner, so when starting in the RPC that calls engine.SetCapture(sess) you first acquire the server capture mutex or call a new server method (e.g., AcquireCaptureOwner/ReserveCapture) that returns an owner token or an error if occupied, store that token with sess, and when cleaning up (the send-failure path and the ctx.Done goroutine) only call engine.SetCapture(nil) or release the capture if the stored owner matches the current owner; alternatively, make AcquireCapture atomically fail if another capture is active and return a clear error to the client so the RPCs cannot evict each other (refer to engine.SetCapture, sess, the ctx.Done goroutine and the send/error cleanup paths).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/filter.go`:
- Around line 387-390: The new forwarder is being published with
m.forwarder.Store(forwarder) before applying any pending capture, allowing a
race where another goroutine uses the fresh forwarder before
forwarder.SetCapture(...) runs; change the order so you first check
m.pendingCapture.Load(), and if non-nil call forwarder.SetCapture(*pc) and only
then call m.forwarder.Store(forwarder) (use the same pc local variable to avoid
re-reading), ensuring the pending capture is applied before the forwarder is
visible to other goroutines.
In `@client/iface/device/device_filter.go`:
- Around line 81-95: Read() currently calls the capture hook (d.capture.Load())
only after FilterOutbound removes rejected packets, so outbound drops are not
recorded; mirror the inbound path in Write() by invoking the capture Offer for
each outbound buffer before running FilterOutbound. Concretely, in
device_filter.go within Read(), when pc := d.capture.Load() != nil, first loop
over bufs[0:n] and call (*pc).Offer(bufs[i][offset:offset+sizes[i]], true) to
record all outbound candidates, then run the existing FilterOutbound removal
loop that uses filter.FilterOutbound(...) on bufs[i][offset:offset+sizes[i]] and
mutates bufs/sizes/n accordingly; ensure you don’t call Offer twice for the same
packet and keep the existing arguments and semantics.
In `@client/internal/debug/debug.go`:
- Around line 351-353: Update the README generation in createArchive() so
readmeContent documents the new capture.pcap artifact and explains that
anonymized bundles intentionally omit capture.pcap; modify the text assembled in
the readmeContent variable (the function/method that composes README.txt) to
mention "capture.pcap" as a packet capture included when available and add a
note that addCaptureFile() will be skipped for anonymized bundles and thus
capture.pcap will not be present in those bundles.
In `@client/internal/engine.go`:
- Around line 2174-2210: The SetCapture logic currently overwrites and later
unconditionally tears down captures; make it ownership-aware in
Engine.SetCapture by tracking the installed capture owner and refusing to
replace an active capture or only clearing when the caller still owns it. Add an
Engine field (e.g., captureOwner) to record the identity of the installed sink
(use the pc value or an allocated token) when setting a non-nil capture; if a
capture is already active return an error instead of replacing it. When
SetCapture(nil) is called, only clear e.afpacketCapture, dev.SetCapture, and
e.setForwarderCapture if the stored captureOwner matches the caller’s identity,
otherwise leave the active capture intact and return an error. Ensure you set
captureOwner when starting capture (both device and AF_PACKET paths) and clear
it when you legitimately stop that same owner.
In `@client/ui/debug.go`:
- Around line 401-405: The code sends timeout := params.duration +
30*time.Second to conn.StartBundleCapture but the server clamps captures to 10
minutes, causing silent truncation; before calling StartBundleCapture (where
StartBundleCaptureRequest and durationpb.New are used), clamp the computed
timeout to a hard constant maxServerCapture := 10*time.Minute (i.e. if
params.duration+30s > maxServerCapture set timeout = maxServerCapture) and then
pass that clamped timeout to durationpb.New, and also surface the truncation to
the caller/UI (e.g., return an explicit error/flag or log a warning) so users
know the requested duration was reduced.
In `@client/wasm/internal/capture/capture.go`:
- Around line 105-107: The exported stop callback created with js.FuncOf in
iface.Set("stop", js.FuncOf(...)) is never released and leaks on repeated
Start() calls; change it to create and store the js.Func in a variable (e.g.,
stopFunc := js.FuncOf(...)), register that variable with iface.Set("stop",
stopFunc) and then call stopFunc.Release() after invoking handle.Stop() (and/or
when the handle is torn down) so the Go-backed JS callback is freed; ensure any
reference is cleared or removed from iface after release to avoid retaining the
function.
---
Duplicate comments:
In `@client/server/capture.go`:
- Around line 120-155: The two RPC paths install captures that race because
engine.SetCapture maintains a single global slot; fix by adding a server-side
ownership guard so captures are serialized or overlaps rejected: introduce an
owner token/ID field on the server (or on the session struct) and require
SetCapture to take/return that owner, so when starting in the RPC that calls
engine.SetCapture(sess) you first acquire the server capture mutex or call a new
server method (e.g., AcquireCaptureOwner/ReserveCapture) that returns an owner
token or an error if occupied, store that token with sess, and when cleaning up
(the send-failure path and the ctx.Done goroutine) only call
engine.SetCapture(nil) or release the capture if the stored owner matches the
current owner; alternatively, make AcquireCapture atomically fail if another
capture is active and return a clear error to the client so the RPCs cannot
evict each other (refer to engine.SetCapture, sess, the ctx.Done goroutine and
the send/error cleanup paths).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 549863f8-0573-41cd-a868-ee27402b274d
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (41)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (13)
- client/Dockerfile
- client/Dockerfile-rootless
- client/cmd/service_params_test.go
- client/cmd/service_params.go
- client/cmd/service.go
- util/capture/afpacket_stub.go
- util/capture/session_test.go
- util/capture/pcap_test.go
- util/capture/capture.go
- proxy/cmd/proxy/cmd/debug.go
- util/capture/pcap.go
- client/cmd/capture.go
- util/capture/filter_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- client/cmd/service_controller.go
- client/firewall/uspfilter/forwarder/forwarder.go
- client/cmd/root.go
- client/server/setconfig_test.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/cmd/debug.go
- proxy/internal/debug/client.go
- client/proto/daemon.proto
lixmal
force-pushed
the
add-packet-capture
branch
from
a4c891c to
7f0833d
Compare
lixmal
force-pushed
the
add-packet-capture
branch
from
7f0833d to
8ad7347
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
client/ui/debug.go (2)
62-62: Consider disabling capture checkbox when anonymize is checked.The
captureCheckremains enabled even whenanonymizeCheckis checked, but the backend will silently skip adding capture data to anonymized bundles. This could confuse users who expect their capture to be included.You could add an
OnChangedhandler toanonymizeCheckthat disables/enablescaptureCheckaccordingly, or show a tooltip explaining the incompatibility.Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` at line 62, The capture checkbox (captureCheck) can remain enabled even when anonymizeCheck is selected, causing user confusion; add an OnChanged handler on anonymizeCheck that disables captureCheck when anonymizeCheck is true (and re-enables it when false), optionally uncheck captureCheck when disabling, and set a tooltip on captureCheck explaining that packet capture is incompatible with anonymized bundles; apply the same change for the other capture/anonymize pair referenced around the second occurrence (lines ~90) and use methods like OnChanged, SetEnabled, SetChecked, and SetTooltip to implement this behavior.
538-553: Verify capture has meaningful data with only 2s collection window.When capture is enabled without the duration mode, the flow is:
- Start capture with 30s timeout
- Sleep 2 seconds
- Create debug bundle (which stops capture internally)
The 2-second window may not capture meaningful traffic depending on network activity. Consider documenting this limitation or adjusting the sleep duration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ui/debug.go` around lines 538 - 553, The current capture flow (when params.capture is true) starts a 30s capture via conn.StartBundleCapture, then only waits time.Sleep(2 * time.Second) before creating/stopping the bundle, which may yield insufficient data; either increase the sleep duration to a more realistic collection window or make the wait configurable (e.g., a captureDuration variable/flag) and document the behavior; update the block around conn.StartBundleCapture / defer stopCtx cancel / time.Sleep to use the new configurable duration or add a comment explaining the 2s limitation so callers know captures may be incomplete.util/capture/filter.go (1)
234-248: Prefer a concrete match-all matcher overnil.Encoding “match all” as a
nilMatchermakes every caller remember an extra nil branch before it can safely use the interface. Returning a trivial matcher keeps the API uniform and removes an easy nil-deref footgun for future call sites.♻️ Proposed refactor
+type matchAllMatcher struct{} + +func (matchAllMatcher) Match([]byte) bool { return true } + func ParseFilter(expr string) (Matcher, error) { tokens := tokenize(expr) if len(tokens) == 0 { - return nil, nil //nolint:nilnil // nil Matcher means "match all" + return matchAllMatcher{}, nil } p := &parser{tokens: tokens} node, err := p.parseOr()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/filter.go` around lines 234 - 248, The ParseFilter function should return a concrete "match all" Matcher instead of nil when the expression is empty: add a trivial allMatcher type that implements Matcher (always returns true) and return &allMatcher{} in the len(tokens) == 0 branch of ParseFilter; update ParseFilter to return that instance (not nil) and adjust any tests or callers that currently expect nil to use the concrete matcher, keeping the rest of parse logic (parser.parseOr, exprMatcher) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/firewall/uspfilter/filter.go`:
- Around line 387-391: The current init path reads m.pendingCapture
(m.pendingCapture.Load()) into pc and calls forwarder.SetCapture(*pc) before
m.forwarder.Store(forwarder), which can miss concurrent SetPacketCapture calls
that happen between those operations; after storing the forwarder, re-check
m.pendingCapture.Load() and, if it's non-nil (and/or differs from the earlier
pc), call forwarder.SetCapture(...) again so any capture update that raced in
between is applied; reference the pendingCapture, SetPacketCapture,
forwarder.SetCapture, and m.forwarder.Store symbols when making this change.
In `@client/wasm/internal/capture/capture.go`:
- Around line 42-59: parseOpts currently assumes non-null/undefined non-string
js.Value is an object and panics when it's not; change parseOpts(jsOpts
js.Value) to validate the type and return an error for invalid types (e.g.,
accept string or object only, otherwise return a descriptive error), adjust its
signature to return (filter string, verbose, ascii bool, err error), and update
both callers Start and StartConsole to handle the returned error (propagate or
convert to the existing error-reporting path instead of panicking). Ensure you
reference and modify the parseOpts function and the Start and StartConsole call
sites to check and handle err from parseOpts.
In `@util/capture/afpacket_linux.go`:
- Around line 53-83: The Start method currently allows multiple starts causing
reuse of the same stopped channel and duplicate readLoop goroutines; update
AFPacketCapture.Start to first check c.started.Load() and return an error if
already started (reject duplicate starts) and also reject starts if the instance
has been stopped previously (e.g., if c.stopped != nil and closed or if you
track a final state), initialize a fresh stopped channel inside Start (c.stopped
= make(chan struct{})) only on the first successful start, set
c.started.Store(true) after successful setup, and ensure Stop closes and clears
the stopped channel and sets started to false/terminal so subsequent Start calls
are rejected; reference AFPacketCapture.Start, AFPacketCapture.Stop, the stopped
channel, started atomic, readLoop, fd and mu when making the changes.
In `@util/capture/text.go`:
- Around line 181-196: The current writeICMPv4 treats every ICMPv4 packet as an
echo and always prints id/seq, which is wrong for non-echo types; update
writeICMPv4 to check icmp.TypeCode.Type() (use layers.ICMPv4TypeEchoRequest and
layers.ICMPv4TypeEchoReply) and only append ", id %d, seq %d" using icmp.Id and
icmp.Seq when the type is Echo Request or Echo Reply, otherwise set detail to
just icmp.TypeCode.String(); keep the rest of the formatting the same and avoid
accessing Id/Seq for non-echo messages.
---
Nitpick comments:
In `@client/ui/debug.go`:
- Line 62: The capture checkbox (captureCheck) can remain enabled even when
anonymizeCheck is selected, causing user confusion; add an OnChanged handler on
anonymizeCheck that disables captureCheck when anonymizeCheck is true (and
re-enables it when false), optionally uncheck captureCheck when disabling, and
set a tooltip on captureCheck explaining that packet capture is incompatible
with anonymized bundles; apply the same change for the other capture/anonymize
pair referenced around the second occurrence (lines ~90) and use methods like
OnChanged, SetEnabled, SetChecked, and SetTooltip to implement this behavior.
- Around line 538-553: The current capture flow (when params.capture is true)
starts a 30s capture via conn.StartBundleCapture, then only waits time.Sleep(2 *
time.Second) before creating/stopping the bundle, which may yield insufficient
data; either increase the sleep duration to a more realistic collection window
or make the wait configurable (e.g., a captureDuration variable/flag) and
document the behavior; update the block around conn.StartBundleCapture / defer
stopCtx cancel / time.Sleep to use the new configurable duration or add a
comment explaining the 2s limitation so callers know captures may be incomplete.
In `@util/capture/filter.go`:
- Around line 234-248: The ParseFilter function should return a concrete "match
all" Matcher instead of nil when the expression is empty: add a trivial
allMatcher type that implements Matcher (always returns true) and return
&allMatcher{} in the len(tokens) == 0 branch of ParseFilter; update ParseFilter
to return that instance (not nil) and adjust any tests or callers that currently
expect nil to use the concrete matcher, keeping the rest of parse logic
(parser.parseOr, exprMatcher) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e436252e-dcab-421c-8536-0148d53a6631
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (41)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (13)
- client/Dockerfile-rootless
- client/cmd/service_installer.go
- client/Dockerfile
- client/cmd/root.go
- client/server/setconfig_test.go
- client/embed/embed.go
- client/cmd/service_params_test.go
- client/cmd/testutil_test.go
- client/cmd/service_controller.go
- client/cmd/service.go
- util/capture/pcap_test.go
- util/capture/afpacket_stub.go
- client/cmd/capture.go
🚧 Files skipped from review as they are similar to previous changes (11)
- client/firewall/uspfilter/forwarder/forwarder.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/cmd/debug.go
- util/capture/capture.go
- proxy/cmd/proxy/cmd/debug.go
- proxy/internal/debug/client.go
- client/proto/daemon.proto
- client/internal/engine.go
- util/capture/session.go
- util/capture/pcap.go
- client/server/capture.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/iface/device/device_filter.go (1)
118-128:⚠️ Potential issue | 🔴 CriticalBug:
droppedcounter increments on wrong branch, causing double-counted return value.The
dropped++statement is inside the branch that keeps packets (adds tofilteredBufs), not the branch that drops them. This causesn += droppedto double-count.Example: 4 input packets, 1 filtered →
filteredBufshas 3,dropped= 3, Write returns 3, finaln= 6 instead of 4.🐛 Proposed fix
filteredBufs := make([][]byte, 0, len(bufs)) dropped := 0 for _, buf := range bufs { - if !filter.FilterInbound(buf[offset:], len(buf)) { + if filter.FilterInbound(buf[offset:], len(buf)) { + dropped++ + } else { filteredBufs = append(filteredBufs, buf) - dropped++ } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/iface/device/device_filter.go` around lines 118 - 128, The dropped counter is being incremented in the wrong branch—move the dropped++ so it increments when a packet is filtered out (i.e., when filter.FilterInbound(...) returns true and you do NOT append to filteredBufs) rather than when you keep a packet; update the loop over bufs (using buf, offset, filter.FilterInbound, filteredBufs) so dropped reflects the number of dropped packets and then keep the existing n, err := d.Device.Write(filteredBufs, offset) followed by n += dropped to compute the correct total written count.
♻️ Duplicate comments (4)
client/firewall/uspfilter/filter.go (1)
387-391:⚠️ Potential issue | 🟡 MinorClose the remaining capture update race during forwarder publish.
Between Line 387 and Line 391, a concurrent
SetPacketCapturecan store a newer value whilem.forwarder.Load()is still nil there, and that update is missed until the next toggle.🔧 Proposed fix
- if pc := m.pendingCapture.Load(); pc != nil { - forwarder.SetCapture(*pc) - } - - m.forwarder.Store(forwarder) + pc := m.pendingCapture.Load() + if pc != nil { + forwarder.SetCapture(*pc) + } + + m.forwarder.Store(forwarder) + + // Apply updates that raced between initial Load and Store. + if latest := m.pendingCapture.Load(); latest != pc { + if latest != nil { + forwarder.SetCapture(*latest) + } else { + forwarder.SetCapture(nil) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/firewall/uspfilter/filter.go` around lines 387 - 391, The code may miss a concurrent SetPacketCapture that occurs between reading m.pendingCapture and calling m.forwarder.Store(forwarder); after storing the forwarder you must re-check pending capture and apply any newer value so no update is lost. Modify the publish path around m.pendingCapture.Load()/forwarder.SetCapture(...) and m.forwarder.Store(forwarder) (symbols: m.pendingCapture.Load, forwarder.SetCapture, m.forwarder.Store, SetPacketCapture) to: read pendingCapture into pc, if non-nil call forwarder.SetCapture(pc), store the forwarder with m.forwarder.Store(forwarder), then immediately reload m.pendingCapture and if it is non-nil and not the same pointer/value you applied earlier call forwarder.SetCapture again with the latest value — ensuring the latest capture is always applied even if SetPacketCapture ran concurrently.util/capture/afpacket_linux.go (1)
53-83:⚠️ Potential issue | 🟠 MajorReject duplicate or restarted
Start()calls.This instance is still reusable from the API surface, but its internals are single-use:
stoppedis shared for the lifetime of the object andreadLoop()always closes it. A secondStart()can overwritec.fd, leak the first socket, and panic on shutdown withclose of closed channel. Starting again afterStop()is also unsafe becauseclosedis terminal but not checked here.🛠️ Suggested guard
func (c *AFPacketCapture) Start() error { + if c.closed.Load() { + return errors.New("capture already stopped") + } + if !c.started.CompareAndSwap(false, true) { + return errors.New("capture already started") + } if c.sess == nil { + c.started.Store(false) return errors.New("nil capture session") } iface, err := net.InterfaceByName(c.ifaceName) if err != nil { + c.started.Store(false) return fmt.Errorf("interface %s: %w", c.ifaceName, err) } fd, err := unix.Socket(unix.AF_PACKET, unix.SOCK_DGRAM|unix.SOCK_NONBLOCK|unix.SOCK_CLOEXEC, int(htons(unix.ETH_P_ALL))) if err != nil { + c.started.Store(false) return fmt.Errorf("create AF_PACKET socket: %w", err) } @@ if err := unix.Bind(fd, addr); err != nil { unix.Close(fd) + c.started.Store(false) return fmt.Errorf("bind to %s: %w", c.ifaceName, err) } @@ - c.started.Store(true) go c.readLoop(fd) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/afpacket_linux.go` around lines 53 - 83, Prevent duplicate or restarted starts by guarding AFPacketCapture.Start: acquire c.mu at the top, check c.started (or a boolean) and return an error like "already started" if true, and check c.stopped (or whether the stopped channel was closed/terminal) and return an error like "cannot restart stopped capture" if set; only after those checks proceed to create the socket, bind, assign c.fd and set c.started.Store(true), then launch readLoop; ensure you never overwrite c.fd without closing the previous fd and that Start returns on error without leaving resources open. Use the AFPacketCapture.Start, c.mu, c.started, c.stopped, c.fd and readLoop/Stop symbols to locate and implement these checks.util/capture/text.go (1)
181-196:⚠️ Potential issue | 🟡 MinorOnly print
id/seqfor echo ICMPv4 packets.For destination-unreachable, time-exceeded, and other non-echo ICMPv4 messages, bytes 4-7 are type-specific payload, not an echo identifier/sequence. The current formatter will print bogus values and make the capture output misleading.
Suggested fix
- detail := fmt.Sprintf("%s, id %d, seq %d", - icmp.TypeCode.String(), icmp.Id, icmp.Seq) + detail := icmp.TypeCode.String() + switch icmp.TypeCode.Type() { + case layers.ICMPv4TypeEchoRequest, layers.ICMPv4TypeEchoReply: + detail = fmt.Sprintf("%s, id %d, seq %d", detail, icmp.Id, icmp.Seq) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/capture/text.go` around lines 181 - 196, The formatter currently always prints icmp.Id and icmp.Seq even for non-echo messages; update writeICMPv4 to only append "id %d, seq %d" when the ICMP type is Echo Request or Echo Reply (check icmp.TypeCode.Type() against layers.ICMPv4TypeEchoRequest and layers.ICMPv4TypeEchoReply), otherwise just print icmp.TypeCode.String() (and do not reference icmp.Id/icm.Seq); adjust the detail variable construction in writeICMPv4 accordingly so destination-unreachable/time-exceeded/etc. don’t show bogus id/seq values.client/server/capture.go (1)
49-55:⚠️ Potential issue | 🟠 MajorGuard the single engine capture slot across both RPCs.
StartCaptureandStartBundleCapturestill install independent sessions into the sameengine.SetCapture(...)slot, andbundleCapture.stop()later clears that slot unconditionally. A running CLI capture can therefore be replaced by a bundle capture (or vice versa), and whichever one stops first tears the other down. This needs a server-wide owner/active-capture guard with a clearAlreadyExists/FailedPreconditionerror instead of silently overwriting the other session.Also applies to: 120-125, 222-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/server/capture.go` around lines 49 - 55, StartCapture and StartBundleCapture both write into the same engine.SetCapture slot and bundleCapture.stop unconditionally clears it; add a server-wide active-capture owner guard (e.g., an activeCapture owner ID or pointer stored on the server struct) that is checked/locked before calling engine.SetCapture in both StartCapture and StartBundleCapture, return a clear AlreadyExists/FailedPrecondition error when another owner exists, and modify bundleCapture.stop (and the Stop path that currently calls bc.engine.SetCapture(nil)) to only clear the engine slot if the stored owner matches the current session (and also clear the owner atomically under the same lock); reference the functions StartCapture, StartBundleCapture, bundleCapture.stop, engine.SetCapture, and the bc.engine/bc.sess usage when applying the guard and conditional-clear logic.
🧹 Nitpick comments (2)
client/wasm/cmd/main.go (1)
532-548: Return capture stats fromstopCapture()for scriptability.At Line 541-Line 547 you already compute stats; returning them (instead of only logging) would make browser-side automation/tests easier.
💡 Minimal tweak
stats := active.Stop() active = nil console := js.Global().Get("console") console.Call("log", fmt.Sprintf("[capture] stopped: %d packets, %d bytes, %d dropped", stats.Packets, stats.Bytes, stats.Dropped)) - return js.Undefined() + return js.ValueOf(map[string]any{ + "packets": stats.Packets, + "bytes": stats.Bytes, + "dropped": stats.Dropped, + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/wasm/cmd/main.go` around lines 532 - 548, The stopFn (stopCapture) currently only logs capture stats; change it to return the stats to JS so scripts/tests can consume them: after stats := active.Stop() build a JS-returnable value (e.g. via js.ValueOf of a Go map/object with fields Packets, Bytes, Dropped or by constructing a JS object) and return that instead of js.Undefined(); keep the existing behavior when active == nil (return js.Undefined() or null) and leave logging in place if desired.proxy/internal/debug/handler.go (1)
713-723: Read final capture stats after teardown.
Stats()is sampled beforeclient.SetCapture(nil)andsess.Stop()run, so the logged packet/byte/drop counts can miss packets still arriving during shutdown. Detaching the capture first will make the completion log deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/internal/debug/handler.go` around lines 713 - 723, The final capture stats are read too early; move the call to sess.Stats() until after the capture is detached and the session stopped so counts include packets arriving during teardown — i.e., after the wait/select completes call client.SetCapture(nil) and sess.Stop() (or any teardown on the session) first, then call sess.Stats() and log via h.logger.Infof; reference the existing symbols timer, r.Context(), client.SetCapture(nil), sess.Stop(), sess.Stats(), and h.logger.Infof to locate and reorder the operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/capture.go`:
- Around line 151-178: captureOutput currently returns a cleanup func that only
prints errors so runCapture still exits 0 even if closing/renaming fails; change
captureOutput's cleanup to return an error (signature: captureOutput(cmd
*cobra.Command) (io.Writer, func() error, error)), have that cleanup propagate
real errors instead of printing, and update callers (e.g., runCapture and the
other similar helper around lines 86-98) to call the cleanup, check its error,
and return a non-zero error when close/rename fails so output finalization
failures are propagated to the CLI caller.
In `@client/cmd/debug.go`:
- Around line 276-285: If stopping the packet capture fails, do not proceed to
create the bundle; update the capture shutdown logic in the code that calls
StopBundleCapture (the captureStarted block) so that on error it returns/aborts
instead of continuing to call DebugBundle(), and only allow bundle creation when
StopBundleCapture succeeded and captureStarted is set to false; reference
StopBundleCapture, captureStarted, DebugBundle(), and the fallback stop logic to
ensure the fallback is still executed later but only after an unsuccessful
bundle creation or exit.
In `@client/server/capture.go`:
- Around line 138-145: Reject negative durations immediately: after extracting d
:= req.GetDuration() and converting dur := d.AsDuration(), if dur < 0 return a
gRPC error with codes.InvalidArgument before proceeding to session
installation/readiness, before calling engine.SetCapture(sess) and before
sending the empty acceptance frame; keep the existing dur > 0 logic for creating
the timeout context but ensure negative values are explicitly handled and cause
an early return with the InvalidArgument error.
---
Outside diff comments:
In `@client/iface/device/device_filter.go`:
- Around line 118-128: The dropped counter is being incremented in the wrong
branch—move the dropped++ so it increments when a packet is filtered out (i.e.,
when filter.FilterInbound(...) returns true and you do NOT append to
filteredBufs) rather than when you keep a packet; update the loop over bufs
(using buf, offset, filter.FilterInbound, filteredBufs) so dropped reflects the
number of dropped packets and then keep the existing n, err :=
d.Device.Write(filteredBufs, offset) followed by n += dropped to compute the
correct total written count.
---
Duplicate comments:
In `@client/firewall/uspfilter/filter.go`:
- Around line 387-391: The code may miss a concurrent SetPacketCapture that
occurs between reading m.pendingCapture and calling
m.forwarder.Store(forwarder); after storing the forwarder you must re-check
pending capture and apply any newer value so no update is lost. Modify the
publish path around m.pendingCapture.Load()/forwarder.SetCapture(...) and
m.forwarder.Store(forwarder) (symbols: m.pendingCapture.Load,
forwarder.SetCapture, m.forwarder.Store, SetPacketCapture) to: read
pendingCapture into pc, if non-nil call forwarder.SetCapture(pc), store the
forwarder with m.forwarder.Store(forwarder), then immediately reload
m.pendingCapture and if it is non-nil and not the same pointer/value you applied
earlier call forwarder.SetCapture again with the latest value — ensuring the
latest capture is always applied even if SetPacketCapture ran concurrently.
In `@client/server/capture.go`:
- Around line 49-55: StartCapture and StartBundleCapture both write into the
same engine.SetCapture slot and bundleCapture.stop unconditionally clears it;
add a server-wide active-capture owner guard (e.g., an activeCapture owner ID or
pointer stored on the server struct) that is checked/locked before calling
engine.SetCapture in both StartCapture and StartBundleCapture, return a clear
AlreadyExists/FailedPrecondition error when another owner exists, and modify
bundleCapture.stop (and the Stop path that currently calls
bc.engine.SetCapture(nil)) to only clear the engine slot if the stored owner
matches the current session (and also clear the owner atomically under the same
lock); reference the functions StartCapture, StartBundleCapture,
bundleCapture.stop, engine.SetCapture, and the bc.engine/bc.sess usage when
applying the guard and conditional-clear logic.
In `@util/capture/afpacket_linux.go`:
- Around line 53-83: Prevent duplicate or restarted starts by guarding
AFPacketCapture.Start: acquire c.mu at the top, check c.started (or a boolean)
and return an error like "already started" if true, and check c.stopped (or
whether the stopped channel was closed/terminal) and return an error like
"cannot restart stopped capture" if set; only after those checks proceed to
create the socket, bind, assign c.fd and set c.started.Store(true), then launch
readLoop; ensure you never overwrite c.fd without closing the previous fd and
that Start returns on error without leaving resources open. Use the
AFPacketCapture.Start, c.mu, c.started, c.stopped, c.fd and readLoop/Stop
symbols to locate and implement these checks.
In `@util/capture/text.go`:
- Around line 181-196: The formatter currently always prints icmp.Id and
icmp.Seq even for non-echo messages; update writeICMPv4 to only append "id %d,
seq %d" when the ICMP type is Echo Request or Echo Reply (check
icmp.TypeCode.Type() against layers.ICMPv4TypeEchoRequest and
layers.ICMPv4TypeEchoReply), otherwise just print icmp.TypeCode.String() (and do
not reference icmp.Id/icm.Seq); adjust the detail variable construction in
writeICMPv4 accordingly so destination-unreachable/time-exceeded/etc. don’t show
bogus id/seq values.
---
Nitpick comments:
In `@client/wasm/cmd/main.go`:
- Around line 532-548: The stopFn (stopCapture) currently only logs capture
stats; change it to return the stats to JS so scripts/tests can consume them:
after stats := active.Stop() build a JS-returnable value (e.g. via js.ValueOf of
a Go map/object with fields Packets, Bytes, Dropped or by constructing a JS
object) and return that instead of js.Undefined(); keep the existing behavior
when active == nil (return js.Undefined() or null) and leave logging in place if
desired.
In `@proxy/internal/debug/handler.go`:
- Around line 713-723: The final capture stats are read too early; move the call
to sess.Stats() until after the capture is detached and the session stopped so
counts include packets arriving during teardown — i.e., after the wait/select
completes call client.SetCapture(nil) and sess.Stop() (or any teardown on the
session) first, then call sess.Stats() and log via h.logger.Infof; reference the
existing symbols timer, r.Context(), client.SetCapture(nil), sess.Stop(),
sess.Stats(), and h.logger.Infof to locate and reorder the operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceffe03a-5a24-4022-ba3d-e023e512e995
⛔ Files ignored due to path filters (2)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (41)
client/Dockerfileclient/Dockerfile-rootlessclient/cmd/capture.goclient/cmd/debug.goclient/cmd/root.goclient/cmd/service.goclient/cmd/service_controller.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.goclient/cmd/testutil_test.goclient/embed/embed.goclient/firewall/uspfilter/filter.goclient/firewall/uspfilter/forwarder/endpoint.goclient/firewall/uspfilter/forwarder/forwarder.goclient/firewall/uspfilter/forwarder/icmp.goclient/iface/device/device_filter.goclient/internal/debug/debug.goclient/internal/engine.goclient/proto/daemon.protoclient/server/capture.goclient/server/debug.goclient/server/server.goclient/server/server_test.goclient/server/setconfig_test.goclient/ui/debug.goclient/wasm/cmd/main.goclient/wasm/internal/capture/capture.goproxy/cmd/proxy/cmd/debug.goproxy/internal/debug/client.goproxy/internal/debug/handler.goutil/capture/afpacket_linux.goutil/capture/afpacket_stub.goutil/capture/capture.goutil/capture/filter.goutil/capture/filter_test.goutil/capture/pcap.goutil/capture/pcap_test.goutil/capture/session.goutil/capture/session_test.goutil/capture/text.go
✅ Files skipped from review due to trivial changes (8)
- client/cmd/testutil_test.go
- client/cmd/service_installer.go
- client/Dockerfile-rootless
- util/capture/pcap_test.go
- util/capture/session_test.go
- util/capture/afpacket_stub.go
- util/capture/pcap.go
- util/capture/capture.go
🚧 Files skipped from review as they are similar to previous changes (18)
- client/Dockerfile
- client/cmd/service_params_test.go
- client/server/setconfig_test.go
- client/cmd/root.go
- client/cmd/service_controller.go
- client/firewall/uspfilter/forwarder/forwarder.go
- client/firewall/uspfilter/forwarder/icmp.go
- client/server/server_test.go
- client/cmd/service_params.go
- client/firewall/uspfilter/forwarder/endpoint.go
- client/proto/daemon.proto
- proxy/cmd/proxy/cmd/debug.go
- client/wasm/internal/capture/capture.go
- util/capture/session.go
- util/capture/filter.go
- client/internal/engine.go
- client/server/server.go
- util/capture/filter_test.go
lixmal
force-pushed
the
add-packet-capture
branch
4 times, most recently
from
b630283 to
abacbbd
Compare
lixmal
force-pushed
the
add-packet-capture
branch
from
abacbbd to
e58c29d
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Add packet capture for decrypted WireGuard traffic. Captures can be streamed live via CLI or included in debug bundles.
Capture methods:
--enable-captureservice flag for the streaming CLI; bundle capture is always availableProduction code footprint: the only code that runs in the normal packet path is a single
atomic.Pointer.Loadnil check in FilteredDevice and the forwarder endpoint. Everything else is only active when a capture session is running.CLI (
netbird debug capture):--pcap) pipeable to tcpdump/Wiresharkhost 10.0.0.1 and tcp port 443,not port 22,net 10.0.0.0/24-v) adds relative seq/ack, TCP options, TTLDebug bundle:
StartBundleCapture/StopBundleCapturegRPC with auto-stop timeoutAlso included:
util/capturepackage with session management, pcap writer, text formatter, and filter parserIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Chores
Tests