Skip to content

Conversation

taylorsilva
Copy link
Member

@taylorsilva taylorsilva commented Mar 2, 2025

Changes proposed by this PR

This is a collection of small fixes across various parts of the atc package.

Release Note

  • Fix unbounded goroutine creation in resource scanner (lidar)
  • Fix potential race condition in Tracker.IterateInterpolatedCreds
  • Optimize SequenceGenerator using atomic types
  • Fix error message in container placement strategy. Previously an unknown placement strategy would result in an error which showed the successfully parsed part of the chain. Now the error will show the unknown strategy that was passed in.
  • Fix: redirect var source diffs to output writer & improve nil handling

analytically and others added 4 commits March 2, 2025 15:13
- Fix variable source diffs printing to stdout instead of provided writer
- Enhance practicallyDifferent to properly handle nil values
- nit: change interface{} to any, slowly modernizing go code

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
The error message when encountering an invalid placement strategy incorrectly
referenced the strategy variable (the accumulating result) rather than the
specific invalid strategy string that caused the error. This resulted in
misleading error messages that showed all strategies added so far instead
of just the invalid one.

Changed the error message to reference the specific invalid strategy string
for clearer troubleshooting.

nit: used newer `range int` feature in Go to make loop more readable

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
Replace mutex-based implementation with Go's atomic types to reduce contention
and improve performance under high concurrency. This change eliminates lock
overhead while maintaining thread safety.

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
…reds

Ensure the read lock is properly released using defer to guarantee thread safety
when iterating over credentials. This prevents a possible race condition if
multiple goroutines access the Tracker concurrently.

nit: modernize code by using any instead of interface{}

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
@taylorsilva taylorsilva added the bug label Mar 2, 2025
@taylorsilva taylorsilva requested a review from a team as a code owner March 2, 2025 20:24
The scanner.scanResources method was creating a goroutine for each
resource without any concurrency limits. For deployments with many
resources, this could lead to resource exhaustion on the web node and
possibly the database.

This commit introduces a concurrency limit of 50 simultaneous resource
checks. The implementation spawns 50 go routines and works through
resources passed down a channel.

Additionally, improved context handling was added to ensure proper
cleanup when the scanner is being shut down or the context is otherwise
canceled. This prevents orphaned goroutines from continuing to run after
their parent process has been signaled to stop.

These changes make the scanner more robust and predictable under high
load while maintaining the existing parallel checking behavior.

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
Co-authored-by: Taylor Silva <dev@taydev.net>
@taylorsilva taylorsilva merged commit 2cfed36 into master Mar 3, 2025
11 checks passed
@taylorsilva taylorsilva deleted the small-fixes branch March 3, 2025 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants