fix(networkstate): a node it has not heard of yet is not a node that is absent

[MorningLightMountain713]

Three lookups on networkStateManager answered questions about the fleet before the node had ever asked what the fleet was, and the answer they gave was "absent".

The defect

search(), includes() and getRandomSocketAddress() each awaited waitIndexesReady. That getter is this.#controller.lock.waitReady() — the indexing lock. Before the first fetch has ever run the lock is free, so the await returns immediately and the lookup reads an empty index. Every node in the fleet comes back absent, and nothing distinguishes that from a node genuinely not being one.

The wait that means "I know the fleet" is a different one. waitIndexesReady is about not reading mid-rebuild; it says nothing about whether there has ever been anything to read.

How it was found

An integration suite caught a node reporting a legitimate peer as not being in the node list, 58 ms before it logged creating its first index:

Network State Indexes created, nodes found: 3
pubkeyIndexSize: 3, socketAddressSize: 3

The membership question had been answered from an index that did not yet exist. Callers that treat a falsy answer as "not a node" then act on it, and a node that has simply not read the fleet yet is indistinguishable from one that is not in it.

What that cost downstream: the peer refused the sync request, so the requesting node collected no completions and took the block-timer route to readiness instead — which is what that fallback is for. Nothing downstream needed changing. The only thing that was wrong was the answer that sent it down that path.

The fix

A node list has three conditions, not two, and the code modelled two:

• never fetched — cannot answer, must wait
• fetched and empty — can answer, and "absent" is the truth
• fetched and populated — answers from the index

What is tracked is the first fetch coming back, not the first population. Those differ exactly when the fleet is legitimately empty: a list that never populates would leave a lookup waiting for the life of the process, which trades a false answer for a hang. Suites that run with nodes: 0 reach that case.

#waitAnswerable() keeps the indexing wait on top of the new one. Mid-rebuild it costs ~10 ms and returns the newer state, which is why it was there; it simply was never the whole condition. Waiters are also released when the manager stops, where no fetch is coming and a waiter would otherwise never return.

A timeout was considered and rejected: bounding the wait reintroduces the false answer on a slow node and only changes when it happens.

Blast radius

These three lookups back getFluxnodesByPubkey, socketAddressInNetworkState, pubkeyInNetworkState, getRandomSocketAddress and getFluxnodeBySocketAddress on networkStateService, which are used across peering, messaging, app sync, port testing and availability checking. The isReady() docblock claimed every accessor on that service conflates unknown with empty; that is now true only of the bulk accessors, and it says so.

Testing

Each test was proven to fail without the fix. The two false-answer tests assert immediately against the old code; the empty-fleet test times out.

Unit: 4,873 passing / 18 pending / 0 failing on this branch, four of them new. eslint clean on all three changed files.

Integration, targeted: the nine suites this change most directly touches — 06-daemon-interaction and 12-ticker-control because they run with empty and shrinking node lists, 19-boundary-conditions because it is the suite that surfaced the defect, and 72, 73, 74, 75, 81, 82 because getRandomSocketAddress is how a node draws a peer for an image. All nine green, 92 tests, zero failures.

Integration, full gate: suites_pass=72 suites_fail=1 at 40a10a4ba. 19-boundary-conditions — the suite that surfaced this defect, and the one that failed the same gate on the same tree without this fix — passes. The single red is 60-volume-mount-ownership, which neither this branch nor #1778 touches and which is already fixed in #1781 higher up the stack: its probe stopped the app container before unmounting, and the stop's own die event drives a reconcile that remounts the volume, so the write it expects to be refused lands on a remounted one. It passes 8/8 run alone.

Worth stating plainly about that gate: it runs on #1778's tree with this fix applied, not on this branch. #1778's suite set is a strict superset of development's — the same 69 suites plus six volume ones, with nothing present only on development — and of the three files changed here, the manager and its unit test are byte-identical on both branches, while the service file differs only by a test-only addition #1778 makes in a different part of it. So the gate exercises every suite this branch would run and six more. It is not, however, literally a gate of this branch.

[Cabecinha84]

Verdict: safe to ACK. The defect is real, the fix is correct and minimal, and I reproduced both the bug and the fix
independently. There are three things worth raising — none of them blocking, one of them worth a two-line follow-up.

State of the PR right now: 1 commit, 3 files, +208/−12, no prior reviews or comments (this is a fresh, untouched PR),
MERGEABLE, zero commits behind development, CI green (build + GitGuardian).

The defect is genuine

search(), includes() and getRandomSocketAddress() awaited waitIndexesReady, which is #controller.lock.waitReady() —
the indexing lock. Before the first fetch that lock has never been taken, so the await resolves immediately and the
lookup reads an empty Map. Every peer comes back absent, and callers (fluxCommunicationUtils broadcast verification,
peer selection, port testing) act on that as "not a Fluxnode". The diagnosis in the PR body matches the code exactly.

