carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

2067 bugcore duplicate key for sync #2513

Merged
mfreeman451 merged 18 commits from refs/pull/2513/head into main 2025-12-06 20:32:37 +00:00
Conversation 2 Commits 18 Files changed 31 +1897 -618
mfreeman451 commented 2025-12-06 15:44:01 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #2068
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2068
Original created: 2025-12-06T15:44:01Z
Original updated: 2025-12-06T20:32:43Z
Original head: carverauto/serviceradar:2067-bugcore-duplicate-key-for-sync
Original base: main
Original merged: 2025-12-06T20:32:37Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

  • I have signed the DCO?
  • The build completes without errors?
  • All tests are passing when running make test?

PR Type

Bug fix, Enhancement

Description

  • Resolve duplicate key constraint violations by enforcing IP uniqueness within batches and against existing database records

  • Implement strong identity (Armis ID, Netbox ID, MAC) aware IP conflict resolution to prevent incorrect device merges during IP churn

  • Add IP collision metrics and observability to track and diagnose duplicate key issues

  • Decouple AGE graph writes from synchronous registry ingest path to prevent backpressure stalls

  • Add drift tolerance thresholds to stats aggregation for handling faker-scale loads

Diagram Walkthrough

flowchart LR
  A["Device Updates"] --> B["Batch Deduplication"]
  B --> C["IP Conflict Resolution"]
  C --> D["Strong Identity Check"]
  D --> E{Different IDs?}
  E -->|Yes| F["Clear Old IP"]
  E -->|No| G["Create Tombstone"]
  F --> H["Publish Updates"]
  G --> H
  H --> I["AGE Graph Writer"]
  I --> J{Async Mode?}
  J -->|Yes| K["Fire & Forget"]
  J -->|No| L["Wait for Result"]
  K --> M["Metrics & Logs"]
  L --> M

File Walkthrough
Relevant files
Configuration changes
1 files
main.go
Ensure synchronous graph writes for age-backfill tool       
+6/-0     
Enhancement
5 files
stats_aggregator.go
Add drift tolerance and improved discrepancy reporting     
+19/-2   
age_graph_writer.go
Implement async/sync graph write modes with configurable behavior
 +77/-28 
canonical_helpers.go
New helper functions for canonical device detection           
+38/-0   
identity_metrics.go
Add IP collision metrics and observability                             
+46/-26 
strong_identity.go
New helper functions to extract strong identity markers   
+53/-0   
Bug fix
2 files
identity_resolver_cnpg.go
Filter tombstoned devices from canonical IP resolution     
+3/-0     
registry.go
Implement IP conflict resolution respecting strong identity
 +461/-33
Tests
6 files
identity_resolver_cnpg_test.go
Add test coverage for tombstone filtering in IP resolution
 +42/-0   
integration_churn_test.go
Integration test for IP churn with different strong identities
 +208/-0 
ip_churn_test.go
Unit tests for IP churn scenarios with strong identity handling
 +168/-0 
registry_cnpg_test.go
Add tests for canonical IP mapping chain resolution           
+65/-0   
registry_dedupe_test.go
Update deduplication tests for strong identity and churn handling
 +141/-6 
registry_test.go
Update existing tests for new conflict resolution behavior
 +9/-0     
Documentation
10 files
proposal.md
Design proposal for strong identity aware IP conflict resolution
 +144/-0 
spec.md
Requirements for strong identity precedence in conflict resolution
 +81/-0   
tasks.md
Implementation tasks for strong identity fix                         
+43/-0   
proposal.md
Design proposal for duplicate key constraint fix                 
+102/-0 
spec.md
Requirements for batch-level IP uniqueness enforcement     
+46/-0   
tasks.md
Implementation tasks for duplicate key constraint fix       
+53/-0   
proposal.md
Design proposal for AGE graph backpressure decoupling       
+13/-0   
spec.md
Requirements for inventory consistency under backpressure
 +22/-0   
