Track multiple sources per vanflow record during purge. by AryanP123 · Pull Request #2572 · skupperproject/skupper

AryanP123 · 2026-09-10T19:24:51Z

Summary by CodeRabbit

Bug Fixes
- Improved source cleanup so removing one source no longer deletes records still associated with other sources.
- Records are now removed only when no associated sources remain.
- Improved handling of records shared by multiple sources, preserving contributions from each source.
- Source removal and record updates now consistently trigger the appropriate change or deletion notifications.
- Source cleanup now works reliably across different store configurations.
- Process reconciliation now correctly recognizes records associated with multiple sources.

coderabbitai · 2026-09-10T19:25:14Z

📝 Walkthrough

Walkthrough

The store now tracks every source that asserts a record. Source purge removes one assertion and deletes the record only when no sources remain. StatusSync, Collector, and process reconciliation use the updated store behavior.

Changes

Source affinity purge

Layer / File(s)	Summary
Source metadata contract `pkg/vanflow/store/store.go`	`Metadata` now stores all asserting sources and provides source membership, add, and remove helpers. The store interface adds `RemoveSource` and `DetachSource`.
Multi-source store behavior `pkg/vanflow/store/syncmap.go`, `pkg/vanflow/store/syncmap_test.go`	The store preserves source snapshots, indexes every source, removes individual source assertions, and retains records asserted by other sources. Tests cover indexing, callbacks, alias protection, retention, and deletion.
Purge and ownership integration `internal/flow/status.go`, `cmd/network-observer/internal/collector/collector.go`, `cmd/network-observer/internal/collector/processes.go`	Purge callers delegate cleanup to `RemoveSource`. Process reconciliation checks source membership with `HasSource` and detaches only the current source.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant StatusSync
  participant Collector
  participant syncMapStore
  participant SourceIndexer
  StatusSync->>syncMapStore: RemoveSource(source)
  Collector->>syncMapStore: RemoveSource(source)
  syncMapStore->>SourceIndexer: Find entries indexed for source
  syncMapStore->>syncMapStore: Remove source assertion
  syncMapStore-->>StatusSync: Return removed count
  syncMapStore-->>Collector: Return removed count

Merge Risk: 🟡 Moderate · up to 3c09d

A caller can corrupt source membership and cause purge to retain or remove records incorrectly. Clone source metadata on all read paths before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: tracking multiple sources for each Vanflow record during purge.
Linked Issues check	✅ Passed	The PR meets the coding requirements in issue `#2568`. `Metadata` stores `Sources []SourceRef`. `AddSource`, `RemoveSource`, and `HasSource` manage source membership. `SourceIndexer` indexes every sourc…
Out of Scope Changes check	✅ Passed	The changed files support issue `#2568`. Store changes implement multi-source tracking and source-specific removal. Purge and process reconciliation changes apply that behavior at the required call site…

Fix all pre-merge checks with AI

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

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: cefe1daa-79be-40ed-99d3-3271ce3510b4

📥 Commits

Reviewing files that changed from the base of the PR and between 2fd49d7 and 425da68.

📒 Files selected for processing (5)

cmd/network-observer/internal/collector/collector.go
internal/flow/status.go
pkg/vanflow/store/store.go
pkg/vanflow/store/syncmap.go
pkg/vanflow/store/syncmap_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

c-kruse · 2026-09-10T20:10:43Z

 type Metadata struct {
 	LastUpdate time.Time

+	// Source is the first source that asserted this record.


Looks like the only place this is used (outside of store) is the network observer process mapping stuff. I think it'd be better to stop carrying around a Source field and to add a HasSource(source SourceRef) bool method instead - the process controller thing will work fine that way.

c-kruse · 2026-09-10T20:35:26Z

+	m.ensureSources()
+	for i, existing := range m.Sources {
+		if sourceRefEqual(existing, source) {
+			m.Sources = append(m.Sources[:i], m.Sources[i+1:]...)


I think we need to be careful to avoid race conditions/consistency problems here and copy the slice instead of mutating/shifting its contents. (maybe in AddSource too?) Store methods return Entry/Metadata by value on purpose so that the caller's copy doesn't get changed out from under it. Now with a slice (instead of a string) and the shared backing array it points to, mutating that slice's contents might break guarantees.

Maybe something like this instead?

i := slices.Index(m.Sources, source) if i < 0 { return false } m.Sources = slices.Delete(slices.Clone(m.Sources), i, i+1) ...

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/vanflow/store/store.go (1)

21-21: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Clone Sources at every syncMapStore ownership boundary.

Replace stores caller-provided Entry values without cloning. Get, List, and Index return entries that share the store's Sources backing arrays. Event callbacks receive the same shared arrays. A caller can change entry.Sources[i] without a store method, so SourceIndex remains stale and RemoveSource can miss records.

Clone entries when syncMapStore accepts, returns, or dispatches them. Add regression tests for mutation through Get, Index, and event handlers.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: cbdad9f7-dbb0-4f6a-91e9-7b9faa532620

📥 Commits

Reviewing files that changed from the base of the PR and between 3770e43 and fbb6f6a.

📒 Files selected for processing (4)

cmd/network-observer/internal/collector/processes.go
pkg/vanflow/store/store.go
pkg/vanflow/store/syncmap.go
pkg/vanflow/store/syncmap_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/vanflow/store/store.go (1)

21-21: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Clone Sources on every read path. Get, List, and Index return shallow Entry copies, while cloneEntry is not used there. A caller can mutate Entry.Sources, changing m.items without updating the source index. Return cloneEntry results from all three methods and add mutation-isolation coverage.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 678528cf-7999-47f9-a7a0-a3db1ad2ac39

📥 Commits

Reviewing files that changed from the base of the PR and between fbb6f6a and 3c09d7a.

📒 Files selected for processing (4)

cmd/network-observer/internal/collector/processes.go
pkg/vanflow/store/store.go
pkg/vanflow/store/syncmap.go
pkg/vanflow/store/syncmap_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

pkg/vanflow/store/syncmap.go
pkg/vanflow/store/syncmap_test.go
pkg/vanflow/store/store.go
cmd/network-observer/internal/collector/processes.go

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Track multiple sources per vanflow record during purge.

425da68

Fixes skupperproject#2568

AryanP123 requested review from c-kruse, fgiorgetti and nluaces as code owners September 10, 2026 19:24

coderabbitai Bot reviewed Sep 10, 2026

View reviewed changes

Comment thread pkg/vanflow/store/syncmap.go Outdated

Comment thread pkg/vanflow/store/syncmap.go

Fix source index snapshots and gofmt.

3770e43

c-kruse reviewed Sep 10, 2026

View reviewed changes

Address review: HasSource and clone Sources slice.

fbb6f6a

coderabbitai Bot reviewed Sep 11, 2026

View reviewed changes

Comment thread cmd/network-observer/internal/collector/processes.go

Use DetachSource in process reconciliation.

3c09d7a

coderabbitai Bot reviewed Sep 11, 2026

View reviewed changes

Navigation Menu

Uh oh!

Track multiple sources per vanflow record during purge. - #2572

Track multiple sources per vanflow record during purge.#2572
AryanP123 wants to merge 4 commits into
skupperproject:mainfrom
AryanP123:vanflow-purge-source-affinity

AryanP123 commented Sep 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

c-kruse Sep 10, 2026

Uh oh!

c-kruse Sep 10, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

AryanP123 commented Sep 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

c-kruse Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

c-kruse Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AryanP123 commented Sep 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 10, 2026 •

edited

Loading