Draft
Conversation
Bios-Marcel
force-pushed
the
restart_lobby_resurrection
branch
from
36fe9f6 to
6a110da
Compare
Bios-Marcel
force-pushed
the
restart_lobby_resurrection
branch
from
6a110da to
c28fb6f
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/game/data.go (1)
167-175:⚠️ Potential issue | 🔴 CriticalCritical bug:
GetPlayerByIDalways returns the first player.Line 169 compares
player.ID == player.IDinstead ofplayer.ID == id, making the condition always true and returning the first player in the list regardless of the requested ID.🐛 Proposed fix
func (lobby *Lobby) GetPlayerByID(id uuid.UUID) *Player { for _, player := range lobby.Players { - if player.ID == player.ID { + if player.ID == id { return player } } return nil }
🤖 Fix all issues with AI agents
In `@internal/api/v1.go`:
- Around line 95-112: In resurrectLobby, validate that data.Lobby is non-nil
before calling lobby methods: after decoding into game.LobbyRestoreData, check
if data.Lobby == nil and if so log the problem and respond with
http.Error(writer, http.StatusText(http.StatusBadRequest),
http.StatusBadRequest) to return 400; only then assign lobby := data.Lobby and
call lobby.Synchronized, state.ResurrectLobby,
lobby.WriteObject/WritePreparedMessage and lobby.ResurrectUnsynchronized as
currently implemented.
- Around line 95-112: The resurrectLobby handler currently decodes base64 JSON
into LobbyRestoreData and calls state.ResurrectLobby without authenticity or TTL
checks; update the flow to validate a cryptographic signature and TTL on the
payload before any state change: extend LobbyRestoreData to include a signature
and timestamp, decode the payload (as done via base64Decoder), verify the
timestamp is within allowed TTL, compute/verify an HMAC or signature using the
server-side secret/public key and the canonical payload (exclude the signature
field) and reject with 401/400 if verification fails, and only then call
state.ResurrectLobby and ResurrectUnsynchronized; keep checks centralized in
resurrectLobby so WriteObject, WritePreparedMessage and the lobby mutex are only
applied after successful validation.
- Around line 95-100: The resurrectLobby handler reads an unbounded request body
via base64.NewDecoder and json.NewDecoder which can lead to memory exhaustion;
wrap request.Body with an io.LimitReader (e.g., limit to a reasonable max like a
few MB) before creating base64.NewDecoder, validate decoding errors and return a
4xx (Bad Request) for payload/decoding issues instead of 500, and ensure you
close or consume the remaining body as appropriate; update references in the
function (resurrectLobby, base64Decoder, json.NewDecoder, and the error handling
branch) to enforce the size cap and return the proper 4xx response on invalid
payloads.
In `@internal/frontend/index.js`:
- Around line 15-16: The TODO-like comment indicates uncertainty about the input
size attribute; either remove the attribute or replace the vague comment with a
clear justification: if the input element (variable input) must display 4
characters by default for UI/UX reasons, update the comment to state that (e.g.,
"set size=4 to show 4 chars for X reason"), otherwise remove the
input.setAttribute("size","4") call; locate the occurrence of
input.setAttribute("size", "4") and apply the change and update the inline
comment accordingly.
- Around line 19-21: The three calls using Number.toString(...) are wrong
because Number.toString is a prototype function; update the input.setAttribute
calls that use Number.toString for the variables value, min, and max to convert
those numeric variables to strings properly (e.g., use String(value) or
value.toString()) so input.setAttribute("value", ...), input.setAttribute("min",
...), and input.setAttribute("max", ...) receive correct string representations;
locate the statements with input.setAttribute in internal/frontend/index.js and
replace the Number.toString usages with a proper string conversion for value,
min, and max.
In `@internal/frontend/lobby.js`:
- Around line 19-40: The catch block and socket.onerror currently call
connectToWebsocket() directly, which can cause immediate recursive calls when
WebSocket construction fails; modify both the try/catch exception handler and
the socket.onerror path (inside the readyState != 1 branch where
connectToWebsocket() is called) to schedule reconnect attempts via setTimeout
and implement simple backoff (e.g., track a retryDelay variable or attempts
counter on the module scope and increase it up to a max), ensure
socketIsConnecting is set appropriately before scheduling, and reset backoff on
a successful connection in the connectToWebsocket or onopen handler so retries
are not tight or recursive.
- Around line 59-66: The fetch to resurrect the lobby uses a hardcoded
'/v1/lobby/resurrect' path which breaks proxy/root-path deployments; update the
call in the block where restoreData is truthy and event.reason === "lobby_gone"
(around the fetch and socketIsConnecting handling) to prepend the application
rootPath (e.g. use rootPath + '/v1/lobby/resurrect' or a template string
`${rootPath}/v1/lobby/resurrect`) so the endpoint respects custom root paths
like other API calls.
In `@internal/game/data.go`:
- Line 109: The comment "Since we don't know how long the restart took, we
extend all timers.\\" contains a trailing backslash typo; remove the backslash
so the comment reads "Since we don't know how long the restart took, we extend
all timers." (look for that exact comment in internal/game/data.go and update
it).
- Around line 106-123: The method ResurrectUnsynchronized currently assumes
lobby.Wordpack maps into WordlistData and calls
WordlistData[lobby.Wordpack].Lowercaser(), which can panic if Wordpack is
empty/invalid; update ResurrectUnsynchronized to validate lobby.Wordpack and the
lookup in WordlistData before calling Lowercaser (e.g., check lobby.Wordpack !=
"" and that the map lookup yields a non-nil/valid LanguageData), and handle the
error path by setting a safe default lowercaser, logging the issue, or returning
early so no nil method is invoked (refer to symbols: ResurrectUnsynchronized,
lobby.Wordpack, WordlistData, Lowercaser).
In `@internal/game/lobby.go`:
- Around line 1159-1167: The shutdown loop can panic if a player's websocket is
nil; update the loop over lobby.Players to skip players where player.ws == nil
before calling player.ws.Async() and player.ws.WriteClose(1012,
[]byte("server_restart")). Use the existing waitGroup (sync.WaitGroup) logic but
only call waitGroup.Add for the number of non-nil connections or decrement with
waitGroup.Done inside the branch that actually spawns Async; ensure references
to lobby.Players, player.ws, Async, and WriteClose are what you modify so
disconnected players are safely skipped.
In `@internal/game/shared.go`:
- Around line 241-245: The LobbySettings struct has an inconsistent JSON tag
usage: add a json tag for the Wordpack field to match ScoreCalculationIdentifier
so marshaling yields a predictable key; update the Wordpack field in the
LobbySettings definition (alongside ScoreCalculationIdentifier and
EditableLobbySettings) to include a json:"wordpack" tag to ensure consistent
serialization of the resurrection payload.
In `@internal/state/lobbies.go`:
- Around line 123-126: The hard-coded 500ms sleep used as a "Graceperiod" (the
time.Sleep(500 * time.Millisecond) call and its preceding comment) should be
made configurable or better documented: introduce a named constant or a
configurable field (e.g., LobbyShutdownGracePeriod or a field on the
Lobbies/config struct) and replace the literal with that symbol, or expand the
comment to explain why 500ms was chosen and under what conditions it can be
changed; update any shutdown/Close/Shutdown function that contains the
time.Sleep to read the value from the new constant/field and add a short unit
test or comment verifying socket flush behavior if applicable.
- Around line 74-85: ResurrectLobby currently acquires a read lock
(globalStateMutex.RLock) but may call addLobby which mutates the shared lobbies
slice, causing a data race; change the locking so that mutation happens under a
write lock: use globalStateMutex.Lock() (and defer Unlock()) when you need to
call addLobby, or perform the check under RLock and if missing upgrade by
releasing RLock and re-checking under Lock before calling addLobby; ensure
getLobby is still safe under the chosen lock strategy and that addLobby is only
invoked while holding the write lock.
Comment on lines
+95
to
+112
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | ||
| var data game.LobbyRestoreData | ||
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | ||
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | ||
| log.Println("Error unmarshalling lobby resurrection data:", err) | ||
| http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| lobby := data.Lobby | ||
| // We add the lobby, while the lobby mutex is aqcuired. This prevents us | ||
| // from attempting to connect to the lobby, before the internal state has | ||
| // been restored correctly. | ||
| lobby.Synchronized(func() { | ||
| if state.ResurrectLobby(lobby) { | ||
| lobby.WriteObject = WriteObject | ||
| lobby.WritePreparedMessage = WritePreparedMessage | ||
| lobby.ResurrectUnsynchronized(&data) |
There was a problem hiding this comment.
Guard against nil lobby payloads before calling methods.
If data.Lobby is nil, lobby.Synchronized will panic and allow a trivial POST to crash the server. Validate and return 400 before use.
🐛 Proposed fix
lobby := data.Lobby
+ if lobby == nil {
+ http.Error(writer, "missing lobby payload", http.StatusBadRequest)
+ return
+ }📝 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.
Suggested change
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | |
| var data game.LobbyRestoreData | |
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | |
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | |
| log.Println("Error unmarshalling lobby resurrection data:", err) | |
| http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | |
| return | |
| } | |
| lobby := data.Lobby | |
| // We add the lobby, while the lobby mutex is aqcuired. This prevents us | |
| // from attempting to connect to the lobby, before the internal state has | |
| // been restored correctly. | |
| lobby.Synchronized(func() { | |
| if state.ResurrectLobby(lobby) { | |
| lobby.WriteObject = WriteObject | |
| lobby.WritePreparedMessage = WritePreparedMessage | |
| lobby.ResurrectUnsynchronized(&data) | |
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | |
| var data game.LobbyRestoreData | |
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | |
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | |
| log.Println("Error unmarshalling lobby resurrection data:", err) | |
| http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | |
| return | |
| } | |
| lobby := data.Lobby | |
| if lobby == nil { | |
| http.Error(writer, "missing lobby payload", http.StatusBadRequest) | |
| return | |
| } | |
| // We add the lobby, while the lobby mutex is aqcuired. This prevents us | |
| // from attempting to connect to the lobby, before the internal state has | |
| // been restored correctly. | |
| lobby.Synchronized(func() { | |
| if state.ResurrectLobby(lobby) { | |
| lobby.WriteObject = WriteObject | |
| lobby.WritePreparedMessage = WritePreparedMessage | |
| lobby.ResurrectUnsynchronized(&data) |
🤖 Prompt for AI Agents
In `@internal/api/v1.go` around lines 95 - 112, In resurrectLobby, validate that
data.Lobby is non-nil before calling lobby methods: after decoding into
game.LobbyRestoreData, check if data.Lobby == nil and if so log the problem and
respond with http.Error(writer, http.StatusText(http.StatusBadRequest),
http.StatusBadRequest) to return 400; only then assign lobby := data.Lobby and
call lobby.Synchronized, state.ResurrectLobby,
lobby.WriteObject/WritePreparedMessage and lobby.ResurrectUnsynchronized as
currently implemented.
🧩 Analysis chain
🏁 Script executed:
rg -n -C2 "Resurrect|LobbyRestoreData|ShutdownTime|signature|HMAC|jwt|token|verify" internalRepository: scribble-rs/scribble.rs
Length of output: 3174
🏁 Script executed:
rg -n "resurrectLobby|/v1/lobby/resurrect" internal/apiRepository: scribble-rs/scribble.rs
Length of output: 285
🏁 Script executed:
cat internal/api/http.go | head -100Repository: scribble-rs/scribble.rs
Length of output: 3716
🏁 Script executed:
rg -n "SetupRoutes" --type go internalRepository: scribble-rs/scribble.rs
Length of output: 690
🏁 Script executed:
fd -e "*.go" -path "*/cmd/*" -o -path "*/main*" | head -10Repository: scribble-rs/scribble.rs
Length of output: 235
🏁 Script executed:
rg -n "\.SetupRoutes\|http\.HandleFunc\|http\.Handle" --type go | head -20Repository: scribble-rs/scribble.rs
Length of output: 49
🏁 Script executed:
find . -name "main.go" -o -name "*main*" | grep -v node_modules | head -5Repository: scribble-rs/scribble.rs
Length of output: 212
🏁 Script executed:
rg -n "SetupRoutes" --type-list | head -5Repository: scribble-rs/scribble.rs
Length of output: 456
🏁 Script executed:
git ls-files | grep -E "(main|cmd)" | head -10Repository: scribble-rs/scribble.rs
Length of output: 202
🏁 Script executed:
cat cmd/scribblers/main.go | head -200Repository: scribble-rs/scribble.rs
Length of output: 2859
Validate cryptographic signature and TTL before accepting resurrection payloads.
The endpoint accepts base64-decoded JSON payloads without any cryptographic verification. The LobbyRestoreData struct contains no signature field, and ResurrectUnsynchronized only adjusts timing without validating authenticity. This allows any client to forge arbitrary lobby state, including player data and game progress, without restriction.
Add HMAC or signature validation to the payload and verify it before calling state.ResurrectLobby().
🤖 Prompt for AI Agents
In `@internal/api/v1.go` around lines 95 - 112, The resurrectLobby handler
currently decodes base64 JSON into LobbyRestoreData and calls
state.ResurrectLobby without authenticity or TTL checks; update the flow to
validate a cryptographic signature and TTL on the payload before any state
change: extend LobbyRestoreData to include a signature and timestamp, decode the
payload (as done via base64Decoder), verify the timestamp is within allowed TTL,
compute/verify an HMAC or signature using the server-side secret/public key and
the canonical payload (exclude the signature field) and reject with 401/400 if
verification fails, and only then call state.ResurrectLobby and
ResurrectUnsynchronized; keep checks centralized in resurrectLobby so
WriteObject, WritePreparedMessage and the lobby mutex are only applied after
successful validation.
Comment on lines
+95
to
+100
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | ||
| var data game.LobbyRestoreData | ||
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | ||
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | ||
| log.Println("Error unmarshalling lobby resurrection data:", err) | ||
| http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) |
There was a problem hiding this comment.
Limit resurrection payload size to prevent memory abuse.
The handler currently reads an unbounded request body through base64+JSON decoding. Cap the size to avoid memory exhaustion and return a 4xx on invalid payloads.
🛡️ Proposed fix
func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) {
var data game.LobbyRestoreData
+ const maxRestorePayloadBytes = 1 << 20 // 1 MiB (tune as needed)
+ request.Body = http.MaxBytesReader(writer, request.Body, maxRestorePayloadBytes)
base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body)
if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil {
log.Println("Error unmarshalling lobby resurrection data:", err)
- http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+ http.Error(writer, "invalid resurrection payload", http.StatusBadRequest)
return
}📝 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.
Suggested change
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | |
| var data game.LobbyRestoreData | |
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | |
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | |
| log.Println("Error unmarshalling lobby resurrection data:", err) | |
| http.Error(writer, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) | |
| func (handler *V1Handler) resurrectLobby(writer http.ResponseWriter, request *http.Request) { | |
| var data game.LobbyRestoreData | |
| const maxRestorePayloadBytes = 1 << 20 // 1 MiB (tune as needed) | |
| request.Body = http.MaxBytesReader(writer, request.Body, maxRestorePayloadBytes) | |
| base64Decoder := base64.NewDecoder(base64.StdEncoding, request.Body) | |
| if err := json.NewDecoder(base64Decoder).Decode(&data); err != nil { | |
| log.Println("Error unmarshalling lobby resurrection data:", err) | |
| http.Error(writer, "invalid resurrection payload", http.StatusBadRequest) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/api/v1.go` around lines 95 - 100, The resurrectLobby handler reads
an unbounded request body via base64.NewDecoder and json.NewDecoder which can
lead to memory exhaustion; wrap request.Body with an io.LimitReader (e.g., limit
to a reasonable max like a few MB) before creating base64.NewDecoder, validate
decoding errors and return a 4xx (Bad Request) for payload/decoding issues
instead of 500, and ensure you close or consume the remaining body as
appropriate; update references in the function (resurrectLobby, base64Decoder,
json.NewDecoder, and the error handling branch) to enforce the size cap and
return the proper 4xx response on invalid payloads.
Comment on lines
+15
to
+16
| // Figure out why I did this exactly. | ||
| input.setAttribute("size", "4"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing or addressing the TODO comment.
The comment "Figure out why I did this exactly" suggests uncertainty about the size="4" attribute. Consider documenting the reason or removing the attribute if it's unnecessary.
🤖 Prompt for AI Agents
In `@internal/frontend/index.js` around lines 15 - 16, The TODO-like comment
indicates uncertainty about the input size attribute; either remove the
attribute or replace the vague comment with a clear justification: if the input
element (variable input) must display 4 characters by default for UI/UX reasons,
update the comment to state that (e.g., "set size=4 to show 4 chars for X
reason"), otherwise remove the input.setAttribute("size","4") call; locate the
occurrence of input.setAttribute("size", "4") and apply the change and update
the inline comment accordingly.
Comment on lines
+19
to
+21
| input.setAttribute("value", Number.toString(value)); | ||
| input.setAttribute("min", Number.toString(min)); | ||
| input.setAttribute("max", Number.toString(max)); |
There was a problem hiding this comment.
Number.toString() is used incorrectly; attributes will receive wrong values.
Number.toString is a prototype method, not a static method. Calling Number.toString(value) ignores the argument and returns "function toString() { [native code] }" or similar, not the stringified number.
🐛 Proposed fix
- input.setAttribute("value", Number.toString(value));
- input.setAttribute("min", Number.toString(min));
- input.setAttribute("max", Number.toString(max));
+ input.setAttribute("value", String(value));
+ input.setAttribute("min", String(min));
+ input.setAttribute("max", String(max));Alternatively, use value.toString(), min.toString(), max.toString().
📝 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.
Suggested change
| input.setAttribute("value", Number.toString(value)); | |
| input.setAttribute("min", Number.toString(min)); | |
| input.setAttribute("max", Number.toString(max)); | |
| input.setAttribute("value", String(value)); | |
| input.setAttribute("min", String(min)); | |
| input.setAttribute("max", String(max)); |
🤖 Prompt for AI Agents
In `@internal/frontend/index.js` around lines 19 - 21, The three calls using
Number.toString(...) are wrong because Number.toString is a prototype function;
update the input.setAttribute calls that use Number.toString for the variables
value, min, and max to convert those numeric variables to strings properly
(e.g., use String(value) or value.toString()) so input.setAttribute("value",
...), input.setAttribute("min", ...), and input.setAttribute("max", ...) receive
correct string representations; locate the statements with input.setAttribute in
internal/frontend/index.js and replace the Number.toString usages with a proper
string conversion for value, min, and max.
Comment on lines
+19
to
+40
| try { | ||
| socket = new WebSocket(`${rootPath}/v1/lobby/ws`); | ||
| } catch (exception) { | ||
| console.log("Connection error:" + exception) | ||
| socketIsConnecting = false; | ||
| connectToWebsocket(); | ||
| return; | ||
| } | ||
|
|
||
| socket.onerror = error => { | ||
| //Is not connected and we haven't yet said that we are done trying to | ||
| //connect, this means that we could never even establish a connection. | ||
| if (socket.readyState != 1 && !hasSocketEverConnected) { | ||
| if (socket.readyState != 1) { | ||
| socketIsConnecting = false; | ||
| showTextDialog("connection-error-dialog", | ||
| '{{.Translation.Get "error-connecting"}}', | ||
| `{{.Translation.Get "error-connecting-text"}}`); | ||
| console.log("Error establishing connection: ", error); | ||
| if (!hasSocketEverConnected) { | ||
| showTextDialog("connection-error-dialog", | ||
| '{{.Translation.Get "error-connecting"}}', | ||
| `{{.Translation.Get "error-connecting-text"}}`); | ||
| console.log("Error establishing connection: ", error); | ||
| } else { | ||
| connectToWebsocket(); | ||
| } |
There was a problem hiding this comment.
Avoid immediate recursive reconnects; schedule retries instead.
Calling connectToWebsocket() directly in the catch/onerror path can recurse and loop tightly if the constructor fails synchronously, risking stack overflow and CPU thrash. Schedule a retry (and ideally add backoff).
🛠️ Proposed fix
} catch (exception) {
console.log("Connection error:" + exception)
socketIsConnecting = false;
- connectToWebsocket();
+ setTimeout(connectToWebsocket, 1000);
return;
}
...
if (!hasSocketEverConnected) {
showTextDialog("connection-error-dialog",
'{{.Translation.Get "error-connecting"}}',
`{{.Translation.Get "error-connecting-text"}}`);
console.log("Error establishing connection: ", error);
} else {
- connectToWebsocket();
+ setTimeout(connectToWebsocket, 1000);
}🤖 Prompt for AI Agents
In `@internal/frontend/lobby.js` around lines 19 - 40, The catch block and
socket.onerror currently call connectToWebsocket() directly, which can cause
immediate recursive calls when WebSocket construction fails; modify both the
try/catch exception handler and the socket.onerror path (inside the readyState
!= 1 branch where connectToWebsocket() is called) to schedule reconnect attempts
via setTimeout and implement simple backoff (e.g., track a retryDelay variable
or attempts counter on the module scope and increase it up to a max), ensure
socketIsConnecting is set appropriately before scheduling, and reset backoff on
a successful connection in the connectToWebsocket or onopen handler so retries
are not tight or recursive.
| func (lobby *Lobby) ResurrectUnsynchronized(restoreData *LobbyRestoreData) { | ||
| lobby.lowercaser = WordlistData[lobby.Wordpack].Lowercaser() | ||
|
|
||
| // Since we don't know how long the restart took, we extend all timers.\ |
There was a problem hiding this comment.
Typo: trailing backslash in comment.
The comment has a trailing backslash that appears to be a typo.
📝 Proposed fix
- // Since we don't know how long the restart took, we extend all timers.\
+ // Since we don't know how long the restart took, we extend all timers.📝 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.
Suggested change
| // Since we don't know how long the restart took, we extend all timers.\ | |
| // Since we don't know how long the restart took, we extend all timers. |
🤖 Prompt for AI Agents
In `@internal/game/data.go` at line 109, The comment "Since we don't know how long
the restart took, we extend all timers.\\" contains a trailing backslash typo;
remove the backslash so the comment reads "Since we don't know how long the
restart took, we extend all timers." (look for that exact comment in
internal/game/data.go and update it).
Comment on lines
+1159
to
+1167
| var waitGroup sync.WaitGroup | ||
| waitGroup.Add(len(lobby.Players)) | ||
| for _, player := range lobby.Players { | ||
| player.ws.Async(func() { | ||
| defer waitGroup.Done() | ||
| player.ws.WriteClose(1012, []byte("server_restart")) | ||
| }) | ||
| } | ||
| waitGroup.Wait() |
There was a problem hiding this comment.
Potential nil pointer dereference in shutdown loop.
If a player is disconnected (player.ws == nil), calling player.ws.Async() will panic. The loop should check for nil websocket connections.
🐛 Proposed fix
var waitGroup sync.WaitGroup
- waitGroup.Add(len(lobby.Players))
for _, player := range lobby.Players {
+ if player.ws == nil {
+ continue
+ }
+ waitGroup.Add(1)
player.ws.Async(func() {
defer waitGroup.Done()
player.ws.WriteClose(1012, []byte("server_restart"))
})
}
waitGroup.Wait()🤖 Prompt for AI Agents
In `@internal/game/lobby.go` around lines 1159 - 1167, The shutdown loop can panic
if a player's websocket is nil; update the loop over lobby.Players to skip
players where player.ws == nil before calling player.ws.Async() and
player.ws.WriteClose(1012, []byte("server_restart")). Use the existing waitGroup
(sync.WaitGroup) logic but only call waitGroup.Add for the number of non-nil
connections or decrement with waitGroup.Done inside the branch that actually
spawns Async; ensure references to lobby.Players, player.ws, Async, and
WriteClose are what you modify so disconnected players are safely skipped.
Comment on lines
+241
to
245
| type LobbySettings struct { | ||
| Wordpack string | ||
| ScoreCalculationIdentifier string `json:"scoreCalculationIdentifier"` | ||
| EditableLobbySettings | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a JSON tag for Wordpack for consistency.
ScoreCalculationIdentifier has a JSON tag, but Wordpack does not. If this struct is serialized for the resurrection payload, the inconsistency may cause unexpected field naming (wordpack vs Wordpack depending on marshaler settings).
♻️ Suggested change
type LobbySettings struct {
- Wordpack string
+ Wordpack string `json:"wordpack"`
ScoreCalculationIdentifier string `json:"scoreCalculationIdentifier"`
EditableLobbySettings
}📝 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.
Suggested change
| type LobbySettings struct { | |
| Wordpack string | |
| ScoreCalculationIdentifier string `json:"scoreCalculationIdentifier"` | |
| EditableLobbySettings | |
| } | |
| type LobbySettings struct { | |
| Wordpack string `json:"wordpack"` | |
| ScoreCalculationIdentifier string `json:"scoreCalculationIdentifier"` | |
| EditableLobbySettings | |
| } |
🤖 Prompt for AI Agents
In `@internal/game/shared.go` around lines 241 - 245, The LobbySettings struct has
an inconsistent JSON tag usage: add a json tag for the Wordpack field to match
ScoreCalculationIdentifier so marshaling yields a predictable key; update the
Wordpack field in the LobbySettings definition (alongside
ScoreCalculationIdentifier and EditableLobbySettings) to include a
json:"wordpack" tag to ensure consistent serialization of the resurrection
payload.
Comment on lines
+74
to
+85
| func ResurrectLobby(lobby *game.Lobby) bool { | ||
| globalStateMutex.RLock() | ||
| defer globalStateMutex.RUnlock() | ||
|
|
||
| existingLobby := getLobby(lobby.LobbyID) | ||
| if existingLobby == nil { | ||
| addLobby(lobby) | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Data race: write operation performed under read lock.
ResurrectLobby acquires a read lock (RLock) but calls addLobby, which mutates the lobbies slice. This causes a data race with concurrent readers and is undefined behavior.
🐛 Proposed fix
func ResurrectLobby(lobby *game.Lobby) bool {
- globalStateMutex.RLock()
- defer globalStateMutex.RUnlock()
+ globalStateMutex.Lock()
+ defer globalStateMutex.Unlock()
existingLobby := getLobby(lobby.LobbyID)
if existingLobby == nil {
addLobby(lobby)
return true
}
return false
}📝 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.
Suggested change
| func ResurrectLobby(lobby *game.Lobby) bool { | |
| globalStateMutex.RLock() | |
| defer globalStateMutex.RUnlock() | |
| existingLobby := getLobby(lobby.LobbyID) | |
| if existingLobby == nil { | |
| addLobby(lobby) | |
| return true | |
| } | |
| return false | |
| } | |
| func ResurrectLobby(lobby *game.Lobby) bool { | |
| globalStateMutex.Lock() | |
| defer globalStateMutex.Unlock() | |
| existingLobby := getLobby(lobby.LobbyID) | |
| if existingLobby == nil { | |
| addLobby(lobby) | |
| return true | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
In `@internal/state/lobbies.go` around lines 74 - 85, ResurrectLobby currently
acquires a read lock (globalStateMutex.RLock) but may call addLobby which
mutates the shared lobbies slice, causing a data race; change the locking so
that mutation happens under a write lock: use globalStateMutex.Lock() (and defer
Unlock()) when you need to call addLobby, or perform the check under RLock and
if missing upgrade by releasing RLock and re-checking under Lock before calling
addLobby; ensure getLobby is still safe under the chosen lock strategy and that
addLobby is only invoked while holding the write lock.
Comment on lines
+123
to
126
|
|
||
| // Graceperiod to make sure sockets can flush everything. | ||
| time.Sleep(500 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the grace period configurable or documenting its purpose.
The 500ms sleep blocks the shutdown goroutine. While the comment explains it's for socket flushing, this magic number could be made configurable or at least documented with more context about why 500ms was chosen.
🤖 Prompt for AI Agents
In `@internal/state/lobbies.go` around lines 123 - 126, The hard-coded 500ms sleep
used as a "Graceperiod" (the time.Sleep(500 * time.Millisecond) call and its
preceding comment) should be made configurable or better documented: introduce a
named constant or a configurable field (e.g., LobbyShutdownGracePeriod or a
field on the Lobbies/config struct) and replace the literal with that symbol, or
expand the comment to explain why 500ms was chosen and under what conditions it
can be changed; update any shutdown/Close/Shutdown function that contains the
time.Sleep to read the value from the new constant/field and add a short unit
test or comment verifying socket flush behavior if applicable.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/game/data.go (1)
167-175:⚠️ Potential issue | 🔴 CriticalCritical:
GetPlayerByIDalways returns the first player due to comparison bug.Line 169 compares
player.ID == player.IDwhich is always true, causing the function to return the first player in the slice regardless of the inputid.🐛 Proposed fix
func (lobby *Lobby) GetPlayerByID(id uuid.UUID) *Player { for _, player := range lobby.Players { - if player.ID == player.ID { + if player.ID == id { return player } } return nil }
🤖 Fix all issues with AI agents
In `@internal/api/ws.go`:
- Around line 53-61: When lobby == nil, handle WriteClose errors and always
close the websocket: after calling upgrader.Upgrade(writer, request) defer or
ensure socket.Close() is called to guarantee cleanup, then capture the error
returned by socket.WriteClose(1000, []byte("lobby_gone")), log that error (e.g.
with the package logger or log.Printf) and, if WriteClose fails, still call
socket.Close() (or ensure the deferred Close runs) before returning; update the
block around upgrader.Upgrade and socket.WriteClose to perform error checking
and explicit socket cleanup.
In `@internal/frontend/lobby.js`:
- Around line 1022-1031: Replace the FIXME and hard-coded English messages in
the shutdown handling block by using localized translation keys instead of
literal strings: when parsed.data exists set restoreData as before, and call
showDialog("shutdown-dialog", t("shutdown.title"),
document.createTextNode(t("shutdown.restoreMessage"))) else call
showDialog("shutdown-dialog", t("shutdown.title"),
document.createTextNode(t("shutdown.genericMessage"))); remove the FIXME
comment; add the keys shutdown.title, shutdown.restoreMessage and
shutdown.genericMessage to the app's translations map (and provide translated
text for each locale) so showDialog and restoreData logic remain unchanged.
In `@internal/game/lobby.go`:
- Around line 46-47: startWordChoiceTimer currently always invokes selectWord
after wordChoiceDurationMs which causes errors if a word was already chosen or
the lobby ended; modify startWordChoiceTimer to check the lobby state (e.g.,
whether a word is already chosen or the lobby is closed) before calling
selectWord and ensure any timers are canceled/cleared when a word is picked or
when the lobby transitions to ended state. Locate and update
startWordChoiceTimer and any timer setup in the lobby (references:
startWordChoiceTimer, selectWord, wordChoiceDurationMs) to store the timer
handle, guard the callback with a short-circuit check (wordChosen/ended flag)
and cancel the timer in the code paths that pick a word or end the lobby so
selectWord is never invoked spuriously.
Comment on lines
53
to
61
| if lobby == nil { | ||
| http.Error(writer, ErrLobbyNotExistent.Error(), http.StatusNotFound) | ||
| socket, err := upgrader.Upgrade(writer, request) | ||
| if err != nil { | ||
| http.Error(writer, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| socket.WriteClose(1000, []byte("lobby_gone")) | ||
| return | ||
| } |
There was a problem hiding this comment.
Handle WriteClose errors and ensure socket cleanup.
When the lobby is not found, the code upgrades the connection and writes a close frame, but:
- The error from
WriteCloseis ignored - The socket may not be fully closed if
WriteClosefails
Consider logging errors and ensuring the socket is closed.
🛡️ Proposed fix
if lobby == nil {
socket, err := upgrader.Upgrade(writer, request)
if err != nil {
http.Error(writer, err.Error(), http.StatusInternalServerError)
return
}
- socket.WriteClose(1000, []byte("lobby_gone"))
+ if err := socket.WriteClose(1000, []byte("lobby_gone")); err != nil {
+ log.Printf("error writing close frame: %v", err)
+ }
return
}📝 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.
Suggested change
| if lobby == nil { | |
| http.Error(writer, ErrLobbyNotExistent.Error(), http.StatusNotFound) | |
| socket, err := upgrader.Upgrade(writer, request) | |
| if err != nil { | |
| http.Error(writer, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| socket.WriteClose(1000, []byte("lobby_gone")) | |
| return | |
| } | |
| if lobby == nil { | |
| socket, err := upgrader.Upgrade(writer, request) | |
| if err != nil { | |
| http.Error(writer, err.Error(), http.StatusInternalServerError) | |
| return | |
| } | |
| if err := socket.WriteClose(1000, []byte("lobby_gone")); err != nil { | |
| log.Printf("error writing close frame: %v", err) | |
| } | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@internal/api/ws.go` around lines 53 - 61, When lobby == nil, handle
WriteClose errors and always close the websocket: after calling
upgrader.Upgrade(writer, request) defer or ensure socket.Close() is called to
guarantee cleanup, then capture the error returned by socket.WriteClose(1000,
[]byte("lobby_gone")), log that error (e.g. with the package logger or
log.Printf) and, if WriteClose fails, still call socket.Close() (or ensure the
deferred Close runs) before returning; update the block around upgrader.Upgrade
and socket.WriteClose to perform error checking and explicit socket cleanup.
Comment on lines
+1022
to
+1031
| console.log("Shutdown event received"); | ||
| if (parsed.data) { | ||
| restoreData = parsed.data; | ||
| // FIXMe Text anpassen! | ||
| showDialog("shutdown-dialog", "Server shutting down", | ||
| document.createTextNode("Sorry, but the server is about to shut down. Attempting to restore lobby on restart ...")); | ||
| } else { | ||
| showDialog("shutdown-dialog", "Server shutting down", | ||
| document.createTextNode("Sorry, but the server is about to shut down. Please come back at a later time.")); | ||
| } |
There was a problem hiding this comment.
Resolve the FIXME and localize shutdown messaging.
There’s a FIXMe and hardcoded English strings; please replace with translation keys or finalized copy.
If you want, I can draft localized strings and wire them into the translation map.
🤖 Prompt for AI Agents
In `@internal/frontend/lobby.js` around lines 1022 - 1031, Replace the FIXME and
hard-coded English messages in the shutdown handling block by using localized
translation keys instead of literal strings: when parsed.data exists set
restoreData as before, and call showDialog("shutdown-dialog",
t("shutdown.title"), document.createTextNode(t("shutdown.restoreMessage"))) else
call showDialog("shutdown-dialog", t("shutdown.title"),
document.createTextNode(t("shutdown.genericMessage"))); remove the FIXME
comment; add the keys shutdown.title, shutdown.restoreMessage and
shutdown.genericMessage to the app's translations map (and provide translated
text for each locale) so showDialog and restoreData logic remain unchanged.
Comment on lines
+46
to
47
| wordChoiceDurationMs = 30000 | ||
| ) |
There was a problem hiding this comment.
Guard or cancel the word-choice timer to avoid spurious errors.
startWordChoiceTimer always calls selectWord, which will error once a word has already been chosen or the lobby ends. This can generate noise and hides real issues.
🛠️ Suggested guard
func (lobby *Lobby) startWordChoiceTimer(durationMs int64) {
timer := time.NewTimer(time.Duration(durationMs) * time.Millisecond)
<-timer.C
lobby.mutex.Lock()
defer lobby.mutex.Unlock()
- if err := lobby.selectWord(int(lobby.preSelectedWord)); err != nil {
+ if lobby.State != Ongoing || len(lobby.wordChoice) == 0 {
+ return
+ }
+ if err := lobby.selectWord(int(lobby.preSelectedWord)); err != nil {
log.Println("Error automatically selecting word:", err)
}
}Also applies to: 594-678, 680-691
🤖 Prompt for AI Agents
In `@internal/game/lobby.go` around lines 46 - 47, startWordChoiceTimer currently
always invokes selectWord after wordChoiceDurationMs which causes errors if a
word was already chosen or the lobby ended; modify startWordChoiceTimer to check
the lobby state (e.g., whether a word is already chosen or the lobby is closed)
before calling selectWord and ensure any timers are canceled/cleared when a word
is picked or when the lobby transitions to ended state. Locate and update
startWordChoiceTimer and any timer setup in the lobby (references:
startWordChoiceTimer, selectWord, wordChoiceDurationMs) to store the timer
handle, guard the callback with a short-circuit check (wordChosen/ended flag)
and cancel the timer in the code paths that pick a word or end the lobby so
selectWord is never invoked spuriously.
Member
Author
|
@copilot PLease do a strict review of this PR with potential commitable change suggestions |
|
@Bios-Marcel I've opened a new pull request, #396, to work on those changes. Once the pull request is ready, I'll request review from you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO
An issue I encountered is, that the current implementation becomes useless once a user manually pressed F5. To solve this, a custom lobby gone page is required OR the lobby needs to be able to show even for an empty lobby.
The latter would be more consistent UI wise. But this means that upon resurrection, the lobby needs to reload, as some of the templated state won't be available.
Fixes #371
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes