Skip to content

fix(zetaclient): snapshot telemetry peer data #4589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alan747271363-art wants to merge 3 commits into
base: main
Choose a base branch
from
+80 −9
Open

fix(zetaclient): snapshot telemetry peer data #4589

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8abeda6
fix(zetaclient): snapshot telemetry peer data
alan747271363-art May 13, 2026
e44a59a
fix(zetaclient): isolate telemetry peer snapshots
alan747271363-art May 13, 2026
2f88a0e
fix(zetaclient): deep copy telemetry peer addresses
alan747271363-art May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions zetaclient/metrics/telemetry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,37 @@ func NewTelemetryServer() *TelemetryServer {
func (t *TelemetryServer) SetPingRTT(rtt map[peer.ID]int64) {
t.mu.Lock()
defer t.mu.Unlock()
t.rtt = rtt
t.rtt = copyPingRTT(rtt)
}

func (t *TelemetryServer) GetPingRTT() map[peer.ID]int64 {
t.mu.Lock()
defer t.mu.Unlock()
return t.rtt
return copyPingRTT(t.rtt)
}

func (t *TelemetryServer) SetConnectedPeers(peers []peer.AddrInfo) {
t.mu.Lock()
defer t.mu.Unlock()
t.connectedPeers = peers
t.connectedPeers = copyConnectedPeers(peers)
}

func (t *TelemetryServer) GetConnectedPeers() []peer.AddrInfo {
t.mu.Lock()
defer t.mu.Unlock()
return t.connectedPeers
return copyConnectedPeers(t.connectedPeers)
}

func copyPingRTT(rtt map[peer.ID]int64) map[peer.ID]int64 {
rttCopy := make(map[peer.ID]int64, len(rtt))
for peerID, duration := range rtt {
rttCopy[peerID] = duration
}
return rttCopy
}

func copyConnectedPeers(peers []peer.AddrInfo) []peer.AddrInfo {
return append([]peer.AddrInfo(nil), peers...)
}
Comment on lines +92 to 106

@greptile-apps greptile-apps Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 peer.AddrInfo has an Addrs []Multiaddr slice field that is not deep-copied here. append([]peer.AddrInfo(nil), peers...) copies each struct by value, so ID is isolated correctly, but the Addrs slice header in every copied element still shares the same backing array as the original. If any caller replaces an element in snapshot[i].Addrs, that change writes through to the stored slice. For the current HTTP-handler read pattern this is harmless, but it leaves the isolation guarantee incomplete compared to the intent of the rest of this PR.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: zetaclient/metrics/telemetry.go
Line: 91-93

Comment:
`peer.AddrInfo` has an `Addrs []Multiaddr` slice field that is not deep-copied here. `append([]peer.AddrInfo(nil), peers...)` copies each struct by value, so `ID` is isolated correctly, but the `Addrs` slice header in every copied element still shares the same backing array as the original. If any caller replaces an element in `snapshot[i].Addrs`, that change writes through to the stored slice. For the current HTTP-handler read pattern this is harmless, but it leaves the isolation guarantee incomplete compared to the intent of the rest of this PR.

How can I resolve this? If you propose a fix, please make it concise.

@alan747271363-art alan747271363-art May 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2f88a0e0: copyConnectedPeers now allocates a new peer.AddrInfo slice and deep-copies each non-nil Addrs slice, so the stored telemetry snapshot no longer shares address slice backing arrays with callers.


// SetP2PID sets p2pid
Expand Down
40 changes: 40 additions & 0 deletions zetaclient/metrics/telemetry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package metrics

import (
"testing"

"github.com/libp2p/go-libp2p/core/peer"
"github.com/stretchr/testify/require"
)

func TestTelemetryServerCopiesPingRTT(t *testing.T) {
telemetry := NewTelemetryServer()
peerID := peer.ID("peer-a")

rtt := map[peer.ID]int64{peerID: 10}
telemetry.SetPingRTT(rtt)

rtt[peerID] = 20
require.Equal(t, int64(10), telemetry.GetPingRTT()[peerID])

snapshot := telemetry.GetPingRTT()
snapshot[peerID] = 30
require.Equal(t, int64(10), telemetry.GetPingRTT()[peerID])
}

func TestTelemetryServerCopiesConnectedPeers(t *testing.T) {
telemetry := NewTelemetryServer()
peerA := peer.ID("peer-a")
peerB := peer.ID("peer-b")
peerC := peer.ID("peer-c")

peers := []peer.AddrInfo{{ID: peerA}}
telemetry.SetConnectedPeers(peers)

peers[0].ID = peerB
require.Equal(t, peerA, telemetry.GetConnectedPeers()[0].ID)

snapshot := telemetry.GetConnectedPeers()
snapshot[0].ID = peerC
require.Equal(t, peerA, telemetry.GetConnectedPeers()[0].ID)
}
6 changes: 3 additions & 3 deletions zetaclient/tss/healthcheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ func HealthcheckWorker(ctx context.Context, server *tss.Server, p HealthcheckPro

// Ping & collect round trip time
var (
host = server.GetP2PHost()
pingRTT = make(map[peer.ID]int64)
mu = sync.Mutex{}
host = server.GetP2PHost()
mu = sync.Mutex{}
)

@greptile-apps greptile-apps Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 mu is now only used inside pinger to guard concurrent goroutine writes to the per-invocation pingRTT map. Since pingRTT is local to each closure call, mu no longer needs to live in the outer scope — moving it inside pinger would make the ownership clear and avoid the appearance that it protects shared state across ticks. If the ticker ever ran overlapping invocations, the shared mu would also impose false contention between two independent ping rounds.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: zetaclient/tss/healthcheck.go
Line: 44-48

Comment:
`mu` is now only used inside `pinger` to guard concurrent goroutine writes to the per-invocation `pingRTT` map. Since `pingRTT` is local to each closure call, `mu` no longer needs to live in the outer scope — moving it inside `pinger` would make the ownership clear and avoid the appearance that it protects shared state across ticks. If the ticker ever ran overlapping invocations, the shared `mu` would also impose false contention between two independent ping rounds.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@alan747271363-art alan747271363-art May 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already addressed on the current branch: mu := sync.Mutex{} is now created inside the pinger function body next to the per-invocation pingRTT map, so independent ping rounds do not share the mutex.


const pingTimeout = 5 * time.Second

pinger := func(ctx context.Context, _ *ticker.Ticker) error {
pingRTT := make(map[peer.ID]int64)
var wg sync.WaitGroup
for _, peerID := range p.WhitelistPeers {
if peerID == host.ID() {
Expand Down
Loading