Avoid retaining hidden simple deferred messages
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# Invocation reporting audit remediation
|
||||
|
||||
Status: remediation in progress; 5 of 7 findings complete.
|
||||
Status: remediation in progress; 6 of 7 findings complete.
|
||||
|
||||
## Review scope
|
||||
|
||||
@@ -381,7 +381,13 @@ owners.
|
||||
|
||||
## 6. Avoid retaining deferred messages that need no presentation work
|
||||
|
||||
Priority: P3. Status: for consideration.
|
||||
Priority: P3. Status: complete.
|
||||
|
||||
- [x] Completed: renderer-free hidden deferred messages record suppression
|
||||
without queue retention. Finalization detaches the queue without an array
|
||||
copy and delivers captured callback lists without recopying them. Hidden rich
|
||||
callbacks, FIFO order, no-content handling, capture, and closure remain covered.
|
||||
Validation: root module suite and focused reporter race checks pass.
|
||||
|
||||
### Finding and complexity
|
||||
|
||||
|
||||
47
reporter.go
47
reporter.go
@@ -249,6 +249,16 @@ func (r DeferredReporter) enqueue(category messageCategory, message string, deta
|
||||
if state.closed {
|
||||
return ErrReporterClosed
|
||||
}
|
||||
if !hasDetails(details) {
|
||||
if message == "" {
|
||||
return nil
|
||||
}
|
||||
if !state.visible(category) {
|
||||
state.diagnostics.submitted = true
|
||||
state.diagnostics.suppressed = true
|
||||
return nil
|
||||
}
|
||||
}
|
||||
state.deferred = append(state.deferred, deferredMessage{
|
||||
category: category,
|
||||
message: message,
|
||||
@@ -268,12 +278,16 @@ func (r *Reporter) finalize(outcome Outcome, completion func(*Reporter)) Outcome
|
||||
state.mu.Unlock()
|
||||
return normalizeOutcome(outcome)
|
||||
}
|
||||
deferred := append([]deferredMessage(nil), state.deferred...)
|
||||
deferred := state.deferred
|
||||
state.deferred = nil
|
||||
state.mu.Unlock()
|
||||
|
||||
for _, message := range deferred {
|
||||
_ = r.submitMessage(message.category, message.message, message.details)
|
||||
for index := range deferred {
|
||||
message := deferred[index]
|
||||
deferred[index] = deferredMessage{}
|
||||
state.mu.Lock()
|
||||
_ = state.submitMessageLocked(message.category, message.message, message.details)
|
||||
state.mu.Unlock()
|
||||
}
|
||||
if completion != nil {
|
||||
completion(r)
|
||||
@@ -339,10 +353,11 @@ func ensureFallbackExplanation(outcome Outcome) Outcome {
|
||||
}
|
||||
|
||||
func hasExplanation(explanation Explanation) bool {
|
||||
if explanation.Message != "" {
|
||||
return true
|
||||
}
|
||||
for _, detail := range explanation.Details {
|
||||
return explanation.Message != "" || hasDetails(explanation.Details)
|
||||
}
|
||||
|
||||
func hasDetails(details []Detail) bool {
|
||||
for _, detail := range details {
|
||||
if detail != nil {
|
||||
return true
|
||||
}
|
||||
@@ -405,16 +420,16 @@ func (r *Reporter) submitMessage(category messageCategory, message string, detai
|
||||
if state.closed {
|
||||
return ErrReporterClosed
|
||||
}
|
||||
return state.submitMessageLocked(category, message, append([]Detail(nil), details...))
|
||||
}
|
||||
|
||||
details = append([]Detail(nil), details...)
|
||||
hasDetails := false
|
||||
for _, detail := range details {
|
||||
if detail != nil {
|
||||
hasDetails = true
|
||||
break
|
||||
}
|
||||
// Queued detail lists have already been captured; delivery need not copy them.
|
||||
func (state *reportingState) submitMessageLocked(category messageCategory, message string, details []Detail) error {
|
||||
if state.closed {
|
||||
return ErrReporterClosed
|
||||
}
|
||||
if message == "" && !hasDetails {
|
||||
rich := hasDetails(details)
|
||||
if message == "" && !rich {
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -423,7 +438,7 @@ func (r *Reporter) submitMessage(category messageCategory, message string, detai
|
||||
destination.submitted = true
|
||||
if !visible {
|
||||
destination.suppressed = true
|
||||
if !hasDetails {
|
||||
if !rich {
|
||||
return nil
|
||||
}
|
||||
}
|
||||
|
||||
69
reporter_deferred_test.go
Normal file
69
reporter_deferred_test.go
Normal file
@@ -0,0 +1,69 @@
|
||||
package commandkit
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestHiddenSimpleDeferredMessagesDoNotAccumulate(t *testing.T) {
|
||||
for _, mode := range []reportingMode{modeOrdinary, modeQuiet} {
|
||||
t.Run(fmt.Sprint(mode), func(t *testing.T) {
|
||||
reporter := newReporter(Streams{})
|
||||
reporter.setVisibility(mode, false, false)
|
||||
for index := 0; index < 256; index++ {
|
||||
if err := reporter.AtEnd().Debug(fmt.Sprintf("hidden %d", index), nil); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := reporter.AtEnd().Warn("hidden warning"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
}
|
||||
// This assertion owns the storage optimization, not a public queue API.
|
||||
if len(reporter.state.deferred) != 0 {
|
||||
t.Fatalf("retained %d renderer-free hidden messages", len(reporter.state.deferred))
|
||||
}
|
||||
got := reporter.finalize(Success(), nil)
|
||||
if got.Status != 0 || got.Presentation.Diagnostics.State != DeliverySuppressed {
|
||||
t.Fatalf("outcome=%#v", got)
|
||||
}
|
||||
if err := reporter.AtEnd().Debug("late"); !errors.Is(err, ErrReporterClosed) {
|
||||
t.Fatalf("closed hidden submission=%v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
noContent := newReporter(Streams{})
|
||||
_ = noContent.AtEnd().Warn("", nil)
|
||||
if got := noContent.finalize(Success(), nil); got.Presentation.Diagnostics.State != DeliveryNone {
|
||||
t.Fatalf("empty deferred submission became content: %#v", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestHiddenDeferredDetailsRemainDeferredAndCaptured(t *testing.T) {
|
||||
var diagnostics bytes.Buffer
|
||||
reporter := newReporter(Streams{Err: &diagnostics})
|
||||
reporter.setVisibility(modeQuiet, false, false)
|
||||
failure := errors.New("hidden rendering failed")
|
||||
var order []string
|
||||
details := []Detail{func(w io.Writer) error {
|
||||
order = append(order, "first")
|
||||
_, _ = io.WriteString(w, "hidden detail")
|
||||
return failure
|
||||
}}
|
||||
_ = reporter.AtEnd().Warn("hidden", details...)
|
||||
details[0] = func(io.Writer) error { t.Error("caller changed captured callback"); return nil }
|
||||
_ = reporter.AtEnd().Info("", func(io.Writer) error { order = append(order, "second"); return nil })
|
||||
if len(order) != 0 {
|
||||
t.Fatal("details ran at submission")
|
||||
}
|
||||
got := reporter.finalize(Success(), nil)
|
||||
if strings.Join(order, ",") != "first,second" || !errors.Is(got.Presentation.Diagnostics.RenderError, failure) || got.Status != 1 {
|
||||
t.Fatalf("order=%v outcome=%#v", order, got)
|
||||
}
|
||||
if strings.Contains(diagnostics.String(), "hidden") || len(reporter.state.deferred) != 0 {
|
||||
t.Fatalf("hidden content leaked or queue retained: %q", diagnostics.String())
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user