Harden status ping path and refresh prebuilt jar#8
Merged
Conversation
Add a short-TTL (1s) cache for the remapped legacy status response in both StatusPacketMixin and LegacyPacketHandler so repeated server-list refreshes reuse a prebuilt packet instead of reconstructing each time. Replace per-connection INFO logging for STATUS translator installs with a sampled summary (every 10s with counts) to eliminate log flood from rapid server-list polling without losing visibility. Made-with: Cursor
Made-with: Cursor
📝 WalkthroughWalkthroughAdds a centralized short-lived (1s TTL) cache for remapped legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StatusPacketMixin
participant LegacyStatusCacheManager
participant LegacyPacketHandler
Client->>StatusPacketMixin: ServerboundStatusRequestPacket
activate StatusPacketMixin
StatusPacketMixin->>LegacyStatusCacheManager: getOrBuildForStatusListener(status)
alt cache hit
LegacyStatusCacheManager-->>StatusPacketMixin: cached ClientboundStatusResponsePacket
else cache miss
LegacyStatusCacheManager->>LegacyPacketHandler: (buildLegacyStatusResponse) remap & build legacy packet
activate LegacyPacketHandler
LegacyPacketHandler-->>LegacyStatusCacheManager: new ClientboundStatusResponsePacket
deactivate LegacyPacketHandler
LegacyStatusCacheManager-->>StatusPacketMixin: new ClientboundStatusResponsePacket
end
StatusPacketMixin->>Client: send ClientboundStatusResponsePacket
StatusPacketMixin->>StatusPacketMixin: cancel original handler
deactivate StatusPacketMixin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 110-111: The STATUS log window is seeded at class init causing the
first post-quiescence ping to report a tiny interval; update the window start
when beginning a new sampling window by resetting statusInstallWindowStart =
System.nanoTime() at the moment statusInstallCount transitions from 0 to 1
(i.e., when incrementing statusInstallCount in LegacyPacketHandler), so the
elapsed interval reflects time since first sample rather than class load; apply
the same change to the analogous block that manages the other status window (the
code around the 228–240 region) to reinitialize its window start on its own 0→1
transition.
- Around line 112-113: Wrap cachedStatusResponse and statusCacheBuiltAt into a
single immutable holder (e.g. class StatusCache {
ClientboundStatusResponsePacket response; long builtAt; }) and replace the two
separate fields with one volatile reference (e.g. private static volatile
StatusCache statusCache). Update all reads to read the single volatile
statusCache and use its .response and .builtAt atomically, and update all writes
to construct a new StatusCache instance and assign it to the volatile reference
(instead of writing cachedStatusResponse and statusCacheBuiltAt separately).
Make changes in LegacyPacketHandler where cachedStatusResponse and
statusCacheBuiltAt are read/written so the publish is atomic and no
stale-response/new-timestamp mix can occur.
In `@src/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java`:
- Around line 24-26: The three volatile fields cachedLegacyResponse,
cachedAtNanos, and cachedSourceStatus cause a race when read/written separately;
replace them with a single immutable holder class (e.g., LegacyStatusCache or
StatusCacheEntry) containing the ClientboundStatusResponsePacket, ServerStatus,
and timestamp, and publish that holder via one volatile reference (e.g.,
volatile LegacyStatusCache cachedStatus). Update all reads to read the single
volatile holder once and all writes to construct a new holder instance and
assign it to the volatile field (update any places that set
cachedLegacyResponse/cachedSourceStatus/cachedAtNanos to instead set the new
holder and adjust access sites to use holder.packet, holder.sourceStatus,
holder.timestamp).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36ce1a4c-1caf-4289-8763-37654298d7e8
⛔ Files ignored due to path filters (1)
prebuilt/legacylink-0.1.1.jaris excluded by!**/*.jar
📒 Files selected for processing (2)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java
Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java (1)
56-75: Consider unifying this cache withLegacyPacketHandler’s status cache.
StatusPacketMixinandLegacyPacketHandlercurrently maintain separate 1-second caches with different keying rules (ServerStatusidentity+TTL here vs TTL-only insrc/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java:568-586). A shared cache/remap utility would reduce drift and keep behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java` around lines 56 - 75, The two separate 1s caches should be unified into a single shared cache utility to avoid drift: extract the caching/remapping logic from StatusPacketMixin.getOrBuildCachedResponse and the block in LegacyPacketHandler into a new shared helper (e.g., LegacyStatusCacheManager) that owns the LegacyStatusCache data and STATUS_CACHE_TTL_NS semantics; then have StatusPacketMixin.getOrBuildCachedResponse and LegacyPacketHandler call that helper to fetch or build the remapped ClientboundStatusResponsePacket, ensuring both use the same keying/TTL rules and removing duplicate cache state from StatusPacketMixin and LegacyPacketHandler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java`:
- Around line 56-75: The two separate 1s caches should be unified into a single
shared cache utility to avoid drift: extract the caching/remapping logic from
StatusPacketMixin.getOrBuildCachedResponse and the block in LegacyPacketHandler
into a new shared helper (e.g., LegacyStatusCacheManager) that owns the
LegacyStatusCache data and STATUS_CACHE_TTL_NS semantics; then have
StatusPacketMixin.getOrBuildCachedResponse and LegacyPacketHandler call that
helper to fetch or build the remapped ClientboundStatusResponsePacket, ensuring
both use the same keying/TTL rules and removing duplicate cache state from
StatusPacketMixin and LegacyPacketHandler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddf6a3a0-88ee-4b85-92d3-ab1e761d999a
⛔ Files ignored due to path filters (1)
prebuilt/legacylink-0.1.1.jaris excluded by!**/*.jar
📒 Files selected for processing (2)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java
Consolidate the mixin and outbound handler status remap caches so both paths use identical TTL and keying behavior from a single utility. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/status/LegacyStatusCacheManager.java (1)
53-55: Optional: move the legacy version name to a constant.Line 54 hardcodes
"26.1.2"while Line 55 usesLegacyLinkConstants.PROTOCOL_26_1_2; extracting the string to constants as well would prevent accidental mismatches later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/status/LegacyStatusCacheManager.java` around lines 53 - 55, Extract the hardcoded legacy version string "26.1.2" into a constant and use it when constructing ServerStatus.Version to avoid mismatches; add a constant (e.g., LEGACY_VERSION_NAME = "26.1.2") near LegacyLinkConstants or inside LegacyStatusCacheManager and replace the literal in the ServerStatus.Version(...) call so both the version name and LegacyLinkConstants.PROTOCOL_26_1_2 are defined from constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/status/LegacyStatusCacheManager.java`:
- Around line 53-55: Extract the hardcoded legacy version string "26.1.2" into a
constant and use it when constructing ServerStatus.Version to avoid mismatches;
add a constant (e.g., LEGACY_VERSION_NAME = "26.1.2") near LegacyLinkConstants
or inside LegacyStatusCacheManager and replace the literal in the
ServerStatus.Version(...) call so both the version name and
LegacyLinkConstants.PROTOCOL_26_1_2 are defined from constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2df52c31-6f0d-479f-8bad-4c8cc6a9163a
📒 Files selected for processing (3)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.javasrc/main/java/dev/ohno/legacylink/status/LegacyStatusCacheManager.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
prebuilt/legacylink-0.1.1.jarfrom the updated status-hardening buildTest plan
./gradlew buildinlegacylinklegacylink-0.1.1.jartominecraft-cabal/server/modsMade with Cursor
Summary by CodeRabbit