Skip to content

feat: pre-play lifecycle timeouts, config keepalive, and favicon caching - #548

Open
carabistouflette wants to merge 3 commits into
Steel-Foundation:masterfrom
carabistouflette:pr4-preplay-lifecycle
Open

feat: pre-play lifecycle timeouts, config keepalive, and favicon caching#548
carabistouflette wants to merge 3 commits into
Steel-Foundation:masterfrom
carabistouflette:pr4-preplay-lifecycle

Conversation

@carabistouflette

@carabistouflette carabistouflette commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Type of change

  • Block implementation
  • Item implementation
  • Command implementation
  • Entity implementation
  • Bug fix
  • New feature
  • Breaking change
  • Refactor / code cleanup
  • Performance improvement

Description

Implements the vanilla pre-play lifecycle:

  • Login deadline: a client still in the login phase 30 seconds after the handshake (vanilla ServerLoginPacketListenerImpl, MAX_TICKS_BEFORE_LOGIN = 600) is kicked with multiplayer.disconnect.slow_login. The read loop wraps packet reads in timeout_at while the deadline is active.
  • Configuration keep-alive: challenges are sent every 15 seconds since the last send, and a challenge left unanswered for a full interval is a disconnect.timeout kick at the next boundary — vanilla ServerCommonPacketListenerImpl.keepConnectionAlive timing. Out-of-order answers are a timeout kick, and accepted answers smooth the round-trip latency that is seeded into the play connection (vanilla CommonListenerCookie.latency()). Uses the existing common CKeepAlive/SKeepAlive packets — no new packet types.
  • Status favicon caching: the favicon is loaded once into Server::favicon at startup (a single warning is logged when the configured path is unreadable), removing blocking filesystem I/O from the per-ping handle_status_request path. The file is validated like vanilla PngInfo: it must be a PNG with a 64x64 IHDR, otherwise it is rejected with the vanilla Couldn't load server icon error.

How this was tested

  • cargo test -p steel-login (22 tests) and cargo test -p steel-core --lib (2467 tests) pass; cargo clippy -r --all-targets clean.
  • New tests: keepalive_decisions_match_vanilla_boundaries and keepalive_answer_smooths_latency_and_rejects_out_of_order pin the vanilla keep-alive boundaries (15 s cadence, unanswered → timeout, out-of-order/duplicate answers rejected, latency smoothing); load_favicon_rejects_invalid_pngs_like_vanilla covers the favicon validation matrix.
  • Manual E2E per the plan: raw socket handshake with intent=Login stalls 32 s → server logs the slow-login kick at t=30 s and closes; status ping answers immediately with the cached favicon (no disk access after startup).

Screenshots / logs

N/A (server-log behavior only).

Checklist

  • Code builds w/o errors or warnings
  • Self-reviewed the diff
  • Docs updated (if applicable) — N/A
  • No leftover debug code / comments

Additional notes

  • Stack: pr4-preplay-lifecycle is stacked on pr3-read-path-hardening.
  • Env: Linux, Rust nightly, Minecraft protocol 776 (0.15.2+mc26.2).

Note: stacked on #547 (pr3-read-path-hardening). Until that merges, this PR's diff includes its commits; the lifecycle changes themselves are the final commits. Incremental diff: carabistouflette/SteelMC@pr3-read-path-hardening...pr4-preplay-lifecycle

@github-actions

Copy link
Copy Markdown

This pull request has conflicts with the base branch "master". Please resolve those so we can test out your changes.

@github-actions

Copy link
Copy Markdown

Conflicts have been resolved! 🎉

…et kicks

- get_raw_packet decodes the length VarInt with a manual 3-byte cap like
  vanilla Varint21FrameDecoder (rejects 'length wider than 21-bit' and
  zero-length frames) instead of an unbounded VarInt read; decode throughput
  measured at +10.5% (criterion, 256-packet stream, p < 0.05)