spec.md
Requirements for AGE graph write decoupling                           
+14/-0   
tasks.md
Implementation tasks for backpressure stabilization           
+14/-0   
Imported from GitHub pull request. Original GitHub pull request: #2068 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2068 Original created: 2025-12-06T15:44:01Z Original updated: 2025-12-06T20:32:43Z Original head: carverauto/serviceradar:2067-bugcore-duplicate-key-for-sync Original base: main Original merged: 2025-12-06T20:32:37Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Resolve duplicate key constraint violations by enforcing IP uniqueness within batches and against existing database records - Implement strong identity (Armis ID, Netbox ID, MAC) aware IP conflict resolution to prevent incorrect device merges during IP churn - Add IP collision metrics and observability to track and diagnose duplicate key issues - Decouple AGE graph writes from synchronous registry ingest path to prevent backpressure stalls - Add drift tolerance thresholds to stats aggregation for handling faker-scale loads ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Device Updates"] --> B["Batch Deduplication"] B --> C["IP Conflict Resolution"] C --> D["Strong Identity Check"] D --> E{Different IDs?} E -->|Yes| F["Clear Old IP"] E -->|No| G["Create Tombstone"] F --> H["Publish Updates"] G --> H H --> I["AGE Graph Writer"] I --> J{Async Mode?} J -->|Yes| K["Fire & Forget"] J -->|No| L["Wait for Result"] K --> M["Metrics & Logs"] L --> M ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Configuration changes</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>main.go</strong><dd><code>Ensure synchronous graph writes for age-backfill tool</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-cf52f5eded1f757087972f6142eed8fde55480f9cccd8fd8350725c1cf6f8a53">+6/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>stats_aggregator.go</strong><dd><code>Add drift tolerance and improved discrepancy reporting</code>&nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-09304e5f27f54a3a8fa2b57cc6a5485b9f7074d7554babde62b51a75753205e0">+19/-2</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>age_graph_writer.go</strong><dd><code>Implement async/sync graph write modes with configurable behavior</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195">+77/-28</a>&nbsp; </td> </tr> <tr> <td><strong>canonical_helpers.go</strong><dd><code>New helper functions for canonical device detection</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-8fab25afd79cee01607b0517d6f567a760af7edd337ab5e16df49c31b9fff452">+38/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_metrics.go</strong><dd><code>Add IP collision metrics and observability</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-fcd2f8364d180b314ea4b2676718fd233a91d0c5ef39daac2681cd9dadf885e1">+46/-26</a>&nbsp; </td> </tr> <tr> <td><strong>strong_identity.go</strong><dd><code>New helper functions to extract strong identity markers</code>&nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-c7fc8f0478a6d3844f48d21a1f41f7b97f28a2b8ef50ca38de9a75d0de27ddbe">+53/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>identity_resolver_cnpg.go</strong><dd><code>Filter tombstoned devices from canonical IP resolution</code>&nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-e758ca8be15d52379ccb94d2b35754f96d2d4d6e80a0f7d0b4a9035c9572518f">+3/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>registry.go</strong><dd><code>Implement IP conflict resolution respecting strong identity</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+461/-33</a></td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>6 files</summary><table> <tr> <td><strong>identity_resolver_cnpg_test.go</strong><dd><code>Add test coverage for tombstone filtering in IP resolution</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-9af43f5d2677c38f3515024e4268531f4125e4368719ae4725c830ef1f89eec2">+42/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>integration_churn_test.go</strong><dd><code>Integration test for IP churn with different strong identities</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-3abb511eee61b92d4a60f2bad7b709b45bc9aa6e80d2071a7155467c128cef62">+208/-0</a>&nbsp; </td> </tr> <tr> <td><strong>ip_churn_test.go</strong><dd><code>Unit tests for IP churn scenarios with strong identity handling</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-ec05d5090619fae980e3e2eff7df333066b9ec9b7238402f7f4c2c03bddbddde">+168/-0</a>&nbsp; </td> </tr> <tr> <td><strong>registry_cnpg_test.go</strong><dd><code>Add tests for canonical IP mapping chain resolution</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-175c7dc3d844415bcdce3b7654e947aec09346eee22ac4e67487b0b491d46e21">+65/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>registry_dedupe_test.go</strong><dd><code>Update deduplication tests for strong identity and churn handling</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-1f004741a27aac652f79ba81b3df1a056c2b48f863ed4f4d0a356aa6b8747f5b">+141/-6</a>&nbsp; </td> </tr> <tr> <td><strong>registry_test.go</strong><dd><code>Update existing tests for new conflict resolution behavior</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-f010972d104404be52d2a8e6e784cb56e31194f90795a69571a12696bcbdc075">+9/-0</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>10 files</summary><table> <tr> <td><strong>proposal.md</strong><dd><code>Design proposal for strong identity aware IP conflict resolution</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-bc74372977e86a0e3f64c1af49c0610fe4d80798113604c17298cba508433003">+144/-0</a>&nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Requirements for strong identity precedence in conflict resolution</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-57a85c7ff6659407d8665b61c6d9d7b09c71a213d1aca1e5fd993c027c14cfc0">+81/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Implementation tasks for strong identity fix</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-1ba2529c7b515076581e20414759e23c0d25c327e312d456620c98c2c0700f6a">+43/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Design proposal for duplicate key constraint fix</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-c47d90ba805c8ea005726fc211e6179821ffa7e92b78054a338f6d5f6b81ab31">+102/-0</a>&nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Requirements for batch-level IP uniqueness enforcement</code>&nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-4a5a9adb68cec10a1c6b76d4788fe9b774c2f0034d1aa780814fb144fc458ad0">+46/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Implementation tasks for duplicate key constraint fix</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-f34fa0a85f29520c314cde60d360176dff077bc0aae86fa99d96764c523ccac3">+53/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Design proposal for AGE graph backpressure decoupling</code>&nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-0f3d67f6fdba1de9788bcec6779a0ada1d4db4ccf2adba4f7e41d52dd49d96eb">+13/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Requirements for inventory consistency under backpressure</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-bd883777cf6a0ef16a04248a71fc2eb8381c82fa4b3659dd1f2bbf728c79c319">+22/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Requirements for AGE graph write decoupling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-3c1e1dd0105ddb7c115893ba7eb1b3127afe90a362425c4bb100603879b694c4">+14/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Implementation tasks for backpressure stabilization</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2068/files#diff-a5ee1715706759a372ec767aa222e7fcb530b75aafc34e7062864be69b146b14">+14/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-12-06 15:44:45 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2068#issuecomment-3620607479
Original created: 2025-12-06T15:44:45Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Unsafe IP sentinel

