Skip to content

CNTRLPLANE-2449: test(sig-builds][Feature:Builds]): ignore watchapi.Bookmark as they are synthetic and used only for signaling sync completion #30885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 3 commits into from
Mar 16, 2026
Merged

CNTRLPLANE-2449: test(sig-builds][Feature:Builds]): ignore watchapi.Bookmark as they are synthetic and used only for signaling sync completion #30885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
620cf35
test(TestWebhookGitHubPing): ignore watchapi.Bookmark as they are syn…
ingvagabund Mar 16, 2026
1cb0ef5
test(RunBuildDeleteTest, RunBuildCompletePodDeleteTest): ignore watch…
ingvagabund Mar 16, 2026
7554a41
test(runPolicy): ignore watchapi.Bookmark as they are synthetic and u…
ingvagabund Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions test/extended/builds/controllers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,25 @@ type buildControllerPodTest struct {
}

func waitForWatch(t testingT, name string, w watchapi.Interface) *watchapi.Event {
select {
case e, ok := <-w.ResultChan():
if !ok {
t.Fatalf("Channel closed waiting for watch: %s", name)
for {
select {
case e, ok := <-w.ResultChan():
if !ok {
t.Fatalf("Channel closed waiting for watch: %s", name)
}
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// object changes) used for caching coordination, so we skip them and continue waiting
// for real events.
if e.Type == watchapi.Bookmark {
continue
}
return &e
case <-time.After(BuildControllersWatchTimeout):
t.Fatalf("Timed out waiting for watch: %s", name)
return nil
}
Comment on lines 130 to 149
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the file in question
find . -name "controllers.go" -path "*/test/extended/builds/*" | head -5

Repository: openshift/origin

Length of output: 97

🏁 Script executed:

# Check if the file exists and get an overview
if [ -f "test/extended/builds/controllers.go" ]; then
  wc -l "test/extended/builds/controllers.go"
else
  echo "File not found at expected path"
fi

Repository: openshift/origin

Length of output: 99

🏁 Script executed:

# Read the specific lines around line 130-149 and 389-404
cat -n test/extended/builds/controllers.go | sed -n '125,155p'

Repository: openshift/origin

Length of output: 1333

🏁 Script executed:

# Read the second function mentioned
cat -n test/extended/builds/controllers.go | sed -n '384,410p'

Repository: openshift/origin

Length of output: 1316

🏁 Script executed:

# Verify Go time.After() behavior by checking documentation/semantics
# Let's also check if there are any other timeout patterns in the file for comparison
grep -n "time.After\|time.NewTimer" test/extended/builds/controllers.go

Repository: openshift/origin

Length of output: 316

🏁 Script executed:

# Check if BuildControllersWatchTimeout is defined and what value it has
grep -n "BuildControllersWatchTimeout" test/extended/builds/controllers.go | head -10

Repository: openshift/origin

Length of output: 324

Move timeout outside loops to ensure absolute timeout, not per-iteration reset.

Lines 146 and 401 create time.After(BuildControllersWatchTimeout) inside select statements within loops. With frequent bookmark events, each iteration resets the timeout to its full duration; the effective timeout becomes per-iteration rather than absolute. This can cause indefinite hangs when bookmark traffic is sustained. Line 394's i-- logic in waitForWatchType compounds the issue by preventing progress while timeout keeps resetting.

Suggested fix 
 func waitForWatch(t testingT, name string, w watchapi.Interface) *watchapi.Event {
+	timeout := time.After(BuildControllersWatchTimeout)
 	for {
 		select {
 		case e, ok := <-w.ResultChan():
 			if !ok {
 				t.Fatalf("Channel closed waiting for watch: %s", name)
 			}
 			if e.Type == watchapi.Bookmark {
 				continue
 			}
 			return &e
-		case <-time.After(BuildControllersWatchTimeout):
+		case <-timeout:
 			t.Fatalf("Timed out waiting for watch: %s", name)
 			return nil
 		}
 	}
 }
@@
 func waitForWatchType(t testingT, name string, w watchapi.Interface, expect watchapi.EventType) *watchapi.Event {
-	tries := 10
-	for i := 0; i < tries; i++ {
+	tries := 10
+	timeout := time.After(BuildControllersWatchTimeout)
+	seen := 0
+	for seen < tries {
 		select {
-		case e := <-w.ResultChan():
+		case e, ok := <-w.ResultChan():
+			if !ok {
+				t.Fatalf("Channel closed waiting for watch: %s", name)
+			}
 			if e.Type == watchapi.Bookmark {
-				i-- // Don't consume a try for bookmark events
 				continue
 			}
+			seen++
 			if e.Type != expect {
 				continue
 			}
 			return &e
-		case <-time.After(BuildControllersWatchTimeout):
+		case <-timeout:
 			t.Fatalf("Timed out waiting for watch: %s", name)
 			return nil
 		}
 	}
 	t.Fatalf("Waited for a %v event with %d tries but never received one", expect, tries)
 	return nil
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/builds/controllers.go` around lines 130 - 149, The timeout is
created inside the loop in waitForWatch (and similarly used in
waitForWatchType), causing the deadline to be reset on each select iteration;
move to an absolute timeout by creating a single deadline/timer before the loop
(e.g., compute deadline := time.Now().Add(BuildControllersWatchTimeout) or
create one time.NewTimer(BuildControllersWatchTimeout)) and use that single
timer (or compute remaining time with time.Until(deadline)) inside the
loop/select so bookmark events cannot extend the overall timeout; ensure to
Stop() any timer when exiting to avoid leaks and adjust waitForWatchType's i--
logic to rely on the absolute deadline instead of per-iteration time.After.

return &e
case <-time.After(BuildControllersWatchTimeout):
t.Fatalf("Timed out waiting for watch: %s", name)
return nil
}
}

Expand Down Expand Up @@ -376,6 +386,14 @@ func waitForWatchType(t testingT, name string, w watchapi.Interface, expect watc
for i := 0; i < tries; i++ {
select {
case e := <-w.ResultChan():
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// object changes) used for caching coordination. We skip them without consuming a try.
if e.Type == watchapi.Bookmark {
i-- // Don't consume a try for bookmark events
continue
}
if e.Type != expect {
continue
}
Expand Down
65 changes: 65 additions & 0 deletions test/extended/builds/run_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/validation"
watchapi "k8s.io/apimachinery/pkg/watch"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
Expand Down Expand Up @@ -82,6 +83,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
// Wait for it to become running
for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
o.Expect(IsBuildComplete(build)).Should(o.BeFalse())
Comment on lines 85 to 95
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, verify the file exists and check its general structure
wc -l test/extended/builds/run_policy.go

Repository: openshift/origin

Length of output: 98

🏁 Script executed:

# Check the imports at the top of the file to verify watchapi import
head -20 test/extended/builds/run_policy.go

Repository: openshift/origin

Length of output: 468

🏁 Script executed:

# Now examine the specific line ranges mentioned in the review
# Check lines 85-95 first
sed -n '80,100p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1091

🏁 Script executed:

# Check all 8 locations to confirm the pattern is repeated
# Lines 111-121
sed -n '106,126p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 892

🏁 Script executed:

# Lines 182-192
sed -n '177,197p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1277

🏁 Script executed:

# Lines 262-272
sed -n '257,277p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1260

🏁 Script executed:

# Lines 322-332
sed -n '317,337p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1281

🏁 Script executed:

# Lines 410-420
sed -n '405,425p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1271

🏁 Script executed:

# Lines 478-488
sed -n '473,493p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1141

🏁 Script executed:

# Lines 509-519
sed -n '504,524p' test/extended/builds/run_policy.go | cat -n

Repository: openshift/origin

Length of output: 1213

Handle closed channels and error events before casting to *buildv1.Build in watch loops.

At lines 86, 112, 183, 263, 323, 411, 479, and 510, the code receives from ResultChan() without checking the ok flag. When the channel closes, this returns a zero-value Event with a nil Object. The subsequent type assertion event.Object.(*buildv1.Build) silently succeeds (returning nil), but accessing fields like build.Status.Phase causes a nil pointer panic. Additionally, the code filters watchapi.Bookmark events but ignores watchapi.Error events; an error event's Object contains *metav1.Status, not *buildv1.Build, causing the type assertion to panic.

Suggested pattern (apply to each loop) 
- event := <-buildWatch.ResultChan()
+ event, ok := <-buildWatch.ResultChan()
+ o.Expect(ok).To(o.BeTrue(), "build watch channel closed unexpectedly")
  if event.Type == watchapi.Bookmark {
      continue
  }
- build := event.Object.(*buildv1.Build)
+ o.Expect(event.Type).NotTo(o.Equal(watchapi.Error), "received watch error event")
+ build, ok := event.Object.(*buildv1.Build)
+ o.Expect(ok).To(o.BeTrue(), "unexpected watch object type: %T", event.Object)

Also applies to: 112, 183, 263, 323, 411, 479, 510

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/builds/run_policy.go` around lines 85 - 95, The watch loops
reading from buildWatch.ResultChan() must check the receive-ok and handle
non-Build event types before doing a straight type assertion; update each loop
(the receive from buildWatch.ResultChan() in run_policy.go) to first capture the
receive like evt, ok := <-buildWatch.ResultChan() and if !ok handle the closed
channel (break/return/fail as appropriate), then skip watchapi.Bookmark as
before, explicitly handle watchapi.Error by casting evt.Object to *metav1.Status
to surface the error, and perform a safe type assertion for *buildv1.Build
(build, isBuild := evt.Object.(*buildv1.Build)) and only access build fields
when isBuild && build != nil to avoid nil pointer panics.

if build.Name == startedBuilds[0] && build.Status.Phase == buildv1.BuildPhaseRunning {
Expand All @@ -100,6 +109,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration

for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
if build.Name == startedBuilds[0] {
if IsBuildComplete(build) {
Expand Down Expand Up @@ -163,6 +180,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
sawCompletion := false
for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
var lastCompletion time.Time
if build.Status.Phase == buildv1.BuildPhaseComplete {
Expand Down Expand Up @@ -235,6 +260,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
var cancelTime, cancelTime2 time.Time
for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
if build.Status.Phase == buildv1.BuildPhasePending || build.Status.Phase == buildv1.BuildPhaseRunning {
if build.Status.Phase == buildv1.BuildPhaseRunning {
Expand Down Expand Up @@ -287,6 +320,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
for done == false {
select {
case event := <-buildWatch.ResultChan():
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
if build.Status.Phase == buildv1.BuildPhasePending {
o.Expect(hasConditionState(build, buildv1.BuildPhasePending, true)).Should(o.BeTrue())
Expand Down Expand Up @@ -367,6 +408,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
for !done {
select {
case event := <-buildWatch.ResultChan():
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
if build.Status.Phase == buildv1.BuildPhasePending || build.Status.Phase == buildv1.BuildPhaseRunning {
if build.Status.Phase == buildv1.BuildPhaseRunning {
Expand Down Expand Up @@ -427,6 +476,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
// Wait for the first build to become running
for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
if build.Name == startedBuilds[0] {
if build.Status.Phase == buildv1.BuildPhaseRunning {
Expand All @@ -450,6 +507,14 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] using build configuration
// will be the last build created.
for {
event := <-buildWatch.ResultChan()
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
e2e.Logf("got event for build %s with phase %s", build.Name, build.Status.Phase)
// The second build should be cancelled
Expand Down
33 changes: 27 additions & 6 deletions test/extended/builds/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
watchapi "k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/rest"
admissionapi "k8s.io/pod-security-admission/api"

Expand Down Expand Up @@ -335,6 +336,14 @@ Loop:
case <-time.After(10 * time.Second):
t.Fatalf("timed out waiting for build event")
case event := <-watch.ResultChan():
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so we skip them and continue waiting
// for real Build events.
if event.Type == watchapi.Bookmark {
continue
}
actual := event.Object.(*buildv1.Build)
Comment on lines 338 to 347
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify unsafe direct type assertions after watch.ResultChan() reads.
# Expected result: occurrences in this file where event.Object is asserted as *buildv1.Build
# without guarding channel closure/object type first.
rg -nP --type=go -C3 'case\s+\w+\s*:=\s*<-\s*\w+\.ResultChan\(\)|\.\s*Object\.\(\*buildv1\.Build\)' test/extended/builds/webhook.go

Repository: openshift/origin

Length of output: 1269

🏁 Script executed:

# Search for all watch.ResultChan() usages in the file
rg -n 'ResultChan\(\)' test/extended/builds/webhook.go

Repository: openshift/origin

Length of output: 144

🏁 Script executed:

# Search for all event.Object type assertions or accesses
rg -n 'event\.Object' test/extended/builds/webhook.go

Repository: openshift/origin

Length of output: 153

🏁 Script executed:

# Get the full context around both flagged sections to understand the watch handling
sed -n '330,410p' test/extended/builds/webhook.go

Repository: openshift/origin

Length of output: 3201

Guard watch channel closure and type assertions to prevent panics and flaky test failures.

The code at lines 338–347 and 395–403 reads from watch.ResultChan() without checking channel closure and directly asserts event.Object.(*buildv1.Build) without verifying the type. This can panic if the channel closes or if the event is not a Build object (e.g., an Error event from the Kubernetes API). Both locations skip only Bookmark events but should also handle Error events and gracefully skip non-Build payloads.

Proposed fix 
@@
-		case event := <-watch.ResultChan():
+		case event, ok := <-watch.ResultChan():
+			if !ok {
+				t.Fatalf("build watch channel closed")
+			}
 			// Ignore bookmark events from WatchList protocol.
@@
 			if event.Type == watchapi.Bookmark {
 				continue
 			}
-			actual := event.Object.(*buildv1.Build)
+			if event.Type == watchapi.Error {
+				t.Fatalf("watch returned error event: %#v", event.Object)
+			}
+			actual, ok := event.Object.(*buildv1.Build)
+			if !ok {
+				continue
+			}
@@
-			case event := <-watch.ResultChan():
+			case event, ok := <-watch.ResultChan():
+				if !ok {
+					t.Fatalf("build watch channel closed")
+				}
 				// Ignore bookmark events from WatchList protocol.
@@
 				if event.Type == watchapi.Bookmark {
 					continue
 				}
-				build := event.Object.(*buildv1.Build)
+				if event.Type == watchapi.Error {
+					t.Fatalf("watch returned error event: %#v", event.Object)
+				}
+				build, ok := event.Object.(*buildv1.Build)
+				if !ok {
+					continue
+				}
 				t.Fatalf("Unexpected build created: %#v", build)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/builds/webhook.go` around lines 338 - 347, When reading from
watch.ResultChan() in the select branches (the case handling at both the block
around event.Type checks and the later similar block), guard against a closed
channel and non-Build payloads by: replace the blind receive with a two-value
receive (event, ok := <-watch.ResultChan()); if !ok or event == nil then
continue/wait instead of proceeding; treat watchapi.Error events specially by
continuing (skip) rather than asserting; and replace the direct type assertion
event.Object.(*buildv1.Build) with a safe assertion (obj, ok :=
event.Object.(*buildv1.Build)); if ok is false then continue so non-Build
objects don’t cause a panic. Ensure you apply these checks in both places where
you currently check for watchapi.Bookmark so the test won’t fail on closed
channels, Error events, or unexpected object types.

t.Logf("Saw build object %#v", actual)
if actual.Status.Phase != buildv1.BuildPhasePending {
Expand Down Expand Up @@ -377,12 +386,24 @@ func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {

// TODO: improve negative testing
timer := time.NewTimer(time.Second * 5)
select {
case <-timer.C:
// nothing should happen
case event := <-watch.ResultChan():
build := event.Object.(*buildv1.Build)
t.Fatalf("Unexpected build created: %#v", build)
Loop:
for {
select {
case <-timer.C:
// nothing should happen - test passed
break Loop
Copy link
Copy Markdown
Member

@ardaguclu ardaguclu Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason of this change?

Copy link
Copy Markdown
Member Author

@ingvagabund ingvagabund Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously there was only a select statement so catching the bookmark event type would trigger the second event := <-watch.ResultChan() case and left the select without waiting again for checking whether there's another (this time valid) *buildv1.Build object.

case event := <-watch.ResultChan():
// Ignore bookmark events from WatchList protocol.
// When watching with an empty ResourceVersion, the apiserver uses WatchList and sends
// bookmark events to signal sync completion. These are synthetic events (not actual
// Build objects) used for caching coordination, so they should not be treated as
// unexpected builds.
if event.Type == watchapi.Bookmark {
continue
}
build := event.Object.(*buildv1.Build)
t.Fatalf("Unexpected build created: %#v", build)
}
}
}
}
Expand Down