Skip to content

Fix handling of watch members in host store - #71

Merged
gonzolino merged 6 commits into
ironcore-dev:mainfrom
sap-contributions:fix/watch-members
Aug 6, 2026
Merged

Fix handling of watch members in host store#71
gonzolino merged 6 commits into
ironcore-dev:mainfrom
sap-contributions:fix/watch-members

Conversation

@gonzolino

@gonzolino gonzolino commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Proposed Changes

  • Add mutex for watch members
  • Populate members on watch creation to make sure the watch contains all
    matched objects from the start

Summary by CodeRabbit

  • Bug Fixes
    • Watches now include existing matching objects immediately when started.
    • Watch results reliably update as objects enter or leave the selected scope.
    • Deletion events are emitted when previously matching objects no longer qualify.
    • Unrelated deletion events are no longer delivered.
    • Improved consistency and reliability of watch notifications during concurrent updates.

@gonzolino
gonzolino requested review from a team as code owners August 4, 2026 08:32
@github-actions github-actions Bot added the bug Something isn't working label Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Store.Watch now initializes membership from existing matching objects before registration. Event delivery synchronizes membership updates and emits deletion events when objects leave the watch scope.

Changes

Watch membership synchronization

Layer / File(s) Summary
Initial watch membership
storeutils/host/watch.go, storeutils/host/store.go
The watch adds a mutex for membership protection. Store.Watch uses the provided context, wraps listing errors, and records matching object IDs before registration.
Event membership updates and validation
storeutils/host/store.go, storeutils/host/store_test.go
Event handling updates membership under the mutex, suppresses deletes for untracked objects, forwards deletes for tracked objects, and emits synthetic deletes when objects leave scope. Tests cover initial membership, filter transitions, and duplicate-event prevention.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Store
  participant Watch
  participant EventConsumer
  Store->>Watch: Enqueue object event
  Watch->>Watch: Lock membership and evaluate filter
  Watch->>Watch: Update tracked object IDs
  Watch-->>EventConsumer: Forward update or synthetic delete
Loading

Possibly related PRs

Suggested labels: size/L

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description includes the required Proposed Changes section and clearly explains the mutex and initial membership changes.
Title check ✅ Passed The title clearly identifies the main change to watch member handling in the host store.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@storeutils/host/store.go`:
- Around line 283-303: Update Store.Watch so the initial s.List snapshot and
watcher registration form one consistent boundary under the store’s
event-publication coordination. Prevent create, update, or delete operations
from being published between snapshot capture and watches.Insert, or capture a
revision and replay all events from that revision before registration; ensure
the watcher’s members and event stream include every change after the snapshot.
- Around line 371-379: Update the event handling around handler.matches so
store.WatchEventTypeDeleted events are processed first: remove the ID and
forward the deletion only when handler.members already contains it. Run the
existing selector matching and transition logic only for non-deleted events,
preserving current behavior for matching and out-of-scope objects.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 170d8ae3-b4eb-4225-bbf6-38d7ba61a182

📥 Commits

Reviewing files that changed from the base of the PR and between aa1a26a and ee2544a.

📒 Files selected for processing (2)
  • storeutils/host/store.go
  • storeutils/host/watch.go

Comment thread storeutils/host/store.go
Comment thread storeutils/host/store.go
@gonzolino

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@gonzolino

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Comment thread storeutils/host/store.go Outdated

@lukasfrank lukasfrank left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you maybe add some simple tests?

Comment thread storeutils/host/store.go
Comment thread storeutils/host/store.go Outdated
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
Deleted events previously went through the filter matcher, which could
silently drop them if the object no longer matched the watch's predicate.

Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@storeutils/host/store_test.go`:
- Line 247: Wrap the long Dummy object literals in the test cases around
dummyStore.Create, including the cases using IDs "delete-untracked" and the line
260 counterpart, so each stays within the 120-character limit without changing
test behavior.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 89ef8a1c-c442-4117-8137-7f3859eb9943

📥 Commits

Reviewing files that changed from the base of the PR and between aa1a26a and e2acaff.

📒 Files selected for processing (3)
  • storeutils/host/store.go
  • storeutils/host/store_test.go
  • storeutils/host/watch.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • storeutils/host/watch.go
  • storeutils/host/store.go

Comment thread storeutils/host/store_test.go Outdated
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
@gonzolino

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
@gonzolino
gonzolino merged commit 2fea714 into ironcore-dev:main Aug 6, 2026
8 checks passed
@gonzolino
gonzolino deleted the fix/watch-members branch August 6, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants