Add backend player presence tracking foundation - #1550
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51842d2764
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| String server = plugin.getBungeeSettings().getServer(); | ||
| sendPresenceMessage(VotingPluginWire.backendStarted(server)); | ||
| sendPresenceMessage(VotingPluginWire.backendHeartbeat(server)); |
There was a problem hiding this comment.
When the plugin is enabled or hot-reloaded while players are already connected, this announces BackendStarted, which clears that backend's proxy-side presence, but it neither reports Bukkit.getOnlinePlayers() nor initiates a snapshot. Those players do not receive a new AdvancedCoreLoginEvent, so they remain absent from the tracker until they reconnect or an external caller manually requests a snapshot; populate the sessions and report the existing players as part of startup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). startPresenceReporting() now schedules seedOnlinePlayerPresence(), which scans Bukkit.getOnlinePlayers(), creates connection sessions, and reports each existing player after the lifecycle starts.
AI disclosure: This reply was written with assistance from ChatGPT.
| for (SnapshotPresence snapshot : snapshotPlayers.values()) { | ||
| long lastPlayerEvent = lastPlayerEventSequences.getOrDefault(snapshot.uuid, 0L); | ||
| if (lastPlayerEvent > pending.eventWatermark) { | ||
| continue; | ||
| } | ||
| putPresence(new PlayerPresence(snapshot.uuid, snapshot.playerName, normalizedServer, |
There was a problem hiding this comment.
If a different UUID logs in under the same case-insensitive player name after beginSnapshot, this check only considers the snapshot player's UUID. putPresence then resolves the name collision by removing the other UUID even when that presence has a post-watermark event, allowing a stale snapshot to evict the newer live session. Before inserting, also compare the current name owner's last event sequence with the snapshot watermark.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Snapshot application now checks the case-insensitive name owner's lastEventSequence against the snapshot watermark before inserting, so a stale snapshot cannot evict a newer UUID that owns the same name.
AI disclosure: This reply was written with assistance from ChatGPT.
| if (presenceHeartbeatTask != null) { | ||
| presenceHeartbeatTask.cancel(false); | ||
| } | ||
| presenceHeartbeatTask = plugin.getTimer().scheduleAtFixedRate(new Runnable() { |
There was a problem hiding this comment.
During plugin disable, cancel(false) does not stop a heartbeat invocation that has already begun. If that invocation passes its enabled check, is paused, and sends after BackendStopped, the proxy processes the stop and then marks the backend available again; because presence expiration is not scheduled here, the stopped backend can remain available indefinitely. Serialize the final heartbeat with shutdown or disable presence reporting before an invocation can pass the send check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Heartbeat sends and shutdown are serialized by presenceLifecycleLock; shutdown sets presenceReporting = false before cancelling the task and sending BackendStopped, so an in-flight heartbeat cannot pass the enabled check and revive the backend afterward.
AI disclosure: This reply was written with assistance from ChatGPT.
| long sequence = ++eventSequence; | ||
| markBackendAvailable(normalizedServer, now); | ||
| PlayerPresence current = playersByUuid.get(playerUuid); | ||
| if (current == null || !current.getServer().equalsIgnoreCase(normalizedServer) | ||
| || !current.getConnectionId().equals(connectionId)) { |
There was a problem hiding this comment.
When a delayed logout arrives after BackendStopped or backend expiry removed the player, this call marks the backend available before determining that the logout is stale and returning false. The rejected event therefore resurrects an unavailable backend and refreshes its timeout even though no current session was changed; move the availability update after the server and connection checks succeed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Fenced logout events now validate generation, timestamp, server, and connection before calling markBackendAvailable; rejected stale logouts leave backend availability and lastSeen unchanged.
AI disclosure: This reply was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 51842d2764
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| } | ||
|
|
||
| private void markBackendAvailable(String server, long now) { | ||
| BackendState state = backends.computeIfAbsent(serverKey(server), key -> new BackendState(server)); |
There was a problem hiding this comment.
In a proxy deployment, a compromised backend plugin/server or messaging broker can send BackendHeartbeat envelopes containing unlimited distinct server values. Each valid value permanently allocates a new BackendState; it is not checked against configured servers, capped, or removed by expireBackends. This permits low-rate, persistent proxy-heap growth until the shared proxy process is exhausted, crossing the backend-to-proxy trust boundary. Bind the server identity to the authenticated transport source/configuration and cap and evict stale state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Modern heartbeat identities are restricted to configured backend names, authenticated with that backend's unique HMAC secret, and accepted only for the current incarnation. Tracker backend state is capped, with unavailable entries evicted under capacity pressure.
AI disclosure: This reply was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 237077c1df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pending.chunkCount = chunkCount; | ||
| pending.chunks.put(chunkIndex, new ArrayList<>(players)); |
There was a problem hiding this comment.
When a proxy has an active snapshot request, a malformed or compromised backend can declare up to 10,000 chunks and send as many as 100,000 players in each; every chunk passes the per-chunk check and is retained here, while the MAX_SNAPSHOT_PLAYERS aggregate limit is checked only after all chunks arrive. The proxy can therefore exhaust its heap long before reaching that check, so reject a chunk when the pending snapshot's cumulative player count would exceed the limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). applySnapshotChunk() now checks pending.playerCount + players.size() before retaining a chunk. Exceeding the aggregate snapshot player limit cancels the pending snapshot immediately.
AI disclosure: This reply was written with assistance from ChatGPT.
| pendingSnapshots.put(serverKey(normalizedServer), new PendingSnapshot(requestId, eventSequence)); | ||
| prunePlayerEventSequences(); |
There was a problem hiding this comment.
If a backend remains healthy but one snapshot chunk is lost, this pending request has no deadline or cancellation path and remains indefinitely unless another snapshot is manually requested or the backend later stops/expires. While it remains, prunePlayerEventSequences() must retain every post-watermark logout tombstone, so normal player churn can grow lastPlayerEventSequences without bound after a single incomplete response; record an expiry and discard stale pending snapshots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Pending snapshots record their request time and expire after two minutes. Expiry runs during heartbeat, snapshot, and backend-expiration activity, removes the pending request, and releases retained tombstones.
AI disclosure: This reply was written with assistance from ChatGPT.
| presenceHeartbeatTask = plugin.getTimer().scheduleAtFixedRate(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| sendActivePresenceMessage(VotingPluginWire.backendHeartbeat(server)); |
There was a problem hiding this comment.
When BungeeSettings.yml changes Server and the plugin configuration is reloaded, VotingPluginMain.reloadPlugin() reloads the settings but leaves the existing BungeeHandler running. This task therefore keeps heartbeating the server name captured during initial load, while subsequent login, logout, snapshot, and shutdown messages read the new name dynamically; the proxy can expire the new backend and retain the obsolete one. Restart presence reporting or read the current configured identity consistently after reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). reloadPresenceReporting() compares the active server name and captured presence secret with the reloaded configuration; a change stops the old lifecycle and starts a new incarnation, so heartbeats and player events use one consistent identity.
AI disclosure: This reply was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477aeb2256
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| getBungeeHandler().reloadPresenceReporting(); | ||
| } |
There was a problem hiding this comment.
When a reload changes UseBungeecoord from true to false, this branch is skipped, so the existing handler remains in presenceReporting mode and continues sending heartbeats every 30 seconds. Quit events are also gated by the newly false setting, leaving the proxy's existing player sessions stale while the heartbeats prevent backend expiry; explicitly stop or close the handler in the disabled branch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). The reload path now explicitly calls disablePresenceReporting() when UseBungeecoord becomes false, serially stopping heartbeats, announcing shutdown, and clearing backend sessions.
AI disclosure: This reply was written with assistance from ChatGPT.
| public static JsonEnvelope backendStopped(String server) { | ||
| return base(SUB_BACKEND_STOPPED).put(K_SERVER, safe(server)).build(); |
There was a problem hiding this comment.
During an overlapping or rapidly restarted backend deployment, a delayed BackendStopped from the previous process is indistinguishable from one sent by the current process because both carry only the configured server name. If the old stop arrives after the replacement's BackendStarted and login messages, the proxy accepts it and clears the replacement's players and availability; include a per-process incarnation ID in lifecycle, heartbeat, and player messages and reject events from obsolete incarnations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Lifecycle, heartbeat, player, snapshot-request, and snapshot envelopes now carry an automatically generated per-process backendIncarnationId. The tracker accepts replacement only through BackendStarted and rejects retired incarnations.
AI disclosure: This reply was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c9c4936d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (removed != null) { | ||
| uuidByPlayerName.remove(nameKey(removed.getPlayerName()), uuid); | ||
| } | ||
| lastPlayerEventSequences.put(uuid, sequence); |
There was a problem hiding this comment.
When any snapshot is pending, a configured but compromised backend can repeatedly send valid login/logout pairs for distinct UUIDs; every logout adds a post-watermark entry here, while the 100,000-player cap checks only playersByUuid. prunePlayerEventSequences() deliberately retains these tombstones until the snapshot expires, so a high-rate sender can exhaust proxy heap within the two-minute window. Apply an aggregate cap to live players plus tombstones, or cancel pending snapshots before retaining more tombstones.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Live player events and retained tombstones share maxTrackedPlayerEvents. If a new event would exceed that aggregate bound while snapshots are pending, the pending snapshots are cancelled and tombstones are pruned before any additional state is retained.
AI disclosure: This reply was written with assistance from ChatGPT.
| if (current != null && current.getServer().equalsIgnoreCase(normalizedServer) | ||
| && current.getLastSeen() > presenceTimestamp) { |
There was a problem hiding this comment.
When a player has moved to another backend and an older login from the former backend is delayed by the transport, the timestamp check is skipped because the current presence's server differs from normalizedServer. The stale event then reaches putPresence() and overwrites the newer destination-server presence, leaving the tracker pointed at the old backend until another event or snapshot repairs it; compare ordering for the UUID regardless of which server currently owns it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). A login that conflicts with a player/name currently owned by another backend cannot overwrite it directly. The proxy requests a destination snapshot; only a matching live session in that snapshot confirms the handoff, so a delayed old-backend login does not reclaim the player.
AI disclosure: This reply was written with assistance from ChatGPT.
| } else if (backendStartedAt < state.backendStartedAt) { | ||
| return false; |
There was a problem hiding this comment.
The new generation fence still treats backendStartedAt as a strictly increasing identifier, although it is populated from System.currentTimeMillis(). If a backend restarts after its clock moves backward—or two incarnations obtain the same millisecond—the replacement's lifecycle, heartbeat, and player events are rejected as older (or as the stopped generation). In the inspected proxy code expireBackendPresence() has no caller, so this can leave the replacement untracked indefinitely; use a collision-resistant incarnation identity with a restart-order mechanism rather than comparing process wall clocks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Backend generations are identified by collision-resistant incarnation UUIDs and advanced by proxy-local receipt of authenticated BackendStarted; backendStartedAt is retained as metadata but is not ordered across processes, so clock rollback and same-millisecond restarts do not reject a replacement.
AI disclosure: This reply was written with assistance from ChatGPT.
|
|
||
| // Transport listeners may run off the Bukkit thread. Snapshot Bukkit state on | ||
| // the server thread before replying. | ||
| plugin.getBukkitScheduler().runTask(plugin, new Runnable() { |
There was a problem hiding this comment.
With shared Redis or MQTT credentials, a compromised backend can publish repeated snapshot requests naming another configured server and its observed backendStartedAt; the validation only checks envelope fields, and every accepted request schedules a new Bukkit main-thread task. Each task scans all online players and emits snapshot chunks even though the proxy will discard unsolicited responses, so a flood can queue enough main-thread work and broker traffic to stall the victim backend. Authenticate requests as proxy-originated and rate-limit or deduplicate them before scheduling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Snapshot requests are HMAC-authenticated with a unique secret for the target backend and must match its active incarnation. The backend deduplicates request IDs and enforces a 30-second monotonic cooldown before scheduling Bukkit work; the proxy applies the same cooldown before sending.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This content was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418ff08fe2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| retireCurrentIncarnation(state); | ||
| long sequence = ++eventSequence; | ||
| removePlayersOnServer(normalizedServer, sequence); | ||
| pendingSnapshots.remove(key); | ||
| state.backendIncarnationId = backendIncarnationId; |
There was a problem hiding this comment.
On shared MQTT, Redis, or database transports, any backend with the common credentials can publish BackendStarted for another configured server with a fresh UUID. This branch treats every non-retired incarnation as a replacement, clears the legitimate backend's players, and makes its subsequent heartbeats and player events fail the generation check until it restarts. Bind the claimed server to a trusted transport identity or otherwise authenticate lifecycle messages before replacing the current incarnation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Every modern presence envelope is signed with HMAC-SHA256 using a unique PresenceServerSecret for that configured backend. The proxy verifies the signature and claimed server before backendStarted() can replace an incarnation, preventing another backend with only its own secret from switching this server's lifecycle.
AI disclosure: This reply was written with assistance from ChatGPT.
| requestId = backendPlayerPresenceTracker.beginSnapshot(server, requestId, backendIncarnationId, | ||
| backendStartedAt, now); |
There was a problem hiding this comment.
When this public method is called twice for the same backend within 30 seconds, the second beginSnapshot replaces the first pending request, but BungeeHandler's new cooldown rejects the second request. The first response then has the wrong request ID and is discarded (or, if it already completed, the second request remains pending without a response), even though this method returns a valid ID. Reuse or reject an existing pending request, or apply the same cooldown before overwriting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). beginSnapshot() now rejects an existing pending request before changing state, and fenced requests also use a persisted 30-second proxy cooldown. A second call therefore cannot replace the first request ID or create a request the backend will discard.
AI disclosure: This reply was written with assistance from ChatGPT.
| if (fenced ? !markBackendAvailable(normalizedServer, backendIncarnationId, backendStartedAt, | ||
| presenceTimestamp, now) | ||
| : !markBackendAvailable(normalizedServer, now)) { |
There was a problem hiding this comment.
Although logout now validates the event before marking the backend available, this login path still updates lastSeen and sets available before checking whether the player or name owner has a newer timestamp. A delayed same-generation login can therefore extend the lifetime of a dead backend despite being rejected; after expireBackends has cleared its players, the same stale login is accepted outright and resurrects stale presence. Perform the ordering checks before refreshing backend state and retain a backend-level timestamp fence across expiry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Fenced login ordering checks run before capacity allocation or markBackendAvailable. Each backend retains a monotonic player-event timestamp fence even after timeout expiry, so a rejected stale login cannot refresh lastSeen or resurrect cleared presence.
AI disclosure: This reply was written with assistance from ChatGPT.
| PlayerPresence current = playersByUuid.get(playerUuid); | ||
| if (current != null && current.getLastSeen() > presenceTimestamp) { | ||
| return false; |
There was a problem hiding this comment.
When a player legitimately moves between backends whose clocks are skewed, this comparison treats their independently generated wall-clock timestamps as one global ordering. If the source backend's clock is ahead of the destination's, every destination login is rejected until its clock passes the old value, leaving the player assigned to the source even though the destination event is newer. Cross-backend moves need proxy-local ordering or another handoff mechanism rather than comparing backend capture times.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). Timestamps are compared only within the producing backend incarnation. Cross-backend conflicts use proxy-local event watermarks plus a destination snapshot handoff, including source/destination logout race protection, so clock skew between backends does not determine ownership.
AI disclosure: This reply was written with assistance from ChatGPT.
| sendPresenceMessage(VotingPluginWire.backendStarted(server, presenceIncarnationId, presenceStartedAt, | ||
| now)); | ||
| sendPresenceMessage(VotingPluginWire.backendHeartbeat(server, presenceIncarnationId, presenceStartedAt, | ||
| nextPresenceTimestamp())); |
There was a problem hiding this comment.
With the default PLUGINMESSAGING transport, starting or restarting an empty backend cannot deliver these one-shot lifecycle messages because Bukkit plugin messages require an online player as a carrier. When the first player later joins, only its login and subsequent heartbeats are sent; if the proxy still holds the previous incarnation, neither message may replace it, so the new backend and player remain rejected indefinitely. Send BackendStarted again when the first carrier becomes available or queue it until delivery succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). The backend now replays the same authenticated BackendStarted incarnation once the first plugin-message carrier is available, before sending that player's modern login. Startup seeding uses the same replay path for players already online.
AI disclosure: This reply was written with assistance from ChatGPT.
| debug("Login: " + player + "/" + uuid + " " + server); | ||
| login(player, uuid, server); |
There was a problem hiding this comment.
When an extended login is rejected by the generation, server, or timestamp checks, the handler still invokes the legacy login flow unconditionally. If the named player is online on a newer backend, a delayed login from the old backend can therefore run checkOnlineVotes with the stale server, send cached rewards there, mark them delivered, and remove them from the cache even though the tracker rejected the event. Run the legacy path only for three-field legacy envelopes or after the new presence event has been accepted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the current head (9ac50dfc). The legacy login() reward path now runs only for a genuine three-field legacy envelope on a backend without authenticated presence configured, or after a modern presence login is accepted. Rejected/stale modern logins—and unsigned legacy bypasses once a secret is configured—cannot drain cached rewards.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review AI disclosure: This content was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ac50dfcaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
| sendPresenceMessage(VotingPluginWire.backendStarted(presenceServer, presenceIncarnationId, | ||
| presenceStartedAt, nextPresenceTimestamp())); |
There was a problem hiding this comment.
When the initial BackendStarted was accepted but this carrier replay is delayed until after the backend expires, nextPresenceTimestamp() makes it look like a newer lifecycle event. expireBackends() leaves the incarnation active and only marks it unavailable, so backendStarted() accepts this replay and revives the dead backend; replay the original start timestamp so an already-seen start remains idempotent while an initially undeliverable plugin message can still initialize the backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f01d48f8. The first-carrier replay now sends BackendStarted with the original presenceStartedAt value as both the generation metadata and lifecycle timestamp. If the original start never had a plugin-message carrier, the proxy can initialize from this replay; if it already accepted the start, the existing lifecycle timestamp fence rejects the replay without refreshing availability. The existing replayedLifecycleMessageCannotReviveExpiredBackend regression covers the receiver behavior, and Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f01d48f867
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (presenceStartedReplayedWithCarrier || !presenceReporting || presenceServer == null | ||
| || presenceIncarnationId == null || !hasPresenceServerSecret()) { |
There was a problem hiding this comment.
When the dedicated proxy restarts while this backend remains running, its new tracker has no current generation, so every subsequent heartbeat and modern login is rejected by isCurrentBackendGeneration. The final tree still keeps presenceStartedReplayedWithCarrier true for the backend's lifetime, meaning the only message capable of recreating that generation is never sent again; presence tracking and the accepted-login reward flow remain disabled until every backend plugin restarts. Reannounce the same idempotent BackendStarted periodically or provide a proxy-restart handshake.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cadc7262. Every authenticated heartbeat now first reannounces the same BackendStarted incarnation with its original lifecycle timestamp. A fresh proxy can therefore reconstruct the generation, while a proxy that already saw it rejects the duplicate lifecycle event without refreshing stale state.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| || !backendPlayerPresenceTracker.applySnapshotChunk(snapshot.server, | ||
| snapshot.requestId, snapshot.chunkIndex, snapshot.chunkCount, snapshot.players, | ||
| snapshot.backendIncarnationId, snapshot.backendStartedAt, | ||
| snapshot.presenceTimestamp, System.currentTimeMillis())) { |
There was a problem hiding this comment.
When a destination login arrives before the source backend's logout, the conflict path rejects the login and requests this snapshot. A successful snapshot can then move the player to the destination, but this listener only updates the tracker and never invokes login(player, uuid, server), so cached online votes/rewards that the rejected login would normally deliver remain queued until a later reconnect. Retain the pending handoff identity and run the login flow once the destination snapshot confirms it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cadc7262. A rejected cross-backend login is retained against the active, bounded snapshot request. When the snapshot completes, the pending entry is removed first and login() runs exactly once only if the tracker confirms the same player UUID, destination server, backend generation, and connection ID. Direct accepted logins, backend replacement/stop/expiry, failed requests, and the two-minute timeout discard stale handoffs.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c908d7f39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public Set<String> expireBackendPresence(long timeoutMillis) { | ||
| long now = System.currentTimeMillis(); | ||
| Set<String> expired = backendPlayerPresenceTracker.expireBackends(now, timeoutMillis); |
There was a problem hiding this comment.
When a backend crashes or loses connectivity without sending BackendStopped, its last heartbeat and all owned players remain available indefinitely. Across the inspected Bungee and Velocity implementations, a repo-wide search finds no call to this newly added expireBackendPresence method, so the timeout logic in the tracker is never exercised; schedule it from both dedicated proxy modes with a timeout greater than the 30-second heartbeat interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cadc7262. Presence maintenance is scheduled centrally through the proxy runtime's platform timer, so both Bungee and Velocity run it every 30 seconds. Backends silent for more than 90 seconds (three heartbeat intervals) are expired along with their players and pending handoffs.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| reannouncePresenceStarted(); | ||
| sendPresenceMessage(VotingPluginWire.backendHeartbeat(presenceServer, presenceIncarnationId, | ||
| presenceStartedAt, nextPresenceTimestamp())); |
There was a problem hiding this comment.
When the proxy restarts while this backend and its players remain online, the fresh periodic BackendStarted replay now restores the backend generation, but this recovery path sends only the start and heartbeat. seedOnlinePlayerPresence() runs only during backend startup/reload, and the proxy's accepted-start listener does not request a snapshot, so players who were already connected remain absent from the new tracker until they reconnect or an external caller manually requests a snapshot; trigger a snapshot or replay the active sessions when recovering the proxy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cadc7262. When a fresh proxy accepts an authenticated backend generation, it immediately requests a complete presence snapshot. The recovery request is retained and retried by the 30-second maintenance task after bounded request expiry until a complete matching snapshot restores already-connected players; successful snapshots and backend stop/expiry clear recovery state.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review the latest head ( AI disclosure: This comment was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadc72621a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!enabled) { | ||
| return; | ||
| } | ||
| expireBackendPresence(PRESENCE_BACKEND_TIMEOUT_MILLIS); |
There was a problem hiding this comment.
After a transport outage longer than 90 seconds, this expiration clears the backend's players, but a later same-incarnation BackendStarted replay is rejected as an idempotent lifecycle timestamp while its following heartbeat merely marks the backend available. Because the completed recovery snapshot previously removed the server from pendingBackendRecoverySnapshots, no new snapshot is requested and players who stayed connected remain absent until they reconnect; detect the unavailable-to-available heartbeat transition or retain a recovery request after expiration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ee8b3e7. Expiration now keeps the backend in recovery-snapshot state instead of removing it. While unavailable, snapshot requests remain safely blocked by the tracker; after a valid same-incarnation heartbeat marks it available again, the 30-second maintenance task requests a fresh snapshot and retains retry state until a complete matching snapshot restores players.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| private void sendPresenceHeartbeat() { | ||
| synchronized (presenceLifecycleLock) { | ||
| if (presenceReporting && presenceServer != null && presenceIncarnationId != null) { | ||
| reannouncePresenceStarted(); |
There was a problem hiding this comment.
When the proxy restarts during an overlapping backend deployment, the new process's start may be accepted first and then the old process's periodic reannouncement here can arrive second; since the fresh tracker has not retired the old UUID, backendStarted() replaces the new incarnation and retires it. When the old process subsequently stops, the surviving new process remains rejected indefinitely because its UUID is now retired. The new periodic reannouncement is fresh evidence for this ordering failure; recovery needs an ordering fence that prevents an older process from reclaiming the generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6ee8b3e7. Competing incarnations of the same configured backend name are now ordered by their immutable backendStartedAt process-start value. An older periodic replay is rejected and retired when a newer incarnation is already current, so its later stop cannot displace the surviving process. This ordering is never applied across different backend names, and the regression covers the newer start arriving first followed by the old replay and delayed stop.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review the latest head ( AI disclosure: This comment was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee8b3e706
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (backendStartedAt <= state.backendStartedAt) { | ||
| retireIncarnation(state, backendIncarnationId); | ||
| return false; |
There was a problem hiding this comment.
When a genuine replacement starts with a clock that is behind the current process—or in the same millisecond—this comparison retires the replacement UUID; after the old process stops or times out, all reannouncements and heartbeats from the surviving replacement remain rejected because that UUID is now retired, leaving the backend unavailable until another restart. The newly added backendStartedAt <= state.backendStartedAt fence is fresh evidence for the previously discussed wall-clock ordering failure; use a restart-order mechanism that does not assume process clocks are strictly increasing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 27649fed. The backendStartedAt comparison has been removed; equal and rolled-back backend clocks can replace the current incarnation. The proxy now atomically persists its bounded current/retired incarnation UUID ordering per configured backend and restores it before message listeners start. Restored active generations begin unavailable, prove liveness by heartbeat, and recover players by snapshot. The regression exercises an equal-timestamp replacement, persists it, restarts the proxy again, rejects the old process replay, and accepts the surviving replacement heartbeat.
The state file is bounded to 1 MiB and contains no player data or authentication secrets.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review the latest head ( AI disclosure: This comment was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27649fed7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (fileSize <= 0L || fileSize > MAX_FILE_BYTES) { | ||
| throw new IOException("Backend generation state file has an invalid size"); |
There was a problem hiding this comment.
When a deployment approaches the permitted 1,024 backends and 64 retired incarnations per backend, save() can produce a file that this check always rejects on the next startup: the retired UUID payload alone can occupy 1,048,576 bytes, before server names, current UUIDs, and metadata are added. The proxy then restores no incarnation fences, allowing an obsolete periodic BackendStarted replay to become current; either reduce the entry bounds, reject oversized snapshots before replacement, or raise/derive the loader limit from the maximum serialized form.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfbfd858. The loader cap is now derived from the exact serialized maximum: header plus 1,024 entries, 128 UTF-16 code units per server name at worst-case modified-UTF size, current UUID/metadata, and 64 retired UUIDs. save() validates the same limits before touching the temporary file. A regression writes all 1,024 maximum-retirement entries, verifies the file exceeds the old 1 MiB cap, then restores all 1,024 successfully.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| } catch (AtomicMoveNotSupportedException e) { | ||
| Files.move(temporary, file, StandardCopyOption.REPLACE_EXISTING); |
There was a problem hiding this comment.
On filesystems/providers that throw AtomicMoveNotSupportedException, this fallback performs an ordinary replacement with no atomicity guarantee. If the proxy or host stops during that replacement, the previous valid ordering fence may be lost or replaced by an unreadable file, after which startup proceeds with an empty tracker and can accept an obsolete incarnation replay. Since this state is specifically the durable ordering fence, retain a recoverable previous copy or fail the update rather than silently degrading to a non-atomic replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfbfd858. The non-atomic replacement fallback was removed. If ATOMIC_MOVE is unsupported, the temporary file is deleted, the existing valid ordering file remains untouched, and the save reports an error instead of silently weakening the restart fence.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
AI disclosure: This commit was created with assistance from ChatGPT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec4447000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (!backendIncarnationId.equals(state.backendIncarnationId)) { | ||
| if (state.retiredIncarnations.contains(backendIncarnationId)) { | ||
| return false; | ||
| } | ||
| // Only a BackendStarted event can advance the proxy-local restart order. The | ||
| // current and retired UUIDs are persisted by the proxy so this ordering | ||
| // survives proxy restarts without comparing backend clocks. | ||
| retireCurrentIncarnation(state); |
There was a problem hiding this comment.
When the proxy restarts during an overlapping backend deployment, its tracker is empty, so if the surviving new incarnation is observed first and an older process reannounces second, this branch treats the old UUID as a replacement and retires the new one; after the old process stops, all future traffic from the surviving process is rejected until another backend restart. Fresh evidence after the earlier fix is that the final tree removed BackendGenerationStateStore, despite the adjacent comment still claiming these UUIDs are persisted, so the restart fence no longer survives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4626577 without bringing files back. While the current incarnation is available, a competing BackendStarted may replace it only when its backendStartedAt is later. Therefore, after a voting-proxy restart, observing the surviving newer process first causes a periodic reannouncement from the overlapping older process to be rejected, and the older process's later stop cannot clear the survivor. If the current process has stopped or expired, a replacement is accepted regardless of clock regression, preserving recovery after a graceful stop or heartbeat timeout. The stale persistence comment was replaced, and regression coverage exercises both restart ordering and clock-regressed replacement after stop.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| if (requestId == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
When a player transfers within 30 seconds after this destination's previous snapshot completed, beginSnapshot() rejects the new request because of its cooldown and there is no active request ID, so this return drops the pending handoff entirely. The destination login has already been rejected as conflicting; once the source logout arrives, the player disappears from tracked presence and the deferred cached-reward login is never completed until another reconnect. Queue the handoff and request its snapshot after the cooldown rather than discarding it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4626577. A conflicting login now queues its handoff before attempting the destination snapshot. If the 30-second snapshot cooldown blocks creation and there is no active request, the handoff remains unassigned instead of being dropped. Presence maintenance retries unassigned handoffs, attaches them to the next allowed snapshot, and also requeues an assigned handoff if its snapshot expires. Added proxy coverage proving the cooldown-blocked handoff is retained and causes a snapshot request once the interval elapses.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| private boolean isExpired(long now) { | ||
| return now >= requestedAt && now - requestedAt >= SNAPSHOT_TIMEOUT_MILLIS; |
There was a problem hiding this comment.
If the proxy wall clock moves backward after a snapshot request whose response is lost, this condition keeps the request pending until wall time reaches the old value again. Because beginSnapshot() refuses to replace a pending request and also rejects when lastSnapshotRequestedAt > now, recovery and handoff snapshots can remain blocked for the entire clock offset, potentially hours. Track timeout and cooldown durations with a monotonic clock or explicitly expire state after a backward jump.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b4626577. Pending snapshots now expire immediately when the observed proxy wall clock moves behind requestedAt, and the snapshot cooldown permits a new request after that backward jump, resetting lastSnapshotRequestedAt to the new clock origin. This prevents a lost request from remaining pinned for the clock offset. Added regression coverage that starts a snapshot, moves the clock backward, verifies the request expires, and verifies a replacement request is accepted immediately.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
|
@codex review Please review the latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec4447000
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Set<String> expired = new LinkedHashSet<>(); | ||
| for (Map.Entry<String, BackendState> entry : backends.entrySet()) { | ||
| BackendState state = entry.getValue(); | ||
| if (state.available && now - state.lastSeen > timeoutMillis) { |
There was a problem hiding this comment.
In non-PLUGINMESSAGING modes, if the proxy host's wall clock moves backward after a backend crashes, now - state.lastSeen remains negative until the clock catches up. The silent backend therefore stays available and its players remain tracked far beyond the configured 90-second timeout, potentially routing votes or rewards using stale presence; measure backend silence with a monotonic clock or explicitly handle backward jumps.
Useful? React with 👍 / 👎.
AI disclosure: This commit was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acde9a97b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| prunePendingPresenceHandoffs(now); | ||
| handoff.requestId = null; | ||
| pendingPresenceHandoffs.put(handoff.playerUuid, handoff); |
There was a problem hiding this comment.
When the same player moves A→B→C before B's snapshot returns, queuing C here overwrites the B handoff but neither invalidates B's active snapshot nor records the C conflict in the tracker's event watermark. The B snapshot can therefore install presence on B; when C's snapshot arrives, applyCompleteSnapshot skips it because B is now a current event newer than C's request watermark. Both deferred login completions are then discarded without delivering cached rewards, and the player remains attributed to B until another presence event. Cancel or fence snapshots belonging to superseded handoffs, or include the newer conflict in snapshot ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 08afe049. A cross-backend login conflict now records a per-player event sequence and destination server before its snapshot begins. In an A→B→C transfer, B's request watermark predates C's conflict marker, so B's later snapshot cannot install stale presence; C's request watermark includes that marker, so C's matching snapshot can become authoritative and complete the remaining deferred login. Added a regression that applies B's snapshot before C's and verifies the player stays on A until C wins.
This commit also adds the requested five-second startup resync for presence-capable transports; PLUGINMESSAGING remains excluded.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08afe0498d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| long sequence = ++eventSequence; | ||
| removePresence(playerUuid, sequence); |
There was a problem hiding this comment.
When A's logout arrives after the C conflict marker but before the B/C snapshots, this removal advances the player's event sequence and replaces its destination marker with A. B's older snapshot then sees no current presence and a newer marker for a different server, so it installs B; that installation subsequently causes C's snapshot to be skipped, leaving the player attributed to B indefinitely. Preserve the newer destination marker when removing the older source connection, or track these fences separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8a222829. Cross-backend transition ordering is now retained separately from ordinary player-event/logout tombstones. In the reported A→B→C sequence, A's delayed logout removes A's live presence but does not overwrite C's newer transition fence; B's request watermark predates that fence and its stale snapshot is skipped, while C's request includes the fence and can still become authoritative.
Added sourceLogoutPreservesNewerDestinationFence, which reproduces the source logout between the B/C requests and responses, applies B first, verifies the player remains unknown/offline, then verifies C wins. The fence is bounded and pruned with the existing retained player-event state.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a222829b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boolean conflictingBackend = backendPlayerPresenceTracker.hasConflictingPresence(player, uuid, server); | ||
| accepted = backendPlayerPresenceTracker.playerOnline(player, uuid, server, event.connectionId, |
There was a problem hiding this comment.
When transport callbacks execute concurrently, the presence state can change between these two synchronized tracker calls. For example, after A's logout makes the precheck return false, B's older snapshot can install the player before C's playerOnline call; that call then rejects C as a cross-backend conflict and records its fence, but the cached conflictingBackend == false prevents requesting C's confirming snapshot or retaining its handoff. Return the rejection reason from the same atomic playerOnline operation instead of checking conflict separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 55ceb234. The proxy no longer calls hasConflictingPresence() before playerOnline(). The tracker now performs validation, conflict detection, destination-fence insertion, and result classification in one synchronized playerOnlineResult() operation. It returns ACCEPTED, CONFLICTING_PRESENCE, or REJECTED; only the atomic conflict result causes the proxy to retain the handoff and request the confirming destination snapshot.
The old split precheck API was removed. Regression coverage now verifies that a valid conflicting login returns CONFLICTING_PRESENCE after recording its fence, while a stale replay returns ordinary REJECTED; existing proxy coverage verifies conflict results retain/retry the handoff through snapshot cooldown.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ceb23429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (result == BackendPlayerPresenceTracker.PlayerOnlineResult.CONFLICTING_PRESENCE) { | ||
| requestBackendPresenceSnapshot(server, | ||
| new PendingPresenceHandoff(player, uuid, server, event.connectionId, | ||
| event.backendIncarnationId, event.backendStartedAt, System.currentTimeMillis())); |
There was a problem hiding this comment.
When transport callbacks for rapid A→B→C moves run concurrently, B can return CONFLICTING_PRESENCE, C can then record the newer destination fence, and B can resume here afterward and overwrite C's pending handoff while starting its snapshot with a watermark that already includes C's fence. Because that fence is not newer than B's snapshot watermark, B's snapshot may install B; C's later snapshot then sees B's installed presence as newer and skips C, leaving presence and deferred reward delivery on the wrong backend. Return an event sequence/token with the conflict result and reject queueing or snapshot creation when that token is no longer the current destination claim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f533c1fe. PlayerOnlineResult now carries the conflict event-sequence token. The proxy stores that token in PendingPresenceHandoff, refuses to replace a higher-sequence handoff with an older one, and uses claim-aware tracker methods for both creating a snapshot and attaching to an already-active request. Those tracker methods validate the token against the current destination claim under the same tracker lock that captures the snapshot watermark.
If B resumes after C records a newer claim, B can neither queue over C nor create/reuse a snapshot; its stale handoff is discarded without removing C. If B acquired the tracker lock before C, B's watermark predates C's fence, so the existing snapshot ordering still rejects B when it arrives. Destination claims are separately bounded, survive source logout and snapshot cooldown, and are invalidated by a newer destination, destination logout, accepted presence, completion, or expiry.
Added supersededDestinationClaimCannotStartOrReuseSnapshot, including a delayed source logout with no pending snapshot, to verify B's token is rejected and C's remains usable.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f533c1fe3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public synchronized UUID getPendingSnapshotRequestIdForDestinationClaim(String server, UUID playerUuid, | ||
| long conflictSequence, long now) { | ||
| if (!isCurrentDestinationClaim(playerUuid, server, conflictSequence)) { | ||
| return null; | ||
| } | ||
| return getPendingSnapshotRequestId(server, now); |
There was a problem hiding this comment.
When a recovery or other snapshot for this server was already pending before the conflicting login, this returns that request solely because the claim is current, even though its eventWatermark is lower than conflictSequence. applyCompleteSnapshot then skips this player because the newer cross-backend fence exceeds the snapshot watermark, while completePendingPresenceHandoffs still removes the handoff and releases its claim, so the destination login and cached-reward delivery are lost until another reconnect. Only attach the handoff to a pending snapshot whose watermark includes its conflict token; otherwise leave it unassigned and retry after the older snapshot completes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a409de8. getPendingSnapshotRequestIdForDestinationClaim() now reads the pending snapshot under the tracker lock and returns its request only when pending.eventWatermark >= conflictSequence. A recovery snapshot that began before the conflict therefore cannot consume or receive the handoff; the handoff remains unassigned, the older snapshot completes normally, and maintenance requests a fresh claim-bound snapshot after cooldown.
Added destinationClaimDoesNotReuseSnapshotWithOlderWatermark, which begins the destination snapshot before the conflict, verifies the claim-aware lookup rejects it, completes it, then verifies a fresh post-cooldown snapshot can be created for the same still-current claim.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
| CrossBackendFence claim = destinationClaims.get(uuid); | ||
| if (claim != null && claim.server.equalsIgnoreCase(server)) { | ||
| destinationClaims.remove(uuid); |
There was a problem hiding this comment.
When a conflicting destination login is queued while snapshot cooldown leaves no request pending, a logout from that destination does not reach this invalidation: playerOffline sees the live presence still on the source and returns false because pendingSnapshots has no destination entry. The stale claim and handoff therefore survive the destination session's logout and maintenance later requests a snapshot for a handoff that is no longer valid; recognize a matching destination claim as sufficient to record the logout and remove it even when no snapshot exists yet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a409de8. Destination claims now retain the exact connection ID as well as server and conflict sequence. playerOffline() accepts a logout for that matching claimed connection even when cooldown leaves no snapshot pending, records the destination tombstone, and invalidates the claim while preserving the live source presence. A stale or different connection does not satisfy this claim path.
Added destinationLogoutInvalidatesClaimWithoutPendingSnapshot, which establishes destination cooldown, records a source→destination conflict with no pending request, applies the destination logout, verifies the source remains authoritative, verifies the token is invalid, and proves it cannot later create a snapshot.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a409de8fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DestinationClaim claim = destinationClaims.get(playerUuid); | ||
| boolean matchingDestinationClaim = claim != null | ||
| && claim.server.equalsIgnoreCase(normalizedServer) && claim.connectionId.equals(connectionId); | ||
| if (!fenced || (!pendingSnapshots.containsKey(key) && !matchingDestinationClaim) |
There was a problem hiding this comment.
When the destination claim's snapshot is already pending, a delayed logout for a different connection ID still passes this condition solely because pendingSnapshots.containsKey(key) is true. It then records an event newer than the snapshot watermark, causing the snapshot to skip the claimed connection; completion subsequently removes the handoff and releases its claim without running the deferred login, even though the current claimed session never logged out. Fresh evidence after the earlier fix is that the new matchingDestinationClaim check remains bypassed by the pending-snapshot alternative; require a matching claim connection whenever a destination claim exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dd9efd52. When a destination claim exists for the logout's server, playerOffline() now requires the logout connection ID to match that claim before it can record a tombstone. The mere presence of a pending snapshot no longer bypasses this check. Pending snapshots still permit an unclaimed destination tombstone, preserving recovery behavior where no claim is involved.
Added mismatchedDestinationLogoutCannotFenceClaimedSnapshot: it creates a pending claim-bound snapshot, sends a later logout with a different connection ID, verifies the claim remains current, and verifies the snapshot installs the claimed connection on the destination.
Java 21 Maven CI passes.
AI disclosure: This reply was written with assistance from ChatGPT.
AI disclosure: This commit was written with assistance from ChatGPT.
|
@codex review Please review latest head (
Java 21 Maven CI passes. AI disclosure: This review request was written with assistance from ChatGPT. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
PLUGINMESSAGINGtransportsPLUGINMESSAGINGand use the player-facing proxy's native online-player/server statePLUGINMESSAGINGtransports, announce backend start/stop, periodically replay the same idempotent start, and send a 30-second heartbeatPLUGINMESSAGINGtransports, run proxy-side presence maintenance every 30 seconds and expire silent backends after 90 secondsDesign notes
For non-
PLUGINMESSAGINGpresence, the configured backendServername remains the stable backend identity; this does not introduce a configured backend instance ID. Every presence-reporting lifecycle automatically generates a collision-resistantbackendIncarnationId. While the current incarnation is available, only aBackendStartedenvelope with a laterbackendStartedAtmay replace it; once the current incarnation stops or expires, a replacement is accepted even if its clock moved backward. Retired incarnation IDs are rejected. Heartbeats, player events, shutdowns, and snapshots cannot claim a backend generation that is not current.Each envelope carries
backendStartedAtandpresenceTimestamp, recorded when the lifecycle/player event is produced or snapshot data has finished being captured. Event timestamps are monotonic within one backend incarnation and reject reordered events from that incarnation. For competing incarnations of the same configured backend,backendStartedAtprevents an overlapping older process from replacing an available newer process after a voting-proxy restart; timestamps never order different backend names. Generation state is intentionally memory-only.Presence writes no generation or player-state files. After a voting-proxy restart, the tracker begins empty and unknown players are treated as offline. Five seconds after startup, the proxy sends a targeted
PresenceResyncRequestto every configured backend. Each backend reannounces its active generation and resets only its snapshot-request cooldown; the proxy then uses the existing generation-bound request to obtain a complete online-player snapshot. Periodic backend reannouncements and bounded snapshot retries remain as fallback recovery.On a presence-enabled transport, a login that conflicts with presence on another backend cannot directly replace it. The tracker atomically detects a conflicting destination, records its event-ordering fence, and returns a typed result with that conflict's event-sequence token. Pending handoffs refuse to replace a newer token, and snapshot creation or attachment atomically verifies that the token is still the current destination claim. Attachment also requires the pending snapshot's event watermark to include the conflict token; an older recovery snapshot completes without consuming the handoff, which then retries with a fresh snapshot. Only a matching snapshot confirms the handoff. This ordering fences an older destination snapshot during rapid A→B→C transfers while allowing C's snapshot to become authoritative. A separate bounded cross-backend transition fence survives a delayed source logout, so removing A cannot erase C's newer claim and let B's stale snapshot win. If the destination is inside its snapshot cooldown, the handoff remains queued and maintenance attaches it to the next allowed snapshot. A matching destination logout invalidates its exact connection claim even while cooldown leaves no snapshot pending; once a claim exists for that destination, a logout with a different connection ID is rejected even if a snapshot is pending. Proxy-local event watermarks and destination logout tombstones protect snapshots from login/logout races without relying on cross-server clock synchronization.
Snapshot collection runs on the Bukkit server thread because broker listeners may be asynchronous. Requests target the active incarnation, are deduplicated before pending state changes, and have matching 30-second proxy/backend cooldowns. Snapshot chunks have aggregate player and payload limits, incomplete snapshots expire after two minutes, and a backward proxy-clock adjustment immediately expires a pending request and resets its cooldown origin. Pending snapshots are cancelled before retained tombstones can exceed their bound.
With
PLUGINMESSAGING, the backend presence protocol is disabled. Backends send only the original login notification used for cached-reward delivery; they do not send extended presence logins, logouts, lifecycle events, heartbeats, resync requests, or snapshots. The player-facing Bungee/Velocity proxy supplies authoritative online-player and current-server state, and the proxy schedules no presence startup resync or maintenance in this mode.Transport security and compatibility
This feature adds no
PresenceServerSecretorPresenceServerSecretsconfiguration and performs no second presence-specific HMAC layer. On presence-enabled transports, each backend's unique configuredServervalue inBungeeSettings.ymlis its trusted identity. All participants with access to that transport must therefore be trusted not to impersonate another backend; a compromised trusted backend is outside this feature's threat model.PLUGINMESSAGINGcarries only the original login notification and other existing plugin messages, so its existingPluginMessageEncryptionoption remains available and unchanged. Other transports retain their existing security model. Generation fencing, identifiers, timestamps, and bounds provide protocol integrity but do not independently authenticate a backend beyond that trust boundary.PLUGINMESSAGINGbackends always send the original login envelope, and the proxy always preserves that compatibility path for cached rewards. The proxy prefers its native current-server lookup for delivery and falls back to the backend's claimed server if native state is not yet available. Legacy login envelopes remain accepted on the other transports as well; new presence-enabled backends send the extended login.This is the presence-foundation PR only. Existing vote routing, Votifier handling, vote-party behavior, and multi-proxy behavior remain unchanged. The dedicated voting-proxy toggle and vote routing through this tracker can be added in the follow-up PR.
Validation
PLUGINMESSAGINGdoes not schedule itPLUGINMESSAGINGdelivery through native proxy server state, and ignore extended presence logins in that modePLUGINMESSAGINGdisables backend presence while standalone transports retain itAI disclosure: This content was written with assistance from ChatGPT.