Description: Clearing an existing device's IP by assigning the literal string "0.0.0.0" may be
interpreted as a real IPv4 address rather than an unset value, potentially colliding with
a legitimate device using 0.0.0.0 or violating invariants; consider using NULL/empty and
ensuring downstream code treats it as cleared.
registry.go [434-441]

Referred Code 
// Clear IP from existing (old) update
existing.IP = "0.0.0.0" 
if existing.Metadata == nil {
	existing.Metadata = map[string]string{}
}
existing.Metadata["_ip_cleared_due_to_churn"] = "true"

// Update map to point to the new owner
Destructive test setup

Description: Integration test drops and recreates production-like tables unconditionally; if
misconfigured to point at a non-disposable database, execution could cause destructive
data loss.
integration_churn_test.go [61-76]

Referred Code 
os.Setenv("ENABLE_DB_MIGRATIONS", "false")

log := logger.NewTestLogger()
ctx := context.Background()

// Initialize DB service
database, err := db.New(ctx, cfg, log)
require.NoError(t, err)
defer database.Close()

// Drop tables to ensure clean schema (destructive, but needed for test consistency on fixture DB)
_, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS device_updates")
require.NoError(t, err)
_, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS unified_devices")
require.NoError(t, err)
Ticket Compliance
🟡
🎫 #2067
🟢 Enforce IP uniqueness within a batch of device updates to avoid intra-batch conflicts.
Resolve IP conflicts against existing database records before publishing, respecting
strong identities to avoid incorrect merges.
Add observability/metrics/logging to detect and quantify IP collisions and conflict
resolutions.
Ensure ingest path stability to avoid backpressure-induced stalls that could exacerbate
sync issues.
⚪ Prevent duplicate key violations on unique index idx_unified_devices_ip_unique_active
during sync by ensuring only one active device per IP is published.
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Limited auditing: New critical actions (IP conflict resolution, tombstone creation, IP clearing) add logs
but do not implement structured audit logs with actor/user context, making it unclear if
audit trail requirements are fully met.

