Repository navigation
Add Control transport diagnostics and safe proxy method switching - #1589
Conversation
ac2c56c to
6c646e9
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
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: 9df6143f2d
ℹ️ 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".
| boolean replaceRuntime = requiresRuntimeReplacement(result) && runtimeReplacement != null; | ||
| synchronized (operationLifecycle) { | ||
| drained = completedTasks.isEmpty(); | ||
| if (replaceRuntime) deferredReplacement = runtimeReplacement; |
There was a problem hiding this comment.
When a proxy-method result is acknowledged, this queues runtimeReplacement, which finishCycle() runs synchronously at line 315 before the whenComplete block completes operationDone at lines 295–301. The resulting reloadCore(true) calls prepareForRuntimeReplacement(), whose connector shutdown waits on that same still-incomplete activeOperation for 65 seconds; because the current callback is responsible for completing it, the wait times out and the platform aborts the reload. Every successful method switch can therefore leave the new method persisted while the old runtime transport remains active and Control is stopped. Dispatch the replacement only after operationDone has been completed, or otherwise avoid waiting on the operation from its own completion callback.
Useful? React with 👍 / 👎.
💡 Codex Review Require a database name before switching to MYSQL
When  Validate only configured socket targets
When the proxy has registered servers that are intentionally absent from VotingPlugin's Recognize comments after plain-scalar apostrophes
When a valid plain YAML scalar contains an apostrophe before a comment, such as ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e8e674d7d
ℹ️ 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".
| return configuration != null && configuration.has("preset") | ||
| && PROXY_METHOD_PRESET.equals(configuration.get("preset").getAsString()); |
There was a problem hiding this comment.
A successful proxy-method READ or PREVIEW also returns a configuration containing the proxy-method preset, so this predicate schedules runtimeReplacement when either result is acknowledged even though nothing was applied. Consequently, merely inspecting or previewing the method performs a disruptive full proxy reload; preserve the operation type in the stored result or otherwise require an APPLY result here.
Useful? React with 👍 / 👎.
| ProxyMethodConfiguration current = new ProxyMethodConfiguration( | ||
| com.bencodez.votingplugin.proxy.control.ProxyMethodConfigurationService.canonical( | ||
| latest.getString("BungeeMethod", "PLUGINMESSAGING"))); |
There was a problem hiding this comment.
When a Bungee configuration omits BungeeMethod, ProxyMethodConfigurationService.read() observes the existing getBungeeMethod() default of SOCKETS, but this persistence check interprets the same file as PLUGINMESSAGING. An apply using the revision returned by the read therefore always raises StaleControlRevisionException for such legacy or minimal configurations; both paths must use the same default.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5366339b16
ℹ️ 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".
| throw new StaleRevisionException(); | ||
| } | ||
| try { | ||
| proxy.getConfig().persistControlProxyMethod(proposal.method().name(), expectedRevision); |
There was a problem hiding this comment.
When an existing BungeeMethod contains a typo or retired value, read() exposes the runtime fallback (PLUGINMESSAGING) and its revision, but this persistence call reaches both platform implementations, which run canonical(...) on the raw invalid value and throw before writing the proposed replacement. Consequently every Control APPLY returns APPLY_FAILED, so Control cannot repair the invalid setting; interpret the current value with the same fallback used when computing the expected revision while continuing to validate the proposed value strictly.
Useful? React with 👍 / 👎.
| return HexFormat.of().formatHex(MessageDigest.getInstance("SHA-256") | ||
| .digest(method.name().getBytes(StandardCharsets.UTF_8))); |
There was a problem hiding this comment.
The revision hashes only the method, although validate() also relies on transport-specific settings such as the Redis host, MQTT endpoint, socket routes, and database configuration. If one of those settings changes on disk after preview but before APPLY while BungeeMethod remains unchanged, the expected revision still matches and validation uses the previously loaded config; persistence then preserves the newly invalid setting, switches methods, and triggers a full runtime restart into a broken transport. Include the preflight inputs in the revision or validate the freshly loaded configuration atomically during persistence.
Useful? React with 👍 / 👎.
|
Follow-up validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53882fd858
ℹ️ 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".
| } | ||
|
|
||
| public void apply(ProxyMethodConfiguration proposal, String expectedRevision) throws IOException { | ||
| validate(proposal); |
There was a problem hiding this comment.
When an operator fixes a proposed transport's prerequisites directly on disk without reloading the proxy, this cached validate(proposal) still sees the old invalid values and rejects the APPLY before persistControlProxyMethod can run its newly added fresh-file validator. For example, adding a missing Redis host to the YAML cannot enable a REDIS switch until the operator performs a separate manual reload; remove the cached preflight from APPLY or validate the same freshly loaded snapshot used by persistence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 078eb6781c
ℹ️ 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".
| throw new IllegalArgumentException("BungeeServer.Port must be set"); | ||
| } | ||
| List<String> blockedServers = config.getBlockedServers(); | ||
| for (String server : proxy.getAllConfiguredServers()) { |
There was a problem hiding this comment.
When SpigotServers contains a stale entry for a server no longer registered with the proxy, this loop never validates it, but VotingPluginProxy.rebuildSocketClients() iterates every configured SpigotServers entry. For example, an unregistered entry with Port: bad passes preflight and is then cast to int during the full runtime replacement, throwing ClassCastException and preventing the SOCKETS transport from starting. Validate the same unblocked getSpigotServers() set that the runtime rebuilds.
Useful? React with 👍 / 👎.
|
Final follow-up validation:
AI-assisted implementation; changes and test coverage were reviewed before publication. @codex review |
|
Current-head follow-up:
@codex review |
|
Stack follow-up:
@codex review |
|
Final current-head review request:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be4e9a92fa
ℹ️ 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 (activeMethod == null || globalMessageProxyHandler == null) { | ||
| return CompletableFuture.completedFuture(CommunicationTestResult.failure(server, activeMethod, | ||
| "TRANSPORT_UNAVAILABLE", "The proxy communication transport is not running")); |
There was a problem hiding this comment.
When MQTT initialization fails, load() catches the exception and continues with mqttHandler == null, while globalMessageProxyHandler is still constructed. This check therefore treats the transport as available; sendMqttEnvelopeServer() silently returns false, and the diagnostic eventually reports TIMEOUT rather than the intended TRANSPORT_UNAVAILABLE. The same issue occurs for a missing socket client, so check the method-specific transport state or propagate the send method's acceptance result before scheduling the timeout.
Useful? React with 👍 / 👎.
|
Current-head follow-up:
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Summary
Safety
Tests
Depends on #1588. Paired WebUI: BenCodez/VotingPlugin-Control#7.