What I verified independently

• The fix works, and the tests genuinely pin it. I swapped development's networkStateManager.js under the new test
  file: 3 of the 4 new tests fail (two assert answered === false and get true; the empty-fleet one times out at 2000ms).
  The fourth (stop releases waiters) passes on development because there is nothing to release there — which is exactly
  what the PR body claims, no overstatement.
• Unit suites are green. networkStateManager.test.js → 25 passing. The eight suites that consume these lookups
  (fluxCommunication, fluxCommunicationUtils, networkStateService, fluxNetworkHelper, availabilityChecker,
  networkHealthMonitor, nodeConfirmationService, placementFeasibility) → 371 passing, no hangs from the new wait.
• eslint is clean on all three changed files.
• A fetcher that throws does not hang callers: the .catch() inside the loop turns it into [], which marks answerable
  and the lookup returns false promptly. I confirmed this by probe.
• The unbounded wait is bounded in practice: daemonServiceUtils.executeCall uses timeout: 40_000, so the first fetch
  always settles. Worst case a lookup waits ~40s once, then never again. Mocha runs with --exit, so no lingering-timer
  risk in CI.

Findings

1. #answerable is never reset — a restarted instance re-exposes the exact bug (low, latent).
   stop() → #releaseWaiters() → #markAnswerable() sets the flag permanently, and reset() clears the indexes but not the
   flag. I confirmed by probe: start() → stop() → start() on the same instance, and a lookup before the second fetch
   returns false immediately — the fixed defect, back.

It is latent today only because networkStateService.stop() nulls stateManager and start() constructs a fresh one, so
production never restarts an instance. But nothing in the class says that, and the class is what the fix lives in. Two
lines in reset() would close it:

this.#answerable = false;
this.#answerableWait = new Promise((resolve) => { this.#onAnswerable = () => { resolve(); this.#onAnswerable = () =>
{}; }; });

This is the one I'd actually ask for, or take as an agreed follow-up.

2. A first fetch that comes back empty still answers "absent" (design gap worth naming).
   #markAnswerable() fires in the !state.length branch, so an empty first fetch makes the node answer "not a Fluxnode"
   for real peers. And the service-level fetcher flattens failure into emptiness — res.status === 'success' ? res.data :
   [] — so an RPC error is indistinguishable from a genuinely empty fleet. On a booting node whose fluxd is not yet
   ready, that is the common path, and the retry loop's own 15s sleep exists precisely because empty at boot means "ask
   again", not "the fleet is empty".

So the fix closes the fetch-in-flight window (~0.5s, which is what the integration gate caught) but not the
daemon-not-ready window. The tension is real and the PR is honest about half of it: waiting on empty would hang
harnesses running nodes: 0. The cleaner resolution — have the fetcher distinguish success-with-empty from failure, and
only mark answerable on the former — preserves the nodes: 0 case while closing the boot window, but reintroduces an
unbounded wait if the daemon errors indefinitely. Worth a comment; not something to hold the PR for, since it is
strictly no worse than today.

3. #onStartComplete() inside #releaseWaiters() isn't needed and slightly widens a pre-existing path (nit).
   Lookups wait on #answerableWait only, so resolving waitStarted does nothing for the stated purpose. What it does do:
   if stop() lands while the initial fetch is in flight and the loop then breaks on aborted without populating, start()
   sails past await this.waitStarted and installs a polling loop on a stopped manager. I checked whether this is a
   regression — it isn't: on development the same zombie loop already happens via the populate path (I measured the
   fetcher still being called 700ms after stop() on both branches). It's a narrow new race into an existing bad state.
   I'd just drop the line.

Nit: @type {() => void | null} on #onAnswerable parses as a function returning void|null; it wants {(() => void) |
null}. Matches the existing #onStartComplete style, so take it or leave it.

Side effects worth knowing about, but fine

nodeConfirmationService.poll() re-arms only after it finishes and now awaits getFluxnodeBySocketAddress, so its first
poll can be delayed by the first fetch. Bounded and self-correcting — this is exactly the caller class the waitStarted
docblock already warns about, and a delay is not a retirement. The isReady() docblock rewrite is accurate: the bulk
accessors (networkState(), nodeCount()) still conflate unknown with empty, and the three lookups no longer do. Callers
reaching these before networkStateService.start() still get false from the !stateManager guard — unchanged by this
PR, same class of gap as finding 2.

Recommendation

ACK. If you want one change first, make it finding 1 — resetting #answerable in reset() — since it is two lines and
keeps the class honest about its own contract. Findings 2 and 3 are fine as review comments or a follow-up.

