carverauto/serviceradar

Fork 0

fix: agent updated to respect sweep interval #2834

Merged

mfreeman451 merged 8 commits from refs/pull/2834/head into staging

2026-02-02 23:43:22 +00:00

mfreeman451 commented

2026-02-02 18:38:02 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2673
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2673
Original created: 2026-02-02T18:38:02Z
Original updated: 2026-02-02T23:43:30Z
Original head: carverauto/serviceradar:2646-bug-scanningsweep-interval-is-being-ignored
Original base: staging
Original merged: 2026-02-02T23:43:22Z 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

Fixed sweep interval configuration being ignored by agent
Refactored gateway sweep config parsing to merge multiple groups
Added ticker reset mechanism to respect interval changes at runtime
Implemented deduplication logic for ports, modes, and networks
Added comprehensive specification and task documentation

Diagram Walkthrough

flowchart LR
  A["Multiple Sweep Groups"] -->|"Merge with dedup"| B["Single SweepConfig"]
  B -->|"Parse interval"| C["Sweep Service"]
  C -->|"Update config"| D["NetworkSweeper"]
  D -->|"Detect interval change"| E["Reset Ticker"]
  E -->|"Apply new interval"| F["Respect configured sweep cycle"]

File Walkthrough

Relevant files

Bug fix

sweep_config_gateway.go

Merge multiple sweep groups with deduplication

pkg/agent/sweep_config_gateway.go

Changed from using only first group to merging all sweep groups
Added deduplication maps for ports, modes, and networks
Implemented merge logic that combines targets while avoiding
duplicates
Uses shortest interval when multiple groups have different intervals
Uses highest concurrency and timeout values across groups

+60/-25

sweeper.go

Implement ticker reset on interval changes

pkg/sweeper/sweeper.go

Added tickerReset channel to signal interval changes
Implemented case handler in Start loop to detect and reset ticker
Modified UpdateConfig to detect interval changes and signal reset
Logs interval changes with new duration for debugging

+17/-0

Enhancement

sweep_service.go

Add logging for sweep config updates

pkg/agent/sweep_service.go

Added logging to display the new interval being applied
Logs sweep group ID for better debugging and traceability

+5/-0

Documentation

proposal.md `Add proposal for sweep interval fix` openspec/changes/fix-sweep-interval-config/proposal.md Documents the issue where configured sweep intervals are ignored Specifies requirements for interval propagation from Profile to Agent Defines behavior for conflicting intervals across multiple sweep jobs Identifies affected specs and code components	+13/-0
spec.md `Define sweep job configuration specifications` openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md Defines requirement for sweep job compiled config output format Specifies custom interval propagation scenario with 6-hour example Documents device query evaluation at compile time Defines merge behavior for multiple sweep jobs with interval selection Specifies agent parsing of device targets with TCP ports Documents port preservation for device-targeted sweeps Defines TCP mode port requirement validation	+61/-0
tasks.md `Add implementation tasks for sweep interval fix` openspec/changes/fix-sweep-interval-config/tasks.md Lists core service updates needed for interval field population Specifies agent verification tasks for reading and respecting intervals Includes validation tasks for end-to-end interval configuration	+12/-0