Referred Code 
// Resolve IP conflicts with existing database records
// This prevents duplicate key constraint violations when a new device has
// an IP that already belongs to an existing active device
batch, dbConflicts := r.resolveIPConflictsWithDB(ctx, batch)
if dbConflicts > 0 {
	r.logger.Info().
		Int("db_ip_conflicts", dbConflicts).
		Msg("Resolved IP conflicts with existing database records")
}

// Publish directly to the device_updates stream
if err := r.db.PublishBatchDeviceUpdates(ctx, batch); err != nil {
	return fmt.Errorf("failed to publish device updates: %w", err)
}

r.applyRegistryStore(canonicalized, tombstones)

if r.graphWriter != nil {
	r.graphWriter.WriteGraph(ctx, canonicalized)
}



 ... (clipped 314 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Magic constant IP: The code uses the string literal "0.0.0.0" to clear IPs without a named constant
or documentation, which may reduce clarity and self-documentation.

Referred Code 
existing.IP = "0.0.0.0" 
if existing.Metadata == nil {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Fallback handling: When identity/device fetches fail during conflict resolution the code logs a warning and
proceeds without strong identity checks, which could lead to incorrect merges without
compensating safeguards.

Referred Code 

if len(conflictingIDs) > 0 {
	devs, err := r.db.GetUnifiedDevicesByIPsOrIDs(ctx, nil, conflictingIDs)
	if err != nil {
		r.logger.Warn().Err(err).Msg("Failed to fetch conflicting devices for identity check")
		// Proceed without strong identity check (fallback to existing behavior)
	} else {
		for _, d := range devs {
			existingDevicesMap[d.DeviceID] = d
		}
	}
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unsafe SQL drops: The integration test executes raw DROP TABLE and CREATE TABLE statements derived from an
environment-provided connection string, which could be risky if accidentally pointed at a
non-disposable database.

Referred Code 

// Drop tables to ensure clean schema (destructive, but needed for test consistency on fixture DB)
_, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS device_updates")
require.NoError(t, err)
_, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS unified_devices")
require.NoError(t, err)

// Ensure device_updates table exists (minimal schema for test matching Go code)
_, err = database.ExecuteQuery(ctx, `
	CREATE TABLE device_updates (
		device_id TEXT NOT NULL,
		ip TEXT,
		discovery_source TEXT,
		agent_id TEXT,
		poller_id TEXT,
		partition TEXT,
		observed_at TIMESTAMPTZ NOT NULL,
		hostname TEXT,
		mac TEXT,
		metadata JSONB,
		available BOOLEAN


 ... (clipped 27 lines)

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2068#issuecomment-3620607479 Original created: 2025-12-06T15:44:45Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/a3095d74f22bca7bb3efdf58ef83de715ca0eef2 --> Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Unsafe IP sentinel </strong></summary><br> <b>Description:</b> Clearing an existing device's IP by assigning the literal string "0.0.0.0" may be <br>interpreted as a real IPv4 address rather than an unset value, potentially colliding with <br>a legitimate device using 0.0.0.0 or violating invariants; consider using NULL/empty and <br>ensuring downstream code treats it as cleared.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR434-R441'>registry.go [434-441]</a></strong><br> <details open><summary>Referred Code</summary> ```go // Clear IP from existing (old) update existing.IP = "0.0.0.0" if existing.Metadata == nil { existing.Metadata = map[string]string{} } existing.Metadata["_ip_cleared_due_to_churn"] = "true" // Update map to point to the new owner ``` </details></details></td></tr> <tr><td><details><summary><strong>Destructive test setup </strong></summary><br> <b>Description:</b> Integration test drops and recreates production-like tables unconditionally; if <br>misconfigured to point at a non-disposable database, execution could cause destructive <br>data loss.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-3abb511eee61b92d4a60f2bad7b709b45bc9aa6e80d2071a7155467c128cef62R61-R76'>integration_churn_test.go [61-76]</a></strong><br> <details open><summary>Referred Code</summary> ```go os.Setenv("ENABLE_DB_MIGRATIONS", "false") log := logger.NewTestLogger() ctx := context.Background() // Initialize DB service database, err := db.New(ctx, cfg, log) require.NoError(t, err) defer database.Close() // Drop tables to ensure clean schema (destructive, but needed for test consistency on fixture DB) _, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS device_updates") require.NoError(t, err) _, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS unified_devices") require.NoError(t, err) ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2067>#2067</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Enforce IP uniqueness within a batch of device updates to avoid intra-batch conflicts.</td></tr> <tr><td>Resolve IP conflicts against existing database records before publishing, respecting <br>strong identities to avoid incorrect merges.</td></tr> <tr><td>Add observability/metrics/logging to detect and quantify IP collisions and conflict <br>resolutions.</td></tr> <tr><td>Ensure ingest path stability to avoid backpressure-induced stalls that could exacerbate <br>sync issues.</td></tr> <tr><td rowspan=1>⚪</td> <td>Prevent duplicate key violations on unique index idx_unified_devices_ip_unique_active <br>during sync by ensuring only one active device per IP is published.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=4>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR347-R681'><strong>Limited auditing</strong></a>: New critical actions (IP conflict resolution, tombstone creation, IP clearing) add logs <br>but do not implement structured audit logs with actor/user context, making it unclear if <br>audit trail requirements are fully met.<br> <details open><summary>Referred Code</summary> ```go // Resolve IP conflicts with existing database records // This prevents duplicate key constraint violations when a new device has // an IP that already belongs to an existing active device batch, dbConflicts := r.resolveIPConflictsWithDB(ctx, batch) if dbConflicts > 0 { r.logger.Info(). Int("db_ip_conflicts", dbConflicts). Msg("Resolved IP conflicts with existing database records") } // Publish directly to the device_updates stream if err := r.db.PublishBatchDeviceUpdates(ctx, batch); err != nil { return fmt.Errorf("failed to publish device updates: %w", err) } r.applyRegistryStore(canonicalized, tombstones) if r.graphWriter != nil { r.graphWriter.WriteGraph(ctx, canonicalized) } ... (clipped 314 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR435-R436'><strong>Magic constant IP</strong></a>: The code uses the string literal &quot;0.0.0.0&quot; to clear IPs without a named constant <br>or documentation, which may reduce clarity and self-documentation.<br> <details open><summary>Referred Code</summary> ```go existing.IP = "0.0.0.0" if existing.Metadata == nil { ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR548-R559'><strong>Fallback handling</strong></a>: When identity/device fetches fail during conflict resolution the code logs a warning and <br>proceeds without strong identity checks, which could lead to incorrect merges without <br>compensating safeguards.<br> <details open><summary>Referred Code</summary> ```go if len(conflictingIDs) > 0 { devs, err := r.db.GetUnifiedDevicesByIPsOrIDs(ctx, nil, conflictingIDs) if err != nil { r.logger.Warn().Err(err).Msg("Failed to fetch conflicting devices for identity check") // Proceed without strong identity check (fallback to existing behavior) } else { for _, d := range devs { existingDevicesMap[d.DeviceID] = d } } } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2068/files#diff-3abb511eee61b92d4a60f2bad7b709b45bc9aa6e80d2071a7155467c128cef62R70-R117'><strong>Unsafe SQL drops</strong></a>: The integration test executes raw DROP TABLE and CREATE TABLE statements derived from an <br>environment-provided connection string, which could be risky if accidentally pointed at a <br>non-disposable database.<br> <details open><summary>Referred Code</summary> ```go // Drop tables to ensure clean schema (destructive, but needed for test consistency on fixture DB) _, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS device_updates") require.NoError(t, err) _, err = database.ExecuteQuery(ctx, "DROP TABLE IF EXISTS unified_devices") require.NoError(t, err) // Ensure device_updates table exists (minimal schema for test matching Go code) _, err = database.ExecuteQuery(ctx, ` CREATE TABLE device_updates ( device_id TEXT NOT NULL, ip TEXT, discovery_source TEXT, agent_id TEXT, poller_id TEXT, partition TEXT, observed_at TIMESTAMPTZ NOT NULL, hostname TEXT, mac TEXT, metadata JSONB, available BOOLEAN ... (clipped 27 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>
qodo-code-review[bot] commented 2025-12-06 15:46:03 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2068#issuecomment-3620608545
Original created: 2025-12-06T15:46:03Z

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect map lookup logic

Correct the logic in resolveIPsToCanonical to use the deviceID from the out map
as the key for looking up the canonical target in resolvedChains, instead of
incorrectly using the ip.

pkg/registry/registry.go [2042-2056]

     func (r *DeviceRegistry) resolveIPsToCanonical(ctx context.Context, ips []string, out map[string]string) error {
 ...
     	// Finally, resolve any merged devices in the result set to their canonical targets
     	if len(out) > 0 {
     		resolvedChains := r.resolveCanonicalIPMappings(ctx, out)
     		// Update out with resolved canonical IDs, removing any that couldn't be resolved (e.g. deleted)
-    		for ip := range out {
-    			if canonical, ok := resolvedChains[ip]; ok && canonical != "" {
+    		for ip, deviceID := range out {
+    			if canonical, ok := resolvedChains[deviceID]; ok && canonical != "" {
     				out[ip] = canonical
     			} else {
+    				// If the original ID couldn't be resolved to a canonical one, remove it.
     				delete(out, ip)
     			}
     		}
     	}
     
     	return resolveErr
     }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: This suggestion fixes a critical bug in resolveIPsToCanonical where an incorrect map key (ip instead of deviceID) is used, which would cause the tombstone chain resolution to fail and return incorrect data.

High
✅ Avoid changing device availability status
Suggestion Impact:The commit refactored the code and introduced createIPClearUpdate, but it still sets IsAvailable: false. However, the change centralizes this behavior; the relevant code path still controls IsAvailable for churn. The suggestion influenced the touched area, but was not adopted—the commit retains IsAvailable: false.

code diff:

+// createIPClearUpdate creates an update that clears the IP from a device due to IP churn
+func createIPClearUpdate(deviceID string) *models.DeviceUpdate {
+	return &models.DeviceUpdate{
+		DeviceID:    deviceID,
+		IP:          "0.0.0.0",
+		Timestamp:   time.Now(),
+		Source:      models.DiscoverySourceServiceRadar,
+		IsAvailable: false,
+		Metadata: map[string]string{
+			"_ip_cleared_due_to_churn": "true",
+		},
+	}
+}

In resolveIPConflictsWithDB, remove the line that sets IsAvailable: false when
creating a clearUpdate for IP churn to avoid incorrectly marking a device as
unavailable.

pkg/registry/registry.go [598-618]

     			if identityMismatch {
     				// Different strong identities sharing same IP -> churn.
     				// Do NOT merge. The new update takes the IP.
     				// We must clear the IP from the existing DB record to avoid constraint violation.
     				
     				clearUpdate := &models.DeviceUpdate{
-    					DeviceID:    existingDeviceID,
-    					IP:          "0.0.0.0", // Clear IP to resolve conflict
-    					Timestamp:   time.Now(),
-    					Source:      models.DiscoverySourceServiceRadar, // System update
-    					IsAvailable: false, // Don't change availability? Or maybe we should?
+    					DeviceID:  existingDeviceID,
+    					IP:        "0.0.0.0", // Clear IP to resolve conflict
+    					Timestamp: time.Now(),
+    					Source:    models.DiscoverySourceServiceRadar, // System update
     					Metadata: map[string]string{
     						"_ip_cleared_due_to_churn": "true",
     					},
     				}
     				// Append clear update FIRST
     				result = append(result, clearUpdate)
     				// Append new update (it will take the IP)
     				result = append(result, update)
     				continue
     			}

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that setting IsAvailable: false during an IP churn event is an incorrect assumption, as the device may still be online, and addresses the uncertainty noted in the code's comment.

Low
General
✅ Simplify tombstone creation logic
Suggestion Impact:The commit introduced a createTombstoneUpdate helper that initially sets Metadata to only {"_merged_into": targetDeviceID}, but it still copies over other metadata from the update afterward. While not fully applying the suggestion, the refactor touched the exact area and centralized tombstone creation, making it easier to adjust. Thus, the suggestion partially influenced the change.

code diff:

+// createTombstoneUpdate creates a tombstone update pointing to the target device
+func createTombstoneUpdate(update *models.DeviceUpdate, targetDeviceID string) *models.DeviceUpdate {
+	tombstone := &models.DeviceUpdate{
+		AgentID:     update.AgentID,
+		PollerID:    update.PollerID,
+		Partition:   update.Partition,
+		DeviceID:    update.DeviceID,
+		Source:      update.Source,
+		IP:          update.IP,
+		Timestamp:   update.Timestamp,
+		IsAvailable: update.IsAvailable,
+		Metadata:    map[string]string{"_merged_into": targetDeviceID},
+	}
+	if update.Metadata != nil {
+		for k, v := range update.Metadata {
+			if k != "_merged_into" {
+				tombstone.Metadata[k] = v
+			}
+		}
+	}
+	return tombstone
+}

Simplify the tombstone creation in resolveIPConflictsWithDB by removing the
redundant copy of all metadata, leaving only the essential _merged_into field.

pkg/registry/registry.go [629-649]

     			// Create tombstone pointing to the existing device
     			tombstone := &models.DeviceUpdate{
     				AgentID:     update.AgentID,
     				PollerID:    update.PollerID,
     				Partition:   update.Partition,
     				DeviceID:    update.DeviceID,
     				Source:      update.Source,
     				IP:          update.IP,
     				Timestamp:   update.Timestamp,
     				IsAvailable: update.IsAvailable,
     				Metadata:    map[string]string{"_merged_into": existingDeviceID},
     			}
-    			// Copy other metadata to preserve identity info
-    			if update.Metadata != nil {
-    				for k, v := range update.Metadata {
-    					if k != "_merged_into" {
-    						tombstone.Metadata[k] = v
-    					}
-    				}
-    			}
     			tombstones = append(tombstones, tombstone)

[Suggestion processed]

Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that copying all metadata to a tombstone is redundant, as it is already being merged into the canonical device, and simplifying it improves code clarity and reduces data duplication.

Low
  • Update
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2068#issuecomment-3620608545 Original created: 2025-12-06T15:46:03Z --- ## PR Code Suggestions ✨ <!-- a3095d7 --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Fix incorrect map lookup logic<!-- not_implemented --></summary> ___ **Correct the logic in <code>resolveIPsToCanonical</code> to use the <code>deviceID</code> from the <code>out</code> map <br>as the key for looking up the canonical target in <code>resolvedChains</code>, instead of <br>incorrectly using the <code>ip</code>.** [pkg/registry/registry.go [2042-2056]](https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR2042-R2056) ```diff func (r *DeviceRegistry) resolveIPsToCanonical(ctx context.Context, ips []string, out map[string]string) error { ... // Finally, resolve any merged devices in the result set to their canonical targets if len(out) > 0 { resolvedChains := r.resolveCanonicalIPMappings(ctx, out) // Update out with resolved canonical IDs, removing any that couldn't be resolved (e.g. deleted) - for ip := range out { - if canonical, ok := resolvedChains[ip]; ok && canonical != "" { + for ip, deviceID := range out { + if canonical, ok := resolvedChains[deviceID]; ok && canonical != "" { out[ip] = canonical } else { + // If the original ID couldn't be resolved to a canonical one, remove it. delete(out, ip) } } } return resolveErr } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion fixes a critical bug in `resolveIPsToCanonical` where an incorrect map key (`ip` instead of `deviceID`) is used, which would cause the tombstone chain resolution to fail and return incorrect data. </details></details></td><td align=center>High </td></tr><tr><td> <details><summary>✅ <s>Avoid changing device availability status</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit refactored the code and introduced createIPClearUpdate, but it still sets IsAvailable: false. However, the change centralizes this behavior; the relevant code path still controls IsAvailable for churn. The suggestion influenced the touched area, but was not adopted—the commit retains IsAvailable: false. code diff: ```diff +// createIPClearUpdate creates an update that clears the IP from a device due to IP churn +func createIPClearUpdate(deviceID string) *models.DeviceUpdate { + return &models.DeviceUpdate{ + DeviceID: deviceID, + IP: "0.0.0.0", + Timestamp: time.Now(), + Source: models.DiscoverySourceServiceRadar, + IsAvailable: false, + Metadata: map[string]string{ + "_ip_cleared_due_to_churn": "true", + }, + } +} ``` </details> ___ **In <code>resolveIPConflictsWithDB</code>, remove the line that sets <code>IsAvailable: false</code> when <br>creating a <code>clearUpdate</code> for IP churn to avoid incorrectly marking a device as <br>unavailable.** [pkg/registry/registry.go [598-618]](https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR598-R618) ```diff if identityMismatch { // Different strong identities sharing same IP -> churn. // Do NOT merge. The new update takes the IP. // We must clear the IP from the existing DB record to avoid constraint violation. clearUpdate := &models.DeviceUpdate{ - DeviceID: existingDeviceID, - IP: "0.0.0.0", // Clear IP to resolve conflict - Timestamp: time.Now(), - Source: models.DiscoverySourceServiceRadar, // System update - IsAvailable: false, // Don't change availability? Or maybe we should? + DeviceID: existingDeviceID, + IP: "0.0.0.0", // Clear IP to resolve conflict + Timestamp: time.Now(), + Source: models.DiscoverySourceServiceRadar, // System update Metadata: map[string]string{ "_ip_cleared_due_to_churn": "true", }, } // Append clear update FIRST result = append(result, clearUpdate) // Append new update (it will take the IP) result = append(result, update) continue } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that setting `IsAvailable: false` during an IP churn event is an incorrect assumption, as the device may still be online, and addresses the uncertainty noted in the code's comment. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>✅ <s>Simplify tombstone creation logic</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit introduced a createTombstoneUpdate helper that initially sets Metadata to only {"_merged_into": targetDeviceID}, but it still copies over other metadata from the update afterward. While not fully applying the suggestion, the refactor touched the exact area and centralized tombstone creation, making it easier to adjust. Thus, the suggestion partially influenced the change. code diff: ```diff +// createTombstoneUpdate creates a tombstone update pointing to the target device +func createTombstoneUpdate(update *models.DeviceUpdate, targetDeviceID string) *models.DeviceUpdate { + tombstone := &models.DeviceUpdate{ + AgentID: update.AgentID, + PollerID: update.PollerID, + Partition: update.Partition, + DeviceID: update.DeviceID, + Source: update.Source, + IP: update.IP, + Timestamp: update.Timestamp, + IsAvailable: update.IsAvailable, + Metadata: map[string]string{"_merged_into": targetDeviceID}, + } + if update.Metadata != nil { + for k, v := range update.Metadata { + if k != "_merged_into" { + tombstone.Metadata[k] = v + } + } + } + return tombstone +} ``` </details> ___ **Simplify the tombstone creation in <code>resolveIPConflictsWithDB</code> by removing the <br>redundant copy of all metadata, leaving only the essential <code>_merged_into</code> field.** [pkg/registry/registry.go [629-649]](https://github.com/carverauto/serviceradar/pull/2068/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR629-R649) ```diff // Create tombstone pointing to the existing device tombstone := &models.DeviceUpdate{ AgentID: update.AgentID, PollerID: update.PollerID, Partition: update.Partition, DeviceID: update.DeviceID, Source: update.Source, IP: update.IP, Timestamp: update.Timestamp, IsAvailable: update.IsAvailable, Metadata: map[string]string{"_merged_into": existingDeviceID}, } - // Copy other metadata to preserve identity info - if update.Metadata != nil { - for k, v := range update.Metadata { - if k != "_merged_into" { - tombstone.Metadata[k] = v - } - } - } tombstones = append(tombstones, tombstone) ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly points out that copying all metadata to a tombstone is redundant, as it is already being merged into the canonical device, and simplifying it improves code clarity and reduces data duplication. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2513
No description provided.