Skip to content

Fix client instability / crashes when zoning frequently - #1384

Open
GeekOfWires wants to merge 9 commits into
psforever:masterfrom
GeekOfWires:fix/zoning-players
Open

Fix client instability / crashes when zoning frequently#1384
GeekOfWires wants to merge 9 commits into
psforever:masterfrom
GeekOfWires:fix/zoning-players

Conversation

@GeekOfWires

Copy link
Copy Markdown
Contributor

Summary

Rapid, repeated continent transfers could destabilize or crash the game client. This branch bundles four independent fixes that close the concrete server-side races and resource limits behind that symptom. Each fix was developed on its own branch and is a separate commit here, so they can be reviewed (or reverted) individually.

Root theme: several parts of the zone-transfer path assumed a transfer runs to completion before the next one starts. When a second transfer begins mid-flight, timers fire on the wrong thread, stale async responses get applied, duplicate object-creates are sent, a transient vehicle reference leaks into the next transfer, and the reliable-packet resend window overflows — any of which can wedge or crash the client.

What's changed

1. Run zoning respawn/countdown timers on the actor thread
respawnTimer (LoadZonePhysicalSpawnPoint) and zoningTimer (beginZoningCountdown) used the closure form of scheduleOnce, so their bodies ran on the global ExecutionContext rather than serialized with actor message processing. cancel() cannot stop an already-dispatched closure, so a superseded transfer's timer still fired, and the respawn body read non-volatile session state (player/continent/interstellarFerry) off-thread — emitting ObjectDelete/ObjectCreate against a stale GUID space. Both are converted to the message-delivery form (scheduleOnce(delay, self, msg)), mirroring the existing SetCurrentAvatar pattern, so the bodies run on the actor thread and cancel() is effective.

2. Discard superseded spawn-point responses and duplicate zoning requests

  • Spawn-point requests to the cluster are async and unordered; when a second transfer started before the first's response arrived, the handler's default branch applied the wrong-zone spawn anyway. An opaque correlation token is now added to the ICS Get*SpawnPoint requests and echoed in SpawnPointResponse; handleSpawnPointResponse discards any response a strictly-newer request has superseded. It never drops the latest request's response, so it cannot soft-lock a legitimate transfer, and token 0 (non-session callers) is always accepted — behavior for those paths is unchanged.
  • Two rapid transfers could queue two BeginZoningMessages; the second re-sent ObjectCreate for entities the client already held ("object already exists" → crash). handleBeginZoning now ignores a BeginZoningMessage when zoneLoaded is already Some(true); a legitimate new load always resets zoneLoaded to None first.

3. Clear the transient interstellarFerry once the avatar respawns
interstellarFerry holds the seated vehicle across a transfer, but was only reset in the vehicle branch of AvatarCreate. A transfer that resolved to an on-foot spawn (droppod landing, a vehicle transfer ending unseated) left it set, and the next infantry re-zone read the stale reference and routed into LoadZoneInVehicle against a vehicle absent from the destination zone — attaching the avatar to a phantom parent GUID. It's now reset unconditionally once the avatar exists in the new zone.

4. Make the SMP retransmit history configurable and large enough for zone loads
The middleware kept only the last 100 reliable SlottedMetaPackets for retransmission. A zone load bursts a large number of reliable ObjectCreate packets; when the client requested a resend (RelatedA) of an evicted subslot, the packet was gone ("no longer logged") and the client permanently missed reliable data. smpHistoryLength is promoted to network.middleware.smp-history-length (default 1024) so it comfortably exceeds a single-zone burst and can be tuned per deployment. The RelatedA lookup is also made null-safe (the history ring holds null slots until it fills), and the eviction warning now names the tunable.

Config

New setting in network.middleware:

smp-history-length = 1024

Cost is roughly length × MTU bytes of retained packet data per session.

Testing

  • sbt Compile/compile succeeds on the merged branch.
  • Verified at runtime that the new config key parses (Config.app.network.middleware.smpHistoryLength == 1024).
  • The server boots cleanly (Flyway migrations apply, login + world session establish).
  • Tested with two clients hopping between continents aggressively with substantial facility state changes — held up without the client instability this addresses.

Known limitations / notes for reviewers

  • Not yet tested with a larger number of clients. Verified with two clients under aggressive back-and-forth zoning and substantial facility state changes; behavior under higher concurrency (bigger zone-load bursts, more contention on the spawn path) still wants a scale test before merge.
  • The cross-transfer GUID ordering barrier was intentionally not added: intra-transfer ordering is already handled by taskThenZoneChange, and handleZoneResponse already guards its driven-vehicle path with a seat check. The interstellarFerry cleanup closes the remaining stale-reference route.
  • Unrelated: Test/compile currently fails on a pre-existing error in LocalActionTest.scala (not found: value Default) on master, so the existing suite can't run against this branch as-is.

GeekOfWires and others added 8 commits July 20, 2026 13:11
The respawnTimer (LoadZonePhysicalSpawnPoint) and zoningTimer
(beginZoningCountdown) used the closure form of scheduleOnce, so their
bodies ran on the global ForkJoinPool rather than serialized with actor
message processing. During rapid/frequent zoning this raced the actor:
- respawnTimer.cancel()/zoningTimer.cancel() are best-effort and cannot
  stop an already-dispatched closure, so a superseded transfer's timer
  still fired;
- the respawn closure read live, non-volatile session state
  (player/continent/interstellarFerry) off-thread, producing
  ObjectDelete/ObjectCreate against a stale/half-updated GUID space and
  crashing the client.

