ARO-28817: fix(server): exclude deleting resources from spec resync response - #570
Draft
twolff-gh wants to merge 2 commits into
Draft
ARO-28817: fix(server): exclude deleting resources from spec resync response#570twolff-gh wants to merge 2 commits into
twolff-gh wants to merge 2 commits into
Conversation
…O-28817) GRPCBrokerService.List and SourceClientImpl.resyncConsumer return soft-deleted resources to the agent during spec resync because FindByConsumerName uses Unscoped(). The agent treats the resync response as "apply these specs" and re-creates ManifestWorks that it already deleted, producing an infinite delete-then-recreate loop. This is the remaining exposure after ARO-28432 (OnCreate guard), AROSLSRE-1547 (MarkAsDeleting idempotency), and AROSLSRE-1633 (OnUpdate guard), which guard event-driven paths but not the resync path. Filtering is safe because the sdk-go source client's resync handler detects resources present on the agent but absent from the server's response and sends a delete_request for them. Jira: ARO-28817 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe DAO, broker resynchronization response, and client status-hash batches now exclude resources with a deletion timestamp. Tests cover deleting and live resources. ChangesResynchronization filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/maestro/server/grpc_broker_test.go`:
- Around line 281-283: Strengthen the List assertions in the exclusion and
inclusion tests around svc.List by decoding each returned ce.Event and verifying
the exact resource ID sets: only live-1 for the deleting-resource exclusion
case, and res-1 and res-2 for the inclusion case. Retain the existing error
checks and use identity-based assertions so duplicates or omitted resources
cannot satisfy the tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df99b5fc-0b8b-409e-a89b-7ba2b5dc03e7
📒 Files selected for processing (3)
cmd/maestro/server/grpc_broker.gocmd/maestro/server/grpc_broker_test.gopkg/client/cloudevents/source_client.go
Comment on lines
+281
to
+283
| evts, err := svc.List(context.Background(), types.ListOptions{ClusterName: "cluster1"}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(evts).To(HaveLen(1), "List must exclude resources marked as deleting") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the returned resource identities, not only the event count.
HaveLen(1) can pass if List returns the deleting resource and omits live-1. HaveLen(2) can pass if it returns the same live resource twice. Decode each ce.Event and assert the exact resource ID sets: {live-1} for the exclusion test and {res-1, res-2} for the inclusion test.
Also applies to: 304-306
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/maestro/server/grpc_broker_test.go` around lines 281 - 283, Strengthen
the List assertions in the exclusion and inclusion tests around svc.List by
decoding each returned ce.Event and verifying the exact resource ID sets: only
live-1 for the deleting-resource exclusion case, and res-1 and res-2 for the
inclusion case. Retain the existing error checks and use identity-based
assertions so duplicates or omitted resources cannot satisfy the tests.
…merName (ARO-28817) Move the root fix from application-layer filtering to the DAO query, adding `AND deleted_at IS NULL` to FindByConsumerName. This follows the FindUndelivered precedent and prevents future callers from silently receiving soft-deleted resources. The application-layer guards in GRPCBrokerService.List and resyncConsumer are kept as defense-in-depth. Also adds TestResyncConsumerExcludesDeletingResources to close the MQTT resync test coverage gap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/dao/mocks/resource.go`:
- Line 78: Update the soft-delete check in FindUndelivered to use the nullable
state, requiring !resource.DeletedAt.Valid instead of
resource.DeletedAt.Time.IsZero(), while preserving the existing consumer-ID
filter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a5ecdcb-3aef-4a8e-932d-e3cee6cf9c76
📒 Files selected for processing (3)
pkg/client/cloudevents/source_client_test.gopkg/dao/mocks/resource.gopkg/dao/resource.go
| var resources api.ResourceList | ||
| for _, resource := range d.resources { | ||
| if resource.ConsumerName == consumerID { | ||
| if resource.ConsumerName == consumerID && resource.Meta.DeletedAt.Time.IsZero() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'resource\.go$' . | sed 's#^\./##'
echo "== inspect target mock =="
file="$(fd 'resource\.go$' . | grep 'pkg/dao/mocks/resource.go' | head -n 1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo "== search dao predicate usage =="
rg -n "deleted_at|DeletedAt|DeletedAt.*IS NULL|IS NULL.*DeletedAt|DeletedAt.Time.IsZero|DeletedAt.Valid" .Repository: openshift-online/maestro
Length of output: 12264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect real DAO predicate =="
sed -n '90,150p' pkg/dao/resource.go | cat -n
echo "== inspect Resource metadata =="
sed -n '1,90p' pkg/api/resource_types.go | cat -n
sed -n '45,65p' pkg/api/metadata_types.go | cat -n
echo "== check go.mod gorm versions =="
rg -n 'github.com/go-gorm/gorm|gorm.io/gorm' go.mod go.sum || true
echo "== deterministic GORM DeletedAt null predicate simulation =="
python3 - <<'PY'
# No repo code is executed; this models the observed filter logic.
cases = [
{"name": "active gorm.DeletedAt default", "Valid": False, "Time": "0001-01-01T00:00:00Z"},
{"name": "soft deleted", "Valid": True, "Time": "2024-01-01T12:00:00Z"},
{"name": "invalid valid state same as active in isZero", "Valid": True, "Time": "0001-01-01T00:00:00Z"},
{"name": "invalid valid state same as deleted in isZero", "Valid": False, "Time": "2024-01-01T12:00:00Z"},
]
print("case\tdeleted_at_null\tmock_Time.IsZero()\tmock_!Valid")
for c in cases:
time_is_zero = c["Time"] == "0001-01-01T00:00:00Z"
print(f"{c['name']}\t{not c['Valid']}\t{time_is_zero}\t{not c['Valid']}")
PYRepository: openshift-online/maestro
Length of output: 6344
Use the nullable soft-delete state in the mock filters.
FindUndelivered also filters on resource.DeletedAt.Time.IsZero() while the DAO uses deleted_at IS NULL; gorm.DeletedAt represents that state with Valid. Use !resource.DeletedAt.Valid here so inconsistent Time/Valid values do not hide or expose deleted resources.
Proposed fix
- if len(resource.Status) == 0 && resource.DeletedAt.Time.IsZero() && resource.CreatedAt.Before(cutoff) {
+ if len(resource.Status) == 0 && !resource.DeletedAt.Valid && resource.CreatedAt.Before(cutoff) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/dao/mocks/resource.go` at line 78, Update the soft-delete check in
FindUndelivered to use the nullable state, requiring !resource.DeletedAt.Valid
instead of resource.DeletedAt.Time.IsZero(), while preserving the existing
consumer-ID filter.
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
1 participant
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.
GRPCBrokerService.List and SourceClientImpl.resyncConsumer return soft-deleted resources to the agent during spec resync because FindByConsumerName uses Unscoped(). The agent treats the resync response as "apply these specs" and re-creates ManifestWorks that it already deleted, producing an infinite delete-then-recreate loop.
This is the remaining exposure after ARO-28432 (OnCreate guard), AROSLSRE-1547 (MarkAsDeleting idempotency), and AROSLSRE-1633 (OnUpdate guard), which guard event-driven paths but not the resync path. Filtering is safe because the sdk-go source client's resync handler detects resources present on the agent but absent from the server's response and sends a delete_request for them.
Jira: ARO-28817
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit