Repository navigation
fix(gvisor): preserve trace connections across interrupted reads - #1052
Conversation
Signed-off-by: Daksh Pathak <daksh.pathak.ug24@nsut.ac.in>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Receiver retries and counters pkg/gvisor/stats_linux.go, pkg/gvisor/receiver_linux.go, pkg/gvisor/interrupted_read_linux_test.go, pkg/gvisor/receiver_linux_test.go |
The receiver adds atomic counters for connection, handshake, frame, read, callback, start-resolution, and drop activity. Packet reads retry EINTR, check cancellation, and bound receive timeouts by the context deadline. Tests cover interrupted reads, cancellation, deadline behavior, invalid frames, and queue drops. |
Probe diagnostics output cmd/gvisor-start-probe/main_linux.go, docs/gvisor-start-probe.md |
The probe adds a disabled-by-default -diagnostics flag. When enabled, it writes receiver statistics as JSON to stderr after receiver.Run returns. The documentation explains counter meanings and limits. |
Signal tests and runsc trial evidence docs/experiments/gvisor-start-loss-20261026/README.md, docs/experiments/gvisor-start-loss-20261026/*.py, docs/experiments/gvisor-start-loss-20261026/evidence/*, docs/experiments/gvisor-start-loss-20261026/evidence-sha256.json |
The experiment scripts inject signals or run repeated starts while collecting diagnostics. The report records two 20-of-20 runsc trials and explains that the controlled reproduction does not establish the cause of the historical missing start. It also records validation limits. |
Node-agent runtime comparison
| Layer / File(s) | Summary |
|---|---|
Comparison setup and evidence docs/experiments/gvisor-start-loss-20261026/comparison-values.yaml, docs/experiments/gvisor-start-loss-20261026/comparison-workloads.json, docs/experiments/gvisor-start-loss-20261026/evidence/node-agent-comparison.json, docs/experiments/gvisor-start-loss-20261026/README.md |
The comparison defines ordinary and gVisor BusyBox workloads and records node-agent results. The report marks the result inconclusive because the ordinary positive control had no retained exec records and the gVisor workload had no final profile. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Possibly related issues
kubescape/kubescape#3424— The issue concerns real-time runsc trace ingestion; this change retries interrupted reads and handshake responses in the gVisor trace receiver.
Possibly related PRs
- kubescape/node-agent#1046 — It adds repeated ID-based start trials and an evidence checker used by this pull request’s investigation.
- kubescape/node-agent#1051 — It records the earlier 19-of-20 trial that this pull request investigates with receiver retries and replacement trials.
Suggested reviewers: matthyx
Merge Risk: 
4e9d7
-
No actionable merge-blocking issue is established. The remaining runtime-visibility comparison is explicitly inconclusive, so it should not be treated as validation of an integration claim.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 7 files. (18 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(gvisor): preserve trace connections across interrupted reads" matches the main change. The receiver now retries reads and handshake sends interrupted by signals such as SIGURG, so valid… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(gvisor): preserve trace connections across interrupted reads" matches the main change. The receiver now retries reads and handshake sends interrupted by signals such as SIGURG, so valid trace connections are no longer closed by an interrupted timed Recvmsg. The title is specific and concise. It does not mention the opt-in --diagnostics flag, but that is secondary and not required to convey the primary change.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 7 files. (18 skipped: 18 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Approve 4e9d7a2318042771f8a587adb54f29dbcd39b4af against main at 3608bcc6e188623d5e15948c690df51ae6cf865e. No material blockers found. Independent code review recommends APPROVE; architecture review is CLEAR.
The fix is needed: target-branch pkg/gvisor/receiver_linux.go:153–171 treats EINTR from timed socket reads as fatal, closing an otherwise valid trace connection before a later start arrives. The new readPacket retries interruption, checks cancellation, and recalculates the receive timeout against the absolute deadline. Other errors still terminate the connection. Numeric atomic counters preserve the payload/privacy boundary and settle before final diagnostics. No new dependencies or normal node-agent startup integration. Scope and resource bounds remain appropriate for the opt-in experiment.
History: #1024 introduced the receiver, #1046 supplied trial tooling, and #1051 recorded the 19/20 run. These are merged prerequisites, not superseding fixes. Issue #894 and accepted design #23 establish project fit. Design #14 is broader companion work; its approval dismissal explicitly concerned LFX timing, not technical rejection. Earlier output/shutdown/privacy/endpoint objections were corrected before the prerequisite PRs were approved. Closed #918 concerns a different eBPF-to-profile loss path. The existing #1051 sender-drop-ledger suggestion remains nonblocking and does not invalidate these zero-reported-drop records.
Bounded GitHub searches covered open/closed/merged PRs with gVisor, SecCheck, EINTR, interrupted, receiver, receiver_linux.go, remote sink and container/start terms, plus gVisor/missing-start issues (50–100 results per query). No superseding fix found within this scope; searches cannot exclude unlinked work. Current PR has no inline threads or changes-requested review; Copilot did not execute because of quota.
Fresh validation in credential-free Bubblewrap isolation with networking disabled, read-only source and private temporary storage, using Linux amd64 Go 1.27.3:
go test -race ./pkg/gvisor ./cmd/gvisor-start-probe -count=1 -timeout=90spassed, including signal recovery, deadline, cancellation, queue-loss and actual-process output tests.go vet ./pkg/gvisor ./cmd/gvisor-start-probepassed.- All 14 Python checker tests passed; both new retained trials passed the count/privacy checker with stderr and ledgers, receiving 20 starts each. All 14 listed evidence hashes matched.
git diff --check origin/main HEADpassed. GitHub reports successful cross-platform build, component, benchmark and security checks for this head (build workflow).
Limits: I did not independently rerun the pre-fix live reproduction, privileged runsc trials, full repository suite, managed GKE or ARM64 trials. Signal recovery is freshly tested on the fixed code; baseline failure is supported by target-code inspection and the retained controlled before/after evidence. Handshake-send interruption has no direct regression test. Diagnostics intentionally documents regular-file stderr. The original missing start remains unexplained, and the node-agent comparison is inconclusive because its ordinary positive control failed; neither proves production readiness, and both remain explicit integration gates.
Immediately before submission, confirmed unchanged head/target, open/non-draft/mergeable status and no new review blockers. Verdict: approve this experimental transport fix.
The live trial in #1051 included a run with one missing start. I added receiver counters to investigate loss and found a reproducible connection failure: a signal interrupting a timed socket read made the receiver close an otherwise valid connection.
This retries interrupted reads and handshake responses while keeping cancellation and the handshake deadline bounded. The probe now has an optional
--diagnosticsflag that writes final counters to stderr without retaining message contents or container identities. Tests cover interruption, deadline handling, malformed input, and queue-loss accounting.The report keeps the distinction between what was reproduced and what remains unknown. A controlled test loses the next start before the fix and receives it afterward. Two fresh real runsc trials received 20/20 starts each. Neither observed an interrupted read, so these results do not prove that this bug caused the earlier missing event.
I also attempted the outstanding node-agent comparison on the local Linux VM. Both ordinary and gVisor workloads completed successfully and their runtime identities were verified, but the ordinary positive control had no retained exec records. The report includes this inconclusive result and setup details. The visibility comparison and production integration gates remain open.
Validation:
git diff --checkpassed.The focused packages were built in an isolated module with cached Go 1.26.8 and their pinned direct dependencies. The repository requires Go 1.27; this was not a full repository build or race run, and dependency files were not changed. No managed GKE or x86_64 validation is claimed.
Follows #1051 and the experiment in kubescape/designs-and-proposals#23.
Summary by CodeRabbit