Imported from GitHub pull request. Original GitHub pull request: #2673 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2673 Original created: 2026-02-02T18:38:02Z Original updated: 2026-02-02T23:43:30Z Original head: carverauto/serviceradar:2646-bug-scanningsweep-interval-is-being-ignored Original base: staging Original merged: 2026-02-02T23:43:22Z 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** - Fixed sweep interval configuration being ignored by agent - Refactored gateway sweep config parsing to merge multiple groups - Added ticker reset mechanism to respect interval changes at runtime - Implemented deduplication logic for ports, modes, and networks - Added comprehensive specification and task documentation ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Multiple Sweep Groups"] -->|"Merge with dedup"| B["Single SweepConfig"] B -->|"Parse interval"| C["Sweep Service"] C -->|"Update config"| D["NetworkSweeper"] D -->|"Detect interval change"| E["Reset Ticker"] E -->|"Apply new interval"| F["Respect configured sweep cycle"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>sweep_config_gateway.go</strong><dd><code>Merge multiple sweep groups with deduplication</code>                      </dd></summary> <hr> pkg/agent/sweep_config_gateway.go <ul><li>Changed from using only first group to merging all sweep groups<br> <li> Added deduplication maps for ports, modes, and networks<br> <li> Implemented merge logic that combines targets while avoiding <br>duplicates<br> <li> Uses shortest interval when multiple groups have different intervals<br> <li> Uses highest concurrency and timeout values across groups</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-0590118d93b1be7997233109d892bea154ed59038b8069290db99dc03080d307">+60/-25</a>  </td> </tr> <tr> <td> <details> <summary><strong>sweeper.go</strong><dd><code>Implement ticker reset on interval changes</code>                              </dd></summary> <hr> pkg/sweeper/sweeper.go <ul><li>Added <code>tickerReset</code> channel to signal interval changes<br> <li> Implemented case handler in Start loop to detect and reset ticker<br> <li> Modified UpdateConfig to detect interval changes and signal reset<br> <li> Logs interval changes with new duration for debugging</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47a">+17/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>sweep_service.go</strong><dd><code>Add logging for sweep config updates</code>                                          </dd></summary> <hr> pkg/agent/sweep_service.go <ul><li>Added logging to display the new interval being applied<br> <li> Logs sweep group ID for better debugging and traceability</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-988114dc491154ea9a249227411b24e463212831cd81b43e0844862ce7a41343">+5/-0</a>      </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Add proposal for sweep interval fix</code>                                            </dd></summary> <hr> openspec/changes/fix-sweep-interval-config/proposal.md <ul><li>Documents the issue where configured sweep intervals are ignored<br> <li> Specifies requirements for interval propagation from Profile to Agent<br> <li> Defines behavior for conflicting intervals across multiple sweep jobs<br> <li> Identifies affected specs and code components</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-f27ffaf5ee9afa6dd25505556d0db3ccac076b59730e2ff03d02d68130efb20c">+13/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>Define sweep job configuration specifications</code>                        </dd></summary> <hr> openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md <ul><li>Defines requirement for sweep job compiled config output format<br> <li> Specifies custom interval propagation scenario with 6-hour example<br> <li> Documents device query evaluation at compile time<br> <li> Defines merge behavior for multiple sweep jobs with interval selection<br> <li> Specifies agent parsing of device targets with TCP ports<br> <li> Documents port preservation for device-targeted sweeps<br> <li> Defines TCP mode port requirement validation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-735b9b6d278ca31f4490b6370265c2be02454766daa41b68beb1591ed7bc42c4">+61/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Add implementation tasks for sweep interval fix</code>                    </dd></summary> <hr> openspec/changes/fix-sweep-interval-config/tasks.md <ul><li>Lists core service updates needed for interval field population<br> <li> Specifies agent verification tasks for reading and respecting <br>intervals<br> <li> Includes validation tasks for end-to-end interval configuration</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-89a9bebff3ea9752ca25b5cb5d00c25ee9a8abcccc5392642f8fd7dc4c1782e9">+12/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-02-02 18:38:44 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2673#issuecomment-3836983779
Original created: 2026-02-02T18:38:44Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Ticker reset DoS

Description: ticker.Reset(newInterval) is invoked without validating newInterval > 0, so a
malformed/zero/negative interval from config could panic or create a tight loop leading to
service disruption (remote config-driven DoS). sweeper.go [426-433]

Referred Code

case <-s.tickerReset:
	s.mu.RLock()
	newInterval := s.config.Interval
	s.mu.RUnlock()
	s.logger.Info().Dur("newInterval", newInterval).Msg("Resetting sweep ticker due to interval change")
	ticker.Reset(newInterval)
case t := <-ticker.C:
	s.logger.Debug().Time("tickTime", t).Msg("Ticker fired, starting periodic sweep")

Ticket Compliance

🟡

🎫 #2646

🟢	When multiple sweep groups/jobs are assigned to the same agent, configs must be merged into one, with deduplication of combined targets/settings, and the shortest interval used when intervals differ.
	Agent should log the effective sweep interval being applied/used for easier debugging.
	Ensure sweep scheduler applies interval changes at runtime (i.e., configuration updates should adjust the running ticker rather than requiring a restart).
🔴	Add unit tests verifying interval propagation in the config compiler.
⚪	Agent must respect the configured sweep interval (e.g., 6 hours) instead of scanning every 5 minutes.
	Sweep interval must propagate correctly from sweep profile/job to the agent-consumable config (`sweep.json` payload) and must not default to 5 minutes when a valid interval is provided.
	Verify end-to-end that a 6-hour interval results in a 6-hour scan cycle (not 5 minutes).

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Comprehensive Audit Trails

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

