Repository navigation
Add bounded cross-node Network Doctor and fix proxy workspace management - #26
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dcd74ebb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (nodes.stream().noneMatch(n -> !proxy(n) && n.nodeId().equals(backend.backendId()))) emit(p, "topology.backend.unenrolled", "Control & Topology", Status.WARNING, | ||
| "A proxy-reported backend is not enrolled in this bounded Control view.", "registry.backends", "Enroll the intended backend or verify the reported identity."); |
There was a problem hiding this comment.
When more than 100 nodes are enrolled, /api/v1/network-doctor evaluates only the first page and sets truncated=true; a proxy on that page can reference an enrolled backend that sorts onto a later page. This condition nevertheless emits topology.backend.unenrolled as WARNING and advises enrollment merely because the backend is absent from the bounded view. Gate this assertion on !truncated or emit UNKNOWN so a large valid network is not misdiagnosed.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
| }; | ||
| renderJsonResult(networkDoctorResults, lastDiagnostics); | ||
| updateSetupChecklist(diagnostics.result); | ||
| try { await Promise.all([refreshConfigurationHealth(), ...Array.from({length: Math.min(3, candidates.length)}, worker)]); } |
There was a problem hiding this comment.
For registries larger than 100 nodes, the inspection workers use the bounded candidates list and deadline, but refreshConfigurationHealth() expands allNodeItems and starts managed-file reads for every supported node without receiving either bound. Those reads are included in this Promise.all, so one online node that does not claim its configuration task can keep the doctor busy until the configuration operation expires (up to 15 minutes), well after the 175-second abort, while a large registry causes an unbounded number of read batches and node IDs to be processed. Scope this branch to the same 100 nodes and race/cancel it with the same deadline.
AGENTS.md reference: AGENTS.md:L99-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e8527700
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function inspectWorkspaceServer(id) { | ||
| if (!isBackend(nodeIndex.get(id))) return; | ||
| if (!isBackend(nodeIndex.get(id)) && !isProxy(nodeIndex.get(id))) return; |
There was a problem hiding this comment.
When this new proxy path calls setActiveTab('overview', true), workspaceHash() creates #servers/<proxy>/overview, but applyNavigationRoute() still rejects every inspectedServerId for which isBackend(...) is false. Consequently, refreshing the page or revisiting that history entry resets a valid proxy-management workspace to Home; the route validator needs to accept supported proxy nodes too.
Useful? React with 👍 / 👎.
| if (isProxy(nodeIndex.get(id)) || !workspace.managementScope) { | ||
| if (!changeWorkspaceTargets(() => workspace.setTargets([id]))) return; | ||
| } |
There was a problem hiding this comment.
After a user opens “Manage proxy settings,” the workspace has managementScope === 'SERVER' with the proxy as its sole target. Clicking “Inspect server overview” on a backend then skips this branch because the destination is not a proxy and the scope is already set, leaving selectedTargetIds on the proxy while selectedServerId and inspectedServerId switch to the backend. General Settings can therefore continue reading or previewing the proxy's bungeeconfig.yml under a backend overview; preserve multi-backend inspection, but replace a proxy-only target when transitioning back to a backend.
AGENTS.md reference: AGENTS.md:L47-L49
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50d8249aa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| NodeRegistry.RegistryPage page = registry.page(0, 100, null); | ||
| var evidence = inspectionOperations.networkEvidence(page.items()); |
There was a problem hiding this comment.
If a node reconnects or changes capabilities between registry.page(...) and networkEvidence(...), the evidence lookup validates retained results against the stale NodeStatus from the page rather than the registry's current session. Re-registering an existing node does not increment the membership revision, so old-session evidence can enter the report and produce PASS results after that session has been invalidated; re-read or session-validate each node while assembling the evidence snapshot.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ed365acc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| function withConfigurationChecks(report, checks) { | ||
| const configurationChecks = (Array.isArray(checks) ? checks : []).slice(0, 100); | ||
| return {...report, configurationChecks, checks: [...(report.checks || []), ...configurationChecks.map(check => ({ |
There was a problem hiding this comment.
When the server report is already near its 500-check limit, this concatenation adds up to four managed-configuration checks, so the rendered and downloaded report can exceed the documented cap—and can do so with truncated: false if the server itself finished just below the threshold. Reserve space for these checks or truncate the combined list while marking the report truncated, so the browser-side consumer preserves the same bound as the evaluator.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbfc5bae94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reader = options.nodeIds ? createConfigurationHealthReader( | ||
| () => generalSettingsTargets(true).filter(target => options.nodeIds.has(target.id)).slice(0, 100), options) |
There was a problem hiding this comment.
When a proxy reports a backend that is not enrolled, generalSettingsTargets(true) creates a placeholder for that relationship, but this filter removes it because its ID is absent from doctorNodes. The resulting health model can contain only the readable proxy and report network-wide settings such as OnlineMode or BedrockPlayerPrefix as PASS despite having no backend evidence. Preserve missing relationship placeholders without scheduling reads, or mark the retained network targets incomplete before generating configuration checks.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e799f812fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const placeholders = allTargets.filter(target => (!omitted.has(target.id) && target.networkOnly)) | ||
| .map(target => ({...target, online: false, supported: false, networkIncomplete: true})); |
There was a problem hiding this comment.
When the first 100 nodes contain a proxy whose enrolled backend sorts onto a later page, this filter drops that backend because global generalSettingsTargets(true) marks every enrolled node as networkOnly: false; the remaining proxy evidence can therefore report OnlineMode or BedrockPlayerPrefix as PASS without its related backend. Fresh evidence in this version is the combination of the new networkOnly-only placeholder filter here and the global initialization of directlyManagedIds from every enrolled node. Preserve all omitted nodes that belong to a requested proxy/backend relationship as incomplete placeholders, not only unenrolled targets.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
| const proxyOnly = this.selectedTargetIds.size === 1 | ||
| && [...this.selectedTargetIds].some(id => (Array.isArray(nodes) ? nodes : []) | ||
| .some(node => node && node.nodeId === id | ||
| && ['VELOCITY', 'BUNGEECORD'].includes(String(node.platform || '').toUpperCase()))); |
There was a problem hiding this comment.
If a user opens a proxy-management workspace, enters Global, and then the registry refreshes—or restores a persisted Global session—selectedTargetIds is empty, so proxyOnly becomes false and reconcile() removes the valid proxy from previousTargetIds. Returning to server tools then produces an empty workspace instead of restoring the proxy and its bungeeconfig.yml target; determine proxy validity independently for previousTargetIds while the current scope is Global.
AGENTS.md reference: AGENTS.md:L47-L49
Useful? React with 👍 / 👎.
| return {...report, truncated, configurationChecks, checks: [...serverChecksKept, ...configurationChecks.map(check => ({ | ||
| id: `configuration.${check.path}`, category: 'Proxy Setup', | ||
| status: Object.hasOwn(rank, check.status) ? check.status : 'UNKNOWN', title: check.title || check.path, |
There was a problem hiding this comment.
When the server returns close to 500 checks but is not itself truncated, adding the four configuration checks can slice off one or more of them and set only the report's truncated Boolean. render() never displays that Boolean, and no control.evidence.truncated check is synthesized here, so the visible doctor output silently omits unverified configuration checks instead of presenting them as UNKNOWN. Reserve a slot for an explicit truncation check or render the truncation state prominently.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38f4081cc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id: `configuration.${check.path}`, category: 'Proxy Setup', | ||
| status: Object.hasOwn(rank, check.status) ? check.status : 'UNKNOWN', title: check.title || check.path, | ||
| explanation: check.message || 'Managed configuration evidence is unavailable.', | ||
| affectedNodes: [...new Set([...(check.nodeIds || []), ...(check.unknownNodeIds || [])])].slice(0, 100), |
There was a problem hiding this comment.
When a proxy reports more than 100 related backends, a configuration health check can contain over 100 nodeIds/unknownNodeIds, but this slice silently removes the remaining IDs while the truncated calculation only considers the number of checks. The rendered and downloaded report can therefore have truncated: false even though affected peers were omitted; include affected-node overflow in the truncation state or add an explicit UNKNOWN truncation check.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a9cad09a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A doctor run owns a separate bounded reader, so an older dashboard flight cannot extend its deadline. | ||
| const reader = options.nodeIds ? createConfigurationHealthReader( | ||
| () => { | ||
| const allTargets = generalSettingsTargets(true); |
There was a problem hiding this comment.
When Run checks is launched from a SERVER or MULTI_SERVER workspace, generalSettingsTargets(true) is seeded from workspace.selectedTargetIds, not from the 100-node doctor page. Filtering that smaller list by options.nodeIds cannot add the other nodes, so runtime checks cover the enrolled page while configuration checks can report PASS after inspecting only the selected workspace, silently omitting unrelated networks. Build the initial target list from options.nodeIds/nodeIndex before expanding relationships so omitted configuration evidence remains UNKNOWN.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d73bc9ea20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| control: {application: 'VotingPlugin Control', registeredNodes: allNodeItems.length, | ||
| nodes: allNodeItems.slice(0, 100).map(item => ({nodeId: item.nodeId, displayName: item.displayName, | ||
| role: roleLabel(item), online: item.online, pluginVersion: item.pluginVersion}))} | ||
| const doctorNodes = allNodeItems.slice(0, 100); |
There was a problem hiding this comment.
If registry membership or topology changes after the last node refresh—or during this run's 175-second collection window—doctorNodes remains the browser's cached allNodeItems, while the final /api/v1/network-doctor request independently evaluates the server's current first page. A newly inserted or newly related node can therefore appear in the server report without participating in configuration reads, allowing checks such as OnlineMode or AutoCreateVoteSites to report PASS despite missing current-node evidence. Return/compare a registry revision and node IDs, or otherwise mark configuration checks UNKNOWN when the snapshots differ.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
1 participant
Network Doctor now evaluates bounded, typed evidence across observed proxy/backend networks instead of showing only one backend's diagnostics. It groups structured FAIL/WARNING/UNKNOWN results and collapses PASS/INFO, with the same structured results in the JSON download.
Checks cover backend identity/normalization, applicable transports and authentication, path-aware Votifier duplication, routing, storage, Vote Sites, rewards/VoteParty settings and declared multi-proxy groups. Offline, old, stale, incomplete or unsupported observations remain UNKNOWN. The read-only checker never creates votes, repairs configuration, exposes raw files/secrets, or scans endpoints.
The additive
data.network-health.v1capability requiresdata.inspect.v1; registration protocol remains version 1. Evidence is validated against a strict schema, current session, server-measured freshness and bounded inventories. Existing managed configuration checks remain included. Seedocs/network-doctor.mdfor supported evidence and explicitly unavailable checks.Validation:
mvn -B testandmvn -B clean verifyeach pass 335 tests; changed frontend scripts passnode --check; all 186 WebUI tests pass;git diff --checkand added-line credential scan pass. Bukkit/BungeeCord/Velocity candidate payloads pass the actual Control schema. Fresh independent read-only review: No findings.Paired VotingPlugin diagnostics and optional VotifierPlus snapshot are additive and can be deployed in either order. Older peers continue normally and produce UNKNOWN where they cannot report evidence.
Also fixes reported management UI defects: proxy node cards and overview topology expose proxy settings and switch only the proxy target after discard checks, API epoch-second timestamps no longer render as January 1970, and staging status wraps long text. Chromium layout probes at 390/768/1280px show no horizontal staging overflow; the reported one-word-per-line overview was not reproduced.
Related: BenCodez/VotingPlugin#1689 (diagnostic producers and proxy inventory), BenCodez/VotifierPlus#186 (optional names-only listener/forwarding API).
Review follow-up: truncated registry views now report out-of-page backend enrollment as UNKNOWN. Managed configuration reads use the same bounded 100-node page and shared abort/deadline as runtime inspections; a separate reader prevents a prior dashboard flight or cached healthy values from extending or contaminating this run. Regression tests cover bounded reads, abort settlement and unavailable evidence.
Navigation follow-up: proxy overview routes survive refresh/history navigation, and opening a backend from a proxy-only workspace switches the actual configuration target back to that backend. Multi-backend inspection is preserved, and cancelled dirty transitions retain the previous target and route. Four behavioral regressions pass.
Evidence follow-up: report assembly revalidates each current registry session and accepted capabilities under the existing per-node session fence. Re-registration or capability loss between page capture and evidence selection produces unavailable evidence (
UNKNOWN), rather than stale healthy results. Two deterministic regressions cover the unchanged membership revision.Combined-report follow-up: server and managed-configuration checks share the 500-check bound. The rendered report and JSON download use the same capped object, preserve upstream truncation and mark any browser-side truncation. Exact-boundary and overflow regressions pass.
Topology evidence follow-up: bounded configuration health reads retain missing and out-of-page related peers as non-readable incomplete placeholders. Network-wide settings remain UNKNOWN without backend evidence; reads stay capped at 100, and matching fully enrolled settings still PASS. Three behavioral regressions cover these distinctions.
Latest review follow-up: doctor configuration evidence follows actual proxy/backend relationships rather than the workspace-only
networkOnlymarker, retaining enrolled out-of-page peers as UNKNOWN while excluding unrelated standalone nodes. Global reconciliation/restore retains a previous proxy-only workspace. Both server and browser report truncation are shown prominently as UNKNOWN outside collapsed PASS/INFO sections. Five additional behavioral regressions pass.Validation note for this follow-up: the first clean verify hit the existing immediate HTTP shutdown thread-liveness assertion. That unchanged focused test passed on recheck, followed by a full clean verify (333 tests, no failures/skips). No assertion or server lifecycle code was changed.
Affected-node bound follow-up: configuration checks whose distinct affected-node IDs exceed 100 now mark the report truncated before clipping the rendered affected-node list. The same flag survives JSON download and activates the visible UNKNOWN notice. Exact 100-node and duplicate-ID countertests remain complete.
Doctor-page follow-up: configuration target construction now accepts explicit doctor-page seed IDs before topology expansion, independent of the active Server/Multi-Server workspace. Ordinary workspace settings retain their default targets. Real target-builder regressions confirm complete cross-network page coverage and UNKNOWN for an offline peer beyond the selected workspace, without changing the selection.
Registry snapshot follow-up: node pages and the final doctor report expose an additive bounded public-metadata fingerprint. The WebUI captures the initial first-page identifier, retaining it through collection, and renders configuration evidence UNKNOWN if the final evaluated snapshot differs or is unsupported. Membership, sessions, capabilities and backend identities invalidate it; heartbeat timestamps/player counts do not. Tests cover response matching, identity/topology changes at the same membership revision, pagination capture, and a refresh during collection. No node protocol or vote-processing change.