fix(telemetry): drain capture pipe before payload log snapshot#166
Open
swarit-stepsecurity wants to merge 1 commit into
Open
Conversation
SnapshotBase64 read the ring buffer without waiting for the reader goroutine to copy in-flight pipe bytes, so lines written immediately before the snapshot (the config-audit block, ending at the yarn audit) were dropped from the uploaded execution log. The console showed runs truncating at "Auditing yarn configuration...". The scan itself always completed and uploaded fine — only the captured log was short. Inject a stripped NUL-delimited sentinel and block until the reader consumes it; FIFO ordering then guarantees all earlier bytes are ringed. Best-effort with a 2s cap so a snapshot never stalls the upload.
There was a problem hiding this comment.
Pull request overview
Fixes a telemetry execution-log truncation issue where SnapshotBase64() could read the ring buffer before the capture reader goroutine had drained recently written stderr bytes, causing the uploaded log snapshot to cut off early.
Changes:
- Add a drain mechanism to
SnapshotBase64()by injecting a sentinel into the capture pipe and waiting (best-effort, time-bounded) for the reader goroutine to consume it before snapshotting. - Update the capture reader goroutine to strip the sentinel from output while continuing to tee into the ring and original stderr.
- Add unit tests to ensure “just written” lines are included in snapshots and that the sentinel never leaks into captured output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/telemetry/logcapture.go | Adds sentinel-based pipe draining and reader-side sentinel stripping to make SnapshotBase64() include in-flight stderr bytes. |
| internal/telemetry/logcapture_test.go | Adds regression tests for snapshot completeness and sentinel stripping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+286
to
+310
| lc.mu.Lock() | ||
| w := lc.pipeWrite | ||
| if w == nil { | ||
| // Capture never started or already finalized; the ring is final and | ||
| // no reader is running to acknowledge a sentinel. | ||
| lc.mu.Unlock() | ||
| return | ||
| } | ||
| ch := make(chan struct{}) | ||
| lc.drainCh = ch | ||
| lc.mu.Unlock() | ||
|
|
||
| if _, err := w.Write([]byte(snapSentinel)); err != nil { | ||
| lc.mu.Lock() | ||
| lc.drainCh = nil | ||
| lc.mu.Unlock() | ||
| return | ||
| } | ||
|
|
||
| select { | ||
| case <-ch: | ||
| case <-lc.done: // reader exited (e.g. a concurrent Finalize) | ||
| case <-time.After(drainTimeout): | ||
| } | ||
| } |
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
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.
SnapshotBase64 read the ring buffer without waiting for the reader goroutine to copy in-flight pipe bytes, so lines written immediately before the snapshot (the config-audit block, ending at the yarn audit) were dropped from the uploaded execution log. The console showed runs truncating at "Auditing yarn configuration...". The scan itself always completed and uploaded fine - only the captured log was short.
Inject a stripped NUL-delimited sentinel and block until the reader consumes it; FIFO ordering then guarantees all earlier bytes are ringed. Best-effort with a 2s cap so a snapshot never stalls the upload.
What does this PR do?
Type of change
Testing
./stepsecurity-dev-machine-guard --verbose./stepsecurity-dev-machine-guard --json | python3 -m json.toolmake lintmake testRelated Issues