Skip to content

feat(pcap): add --timeout and clean Ctrl-C shutdown to ios pcap - #800

Open
danielpaulus wants to merge 2 commits into
mainfrom
feat/issue-487-pcap-timeout
Open

feat(pcap): add --timeout and clean Ctrl-C shutdown to ios pcap#800
danielpaulus wants to merge 2 commits into
mainfrom
feat/issue-487-pcap-timeout

Conversation

@danielpaulus

Copy link
Copy Markdown
Owner

Problem

ios pcap captured forever: the read loop in ios/pcap/pcap.go had no cancellation mechanism, so the only way to end a capture was to kill the process. Issue #487 asks for collecting a pcap for a limited time.

Design

  • pcap.Start now takes a context.Context. The capture loop is extracted into capture(ctx, conn, w), which reads pcapd frames and streams pcap records to the writer until the context is done or the connection fails.
  • Since the plist decode blocks on a socket read, cancellation works by closing the service connection when the context fires, which unblocks the pending read. The loop then maps that read error to a clean nil return (ctx.Err() != nil), so the file is finalized (synced + closed) and remains a valid pcap.
  • capture depends only on a tiny captureConn interface (Reader()/Close()) satisfied by ios.DeviceConnectionInterface, and writes to an io.Writer — this is what makes the loop unit-testable without a device.
  • CLI: ios pcap [--timeout=<duration>] (Go duration syntax: 30s, 2m, 1h). The command always installs a signal.NotifyContext for SIGINT/SIGTERM, so Ctrl-C now also ends the capture cleanly instead of killing the process mid-write; --timeout layers a context.WithTimeout on top.

Packets are still written to the file incrementally as they arrive (streaming behavior unchanged), and a real connection error (context still active) is still surfaced as an error.

Implementation

  • ios/pcap/pcap.go: context-aware Start, new capture loop with a watchdog goroutine that closes the connection on ctx.Done(); writePacket generalized to io.Writer with write errors checked; pcap global header factored into writePcapHeader (reused by tests); f.Sync() on clean stop.
  • cmd_device_debug.go: runPCAPCommand parses --timeout, builds the signal + timeout context, calls pcap.Start(ctx, device).
  • main.go: docopt usage and help text for --timeout=<duration>.
  • ios/pcap/pcap_test.go: new device-free tests (see test plan).

Options considered

  1. Close-connection-on-cancel (chosen): a goroutine closes the pcapd connection when the context is done; the blocked read errors out and the loop treats it as a clean stop. Preferred because it needs no read deadlines or protocol changes, stops immediately even when the device is idle (no packet traffic), and matches how other go-ios commands already stop blocking services.
  2. SetReadDeadline polling: set short deadlines on the underlying net.Conn and check the context between reads. Rejected: capture would be tied to a concrete net.Conn (worse testability), and it burns wakeups while idle.
  3. CLI-side time.AfterFunc(os.Exit): trivial, but exits mid-write and can truncate the pcap record — exactly what this PR is meant to avoid, and it leaves the library API uncancelable for embedders.

Test plan

  • go build ./... and full go test ./... pass; go test -race ./ios/pcap/ passes; gofmt -l / go vet clean on changed files.
  • New unit tests drive capture against an in-memory fake pcapd connection (io.Pipe) feeding canned wire frames (length-prefixed binary plist wrapping a struc-packed IOSPacketHeader + payload):
    • packets written until context cancellation, then the loop returns nil; output validated as a well-formed pcap with pcapgo (payloads, lengths, timestamps, clean EOF),
    • deadline expiry stops a read blocked on an idle connection,
    • a connection error with an active context is still returned as an error.
  • Fixture tests for the existing parsing: fromBytes (plist → bytes), getPacket (header fields, fake-ethernet prefix when FramePreLength == 0, iOS 15 beta4 extended header skip, pid/process-name filters).
  • Real-device e2e suite to be run via /test-devices.

Fixes #487

🤖 Generated with Claude Code

https://claude.ai/code/session_01J8eMENxJ1nec9CeHp4tjWk

The pcap capture loop ran forever with no way to stop it other than
killing the process, which was the only way to end a capture and risked
truncated output. Make the loop context-aware: 'ios pcap' now accepts
--timeout=<duration> (e.g. 30s, 2m) and also stops cleanly on
Ctrl-C/SIGTERM. On cancellation the blocked read is unblocked by closing
the service connection and the pcap file is flushed and closed, leaving
a valid capture file. Packets are still streamed to the file as they
arrive.

Adds device-free unit tests driving the capture loop with an in-memory
fake pcapd connection (cancellation, deadline, and error paths; output
validated with pcapgo) plus fixture tests for the pcapd frame parsing
(fromBytes/getPacket).

Fixes #487

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J8eMENxJ1nec9CeHp4tjWk
@danielpaulus

Copy link
Copy Markdown
Owner Author

/test-devices

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🧪 Running real-device tests on PR #800run.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

❌ Real-device tests failed — see run.

…le-close

The context watchdog in capture() closed the connection on cancellation
while Start() also deferred its own close of the same connection. Because
capture() returned as soon as the read unblocked, the watchdog's Close()
could still be running when Start()'s deferred Close() fired, closing the
same connection from two goroutines. It also classified any read error as
a clean stop whenever ctx was done, so a genuine error coinciding with
cancellation was nondeterministically suppressed.

capture() now waits for the watchdog goroutine to finish before returning,
so the watchdog's Close() completes before the caller's deferred close runs,
and only reports a clean stop when the watchdog (not an unrelated failure)
closed the connection. Adds a test proving capture waits for the watchdog's
Close and closes the connection exactly once.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J8eMENxJ1nec9CeHp4tjWk
@danielpaulus

Copy link
Copy Markdown
Owner Author

Adversarial review (+ Codex cross-check) done. Verdict: solid PR — one design defect fixed, rest confirmed sound.

Fixed (pushed to this branch):

  • Concurrent double-close / error-classification race in capture(). The watchdog goroutine closed the connection on cancel, but capture() returned as soon as the read unblocked — so the watchdog’s conn.Close() could still be running when Start()’s defer intf.Close() fired, closing the same conn from two goroutines. (Benign on *net.TCPConn, which is close-safe, but not guaranteed by the captureConn contract.) The old ctx.Err() != nil check also suppressed a genuine read error whenever it happened to coincide with cancellation. capture() now waits for the watchdog goroutine before returning (watchdog owns the close; the deferred close never overlaps it) and only reports a clean stop when the watchdog — not an unrelated failure — closed the conn. Added TestCaptureWaitsForWatchdogCloseBeforeReturning proving capture blocks until the watchdog’s Close() completes and closes the conn exactly once.

Reviewed and dismissed:

  • Sync only on success path — on error paths the deferred f.Close() still finalizes a structurally valid pcap, and returning the original error is correct; not worth changing.
  • Zero-packet file validity — global header is written in createPcap(), so a 0-packet capture is a valid empty pcap. Fine.
  • Signature change Start(ctx, device) — only in-repo caller is cmd_device_debug.go (updated); restapi does not import pcap.Start. No broken callers.

Test status: go build ./... clean, go test ./... green, go test -race ./ios/pcap/ clean (also verified over 100 iterations), gofmt -l clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pcap collection for specific time

1 participant