🐛 using bg goroutine to handle work deleting - #209
Merged
openshift-merge-bot[bot] merged 1 commit intoMar 10, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a context-driven background deletion checker for ManifestWork resources, moves deletion lifecycle logic into a new helper, updates NewManifestWorkAgentClient to accept context.Context, and adjusts tests and callsites to use the context-aware API and validate periodic deletion behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cloudevents/clients/work/agent/client/manifestwork_test.go (1)
687-689: Userequire.Eventuallyhere instead of sleeping the whole ticker interval.These waits add about 9 seconds to the package and are still timing-sensitive if the goroutine starts late or the CI node is slow. Poll for the publish/store state instead of sleeping the full interval.
Example pattern
require.Eventually(t, func() bool { published := mockClient.getPublishedWorks() _, exists, err := watcherStore.Get(ctx, "test-cluster", "test-work-to-delete") return err == nil && !exists && len(published) > 0 }, workDeletionCheckInterval+time.Second, 50*time.Millisecond)Also applies to: 769-770, 827-828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go` around lines 687 - 689, Replace the time.Sleep usage in the test with a polling assertion using require.Eventually: instead of sleeping for workDeletionCheckInterval+1*time.Second, poll until the expected conditions are met (e.g., mockClient.getPublishedWorks() shows a publish and watcherStore.Get(ctx, "test-cluster", "test-work-to-delete") returns not exists) within the overall timeout workDeletionCheckInterval+time.Second and with a short tick (e.g., 50ms); apply the same change to the other sleep sites in this file that wait for the deletion check cycle so tests become robust to goroutine/CI slowness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 64-90: The ticker deletion loop is mutating cached *ManifestWork
objects returned by watcherStore.ListAll and calling client.deleteWork outside
the store mutex, which can race with
Patch/meta.SetStatusCondition/Publish/watcherStore.Delete; fix by re-reading the
latest work from the store under the store mutex (use c.Lock()/c.Unlock() around
a fresh lookup of the work), make a deep copy of that object and perform all
mutations (meta.SetStatusCondition, Publish payload creation) on the copy, then
call Patch/Delete as the serialized operation; ensure watcherStore.Delete and
Publish use the deep copy payload and that Patch remains the serialized
operation to avoid interleaving with concurrent updates (apply same change to
the other ticker path at the referenced lines).
---
Nitpick comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go`:
- Around line 687-689: Replace the time.Sleep usage in the test with a polling
assertion using require.Eventually: instead of sleeping for
workDeletionCheckInterval+1*time.Second, poll until the expected conditions are
met (e.g., mockClient.getPublishedWorks() shows a publish and
watcherStore.Get(ctx, "test-cluster", "test-work-to-delete") returns not exists)
within the overall timeout workDeletionCheckInterval+time.Second and with a
short tick (e.g., 50ms); apply the same change to the other sleep sites in this
file that wait for the deletion check cycle so tests become robust to
goroutine/CI slowness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0860231d-edb0-4bbf-bc66-b3b4916e6719
📒 Files selected for processing (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/clients/work/agent/client/manifestwork_test.gopkg/cloudevents/clients/work/clientholder.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go`:
- Around line 687-689: The comment above the Sleep call in manifestwork_test.go
is stale—`workDeletionCheckInterval` is 2 seconds, not 5—so update the comment
to reflect the actual interval or remove the misleading reference; locate the
Sleep invocation that uses workDeletionCheckInterval and modify the comment text
to say the check runs every 2 seconds (or generically "runs at
workDeletionCheckInterval") to keep the comment accurate.
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 279-282: The parse of the CloudEvents data type in ManifestWork
processing (types.ParseCloudEventsDataType called for
common.CloudEventsDataTypeAnnotationKey) returns an error causing deleteWork to
repeatedly return errors for works that are already marked for deletion
(DeletionTimestamp set) with no finalizers; modify deleteWork (and the code path
that reads work.Annotations[common.CloudEventsDataTypeAnnotationKey]) to treat a
missing/malformed annotation as a non-fatal condition during delete: either
remove the work from the local store with a single warning log or mark the work
as skipped/invalid and stop requeuing it (e.g., track a failure flag or
increment an attempt counter) so the ticker loop does not continually log the
same error; ensure changes reference deleteWork, ParseCloudEventsDataType,
common.CloudEventsDataTypeAnnotationKey and the DeletionTimestamp/finalizers
checks so only active/non-deleting works fail hard while deleting works are
cleaned up or silenced.
- Around line 299-307: The current flow in deleteWork calls
c.cloudEventsClient.Publish then c.watcherStore.Delete, which can cause
duplicate deletion events if Delete fails; either reorder to call
c.watcherStore.Delete(work) before c.cloudEventsClient.Publish(ctx, eventType,
workToPublish) (accepting potential lost events), or implement a small two-step
tracking state in watcherStore (e.g., add/use methods like
MarkPendingDeletion(work)/ClearPendingDeletion(work) around Publish) so you mark
the work as "pending publish" before calling c.cloudEventsClient.Publish and
only remove the pending marker after successful Delete; update deleteWork to use
the chosen approach and ensure metrics and error handling
(errors.NewInternalError and metrics.IncreaseWorkProcessedCounter) reflect the
new flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10591b25-0356-4d2c-9c91-5b9184ca3073
📒 Files selected for processing (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/clients/work/agent/client/manifestwork_test.gopkg/cloudevents/clients/work/clientholder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/clients/work/clientholder.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/cloudevents/clients/work/agent/client/manifestwork_test.go (2)
29-29: Add a publish-failure case for the ticker delete path.
mockCloudEventsClient.publishErroris wired up, but none of the new periodic deletion tests exercise the branch wherePublishfails and the work stays in the store. That retry/error path is where regressions and repeated log noise are most likely, so it deserves explicit coverage.Also applies to: 641-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go` at line 29, Add a unit test that exercises the periodic-deletion ticker path when Publish fails by wiring mockCloudEventsClient.publishError to a non-nil error and asserting that the work remains in the store and the retry/error handling path is taken; update the tests around the ticker delete scenarios (the new periodic deletion tests in manifestwork_test.go) to include a case where mockCloudEventsClient.publishError is set, invoke the ticker-driven delete logic (the same test helper that triggers the Publish call), and assert that Publish was called, no deletion occurred from the store, and any expected retry/logging side effects happened.
27-28: Assert the emittedCloudEventsTypehere.This happy-path test only verifies the published
ManifestWorkpayload and store removal. IfdeleteWorkpublishes the wrongAction/SubResource/datatype, the test still passes even though the source would route the event differently. The mock already recordspublishedTypes, so please expose and assert that too.Also applies to: 641-721
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go` around lines 27 - 28, The test currently only asserts publishedWorks and should also assert the recorded publishedTypes to ensure deleteWork emitted the correct CloudEventsType (Action, SubResource, datatype); update the manifestwork_test.go happy-path test(s) to check the mock's publishedTypes slice (the publishedTypes variable declared alongside publishedWorks) contains the expected types (e.g., expected Action/SubResource/datatype values for the deleteWork event) after invoking the code under test, and add similar assertions in the other listed test blocks (lines ~641-721) so the event routing metadata is validated alongside the payload and store removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 272-307: The code currently holds c.Lock() across the long-lived
c.cloudEventsClient.Publish call; remove the external call from the critical
section by copying/marking the work to publish while the lock is held (use
latestWork := c.watcherStore.Get(...) and build workToPublish and, if needed,
set a pending-delete marker or a status flag on the in-memory object), then call
c.Unlock() before invoking c.cloudEventsClient.Publish; also wrap the Publish
call in a bounded context (context.WithTimeout) to avoid hangs, and if Publish
returns a non-nil error reacquire c.Lock() to update any state/conditions or
convert the error via cloudeventserrors.ToStatusError. Ensure references:
c.Lock()/c.Unlock(), watcherStore.Get, workToPublish, cloudEventsClient.Publish,
and cloudeventserrors.ToStatusError.
- Around line 51-55: The background goroutine started in
NewManifestWorkAgentClient calls deleteWork every 2s which dereferences
c.cloudEventsClient (c.cloudEventsClient.Publish) without a nil check; update
the code to guard against a nil cloudEventsClient—either validate
cloudEventsClient is non-nil in NewManifestWorkAgentClient before spawning the
goroutine (and avoid starting the goroutine or return an error/early exit) or
add a nil check at the start of deleteWork so it returns gracefully when
c.cloudEventsClient is nil instead of calling Publish; refer to
NewManifestWorkAgentClient, deleteWork, and c.cloudEventsClient.Publish to
locate and patch the logic.
---
Nitpick comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go`:
- Line 29: Add a unit test that exercises the periodic-deletion ticker path when
Publish fails by wiring mockCloudEventsClient.publishError to a non-nil error
and asserting that the work remains in the store and the retry/error handling
path is taken; update the tests around the ticker delete scenarios (the new
periodic deletion tests in manifestwork_test.go) to include a case where
mockCloudEventsClient.publishError is set, invoke the ticker-driven delete logic
(the same test helper that triggers the Publish call), and assert that Publish
was called, no deletion occurred from the store, and any expected retry/logging
side effects happened.
- Around line 27-28: The test currently only asserts publishedWorks and should
also assert the recorded publishedTypes to ensure deleteWork emitted the correct
CloudEventsType (Action, SubResource, datatype); update the manifestwork_test.go
happy-path test(s) to check the mock's publishedTypes slice (the publishedTypes
variable declared alongside publishedWorks) contains the expected types (e.g.,
expected Action/SubResource/datatype values for the deleteWork event) after
invoking the code under test, and add similar assertions in the other listed
test blocks (lines ~641-721) so the event routing metadata is validated
alongside the payload and store removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5aaee00-6c33-404f-b256-7f18f6a5e3b2
📒 Files selected for processing (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/clients/work/agent/client/manifestwork_test.gopkg/cloudevents/clients/work/clientholder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/clients/work/clientholder.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.go (3)
263-299:⚠️ Potential issue | 🟠 MajorDo not hold
c.Lock()acrossPublish.
Publishis an external call on the long-lived constructor context. If the broker stalls, every concurrentPatchwaits behind this mutex until that call returns. Build the payload under lock, release it before the network call, and reacquire only for store mutation/bookkeeping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go` around lines 263 - 299, The code holds c.Lock()/defer c.Unlock() across a network call (c.cloudEventsClient.Publish), causing blocking; instead, limit the critical section to only reading/mutating shared state: use c.Lock() around watcherStore.Get and building workToPublish (references: c.Lock(), latestWork, workToPublish, meta.SetStatusCondition), then c.Unlock() before calling c.cloudEventsClient.Publish(ctx, ...); after Publish, reacquire the lock only if you need to update the store/bookkeeping (e.g., any mutations to watcherStore or fields on c), and convert Publish errors to a status error as before (cloudeventserrors.ToStatusError with common.ManifestWorkGR, latestWork.Name).
52-84:⚠️ Potential issue | 🟠 MajorSkip the background delete loop when no CloudEvents publisher is configured.
NewManifestWorkAgentClient(..., nil)is still used, but the new worker runs unconditionally anddeleteWorkdereferencesc.cloudEventsClient.Publish. A deleting work will now panic the process instead of failing gracefully.Possible fix
func NewManifestWorkAgentClient( ctx context.Context, watcherStore store.ClientWatcherStore[*workv1.ManifestWork], cloudEventsClient generic.CloudEventsClient[*workv1.ManifestWork], ) *ManifestWorkAgentClient { client := &ManifestWorkAgentClient{ cloudEventsClient: cloudEventsClient, watcherStore: watcherStore, } + + if cloudEventsClient == nil { + return client + } // Start a background goroutine to periodically check for works that need deletion. go wait.UntilWithContext(ctx, func(ctx context.Context) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go` around lines 52 - 84, NewManifestWorkAgentClient currently always starts the background goroutine that calls deleteWork which dereferences c.cloudEventsClient.Publish and will panic when cloudEventsClient is nil; update NewManifestWorkAgentClient to skip starting the wait.UntilWithContext worker when cloudEventsClient == nil (i.e., only launch the deletion loop if cloudEventsClient != nil), and also add a defensive nil check inside deleteWork (or the Publish call site) to return gracefully if c.cloudEventsClient is nil; refer to NewManifestWorkAgentClient, deleteWork, cloudEventsClient.Publish, watcherStore.ListAll, wait.UntilWithContext, and workDeletionCheckInterval when making the change.
297-305:⚠️ Potential issue | 🟠 MajorA successful deletion publish can be replayed on every scan if local removal fails.
The code publishes first and only then deletes from
watcherStore. IfPublishsucceeds butDeletefails, the work stays in cache and the next 2-second sweep emits another deletion event for the same object. Please make those two steps idempotent together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go` around lines 297 - 305, Do the delete-first-then-publish flow to make the pair idempotent: call c.watcherStore.Delete(latestWork) before c.cloudEventsClient.Publish(...), and if Delete fails return immediately (no Publish); if Delete succeeds but Publish fails, attempt to restore the work back into the watcherStore (e.g. re-add or put latestWork) and return the publish error; use c.watcherStore.Delete, c.watcherStore.Add/Put (or equivalent), c.cloudEventsClient.Publish and metrics.IncreaseWorkProcessedCounter to update metrics in the appropriate branches so you don’t end up repeatedly publishing the same deletion on subsequent sweeps.
🧹 Nitpick comments (1)
pkg/cloudevents/clients/work/agent/client/manifestwork_test.go (1)
687-689: Make the deletion interval injectable in tests instead of sleeping for real time.These cases now wait
workDeletionCheckInterval + 1seach, so the suite pays several seconds of wall-clock time and still depends on scheduler timing. A test hook for the ticker/interval would make this path much faster and more deterministic.Also applies to: 769-770, 827-828, 853-854
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go` around lines 687 - 689, The tests currently sleep for real time using workDeletionCheckInterval + 1s which makes them slow and flaky; make the deletion-check interval injectable (or the ticker controllable) so tests can drive checks deterministically. Change the code that starts the deletion checker (the logic that references workDeletionCheckInterval) to accept an interval parameter or a ticker/channel factory (or expose a package-level variable like workDeletionTickerFactory) so tests can supply a short interval or a fake ticker/clock, then update manifestwork_test.go to replace time.Sleep(...) with triggering the fake ticker or using a microsecond interval; update all occurrences that sleep (the uses of workDeletionCheckInterval in tests) to use the new injectable hook. Ensure the production default still uses the original workDeletionCheckInterval value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 263-299: The code holds c.Lock()/defer c.Unlock() across a network
call (c.cloudEventsClient.Publish), causing blocking; instead, limit the
critical section to only reading/mutating shared state: use c.Lock() around
watcherStore.Get and building workToPublish (references: c.Lock(), latestWork,
workToPublish, meta.SetStatusCondition), then c.Unlock() before calling
c.cloudEventsClient.Publish(ctx, ...); after Publish, reacquire the lock only if
you need to update the store/bookkeeping (e.g., any mutations to watcherStore or
fields on c), and convert Publish errors to a status error as before
(cloudeventserrors.ToStatusError with common.ManifestWorkGR, latestWork.Name).
- Around line 52-84: NewManifestWorkAgentClient currently always starts the
background goroutine that calls deleteWork which dereferences
c.cloudEventsClient.Publish and will panic when cloudEventsClient is nil; update
NewManifestWorkAgentClient to skip starting the wait.UntilWithContext worker
when cloudEventsClient == nil (i.e., only launch the deletion loop if
cloudEventsClient != nil), and also add a defensive nil check inside deleteWork
(or the Publish call site) to return gracefully if c.cloudEventsClient is nil;
refer to NewManifestWorkAgentClient, deleteWork, cloudEventsClient.Publish,
watcherStore.ListAll, wait.UntilWithContext, and workDeletionCheckInterval when
making the change.
- Around line 297-305: Do the delete-first-then-publish flow to make the pair
idempotent: call c.watcherStore.Delete(latestWork) before
c.cloudEventsClient.Publish(...), and if Delete fails return immediately (no
Publish); if Delete succeeds but Publish fails, attempt to restore the work back
into the watcherStore (e.g. re-add or put latestWork) and return the publish
error; use c.watcherStore.Delete, c.watcherStore.Add/Put (or equivalent),
c.cloudEventsClient.Publish and metrics.IncreaseWorkProcessedCounter to update
metrics in the appropriate branches so you don’t end up repeatedly publishing
the same deletion on subsequent sweeps.
---
Nitpick comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go`:
- Around line 687-689: The tests currently sleep for real time using
workDeletionCheckInterval + 1s which makes them slow and flaky; make the
deletion-check interval injectable (or the ticker controllable) so tests can
drive checks deterministically. Change the code that starts the deletion checker
(the logic that references workDeletionCheckInterval) to accept an interval
parameter or a ticker/channel factory (or expose a package-level variable like
workDeletionTickerFactory) so tests can supply a short interval or a fake
ticker/clock, then update manifestwork_test.go to replace time.Sleep(...) with
triggering the fake ticker or using a microsecond interval; update all
occurrences that sleep (the uses of workDeletionCheckInterval in tests) to use
the new injectable hook. Ensure the production default still uses the original
workDeletionCheckInterval value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c298f7b0-f073-464a-94f9-57db94b48589
📒 Files selected for processing (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/clients/work/agent/client/manifestwork_test.gopkg/cloudevents/clients/work/clientholder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/clients/work/clientholder.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/cloudevents/clients/work/agent/client/manifestwork.go (2)
52-56:⚠️ Potential issue | 🟠 MajorGuard the background delete path when
cloudEventsClientis nil.
NewManifestWorkAgentClient(ctx, watcherStore, nil)is already used in these tests. As soon as a deleting work reaches the store, the goroutine will hitc.cloudEventsClient.Publish(...)and panic instead of returning an error. Either reject a nil client up front or short-circuitdeleteWorkbefore dereferencing it.Also applies to: 283-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go` around lines 52 - 56, NewManifestWorkAgentClient creates a ManifestWorkAgentClient whose background delete goroutine calls deleteWork and unconditionally uses c.cloudEventsClient.Publish, which panics when NewManifestWorkAgentClient is called with a nil cloudEventsClient in tests; fix by guarding the background delete path: in NewManifestWorkAgentClient either reject nil by returning an error or (preferred) initialize the client but make deleteWork check c.cloudEventsClient for nil and short-circuit (return an error or skip publish) before calling cloudEventsClient.Publish; update ManifestWorkAgentClient/deleteWork to safely handle a nil cloudEventsClient (and similarly protect any other publish calls around the referenced publish site at lines ~283-285).
283-289:⚠️ Potential issue | 🟡 Minor
Publish-then-Deletecan emit the same deletion more than once.If
Publishsucceeds butwatcherStore.Deletefails, the work stays cached and the next sweep republishes the same deleted status. Make this step locally idempotent, or track a pending-deletion state so a successful send is not retried as a fresh event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go` around lines 283 - 289, The code can republish the same deletion if Publish succeeds but watcherStore.Delete fails; fix by making the send-delete sequence locally idempotent: before calling c.cloudEventsClient.Publish, acquire the same mutex (use c.Lock()/c.Unlock()) and mark the work as "pending deletion" in the watcherStore (add a method like watcherStore.SetPendingDeletion(work.Name) or a boolean on the stored entry), then release the lock and call Publish; on Publish failure clear that pending flag, on Publish success attempt watcherStore.Delete and if Delete fails leave the pending-deletion marker so subsequent sweeps skip republishing (and add a background retry to eventually call watcherStore.Delete). Ensure you reference and update workToPublish and work.Name when setting/clearing the pending state and keep using c.watcherStore.Delete for the final removal.
🧹 Nitpick comments (1)
pkg/cloudevents/clients/work/agent/client/manifestwork_test.go (1)
687-720: Replace fixed sleeps withrequire.Eventually.These assertions assume the ticker fires and the goroutine gets scheduled within
workDeletionCheckInterval + 1s. On a loaded CI runner that can still race and make the new deletion tests flaky. Poll the expected publish/store state instead of sleeping a guessed interval.Example pattern
- // Wait for at least one deletion check cycle to complete - // The check runs every 2 seconds, so we wait a bit longer - time.Sleep(workDeletionCheckInterval + 1*time.Second) + require.Eventually(t, func() bool { + for _, pw := range mockClient.getPublishedWorks() { + if pw.Name != "test-work-to-delete" { + continue + } + + _, exists, err := watcherStore.Get(ctx, "test-cluster", "test-work-to-delete") + return err == nil && !exists + } + return false + }, 2*workDeletionCheckInterval, 100*time.Millisecond)Also applies to: 769-781, 827-839
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go` around lines 687 - 720, The test uses a fixed time.Sleep(wait) around workDeletionCheckInterval which can race on CI; replace those sleeps with require.Eventually polls that repeatedly call mockClient.getPublishedWorks() to find the ManifestWork named "test-work-to-delete" and assert the deleted condition (Type == common.ResourceDeleted, Status == metav1.ConditionTrue, Reason == "ManifestsDeleted"), and separately poll watcherStore.Get(ctx, "test-cluster", "test-work-to-delete") until it no longer exists; use a conservative timeout (e.g. several multiples of workDeletionCheckInterval) and a short polling interval to avoid flakes and apply the same replacement for the other occurrences around lines referenced (769-781, 827-839).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 69-81: The sweeper is iterating watcherStore.ListAll() and may
delete works from other namespaces; change the logic in the loop that calls
client.deleteWork to only operate on works matching this client's namespace
(c.namespace) or, if the watcherStore supports a namespace-scoped listing,
replace ListAll() with a namespaced list call (e.g., watcherStore.List(ctx,
c.namespace)) before the for loop so that client.deleteWork(ctx, work) is only
called for works where work.Namespace == c.namespace. Ensure the check
references c.namespace and the delete invocation remains client.deleteWork.
---
Duplicate comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork.go`:
- Around line 52-56: NewManifestWorkAgentClient creates a
ManifestWorkAgentClient whose background delete goroutine calls deleteWork and
unconditionally uses c.cloudEventsClient.Publish, which panics when
NewManifestWorkAgentClient is called with a nil cloudEventsClient in tests; fix
by guarding the background delete path: in NewManifestWorkAgentClient either
reject nil by returning an error or (preferred) initialize the client but make
deleteWork check c.cloudEventsClient for nil and short-circuit (return an error
or skip publish) before calling cloudEventsClient.Publish; update
ManifestWorkAgentClient/deleteWork to safely handle a nil cloudEventsClient (and
similarly protect any other publish calls around the referenced publish site at
lines ~283-285).
- Around line 283-289: The code can republish the same deletion if Publish
succeeds but watcherStore.Delete fails; fix by making the send-delete sequence
locally idempotent: before calling c.cloudEventsClient.Publish, acquire the same
mutex (use c.Lock()/c.Unlock()) and mark the work as "pending deletion" in the
watcherStore (add a method like watcherStore.SetPendingDeletion(work.Name) or a
boolean on the stored entry), then release the lock and call Publish; on Publish
failure clear that pending flag, on Publish success attempt watcherStore.Delete
and if Delete fails leave the pending-deletion marker so subsequent sweeps skip
republishing (and add a background retry to eventually call
watcherStore.Delete). Ensure you reference and update workToPublish and
work.Name when setting/clearing the pending state and keep using
c.watcherStore.Delete for the final removal.
---
Nitpick comments:
In `@pkg/cloudevents/clients/work/agent/client/manifestwork_test.go`:
- Around line 687-720: The test uses a fixed time.Sleep(wait) around
workDeletionCheckInterval which can race on CI; replace those sleeps with
require.Eventually polls that repeatedly call mockClient.getPublishedWorks() to
find the ManifestWork named "test-work-to-delete" and assert the deleted
condition (Type == common.ResourceDeleted, Status == metav1.ConditionTrue,
Reason == "ManifestsDeleted"), and separately poll watcherStore.Get(ctx,
"test-cluster", "test-work-to-delete") until it no longer exists; use a
conservative timeout (e.g. several multiples of workDeletionCheckInterval) and a
short polling interval to avoid flakes and apply the same replacement for the
other occurrences around lines referenced (769-781, 827-839).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c2d2d5-6bba-431c-a55d-87bf3c512f16
📒 Files selected for processing (3)
pkg/cloudevents/clients/work/agent/client/manifestwork.gopkg/cloudevents/clients/work/agent/client/manifestwork_test.gopkg/cloudevents/clients/work/clientholder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudevents/clients/work/clientholder.go
Signed-off-by: Wei Liu <liuweixa@redhat.com>
Member
Author
|
/assign @qiujian16 |
Member
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit Mar 10, 2026
3041045
into
open-cluster-management-io:main
9 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The work cannot be deleted on the agent when deleting a work after it created, e.g.
Use a background goroutine to handle deleting works
Related issue(s)
Fixes #
Summary by CodeRabbit