Convert both to the message-delivery form scheduleOnce(delay, self, msg),
mirroring the existing SetCurrentAvatar pattern. The countdown continuation
and the resolved respawn destination travel as ZoningCountdownTick /
ZoningSpawnPointRespawn messages handled on the actor thread, making the
cancel() guards genuinely effective and the state reads consistent.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rapid/frequent zoning produced two client-crashing packet sequences:

1. Stale SpawnPointResponse force-load. A spawn-point request to the
   cluster is async and unordered; when a player started transfer B
   before transfer A's response arrived, the handler's default branch
   applied A's (wrong-zone) spawn under B's intent, sending a
   LoadMapMessage/spawn for the wrong zone. The "zoning was not in order"
   branch recognised the disorder but still called resolveZoningSpawnPointLoad.

   Add an opaque correlation token to the ICS Get*SpawnPoint requests,
   echoed verbatim in SpawnPointResponse. Each session-originated request
   is stamped via nextSpawnPointToken() (monotonic per session).
   handleSpawnPointResponse discards any response whose non-zero token is
   not the latest issued — i.e. a strictly newer request has superseded it.
   This never drops the most recent request's response, so it cannot
   soft-lock a legitimate transfer; token 0 (non-session callers) is always
   accepted, preserving existing behaviour.

2. Duplicate ObjectCreate from a second BeginZoningMessage. Two rapid
   transfers queue two BeginZoningMessages; the second re-dumped
   ObjectCreate for every entity in a zone the client had already loaded
   ("object already exists" -> crash). handleBeginZoning now ignores a
   BeginZoningMessage when zoneLoaded is already Some(true); a legitimate
   new load always resets zoneLoaded to None first.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
interstellarFerry holds the seated vehicle across a zone transfer (the
vehicle does not yet exist in the destination zone's GUID space). It is
set on a vehicle/droppod transfer but was only reset to None in the
vehicle branch of AvatarCreate. When a transfer that set it instead
resolved to an on-foot spawn (droppod landing, a vehicle transfer that
ended with the player unseated), the infantry and spectator branches left
it set.

A subsequent infantry re-zone then read that stale reference at the head
of LoadZonePhysicalSpawnPoint (interstellarFerry.orElse(...)) and routed
into LoadZoneInVehicle against a vehicle GUID absent from the destination
zone -- an ObjectCreate attaching the avatar to a phantom parent, which
crashes the client. The field's own doc-comment warns that leaving it set
"prior to a subsequent transfer may cause unstable vehicle associations,
with memory leak potential."

Reset interstellarFerry unconditionally once the avatar has been created
in the new zone, so no later transfer can inherit a stale vehicle
association. The intra-transfer unregister->register ordering is already
sequenced by taskThenZoneChange (the unregister subtask runs before the
FindZone that drives registration), and handleZoneResponse already guards
its driven-vehicle path with a seat check; this closes the remaining
stale-reference route.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The middleware retained only the last 100 reliable SlottedMetaPackets for
retransmission (smpHistoryLength). A zone load bursts a large number of
reliable ObjectCreate packets; when that burst exceeds the window and the
client requests a resend (RelatedA) of an evicted subslot, the packet is
gone ("no longer logged") and the client permanently misses reliable data,
wedging or crashing it. Frequent zoning made this routine because each
transfer is a fresh burst with no quiet period to drain the buffer.

- Promote smpHistoryLength to network.middleware.smp-history-length in
  config (default 1024), so it comfortably exceeds a single-zone burst and
  can be tuned per deployment. Cost is ~(length * MTU) bytes per session.
- Make the RelatedA lookup null-safe: the history ring holds null slots
  until it fills, and .find(_.subslot == ...) dereferenced them, so a
  not-found request before the ring filled could NPE instead of falling
  through to the not-found branches (more exposed with a larger ring).
- Make the eviction warning actionable by naming the tunable.

Verified Config parses the new field at runtime (smpHistoryLength = 1024).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reword the comments added by this branch so they describe the behaviour
of the code as it now stands, rather than contrasting it with the code it
replaced. A reader arriving at these files has no view of the previous
implementation, so that framing carries no meaning for them; the
before-and-after reasoning belongs in the commit messages, where it
already is.

No behavioural change: comments and one configuration comment only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@GeekOfWires

Copy link
Copy Markdown
Contributor Author

Addressed the review note about comments describing the change rather than the current behaviour. Pushed as Improve comment etiquette for our previous work — comments only, no behavioural change, build verified before committing.

Smaller footprint here than on the sibling PRs: 2 files / 9 lines, since most of this branch's comments already described the code as it stands.

  • application.conf — the smp-history-length note referenced "the old fixed value of 100", which means nothing to a reader who never saw it. It now states what the window has to accommodate (a zone load bursts a large number of reliable ObjectCreate packets, and frequent zoning is the case that drives the requirement) without anchoring to a value that no longer exists in the tree.
  • ZoningOperations — the two scaladoc blocks on the countdown and respawn messages were phrased as "carrying this as a message (rather than executing it inside the timer closure) guarantees …". They now state the property directly: the continuation travels as a message so the logic runs on the actor thread, serialized with other session activity, which is what makes a concurrent transfer's cancel() effective and keeps session/continent/interstellarFerry from being read half-updated off the global pool.

The reasoning itself is unchanged and still present — only the framing that depended on knowing the previous implementation was removed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant