refactor: move federated server logic to its own service#3
Conversation
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Architecture: Introduced Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The refactor isolates federation concerns into a dedicated type, improving separation of concerns. Moving Scalability & Extensibility: The Risks: The |
Greptile SummaryThis PR extracts the federated-server proxy logic out of
Confidence Score: 3/5The refactoring cleanly separates the federated-server concern, but two runtime correctness issues in the new federation.go file should be resolved before merging. The empty-slice panic in handleConn is a real crash path that fires whenever a client connects while no online nodes have been discovered yet. The double node.Start call is inherited from the original code but now codified in the new service. core/p2p/federation.go needs attention for the empty-tunnel-address panic in handleConn and the double Start call sequence. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as FederatedCLI.Run
participant FS as FederatedServer
participant Node as p2p Node
participant SD as ServiceDiscoverer
participant DT as discoveryTunnels
participant HC as handleConn
CLI->>FS: NewFederatedServer(addr, FederatedID, token)
CLI->>FS: Start(ctx)
FS->>Node: NewNode(token)
FS->>Node: n.Start(ctx) first Start
FS->>SD: ServiceDiscoverer(ctx, n, token, FederatedID, nil)
SD->>DT: discoveryTunnels(ctx, n, token, servicesID)
DT->>Node: n.Start(ctx) second Start
DT-->>SD: chan NodeData
SD-->>FS: nil
FS->>FS: proxy(ctx, n)
loop accept connections
FS->>FS: l.Accept()
FS->>HC: go handleConn(conn)
HC->>HC: GetAvailableNodes(FederatedID)
alt tunnelAddresses empty
HC-->>HC: PANIC rand.IntN(0)
else at least one online node
HC->>HC: rand pick tunnel address
HC->>HC: net.Dial(tunnelAddr)
HC->>HC: copyStream bidirectional
end
end
Reviews (1): Last reviewed commit: "refactor: move federated server logic to..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
If all federated nodes are offline, tunnelAddresses is empty and rand.IntN(0) panics at runtime. The refactoring extracted this into a standalone handleConn function, preserving the crash path. Any incoming connection when the cluster has no live members will terminate the goroutine with a panic rather than returning a clean error to the caller.
| err = n.Start(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("starting a new node: %w", err) | ||
| } | ||
|
|
||
| if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil); err != nil { |
There was a problem hiding this comment.
FederatedServer.Start calls n.Start(ctx) on line 28, then ServiceDiscoverer → discoveryTunnels (in p2p.go) calls n.Start(ctx) a second time on the same node instance. This mirrors the original FederatedCLI.Run behaviour but is carried forward here. Depending on how edgevpn's node.Start handles duplicate calls (panic, error, or silent no-op), this could result in unexpected runtime behaviour.
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
rand.IntN so an empty slice does not cause a panic. The connection should be closed cleanly with a log message instead.
| // open a TCP stream to one of the tunnels | |
| // chosen randomly | |
| // TODO: optimize this and track usage | |
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] | |
| // open a TCP stream to one of the tunnels | |
| // chosen randomly | |
| // TODO: optimize this and track usage | |
| if len(tunnelAddresses) == 0 { | |
| log.Error().Msg("No online tunnel nodes available") | |
| conn.Close() | |
| return | |
| } | |
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
![codity-dm[bot]](https://avatars.githubusercontent.com/u/234452856?s=60&v=4){kind=small}
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
If tunnelAddresses is empty (no online nodes), rand.IntN(0) panics with a runtime error; add a guard before indexing.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
If `tunnelAddresses` is empty (no online nodes), `rand.IntN(0)` panics with a runtime error; add a guard before indexing.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) { | ||
| defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy | ||
| io.Copy(dst, src) | ||
| return fs.Start(context.Background()) |
There was a problem hiding this comment.
The new code passes context.Background() instead of the CLI-provided context, so cancellation signals (e.g., SIGINT) from the caller are ignored and the server cannot be gracefully shut down.
| return fs.Start(context.Background()) | |
| return fs.Start(ctx.Context) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 18-18
Issue Type: functional-high
Severity: high
Issue Description:
The new code passes `context.Background()` instead of the CLI-provided context, so cancellation signals (e.g., SIGINT) from the caller are ignored and the server cannot be gracefully shut down.
Current Code:
return fs.Start(context.Background())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func NewFederatedServer(listenAddr, service, p2pToken string) *FederatedServer { | ||
| return &FederatedServer{ | ||
| listenAddr: listenAddr, | ||
| service: service, | ||
| p2ptoken: p2pToken, | ||
| } | ||
| } |
There was a problem hiding this comment.
The p2p token is stored as a plain string field with no validation or sanitization; an empty or malformed token passed to NewFederatedServer will propagate silently and be used to initialize a network node, potentially allowing unauthenticated federation. Add a non-empty check and return an error or panic early.
Suggested fix
func NewFederatedServer(listenAddr, service, p2pToken string) (*FederatedServer, error) {
if p2pToken == "" {
return nil, fmt.Errorf("p2pToken must not be empty")
}
if listenAddr == "" {
return nil, fmt.Errorf("listenAddr must not be empty")
}
return &FederatedServer{
listenAddr: listenAddr,
service: service,
p2ptoken: p2pToken,
}, nil
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federatedServer.go
Lines: 7-13
Issue Type: security-high
Severity: high
Issue Description:
The p2p token is stored as a plain string field with no validation or sanitization; an empty or malformed token passed to NewFederatedServer will propagate silently and be used to initialize a network node, potentially allowing unauthenticated federation. Add a non-empty check and return an error or panic early.
Current Code:
func NewFederatedServer(listenAddr, service, p2pToken string) *FederatedServer {
return &FederatedServer{
listenAddr: listenAddr,
service: service,
p2ptoken: p2pToken,
}
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| } | ||
| // ll.Info("Binding local port on", srcaddr) | ||
|
|
||
| ledger, _ := node.Ledger() |
There was a problem hiding this comment.
The error returned by node.Ledger() is silently discarded with _; a nil ledger will cause a nil-pointer panic on the subsequent ledger.Announce call.
Suggested fix
ledger, err := node.Ledger()
if err != nil {
log.Error().Err(err).Msg("Error getting ledger")
return err
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 51-51
Issue Type: robustness-high
Severity: high
Issue Description:
The error returned by `node.Ledger()` is silently discarded with `_`; a nil ledger will cause a nil-pointer panic on the subsequent `ledger.Announce` call.
Current Code:
ledger, _ := node.Ledger()
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelAddresses = append(tunnelAddresses, v.TunnelAddress) | ||
| } else { | ||
| log.Info().Msgf("Node %s is offline", v.ID) | ||
| } | ||
| } | ||
|
|
||
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] | ||
|
|
||
| tunnelConn, err := net.Dial("tcp", tunnelAddr) |
There was a problem hiding this comment.
TunnelAddress values sourced from the distributed ledger (untrusted peers) are passed directly to net.Dial without validation, allowing a malicious peer to redirect connections to arbitrary internal hosts (SSRF).
Suggested fix
// Validate that tunnelAddr resolves to an expected loopback/VPN range before dialing.
// Example: parse host and verify it is within the allowed CIDR before appending.
host, _, err := net.SplitHostPort(v.TunnelAddress)
if err != nil {
log.Warn().Msgf("Skipping invalid tunnel address %s", v.TunnelAddress)
continue
}
ip := net.ParseIP(host)
if ip == nil || !allowedCIDR.Contains(ip) {
log.Warn().Msgf("Tunnel address %s outside allowed range, skipping", v.TunnelAddress)
continue
}
tunnelAddresses = append(tunnelAddresses, v.TunnelAddress)Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 97-108
Issue Type: security-high
Severity: high
Issue Description:
`TunnelAddress` values sourced from the distributed ledger (untrusted peers) are passed directly to `net.Dial` without validation, allowing a malicious peer to redirect connections to arbitrary internal hosts (SSRF).
Current Code:
tunnelAddresses = append(tunnelAddresses, v.TunnelAddress)
}
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
tunnelConn, err := net.Dial("tcp", tunnelAddr)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return errors.New("context canceled") | ||
| default: | ||
| log.Debug().Msg("New for connection") | ||
| // Listen for an incoming connection. | ||
| conn, err := l.Accept() | ||
| if err != nil { | ||
| fmt.Println("Error accepting: ", err.Error()) | ||
| continue | ||
| } | ||
|
|
||
| // Handle connections in a new goroutine, forwarding to the p2p service | ||
| go handleConn(conn) | ||
| } | ||
| } |
There was a problem hiding this comment.
The proxy accept loop spins in the default branch without blocking, consuming 100% CPU when no connections arrive; l.Accept() already blocks, but the surrounding select with a default case means the goroutine never yields to the scheduler between the context check and the blocking Accept call on a busy loop iteration: replace with a blocking accept and a separate context-cancellation mechanism.
Suggested fix
go func() {
<-ctx.Done()
l.Close()
}()
for {
conn, err := l.Accept()
if err != nil {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
log.Error().Err(err).Msg("Error accepting")
continue
}
}
go handleConn(conn)
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 73-89
Issue Type: performance-high
Severity: high
Issue Description:
The `proxy` accept loop spins in the `default` branch without blocking, consuming 100% CPU when no connections arrive; `l.Accept()` already blocks, but the surrounding `select` with a `default` case means the goroutine never yields to the scheduler between the context check and the blocking `Accept` call on a busy loop iteration: replace with a blocking accept and a separate context-cancellation mechanism.
Current Code:
for {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
log.Debug().Msg("New for connection")
conn, err := l.Accept()
if err != nil {
fmt.Println("Error accepting: ", err.Error())
continue
}
go handleConn(conn)
}
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) { | ||
| defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy | ||
| io.Copy(dst, src) | ||
| } |
There was a problem hiding this comment.
io.Copy errors are silently ignored in copyStream; log or surface the error so connection failures are observable.
Suggested fix
func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) {
defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy
if _, err := io.Copy(dst, src); err != nil {
log.Debug().Err(err).Msg("copyStream error")
}
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 124-127
Issue Type: robustness-medium
Severity: medium
Issue Description:
`io.Copy` errors are silently ignored in `copyStream`; log or surface the error so connection failures are observable.
Current Code:
func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) {
defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy
io.Copy(dst, src)
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security ScanSecurity scan in progress... Analyzing code for:
Results will be posted shortly... |
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Architecture: Introduced CLI Integration: Updated Build Tags: Added a stub Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The refactor isolates federated server concerns into a dedicated type, making the CLI command a thin wrapper. This separates concerns and improves testability. Moving Scalability & Extensibility: The Risks: The |
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
If tunnelAddresses is empty, rand.IntN(0) panics with a runtime error; add a guard to close the connection and return early when no online nodes are available.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
If `tunnelAddresses` is empty, `rand.IntN(0)` panics with a runtime error; add a guard to close the connection and return early when no online nodes are available.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) { | ||
| defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy | ||
| io.Copy(dst, src) | ||
| return fs.Start(context.Background()) |
There was a problem hiding this comment.
The new code passes context.Background() instead of the CLI-provided context, so cancellation signals (e.g., SIGINT) from the caller are ignored and the server cannot be gracefully shut down.
| return fs.Start(context.Background()) | |
| return fs.Start(ctx.Context) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 18-18
Issue Type: robustness-high
Severity: high
Issue Description:
The new code passes `context.Background()` instead of the CLI-provided context, so cancellation signals (e.g., SIGINT) from the caller are ignored and the server cannot be gracefully shut down.
Current Code:
return fs.Start(context.Background())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| } | ||
| // ll.Info("Binding local port on", srcaddr) | ||
|
|
||
| ledger, _ := node.Ledger() |
There was a problem hiding this comment.
The error from node.Ledger() is silently discarded with _; if ledger acquisition fails, subsequent calls on the nil ledger will panic.
Suggested fix
ledger, err := node.Ledger()
if err != nil {
log.Error().Err(err).Msg("Error getting ledger")
return err
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 51-51
Issue Type: robustness-high
Severity: high
Issue Description:
The error from `node.Ledger()` is silently discarded with `_`; if ledger acquisition fails, subsequent calls on the nil ledger will panic.
Current Code:
ledger, _ := node.Ledger()
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return errors.New("context canceled") | ||
| default: | ||
| log.Debug().Msg("New for connection") | ||
| // Listen for an incoming connection. | ||
| conn, err := l.Accept() | ||
| if err != nil { | ||
| fmt.Println("Error accepting: ", err.Error()) | ||
| continue | ||
| } | ||
|
|
||
| // Handle connections in a new goroutine, forwarding to the p2p service | ||
| go handleConn(conn) | ||
| } | ||
| } |
There was a problem hiding this comment.
When l.Accept() returns an error after context cancellation, the loop continues to the default branch and calls l.Accept() again instead of exiting; check ctx.Done() before retrying or use a non-blocking select with a timeout.
Suggested fix
for {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
}
log.Debug().Msg("New for connection")
conn, err := l.Accept()
if err != nil {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
}
log.Error().Err(err).Msg("Error accepting connection")
continue
}
go handleConn(conn)
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 73-89
Issue Type: robustness-medium
Severity: medium
Issue Description:
When `l.Accept()` returns an error after context cancellation, the loop continues to the `default` branch and calls `l.Accept()` again instead of exiting; check `ctx.Done()` before retrying or use a non-blocking select with a timeout.
Current Code:
for {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
log.Debug().Msg("New for connection")
conn, err := l.Accept()
if err != nil {
fmt.Println("Error accepting: ", err.Error())
continue
}
go handleConn(conn)
}
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
There was a problem hiding this comment.
In federation.go handleConn, if tunnelAddresses is empty (no online nodes), rand.IntN(0) panics with a divide-by-zero, crashing the goroutine and potentially the process; add a length check and close the connection before returning.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 100-104
Issue Type: security-critical
Severity: critical
Issue Description:
In `federation.go` `handleConn`, if `tunnelAddresses` is empty (no online nodes), `rand.IntN(0)` panics with a divide-by-zero, crashing the goroutine and potentially the process; add a length check and close the connection before returning.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 37.7s View vulnerability details (2 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. 2. SEC-400 (CWE-400) MEDIUM File:
Fix: Before calling rand.IntN, verify that tunnelAddresses is not empty. If no peers are available, reject the connection or wait until one is available instead of panicking. Security scan powered by Codity.ai |
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Core: New CLI: Build System: Added Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The extraction intentionally keeps Risks: (Intentional) |
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
tunnelAddresses may be empty when no nodes are online, causing rand.IntN(0) to panic with an index-out-of-range; add a guard before indexing.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
`tunnelAddresses` may be empty when no nodes are online, causing `rand.IntN(0)` to panic with an index-out-of-range; add a guard before indexing.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) { | ||
| defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy | ||
| io.Copy(dst, src) | ||
| return fs.Start(context.Background()) |
There was a problem hiding this comment.
The Run method ignores the incoming ctx parameter and passes context.Background() to fs.Start, preventing proper cancellation or shutdown propagation from the CLI framework.
| return fs.Start(context.Background()) | |
| return fs.Start(ctx.Context) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 18-18
Issue Type: functional-high
Severity: high
Issue Description:
The `Run` method ignores the incoming `ctx` parameter and passes `context.Background()` to `fs.Start`, preventing proper cancellation or shutdown propagation from the CLI framework.
Current Code:
return fs.Start(context.Background())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| } | ||
| // ll.Info("Binding local port on", srcaddr) | ||
|
|
||
| ledger, _ := node.Ledger() |
There was a problem hiding this comment.
The error return from node.Ledger() is silently discarded; if ledger acquisition fails, subsequent calls on a nil ledger will panic.
Suggested fix
ledger, err := node.Ledger()
if err != nil {
return fmt.Errorf("getting ledger: %w", err)
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 51-51
Issue Type: robustness-high
Severity: high
Issue Description:
The error return from `node.Ledger()` is silently discarded; if ledger acquisition fails, subsequent calls on a nil ledger will panic.
Current Code:
ledger, _ := node.Ledger()
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelConn, err := net.Dial("tcp", tunnelAddr) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error connecting to tunnel") | ||
| return | ||
| } |
There was a problem hiding this comment.
When handleConn returns early after a dial error, conn is never closed, leaking the accepted connection.
Suggested fix
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
conn.Close()
return
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 108-112
Issue Type: robustness-high
Severity: high
Issue Description:
When `handleConn` returns early after a dial error, `conn` is never closed, leaking the accepted connection.
Current Code:
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return errors.New("context canceled") | ||
| default: | ||
| log.Debug().Msg("New for connection") | ||
| // Listen for an incoming connection. | ||
| conn, err := l.Accept() | ||
| if err != nil { | ||
| fmt.Println("Error accepting: ", err.Error()) | ||
| continue | ||
| } | ||
|
|
||
| // Handle connections in a new goroutine, forwarding to the p2p service | ||
| go handleConn(conn) | ||
| } | ||
| } |
There was a problem hiding this comment.
l.Accept() blocks indefinitely in the default branch, so the ctx.Done() case is never reached while waiting for a connection; use a deadline or a separate goroutine to make context cancellation responsive.
Suggested fix
go func() {
<-ctx.Done()
l.Close()
}()
for {
conn, err := l.Accept()
if err != nil {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
log.Error().Err(err).Msg("Error accepting")
continue
}
}
go handleConn(conn)
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 73-89
Issue Type: robustness-medium
Severity: medium
Issue Description:
`l.Accept()` blocks indefinitely in the `default` branch, so the `ctx.Done()` case is never reached while waiting for a connection; use a deadline or a separate goroutine to make context cancellation responsive.
Current Code:
for {
select {
case <-ctx.Done():
return errors.New("context canceled")
default:
log.Debug().Msg("New for connection")
// Listen for an incoming connection.
conn, err := l.Accept()
if err != nil {
fmt.Println("Error accepting: ", err.Error())
continue
}
// Handle connections in a new goroutine, forwarding to the p2p service
go handleConn(conn)
}
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 22.1s View vulnerability details (1 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. Security scan powered by Codity.ai |
Dependency vulnerability scanning
Top 10 Vulnerabilities (171 total found)1. stdlib < 1.16.1 CVE: GO-2021-0067
2. stdlib < 1.15.5 CVE: GO-2021-0069
3. stdlib < 1.14.7 CVE: GO-2021-0142
4. stdlib < 1.3.2 CVE: GO-2021-0154
5. stdlib < 1.4.3 CVE: GO-2021-0159
6. stdlib < 1.5.3 CVE: GO-2021-0160
7. stdlib < 1.6.1 CVE: GO-2021-0163
8. stdlib < 1.7.4 CVE: GO-2021-0172
9. stdlib < 1.9.1 CVE: GO-2021-0178
10. stdlib < 1.14.5 CVE: GO-2021-0223
161 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
Dependency vulnerability scanning
View vulnerability details (4 items)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
Powered by Codity.ai · Docs |
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Networking: New CLI: Federated command now delegates to Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The split follows a clear layering pattern. CLI handles command parsing and flag binding. Scalability & Extensibility: The Risks:
Merge StatusNOT MERGEABLE — PR Score 34/100, below threshold (50)
|
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
Selecting a random tunnel without checking for an empty list can panic, so return early when no online federated nodes are available.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online federated nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
Selecting a random tunnel without checking for an empty list can panic, so return early when no online federated nodes are available.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelConn, err := net.Dial("tcp", tunnelAddr) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error connecting to tunnel") | ||
| return |
There was a problem hiding this comment.
The accepted client connection is left open on dial failure, so close it before returning.
Suggested fix
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
conn.Close()
return
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 108-111
Issue Type: robustness-high
Severity: high
Issue Description:
The accepted client connection is left open on dial failure, so close it before returning.
Current Code:
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 33.0s View vulnerability details (2 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. 2. SEC-001 (CWE-703) MEDIUM File:
Fix: Before calling Security scan powered by Codity.ai |
Dependency vulnerability scanning
Top 10 Vulnerabilities (171 total found)1. github.com/mudler/LocalAI 0+ CVE: GO-2024-3253
2. github.com/mudler/LocalAI 0+ CVE: GO-2025-3542
3. stdlib < 1.16.1 CVE: GO-2021-0067
4. stdlib < 1.15.5 CVE: GO-2021-0069
5. stdlib < 1.14.7 CVE: GO-2021-0142
6. stdlib < 1.3.2 CVE: GO-2021-0154
7. stdlib < 1.4.3 CVE: GO-2021-0159
8. stdlib < 1.5.3 CVE: GO-2021-0160
9. stdlib < 1.6.1 CVE: GO-2021-0163
10. stdlib < 1.7.4 CVE: GO-2021-0172
161 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
Dependency vulnerability scanning
View vulnerability details (4 items)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
Powered by Codity.ai · Docs |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Networking: New CLI: Build System: Added stub Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Moved P2P logic out of CLI to improve testability and follow package boundaries. The Risks: Merge StatusNOT MERGEABLE — PR Score 34/100, below threshold (50)
|
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
Selecting a random tunnel without checking for an empty slice will panic, so return early when no online nodes are available.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online federated nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
Selecting a random tunnel without checking for an empty slice will panic, so return early when no online nodes are available.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelConn, err := net.Dial("tcp", tunnelAddr) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error connecting to tunnel") | ||
| return |
There was a problem hiding this comment.
The accepted client connection is leaked when net.Dial fails, so close conn before returning on this error path.
Suggested fix
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
conn.Close()
return
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 108-111
Issue Type: robustness-high
Severity: high
Issue Description:
The accepted client connection is leaked when net.Dial fails, so close conn before returning on this error path.
Current Code:
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 31.1s View vulnerability details (2 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. 2. SEC-DO-001 (CWE-400) MEDIUM File:
Fix: Before calling rand.IntN, verify that tunnelAddresses is non-empty and handle the case where no peers are available by rejecting the connection or waiting for a node to come online. Security scan powered by Codity.ai |
Dependency vulnerability scanning
Top 10 Vulnerabilities (171 total found)1. github.com/mudler/LocalAI 0+ CVE: GO-2024-3253
2. github.com/mudler/LocalAI 0+ CVE: GO-2025-3542
3. stdlib < 1.16.1 CVE: GO-2021-0067
4. stdlib < 1.15.5 CVE: GO-2021-0069
5. stdlib < 1.14.7 CVE: GO-2021-0142
6. stdlib < 1.3.2 CVE: GO-2021-0154
7. stdlib < 1.4.3 CVE: GO-2021-0159
8. stdlib < 1.5.3 CVE: GO-2021-0160
9. stdlib < 1.6.1 CVE: GO-2021-0163
10. stdlib < 1.7.4 CVE: GO-2021-0172
161 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
Dependency vulnerability scanning
Top 10 Vulnerabilities (23 total found)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
5. torch 2.12.0 CVE: PYSEC-2025-210
6. torch 2.12.0 CVE: PYSEC-2025-194
7. torch 2.12.0 CVE: PYSEC-2025-196
8. torch 2.12.0 CVE: PYSEC-2025-195
9. torch 2.12.0 CVE: PYSEC-2025-193
10. torch 2.12.0 CVE: PYSEC-2025-192
13 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 8 packagesMPL-2.0 (6 packages):
Mozilla Public License 2.0 (MPL 2.0) (2 packages):
Unknown Licenses - 53 packages
...and 33 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/LocalAI · PR #3Scanned: 2026-05-21 10:28 UTC | Score: 57/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-001]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
core/p2p/federation.go |
0 | 2 | 2 | 2 | 6 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Core: New CLI: Build System: Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The split between Scalability & Extensibility: Risks: Random tunnel selection in Merge StatusNOT MERGEABLE — PR Score 32/100, below threshold (50)
|
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
Selecting a random tunnel from an empty slice panics, so return early when no online federated nodes are available.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online federated nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
Selecting a random tunnel from an empty slice panics, so return early when no online federated nodes are available.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| func copyStream(closer chan struct{}, dst io.Writer, src io.Reader) { | ||
| defer func() { closer <- struct{}{} }() // connection is closed, send signal to stop proxy | ||
| io.Copy(dst, src) | ||
| return fs.Start(context.Background()) |
There was a problem hiding this comment.
This ignores the CLI context and starts the server with a detached background context, so shutdown and cancellation will not propagate; pass the provided context instead.
| return fs.Start(context.Background()) | |
| return fs.Start(ctx) |
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 18-18
Issue Type: functional-high
Severity: high
Issue Description:
This ignores the CLI context and starts the server with a detached background context, so shutdown and cancellation will not propagate; pass the provided context instead.
Current Code:
return fs.Start(context.Background())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelConn, err := net.Dial("tcp", tunnelAddr) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error connecting to tunnel") | ||
| return |
There was a problem hiding this comment.
The accepted client connection is leaked on dial failure, so close conn before returning.
Suggested fix
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
conn.Close()
return
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 108-111
Issue Type: robustness-high
Severity: high
Issue Description:
The accepted client connection is leaked on dial failure, so close conn before returning.
Current Code:
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 23.4s View vulnerability details (1 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. Security scan powered by Codity.ai |
Dependency vulnerability scanning
Top 10 Vulnerabilities (171 total found)1. stdlib < 1.16.1 CVE: GO-2021-0067
2. stdlib < 1.15.5 CVE: GO-2021-0069
3. stdlib < 1.14.7 CVE: GO-2021-0142
4. stdlib < 1.3.2 CVE: GO-2021-0154
5. stdlib < 1.4.3 CVE: GO-2021-0159
6. stdlib < 1.5.3 CVE: GO-2021-0160
7. stdlib < 1.6.1 CVE: GO-2021-0163
8. stdlib < 1.7.4 CVE: GO-2021-0172
9. stdlib < 1.9.1 CVE: GO-2021-0178
10. stdlib < 1.14.5 CVE: GO-2021-0223
161 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
Dependency vulnerability scanning
Top 10 Vulnerabilities (23 total found)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
5. torch 2.12.0 CVE: PYSEC-2025-210
6. torch 2.12.0 CVE: PYSEC-2025-194
7. torch 2.12.0 CVE: PYSEC-2025-196
8. torch 2.12.0 CVE: PYSEC-2025-195
9. torch 2.12.0 CVE: PYSEC-2025-193
10. torch 2.12.0 CVE: PYSEC-2025-192
13 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 8 packagesMPL-2.0 (6 packages):
Mozilla Public License 2.0 (MPL 2.0) (2 packages):
Unknown Licenses - 48 packages
...and 28 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/LocalAI · PR #3Scanned: 2026-05-21 10:44 UTC | Score: 54/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-001]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
core/p2p/federation.go |
0 | 2 | 3 | 2 | 7 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaP2P Package: Added CLI: Reduced Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The split follows existing build tag patterns ( Scalability & Extensibility: Out of scope. This is a pure refactor with no behavior change. Risks:
Merge StatusNOT MERGEABLE — PR Score 33/100, below threshold (50)
|
| } | ||
|
|
||
| func (f *FederatedCLI) Run(ctx *cliContext.Context) error { | ||
| fs := p2p.NewFederatedServer(f.Address, p2p.FederatedID, f.Peer2PeerToken) |
There was a problem hiding this comment.
In a non-p2p build, running the federated CLI now always fails with "not implemented" because this caller was switched to FederatedServer.Start while the new implementation exists only in the p2p-only file; trigger: build without the p2p tag and execute the federated command.
Either move the real FederatedServer.Start implementation into an untagged file, add the same build tag to the CLI command so it is unavailable without p2p, or preserve the previous non-tagged implementation path.
Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/cli/federated.go
Lines: 16-16
Issue Type: functional-critical
Severity: critical
Issue Description:
In a non-p2p build, running the federated CLI now always fails with "not implemented" because this caller was switched to FederatedServer.Start while the new implementation exists only in the p2p-only file; trigger: build without the `p2p` tag and execute the federated command.
Current Code:
fs := p2p.NewFederatedServer(f.Address, p2p.FederatedID, f.Peer2PeerToken)
return fs.Start(context.Background())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| // open a TCP stream to one of the tunnels | ||
| // chosen randomly | ||
| // TODO: optimize this and track usage | ||
| tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))] |
There was a problem hiding this comment.
Selecting a tunnel with rand.IntN(len(tunnelAddresses)) panics when no online federated nodes are available, so guard the empty slice and close the client connection cleanly.
Suggested fix
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online federated nodes available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 106-106
Issue Type: functional-critical
Severity: critical
Issue Description:
Selecting a tunnel with rand.IntN(len(tunnelAddresses)) panics when no online federated nodes are available, so guard the empty slice and close the client connection cleanly.
Current Code:
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| tunnelConn, err := net.Dial("tcp", tunnelAddr) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Error connecting to tunnel") | ||
| return |
There was a problem hiding this comment.
The accepted client connection is leaked on tunnel dial failure, so close conn before returning from the error path.
Suggested fix
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
conn.Close()
return
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 108-111
Issue Type: robustness-high
Severity: high
Issue Description:
The accepted client connection is leaked on tunnel dial failure, so close conn before returning from the error path.
Current Code:
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| } | ||
|
|
||
| // Handle connections in a new goroutine, forwarding to the p2p service | ||
| go handleConn(conn) |
There was a problem hiding this comment.
Spawning an unbounded goroutine for every accepted connection allows trivial resource exhaustion, so enforce a concurrency limit or backpressure before launching handleConn.
Suggested fix
select {
case sem <- struct{}{}:
go func() {
defer func() { <-sem }()
handleConn(conn)
}()
default:
log.Error().Msg("Too many concurrent connections")
conn.Close()
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/p2p/federation.go
Lines: 87-87
Issue Type: performance-high
Severity: high
Issue Description:
Spawning an unbounded goroutine for every accepted connection allows trivial resource exhaustion, so enforce a concurrency limit or backpressure before launching handleConn.
Current Code:
go handleConn(conn)
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Security Scan Summary
Consider addressing security findings before merging Scan completed in 21.9s View vulnerability details (1 items)1. MATH_RANDOM_USED (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)) HIGH File:
Fix: Review and fix the security issue following OWASP guidelines. Security scan powered by Codity.ai |
Dependency vulnerability scanning
Top 10 Vulnerabilities (171 total found)1. stdlib < 1.16.1 CVE: GO-2021-0067
2. stdlib < 1.15.5 CVE: GO-2021-0069
3. stdlib < 1.14.7 CVE: GO-2021-0142
4. stdlib < 1.3.2 CVE: GO-2021-0154
5. stdlib < 1.4.3 CVE: GO-2021-0159
6. stdlib < 1.5.3 CVE: GO-2021-0160
7. stdlib < 1.6.1 CVE: GO-2021-0163
8. stdlib < 1.7.4 CVE: GO-2021-0172
9. stdlib < 1.9.1 CVE: GO-2021-0178
10. stdlib < 1.14.5 CVE: GO-2021-0223
161 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
Dependency vulnerability scanning
Top 10 Vulnerabilities (23 total found)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
5. torch 2.12.0 CVE: PYSEC-2025-210
6. torch 2.12.0 CVE: PYSEC-2025-194
7. torch 2.12.0 CVE: PYSEC-2025-196
8. torch 2.12.0 CVE: PYSEC-2025-195
9. torch 2.12.0 CVE: PYSEC-2025-193
10. torch 2.12.0 CVE: PYSEC-2025-192
13 more vulnerabilities not shown. Update dependencies to fix these issues. Powered by Codity.ai · Docs |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 8 packagesMPL-2.0 (6 packages):
Mozilla Public License 2.0 (MPL 2.0) (2 packages):
Unknown Licenses - 53 packages
...and 33 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/LocalAI · PR #3Scanned: 2026-05-21 11:31 UTC | Score: 55/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-001]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
core/p2p/federation.go |
0 | 2 | 3 | 1 | 6 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
chay2199
left a comment
chay2199
left a comment
There was a problem hiding this comment.
Code Review: refactor: move federated server logic to its own service
The refactor cleanly extracts the federated proxy logic into a FederatedServer struct inside the p2p package, slimming down cli/federated.go to a thin three-liner. The structural intent — struct in an untagged file, implementation behind //go:build p2p, stub in //go:build !p2p — is sound. However, there is a hard compilation error and a crash-level bug that must be fixed before merging.
🔴 Critical — Must Fix
1. Compilation error: ServiceDiscoverer called with wrong arity (federation.go:34)
The actual signature of ServiceDiscoverer in p2p.go is:
func ServiceDiscoverer(ctx context.Context, n *node.Node, token, servicesID string,
discoveryFunc func(serviceID string, node schema.NodeData), allocate bool) errorBut federation.go calls it with only 5 arguments:
if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil); err != nil {The allocate bool parameter is missing. This is a hard build failure under the p2p build tag. Given that handleConn later calls GetAvailableNodes and walks tunnel addresses, allocate almost certainly needs to be true here; without it, nodes will never have a TunnelAddress set (see ensureService), and every connection attempt will silently hit an empty slice.
Fix:
if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil, true); err != nil {2. Nil/empty-slice panic in handleConn (federation.go:97)
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]If every discovered node reports itself offline, tunnelAddresses is empty and rand.IntN(0) panics, crashing the goroutine with no log or graceful degradation. This was a latent bug in the original Proxy function that this refactor perpetuates — and now the panic happens inside a goroutine with no recovery.
Fix:
if len(tunnelAddresses) == 0 {
log.Error().Msg("no online tunnel nodes available, dropping connection")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]🟡 Moderate — Should Fix
3. p2p_disabled.go stub is out of sync with the real signature
The stub in p2p_disabled.go is:
func ServiceDiscoverer(ctx context.Context, node *node.Node, token, servicesID string, fn func()) error {The real signature has a func(serviceID string, node schema.NodeData) callback and an extra allocate bool. These must match exactly or any !p2p build caller will fail to compile. Even if no caller currently hits this code path in non-p2p builds, mismatched stubs are fragile and break the compiler contract.
Fix:
func ServiceDiscoverer(ctx context.Context, n *node.Node, token, servicesID string,
discoveryFunc func(serviceID string, node schema.NodeData), allocate bool) error {
return fmt.Errorf("not implemented")
}4. Stale commented-out code carried over into federation.go (lines ~60–66)
// Retrieve current ID for ip in the blockchain
//_, found := ledger.GetKey(protocol.UsersLedgerKey, node.Host().ID().String())
// If mismatch, update the blockchain
//if !found {
...
// }These dead comment blocks were copied verbatim from the old Proxy function. The refactor is a good opportunity to clean them up. Either delete them or, if the conditional logic is intended, restore the actual code and track it as a TODO.
5. handleConn leaks conn on upstream dial failure
tunnelConn, err := net.Dial("tcp", tunnelAddr)
if err != nil {
log.Error().Err(err).Msg("Error connecting to tunnel")
return // ← conn is never closed
}When the dial fails, the inbound conn is never closed, leaking a file descriptor on every failed upstream connection.
Fix: Add conn.Close() before returning in the error path.
🟢 Positive Observations
- Architecture is correct. The split between
federatedServer.go(no tag — struct only),federation.go(p2ptag — impl), andp2p_disabled.go(!p2ptag — stub) follows the established pattern in this package precisely. cli/federated.gois now a clean 3-line delegate — this is the right call and makes the CLI layer trivially testable.copyStreamconsolidation is correct. Removing it fromp2p.goand defining it once infederation.go(same build tag, same package) is clean;allocateLocalServiceinp2p.gocallscopyStreamand both files share thep2ptag, so the symbol is visible at compile time.handleConnextraction improves readability and opens the door to unit testing the routing logic independently.
Summary
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Blocker | federation.go:34 |
ServiceDiscoverer called with 5 args, needs 6 — won't compile under p2p tag |
| 2 | 🔴 Blocker | federation.go:97 |
Panic when tunnelAddresses is empty |
| 3 | 🟡 Should Fix | p2p_disabled.go |
ServiceDiscoverer stub signature doesn't match real function |
| 4 | 🟡 Should Fix | federation.go:60–66 |
Dead commented-out code should be removed |
| 5 | 🟡 Should Fix | federation.go:93 |
conn leaked on upstream dial failure |
chay2199
left a comment
chay2199
left a comment
There was a problem hiding this comment.
Code Review: refactor — move federated server logic to its own service
Overall: The structural intent is sound — extracting the federated proxy into its own p2p.FederatedServer type is a clear improvement over the CLI carrying that logic. However, the PR introduces one critical compilation error and a latent panic, and leaves a stub signature inconsistency that will quietly rot.
🔴 Critical — Compilation Error
federation.go:27 — Wrong arity calling ServiceDiscoverer
// federation.go (new, p2p-tagged)
if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil); err != nil {The actual signature in p2p.go (also p2p-tagged) is:
func ServiceDiscoverer(
ctx context.Context,
n *node.Node,
token, servicesID string,
discoveryFunc func(serviceID string, node schema.NodeData),
allocate bool, // ← missing from the call site
) errorThe call passes 5 arguments but the function requires 6. This will fail to compile with the p2p build tag. The allocate flag likely should be false here since the federated server is proxying outbound, not allocating a local tunnel.
Fix:
if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil, false); err != nil {🟠 Bug — Panic on Empty Node List
federation.go:92 — Unguarded index into potentially empty slice
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]If no nodes are online, tunnelAddresses will be empty and rand.IntN(0) panics. This was a pre-existing problem in the old federated.go closure, but migrating it into a named function is a good opportunity to fix it rather than carry the bug forward.
Fix:
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel addresses available")
conn.Close()
return
}
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]🟡 Maintenance Hazard — Stub Signature Divergence in p2p_disabled.go
The !p2p stub still uses the old ServiceDiscoverer signature:
// p2p_disabled.go
func ServiceDiscoverer(ctx context.Context, node *node.Node, token, servicesID string, fn func()) error {The real implementation now takes discoveryFunc func(serviceID string, node schema.NodeData), allocate bool. These compile in mutually exclusive build variants so there's no immediate error — but any future caller in non-build-tagged code would be forced to pick one signature and silently break the other build. Please keep the stub in sync:
func ServiceDiscoverer(ctx context.Context, node *node.Node, token, servicesID string, discoveryFunc func(string, schema.NodeData), allocate bool) error {
return fmt.Errorf("not implemented")
}🟡 Design Note — handleConn Has No Context Awareness
handleConn(conn net.Conn) is a detached goroutine with no way to be interrupted if the parent ctx is cancelled (e.g. on shutdown). Since this is now a named function it's worth threading the context through:
func handleConn(ctx context.Context, conn net.Conn) { ... }And use net.Dialer.DialContext instead of net.Dial so the outbound connection respects cancellation.
✅ What's Done Well
- Build tag separation is correct:
federatedServer.gocarries the struct (no tag, always compiled),federation.gocarries thep2p-only implementation, andp2p_disabled.gocarries the stub — idiomatic Go for conditional compilation. - Error message fix: the second error in
Startnow correctly says"starting a new node"instead of the copy-paste"creating a new node". copyStreamremoval fromp2p.gois safe — it's now defined infederation.go(same package, same build tag), soproxyP2PConnectioninp2p.gowill still resolve it at compile time.- The
cli/federated.gosimplification from ~120 lines to 3 is exactly what a good refactor looks like.
Summary
| Severity | File | Issue |
|---|---|---|
| 🔴 Critical | core/p2p/federation.go:27 |
ServiceDiscoverer called with 5 args, needs 6 — compile error |
| 🟠 Bug | core/p2p/federation.go:92 |
Panic when no online nodes (rand.IntN(0)) |
| 🟡 Maintenance | core/p2p/p2p_disabled.go |
ServiceDiscoverer stub has stale/mismatched signature |
| 🟡 Design | core/p2p/federation.go |
handleConn ignores context, cannot be cleanly cancelled on shutdown |
The compilation error must be fixed before merging. The panic fix is strongly recommended given this is network-facing connection-handling code.
chay2199
left a comment
chay2199
left a comment
There was a problem hiding this comment.
Code Review: "refactor: move federated server logic to its own service"
The overall direction is solid — extracting the inline Proxy/copyStream logic from cli/federated.go into a proper FederatedServer type in core/p2p follows a cleaner separation of concerns, and the build-tag discipline (//go:build p2p on federation.go, stub in p2p_disabled.go) is the right approach. However, there are two compile-breaking issues and a panic that must be fixed before merge.
🔴 Critical: ServiceDiscoverer argument mismatch — compile error under p2p tag
In federation.go, the call is:
if err := ServiceDiscoverer(ctx, n, fs.p2ptoken, FederatedID, nil); err != nil {That passes 5 arguments. But the actual signature in p2p.go (unchanged by this PR) is:
func ServiceDiscoverer(
ctx context.Context,
n *node.Node,
token, servicesID string,
discoveryFunc func(serviceID string, node schema.NodeData),
allocate bool, // ← 6th parameter, omitted in call
) errorThis will fail to compile under the p2p build tag. The missing allocate bool argument needs to be passed (likely false for the federated server use case, since it only discovers, it doesn't allocate).
🔴 Critical: p2p_disabled.go stub signature doesn't match the real function
The newly added stub in p2p_disabled.go:
func ServiceDiscoverer(ctx context.Context, node *node.Node, token, servicesID string, fn func()) error {has a different signature from the real implementation — wrong parameter count (5 vs 6), and fn func() instead of discoveryFunc func(serviceID string, node schema.NodeData). Any code compiled without the p2p tag that calls ServiceDiscoverer with the real signature will fail. The stub must exactly mirror the real signature:
func ServiceDiscoverer(ctx context.Context, n *node.Node, token, servicesID string, discoveryFunc func(serviceID string, node schema.NodeData), allocate bool) error {
return fmt.Errorf("not implemented")
}🟠 High: Panic when no nodes are online in handleConn
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]If all known nodes are offline, tunnelAddresses is empty and rand.IntN(0) panics. This code path is reachable in normal operation (e.g. transient network issues). Add a guard:
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}This was also present in the original Proxy goroutine — the refactor is a good opportunity to fix this pre-existing bug.
🟡 Medium: handleConn cannot respect context cancellation
go handleConn(conn)handleConn has no context.Context parameter. Long-lived proxy connections (the two copyStream goroutines) will outlive any context cancellation — e.g. a graceful shutdown signal. The signature should become handleConn(ctx context.Context, conn net.Conn), and the blocking <-closer should be wrapped with a select on ctx.Done().
🟡 Medium: CLI context discarded — context.Background() used instead
In federated.go:
func (f *FederatedCLI) Run(ctx *cliContext.Context) error {
fs := p2p.NewFederatedServer(f.Address, p2p.FederatedID, f.Peer2PeerToken)
return fs.Start(context.Background()) // ← ignores the CLI context
}The cliContext.Context should carry cancellation signals (SIGINT, etc.). fs.Start should receive a properly derived context from the CLI context, not a fresh background one. This was a bug in the original code and should be fixed here.
🟢 Minor: Leftover commented-out code
federation.go carries over several stale comments from the original Proxy function:
//_, found := ledger.GetKey(protocol.UsersLedgerKey, node.Host().ID().String())
// If mismatch, update the blockchain
//if !found {
// }These should be removed or converted to a documented // TODO.
🟢 Minor: fmt.Println for error logging
fmt.Println("Error accepting: ", err.Error())The surrounding code uses zerolog. This should be:
log.Error().Err(err).Msg("Error accepting connection")✅ What's done well
- The struct/method split across
federatedServer.go(no build tag, struct only) +federation.go(p2ptag, implementation) +p2p_disabled.gostub is the correct pattern for conditional compilation in this codebase. - The error message fix (
"starting a new node"vs the copy-paste"creating a new node") on then.Starterror path is a good catch. - Moving
copyStreamfromp2p.gotofederation.goscopes it to where it's actually used.
Bottom line: The two compile errors (argument count mismatch + stub signature) and the handleConn panic must be fixed before merge. The context propagation issue should also be addressed since this PR already touches that code path.
chay2199
left a comment
chay2199
left a comment
There was a problem hiding this comment.
Code Review: Federated Server Refactor
Overall this is a clean, well-motivated refactor. Moving the Proxy / copyStream logic out of the CLI layer and into a proper FederatedServer service type in the p2p package makes the separation of concerns much cleaner, and the //go:build p2p / !p2p stub pattern is used correctly. A few issues worth addressing before merge:
🔴 Potential Panic — empty tunnelAddresses slice
core/p2p/federation.go, handleConn()
tunnelAddr := tunnelAddresses[rand.IntN(len(tunnelAddresses))]If no peers are currently online, tunnelAddresses will be empty and rand.IntN(0) panics. This was present in the original cli/federated.go code but this refactor is a good opportunity to fix it:
if len(tunnelAddresses) == 0 {
log.Error().Msg("No online tunnel nodes available")
conn.Close()
return
}🔴 Ignored error — potential nil-pointer dereference on ledger
core/p2p/federation.go, proxy()
ledger, _ := node.Ledger()The error from node.Ledger() is silently discarded. If Ledger() returns nil, err, the subsequent ledger.Announce(...) call will panic. The same pattern exists in p2p.go's nodeAnnounce / proxyP2PConnection, but those callers have been present longer. Suggest:
ledger, err := node.Ledger()
if err != nil {
log.Error().Err(err).Msg("Failed to get ledger")
return err
}🟡 handleConn hardcodes FederatedID — coupling concern
core/p2p/federation.go
func handleConn(conn net.Conn) {
for _, v := range GetAvailableNodes(FederatedID) {handleConn is a free function that reaches directly for the package-level FederatedID constant. Since FederatedServer already carries fs.service, converting this to a method would decouple it and make the service ID explicit:
func (fs *FederatedServer) handleConn(conn net.Conn) {
for _, v := range GetAvailableNodes(fs.service) {The goroutine spawn in proxy would become go fs.handleConn(conn). This also makes the code easier to test in isolation.
🟡 fmt.Println instead of structured logging
core/p2p/federation.go, proxy()
fmt.Println("Error accepting: ", err.Error())The rest of the file (and codebase) uses zerolog. This line should be:
log.Error().Err(err).Msg("Error accepting connection")🟢 Minor — p2ptoken field name inconsistency
In federatedServer.go, the constructor parameter is p2pToken (camelCase with capital T) but the struct field is p2ptoken (all lowercase). While not a bug, making the field p2pToken would be more idiomatic and consistent with the parameter name.
✅ Positives
cli/federated.gois now beautifully simple — 3 lines of logic instead of 100+.- The build-tag pattern (
federation.gofor the real impl, stub inp2p_disabled.go) is correct and consistent with the rest of the package. context.Background()in the oldn.Start()call is replaced with the passedctxinfs.Start()— a real correctness improvement for context cancellation propagation.copyStreammoving fromp2p.gotofederation.gois correct; both are//go:build p2p, soproxyP2PConnectioninp2p.gocan still resolve it within the same package build.
3 participants
No description provided.