Skip to content

Commit ef6e111

Browse files
fix(pipeline): decrement inflight metrics before write-failure guard (llm-d#591)
After a persistence failure in Drain(), the firstErr guard skipped Receive() for remaining results. Since Receive() was the only site that decremented inflight gauges for async-path results, the counters leaked on write failures. Move the metrics decrement block from Receive() into Drain(), before the firstErr check, so counters are always cleaned up regardless of write success. Signed-off-by: Madhu Goutham Reddy Ambati <mambati@redhat.com> Co-authored-by: Lior Aronovich <243445518+lioraron@users.noreply.github.com>
1 parent 17ab15d commit ef6e111

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

internal/processor/pipeline/collector.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ func (c *ResultCollector) Drain(ctx context.Context, resultCh <-chan ResultItem)
5656
if !c.pending.Resolve(&msg) {
5757
continue
5858
}
59+
if !msg.SubmittedAt.IsZero() {
60+
metrics.DecProcessorInflightRequests()
61+
metrics.DecModelInflightRequests(msg.ModelID)
62+
metrics.RecordModelRequestExecutionDuration(time.Since(msg.SubmittedAt), msg.ModelID)
63+
}
5964
if firstErr != nil {
6065
continue
6166
}
@@ -95,12 +100,6 @@ func (c *ResultCollector) Receive(msg ResultItem) error {
95100
return fmt.Errorf("write output for %s: %w", msg.RequestID, err)
96101
}
97102

98-
if !msg.SubmittedAt.IsZero() {
99-
metrics.DecProcessorInflightRequests()
100-
metrics.DecModelInflightRequests(msg.ModelID)
101-
metrics.RecordModelRequestExecutionDuration(time.Since(msg.SubmittedAt), msg.ModelID)
102-
}
103-
104103
if line.isSuccess() {
105104
c.tracker.RecordSuccess(msg)
106105
if msg.Response != nil {

internal/processor/pipeline/collector_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
package pipeline
22

33
import (
4+
"bufio"
45
"context"
56
"encoding/json"
7+
"fmt"
68
"testing"
9+
"time"
710

811
"github.com/go-logr/logr"
912

@@ -138,6 +141,57 @@ func TestResultCollector_DrainProcessesAllResultsAfterCancel(t *testing.T) {
138141
}
139142
}
140143

144+
type failAfterNWriter struct {
145+
remaining int
146+
}
147+
148+
func (w *failAfterNWriter) Write(p []byte) (int, error) {
149+
if w.remaining <= 0 {
150+
return 0, fmt.Errorf("simulated write failure")
151+
}
152+
w.remaining--
153+
return len(p), nil
154+
}
155+
156+
func TestResultCollector_DrainDecrementsMetricsAfterWriteFailure(t *testing.T) {
157+
outputFile := tempFile(t)
158+
errorFile := tempFile(t)
159+
pending := NewPendingRequests(0)
160+
tracker := NewProgressTracker(3, nil, "test-job", 0, logr.Discard())
161+
collector := NewResultCollector(outputFile, errorFile, pending, tracker, logr.Discard())
162+
163+
collector.output = bufio.NewWriterSize(&failAfterNWriter{remaining: 1}, 1)
164+
165+
now := time.Now()
166+
results := []ResultItem{
167+
{RequestID: "req-1", CustomID: "c-1", SubmittedAt: now, Response: &batch_types.ResponseData{StatusCode: 200, RequestID: "req-1", Body: map[string]any{"ok": true}}},
168+
{RequestID: "req-2", CustomID: "c-2", SubmittedAt: now, Response: &batch_types.ResponseData{StatusCode: 200, RequestID: "req-2", Body: map[string]any{"ok": true}}},
169+
{RequestID: "req-3", CustomID: "c-3", SubmittedAt: now, Response: &batch_types.ResponseData{StatusCode: 200, RequestID: "req-3", Body: map[string]any{"ok": true}}},
170+
}
171+
for _, r := range results {
172+
pending.Store(RequestItem{RequestID: r.RequestID, CustomID: r.CustomID, SubmittedAt: r.SubmittedAt})
173+
}
174+
175+
ch := make(chan ResultItem, len(results))
176+
for _, r := range results {
177+
ch <- r
178+
}
179+
close(ch)
180+
181+
err := collector.Drain(context.Background(), ch)
182+
if err == nil {
183+
t.Fatal("expected write failure error from Drain")
184+
}
185+
186+
var remaining int
187+
pending.DrainUnresolved(func(_ RequestItem) {
188+
remaining++
189+
})
190+
if remaining != 0 {
191+
t.Fatalf("pending requests remaining = %d, want 0 (all should be resolved despite write failure)", remaining)
192+
}
193+
}
194+
141195
func TestProgressTracker_AddFailed(t *testing.T) {
142196
tracker := NewProgressTracker(10, nil, "test-job", 0, logr.Discard())
143197
tracker.AddFailed(5)

0 commit comments

Comments
 (0)