[Bug]: `RemoveHost` leaks orphan `nodes` rows when `host.Nodes` cache drifts, breaking peer lists

Contact Details

[email protected]

What happened?

Summary

logic.RemoveHost deletes nodes by iterating the denormalized Host.Nodes cache (schema/hosts.go, datatypes.JSONSlice[string]) instead of querying the nodes table by HostID. If a node row exists with HostID == host.ID but its ID is not in host.Nodes, the node is left behind as an orphan after the host is deleted. The orphan stays visible in peer pulls. Other peers see its address mapped to a dead public key, DNS resolves to it, and netclient ping shows it as connected=false.

?force=true does not help. The flag only bypasses the "host still has associated nodes" guard. It does not widen the deletion sweep.

Where

logic/hosts.go:323, RemoveHost:

func RemoveHost(h *schema.Host, forceDelete bool) error {
    if !forceDelete && len(h.Nodes) > 0 {
        return fmt.Errorf("host still has associated nodes")
    }
    if len(h.Nodes) > 0 {
        if err := DisassociateAllNodesFromHost(h.ID.String()); err != nil {
            return err
        }
    }
    err := h.Delete(db.WithContext(context.TODO()))
    ...
}

DisassociateAllNodesFromHost (logic/hosts.go:427) also iterates host.Nodes, so it inherits the same blind spot.

The authoritative relationship is nodes.HostID. Host.Nodes is a cache. Trusting the cache as authoritative is the bug.

How host.Nodes drifts from the nodes table

1. Race between enroll and delete. AssociateNodeToHost (logic/hosts.go:378) reads the host, creates a node row, then UpsertHost to append the node ID. RemoveHost reads the host, walks its Nodes snapshot, then deletes the host. With no transaction across either path, this interleaving is reachable:

   • T1 (delete): reads Host, sees Nodes=[N_old]
   • T2 (enroll): reads Host, creates node row N_new with HostID=H, appends and upserts Nodes=[N_old, N_new]
   • T1: walks [N_old], deletes N_old, calls h.Delete()
   • End state: N_new row exists in nodes table, host row gone, peer pulls still surface N_new's address.

2. Partial failure in DisassociateAllNodesFromHost. At logic/hosts.go:452 it writes host.Nodes = failedNodes and UpsertHost(host) before RemoveHost proceeds to h.Delete(). If anything between those crashes the server, the host vanishes but failed node rows remain.

Reproduction

1. Enroll a host on network N, get its host ID H.
2. Loop: netclient leave N then netclient join on the same machine, keeping the netclient state directory so the private key and node ID persist. On each iteration, immediately issue DELETE /api/hosts/{H}?force=true from another shell so the two are concurrent.
3. After a few iterations, query the nodes table directly: SELECT id, host_id, address FROM nodes WHERE network = 'N';. You will see node rows whose host_id no longer corresponds to any row in hosts.
4. From any other peer on N, netclient ping lists the orphan with CONNECTED=false, and *.nm.internal DNS for the original hostname resolves to the orphan's VPN address.

We hit this in production via a slower path: a Docker volume that preserved netclient state across what was supposed to be a clean reinstall (so the private key and node identity persisted across re-enroll), then a UI delete that completed concurrent with the host reconnecting on boot. End state: two node rows for the same hostname, only one in host.Nodes. The host got deleted, the other became an orphan visible to every peer. The UI's "Force delete" button only cleared it because we ended up hitting DELETE /api/nodes/{network}/{node_id}?force=true directly, bypassing the host path entirely.

Suggested fix

In RemoveHost, sweep by HostID instead of (or in addition to) walking host.Nodes:

func RemoveHost(h *schema.Host, forceDelete bool) error {
    allNodes, err := GetAllNodes()
    if err != nil {
        return err
    }
    var related []models.Node
    for i := range allNodes {
        if allNodes[i].HostID == h.ID {
            related = append(related, allNodes[i])
        }
    }
    if !forceDelete && len(related) > 0 {
        return fmt.Errorf("host still has associated nodes")
    }
    for i := range related {
        cleanupNodeReferences(&related[i])
        if err := DeleteNodeByID(&related[i]); err != nil {
            slog.Error("RemoveHost: failed to delete node", "node", related[i].ID, "host", h.ID, "error", err)
        }
    }
    return h.Delete(db.WithContext(context.TODO()))
}

This makes deletion idempotent against host.Nodes drift, closes the enroll vs delete race window (the read happens after the new node is committed or not at all), and folds DisassociateAllNodesFromHost's job into the same authoritative pass. The forceDelete flag retains its original meaning, a guard rather than a deletion driver.

A complementary cleanup would be a periodic sweep that deletes node rows whose HostID does not match any row in hosts. Useful as a backstop for orphans created before this fix lands.

Why ?force=true is not enough today

?force=true short-circuits the "host still has associated nodes" check and proceeds through DisassociateAllNodesFromHost. But that function also iterates host.Nodes, so the "force" and "non-force" paths share the same read source. Widening the read source is what fixes it.

Impact

• Peer lists across the mesh include addresses pointing at dead public keys.
• DNS (*.nm.internal) resolves to those orphan addresses, so traffic to the original hostname black-holes.
• No obvious UI surface corresponds to "orphan node row". The host is gone, so delete-by-host cannot reach them. Manual cleanup requires DB access or hitting DELETE /api/nodes/{network}/{node_id}?force=true directly.

Version

v1.5.1

What OS are you using?

Linux

Relevant log output

Contributing guidelines

• Yes, I did.