Fix loadConcurrency slot leak on forced cancel

[lubronzhan]

Summary

• Fixes node-agent: data-path watcher goroutine outlives deletion of its own DataUpload CR, permanently holding a loadConcurrency slot #10207: a microServiceBRWatcher goroutine in node-agent (and the loadConcurrency slot it holds in dataPathMgr) could outlive the deletion of its DataUpload/DataDownload CR, since the actual data-mover pod it's waiting on never terminated.
• Root cause: the data-mover pod (pkg/datamover) only reacts to cancellation via Spec.Cancel turning true (its informer only registers an UpdateFunc). If the CR is deleted directly instead — e.g. its finalizer is force-removed, bypassing the normal Spec.Cancel handshake — the pod's informer gets a Delete event it silently ignores, the pod keeps running obliviously, node-agent's watcher never sees it terminate, and the dataPathMgr entry/loadConcurrency slot leak forever.
• An earlier version of this PR called closeDataPath directly from tryCancelDataUpload/tryCancelDataDownload in the node-agent controllers. Per review feedback, that was reverted: releasing the local watcher there doesn't wait for confirmation the pod actually stopped, which can abandon a still-running data-mover pod.
• Fix: add a DeleteFunc handler alongside the existing UpdateFunc on the pod's own DataUpload/DataDownload informer (pkg/datamover/backup_micro_service.go, restore_micro_service.go). If the CR is deleted while the pod is still InProgress, the pod now cancels and exits cleanly through the same path as a normal cancel, regardless of which node's reconciler performed the deletion. node-agent's existing watcher then observes the pod's real termination and releases its dataPathMgr entry through the normal completion callback — no changes needed on the node-agent controller side.

Known trade-off (documented in code comments)

Adding a second, independent trigger (DeleteFunc) alongside the existing Spec.Cancel-driven UpdateFunc opens a narrow race: if both fire for the same object before its data path was ever created, a redundant resultSignal send can block until the pod process exits, which happens within moments regardless via Shutdown()/funcExitWithMessage. This is a same-process, sub-second goroutine leak with no effect on the operation's outcome or on node-agent's own cleanup (which watches the pod's actual phase, not this internal channel) — considered acceptable given the added complexity a guard would require versus this bounded, effectively unobservable impact.

Test plan

• Added TestHandleDataUploadDelete/TestHandleDataDownloadDelete covering: direct object, informer tombstone, wrong name, wrong phase, and non-DataUpload/DataDownload objects.
• Verified new tests fail without the DeleteFunc wiring and pass with it.
• go build ./... and go test ./pkg/controller/... ./pkg/datamover/... ./pkg/datapath/... pass (one pre-existing, unrelated TestAPIs envtest failure present on main too).

AI-Tool-Used: Claude Code
AI-Tool-Use-Level: Category 2 (Medium)
AI-Code-Category: Category 1 (Production)

[netlify]

👷 Deploy request for velero pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	293339e

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[kaovilai]

dataPathMgr is local to each node-agent, but any node’s reconciler can win the forced-cancel status update above. If a non-owning node marks this DataUpload as Canceled, this call finds no local data path; meanwhile, the owning node subsequently sees a terminal CR and removes the finalizer without closing its watcher. The original watcher and loadConcurrency slot can therefore still leak. Please ensure an InProgress forced cancellation is finalized by du.Status.Node, or have terminal-state reconciliation close any matching local data path before removing the finalizer. The same issue applies to DataDownload. A multi-node regression test where only the owning reconciler has the watcher would cover this race.

[Lyndon-Li]

tryCancelDataUpload/tryCancelDataDownload (invoked when a CR is being deleted, or when its cancel isn't handled within the cancel delay window) mark the CR Canceled directly without calling closeDataPath

This is as the expectation. Actually, we cannot call the closeDataPath at this place, otherwise, the data mover pod will left as orphan.
The right approach is, set cancel to du -> data mover pod detects the cancel -> data mover pod close itself -> vgdp watcher detects the close of the pod -> vgdp watcher notify the controller.

[lubronzhan]

tryCancelDataUpload/tryCancelDataDownload (invoked when a CR is being deleted, or when its cancel isn't handled within the cancel delay window) mark the CR Canceled directly without calling closeDataPath

This is as the expectation. Actually, we cannot call the closeDataPath at this place, otherwise, the data mover pod will left as orphan. The right approach is, set cancel to du -> data mover pod detects the cancel -> data mover pod close itself -> vgdp watcher detects the close of the pod -> vgdp watcher notify the controller.

Got it. Then the problem is somehow the DataUPload CR disappeared before the data mover pod act on it? Then in this case, should we consider handle this special case? By adding a DeleteFunc maybe?

[lubronzhan]

tryCancelDataUpload/tryCancelDataDownload (invoked when a CR is being deleted, or when its cancel isn't handled within the cancel delay window) mark the CR Canceled directly without calling closeDataPath

This is as the expectation. Actually, we cannot call the closeDataPath at this place, otherwise, the data mover pod will left as orphan. The right approach is, set cancel to du -> data mover pod detects the cancel -> data mover pod close itself -> vgdp watcher detects the close of the pod -> vgdp watcher notify the controller.

Got it. Then the problem is somehow the DataUPload CR disappeared before the data mover pod act on it? Then in this case, should we consider handle this special case? By adding a DeleteFunc maybe?

Ohk, there might be a race between UpdateFunc and DeleteFunc, and cancelDataUpload/cancelDataDownload are not idempotent. So the second r.resultSignal <- dataPathResult could result in a gorutine leak. But this leak is part of data-mover, and as part of deletion process, so it's only ephermeral and could be ignored @Lyndon-Li do you think it's worthy to add?

[Lyndon-Li]

I think we still need to figure out some questions:

1. When the DU CR is deleted, what happened to the data mover pod?
2. If the data mover pod is either deleted or completed or failed, why the VGDP watch fell to waiting forever?

As my understanding and the expectation:

1. DU deletion should not be a trigger of data mover cancel. Two reasons: a. it is not a normal operation, DU is never deleted under any of the normal cluster operations; b. it is adding the complexity
2. No matter whether DU is there, the data mover pod should be able to complete appropriately
3. Once the data mover pod completes, VGDP watcher should be able to complete
4. So even if DU deletion happens by mistake, the only consequence is the data mover pod will continue until complete, which is not unacceptable