One caveat you should weigh yourself: the full integration gate quoted in the PR body was run on #1778's tree, not on
this branch. The author states that plainly and the reasoning for why it's equivalent is sound, but it is not
literally a gate of this branch.

[MorningLightMountain713]

Thanks — all three are addressed, and the second one moved further than you proposed once I traced what actually reaches those lookups. Head is 618d01c31. Unit suite 4,883 passing / 18 pending / 0 failing, lint clean, and every change below has a test that fails without it.

1. #answerable is never reset — and neither was #started

Fixed in reset(), where you put it. I first argued for start() on the grounds that clearing it in reset() leaves a stopped manager waiting on a promise nothing resolves — but reset() has exactly one caller, stop(), and a stopped manager blocking is the same state a freshly built one is in. Returning the object to how it was built is what the method is for.

Checking that turned up a second field in the same position: #started was not rewound either, so a torn-down manager reported itself running and isReady() agreed. Both go now.

Four tests on the restart: the finding itself, all three lookups rather than one, that it rewinds on every stop rather than the first, and a control asserting an empty fleet still answers promptly rather than hanging — that one passes with and without the fix, deliberately, since it exists to catch the fix over-reaching.

2. An empty first fetch answers absent

You graded this worth a comment. Tracing every caller made it worth a change instead, and in a different place.

Two of the five sites are already gated on the daemon having answered — nodeConfirmationService.poll() returns before its lookup unless daemonConfirmed, which only comes from a successful getFluxNodeStatus(), and deterministicFluxList awaits readiness. The other three are inbound peer message handlers: verifySyncRequest, batchVerifyBroadcasts, verifyFluxBroadcast. Against an empty list those can only produce a rejection, which is what the gate run caught.

Then the part that changes the shape of it: inbound peers are only accepted once the node is confirmed, and confirmation requires a live daemon. So the case I was worried about — a node with a dead fluxd reporting real peers absent — cannot have peers connected to report to. And a daemon dying later doesn't reach it either: a failed fetch returns an empty list, and this.#state = state sits inside if (state.length), so the existing index is never wiped.

What is left is one window, and it is the one you measured 58ms of. Confirmation and the node list are different calls to the same daemon — one carrying a single record, one carrying every node — so the door opens on the first and we cannot check anyone until the second. On a live node holding 6091 nodes:

13:40:05.280  nodeConfirmationService - Confirmation gained
13:40:05.280  Now accepting peer connections
13:40:06.635  Network state fetch finished, elapsed: 1266.13 ms
13:40:06.671  Network State Indexes created, nodes found: 6091, elapsed: 35.76 ms

1,391 ms of accepting peers we could not validate. So allowConnections() now waits for the list as well as for confirmation. There is no point peering before we can check who is peering with us.

Only the first open waits — once the list is here isReady() is true and the callback runs inline, so regaining confirmation reconnects immediately. Existing peers are never dropped: the flag is read once, on a connection not yet added, and disconnectAll() is unchanged. A peer refused in that window is not penalised either — recordFailedConnection, which arms the 2/5/10/15-minute backoff, is only reached from the /flux/addoutgoingpeer HTTP path, never from a websocket close — so it simply returns on its next discovery pass.

Outbound needed nothing: fluxDiscovery dials using getRandomSocketAddress(), which has nothing to return while the list is empty.

Four tests, three red without it. The third covers the branch that only exists because of this: the list can arrive after confirmation was lost, and without a re-check the callback hands the door back to a node that just dropped every peer.

The flattening you identified is still there — res.status === 'success' ? res.data : [] in the service and the .catch(() => []) in the loop — and still cannot tell a failed fetch from an empty fleet. With the door shut until we are populated, nothing is asking during that window, so it no longer decides anything. Worth separating properly when nodeCount() and networkState() get the same treatment.

3. #onStartComplete() inside #releaseWaiters()

Your diagnosis is right and the line stays. Dropping it strands start(): it is parked on await this.waitStarted, and that release is the only thing that wakes it when a stop lands mid-fetch. Without it the promise never settles.

The fix belongs in start(), which now arms the refresh loop only for a manager that actually got its list. Two routes into the zombie turn out to be closed already — an abort during the retry sleep rejects it, so fetchNetworkState() throws and start() never reaches the updater; an abort during a live fetch blocks on the fetch lock, and the fetch then populates normally. What gets through is the abort landing after a retry sleep has fired and before the next iteration's check, which is what the test reproduces: ten further fetches after the stop, against two with the guard.

Not the abort flag, for this: abort() installs a fresh AbortController on its way out, so by the time start() resumes it can already read as never aborted. #started is the manager's own record of having a list.

The nit

@type {() => void | null} is corrected.

Still open

The gate quoted in the body ran on #1778's tree, and so did the one after it. This branch has still never been gated on its own.

[Cabecinha84]

ack