Skip to content

Conversation

Copy link

Copilot AI commented Oct 28, 2025

Code review identified a critical concurrency bug and logging inefficiencies.

Critical Fix

networking/cmd/main.go: Calling wg.Done() without corresponding wg.Add(1) causes panic when OAM server is disabled.

// Before - panics if oamservaddr is empty
setupOamServer(*enableTracing, *oamservaddr)  // conditionally calls wg.Add(1)
<-sigChan
close(termChan)
nbHandler.Stop()
netClient.Stop()
wg.Done()  // ❌ panic: sync: negative WaitGroup counter

// After
wg.Wait()  // ✅ properly waits for OAM goroutine if started

Error Logging

os-resource/internal/fsclient/fsclient.go: Removed duplicate error values in 8 log statements.

// Before - error logged twice
zlog.InfraSec().Error().Err(err).Msgf("Failed to connect: %v", err)

// After - Err(err) already includes error details
zlog.InfraSec().Error().Err(err).Msg("Failed to connect")

Documentation

  • Fixed broken telemetry-manager/telemetry/ link in README
  • Fixed malformed URL with $0 suffix

Build Artifacts

  • Added .gitignore files for networking/ and os-resource/ to exclude out/ directory and compiled binaries
Original prompt

do a code review and suggest chagnes


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Review code and suggest improvements Fix WaitGroup panic and redundant error logging Oct 28, 2025
Copilot AI requested a review from krishnajs October 28, 2025 16:17
@github-actions
Copy link

This pull request is stale because it has been open 30 days with no activity. Make a comment or update the PR to avoid closing PR after 15 days.

@github-actions github-actions bot added the Stale label Nov 28, 2025
@github-actions
Copy link

This pull request was automatically closed due to inactivity

@github-actions github-actions bot closed this Dec 13, 2025
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