refactor(gpufault): the fault relevance window lives with the contract

executor/pkg/plugin/k8s/plugin_manager.go

@@ -363,32 +363,6 @@ func objectKeyFor(resource client.Object) watchedObjectKey {
 	}
 }
 
-// gpuFaultRelevanceWindow bounds how long before a failure a fault can still explain
-// it. Which pod a fault belongs to is settled by the UID when the pod's UID is known
-// (see classifyGpuFailure for the one case it is not); the window only separates the
-// fault that explains this failure from one the node saw much earlier. It is measured
-// from the failure's own time, not from when classification runs, so a slow reconcile
-// cannot age a fault out. Thirty minutes spans the slow paths between a fault and the
-// failure it causes: a container left wedged after a bus fault until the kubelet gives
-// up on it, and a node going NotReady with its pods evicted only after the
-// node-monitor grace period and eviction timeout.
-//
-// A fault that was still firing inside the window counts even if it started before it,
-// because what the window bounds is how stale a fault's last sign of life may be, not how
-// old the fault is. See faultOverlapsFailure.
-const gpuFaultRelevanceWindow = 30 * time.Minute
-
-// gpuFaultAfterFailureSlack is how far past the failure a fault may first be recorded and
-// still count. The kernel line and the container's termination are stamped by different
-// processes on the same node and the daemon reads the kernel log with a small lag, so a
-// fault can first be recorded moments after the failure it caused; a fault that only
-// started later than that cannot have caused it.
-//
-// It bounds when a fault started, not when it stopped. Hardware that keeps faulting after
-// the container died goes on being observed for as long as it goes on faulting, and that
-// says nothing about whether it caused the failure. See faultOverlapsFailure.
-const gpuFaultAfterFailureSlack = 2 * time.Minute
-
 // classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into
 // the failure the plugin reported, so that a fault the node saw becomes the code and
 // the message the user reads. Anything that is not a failed pod is left alone.
@@ -436,7 +410,7 @@ func (pm *PluginManager) classifyGpuFailure(
 		if resource.GetUID() != "" && event.RegardingUID != resource.GetUID() {
 			continue
 		}
-		if !faultOverlapsFailure(event, failureAt) {
+		if !gpufault.RelevantToFailure(event.CreatedAt, event.LastObservedAt, failureAt) {
 			logger.Debugf(context.TODO(),
 				"ignoring GPU fault event %q on %s: active %s to %s, which does not reach the failure at %s",
 				event.Reason, objectKeyFor(resource).Name, event.CreatedAt, event.LastObservedAt, failureAt)
@@ -450,43 +424,6 @@ func (pm *PluginManager) classifyGpuFailure(
 	return gpufault.ClassifyFailure(phaseInfo, faults)
 }
 
-// faultOverlapsFailure reports whether a fault event was active close enough to the
-// failure to explain it.
-//
-// A fault that keeps repeating is aggregated into a single event whose last observation
-// moves with every repeat, so an event describes an interval and not a moment: it was
-// first recorded at CreatedAt and was still firing at LastObservedAt. The failure has an
-// interval of its own, the window before it in which a fault could have caused it and the
-// small slack after it in which a fault it caused could still be recorded. The event
-// counts when those two intervals overlap.
-//
-// Testing the last observation alone, as this used to, drops the fault that matters most:
-// hardware that keeps faulting after the container died has a last observation well past
-// the failure, so the longer it goes on the more certainly it was discarded. Testing the
-// creation alone drops the opposite case, a fault that started before the window opened
-// and was still firing when the task died. Overlap keeps both and still rejects a fault
-// that only started after the failure, or one that had stopped firing before the window
-// opened.
-func faultOverlapsFailure(event *eventInfo, failureAt time.Time) bool {
-	activeFrom, activeUntil := event.CreatedAt, event.LastObservedAt
-	if activeFrom.IsZero() {
-		activeFrom = activeUntil
-	}
-	if activeUntil.IsZero() {
-		activeUntil = activeFrom
-	}
-	if activeFrom.IsZero() || activeUntil.Before(activeFrom) {
-		// No usable time at all, or a last observation older than the creation, which no
-		// honest recorder produces. Nothing can be concluded, so it does not explain.
-		return false
-	}
-
-	relevantFrom := failureAt.Add(-gpuFaultRelevanceWindow)
-	relevantUntil := failureAt.Add(gpuFaultAfterFailureSlack)
-
-	return !activeFrom.After(relevantUntil) && !activeUntil.Before(relevantFrom)
-}
-
 // phaseInfoOccurredAt is the time the plugin put on the failure, or the zero time when it
 // put none there.
 func phaseInfoOccurredAt(phaseInfo pluginsCore.PhaseInfo) time.Time {

executor/pkg/plugin/k8s/plugin_manager_test.go

@@ -342,7 +342,7 @@ func TestClassifyGpuFailure(t *testing.T) {
 	// Recency is measured against the clock now, so the fixtures have to sit relative to
 	// it: base is inside the relevance window, stale is well outside it.
 	base := time.Now().Add(-time.Minute)
-	stale := time.Now().Add(-2 * gpuFaultRelevanceWindow)
+	stale := time.Now().Add(-2 * gpufault.RelevanceWindow)
 
 	tests := []struct {
 		name        string
@@ -554,7 +554,7 @@ func TestClassifyGpuFailureRelevanceIsAnInterval(t *testing.T) {
 	})
 
 	t.Run("a fault that only started after the failure does not explain it", func(t *testing.T) {
-		started := failedAt.Add(gpuFaultAfterFailureSlack + time.Minute)
+		started := failedAt.Add(gpufault.AfterFailureSlack + time.Minute)
 		got := classify(t, started, started.Add(5*time.Minute))
 		assert.Equal(t, "UnknownError", got.Err().GetCode())
 		assert.Nil(t, got.Err().GetGpuFault())

flyteplugins/go/tasks/pluginmachinery/gpufault/relevance.go

@@ -0,0 +1,69 @@
+package gpufault
+
+import "time"
+
+// RelevanceWindow bounds how long before a failure a fault can still explain it. It only
+// separates the fault that explains this failure from one the node saw much earlier;
+// which pod a fault belongs to is a question the consumer settles for itself, before it
+// gets here. It is measured from the failure's own time rather than from when the check
+// runs, so a slow consumer cannot age a fault out.
+//
+// Thirty minutes spans the slow paths between a fault and the failure it causes: a
+// container left wedged after a bus fault until the kubelet gives up on it, and a node
+// going NotReady with its pods evicted only after the node-monitor grace period and
+// eviction timeout.
+//
+// A fault that was still firing inside the window counts even if it started before it,
+// because what the window bounds is how stale a fault's last sign of life may be, not how
+// old the fault is. See RelevantToFailure.
+const RelevanceWindow = 30 * time.Minute
+
+// AfterFailureSlack is how far past the failure a fault may first be recorded and still
+// count. The kernel line and the container's termination are stamped by different
+// processes on the same node and the daemon reads the kernel log with a small lag, so a
+// fault can first be recorded moments after the failure it caused; a fault that only
+// started later than that cannot have caused it.
+//
+// It bounds when a fault started, not when it stopped. Hardware that keeps faulting after
+// the container died goes on being observed for as long as it goes on faulting, and that
+// says nothing about whether it caused the failure. See RelevantToFailure.
+const AfterFailureSlack = 2 * time.Minute
+
+// RelevantToFailure reports whether a fault was active close enough to a failure to
+// explain it.
+//
+// A fault that keeps repeating is aggregated into a single event whose last observation
+// moves with every repeat, so a fault describes an interval and not a moment: it was first
+// recorded at activeFrom and was still firing at activeUntil. The failure has an interval
+// of its own, the window before it in which a fault could have caused it and the small
+// slack after it in which a fault it caused could still be recorded. The fault counts when
+// those two intervals overlap.
+//
+// Testing the last observation alone drops the fault that matters most: hardware that
+// keeps faulting after the container died has a last observation well past the failure, so
+// the longer it goes on the more certainly it would be discarded. Testing the first
+// recording alone drops the opposite case, a fault that started before the window opened
+// and was still firing when the task died. Overlap keeps both and still rejects a fault
+// that only started after the failure, or one that had stopped firing before the window
+// opened.
+//
+// Either time may be zero, and the other then stands in for it, which is how a fault
+// recorded once rather than aggregated is judged as the moment it is. A fault with neither
+// time, or one whose last observation precedes its first recording, explains nothing: no
+// honest recorder produces either, and nothing can be concluded from them.
+func RelevantToFailure(activeFrom, activeUntil, failureAt time.Time) bool {
+	if activeFrom.IsZero() {
+		activeFrom = activeUntil
+	}
+	if activeUntil.IsZero() {
+		activeUntil = activeFrom
+	}
+	if activeFrom.IsZero() || activeUntil.Before(activeFrom) {
+		return false
+	}
+
+	relevantFrom := failureAt.Add(-RelevanceWindow)
+	relevantUntil := failureAt.Add(AfterFailureSlack)
+
+	return !activeFrom.After(relevantUntil) && !activeUntil.Before(relevantFrom)
+}

flyteplugins/go/tasks/pluginmachinery/gpufault/relevance_test.go

@@ -0,0 +1,122 @@
+package gpufault
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRelevantToFailure(t *testing.T) {
+	failedAt := time.Date(2026, 8, 25, 12, 0, 0, 0, time.UTC)
+
+	tests := []struct {
+		name        string
+		activeFrom  time.Time
+		activeUntil time.Time
+		want        bool
+	}{
+		{
+			name:        "a fault recorded once, just before the failure",
+			activeFrom:  failedAt.Add(-time.Minute),
+			activeUntil: failedAt.Add(-time.Minute),
+			want:        true,
+		},
+		{
+			// Dying hardware goes on faulting after the container is gone. Judging this
+			// by its last observation alone would discard it, and the longer it kept
+			// faulting the more certainly it would be discarded.
+			name:        "a fault that started before the failure and kept firing well after it",
+			activeFrom:  failedAt.Add(-time.Minute),
+			activeUntil: failedAt.Add(11 * time.Minute),
+			want:        true,
+		},
+		{
+			// Judging this by its first recording alone would discard it.
+			name:        "a fault older than the window but still firing at the failure",
+			activeFrom:  failedAt.Add(-40 * time.Minute),
+			activeUntil: failedAt,
+			want:        true,
+		},
+		{
+			name:        "a fault spanning the whole window and beyond in both directions",
+			activeFrom:  failedAt.Add(-10 * time.Hour),
+			activeUntil: failedAt.Add(10 * time.Hour),
+			want:        true,
+		},
+		{
+			name:        "a fault first recorded inside the slack",
+			activeFrom:  failedAt.Add(AfterFailureSlack - time.Second),
+			activeUntil: failedAt.Add(time.Hour),
+			want:        true,
+		},
+		{
+			name:        "a fault that only started after the slack",
+			activeFrom:  failedAt.Add(AfterFailureSlack + time.Second),
+			activeUntil: failedAt.Add(time.Hour),
+			want:        false,
+		},
+		{
+			name:        "a fault that stopped firing just before the window opened",
+			activeFrom:  failedAt.Add(-90 * time.Minute),
+			activeUntil: failedAt.Add(-RelevanceWindow - time.Second),
+			want:        false,
+		},
+		{
+			name:        "a fault still firing exactly as the window opens",
+			activeFrom:  failedAt.Add(-90 * time.Minute),
+			activeUntil: failedAt.Add(-RelevanceWindow),
+			want:        true,
+		},
+		{
+			// A fault recorded once rather than aggregated carries no last observation,
+			// and is judged as the moment it is.
+			name:       "only a first recording, inside the window",
+			activeFrom: failedAt.Add(-time.Minute),
+			want:       true,
+		},
+		{
+			name:       "only a first recording, outside the window",
+			activeFrom: failedAt.Add(-2 * RelevanceWindow),
+			want:       false,
+		},
+		{
+			name:        "only a last observation, inside the window",
+			activeUntil: failedAt.Add(-time.Minute),
+			want:        true,
+		},
+		{
+			name:        "only a last observation, outside the window",
+			activeUntil: failedAt.Add(-2 * RelevanceWindow),
+			want:        false,
+		},
+		{
+			name: "no times at all explains nothing",
+			want: false,
+		},
+		{
+			// No honest recorder produces this, so nothing can be concluded from it.
+			name:        "a last observation older than the first recording explains nothing",
+			activeFrom:  failedAt,
+			activeUntil: failedAt.Add(-time.Hour),
+			want:        false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, RelevantToFailure(tt.activeFrom, tt.activeUntil, failedAt))
+		})
+	}
+}
+
+// The window is measured from the failure rather than from when the check runs, so a
+// consumer that gets to a failure late reaches the same verdict as one that got there
+// immediately.
+func TestRelevantToFailureIsAnchoredOnTheFailure(t *testing.T) {
+	failedAt := time.Now().Add(-6 * time.Hour)
+	faultedAt := failedAt.Add(-5 * time.Minute)
+
+	assert.True(t, RelevantToFailure(faultedAt, faultedAt, failedAt))
+	assert.False(t, RelevantToFailure(faultedAt, faultedAt, time.Now()))
+}