Skip to content

Commit 46cde90

Browse files
committed
[review-fix 2] Retry one-shots under backpressure
- Retain one-shot contexts until the collector processing channel accepts them, so full-channel drops remain retryable within the selection window. - Acknowledge accepted one-shots with deadline identity checks to avoid deleting a newer same-path window. - Cover store acknowledgment, cross-window races, and processing-channel backpressure. Source: review feedback Validation: Bazel race tests for collector and path-test store
1 parent f093369 commit 46cde90

4 files changed

Lines changed: 78 additions & 8 deletions

File tree

comp/networkpath/npcollector/impl/npcollector.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,7 @@ func (s *npCollectorImpl) flush() {
595595
s.logger.Tracef("flushed ptConf %s:%d", ptConf.Pathtest.Hostname, ptConf.Pathtest.Port)
596596
select {
597597
case s.pathtestProcessingChan <- ptConf:
598+
s.pathtestStore.AcknowledgeOneShot(ptConf.Pathtest)
598599
_ = s.statsdClient.Incr(common.NetworkPathCollectorMetricPrefix+"flush.pathtest_processed", []string{}, 1)
599600
default:
600601
_ = s.statsdClient.Incr(common.NetworkPathCollectorMetricPrefix+"flush.pathtest_dropped", []string{"reason:processing_chan_full"}, 1)

comp/networkpath/npcollector/impl/npcollector_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1678,6 +1678,31 @@ func Test_npCollectorImpl_flush(t *testing.T) {
16781678
assert.Equal(t, 2, len(npCollector.pathtestProcessingChan))
16791679
}
16801680

1681+
func TestNpCollectorFlushRetriesOneShotAfterProcessingChannelBackpressure(t *testing.T) {
1682+
_, collector := newTestNpCollector(t, map[string]any{
1683+
"network_path.connections_monitoring.baseline_tests.enabled": true,
1684+
"network_path.collector.processing_chan_size": 1,
1685+
}, &teststatsd.Client{}, nil)
1686+
deadline := time.Now().Add(time.Minute)
1687+
collector.pathtestStore.Add(&common.Pathtest{
1688+
Hostname: "baseline",
1689+
Origin: payload.PathOriginNetworkTraffic,
1690+
OneShot: true,
1691+
ExecutionDeadline: deadline,
1692+
DynamicTestProfile: payload.DynamicTestProfileBaseline,
1693+
})
1694+
collector.pathtestProcessingChan <- &pathteststore.PathtestContext{}
1695+
1696+
collector.flush()
1697+
assert.Equal(t, 1, collector.pathtestStore.GetContextsCount(), "full channel must retain the one-shot")
1698+
<-collector.pathtestProcessingChan
1699+
1700+
collector.flush()
1701+
require.Len(t, collector.pathtestProcessingChan, 1)
1702+
assert.Equal(t, "baseline", (<-collector.pathtestProcessingChan).Pathtest.Hostname)
1703+
assert.Zero(t, collector.pathtestStore.GetContextsCount())
1704+
}
1705+
16811706
func Test_npCollectorImpl_flushLoop(t *testing.T) {
16821707
// GIVEN
16831708
agentConfigs := map[string]any{

comp/networkpath/npcollector/impl/pathteststore/pathteststore.go

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -171,18 +171,18 @@ func (f *Store) Flush() []*PathtestContext {
171171
if ptConfigCtx.nextRun.After(now) || !f.rateLimiter.AllowN(now, 1) {
172172
continue
173173
}
174+
if ptConfigCtx.Pathtest.OneShot {
175+
// Keep one-shots until the collector acknowledges that its processing
176+
// channel accepted the snapshot. A full channel must leave the selection
177+
// available for the next flush attempt.
178+
pathtestsToFlush = append(pathtestsToFlush, ptConfigCtx.snapshot())
179+
continue
180+
}
174181
if !ptConfigCtx.lastFlushTime.IsZero() {
175182
ptConfigCtx.lastFlushInterval = now.Sub(ptConfigCtx.lastFlushTime)
176183
}
177184
ptConfigCtx.lastFlushTime = now
178185
pathtestsToFlush = append(pathtestsToFlush, ptConfigCtx.snapshot())
179-
if ptConfigCtx.Pathtest.OneShot {
180-
if ptConfigCtx.Pathtest.DynamicTestProfile == payload.DynamicTestProfileBaseline {
181-
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_dispatched", []string{}, 1) //nolint:errcheck
182-
}
183-
delete(f.contexts, key)
184-
continue
185-
}
186186
ptConfigCtx.nextRun = ptConfigCtx.nextRun.Add(f.config.Interval)
187187
}
188188

@@ -191,6 +191,28 @@ func (f *Store) Flush() []*PathtestContext {
191191
return pathtestsToFlush
192192
}
193193

194+
// AcknowledgeOneShot removes a one-shot after the collector processing channel
195+
// accepts it. The deadline check prevents a late acknowledgment from removing a
196+
// same-path selection admitted for a newer window.
197+
func (f *Store) AcknowledgeOneShot(pathtest *common.Pathtest) {
198+
if !pathtest.OneShot {
199+
return
200+
}
201+
202+
f.contextsMutex.Lock()
203+
defer f.contextsMutex.Unlock()
204+
205+
hash := pathtest.GetHash()
206+
pathtestCtx, ok := f.contexts[hash]
207+
if !ok || !pathtestCtx.Pathtest.OneShot || !pathtestCtx.Pathtest.ExecutionDeadline.Equal(pathtest.ExecutionDeadline) {
208+
return
209+
}
210+
delete(f.contexts, hash)
211+
if pathtestCtx.Pathtest.DynamicTestProfile == payload.DynamicTestProfileBaseline {
212+
f.statsdClient.Incr(networkPathStoreMetricPrefix+"baseline_dispatched", []string{}, 1) //nolint:errcheck
213+
}
214+
}
215+
194216
// Add new pathtest
195217
func (f *Store) Add(pathtestToAdd *common.Pathtest) {
196218
f.logger.Tracef("Add new Pathtest: %+v", pathtestToAdd)

comp/networkpath/npcollector/impl/pathteststore/pathteststore_test.go

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,8 @@ func TestPathtestStoreOneShotDispatchesOnceAndDeletes(t *testing.T) {
207207
flushed := store.Flush()
208208
require.Len(t, flushed, 1)
209209
assert.True(t, flushed[0].Pathtest.OneShot)
210+
assert.Equal(t, 1, store.GetContextsCount(), "one-shot must remain pending until channel admission")
211+
store.AcknowledgeOneShot(flushed[0].Pathtest)
210212
assert.Zero(t, store.GetContextsCount())
211213
assert.Empty(t, store.Flush())
212214
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_dispatched"].Sum)
@@ -236,12 +238,32 @@ func TestPathtestStoreReplacesExpiredOneShotBeforeFlush(t *testing.T) {
236238
replacement := store.contexts[(&common.Pathtest{Hostname: "baseline"}).GetHash()]
237239
require.NotNil(t, replacement)
238240
assert.Equal(t, newDeadline, replacement.runUntil)
239-
assert.Len(t, store.Flush(), 1, "the next window's replacement must remain dispatchable")
241+
flushed := store.Flush()
242+
require.Len(t, flushed, 1, "the next window's replacement must remain dispatchable")
243+
store.AcknowledgeOneShot(flushed[0].Pathtest)
240244
assert.Zero(t, store.GetContextsCount())
241245
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_expired"].Sum)
242246
assert.Equal(t, int64(1), stats.GetCountSummaries()[networkPathStoreMetricPrefix+"baseline_dispatched"].Sum)
243247
}
244248

249+
func TestPathtestStoreOneShotAcknowledgmentDoesNotRemoveNewWindow(t *testing.T) {
250+
now := time.Date(2026, 8, 15, 12, 0, 0, 0, time.UTC)
251+
store := NewPathtestStore(Config{ContextsLimit: 10}, logmock.New(t), &statsd.NoOpClient{}, func() time.Time { return now })
252+
store.Add(&common.Pathtest{Hostname: "baseline", OneShot: true, ExecutionDeadline: now.Add(30 * time.Minute)})
253+
flushed := store.Flush()
254+
require.Len(t, flushed, 1)
255+
oldWindow := flushed[0].Pathtest
256+
now = now.Add(30 * time.Minute)
257+
newDeadline := now.Add(30 * time.Minute)
258+
store.Add(&common.Pathtest{Hostname: "baseline", OneShot: true, ExecutionDeadline: newDeadline})
259+
260+
store.AcknowledgeOneShot(oldWindow)
261+
262+
context := store.contexts[oldWindow.GetHash()]
263+
require.NotNil(t, context)
264+
assert.Equal(t, newDeadline, context.runUntil)
265+
}
266+
245267
func TestPathtestStoreRecurringDispatchesAtTTLBoundary(t *testing.T) {
246268
now := time.Date(2026, 8, 15, 12, 0, 0, 0, time.UTC)
247269
store := NewPathtestStore(Config{

0 commit comments

Comments
 (0)