Status: Passed

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: Passed

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

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: Robust Error Handling and Edge Case Management

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

Status:
Interval not validated: The new ticker reset path calls ticker.Reset(newInterval) without validating newInterval
> 0, which can cause runtime panics or undefined scheduling when interval is
zero/invalid.

Referred Code

case <-s.tickerReset:
	s.mu.RLock()
	newInterval := s.config.Interval
	s.mu.RUnlock()
	s.logger.Info().Dur("newInterval", newInterval).Msg("Resetting sweep ticker due to interval change")
	ticker.Reset(newInterval)
case t := <-ticker.C:

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:
Unvalidated external values: The merged gateway sweep config consumes external inputs (e.g., group.Ports,
group.Schedule) with minimal validation, so it requires verification that upstream schema
validation prevents invalid ports/intervals from reaching runtime scheduling.

Referred Code

// Merge all groups into a single configuration
config := &SweepConfig{
	ConfigHash: sweep.ConfigHash,
}

// To avoid duplicate ports and modes
portMap := make(map[int]struct{})
modeMap := make(map[models.SweepMode]struct{})
networkMap := make(map[string]struct{})

for i, group := range sweep.Groups {
	// Use the first group's ID as the primary sweep group ID for result attribution
	if i == 0 {
		sweepGroupID := group.SweepGroupID
		if sweepGroupID == "" {
			sweepGroupID = group.ID
		}
		config.SweepGroupID = sweepGroupID
	}

	// Merge networks


 ... (clipped 44 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/2673#issuecomment-3836983779 Original created: 2026-02-02T18:38:44Z --- ## PR Compliance Guide 🔍  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=1>⚪</td> <td><details><summary><strong>Ticker reset DoS </strong></summary><br> <b>Description:</b> <code>ticker.Reset(newInterval)</code> is invoked without validating <code>newInterval > 0</code>, so a <br>malformed/zero/negative interval from config could panic or create a tight loop leading to <br>service disruption (remote config-driven DoS). <strong><a href='https://github.com/carverauto/serviceradar/pull/2673/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47aR426-R433'>sweeper.go [426-433]</a></strong><br> <details open><summary>Referred Code</summary> ```go case <-s.tickerReset: s.mu.RLock() newInterval := s.config.Interval s.mu.RUnlock() s.logger.Info().Dur("newInterval", newInterval).Msg("Resetting sweep ticker due to interval change") ticker.Reset(newInterval) case t := <-ticker.C: s.logger.Debug().Time("tickTime", t).Msg("Ticker fired, starting periodic sweep") ``` </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/2646>#2646</a></summary> <table width='100%'><tbody> <tr><td rowspan=3>🟢</td> <td>When multiple sweep groups/jobs are assigned to the same agent, configs must be merged <br>into one, with deduplication of combined targets/settings, and the shortest interval used <br>when intervals differ.<br></td></tr> <tr><td>Agent should log the effective sweep interval being applied/used for easier debugging. </td></tr> <tr><td>Ensure sweep scheduler applies interval changes at runtime (i.e., configuration updates <br>should adjust the running ticker rather than requiring a restart).<br></td></tr> <tr><td rowspan=1>🔴</td> <td>Add unit tests verifying interval propagation in the config compiler. </td></tr> <tr><td rowspan=3>⚪</td> <td>Agent must respect the configured sweep interval (e.g., 6 hours) instead of scanning every <br>5 minutes.<br></td></tr> <tr><td>Sweep interval must propagate correctly from sweep profile/job to the agent-consumable <br>config (<code>sweep.json</code> payload) and must not default to 5 minutes when a valid interval is <br>provided.<br></td></tr> <tr><td>Verify end-to-end that a 6-hour interval results in a 6-hour scan cycle (not 5 minutes). </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=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:** 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: 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:** 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 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=1>🔴</td> <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/2673/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47aR426-R432'><strong>Interval not validated</strong></a>: The new ticker reset path calls <code>ticker.Reset(newInterval)</code> without validating <code>newInterval </code><br><code>> 0</code>, which can cause runtime panics or undefined scheduling when interval is <br>zero/invalid.<br> <details open><summary>Referred Code</summary> ```go case <-s.tickerReset: s.mu.RLock() newInterval := s.config.Interval s.mu.RUnlock() s.logger.Info().Dur("newInterval", newInterval).Msg("Resetting sweep ticker due to interval change") ticker.Reset(newInterval) case t := <-ticker.C: ``` </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 rowspan=1>⚪</td> <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/2673/files#diff-0590118d93b1be7997233109d892bea154ed59038b8069290db99dc03080d307R93-R157'><strong>Unvalidated external values</strong></a>: The merged gateway sweep config consumes external inputs (e.g., <code>group.Ports</code>, <br><code>group.Schedule</code>) with minimal validation, so it requires verification that upstream schema <br>validation prevents invalid ports/intervals from reaching runtime scheduling.<br> <details open><summary>Referred Code</summary> ```go // Merge all groups into a single configuration config := &SweepConfig{ ConfigHash: sweep.ConfigHash, } // To avoid duplicate ports and modes portMap := make(map[int]struct{}) modeMap := make(map[models.SweepMode]struct{}) networkMap := make(map[string]struct{}) for i, group := range sweep.Groups { // Use the first group's ID as the primary sweep group ID for result attribution if i == 0 { sweepGroupID := group.SweepGroupID if sweepGroupID == "" { sweepGroupID = group.ID } config.SweepGroupID = sweepGroupID } // Merge networks ... (clipped 44 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  </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

2026-02-02 18:40:06 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2673#issuecomment-3836989177
Original created: 2026-02-02T18:40:06Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ ~~Re-evaluate the sweep group merging strategy~~ Suggestion Impact: The commit removed the global “merge all groups” logic (including choosing the shortest interval) by switching to returning a per-group SweepGroupsConfig where each group retains its own Interval. The spec text that required using the shortest interval as the global interval was also removed. code diff: -func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (SweepConfig, error) { +func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (SweepGroupsConfig, error) { if len(configJSON) == 0 { return nil, nil } @@ -87,74 +87,44 @@ if len(sweep.Groups) == 0 { log.Info().Msg("Gateway sweep config contained no groups; clearing sweep targets") - return &SweepConfig{ConfigHash: sweep.ConfigHash}, nil - } - - // Merge all groups into a single configuration - config := &SweepConfig{ + return &SweepGroupsConfig{ConfigHash: sweep.ConfigHash}, nil + } + + config := &SweepGroupsConfig{ ConfigHash: sweep.ConfigHash, - } - - // To avoid duplicate ports and modes - portMap := make(map[int]struct{}) - modeMap := make(map[models.SweepMode]struct{}) - networkMap := make(map[string]struct{}) - - for i, group := range sweep.Groups { - // Use the first group's ID as the primary sweep group ID for result attribution - if i == 0 { - sweepGroupID := group.SweepGroupID - if sweepGroupID == "" { - sweepGroupID = group.ID - } - config.SweepGroupID = sweepGroupID - } - - // Merge networks - for _, target := range normalizeTargets(group.Targets, log) { - if _, exists := networkMap[target]; !exists { - config.Networks = append(config.Networks, target) - networkMap[target] = struct{}{} - } - } - - // Merge ports - for _, port := range group.Ports { - if _, exists := portMap[port]; !exists { - config.Ports = append(config.Ports, port) - portMap[port] = struct{}{} - } - } - - // Merge sweep modes - for _, mode := range parseSweepModes(group.Modes, log) { - if _, exists := modeMap[mode]; !exists { - config.SweepModes = append(config.SweepModes, mode) - modeMap[mode] = struct{}{} - } - } - - // Merge device targets - config.DeviceTargets = append(config.DeviceTargets, convertDeviceTargets(group.DeviceTargets, log)...) - - // Merge settings - if group.Settings.Concurrency > config.Concurrency { - config.Concurrency = group.Settings.Concurrency + Groups: make([]SweepGroupConfig, 0, len(sweep.Groups)), + } + + for _, group := range sweep.Groups { + sweepGroupID := group.SweepGroupID + if sweepGroupID == "" { + sweepGroupID = group.ID + } + + groupConfig := SweepGroupConfig{ + ID: group.ID, + SweepGroupID: sweepGroupID, + Networks: normalizeTargets(group.Targets, log), + Ports: group.Ports, + SweepModes: parseSweepModes(group.Modes, log), + DeviceTargets: convertDeviceTargets(group.DeviceTargets, log), + Concurrency: group.Settings.Concurrency, + ScheduleType: strings.ToLower(strings.TrimSpace(group.Schedule.Type)), + CronExpression: strings.TrimSpace(group.Schedule.Cron), + ConfigHash: sweep.ConfigHash, } if interval, ok := parseScheduleInterval(group.Schedule, log); ok { - if config.Interval == 0 \|\| interval < config.Interval { - config.Interval = interval - } + groupConfig.Interval = interval } if timeout, err := parseDurationValue(group.Settings.Timeout); err == nil { - if timeout > config.Timeout { - config.Timeout = timeout - } + groupConfig.Timeout = timeout } else if group.Settings.Timeout != "" { log.Warn().Err(err).Str("timeout", group.Settings.Timeout).Msg("Invalid sweep timeout") } + + config.Groups = append(config.Groups, groupConfig) } return config, nil # File: openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md @@ -1,62 +1 @@ -## MODIFIED Requirements -### Requirement: Sweep Job Compiled Config Output -The system SHALL compile sweep job configurations into the agent-consumable JSON format matching the existing `sweep.json` schema. - -#### Scenario: Compile sweep config for agent -- GIVEN a sweep job assigned to an agent -- WHEN the agent polls for config -- THEN the compiled config SHALL include: - - `networks`: CIDR ranges from device query evaluation - - `ports`: from selected profile or job override - - `sweep_modes`: ["tcp", "icmp", "tcp_connect"] based on profile - - `interval`: scan interval duration - - `concurrency`: parallel scan threads - - `timeout`: per-target timeout - - `icmp_count`, `high_perf_icmp`, `icmp_rate_limit`: ICMP settings - - `device_targets`: per-device configurations with metadata - -#### Scenario: Custom interval propagation -- GIVEN a sweep job with a configured interval of "6h" (21600s) provided via its profile -- WHEN the config is compiled -- THEN the `interval` field in the JSON SHALL reflect the 6 hour duration -- AND SHALL NOT default to 5 minutes - -#### Scenario: Device query evaluation at compile time -- GIVEN a sweep job with device query "tags.env = 'prod'" -- WHEN the config is compiled -- THEN the query SHALL be evaluated against current device inventory -- AND matching device IPs SHALL populate `networks` as /32 CIDRs -- AND device metadata SHALL populate `device_targets` entries - -#### Scenario: Merge multiple sweep jobs -- GIVEN multiple sweep jobs assigned to the same agent -- WHEN the agent polls for config -- THEN the configs SHALL be merged into a single sweep config -- AND networks and device_targets SHALL be combined -- AND the most restrictive settings SHALL be used for shared targets -- AND the shortest interval SHALL be used as the global interval if intervals differ - The current strategy of merging multiple sweep groups by using the shortest interval can cause unintended high-frequency scanning. It is suggested to re-evaluate this merging logic to prevent unexpected network load. Examples: pkg/agent/sweep_config_gateway.go [145-149] `if interval, ok := parseScheduleInterval(group.Schedule, log); ok { if config.Interval == 0 \|\| interval < config.Interval { config.Interval = interval } }` openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md [38] `- AND the shortest interval SHALL be used as the global interval if intervals differ` Solution Walkthrough: Before: `func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (SweepConfig, error) { // ... if len(sweep.Groups) == 0 { return &SweepConfig{}, nil } // Only the first group was used, others were ignored. group := sweep.Groups[0] if len(sweep.Groups) > 1 { log.Warn("using first group only") } config := &SweepConfig{ Networks: group.Targets, Ports: group.Ports, // ... other settings from the first group } // ... return config, nil }` After: func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (SweepConfig, error) { // ... config := &SweepConfig{} for _, group := range sweep.Groups { // Networks, ports, modes are merged config.Networks = append(config.Networks, group.Targets...) config.Ports = append(config.Ports, group.Ports...) // ... // The shortest interval across all groups is chosen. if interval, ok := parseScheduleInterval(group.Schedule, log); ok { if config.Interval == 0 \|\| interval < config.Interval { config.Interval = interval } } } // ... return config, nil } Suggestion importance[1-10]: 9 __ Why: This suggestion raises a critical design concern about the sweep group merging strategy, as using the shortest interval could cause unintended high network load, which is a significant potential issue.	High
Possible issue	Deduplicate device targets to prevent redundant scans Deduplicate `DeviceTargets` by their network address and merge their `SweepModes` to prevent redundant scans and ensure all scan modes are included. pkg/agent/sweep_config_gateway.go [137-138] // Merge device targets - config.DeviceTargets = append(config.DeviceTargets, convertDeviceTargets(group.DeviceTargets, log)...) + if i == 0 { + config.DeviceTargets = convertDeviceTargets(group.DeviceTargets, log) + } else { + // Deduplicate device targets by network, merging sweep modes + existingTargets := make(map[string]models.DeviceTarget, len(config.DeviceTargets)) + for i := range config.DeviceTargets { + existingTargets[config.DeviceTargets[i].Network] = &config.DeviceTargets[i] + } + for _, newTarget := range convertDeviceTargets(group.DeviceTargets, log) { + if existing, found := existingTargets[newTarget.Network]; found { + // Merge sweep modes + modeSet := make(map[models.SweepMode]struct{}) + for _, mode := range existing.SweepModes { + modeSet[mode] = struct{}{} + } + for _, mode := range newTarget.SweepModes { + modeSet[mode] = struct{}{} + } + + mergedModes := make([]models.SweepMode, 0, len(modeSet)) + for mode := range modeSet { + mergedModes = append(mergedModes, mode) + } + existing.SweepModes = mergedModes + } else { + config.DeviceTargets = append(config.DeviceTargets, newTarget) + existingTargets[newTarget.Network] = &config.DeviceTargets[len(config.DeviceTargets)-1] + } + } + } + Apply / Chat* Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that `DeviceTargets` are not deduplicated, unlike other merged fields, which could lead to redundant scans. The proposed fix to merge properties of duplicate targets is a robust improvement.	Medium
Possible issue	Ensure ticker reset signal is not dropped To prevent dropping ticker reset signals during rapid configuration updates, drain the `tickerReset` channel before sending a new signal. pkg/sweeper/sweeper.go [727-733] `- if config.Interval != oldInterval { + if config.Interval != oldInterval && config.Interval > 0 { + // Drain any pending signal to ensure the latest one is processed. select { - case s.tickerReset <- struct{}{}: + case <-s.tickerReset: default: - // Signal already pending } + s.tickerReset <- struct{}{} }` Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential race condition where a ticker reset signal could be dropped, making the new interval update mechanism more robust.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2673#issuecomment-3836989177 Original created: 2026-02-02T18:40:06Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>✅ <s>Re-evaluate the sweep group merging strategy</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit removed the global “merge all groups” logic (including choosing the shortest interval) by switching to returning a per-group SweepGroupsConfig where each group retains its own Interval. The spec text that required using the shortest interval as the global interval was also removed. code diff: ```diff -func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (*SweepConfig, error) { +func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (*SweepGroupsConfig, error) { if len(configJSON) == 0 { return nil, nil } @@ -87,74 +87,44 @@ if len(sweep.Groups) == 0 { log.Info().Msg("Gateway sweep config contained no groups; clearing sweep targets") - return &SweepConfig{ConfigHash: sweep.ConfigHash}, nil - } - - // Merge all groups into a single configuration - config := &SweepConfig{ + return &SweepGroupsConfig{ConfigHash: sweep.ConfigHash}, nil + } + + config := &SweepGroupsConfig{ ConfigHash: sweep.ConfigHash, - } - - // To avoid duplicate ports and modes - portMap := make(map[int]struct{}) - modeMap := make(map[models.SweepMode]struct{}) - networkMap := make(map[string]struct{}) - - for i, group := range sweep.Groups { - // Use the first group's ID as the primary sweep group ID for result attribution - if i == 0 { - sweepGroupID := group.SweepGroupID - if sweepGroupID == "" { - sweepGroupID = group.ID - } - config.SweepGroupID = sweepGroupID - } - - // Merge networks - for _, target := range normalizeTargets(group.Targets, log) { - if _, exists := networkMap[target]; !exists { - config.Networks = append(config.Networks, target) - networkMap[target] = struct{}{} - } - } - - // Merge ports - for _, port := range group.Ports { - if _, exists := portMap[port]; !exists { - config.Ports = append(config.Ports, port) - portMap[port] = struct{}{} - } - } - - // Merge sweep modes - for _, mode := range parseSweepModes(group.Modes, log) { - if _, exists := modeMap[mode]; !exists { - config.SweepModes = append(config.SweepModes, mode) - modeMap[mode] = struct{}{} - } - } - - // Merge device targets - config.DeviceTargets = append(config.DeviceTargets, convertDeviceTargets(group.DeviceTargets, log)...) - - // Merge settings - if group.Settings.Concurrency > config.Concurrency { - config.Concurrency = group.Settings.Concurrency + Groups: make([]SweepGroupConfig, 0, len(sweep.Groups)), + } + + for _, group := range sweep.Groups { + sweepGroupID := group.SweepGroupID + if sweepGroupID == "" { + sweepGroupID = group.ID + } + + groupConfig := SweepGroupConfig{ + ID: group.ID, + SweepGroupID: sweepGroupID, + Networks: normalizeTargets(group.Targets, log), + Ports: group.Ports, + SweepModes: parseSweepModes(group.Modes, log), + DeviceTargets: convertDeviceTargets(group.DeviceTargets, log), + Concurrency: group.Settings.Concurrency, + ScheduleType: strings.ToLower(strings.TrimSpace(group.Schedule.Type)), + CronExpression: strings.TrimSpace(group.Schedule.Cron), + ConfigHash: sweep.ConfigHash, } if interval, ok := parseScheduleInterval(group.Schedule, log); ok { - if config.Interval == 0 || interval < config.Interval { - config.Interval = interval - } + groupConfig.Interval = interval } if timeout, err := parseDurationValue(group.Settings.Timeout); err == nil { - if timeout > config.Timeout { - config.Timeout = timeout - } + groupConfig.Timeout = timeout } else if group.Settings.Timeout != "" { log.Warn().Err(err).Str("timeout", group.Settings.Timeout).Msg("Invalid sweep timeout") } + + config.Groups = append(config.Groups, groupConfig) } return config, nil # File: openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md @@ -1,62 +1 @@ -## MODIFIED Requirements -### Requirement: Sweep Job Compiled Config Output -The system SHALL compile sweep job configurations into the agent-consumable JSON format matching the existing `sweep.json` schema. - -#### Scenario: Compile sweep config for agent -- **GIVEN** a sweep job assigned to an agent -- **WHEN** the agent polls for config -- **THEN** the compiled config SHALL include: - - `networks`: CIDR ranges from device query evaluation - - `ports`: from selected profile or job override - - `sweep_modes`: ["tcp", "icmp", "tcp_connect"] based on profile - - `interval`: scan interval duration - - `concurrency`: parallel scan threads - - `timeout`: per-target timeout - - `icmp_count`, `high_perf_icmp`, `icmp_rate_limit`: ICMP settings - - `device_targets`: per-device configurations with metadata - -#### Scenario: Custom interval propagation -- **GIVEN** a sweep job with a configured interval of "6h" (21600s) provided via its profile -- **WHEN** the config is compiled -- **THEN** the `interval` field in the JSON SHALL reflect the 6 hour duration -- **AND** SHALL NOT default to 5 minutes - -#### Scenario: Device query evaluation at compile time -- **GIVEN** a sweep job with device query "tags.env = 'prod'" -- **WHEN** the config is compiled -- **THEN** the query SHALL be evaluated against current device inventory -- **AND** matching device IPs SHALL populate `networks` as /32 CIDRs -- **AND** device metadata SHALL populate `device_targets` entries - -#### Scenario: Merge multiple sweep jobs -- **GIVEN** multiple sweep jobs assigned to the same agent -- **WHEN** the agent polls for config -- **THEN** the configs SHALL be merged into a single sweep config -- **AND** networks and device_targets SHALL be combined -- **AND** the most restrictive settings SHALL be used for shared targets -- **AND** the shortest interval SHALL be used as the global interval if intervals differ - ``` </details> ___ **The current strategy of merging multiple sweep groups by using the shortest <br>interval can cause unintended high-frequency scanning. It is suggested to <br>re-evaluate this merging logic to prevent unexpected network load.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-0590118d93b1be7997233109d892bea154ed59038b8069290db99dc03080d307R145-R149">pkg/agent/sweep_config_gateway.go [145-149]</a> </summary> ```go if interval, ok := parseScheduleInterval(group.Schedule, log); ok { if config.Interval == 0 || interval < config.Interval { config.Interval = interval } } ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2673/files#diff-735b9b6d278ca31f4490b6370265c2be02454766daa41b68beb1591ed7bc42c4R38-R38">openspec/changes/fix-sweep-interval-config/specs/sweep-jobs/spec.md [38]</a> </summary> ```markdown - **AND** the shortest interval SHALL be used as the global interval if intervals differ ``` </details> ### Solution Walkthrough: #### Before: ```markdown func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (*SweepConfig, error) { // ... if len(sweep.Groups) == 0 { return &SweepConfig{}, nil } // Only the first group was used, others were ignored. group := sweep.Groups[0] if len(sweep.Groups) > 1 { log.Warn("using first group only") } config := &SweepConfig{ Networks: group.Targets, Ports: group.Ports, // ... other settings from the first group } // ... return config, nil } ``` #### After: ```markdown func parseGatewaySweepConfig(configJSON []byte, log logger.Logger) (*SweepConfig, error) { // ... config := &SweepConfig{} for _, group := range sweep.Groups { // Networks, ports, modes are merged config.Networks = append(config.Networks, group.Targets...) config.Ports = append(config.Ports, group.Ports...) // ... // The shortest interval across all groups is chosen. if interval, ok := parseScheduleInterval(group.Schedule, log); ok { if config.Interval == 0 || interval < config.Interval { config.Interval = interval } } } // ... return config, nil } ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion raises a critical design concern about the sweep group merging strategy, as using the shortest interval could cause unintended high network load, which is a significant potential issue. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Deduplicate device targets to prevent redundant scans</summary> ___ **Deduplicate <code>DeviceTargets</code> by their network address and merge their <code>SweepModes</code> to <br>prevent redundant scans and ensure all scan modes are included.** [pkg/agent/sweep_config_gateway.go [137-138]](https://github.com/carverauto/serviceradar/pull/2673/files#diff-0590118d93b1be7997233109d892bea154ed59038b8069290db99dc03080d307R137-R138) ```diff // Merge device targets - config.DeviceTargets = append(config.DeviceTargets, convertDeviceTargets(group.DeviceTargets, log)...) + if i == 0 { + config.DeviceTargets = convertDeviceTargets(group.DeviceTargets, log) + } else { + // Deduplicate device targets by network, merging sweep modes + existingTargets := make(map[string]*models.DeviceTarget, len(config.DeviceTargets)) + for i := range config.DeviceTargets { + existingTargets[config.DeviceTargets[i].Network] = &config.DeviceTargets[i] + } + for _, newTarget := range convertDeviceTargets(group.DeviceTargets, log) { + if existing, found := existingTargets[newTarget.Network]; found { + // Merge sweep modes + modeSet := make(map[models.SweepMode]struct{}) + for _, mode := range existing.SweepModes { + modeSet[mode] = struct{}{} + } + for _, mode := range newTarget.SweepModes { + modeSet[mode] = struct{}{} + } + + mergedModes := make([]models.SweepMode, 0, len(modeSet)) + for mode := range modeSet { + mergedModes = append(mergedModes, mode) + } + existing.SweepModes = mergedModes + } else { + config.DeviceTargets = append(config.DeviceTargets, newTarget) + existingTargets[newTarget.Network] = &config.DeviceTargets[len(config.DeviceTargets)-1] + } + } + } + ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that `DeviceTargets` are not deduplicated, unlike other merged fields, which could lead to redundant scans. The proposed fix to merge properties of duplicate targets is a robust improvement. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Ensure ticker reset signal is not dropped</summary> ___ **To prevent dropping ticker reset signals during rapid configuration updates, <br>drain the <code>tickerReset</code> channel before sending a new signal.** [pkg/sweeper/sweeper.go [727-733]](https://github.com/carverauto/serviceradar/pull/2673/files#diff-d11dee1504de681453c5a04e22ae44d64820221ac6b84a1630d3a3929dace47aR727-R733) ```diff - if config.Interval != oldInterval { + if config.Interval != oldInterval && config.Interval > 0 { + // Drain any pending signal to ensure the latest one is processed. select { - case s.tickerReset <- struct{}{}: + case <-s.tickerReset: default: - // Signal already pending } + s.tickerReset <- struct{}{} } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This suggestion correctly identifies a potential race condition where a ticker reset signal could be dropped, making the new interval update mechanism more robust. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>

mfreeman451 commented

2026-02-02 19:24:20 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2673#discussion_r2755869682
Original created: 2026-02-02T19:24:20Z
Original path: pkg/agent/push_loop.go
Original line: 804

seems like we should be able to set this

Imported GitHub PR review comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2673#discussion_r2755869682 Original created: 2026-02-02T19:24:20Z Original path: pkg/agent/push_loop.go Original line: 804 --- seems like we should be able to set this

mfreeman451 commented

2026-02-02 23:13:26 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2673#discussion_r2756421047
Original created: 2026-02-02T23:13:26Z
Original path: pkg/agent/sync_runtime.go
Original line: 383

Imported GitHub PR review comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2673#discussion_r2756421047 Original created: 2026-02-02T23:13:26Z Original path: pkg/agent/sync_runtime.go Original line: 383 --- ??