POC only: Bifrost#5319
Conversation
Chunk 2 of the Bifrost First Contact plan. Adds a Nakama engine subsystem that bridges Gestalt chat events to a shared Nakama chat channel, enabling cross-game messaging. Includes NakamaConfig (system properties), NakamaSubSystem (connection lifecycle, send/receive), NakamaSystem (entity system for chat events), and engine registration. Uses Nakama Java SDK 2.5.3 via JitPack. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add integration tests that verify auth, channel join, and message send/receive against a live Nakama server. Tests are tagged @integration and excluded from normal builds. Also split NakamaConfig into separate grpcPort (7349) and wsPort (7350) since the SDK uses gRPC for API and WebSocket for realtime — these map to different NodePorts in k8s. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace hardcoded Nakama config with AutoConfig integration following the DiscordRPC pattern. Config auto-saves to ~/.terasology/configs/nakama/. Upgrade protobuf from 3.22.5 to 4.28.2 to match Nakama SDK requirements — EntityData.java is auto-generated by the protobuf Gradle plugin so no manual regeneration needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Incoming Nakama messages now appear in the in-game chat overlay via Console.addMessage with CoreMessageType.CHAT. Messages are queued on the WebSocket thread and drained on the game thread in preUpdate to avoid threading issues. Console is lazily resolved from the current GameState context. Also fix outbound player name to use DisplayNameComponent instead of the raw EntityRef debug string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a new Nakama subsystem module for multiplayer chat integration, enabling device authentication, websocket connections, and message queuing. It includes configuration classes, engine subsystem and entity system implementation, unit and integration tests, and updates the Protobuf dependency to version 4.28.2. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Thread
participant NakamaSubSys as NakamaSubSystem
participant NakamaServer as Nakama Server
participant MessageQueue as Message Queue
participant GameThread as Game Thread / Console
Note over Client,GameThread: Initialization Flow
Client->>NakamaSubSys: initialise(enabled=true)
NakamaSubSys->>NakamaServer: authenticate(deviceId)
NakamaServer-->>NakamaSubSys: session token
NakamaSubSys->>NakamaServer: openWebSocket()
NakamaServer-->>NakamaSubSys: connected
NakamaSubSys->>NakamaServer: joinChannel(channel)
Note over Client,GameThread: Incoming Message Flow
NakamaServer->>NakamaSubSys: ChannelMessage (JSON)
NakamaSubSys->>MessageQueue: enqueue(parsedMessage)
GameThread->>NakamaSubSys: preUpdate()
NakamaSubSys->>MessageQueue: drain()
NakamaSubSys->>GameThread: Console.addMessage(CHAT)
Note over Client,GameThread: Outgoing Message Flow
GameThread->>NakamaSubSys: sendChatMessage(player, text)
NakamaSubSys->>NakamaServer: sendChannelMessage(JSON payload)
NakamaServer-->>NakamaSubSys: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
subsystems/Nakama/build.gradle.kts (1)
12-15: Unusual custom class output directories — what's the rationale?Overriding
destinationDirectorytobuild/classesandbuild/testClassesdeviates from Gradle's default (build/classes/java/mainandbuild/classes/java/test). This may cause issues with IDE integration, tooling, or other plugins expecting standard paths.If there's no specific requirement, consider removing this customization.
♻️ Remove custom output directories
-configure<SourceSetContainer> { - main { java.destinationDirectory.set(layout.buildDirectory.dir("classes")) } - test { java.destinationDirectory.set(layout.buildDirectory.dir("testClasses")) } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Nakama/build.gradle.kts` around lines 12 - 15, The custom output path override in the configure<SourceSetContainer> block (main { java.destinationDirectory.set(layout.buildDirectory.dir("classes")) } and test { java.destinationDirectory.set(layout.buildDirectory.dir("testClasses")) }) should be removed to restore Gradle defaults; delete or comment out those java.destinationDirectory.set(...) lines so the project uses Gradle's standard build/classes/java/main and build/classes/java/test layout (or, if a nonstandard path is required, document the rationale and ensure IDE/tooling configs are updated accordingly).facades/PC/build.gradle.kts (1)
22-27: JitPack repository is duplicated across build files.This repository block is also declared in
subsystems/Nakama/build.gradle.kts. Consider centralizing in rootbuild.gradle.ktsorsettings.gradle.ktsdependencyResolutionManagementto avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@facades/PC/build.gradle.kts` around lines 22 - 27, The JitPack maven repository block is duplicated in facades/PC/build.gradle.kts and subsystems/Nakama; remove the repositories { maven { name = "JitPack" ... } } block from facades/PC/build.gradle.kts and centralize it in the root build configuration (either in root build.gradle.kts’s repositories or, preferably, in settings.gradle.kts under dependencyResolutionManagement { repositories { maven { url = uri("https://jitpack.io") } } }) so all subprojects inherit it; after moving, confirm subprojects still resolve dependencies and optionally keep the repository name "JitPack" if you rely on that label.subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaConfigTest.java (1)
9-19: Test class name is misleading — it testsNakamaAutoConfig, notNakamaConfig.The class is named
NakamaConfigTestbut the test method instantiates and verifiesNakamaAutoConfig. Consider renaming toNakamaAutoConfigTestfor clarity.♻️ Suggested rename
-class NakamaConfigTest { +class NakamaAutoConfigTest { `@Test` void autoConfigDefaultsAreDisabled() {Also rename the file to
NakamaAutoConfigTest.java.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaConfigTest.java` around lines 9 - 19, Rename the test class NakamaConfigTest to NakamaAutoConfigTest to match the SUT being tested (NakamaAutoConfig); update the top-level class declaration from "class NakamaConfigTest" to "class NakamaAutoConfigTest" and move/rename the file to NakamaAutoConfigTest.java, and then update any references/imports/usages (test runners or suites) that refer to NakamaConfigTest so they point to NakamaAutoConfigTest; keep the test method autoConfigDefaultsAreDisabled and its assertions targeting NakamaAutoConfig unchanged.subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java (1)
143-158: Blocking.get()insendChatMessage()can stall the game thread.
writeChatMessage().get()blocks the game thread until the message is acknowledged. If the Nakama server is slow or unresponsive, this will freeze the game. For a POC this may be acceptable, but consider fire-and-forget or async with timeout for production.Alternative: Fire-and-forget with error callback
try { JsonObject content = new JsonObject(); content.addProperty("game", GAME_ID); content.addProperty("player", playerName); content.addProperty("text", text); - socket.writeChatMessage(channel.getId(), content.toString()).get(); + socket.writeChatMessage(channel.getId(), content.toString()) + .exceptionally(e -> { + logger.warn("Nakama: failed to send message", e); + return null; + }); return true; - } catch (Exception e) { - logger.warn("Nakama: failed to send message", e); - return false; - } + } catch (Exception e) { + logger.warn("Nakama: failed to send message", e); + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java` around lines 143 - 158, sendChatMessage currently blocks by calling socket.writeChatMessage(...).get(), which can freeze the game thread; change it to fire-and-forget or an async call with a timeout by removing the blocking .get() and instead use the CompletableFuture returned by socket.writeChatMessage(channel.getId(), ...) with a non-blocking handler (e.g., thenAccept/whenComplete/exceptionally) to log failures and optionally orTimeout/completeOnTimeout to enforce a maximum wait; ensure sendChatMessage returns immediately (true on dispatch) and reference the sendChatMessage method, socket.writeChatMessage(...) call and channel.getId() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@settings.gradle.kts`:
- Line 29: The project bumped protobuf to 4.28.2 (val protobuf =
version("protobuf", "4.28.2")), so regenerate all .proto artifacts under
engine/src/main/protobuf/ with a matching protoc 4.x (or at least protoc ≥3.22
for 4.x compatibility), recompile the generated Java sources, and replace any
out-of-date generated files; then audit and adjust usages of
com.google.protobuf.Descriptors in EntityDataJSONFormat.java and
ProtobufPersistedDataSerializer.java for API changes introduced in protobuf 4.x
(fix method calls or imports as needed), rebuild and run the full test suite to
confirm compatibility before merging.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaConfig.java`:
- Around line 53-54: The current direct calls to Integer.parseInt in
NakamaConfig
(config.setGrpcPort(Integer.parseInt(System.getProperty("nakama.grpcPort",
"7349"))) and
config.setWsPort(Integer.parseInt(System.getProperty("nakama.wsPort", "7350"))))
can throw NumberFormatException; add a defensive helper (e.g., private static
int parseIntOrDefault(String value, int defaultValue)) in NakamaConfig that
returns the default when the value is null, empty, or unparsable, and replace
those Integer.parseInt calls to use
parseIntOrDefault(System.getProperty("nakama.grpcPort"), 7349) and
parseIntOrDefault(System.getProperty("nakama.wsPort"), 7350).
- Around line 49-58: NakamaConfig is unused and its fromSystemProperties() also
risks NumberFormatException; remove the dead NakamaConfig class entirely (it’s
not referenced by NakamaSubSystem.initialise()) and delete
fromSystemProperties() to avoid dead code, or if you prefer to keep
functionality, update the fromSystemProperties() method to validate/parses ports
safely by using Integer.parseInt inside a try-catch (or a helper
parseWithDefault method) for nakama.grpcPort and nakama.wsPort, log parse
errors, and fall back to the default port values; reference: class NakamaConfig,
method fromSystemProperties(), and caller NakamaSubSystem.initialise().
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`:
- Around line 201-203: isConnected() reads socket and channel without
synchronization while cleanup() (invoked from preShutdown()) can null them,
causing a race that may let isConnected() return true then cause NPEs in
sendChatMessage(); fix by introducing a single thread-safe connection state
(e.g., an AtomicBoolean connectionOpen or by synchronizing access) and use that
in isConnected(), cleanup(), and sendChatMessage() instead of directly reading
socket/channel, or surround reads/writes of socket and channel with a consistent
lock to prevent the race between isConnected(), cleanup(), and
sendChatMessage().
- Around line 125-132: The suppressOutbound flag has a race between the Nakama
socket thread (handleIncomingMessage where
incomingMessageHandler.accept(formatted) is invoked) and the game thread
(sendChatMessage), so change the usage to synchronize access: introduce a
private final lock object (e.g., suppressLock) and wrap the set-to-true, handler
invocation, and set-to-false inside a synchronized(suppressLock) block in
handleIncomingMessage, and also read the flag inside a
synchronized(suppressLock) block in sendChatMessage (or replace the boolean with
the same lock-guarded check), ensuring both threads use the same lock when
accessing suppressOutbound to eliminate the race.
- Around line 168-175: The NakamaSystem is being registered twice because
NakamaSubSystem.registerSystems manually creates and registers a NakamaSystem
while the NakamaSystem class is also marked with `@RegisterSystem` for
auto-discovery; remove the `@RegisterSystem` annotation from the NakamaSystem
class (or alternatively stop manual registration) so only one instance is
created—open the NakamaSystem class, delete the `@RegisterSystem` annotation
declaration, and keep the manual registration in NakamaSubSystem.registerSystems
as the single registration path.
- Around line 83-106: The connect() method currently blocks engine init by
calling authenticateDevice().get(), socket.connect(...).get(), and
joinChat(...).get() without timeouts; change these to use get(timeout, TimeUnit)
(add import java.util.concurrent.TimeUnit) with a reasonable timeout for each
call and handle TimeoutException/InterruptedException/ExecutionException to call
cleanup() and log appropriately, or alternatively run the entire connect() logic
off the main thread (e.g., submit to an executor) and set session/socket/channel
atomically on success so authenticateDevice, socket.connect, and joinChat no
longer block initialization; update exception handling around
authenticateDevice, socket.connect, and joinChat to avoid indefinite blocking
and ensure cleanup() is called on failure.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSystem.java`:
- Around line 18-24: The class-level `@RegisterSystem` on NakamaSystem causes an
auto-instantiated system that lacks the NakamaSubSystem wiring (set via
setNakamaSubSystem), producing duplicate instances because
NakamaSubSystem.registerSystems() also manually registers NakamaSystem; fix by
either removing the `@RegisterSystem` annotation from the NakamaSystem class so
only the manually-registered instance (created in
NakamaSubSystem.registerSystems()) exists, or keep `@RegisterSystem` but remove
setter injection and instead obtain the dependency inside
NakamaSystem.initialise() using context.get(NakamaSubSystem.class) (and remove
or ignore setNakamaSubSystem) so the discovered instance is correctly wired.
In
`@subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaIntegrationTest.java`:
- Around line 64-65: The test uses the wrong method name for session expiry
check: replace the call to session.IsExpired() with the Java SDK method
session.isExpired() in NakamaIntegrationTest (update the assertion using
session.isExpired()). Ensure the assertion remains
assertFalse(session.isExpired(), "Session should not be expired") so it compiles
with Nakama Java SDK 2.5.3.
---
Nitpick comments:
In `@facades/PC/build.gradle.kts`:
- Around line 22-27: The JitPack maven repository block is duplicated in
facades/PC/build.gradle.kts and subsystems/Nakama; remove the repositories {
maven { name = "JitPack" ... } } block from facades/PC/build.gradle.kts and
centralize it in the root build configuration (either in root build.gradle.kts’s
repositories or, preferably, in settings.gradle.kts under
dependencyResolutionManagement { repositories { maven { url =
uri("https://jitpack.io") } } }) so all subprojects inherit it; after moving,
confirm subprojects still resolve dependencies and optionally keep the
repository name "JitPack" if you rely on that label.
In `@subsystems/Nakama/build.gradle.kts`:
- Around line 12-15: The custom output path override in the
configure<SourceSetContainer> block (main {
java.destinationDirectory.set(layout.buildDirectory.dir("classes")) } and test {
java.destinationDirectory.set(layout.buildDirectory.dir("testClasses")) })
should be removed to restore Gradle defaults; delete or comment out those
java.destinationDirectory.set(...) lines so the project uses Gradle's standard
build/classes/java/main and build/classes/java/test layout (or, if a nonstandard
path is required, document the rationale and ensure IDE/tooling configs are
updated accordingly).
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`:
- Around line 143-158: sendChatMessage currently blocks by calling
socket.writeChatMessage(...).get(), which can freeze the game thread; change it
to fire-and-forget or an async call with a timeout by removing the blocking
.get() and instead use the CompletableFuture returned by
socket.writeChatMessage(channel.getId(), ...) with a non-blocking handler (e.g.,
thenAccept/whenComplete/exceptionally) to log failures and optionally
orTimeout/completeOnTimeout to enforce a maximum wait; ensure sendChatMessage
returns immediately (true on dispatch) and reference the sendChatMessage method,
socket.writeChatMessage(...) call and channel.getId() when making the change.
In
`@subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaConfigTest.java`:
- Around line 9-19: Rename the test class NakamaConfigTest to
NakamaAutoConfigTest to match the SUT being tested (NakamaAutoConfig); update
the top-level class declaration from "class NakamaConfigTest" to "class
NakamaAutoConfigTest" and move/rename the file to NakamaAutoConfigTest.java, and
then update any references/imports/usages (test runners or suites) that refer to
NakamaConfigTest so they point to NakamaAutoConfigTest; keep the test method
autoConfigDefaultsAreDisabled and its assertions targeting NakamaAutoConfig
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d269fab4-d6a4-49ba-8b3d-46ea741e1064
📒 Files selected for processing (11)
engine/src/main/java/org/terasology/engine/core/TerasologyEngine.javafacades/PC/build.gradle.ktsfacades/PC/src/main/java/org/terasology/engine/Terasology.javasettings.gradle.ktssubsystems/Nakama/build.gradle.ktssubsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaAutoConfig.javasubsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaConfig.javasubsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.javasubsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSystem.javasubsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaConfigTest.javasubsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaIntegrationTest.java
| library("mockito-junit", "org.mockito", "mockito-junit-jupiter").versionRef(mockito) | ||
| // protobuf does not work as the others, see https://github.com/google/protobuf-gradle-plugin/issues/563 | ||
| val protobuf = version("protobuf", "3.22.5") | ||
| val protobuf = version("protobuf", "4.28.2") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for protobuf usage in the codebase
echo "=== Finding .proto files ==="
fd -e proto
echo ""
echo "=== Checking protobuf API usage patterns that may have changed ==="
rg -n 'parseFrom|toByteArray|newBuilder|getDescriptor' --type java -C 2 | head -100
echo ""
echo "=== Checking for protobuf imports ==="
rg -n 'import com.google.protobuf' --type java | head -50Repository: MovingBlocks/Terasology
Length of output: 15943
🌐 Web query:
What are the breaking changes between protobuf-java 3.x and 4.x?
💡 Result:
The primary breaking changes in protobuf-java 4.x (starting with v4.26.0 / protoc v26.0, released March 2024) compared to 3.x revolve around internal cleanups and support for new features like Protobuf Editions, breaking binary compatibility with older generated code. Key breaking changes: - Removal of GeneratedMessageV3 class and related APIs (e.g., PARSER field replaced by parser method; makeExtensionsImmutable and other deprecated methods removed). Generated messages now extend GeneratedMessage. - Breaking compatibility with pre-v3.22 generated code from 3.x; requires regeneration with protoc >=3.22 (ideally 3.25.x) for runtime compatibility via shims added in later 4.x releases (e.g., 4.27.4+, 4.33+ supports all 3.x gencode). - Removal of deprecated descriptor syntax APIs; use feature accessors like FieldDescriptor.hasPresence, EnumDescriptor.isClosed. - Runtime methods for v2 gencode compatibility removed. - Introduction of "poison pills" to enforce gencode/runtime version matching. - Refactoring of Java generator into separate packages. - Support for Editions (edition = "2023" etc.) changes generated code; old proto2/proto3 syntax still supported but may warn. Migration steps: - Regenerate .proto files with matching/newer protoc version (protoc 25.x+ for 3.x/4.x compatibility). - Update non-generated code referencing removed classes/methods (use Message interface instead of GeneratedMessageV3). - For libraries, use protoc 25.x for 3.x+4.x support; latest for 4.x+5.x. - protobuf-java 3.x supported until March 2027; 4.x until 2028. Binary wire format remains unchanged. Later 4.x versions (4.27+) added shims for gradual migration.
Citations:
- 1: https://protobuf.dev/support/migration/
- 2: https://protobuf.dev/news/2023-12-05
- 3: Offer a better migration path from protobuf-java 3.x to 4.x protocolbuffers/protobuf#16452
- 4: https://protobuf.dev/support/version-support/
- 5: Update to Protobuf 4.x grpc/grpc-java#11015
- 6: https://github.com/protocolbuffers/protobuf/releases
- 7: Library Compatibility with Protobuf-Java 4.26.x+ googleapis/java-pubsub#2034
- 8: https://github.com/protocolbuffers/protobuf/releases/tag/v25.9
- 9: https://newreleases.io/project/github/protocolbuffers/protobuf/release/v26.1
- 10: https://newreleases.io/project/github/protocolbuffers/protobuf/release/v27.0
- 11:
makeExtensionsImmutable()disappeared in 4.26.0 from the supposedly backwards-compatible GeneratedMessageV3 protocolbuffers/protobuf#19540 - 12: issue with upgrade com.google.protobuf:protobuf-java to 4.26.1 protocolbuffers/protobuf#16409
Protobuf 4.x requires .proto files to be regenerated with matching protoc version.
Protobuf 4.x has breaking changes from 3.x, particularly removal of GeneratedMessageV3 and changes to descriptor APIs. The .proto files in engine/src/main/protobuf/ must be regenerated with protoc 4.x (or ≥3.22 for 4.x compatibility).
The codebase's heavy use of builder patterns (.newBuilder(), .addField(), .build()) is compatible with 4.x. However, verify that code using com.google.protobuf.Descriptors (in EntityDataJSONFormat.java, ProtobufPersistedDataSerializer.java) works correctly with the new API, then run full test suite to confirm compatibility before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@settings.gradle.kts` at line 29, The project bumped protobuf to 4.28.2 (val
protobuf = version("protobuf", "4.28.2")), so regenerate all .proto artifacts
under engine/src/main/protobuf/ with a matching protoc 4.x (or at least protoc
≥3.22 for 4.x compatibility), recompile the generated Java sources, and replace
any out-of-date generated files; then audit and adjust usages of
com.google.protobuf.Descriptors in EntityDataJSONFormat.java and
ProtobufPersistedDataSerializer.java for API changes introduced in protobuf 4.x
(fix method calls or imports as needed), rebuild and run the full test suite to
confirm compatibility before merging.
| public static NakamaConfig fromSystemProperties() { | ||
| NakamaConfig config = new NakamaConfig(); | ||
| config.setEnabled(Boolean.parseBoolean(System.getProperty("nakama.enabled", "false"))); | ||
| config.setHost(System.getProperty("nakama.host", "localhost")); | ||
| config.setGrpcPort(Integer.parseInt(System.getProperty("nakama.grpcPort", "7349"))); | ||
| config.setWsPort(Integer.parseInt(System.getProperty("nakama.wsPort", "7350"))); | ||
| config.setChannel(System.getProperty("nakama.channel", "bifrost.lobby")); | ||
| config.setPlayerName(System.getProperty("nakama.playerName", "")); | ||
| return config; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether NakamaConfig is actually used anywhere in the codebase
echo "=== Searching for NakamaConfig usage ==="
rg -n 'NakamaConfig' --type java -g '!**/NakamaConfig.java' -g '!**/NakamaConfigTest.java'Repository: MovingBlocks/Terasology
Length of output: 107
🏁 Script executed:
# Find and examine NakamaSubSystem.java to verify it only uses NakamaAutoConfig
find . -name "NakamaSubSystem.java" -type fRepository: MovingBlocks/Terasology
Length of output: 153
🏁 Script executed:
# Check NakamaSubSystem's usage of config classes
rg -n "initialise\|NakamaAutoConfig\|NakamaConfig" subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/ --type javaRepository: MovingBlocks/Terasology
Length of output: 49
🏁 Script executed:
# Look at the actual NakamaConfig file to check for exception handling and comments
cat -n subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaConfig.javaRepository: MovingBlocks/Terasology
Length of output: 2875
🏁 Script executed:
cat -n subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.javaRepository: MovingBlocks/Terasology
Length of output: 10875
🏁 Script executed:
# Also check what files exist in the Nakama subsystem directory
ls -la subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/Repository: MovingBlocks/Terasology
Length of output: 453
NakamaConfig is dead code — never called anywhere in the codebase.
The search returned zero matches for NakamaConfig usage outside its own file. NakamaSubSystem.initialise() exclusively uses NakamaAutoConfig (lines 71, 86, 90, 99–100) and never references NakamaConfig.fromSystemProperties(). This entire class should be removed.
Additionally, fromSystemProperties() lacks exception handling: Integer.parseInt() at lines 53–54 will throw NumberFormatException if nakama.grpcPort or nakama.wsPort contain non-numeric values. Wrap with try-catch or use a helper method to log and fall back to defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaConfig.java`
around lines 49 - 58, NakamaConfig is unused and its fromSystemProperties() also
risks NumberFormatException; remove the dead NakamaConfig class entirely (it’s
not referenced by NakamaSubSystem.initialise()) and delete
fromSystemProperties() to avoid dead code, or if you prefer to keep
functionality, update the fromSystemProperties() method to validate/parses ports
safely by using Integer.parseInt inside a try-catch (or a helper
parseWithDefault method) for nakama.grpcPort and nakama.wsPort, log parse
errors, and fall back to the default port values; reference: class NakamaConfig,
method fromSystemProperties(), and caller NakamaSubSystem.initialise().
| config.setGrpcPort(Integer.parseInt(System.getProperty("nakama.grpcPort", "7349"))); | ||
| config.setWsPort(Integer.parseInt(System.getProperty("nakama.wsPort", "7350"))); |
There was a problem hiding this comment.
Integer.parseInt() can throw NumberFormatException for malformed system properties.
If nakama.grpcPort or nakama.wsPort contain non-numeric values, this will crash. Consider wrapping in try-catch or using a helper that returns the default on parse failure.
🛡️ Proposed defensive parsing
- config.setGrpcPort(Integer.parseInt(System.getProperty("nakama.grpcPort", "7349")));
- config.setWsPort(Integer.parseInt(System.getProperty("nakama.wsPort", "7350")));
+ config.setGrpcPort(parseIntOrDefault(System.getProperty("nakama.grpcPort"), 7349));
+ config.setWsPort(parseIntOrDefault(System.getProperty("nakama.wsPort"), 7350));Add a helper method:
private static int parseIntOrDefault(String value, int defaultValue) {
if (value == null || value.isEmpty()) {
return defaultValue;
}
try {
return Integer.parseInt(value);
} catch (NumberFormatException e) {
return defaultValue;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaConfig.java`
around lines 53 - 54, The current direct calls to Integer.parseInt in
NakamaConfig
(config.setGrpcPort(Integer.parseInt(System.getProperty("nakama.grpcPort",
"7349"))) and
config.setWsPort(Integer.parseInt(System.getProperty("nakama.wsPort", "7350"))))
can throw NumberFormatException; add a defensive helper (e.g., private static
int parseIntOrDefault(String value, int defaultValue)) in NakamaConfig that
returns the default when the value is null, empty, or unparsable, and replace
those Integer.parseInt calls to use
parseIntOrDefault(System.getProperty("nakama.grpcPort"), 7349) and
parseIntOrDefault(System.getProperty("nakama.wsPort"), 7350).
| private void connect() { | ||
| try { | ||
| String deviceId = getOrCreateDeviceId(); | ||
| client = new DefaultClient("defaultkey", autoConfig.host.get(), autoConfig.grpcPort.get(), false); | ||
| session = client.authenticateDevice(deviceId).get(); | ||
| logger.info("Nakama: authenticated as {}", session.getUserId()); | ||
|
|
||
| socket = client.createSocket(autoConfig.host.get(), autoConfig.wsPort.get(), false); | ||
| socket.connect(session, new AbstractSocketListener() { | ||
| @Override | ||
| public void onChannelMessage(ChannelMessage message) { | ||
| handleIncomingMessage(message); | ||
| } | ||
| }).get(); | ||
|
|
||
| // Join the shared chat channel | ||
| channel = socket.joinChat(autoConfig.channel.get(), ChannelType.ROOM).get(); | ||
| logger.info("Nakama: joined channel '{}'", autoConfig.channel.get()); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.warn("Nakama: connection failed, continuing without cross-game chat", e); | ||
| cleanup(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocking .get() calls during connect() may hang engine initialization.
The authenticateDevice(), connect(), and joinChat() calls all use unbounded .get() waits. If the Nakama server is unreachable or slow, these will block the game engine initialization indefinitely.
Consider using .get(timeout, TimeUnit) or moving connection to a background thread with a callback to set the socket/channel atomically once ready.
Proposed fix: Add timeouts to blocking calls
- session = client.authenticateDevice(deviceId).get();
+ session = client.authenticateDevice(deviceId).get(10, TimeUnit.SECONDS);
logger.info("Nakama: authenticated as {}", session.getUserId());
socket = client.createSocket(autoConfig.host.get(), autoConfig.wsPort.get(), false);
socket.connect(session, new AbstractSocketListener() {
`@Override`
public void onChannelMessage(ChannelMessage message) {
handleIncomingMessage(message);
}
- }).get();
+ }).get(10, TimeUnit.SECONDS);
// Join the shared chat channel
- channel = socket.joinChat(autoConfig.channel.get(), ChannelType.ROOM).get();
+ channel = socket.joinChat(autoConfig.channel.get(), ChannelType.ROOM).get(10, TimeUnit.SECONDS);Add import:
import java.util.concurrent.TimeUnit;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void connect() { | |
| try { | |
| String deviceId = getOrCreateDeviceId(); | |
| client = new DefaultClient("defaultkey", autoConfig.host.get(), autoConfig.grpcPort.get(), false); | |
| session = client.authenticateDevice(deviceId).get(); | |
| logger.info("Nakama: authenticated as {}", session.getUserId()); | |
| socket = client.createSocket(autoConfig.host.get(), autoConfig.wsPort.get(), false); | |
| socket.connect(session, new AbstractSocketListener() { | |
| @Override | |
| public void onChannelMessage(ChannelMessage message) { | |
| handleIncomingMessage(message); | |
| } | |
| }).get(); | |
| // Join the shared chat channel | |
| channel = socket.joinChat(autoConfig.channel.get(), ChannelType.ROOM).get(); | |
| logger.info("Nakama: joined channel '{}'", autoConfig.channel.get()); | |
| } catch (Exception e) { | |
| logger.warn("Nakama: connection failed, continuing without cross-game chat", e); | |
| cleanup(); | |
| } | |
| } | |
| private void connect() { | |
| try { | |
| String deviceId = getOrCreateDeviceId(); | |
| client = new DefaultClient("defaultkey", autoConfig.host.get(), autoConfig.grpcPort.get(), false); | |
| session = client.authenticateDevice(deviceId).get(10, TimeUnit.SECONDS); | |
| logger.info("Nakama: authenticated as {}", session.getUserId()); | |
| socket = client.createSocket(autoConfig.host.get(), autoConfig.wsPort.get(), false); | |
| socket.connect(session, new AbstractSocketListener() { | |
| `@Override` | |
| public void onChannelMessage(ChannelMessage message) { | |
| handleIncomingMessage(message); | |
| } | |
| }).get(10, TimeUnit.SECONDS); | |
| // Join the shared chat channel | |
| channel = socket.joinChat(autoConfig.channel.get(), ChannelType.ROOM).get(10, TimeUnit.SECONDS); | |
| logger.info("Nakama: joined channel '{}'", autoConfig.channel.get()); | |
| } catch (Exception e) { | |
| logger.warn("Nakama: connection failed, continuing without cross-game chat", e); | |
| cleanup(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`
around lines 83 - 106, The connect() method currently blocks engine init by
calling authenticateDevice().get(), socket.connect(...).get(), and
joinChat(...).get() without timeouts; change these to use get(timeout, TimeUnit)
(add import java.util.concurrent.TimeUnit) with a reasonable timeout for each
call and handle TimeoutException/InterruptedException/ExecutionException to call
cleanup() and log appropriately, or alternatively run the entire connect() logic
off the main thread (e.g., submit to an executor) and set session/socket/channel
atomically on success so authenticateDevice, socket.connect, and joinChat no
longer block initialization; update exception handling around
authenticateDevice, socket.connect, and joinChat to avoid indefinite blocking
and ensure cleanup() is called on failure.
| if (incomingMessageHandler != null) { | ||
| suppressOutbound = true; | ||
| try { | ||
| incomingMessageHandler.accept(formatted); | ||
| } finally { | ||
| suppressOutbound = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
suppressOutbound flag has a race condition between socket thread and game thread.
handleIncomingMessage() is called from the Nakama socket thread, setting suppressOutbound = true, calling the handler, then setting it back to false. Meanwhile, sendChatMessage() reads suppressOutbound on the game thread. If a chat event fires during handler execution but after the flag is reset to false, the message may be re-forwarded.
For a POC this is acceptable, but production code should use proper synchronization or a thread-local/request-scoped approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`
around lines 125 - 132, The suppressOutbound flag has a race between the Nakama
socket thread (handleIncomingMessage where
incomingMessageHandler.accept(formatted) is invoked) and the game thread
(sendChatMessage), so change the usage to synchronize access: introduce a
private final lock object (e.g., suppressLock) and wrap the set-to-true, handler
invocation, and set-to-false inside a synchronized(suppressLock) block in
handleIncomingMessage, and also read the flag inside a
synchronized(suppressLock) block in sendChatMessage (or replace the boolean with
the same lock-guarded check), ensuring both threads use the same lock when
accessing suppressOutbound to eliminate the race.
| @Override | ||
| public void registerSystems(ComponentSystemManager componentSystemManager) { | ||
| if (autoConfig != null && autoConfig.enabled.get()) { | ||
| NakamaSystem nakamaSystem = new NakamaSystem(); | ||
| nakamaSystem.setNakamaSubSystem(this); | ||
| componentSystemManager.register(nakamaSystem); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate NakamaSystem registration—see comment in NakamaSystem.java.
As noted in the review of NakamaSystem.java, this manually registers a NakamaSystem instance while the @RegisterSystem annotation on the class causes auto-discovery of a separate instance. Remove the annotation from NakamaSystem to fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`
around lines 168 - 175, The NakamaSystem is being registered twice because
NakamaSubSystem.registerSystems manually creates and registers a NakamaSystem
while the NakamaSystem class is also marked with `@RegisterSystem` for
auto-discovery; remove the `@RegisterSystem` annotation from the NakamaSystem
class (or alternatively stop manual registration) so only one instance is
created—open the NakamaSystem class, delete the `@RegisterSystem` annotation
declaration, and keep the manual registration in NakamaSubSystem.registerSystems
as the single registration path.
| public boolean isConnected() { | ||
| return socket != null && channel != null; | ||
| } |
There was a problem hiding this comment.
isConnected() is not thread-safe with cleanup().
isConnected() reads socket and channel without synchronization while cleanup() (called from preShutdown()) nulls them. This could lead to isConnected() returning true followed by NPE in sendChatMessage(). The null checks in sendChatMessage() provide some protection, but the pattern is fragile.
For a POC this is acceptable; production code should use a single atomic state variable or proper locking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSubSystem.java`
around lines 201 - 203, isConnected() reads socket and channel without
synchronization while cleanup() (invoked from preShutdown()) can null them,
causing a race that may let isConnected() return true then cause NPEs in
sendChatMessage(); fix by introducing a single thread-safe connection state
(e.g., an AtomicBoolean connectionOpen or by synchronizing access) and use that
in isConnected(), cleanup(), and sendChatMessage() instead of directly reading
socket/channel, or surround reads/writes of socket and channel with a consistent
lock to prevent the race between isConnected(), cleanup(), and
sendChatMessage().
| @RegisterSystem | ||
| public class NakamaSystem extends BaseComponentSystem { | ||
| private NakamaSubSystem nakamaSubSystem; | ||
|
|
||
| public void setNakamaSubSystem(NakamaSubSystem subsystem) { | ||
| this.nakamaSubSystem = subsystem; | ||
| } |
There was a problem hiding this comment.
Potential duplicate system registration due to @RegisterSystem annotation.
The @RegisterSystem annotation causes the engine to auto-discover and instantiate this class during loadSystems(). However, NakamaSubSystem.registerSystems() also manually creates and registers a NakamaSystem instance. This results in two separate instances: one auto-discovered (without nakamaSubSystem set) and one manually registered (with nakamaSubSystem wired). The auto-discovered instance will silently swallow chat events because its nakamaSubSystem field is null.
Either remove @RegisterSystem and rely solely on manual registration, or use context.get() to look up NakamaSubSystem in initialise() instead of setter injection.
Proposed fix: Remove `@RegisterSystem`
-@RegisterSystem
public class NakamaSystem extends BaseComponentSystem {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @RegisterSystem | |
| public class NakamaSystem extends BaseComponentSystem { | |
| private NakamaSubSystem nakamaSubSystem; | |
| public void setNakamaSubSystem(NakamaSubSystem subsystem) { | |
| this.nakamaSubSystem = subsystem; | |
| } | |
| public class NakamaSystem extends BaseComponentSystem { | |
| private NakamaSubSystem nakamaSubSystem; | |
| public void setNakamaSubSystem(NakamaSubSystem subsystem) { | |
| this.nakamaSubSystem = subsystem; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/main/java/org/terasology/subsystem/nakama/NakamaSystem.java`
around lines 18 - 24, The class-level `@RegisterSystem` on NakamaSystem causes an
auto-instantiated system that lacks the NakamaSubSystem wiring (set via
setNakamaSubSystem), producing duplicate instances because
NakamaSubSystem.registerSystems() also manually registers NakamaSystem; fix by
either removing the `@RegisterSystem` annotation from the NakamaSystem class so
only the manually-registered instance (created in
NakamaSubSystem.registerSystems()) exists, or keep `@RegisterSystem` but remove
setter injection and instead obtain the dependency inside
NakamaSystem.initialise() using context.get(NakamaSubSystem.class) (and remove
or ignore setNakamaSubSystem) so the discovered instance is correctly wired.
| assertNotNull(session.getUserId(), "Should receive a user ID after auth"); | ||
| assertFalse(session.IsExpired(), "Session should not be expired"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual method signature in Nakama SDK for Session
# Search for IsExpired or isExpired methods
ast-grep --pattern 'class Session {
$$$
$_Expired($$$) {
$$$
}
$$$
}'
# Alternatively, search in gradle dependencies or documentation
rg -i 'isexpired|IsExpired' --type javaRepository: MovingBlocks/Terasology
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check imports and the Session class definition in the test file
head -100 subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaIntegrationTest.java | grep -E "^import|class|Session"Repository: MovingBlocks/Terasology
Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Search for Nakama SDK dependencies in build files
fd -e gradle -e pom\.xml | xargs grep -l -i nakamaRepository: MovingBlocks/Terasology
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for Session class definition in the codebase
rg 'class Session' --type java -A 5Repository: MovingBlocks/Terasology
Length of output: 2058
🏁 Script executed:
#!/bin/bash
# Find the Nakama SDK version in build configuration
find subsystems/Nakama -name "build.gradle" -o -name "pom.xml" | xargs cat | grep -A 2 -B 2 -i nakamaRepository: MovingBlocks/Terasology
Length of output: 49
🌐 Web query:
Nakama SDK Java Session IsExpired method documentation
💡 Result:
The Nakama Java SDK provides a Session interface (com.heroiclabs.nakama.ISession or similar) with an isExpired method or property that checks if the session token has expired. This is used to determine if the session is still valid before making requests to the Nakama server. From official documentation and analogous SDKs: - It is a boolean getter indicating whether the session has expired compared to the current time. - Recommended to check at app startup: store the auth token and verify if expired; if so, reauthenticate or restore. - Related methods include hasExpired(DateTime offset) for checking against a specific time, and isRefreshExpired for the refresh token. - Usage example pattern (from Java docs and .NET equivalent): if (session.isExpired) { // reauth } Full API docs: https://heroiclabs.github.io/nakama-java/ (Javadoc for nakama-java 2.5.0 and later). Source code is in the GitHub repo https://github.com/heroiclabs/nakama-java, where Session is implemented as DefaultSession with JWT parsing to compute expiry from the 'exp' claim against current Unix time. Always store session.getAuthToken persistently and check isExpired on restore via DefaultSession.restore(authToken, refreshToken).
Citations:
- 1: https://github.com/heroiclabs/nakama-java/blob/master/README.md
- 2: https://heroiclabs.com/docs/nakama/concepts/session/
- 3: https://dotnet.docs.heroiclabs.com/html/class_satori_1_1_session.html
- 4: https://heroiclabs.com/docs/nakama/client-libraries/java/
- 5: https://heroiclabs.com/docs/nakama/client-libraries/java/index.html
- 6: https://heroiclabs.github.io/nakama-java/
🌐 Web query:
site:github.com/heroiclabs/nakama-java Session class isExpired method
💡 Result:
The Session class isExpired method appears in the context of the Nakama game server Java client SDK from Heroic Labs. The official README recommends storing the authentication token from the session and checking at startup if it has expired using the isExpired method. If expired, reauthenticate is required. No explicit method signature or implementation details were found in the searched sources, but the usage implies Session has a boolean-returning isExpired method to check session token expiry.
Citations:
🌐 Web query:
nakama-java DefaultSession isExpired source code
💡 Result:
The source code for nakama-java's DefaultSession class, specifically the isExpired method, is available in the official GitHub repository at https://github.com/heroiclabs/nakama-java. DefaultSession implements the Session interface and provides a static restore method for sessions from auth tokens, as shown in the README: Session session = DefaultSession.restore(authtoken); Direct source code snippets for the isExpired implementation were not found in search results, but analogous implementations in other Nakama clients (e.g., .NET Session.IsExpired property compares ExpireTime UNIX timestamp to current time using Epoch DateTime(1970,1,1); Dart Session.isExpired getter) suggest it checks if the session's expire time (from JWT) has passed current time. The full API documentation is at https://heroiclabs.github.io/nakama-java/, and latest release v2.5.3 (Dec 2024) includes session auto-refresh logic. Check the repository's src/main/java/com/heroiclabs/nakama/session/DefaultSession.java for exact code, likely: public boolean isExpired { return System.currentTimeMillis / 1000 >= expireTime; }
Citations:
- 1: https://github.com/heroiclabs/nakama-java/blob/master/README.md
- 2: https://heroiclabs.com/docs/nakama/client-libraries/java/
- 3: https://dotnet.docs.heroiclabs.com/html/class_nakama_1_1_session.html
- 4: https://pub.dev/documentation/nakama/latest/nakama/Session-class.html
- 5: https://github.com/heroiclabs/nakama-java/releases
- 6: https://github.com/heroiclabs/nakama-java
- 7: https://heroiclabs.github.io/nakama-java/
🏁 Script executed:
#!/bin/bash
# Search for gradle/maven files with nakama in subsystems/Nakama directory
find subsystems/Nakama -type f \( -name "*.gradle" -o -name "*.gradle.kts" -o -name "pom.xml" \) -exec cat {} \;Repository: MovingBlocks/Terasology
Length of output: 1331
🏁 Script executed:
#!/bin/bash
# Check if the test file has any skip annotations or is known to fail
grep -B 5 "NakamaIntegrationTest\|class NakamaIntegrationTest\|IsExpired" subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaIntegrationTest.java | head -50Repository: MovingBlocks/Terasology
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about method naming in nakama integration
find subsystems/Nakama -type f \( -name "*.java" -o -name "*.md" -o -name "*.txt" \) -exec grep -l "IsExpired\|isExpired" {} \;Repository: MovingBlocks/Terasology
Length of output: 157
Change session.IsExpired() to session.isExpired().
The Nakama Java SDK (version 2.5.3) uses isExpired() with lowercase 'i' to follow Java naming conventions. The capital 'I' in IsExpired() is the .NET convention and does not exist in the Java SDK.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@subsystems/Nakama/src/test/java/org/terasology/subsystem/nakama/NakamaIntegrationTest.java`
around lines 64 - 65, The test uses the wrong method name for session expiry
check: replace the call to session.IsExpired() with the Java SDK method
session.isExpired() in NakamaIntegrationTest (update the assertion using
session.isExpired()). Ensure the assertion remains
assertFalse(session.isExpired(), "Session should not be expired") so it compiles
with Nakama Java SDK 2.5.3.
1 participant
Not for merging, at least not yet! Details at https://siliconsaga.net/bifrost-first-contact/