Repository navigation
A2 layer 2: one relay for every server operation - #81
Conversation
Companion wraps a message for the server in TO_SERVER; the game client checks only the envelope (the game session it is valid in, whether the server has TotalDebug) and forwards it in NeoForge chunks unread. The server decodes it with the same protocol classes and hands it to ServerOperations; answers come back as FROM_SERVER. RELAY_FAILED names an undeliverable request. Companion now asks for the server's class manifest with MANIFEST_REQUEST once the game plays a world or a server with TotalDebug, so the client no longer caches and replays it. A new manifest baseline ends the runs on the server. Replaces the per-feature forwarded payloads, the server-source request payload and the client's manifest cache. Protocol 32, network protocol 7. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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: a2835d84a1
ℹ️ 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".
| } | ||
| runner.runScript(payload.scriptId(), payload.bytecode(), payload.environment(), subject, | ||
| payload.subjectExpectedId()); | ||
| runner.runScript(payload.scriptId(), payload.bytecode(), environment, subject, payload.subjectExpectedId()); |
There was a problem hiding this comment.
When Companion is restarted while a server-side script remains active, the new process restarts its run counter at 1, while the server's ScriptRunner survives because it is scoped to the still-connected player. Passing payload.scriptId() directly therefore reuses the old execution ID: the new run can be rejected as a duplicate, and subsequent results from the old run can be attributed to the new run. Preserve a game-lifetime execution ID translation for server runs, as the removed client transport did.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23d67d1. Both findings had one cause: the manifest session bounds the server's runs, and only Companion's side acted on that. Now the server closes the previous runner when a new manifest session starts, and drops that runner's late results, so a restarted Companion's ids cannot collide with it. Companion also counts a result only from the side the run went to, so a stale server result cannot end a client run with the same id.
| this.manifestRequestedFor = null; | ||
| return; | ||
| } | ||
| if (target.equals(this.manifestRequestedFor)) return; |
There was a problem hiding this comment.
When the player disconnects and rejoins the same address between the ChangePublisher's one-second samples, the intermediate menu state is never published and the next PlayingPayload.Multiplayer equals the previously published value. ClientRelay.serverLeft() only sends an unavailable manifest, so manifestRequestedFor still names that address and this guard suppresses the new manifest request; the server removed the old manifest session on logout, leaving server-side scripts unavailable until another location change or Companion reconnect. Clear this key on the server-left notification or force the playing state to republish.
AGENTS.md reference: companion/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 23d67d1 (same cause as the P1). The client now publishes Menu at once when it leaves a server, instead of waiting for the next one-second check. A rejoin of the same address is then a change, and Companion asks the new server session for its manifest.
A restarted Companion counts its run ids from 1 again, while the server's runner of the previous session could still run and report under the same ids. The server now closes the previous runner when it starts a new manifest session and drops its late results, and Companion counts a result only from the side the run went to. Leaving a server is published to Companion at once, so a rejoin of the same server between two checks still asks the new server session for its manifest. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Took 2 minutes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23d67d1a7e
ℹ️ 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".
| sendResult(server, sessionPlayer, scriptId, ExecutionResult.fromStatus(ExecutionStatus.COMPILATION_FAILED, message), | ||
| () -> true); |
There was a problem hiding this comment.
When Companion restarts after a server request is rejected but before the single-threaded result encoder drains, this unconditional predicate lets the old terminal failure be delivered after sendManifest has replaced the session. Because the new Companion reuses run IDs from 1, that failure can then terminate an unrelated new run with the same ID. The new runner-session guard is fresh evidence that the prior ID-collision fix remains incomplete: it protects runner-produced results, but not asynchronously queued policy, session, environment, or runner-setup failures. Bind these failures to the manifest session that received the request, or retain game-lifetime execution IDs.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dd835e. This had the same cause as the Companion-close finding: a server run was not bound to the Companion session that asked for it. Now every run belongs to the manifest session that took its request, and everything sent about it is guarded by that session still being current. That covers the policy, session, environment and runner-setup failures queued before the run started, as well as the runner's own results.
| public synchronized void close() { | ||
| this.forwardedResults.clear(); | ||
| var detachedRuns = new ArrayList<>(this.activeRuns.values()); | ||
| // Detach observers before cancellation can synchronously deliver a result. | ||
| this.activeRuns.clear(); | ||
| for (Run run : detachedRuns) { | ||
| if (run.side() != Side.SERVER) { | ||
| continue; | ||
| } | ||
| try { | ||
| if (this.serverTransport.availability().available()) { | ||
| this.serverTransport.stop(new StopServerScriptPayload(run.executionId())); | ||
| } | ||
| } catch (RuntimeException exception) { | ||
| TotalDebug.LOGGER.debug("Unable to stop server script {} while closing", run.executionId(), exception); | ||
| } | ||
| } | ||
| if (this.runner != null) { | ||
| this.runner.close(); |
There was a problem hiding this comment.
When Companion disconnects or exits while a server-side script is active but the player remains in the game, the session-closed handler reaches this method, which now only closes the client runner. The removed implementation explicitly stopped tracked server executions, while the new relay sends no equivalent session-close signal, so the server's RunnerSession can keep the script running until the player logs out, the server stops, or another Companion connects and requests a manifest. Add server-session cleanup through the relay instead of abandoning active server runs.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dd835e. When the Companion session closes, the client now sends COMPANION_LEFT through the relay; it is the one relayed message the client writes itself. The server ends that player's manifest session and closes its runner. The same method, ServerScriptService.endSession, runs on logout.
| public void sendFromServer(RelayedMessage message) { | ||
| send(new FromServerMessage(message)); |
There was a problem hiding this comment.
When a server script produces an execution result near ExecutionResultCodec.MAX_WIRE_BYTES, the result is already sized to consume the SCNet frame budget for a direct ExecutionResultMessage. Wrapping that encoded body in RelayedMessage and then FromServerMessage adds correlation, session, message-ID, and length fields, so the game-to-Companion frame exceeds DEFAULT_MAX_FRAME_SIZE and the terminal result cannot be delivered. Budget server results for the extra envelope bytes before forwarding them, otherwise sufficiently large logs or values leave the run without its result.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dd835e. RelayedMessage.MAX_BODY_BYTES is now the Companion frame size minus the worst-case envelope. ExecutionResultCodec.MAX_WIRE_BYTES is budgeted from that, so a server result fits both sent directly and relayed. A test writes the largest body with the longest session and checks that it fits one frame.
| /** What the client takes from the server, whose class manifest is its largest message. */ | ||
| public static RelayAssembler toClient() { | ||
| return new RelayAssembler(RelayedMessage.MAX_BODY_BYTES, 8); |
There was a problem hiding this comment.
When the client is connected to a hostile or malfunctioning TotalDebug server, that server can start eight transfer UUIDs and send almost 33 MiB into each without ever sending their final chunks. The size check is per Transfer, while incomplete entries have no timeout and remain until eviction or disconnect, so this assembler can retain roughly 264 MiB of payload data, potentially substantially more because each ByteArrayOutputStream over-allocates; the server-facing assembler similarly permits four separate 2 MiB allocations per player. Enforce a shared byte budget across all in-flight transfers rather than multiplying the nominal cap by maxTransfers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3dd835e. All transfers in progress on a connection now share one byte budget: 2 MiB per player on the server, and one relay body on the client. Chunks are held as they arrived and copied once when the message completes, so there is no ByteArrayOutputStream over-allocation.
…elay A run now belongs to the manifest session that took its request. Everything the server sends about it, including failures queued before the run started, is dropped once that session ended. The session ends when Companion asks for a new one, when the player leaves, or when Companion's connection closes: the client then sends COMPANION_LEFT, the one relayed message it writes itself. A relayed message and its envelope fit one Companion frame, and execution results are budgeted for it. The transfers in progress on one connection share one byte budget and are held as the chunks that arrived. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd835e660
ℹ️ 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 ServerOperations(ServerScriptService scripts) { | ||
| Objects.requireNonNull(scripts, "scripts"); | ||
| this.operations = Map.of( | ||
| ManifestRequestMessage.class, (player, message) -> scripts.sendManifest(player), |
There was a problem hiding this comment.
A modified client can send this empty request repeatedly, and every message starts a fresh session and calls sendManifest; while the shared manifest is building this accumulates unbounded completion callbacks, and after it is ready each request can amplify into a baseline response of up to 32 MiB. This bypasses the relay's byte-budget protections and lets any connected player consume server memory and outbound bandwidth, so reject, coalesce, or rate-limit requests while that player's manifest session is already current.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72aaf08. The manifest serves only server scripts, so the server now applies the script policy (enableScripts, operator-only) before it starts a session. A refused player gets the reason as an unavailable manifest, not a session, so their repeated requests cost a few bytes each. A player the policy admits can already run arbitrary code on the server, so rate-limiting their requests would not bound anything. As a side effect, Companion now shows the policy reason before you compile, instead of refusing the run afterwards.
The manifest serves only server scripts. A request is a few bytes and its answer up to 32 MiB, so the server now checks the script policy first and answers a refused player with the reason instead of a session. A player the policy admits can already run code on the server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A2 layer 2 (see
docs/MOD_SIDES.md): one generic path from Companion to the server.What changes
TO_SERVER(correlation + game session). The game client checks only the envelope and forwards it inToServerNeoForge chunks without reading it. The server decodes it with the same protocol classes and dispatches throughServerOperations. Answers travel back asToCompanionchunks →FROM_SERVER, which Companion unwraps onto its normal message bus.RELAY_FAILEDnames an undeliverable request: world left since the target was inspected, no world joined, or the server lacks TotalDebug.MANIFEST_REQUESTwhenPLAYINGreports singleplayer, or multiplayer with TotalDebug. The client's manifest cache and replay are gone. A new manifest baseline (offset 0) ends the runs on the server.RunServerScriptPayload,StopServerScriptPayload,ServerSourceRequestPayload,ServerManifestPayload,ServerScriptTransportand their tests.ClientScriptServiceruns client scripts only.Notes
Tests
:protocol:test :mod:test :companion:testpass. They cover chunk splitting and reassembly, dropped or oversized transfers, envelope round trips, the refusal of unknown ids and trailing bytes, run stop and relay-failure routing by side, and server-session end.🤖 Generated with Claude Code