Workaround Premature EOF error in live replays (and some other fixes)#1152
Merged
Gatsik merged 12 commits intoFAForever:developfrom Jan 25, 2026
Merged
Workaround Premature EOF error in live replays (and some other fixes)#1152Gatsik merged 12 commits intoFAForever:developfrom
Premature EOF error in live replays (and some other fixes)#1152Gatsik merged 12 commits intoFAForever:developfrom
Conversation
but not hosting another one this way it is possible to generate new maps for the next game faster
📝 WalkthroughWalkthroughAdds Windows-only named-pipe live replay streaming (StreamWriter, LiveReplayStreamer), integrates it into replay start flow and GameRunner/ClientWindow, adds a replay filters UI with splitter, a pipe-path config and settings toggle, and small UI/model/chat tweaks (filters, COOP, sticky chat scroll). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as ClientWindow
participant Runner as GameRunner
participant Streamer as LiveReplayStreamer
participant Writer as StreamWriter
participant Server as ReplayServer
participant Pipe as NamedPipe
User->>Client: double-click live replay
Client->>Runner: request start_live_replay(gurl)
Runner->>Streamer: start_live_replay(gurl)
Streamer->>Streamer: check platform & game/pipe_live_replay
alt Windows + pipe enabled
Streamer->>Writer: create & start writer
Writer->>Server: connect via WebSocket
Server-->>Writer: send replay data
Writer->>Pipe: write data chunks
Server-->>Writer: send terminator
Writer->>Pipe: final write & close
Writer-->>Streamer: writer_closed
else Fallback
Streamer->>Runner: fall back to in-process replay handling
end
Streamer-->>Client: notify streaming started/closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@res/client/client.css`:
- Around line 384-387: The CSS selector currently targets QLineEdit#mapName and
any widget with id playerName; narrow it to only QLineEdit widgets by changing
the second selector to QLineEdit#playerName so both selectors read
QLineEdit#mapName and QLineEdit#playerName, ensuring only QLineEdit instances
are styled.
In `@src/fa/game_runner.py`:
- Around line 69-76: The early return that checks game.has_live_replay and
Settings.get("game/pipe_live_replay", True, type=bool) happens before the
GameState.OPEN/PLAYING branch and thus prevents _join_game_from_url(gurl) from
running; move the live-replay delay guard into the PLAYING branch so OPEN games
can still be joined by URL and only playing games invoke
game.warn_live_delay(self._client_window) (call that check inside the
GameState.PLAYING branch before calling
_live_replay_streamer.start_live_replay(gurl)); if the delay should always apply
regardless of the Settings flag, remove the Settings.get(...) gate and always
call game.warn_live_delay(...) in the PLAYING branch.
In `@src/model/game.py`:
- Around line 306-316: In warn_live_delay, guard against self.launched_at being
None and clamp the computed remaining seconds to >=0 before calling time.gmtime:
check if self.launched_at is None and handle (e.g., treat remaining as
LIVE_REPLAY_DELAY_SECS), compute remaining = max(0, self.LIVE_REPLAY_DELAY_SECS
- (time.time() - self.launched_at or 0)), then use that clamped remaining when
building wait_str; update references in warn_live_delay to avoid passing None
into time.gmtime and ensure the message shows a non-negative wait time.
In `@src/replays/_replayswidget.py`:
- Around line 1193-1203: The function _startReplay currently returns early if
the named player is missing before checking game_url, preventing start when a
valid game_url is provided; modify _startReplay to check if game_url is not None
first and call self.client.live_replay_streamer.start_live_replay(game_url)
immediately, and only perform self._playerset.get_by_name(name) and subsequent
player.currentGame handling when game_url is None, keeping calls to
player.currentGame.url(player.id) and start_live_replay unchanged.
🧹 Nitpick comments (3)
src/util/settings_menus.py (1)
402-402: Avoid persisting a Windows-only setting on non-Windows.With the checkbox disabled off‑Windows,
save_settingsstill writesgame/pipe_live_replay=False, which can unintentionally override defaults if the config is later used on Windows. Consider gating the write.♻️ Suggested guard
- Settings.set("game/pipe_live_replay", self.ui.liveReplaysWorkaround.isChecked()) + if sys.platform == "win32": + Settings.set("game/pipe_live_replay", self.ui.liveReplaysWorkaround.isChecked())src/games/_gameswidget.py (1)
239-247: Remove direct manipulation of replays widget lists (or route through the widget).
process_mod_infodirectly mutatesself.client.replays.modListandself.client.replays.featuredModComboBoxat lines 241–246 and 254–255. While the initialization order ensuresreplaysexists before these methods are called, the tight coupling violates separation of concerns—the FIXME already flags this. Consider adding a public method on the replays widget to handle these updates, allowingGamesWidgetto call it indirectly rather than directly manipulating replays' internals.Alternatively, if direct access is unavoidable for now, guard against future initialization changes with
getattr(self.client, "replays", None)checks.res/replays/replays.ui (1)
37-187: Add the new filter controls to the tab order.The new filter widgets don’t appear in the
<tabstops>list, so keyboard navigation may skip them. Consider adding them for accessibility.
1. it pollutes logs 2. all server messages are logged with the same level (debug)
[Windows only] Game is not able to process lua structures larger than 2047 bytes through TCP socket FAForever can send a lot of lobby options in replay header, which results in breaking live replays The drawback of this workaround is a sudden stop of a replay at the end
with 99 the ':' sometimes went to the next line
Gatsik
force-pushed
the
live-replay-fix
branch
from
05f6985 to
149cdc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@res/replays/replays.ui`:
- Around line 37-187: Add the new filter widgets to the UI tab order by adding
them to the <tabstops> section so they are keyboard-focusable; include
gameTypeComboBox, featuredModComboBox, maxPlayersComboBox, numPlayersComboBox,
and hideModdedCheckBox in the tab order (ensure the sequence makes sense for
accessibility and navigation). Use the exact widget names (gameTypeComboBox,
featuredModComboBox, maxPlayersComboBox, numPlayersComboBox, hideModdedCheckBox)
when adding entries to <tabstops> so the QSplitter/QGroupBox receives proper tab
navigation.
In `@src/fa/replaylivestreamer.py`:
- Around line 186-193: In on_writer_ready, if replay(self.game_url) returns
False you must explicitly close the socket/writer before clearing its reference;
update the failure branch to call the writer shutdown/close routine on
self.writer (e.g., self.writer.close(), and await/wait_closed if it’s an async
StreamWriter) and then call self.on_writer_closed() (or ensure on_writer_closed
is invoked after the close completes) so the socket is not left open and
unreferenced.
- Around line 79-91: The ConnectNamedPipe return handling in
replaylivestreamer.py is incorrect: change the logic around
win32pipe.ConnectNamedPipe (currently stored in self.pipe_connected) to accept
both a 0 return and win32file.ERROR_PIPE_CONNECTED as successful connections,
and only treat other non-zero return codes as failures; on failure log the
corrected message ("Could not connect named pipe. ConnectNamedPipe returned
%s"), call win32file.CloseHandle(pipe), emit self._close.emit() and return.
Locate uses of CreateNamedPipe, self.pipe_connected, and ConnectNamedPipe to
implement this change and import or reference win32file.ERROR_PIPE_CONNECTED as
needed.
In `@src/replays/_replayswidget.py`:
- Around line 250-253: The splitter size restoration uses splitter_sizes from
Settings.get_list but only checks len(splitter) == 2 before calling
self.splitter.setSizes, which can pass an empty or mismatched list and collapse
panes; update the guard to ensure splitter_sizes is non-empty and its length
matches the splitter's child count (compare len(splitter_sizes) to len(splitter)
or self.splitter.count()) before calling setSizes on self.splitter, so only a
valid sizes list is applied.
♻️ Duplicate comments (2)
src/model/game.py (1)
306-317: Guard against missing launch time and clamp remaining delay.
assertcan be stripped, andlaunched_atcan still beNone; also the remaining time can go negative. Consider a safe guard with clamping beforetime.gmtime.🐛 Proposed fix
def warn_live_delay(self, parent: QWidget | None = None) -> None: - assert self.launched_at is not None - delta = time.gmtime(self.LIVE_REPLAY_DELAY_SECS - (time.time() - self.launched_at)) + if self.launched_at is None: + QMessageBox.information( + parent, + "5 Minute Live Game Delay", + "Live replay timing isn't available yet.", + ) + return + remaining = max(self.LIVE_REPLAY_DELAY_SECS - (time.time() - self.launched_at), 0) + delta = time.gmtime(remaining) wait_str = time.strftime('%M Min %S Sec', delta)src/fa/game_runner.py (1)
72-75: Live‑replay delay should block regardless ofpipe_live_replay.When
pipe_live_replayis disabled andhas_live_replayis false, the code still attempts to start a replay, which is likely to fail for the same “premature” reason. Consider guarding solely onhas_live_replayand return after warning.🛠️ Proposed fix
- elif game.state == GameState.PLAYING: - if not game.has_live_replay and Settings.get("game/pipe_live_replay", True, type=bool): - game.warn_live_delay(self._client_window) - else: - self._live_replay_streamer.start_live_replay(gurl) + elif game.state == GameState.PLAYING: + if not game.has_live_replay: + game.warn_live_delay(self._client_window) + return + self._live_replay_streamer.start_live_replay(gurl)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/chat/channel_widget.py`:
- Around line 233-243: The append_line method moves the QTextCursor to End via
cursor.movePosition and self.chat_area.setTextCursor which can cause the view to
scroll and emit valueChanged before _sticky_scroll.save_scroll() has captured
whether the view was already at bottom; adjust save/restore so the
"was-at-bottom" state is captured prior to any cursor/scroll-changing calls (or
temporarily disable tracking while inserting). Specifically, update the logic
around _sticky_scroll.save_scroll()/_sticky_scroll.restore_scroll() used by
append_line and the similar block at lines 343-360 so save_scroll records the
current _is_set_to_maximum (or equivalent), then perform cursor.movePosition and
setTextCursor and insertHtml, and finally have restore_scroll consult the saved
flag to decide whether to jump to bottom or leave the user scroll position alone
(alternatively, have save_scroll disable valueChanged handling during insertion
and re-enable it in restore_scroll).
🧹 Nitpick comments (1)
src/chat/channel_widget.py (1)
365-365: Markmin_as intentionally unused.
min_isn’t referenced; consider renaming to_minor_to make intent explicit and avoid unused-arg noise while keeping the two-arg slot signature.Please confirm the connected signal always passes two args (e.g.,
rangeChanged(int, int)), so renaming the unused one is safe.
StickyScroll is still needed though, because it restores scrollbar position on ladder warning visibility changes see e307388
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Style
✏️ Tip: You can customize this high-level summary in your review settings.