[itsvishalyadav]

Overview

This PR fixes #486

Signed Commits

• Yes, I signed my commits.

SelfHealingWatch reconnected with an empty resourceVersion, so after every watch timeout the API server replayed every object as a synthetic ADDED event and continuous scanning scanned each of them again. A watch error event was also forwarded, and its *metav1.Status then failed the e.Object.(metav1.Object) assertion in service.go and panicked.

• RunUntilWatchCloses records the version of each event's object with watcher.ResumeFrom, so the next watch carries on from there.
• An error event ends the watch (it is logged, not forwarded). ResumeFrom already forgets the version when the event says it has expired, and Run calls watcher.DropGoneVersion when creating the watch fails with 410 Gone, so the watch then starts over from the current state.
• To reuse the fix(watcher): resume watches from the last object, not from an error's Status #439 helpers, resumeFrom and dropGoneVersion in the watcher package are exported as ResumeFrom and DropGoneVersion; their two callers there are renamed and nothing else changes.

How to Test

go test ./continuousscanning/ -run 'TestSelfHealingWatchReconnect' -count=1 -v

TestSelfHealingWatchReconnect has three cases: resuming from the last object, an error event not being forwarded (and keeping the version), and an expired version being forgotten. On main they fail with expected: "8" actual: "", expected: "7" actual: "" and should have 1 item(s), but has 2; with the fix they pass.

go test ./continuousscanning/ -count=1 and go test ./watcher/ -skip TestRegistryCommandWatch -count=1 pass (TestRegistryCommandWatch needs Docker and fails on Windows on main too), go build and go vet of ./watcher/... ./continuousscanning/... ./mainhandler/... are clean, and golangci-lint reports nothing new. I haven't run it on a live cluster.

Checklist before requesting a review

• My code follows the style guidelines of this project
• I have commented on my code, particularly in hard-to-understand areas
• I have performed a self-review of my code
• If it is a core feature, I have added thorough tests.
• New and existing unit tests pass locally with my changes

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 40c0b795-1a4a-4c62-a4fb-08ece403e4c5

Commits

Reviewing files that changed from the base of the PR and between 47e9ae6 and 3759d81.

Files selected for processing: 5

• continuousscanning/watchbuilder.go
• continuousscanning/watchbuilder_test.go
• watcher/commandswatcher.go
• watcher/watchretry.go
• watcher/watchretry_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[blacksoxx]

No blocking finding from a read of 3759d81 against main 47e9ae6. ResumeFrom keeps the version on a non-410 error event and forgets it on Expired or Gone, and Run drops it when the watch request itself comes back 410, so the three test cases match what the helper does. I had the same panic on my list; stopping the watch on the error event fixes it where it starts, and the resume removes the replay, which a guard in the service alone would not have.

Two P3s, your call:

• inline, on the resume: bookmarks.
• service.go:52 and :68 still assert e.Object.(metav1.Object) without the ok form. Error events no longer reach them, so the panic path is closed; a comma-ok there would keep it closed if another event source is ever plugged into that channel.

Not run: the PR's tests, only read; the helpers on main and the fixtures were checked in code.

[blacksoxx]

[P3] The watch is created with ListOptions{} and no AllowWatchBookmarks. On a quiet GVR the resumed version can age past the server's window between reconnects, which turns the next reconnect into a 410, DropGoneVersion and a full replay anyway. AllowWatchBookmarks: true in listen keeps the version fresh, and ResumeFrom already records Bookmark events.

[matthyx]

Approve at 3759d81 (base main 47e9ae6).

• Need: #486 is real in the code. SelfHealingWatch reconnects with an empty resourceVersion, so each reconnect replays every object as ADDED and rescans it. Forwarded watch.Error events carry a *metav1.Status and would panic at the metav1.Object assertion in service.go.
• Fix: it reuses the helpers from #439, so ResumeFrom records the version and DropGoneVersion handles a 410 on watch creation. An error event stops the watch without being forwarded, and the version is kept unless the server says it expired. The watcher package has no dependency on continuousscanning, so there is no import cycle. The export renames are mechanical.
• Validation: go build, go vet and go test ./continuousscanning/ pass at the head. The new TestSelfHealingWatchReconnect passes with the fix. With watchbuilder.go reverted to main it fails with the expected "8"/"" and "7"/"". CI is green. I did not run it on a live cluster, and I did not re-run ./watcher tests.
• No duplicate PRs found. #439 is the earlier fix for the watcher package and is complementary.
• No blockers. The two P3s from blacksoxx (AllowWatchBookmarks, comma-ok in service.go) are optional follow-ups. Run also retries without backoff when watch creation fails, but that is pre-existing.