diff --git a/internal/servicediscovery/service.go b/internal/servicediscovery/service.go index a810989be..a3f181795 100644 --- a/internal/servicediscovery/service.go +++ b/internal/servicediscovery/service.go @@ -3035,6 +3035,15 @@ func (s *Service) GetDiscoveryByResource(resourceType ResourceType, targetID, re ResourceID: resourceID, } aliasIDs := []string{MakeResourceID(resourceType, targetID, resourceID)} + // A forked host identity is not reachable through normalizeDiscoveryRequest, + // which only matches exact snapshot IDs and hostnames. Seed the equivalent + // spellings of the target so records stored under a fork still resolve. + for _, candidate := range s.equivalentDiscoveryTargetIDs(targetID) { + aliasIDs = append(aliasIDs, MakeResourceID(resourceType, candidate, candidate)) + if resourceID != "" && resourceID != candidate { + aliasIDs = append(aliasIDs, MakeResourceID(resourceType, candidate, resourceID)) + } + } req = s.normalizeDiscoveryRequest(req, &aliasIDs) d, err := s.store.GetByResource(resourceType, req.TargetID, req.ResourceID) @@ -3091,11 +3100,38 @@ func (s *Service) ListDiscoveriesByType(resourceType ResourceType) ([]*ResourceD } // ListDiscoveriesByTarget returns discoveries for a specific target ID. +// +// The store matches TargetID byte-for-byte, but callers legitimately hold a +// different spelling of the same target than the record was stored under: a PVE +// node reports its linked agent by base agent UUID while the host record itself +// may have forked onto a suffixed identity. Resolve the request through +// equivalentDiscoveryTargetIDs so those spellings still find their records. func (s *Service) ListDiscoveriesByTarget(targetID string) ([]*ResourceDiscovery, error) { - discoveries, err := s.store.ListByTarget(targetID) + candidates := s.equivalentDiscoveryTargetIDs(targetID) + if len(candidates) == 0 { + return nil, nil + } + + all, err := s.store.List() if err != nil { return nil, fmt.Errorf("list discoveries by target %q: %w", targetID, err) } + + wanted := make(map[string]struct{}, len(candidates)) + for _, candidate := range candidates { + wanted[candidate] = struct{}{} + } + + discoveries := make([]*ResourceDiscovery, 0, len(all)) + for _, d := range all { + if d == nil { + continue + } + if _, ok := wanted[strings.TrimSpace(d.TargetID)]; ok { + discoveries = append(discoveries, d) + } + } + discoveries = s.deduplicateDiscoveries(discoveries) for _, d := range discoveries { s.upgradeCLIAccessIfNeeded(d) @@ -3103,6 +3139,104 @@ func (s *Service) ListDiscoveriesByTarget(targetID string) ([]*ResourceDiscovery return discoveries, nil } +// equivalentDiscoveryTargetIDs returns every target ID that identifies the same +// target as targetID, starting with targetID itself. +// +// Host identities fork onto a "-" form when an agent re-enrolls +// under a new token against an existing record (see the host token binding logic +// in internal/monitoring) - re-running an install with --proxmox is enough to +// trigger it. Discovery then stores records under the forked host ID while the +// PVE node still reports the base agent UUID as its linked agent, so a lookup by +// base UUID finds nothing. Bridge the two through the live snapshot. +// +// Expansion is limited to spellings that provably name the requested target: a +// matched node's linked agent ID and name, a matched host's own ID/hostname +// pairing, and hex fork suffixes of any of those. Hostname equality is never +// followed to a different host's ID - multi-estate deployments legitimately +// reuse node names and hostnames, and a transitive hop would surface another +// estate's records here, in discovery readiness, and in AI context. +func (s *Service) equivalentDiscoveryTargetIDs(targetID string) []string { + trimmed := strings.TrimSpace(targetID) + if trimmed == "" { + return nil + } + + ids := []string{trimmed} + seen := map[string]struct{}{trimmed: {}} + add := func(id string) { + id = strings.TrimSpace(id) + if id == "" { + return + } + if _, ok := seen[id]; ok { + return + } + seen[id] = struct{}{} + ids = append(ids, id) + } + + snap, ok := s.getSnapshot() + if !ok { + return ids + } + + for _, node := range snap.Nodes { + if node.LinkedAgentID == trimmed || node.ID == trimmed || node.Name == trimmed { + add(node.LinkedAgentID) + add(node.Name) + } + } + for _, host := range snap.Hosts { + switch { + case host.ID == trimmed: + add(host.Hostname) + case host.Hostname != "" && host.Hostname == trimmed: + add(host.ID) + } + } + + // A fork suffix can hang off any spelling collected above, not just the + // requested one: a node-name lookup reaches the fork through the node's + // linked agent UUID. + direct := ids + for _, host := range snap.Hosts { + for _, candidate := range direct { + if isForkedHostIdentity(host.ID, candidate) { + add(host.ID) + break + } + } + } + + return ids +} + +// isForkedHostIdentity reports whether candidate is base carrying one or more +// host-identity fork suffixes. Forks append "-", and a base longer than 40 +// characters is truncated before the next suffix is appended, so a twice-forked +// identity carries a partial suffix followed by a full one. Requiring every +// trailing segment to be hex keeps unrelated IDs that merely share a prefix out. +func isForkedHostIdentity(candidate, base string) bool { + if candidate == "" || base == "" || candidate == base { + return false + } + tail, ok := strings.CutPrefix(candidate, base+"-") + if !ok { + return false + } + for _, segment := range strings.Split(tail, "-") { + if segment == "" { + return false + } + for _, r := range segment { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') { + return false + } + } + } + return true +} + // deduplicateDiscoveries filters out redundant discoveries where a PVE node // is represented by both its Node Name and its Linked Host Agent ID. // The Host Agent ID is preferred. diff --git a/internal/servicediscovery/service_test.go b/internal/servicediscovery/service_test.go index 33478c13c..90a12b1f6 100644 --- a/internal/servicediscovery/service_test.go +++ b/internal/servicediscovery/service_test.go @@ -2455,3 +2455,215 @@ func TestService_DiscoverResource_ReturnsUpgradedCachedDiscovery(t *testing.T) { t.Fatalf("expected cleaned AI reasoning without legacy URL note, got %q", got.AIReasoning) } } + +// Regression: a host identity that forked onto a "-" spelling (an +// agent re-enrolling under a new token against an existing record) stored its +// discovery under the forked host ID, while the PVE node kept reporting the base +// agent UUID as its linked agent. The store matches TargetID byte-for-byte, so +// the resource drawer's list-by-agent call returned an empty 200 and the +// Discovery tab reported "no saved discovery run" for a host it had analyzed. +func TestService_ListDiscoveriesByTarget_ResolvesForkedHostIdentity(t *testing.T) { + const baseAgentID = "342f337b-d2a9-4316-a998-d09a2abe3e8f" + const forkedAgentID = baseAgentID + "-bffd0339" + + store, err := NewStore(t.TempDir()) + if err != nil { + t.Fatalf("NewStore error: %v", err) + } + store.crypto = nil + + service := NewService(store, nil, DefaultConfig()) + service.SetReadState(readStateFromSnapshot(StateSnapshot{ + Hosts: []Host{{ID: forkedAgentID, Hostname: "nova.biz", Status: "online"}}, + Nodes: []Node{{ID: "node-nova", Name: "nova", LinkedAgentID: baseAgentID}}, + })) + + discovery := &ResourceDiscovery{ + ID: MakeResourceID(ResourceTypeAgent, forkedAgentID, forkedAgentID), + ResourceType: ResourceTypeAgent, + TargetID: forkedAgentID, + ResourceID: forkedAgentID, + Hostname: "nova.biz", + ServiceType: "proxmox", + } + if err := store.Save(discovery); err != nil { + t.Fatalf("Save error: %v", err) + } + + got, err := service.ListDiscoveriesByTarget(baseAgentID) + if err != nil { + t.Fatalf("ListDiscoveriesByTarget error: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 discovery for base agent ID, got %d", len(got)) + } + if got[0].TargetID != forkedAgentID { + t.Fatalf("TargetID = %q, want %q", got[0].TargetID, forkedAgentID) + } + + // The forked spelling must keep working, as must the single-record path. + if direct, err := service.ListDiscoveriesByTarget(forkedAgentID); err != nil || len(direct) != 1 { + t.Fatalf("ListDiscoveriesByTarget(forked) = %d discoveries, err %v; want 1, nil", len(direct), err) + } + byResource, err := service.GetDiscoveryByResource(ResourceTypeAgent, baseAgentID, baseAgentID) + if err != nil { + t.Fatalf("GetDiscoveryByResource error: %v", err) + } + if byResource == nil || byResource.TargetID != forkedAgentID { + t.Fatalf("GetDiscoveryByResource returned %#v, want the forked record", byResource) + } +} + +// A base longer than 40 characters is truncated before the next fork suffix is +// appended, so a twice-forked identity carries a partial suffix then a full one. +func TestService_ListDiscoveriesByTarget_ResolvesTwiceForkedHostIdentity(t *testing.T) { + const baseAgentID = "ea79df24-0d3b-453f-8ce0-cc08e2b96f86" + const forkedAgentID = baseAgentID + "-d3b-83adbde6" + + store, err := NewStore(t.TempDir()) + if err != nil { + t.Fatalf("NewStore error: %v", err) + } + store.crypto = nil + + service := NewService(store, nil, DefaultConfig()) + service.SetReadState(readStateFromSnapshot(StateSnapshot{ + Hosts: []Host{{ID: forkedAgentID, Hostname: "luna.vp.diskiller.net", Status: "online"}}, + })) + + if err := store.Save(&ResourceDiscovery{ + ID: MakeResourceID(ResourceTypeAgent, forkedAgentID, forkedAgentID), + ResourceType: ResourceTypeAgent, + TargetID: forkedAgentID, + ResourceID: forkedAgentID, + Hostname: "luna.vp.diskiller.net", + ServiceType: "proxmox", + }); err != nil { + t.Fatalf("Save error: %v", err) + } + + got, err := service.ListDiscoveriesByTarget(baseAgentID) + if err != nil { + t.Fatalf("ListDiscoveriesByTarget error: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 discovery for twice-forked identity, got %d", len(got)) + } +} + +// Target expansion must not sweep in unrelated hosts that merely share a prefix, +// and must stay quiet when no snapshot is available. +func TestService_EquivalentDiscoveryTargetIDs_Bounds(t *testing.T) { + const baseAgentID = "342f337b-d2a9-4316-a998-d09a2abe3e8f" + + service := NewService(nil, nil, DefaultConfig()) + if got := service.equivalentDiscoveryTargetIDs(baseAgentID); len(got) != 1 || got[0] != baseAgentID { + t.Fatalf("without a snapshot, expected only the requested ID, got %#v", got) + } + if got := service.equivalentDiscoveryTargetIDs(" "); got != nil { + t.Fatalf("expected nil for a blank target, got %#v", got) + } + + service.SetReadState(readStateFromSnapshot(StateSnapshot{ + Hosts: []Host{ + {ID: baseAgentID + "-bffd0339", Hostname: "nova.biz", Status: "online"}, + {ID: baseAgentID + "-replica", Hostname: "clone.biz", Status: "online"}, + {ID: "unrelated-agent", Hostname: "other.biz", Status: "online"}, + }, + })) + + got := service.equivalentDiscoveryTargetIDs(baseAgentID) + for _, unwanted := range []string{baseAgentID + "-replica", "unrelated-agent"} { + for _, id := range got { + if id == unwanted { + t.Fatalf("expansion pulled in %q: %#v", unwanted, got) + } + } + } + var sawFork bool + for _, id := range got { + if id == baseAgentID+"-bffd0339" { + sawFork = true + } + } + if !sawFork { + t.Fatalf("expansion missed the hex-suffixed fork: %#v", got) + } +} + +func TestIsForkedHostIdentity(t *testing.T) { + const base = "342f337b-d2a9-4316-a998-d09a2abe3e8f" + tests := []struct { + candidate string + want bool + }{ + {base + "-bffd0339", true}, + {base + "-d3b-83adbde6", true}, + {base, false}, + {base + "-replica", false}, + {base + "-", false}, + {base + "-bffd0339-", false}, + {"other-" + base, false}, + {"", false}, + } + for _, tt := range tests { + if got := isForkedHostIdentity(tt.candidate, base); got != tt.want { + t.Errorf("isForkedHostIdentity(%q, base) = %v, want %v", tt.candidate, got, tt.want) + } + } +} + +// Multi-estate deployments legitimately run same-named nodes and hosts. A +// lookup by one estate's agent UUID must not hop through the shared name to the +// other estate's host ID and surface its records - the drawer, discovery +// readiness, and AI context would all show the wrong host's results. +func TestService_ListDiscoveriesByTarget_DoesNotBridgeSharedHostnames(t *testing.T) { + const agentA = "aaaaaaaa-1111-4316-a998-d09a2abe3e8f" + const agentB = "bbbbbbbb-2222-4316-a998-d09a2abe3e8f" + + store, err := NewStore(t.TempDir()) + if err != nil { + t.Fatalf("NewStore error: %v", err) + } + store.crypto = nil + + service := NewService(store, nil, DefaultConfig()) + service.SetReadState(readStateFromSnapshot(StateSnapshot{ + Hosts: []Host{ + {ID: agentA, Hostname: "pve1", Status: "online"}, + {ID: agentB, Hostname: "pve1", Status: "online"}, + }, + Nodes: []Node{ + {ID: "estate-a/pve1", Name: "pve1", LinkedAgentID: agentA}, + {ID: "estate-b/pve1", Name: "pve1", LinkedAgentID: agentB}, + }, + })) + + // Estate B has run discovery; estate A has not. + if err := store.Save(&ResourceDiscovery{ + ID: MakeResourceID(ResourceTypeAgent, agentB, agentB), + ResourceType: ResourceTypeAgent, + TargetID: agentB, + ResourceID: agentB, + Hostname: "pve1", + ServiceType: "proxmox", + }); err != nil { + t.Fatalf("Save error: %v", err) + } + + got, err := service.ListDiscoveriesByTarget(agentA) + if err != nil { + t.Fatalf("ListDiscoveriesByTarget error: %v", err) + } + if len(got) != 0 { + t.Fatalf("expected no discoveries for estate A, got %d (leaked across shared hostname)", len(got)) + } + + own, err := service.ListDiscoveriesByTarget(agentB) + if err != nil { + t.Fatalf("ListDiscoveriesByTarget(agentB) error: %v", err) + } + if len(own) != 1 { + t.Fatalf("expected estate B to keep finding its own record, got %d", len(own)) + } +}