fix wg.Done()

[kishori82]

Summary by CodeRabbit

• Bug Fixes

  • Improved synchronization in peer connection handling to enhance reliability and timing of stream operations.

• Chores

  • Increased runtime connection logging to provide clearer IP/connection visibility for debugging.

[Copilot]

Pull Request Overview

This PR fixes incorrect wg.Done() calls that were placed in goroutines without corresponding wg.Add() calls. The removed defer wg.Done() statements at lines 98 and 106 were causing WaitGroup synchronization issues since these goroutines were never added to the WaitGroup counter.

Key Changes:

• Removed two incorrect defer wg.Done() calls from goroutines that handle file output
• Reorganized defer conn.Close() placement (now before error check)
• Minor formatting adjustments

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Removed WaitGroup completion calls from two per-IP writer goroutines and added an immediate runtime print of the peer IP after creating the gRPC client; no exported API changes.

Changes

Cohort / File(s)	Summary
gRPC P2P client — writer/gRPC setup grpc_p2p_client/p2p_client_multi_streams_subscribe.go	Removed defer wg.Done() from the data and trace writer goroutines; added a runtime print of the peer IP right after gRPC client creation (keeps the existing IP print before error handling). No public declarations changed.

Sequence Diagram(s)

(omitted — changes are limited to minor goroutine sync and logging edits; control flow unchanged)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Pay attention to goroutine lifecycle and synchronization: confirm writers still signal completion or are intentionally unmanaged.
• Check for potential goroutine leaks or cases where wg.Wait() expectations no longer hold.
• Validate that the added immediate IP print doesn't expose sensitive data in production logs.

Possibly related PRs

• Kmk grpc add example #53 — touches the same file and the per-IP writer/goroutine setup; likely overlaps with synchronization or writer initialization changes.

Suggested reviewers

• paiva

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Go Build And Test Rationale	⚠️ Warning	Pull request removes defer wg.Done() calls from goroutines, creating deadlock vulnerability with zero test coverage for affected concurrency logic.	Restore defer wg.Done() calls in all goroutines and add comprehensive unit tests with context timeouts and race detection to CI pipeline.
Concurrency Safety	⚠️ Warning	Pull request removes defer wg.Done() from outer goroutines without corresponding wg.Add() calls, breaking synchronization and creating race conditions when channels are closed while writeToFile goroutines still access them.	Restore defer wg.Done() statements and add wg.Add(1) calls before spawning goroutines, or ensure channels close only after writeToFile completes via dataDone and traceDone signals.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix wg.Done()' is directly related to the main change in the changeset, which removed defer wg.Done() statements from goroutines, clearly summarizing the core modification.
Security Considerations	✅ Passed	Security check criteria (SQL injection, command injection, path traversal, unvalidated inputs, hardcoded credentials, insecure cryptography, missing rate limiting) were not found in the code modifications.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch kmk-fix-subscribe

---

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

[coderabbitai]

Actionable comments posted: 1

Caution

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

⚠️ Outside diff range comments (1)

grpc_p2p_client/p2p_client_multi_streams_subscribe.go (1)

96-102: Remove unnecessary wrapper goroutines.

The outer goroutine at line 98 immediately launches writeToFile in a nested goroutine and exits, serving no purpose. This adds unnecessary goroutine overhead. Launch writeToFile directly instead.

Also at: lines 105-109

Apply this diff to remove the wrapper:

 if *outputData != "" {
 	dataDone = make(chan bool)
-	go func() {
-		header := fmt.Sprintf("receiver\tsender\tsize\tsha256(msg)")
-		go writeToFile(ctx, dataCh, dataDone, *outputData, header)
-	}()
+	header := fmt.Sprintf("receiver\tsender\tsize\tsha256(msg)")
+	go writeToFile(ctx, dataCh, dataDone, *outputData, header)
 }

Apply similar changes at lines 105-109:

 if *outputTrace != "" {
 	traceDone = make(chan bool)
-	go func() {
-		header := "" //fmt.Sprintf("sender\tsize\tsha256(msg)")
-		go writeToFile(ctx, traceCh, traceDone, *outputTrace, header)
-	}()
+	header := "" //fmt.Sprintf("sender\tsize\tsha256(msg)")
+	go writeToFile(ctx, traceCh, traceDone, *outputTrace, header)
 }

📜 Review details

Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08befa5 and 1a448b7.

📒 Files selected for processing (1)

• grpc_p2p_client/p2p_client_multi_streams_subscribe.go (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*

⚙️ CodeRabbit configuration file

**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):

• Do NOT post separate comments for each occurrence.
• Post a single primary comment on the first occurrence.
• In that comment, include a short list of “Also at: file:line” references
  for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
  Prioritize signal over volume: one strong comment that references multiple
  locations is preferred over many near-identical comments.

Files:

• grpc_p2p_client/p2p_client_multi_streams_subscribe.go

**/*.go

⚙️ CodeRabbit configuration file

Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.

Files:

• grpc_p2p_client/p2p_client_multi_streams_subscribe.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Test Docker Setup

🔇 Additional comments (1)

grpc_p2p_client/p2p_client_multi_streams_subscribe.go (1)

96-128: Synchronization logic is correct after removing wg.Done() from writers.

The WaitGroup correctly tracks only the per-IP receiver goroutines (lines 112-118). After receivers complete, channels are closed (lines 121-122), allowing writer goroutines to drain and signal completion via done channels (lines 123-128). This separation is proper—writers operate on a different lifecycle than receivers.