- compressed packets declaring a size below the server threshold are rejected
  with PacketError::BelowThreshold before inflation, matching vanilla
  CompressionDecoder; the check only applies to nonzero (compressed) payloads
- pre-play connections are kicked with multiplayer.disconnect.invalid_packet
  when packet processing fails instead of logging and continuing
- play connections close when a packet fails to process (desynchronized
  stream) and unrecognized immediate packets log at debug level
- adds packet_reader_framing criterion bench and vanilla-parity regression
  tests for all three framing checks
- login phase gets a 30-second deadline like vanilla
  ServerLoginPacketListenerImpl (MAX_TICKS_BEFORE_LOGIN = 600); expiring
  clients are kicked with multiplayer.disconnect.slow_login
- configuration phase sends vanilla-timed keep-alives (challenge every 15
  idle seconds, disconnect after 30 unanswered) using the existing common
  CKeepAlive/SKeepAlive packets; out-of-order answers are a timeout kick
- favicon is loaded once into Server::favicon at startup (warn once when the
  configured path is unreadable), removing per-ping disk I/O from the status
  request path
- config keep-alive now matches vanilla ServerCommonPacketListenerImpl
  exactly: a challenge every 15 s since the last send, and disconnect.timeout
  at the next boundary (15 s after an unanswered challenge). The previous
  30 s threshold lagged vanilla and landed 30-45 s after the challenge on
  the 15 s tick.
- keep-alive decisions are pure tracker logic (KeepAliveDecision), unit
  tested against the vanilla boundaries including out-of-order answers
- accepted answers smooth the round trip into a latency seeded into the
  play connection, like vanilla's CommonListenerCookie.latency()
- the periodic challenge write and the timeout kick are bounded (1 s): a
  wedged socket closes the connection instead of stalling the read loop
- favicon loading validates the PNG IHDR like vanilla PngInfo (64x64) and
  logs the vanilla 'Couldn't load server icon' error instead of serving
  arbitrary bytes
@carabistouflette
carabistouflette marked this pull request as ready for review August 30, 2026 14:25
connection_update = connection_updates_recv.recv() => {
IncomingEvent::ConnectionUpdate(connection_update)
}
_ = config_keepalive_tick.tick() => IncomingEvent::KeepAliveTick,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this tick can cancel get_raw_packet after some bytes was already read then remaining packet body is parsed as new frame anilla keeps incomplete frame bytes

let mut connection = None;
// One-second ticks give the vanilla 15-second send and timeout boundaries
// one-second resolution on a single shared timer.
let mut config_keepalive_tick = interval(Duration::from_secs(1));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vanilla checks config keepalive every server connection tick not with separate 1s timer timing is different here

match decision {
KeepAliveDecision::None => {}
KeepAliveDecision::Send(challenge) => {
if timeout(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vanilla only queues this keepalive and has no 1s write timeout slow valid client can be closed too early

.on_chunk_batch_received_by_client(packet.desired_chunks_per_tick);
}
ImmediatePlayPacket::Unknown(id) => log::info!("play packet id {id} is not known"),
ImmediatePlayPacket::Unknown(id) => log::debug!("play packet id {id} is not known"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it now debug any reason for it?

}

// Vanilla uses `Util.getMillis()` as the challenge id.
let challenge = SystemTime::now()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Util.getMillis() uses monotonic time, not unix time

// Vanilla uses `Util.getMillis()` as the challenge id.
let challenge = SystemTime::now()
.duration_since(UNIX_EPOCH)
.expect("System time before UNIX EPOCH")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean hm would be an edge case where it could trigger so idk i dont lioe thaz

Ok(ConnectionAction::none())
}
config::S_KEEP_ALIVE => {
let packet = SKeepAlive::read_packet(data)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this new handler accepts keepalive with extra trailing bytes because payload end is not checked vanilla rejects it before handler

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

This pull request has conflicts with the base branch "master". Please resolve those so we can test out your changes.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Conflicts have been resolved! 🎉

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

This pull request has conflicts with the base branch "master". Please resolve those so we can test out your changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants