Conversation
Poh Transports without Rs2Cache
…2CacheManager - Add async save/load operations (savePersistentCachesAsync, loadCachesAsync) using CompletableFuture - Implement file-based cache storage under .runelite/microbot-profiles to prevent profile bloat - Add cloud sync configuration option for optional RuneLite profile synchronization - Enhance CacheMetadata with data integrity checks and debugging information - Improve profile change handling with async operations and proper operation chaining - Add dedicated thread pool (cacheManagerExecutor) for cache persistence operations - Implement operation tracking to prevent concurrent save/load conflicts - Update skill level checks in PathfinderConfig to use cached values when available - Add graceful shutdown handling with timeout for ongoing cache operations - Improve error handling and logging throughout cache management system
- Implement dual storage system: file-based (primary) + compressed profile sync (secondary) - Add CompressionUtils with GZip compression and Base64 encoding for profile storage - Update CacheSerializationManager with cloud sync methods (saveCacheToProfile, loadCacheFromProfile) - Integrate dual storage in Rs2CacheManager for all persistent caches - Add configuration option enableCacheCloudSync for optional cross-device sync - Files provide fast local access while compressed profiles enable device synchronization - Size limit of 64KB for compressed profile data to prevent profile bloat - File-first loading strategy with profile fallback for optimal performance
Async Cache Operations and File-Based Storage Implementation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
runelite-client/src/main/resources/net/runelite/client/plugins/microbot/shortestpath/poh_icon.pngis excluded by!**/*.png
📒 Files selected for processing (26)
runelite-client/pom.xml(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/devtools/LocationOverlay.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/MicrobotPlugin.java(6 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/inventorysetups/MInventorySetupsPlugin.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PohPanel.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java(4 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/CheckboxPanel.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/EnumListPanel.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/JewelleryBoxPanel.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java(4 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/ActorModel.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/Rs2InventorySetup.java(4 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2Cache.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2CacheManager.java(18 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2PohCache.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/CacheSerializationManager.java(9 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapter.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/containers/FixedSizeQueue.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/discord/Rs2Discord.java(2 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/magic/Rs2Magic.java(0 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/NexusPortal.java(3 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/PohPortal.java(1 hunks)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java(2 hunks)runelite-client/src/main/java/net/runelite/client/ui/ClientUI.java(1 hunks)runelite-client/src/main/resources/net/runelite/client/runelite.properties(1 hunks)runelite-client/src/test/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapterTest.java(0 hunks)
💤 Files with no reviewable changes (5)
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapter.java
- runelite-client/src/test/java/net/runelite/client/plugins/microbot/util/cache/serialization/PohTeleportDataAdapterTest.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/containers/FixedSizeQueue.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2PohCache.java
- runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/magic/Rs2Magic.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-11T09:51:12.404Z
Learnt from: Krulvis
PR: chsami/Microbot#1506
File: runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/walker/Rs2Walker.java:1412-1423
Timestamp: 2025-09-11T09:51:12.404Z
Learning: In the microbot pathfinding system, global (null-keyed) transports like teleportation spells are automatically added to the player's current worldpoint by PathfinderConfig.refreshTeleports method during pathfinder refresh, which filters usableTeleports by wilderness level and adds them to the transport map at the packedLocation, eliminating the need to manually union null-keyed transports when iterating through available transports.
Applied to files:
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java
📚 Learning: 2025-08-23T16:17:37.615Z
Learnt from: g-mason0
PR: chsami/Microbot#1423
File: runelite-client/src/main/java/net/runelite/client/plugins/microbot/externalplugins/MicrobotPluginManager.java:366-389
Timestamp: 2025-08-23T16:17:37.615Z
Learning: The ScheduledExecutorService injected into MicrobotPluginManager (and other RuneLite classes) is configured as a single-threaded executor via Executors.newSingleThreadScheduledExecutor() at RuneLiteModule.java line 130, wrapped in ExecutorServiceExceptionLogger. This ensures all async tasks execute sequentially, preventing race conditions.
Applied to files:
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2CacheManager.java
🧬 Code graph analysis (3)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/ShortestPathPlugin.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PohPanel.java (1)
PohPanel(28-261)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/pathfinder/PathfinderConfig.java (1)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PohPanel.java (1)
PohPanel(28-261)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PohPanel.java (4)
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/CheckboxPanel.java (1)
CheckboxPanel(17-155)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/EnumListPanel.java (1)
EnumListPanel(16-109)runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/JewelleryBoxPanel.java (1)
JewelleryBoxPanel(17-70)runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/PohTransport.java (1)
PohTransport(15-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| { | ||
| dataManager.loadConfig(); | ||
| SwingUtilities.invokeLater(() -> panel.redrawOverviewPanel(true)); | ||
| addInventorySetup("default"); |
There was a problem hiding this comment.
Automatic "default" setup creation overwrites existing setup on every startup.
The addInventorySetup("default") call will update an existing "default" setup with the current inventory/equipment state on every plugin startup (see line 1011 where updateCurrentSetup is called when the setup already exists). This means:
- Any user-defined "default" setup will be overwritten with whatever items they happen to have equipped/in inventory at startup.
- If called before the player's inventory/equipment data is fully loaded, it could capture an invalid state.
Consider either:
- Only creating "default" if it doesn't exist (add explicit check before calling)
- Creating it with a different name like "auto_default" to avoid conflicts
- Documenting this behavior clearly for users
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/inventorysetups/MInventorySetupsPlugin.java
around line 328, the unconditional call addInventorySetup("default") overwrites
any existing "default" setup and may capture incomplete player data on startup;
change this so it only creates an auto-setup when safe: first check whether a
setup named "default" already exists and skip creation if it does (or use a
non-conflicting name like "auto_default"), and ensure the creation runs only
after inventory/equipment data is fully loaded (e.g., wait for the
player/inventory load event or a ready flag) to avoid capturing an invalid
state.
Inconsistent logic: "default" setup behavior
The startup sequence automatically creates or updates a "default" inventory setup with the player's current inventory/equipment state. However, looking at the addInventorySetup(String name) implementation (line 998-1050), if "default" already exists, it calls updateCurrentSetup(inventorySetup, false) which overwrites the existing setup with the current player state without confirmation.
This creates inconsistent behavior:
- In this file: "default" is treated as disposable and gets overwritten on every startup
- In Rs2InventorySetup.java (lines 54-56): "default" is used as a fallback safety net, implying it should be a stable, reliable setup
The result: Users who manually configure a "default" setup will have it unexpectedly overwritten on every plugin startup with whatever inventory/equipment they happen to have at that moment.
Recommendation: Consider one of these approaches:
- Only create "default" if it doesn't exist (skip the update path)
- Use a different name like "startup_snapshot" for the auto-created setup
- Document that "default" is special and always reflects startup state
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/inventorysetups/MInventorySetupsPlugin.java
around line 328, the startup call addInventorySetup("default") causes an
existing user-defined "default" setup to be silently overwritten; change the
startup logic to only create the "default" setup if it does not already exist
(or alternatively, adjust addInventorySetup to skip the update path when
name.equals("default")). Concretely: check the existing setups collection for a
setup named "default" before calling addInventorySetup, and only call it when
absent (or modify addInventorySetup to return early when name is "default" and
an existing setup is found) so user-defined defaults are preserved.
| try { | ||
| // Use async save but wait for completion during shutdown | ||
| Rs2CacheManager.savePersistentCachesAsync().get(30, java.util.concurrent.TimeUnit.SECONDS); | ||
| log.info("Successfully saved all caches asynchronously during shutdown"); | ||
| } catch (Exception ex) { | ||
| log.error("Failed to save caches during shutdown: {}", ex.getMessage(), ex); | ||
| // Fallback to synchronous save if async fails | ||
| Rs2CacheManager.savePersistentCaches(); | ||
| } |
There was a problem hiding this comment.
Race condition: timeout doesn't cancel async save.
When savePersistentCachesAsync().get(30, TimeUnit.SECONDS) times out, the async operation continues running in the background. Falling back to savePersistentCaches() (line 550) creates concurrent writes to the same cache files, risking data corruption. The timeout exception only means "we stopped waiting", not "the operation stopped".
Consider these options:
- Cancel the async operation before falling back (if the API supports cancellation)
- Don't fall back to synchronous save on timeout—just log the timeout and let the async operation complete
- Use a longer timeout if 30 seconds is insufficient
Example fix (option 2):
try {
- // Use async save but wait for completion during shutdown
- Rs2CacheManager.savePersistentCachesAsync().get(30, java.util.concurrent.TimeUnit.SECONDS);
- log.info("Successfully saved all caches asynchronously during shutdown");
+ // Use async save but wait for completion during shutdown
+ Rs2CacheManager.savePersistentCachesAsync().get(30, java.util.concurrent.TimeUnit.SECONDS);
+ log.info("Successfully saved all caches asynchronously during shutdown");
+} catch (java.util.concurrent.TimeoutException tex) {
+ log.error("Cache save timed out after 30 seconds, but async operation continues: {}", tex.getMessage());
+ // Don't fall back - let the async operation complete in background
} catch (Exception ex) {
log.error("Failed to save caches during shutdown: {}", ex.getMessage(), ex);
- // Fallback to synchronous save if async fails
+ // Fallback to synchronous save only if async operation actually failed (not timed out)
Rs2CacheManager.savePersistentCaches();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Use async save but wait for completion during shutdown | |
| Rs2CacheManager.savePersistentCachesAsync().get(30, java.util.concurrent.TimeUnit.SECONDS); | |
| log.info("Successfully saved all caches asynchronously during shutdown"); | |
| } catch (Exception ex) { | |
| log.error("Failed to save caches during shutdown: {}", ex.getMessage(), ex); | |
| // Fallback to synchronous save if async fails | |
| Rs2CacheManager.savePersistentCaches(); | |
| } | |
| try { | |
| // Use async save but wait for completion during shutdown | |
| Rs2CacheManager.savePersistentCachesAsync().get(30, java.util.concurrent.TimeUnit.SECONDS); | |
| log.info("Successfully saved all caches asynchronously during shutdown"); | |
| } catch (java.util.concurrent.TimeoutException tex) { | |
| log.error("Cache save timed out after 30 seconds, but async operation continues: {}", tex.getMessage()); | |
| // Don't fall back - let the async operation complete in background | |
| } catch (Exception ex) { | |
| log.error("Failed to save caches during shutdown: {}", ex.getMessage(), ex); | |
| // Fallback to synchronous save only if async operation actually failed (not timed out) | |
| Rs2CacheManager.savePersistentCaches(); | |
| } |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/MicrobotPlugin.java
around lines 543 to 551, the code calls savePersistentCachesAsync().get(30,
TimeUnit.SECONDS) and falls back to savePersistentCaches() on any Exception
which can cause concurrent writes if the async task actually timed out but kept
running. Modify the catch to distinguish TimeoutException from other failures:
if the caught exception is a TimeoutException, do NOT call the synchronous save
(instead log the timeout and optionally increase timeout or wait longer), but if
the Future supports cancellation call future.cancel(true) before falling back;
for non-timeout exceptions perform the synchronous fallback as before; ensure
you obtain and retain the Future returned by savePersistentCachesAsync() so you
can inspect/cancel it and update logging accordingly.
| public Set<PohTeleport> getTeleports() { | ||
| Set<PohTeleport> teleports = new HashSet<>(); | ||
| if (mountedDigsiteCb.isSelected()) { | ||
| teleports.addAll(Arrays.asList(MountedDigsite.values())); | ||
| } | ||
| if (mountedXericsCb.isSelected()) { | ||
| teleports.addAll(Arrays.asList(MountedXerics.values())); | ||
| } | ||
| if (mountedGloryCb.isSelected()) { | ||
| teleports.addAll(Arrays.asList(MountedGlory.values())); | ||
| } | ||
| if (mountedMythCapeCb.isSelected()) { | ||
| teleports.addAll(Arrays.asList(MountedMythical.values())); | ||
| } | ||
| return teleports; | ||
| } |
There was a problem hiding this comment.
Logical inconsistency: getTeleports() excludes Fairy Ring, Spirit Tree, and Wilderness Obelisk.
The method only returns teleports for the four mounted items (Digsite, Xerics, Glory, Mythical) but completely ignores the fairyRingCb, spiritTreeCb, and wildernessObeliskCb checkboxes. These three checkboxes are:
- Managed and persisted to config (lines 90-92, 114-115, 124-125)
- Auto-detected via
detectPohFacilities()(lines 133-134) - Yet their selections have no effect on the returned teleport set
This creates a disconnect where user selections for these three features may be silently ignored. Either these checkboxes should be included in the teleport set, or their purpose needs clarification if they're consumed elsewhere.
</review_comment_end -->
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/components/CheckboxPanel.java
around lines 139 to 154, the getTeleports() method currently only adds mounted
item teleports and ignores the fairyRingCb, spiritTreeCb, and
wildernessObeliskCb checkboxes; update the method to check those three
checkboxes and add their corresponding PohTeleport values (e.g.,
FairyRing.values(), SpiritTree.values(), WildernessObelisk.values() or the
actual enum names used for those teleports) into the teleports set when
selected, and add any necessary imports so the enums compile.
| .filter(e -> e.getValue().stream().anyMatch(t -> t.getType() == type)).findFirst().ifPresent(e -> { | ||
| WorldPoint existingRingPoint = e.getKey(); | ||
| for (Transport existingRingTransport : new HashSet<>(e.getValue())) { | ||
| if (existingRingTransport.getType() != type) continue; | ||
| // add from poh | ||
| newTransportsMap | ||
| .computeIfAbsent(pohExitPortal, k -> new HashSet<>()) | ||
| .add(new Transport(pohTempTransport, existingRingTransport)); | ||
|
|
||
| // add to poh | ||
| newTransportsMap | ||
| .computeIfAbsent(existingRingPoint, k -> new HashSet<>()) | ||
| .add(new Transport(existingRingTransport, pohTempTransport)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Connect PoH transports to every matching node
Line 242 currently stops at the first matching world point because of findFirst(), so only one fairy ring/spirit tree gets linked back to the PoH despite the method promising to wire all of them. The remaining transports stay disconnected and pathfinding silently loses those teleports. Iterate over every matching entry instead.
- transportsMap.entrySet().stream()
- .filter(e -> e.getValue().stream().anyMatch(t -> t.getType() == type)).findFirst().ifPresent(e -> {
- WorldPoint existingRingPoint = e.getKey();
- for (Transport existingRingTransport : new HashSet<>(e.getValue())) {
- if (existingRingTransport.getType() != type) continue;
- // add from poh
- newTransportsMap
- .computeIfAbsent(pohExitPortal, k -> new HashSet<>())
- .add(new Transport(pohTempTransport, existingRingTransport));
-
- // add to poh
- newTransportsMap
- .computeIfAbsent(existingRingPoint, k -> new HashSet<>())
- .add(new Transport(existingRingTransport, pohTempTransport));
- }
- });
+ transportsMap.entrySet().stream()
+ .filter(e -> e.getValue().stream().anyMatch(t -> t.getType() == type))
+ .forEach(e -> {
+ WorldPoint existingPoint = e.getKey();
+ for (Transport existingTransport : new HashSet<>(e.getValue())) {
+ if (existingTransport.getType() != type) {
+ continue;
+ }
+ newTransportsMap
+ .computeIfAbsent(pohExitPortal, k -> new HashSet<>())
+ .add(new Transport(pohTempTransport, existingTransport));
+ newTransportsMap
+ .computeIfAbsent(existingPoint, k -> new HashSet<>())
+ .add(new Transport(existingTransport, pohTempTransport));
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .filter(e -> e.getValue().stream().anyMatch(t -> t.getType() == type)).findFirst().ifPresent(e -> { | |
| WorldPoint existingRingPoint = e.getKey(); | |
| for (Transport existingRingTransport : new HashSet<>(e.getValue())) { | |
| if (existingRingTransport.getType() != type) continue; | |
| // add from poh | |
| newTransportsMap | |
| .computeIfAbsent(pohExitPortal, k -> new HashSet<>()) | |
| .add(new Transport(pohTempTransport, existingRingTransport)); | |
| // add to poh | |
| newTransportsMap | |
| .computeIfAbsent(existingRingPoint, k -> new HashSet<>()) | |
| .add(new Transport(existingRingTransport, pohTempTransport)); | |
| } | |
| }); | |
| transportsMap.entrySet().stream() | |
| .filter(e -> e.getValue().stream().anyMatch(t -> t.getType() == type)) | |
| .forEach(e -> { | |
| WorldPoint existingPoint = e.getKey(); | |
| for (Transport existingTransport : new HashSet<>(e.getValue())) { | |
| if (existingTransport.getType() != type) { | |
| continue; | |
| } | |
| newTransportsMap | |
| .computeIfAbsent(pohExitPortal, k -> new HashSet<>()) | |
| .add(new Transport(pohTempTransport, existingTransport)); | |
| newTransportsMap | |
| .computeIfAbsent(existingPoint, k -> new HashSet<>()) | |
| .add(new Transport(existingTransport, pohTempTransport)); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/shortestpath/PohPanel.java
around lines 242 to 256, the stream currently uses findFirst().ifPresent(...)
which stops after the first matching entry so only one matching world point is
linked; change the terminal operation to forEach(...) so the lambda runs for
every matching entry. Replace .filter(...).findFirst().ifPresent(e -> { ... })
with .filter(...).forEach(e -> { ... }) (keeping the existing body unchanged) so
all matching transports are connected to the PoH.
| * @return The timestamp when the key was cached, or -1 if not found | ||
| */ | ||
| public Long getCacheTimestamp(K key) { | ||
| return cacheTimestamps.get(key); | ||
| public long getCacheTimestamp(K key) { | ||
| Long timestamp = cacheTimestamps.get(key); | ||
| return timestamp != null ? timestamp : 0L; | ||
| } |
There was a problem hiding this comment.
Javadoc does not match implementation.
Line 580 states that the method returns -1 if not found, but Line 584 returns 0L for missing entries.
Apply this diff to fix the documentation:
/**
* Gets the cache timestamp for a specific key.
*
* @param key The key to get the timestamp for
- * @return The timestamp when the key was cached, or -1 if not found
+ * @return The timestamp when the key was cached, or 0L if not found
*/
public long getCacheTimestamp(K key) {
Long timestamp = cacheTimestamps.get(key);
return timestamp != null ? timestamp : 0L;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @return The timestamp when the key was cached, or -1 if not found | |
| */ | |
| public Long getCacheTimestamp(K key) { | |
| return cacheTimestamps.get(key); | |
| public long getCacheTimestamp(K key) { | |
| Long timestamp = cacheTimestamps.get(key); | |
| return timestamp != null ? timestamp : 0L; | |
| } | |
| /** | |
| * Gets the cache timestamp for a specific key. | |
| * | |
| * @param key The key to get the timestamp for | |
| * @return The timestamp when the key was cached, or 0L if not found | |
| */ | |
| public long getCacheTimestamp(K key) { | |
| Long timestamp = cacheTimestamps.get(key); | |
| return timestamp != null ? timestamp : 0L; | |
| } |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/Rs2Cache.java
around lines 580 to 585, the Javadoc claims the method returns -1 when a key is
not found but the implementation returns 0L; update the Javadoc @return
description to state that the method returns 0L if the key is not found (or
adjust both doc and description text to match the 0L behavior), ensuring the
comment and code agree.
| public static CompletableFuture<Void> loadCachesAsync(String newRsProfileKey) { | ||
| // Ensure only one load operation at a time | ||
| if (cacheLoadingInProgress.compareAndSet(false, true)) { | ||
| CompletableFuture<Void> loadOperation = CompletableFuture.runAsync(() -> { | ||
| try { | ||
| retryLoadCacheWithValidation(newRsProfileKey, 0); | ||
| } catch (Exception e) { | ||
| log.error("Failed during async cache loading for profile: {}", newRsProfileKey, e); | ||
| cacheLoadingInProgress.set(false); // reset flag on unexpected error | ||
| throw new RuntimeException("Async cache load failed", e); | ||
| } | ||
| }, getInstance().cacheManagerExecutor); | ||
|
|
||
| // Track the current operation | ||
| currentLoadOperation.set(loadOperation); | ||
|
|
||
| return loadOperation.whenComplete((result, ex) -> { | ||
| // Clear the operation reference when done | ||
| currentLoadOperation.compareAndSet(loadOperation, null); | ||
| if (ex != null) { | ||
| log.error("Async cache loading failed for profile: {}", newRsProfileKey, ex); | ||
| } else { | ||
| log.info("Async cache loading completed for profile: {}", newRsProfileKey); | ||
| } | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Async load future resolves before caches finish loading
loadCachesAsync wraps the retry logic in CompletableFuture.runAsync, but retryLoadCacheWithValidation schedules subsequent attempts on cleanupExecutor and returns immediately. When the first attempt finds no player name, the retry is queued and the CompletableFuture completes right away, yet the actual load is still pending. As a result handleProfileChange, shutdown waits, and the logging in whenComplete all observe a “completed” load while the real work is still running, so we can save/clear caches or shut down executors mid-load. Please refactor the retry flow so the returned future only completes once the final retry either loads successfully or definitively gives up (e.g., make retryLoadCacheWithValidation return/complete a CompletableFuture<Void> that you pass through the retries).
| // mark as loaded but stale until next save | ||
| CacheMetadata loadedMetadata = CacheMetadata.createWithCurrentUtcTime(VERSION, SESSION_ID, true, json.length(), configKey); | ||
| String updatedMetadataJson = gson.toJson(loadedMetadata, CacheMetadata.class); | ||
| Files.write(metadataFile, updatedMetadataJson.getBytes(), StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING); | ||
|
|
There was a problem hiding this comment.
Loading marks metadata stale permanently, blocking future loads
After a successful load you overwrite the metadata file with stale=true. If the client exits before another save occurs, the next startup sees that stale flag and refuses to load the cache even though the JSON is intact. In practice this bricks persistence for anyone who loads but never triggers a save in the same session. Please keep the metadata fresh (or avoid rewriting it) on load so that previously saved caches remain usable across sessions.
Suggested fix:
- CacheMetadata loadedMetadata = CacheMetadata.createWithCurrentUtcTime(VERSION, SESSION_ID, true, json.length(), configKey);
- String updatedMetadataJson = gson.toJson(loadedMetadata, CacheMetadata.class);
- Files.write(metadataFile, updatedMetadataJson.getBytes(), StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
+ // keep existing metadata so saved caches remain loadable until we successfully persist again📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // mark as loaded but stale until next save | |
| CacheMetadata loadedMetadata = CacheMetadata.createWithCurrentUtcTime(VERSION, SESSION_ID, true, json.length(), configKey); | |
| String updatedMetadataJson = gson.toJson(loadedMetadata, CacheMetadata.class); | |
| Files.write(metadataFile, updatedMetadataJson.getBytes(), StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING); | |
| // mark as loaded but stale until next save | |
| // keep existing metadata so saved caches remain loadable until we successfully persist again |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/cache/serialization/CacheSerializationManager.java
around lines 393 to 397, the code currently rewrites the metadata on load with
stale=true which permanently prevents future loads if the client exits before a
save; stop marking metadata stale on successful load — either avoid rewriting
the metadata file entirely during load, or if you must update it, write it with
stale=false (and update only timestamp/session fields as needed) so previously
saved caches remain usable across sessions. Ensure the metadata write path on
load does not flip the stale flag to true.
| if (!response.isSuccessful()) { | ||
| Microbot.log("Failed to send Discord notification. Error Code: " + response.code() | ||
| + " - URL marked as invalid: " + webHookUrl); | ||
| } |
There was a problem hiding this comment.
Inconsistent error message for unsuccessful HTTP responses.
An unsuccessful HTTP response (401, 404, 429, etc.) indicates a failed request, not an invalid URL format. The message "URL marked as invalid" is misleading here, as the URL format may be perfectly valid—the request could fail due to authentication, expired webhook, rate limiting, or wrong endpoint.
Consider revising the message:
- Microbot.log("Failed to send Discord notification. Error Code: " + response.code()
- + " - URL marked as invalid: " + webHookUrl);
+ Microbot.log("Failed to send Discord notification to URL: " + webHookUrl
+ + " - HTTP Error Code: " + response.code());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!response.isSuccessful()) { | |
| Microbot.log("Failed to send Discord notification. Error Code: " + response.code() | |
| + " - URL marked as invalid: " + webHookUrl); | |
| } | |
| if (!response.isSuccessful()) { | |
| Microbot.log("Failed to send Discord notification to URL: " + webHookUrl | |
| " - HTTP Error Code: " + response.code()); | |
| } |
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/discord/Rs2Discord.java
around lines 88 to 91, the error log for unsuccessful HTTP responses incorrectly
states "URL marked as invalid" which is misleading; change the log to reflect a
failed request and include the HTTP status code, response message/body if
available, and the webhook URL for context (e.g., "Failed to send Discord
notification. HTTP {code}: {message} - webhook: {webHookUrl}"), so capture
response.code() and response.message()/body() and pass them into Microbot.log
instead of claiming the URL is invalid.
| public int varbitValue() { | ||
| int ordinal = ordinal(); | ||
| //Since you get Varrock GE for free with Varrock, there's no varbit value for it | ||
| return ordinal > 2 ? ordinal : ordinal - 1; | ||
| return ordinal; | ||
| } |
There was a problem hiding this comment.
Restore the Varrock GE varbit gap
getAvailableTeleports still treats varbit value 1 as “Varrock (+ GE if diary)”, which reflects the fact that Varrock GE never had its own varbit slot. Returning ordinal() here now reports Varrock GE’s ordinal (1) as if it mapped to a real varbit, so any UI/logic consuming varbitValue() will think a standalone Varrock GE varbit exists and attempt to read/write index 1, clobbering the actual Varrock entry. Please keep the old offset/sentinel (or otherwise skip Varrock GE explicitly) so the mapping stays consistent with the remaining logic and the in-game varbits.
🤖 Prompt for AI Agents
In
runelite-client/src/main/java/net/runelite/client/plugins/microbot/util/poh/data/NexusPortal.java
around lines 75 to 79, varbitValue() currently returns ordinal() which
incorrectly claims Varrock GE has its own varbit slot; restore the original gap
by returning a sentinel for the Varrock GE enum constant (e.g., -1 or a
dedicated SKIP value) and for any enum constants after Varrock GE return
ordinal()-1 (shift down by one) so the varbit indices keep the historical hole
and do not clobber the real Varrock varbit.
| public Rs2InventorySetup(String name, ScheduledFuture<?> mainScheduler) { | ||
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream().filter(Objects::nonNull).filter(x -> x.getName().equalsIgnoreCase(name)).findFirst().orElse(null); | ||
| _mainScheduler = mainScheduler; | ||
| if (inventorySetup == null) { | ||
| Microbot.showMessage("Inventory load with name " + name + " not found!", 10); | ||
| Microbot.pauseAllScripts.compareAndSet(false, true); | ||
| } | ||
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream().filter(Objects::nonNull).filter(x -> x.getName().equals("default")).findFirst().orElse(null); | ||
| } | ||
| _mainScheduler = mainScheduler; |
There was a problem hiding this comment.
Fallback to "default" setup doesn't guarantee non-null inventorySetup.
The constructor falls back to "default" if the requested setup isn't found (lines 53-55), but there's no handling if "default" also doesn't exist. In that case, inventorySetup remains null, yet there's no validation or warning. This creates a timing dependency with the automatic "default" creation in MInventorySetupsPlugin.java line 328.
Methods like getInventoryItems() (line 462) and getEquipmentItems() (line 471) will throw NPEs if inventorySetup is null, since they call inventorySetup.getInventory() and inventorySetup.getEquipment() directly before the null filter.
Consider adding validation:
if (inventorySetup == null) {
inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream().filter(Objects::nonNull).filter(x -> x.getName().equals("default")).findFirst().orElse(null);
}
+if (inventorySetup == null) {
+ Microbot.log("Inventory setup '" + name + "' and fallback 'default' not found", Level.ERROR);
+}
_mainScheduler = mainScheduler;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Rs2InventorySetup(String name, ScheduledFuture<?> mainScheduler) { | |
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream().filter(Objects::nonNull).filter(x -> x.getName().equalsIgnoreCase(name)).findFirst().orElse(null); | |
| _mainScheduler = mainScheduler; | |
| if (inventorySetup == null) { | |
| Microbot.showMessage("Inventory load with name " + name + " not found!", 10); | |
| Microbot.pauseAllScripts.compareAndSet(false, true); | |
| } | |
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream().filter(Objects::nonNull).filter(x -> x.getName().equals("default")).findFirst().orElse(null); | |
| } | |
| _mainScheduler = mainScheduler; | |
| public Rs2InventorySetup(String name, ScheduledFuture<?> mainScheduler) { | |
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream() | |
| .filter(Objects::nonNull) | |
| .filter(x -> x.getName().equalsIgnoreCase(name)) | |
| .findFirst() | |
| .orElse(null); | |
| if (inventorySetup == null) { | |
| inventorySetup = MInventorySetupsPlugin.getInventorySetups().stream() | |
| .filter(Objects::nonNull) | |
| .filter(x -> x.getName().equals("default")) | |
| .findFirst() | |
| .orElse(null); | |
| } | |
| if (inventorySetup == null) { | |
| Microbot.log("Inventory setup '" + name + "' and fallback 'default' not found", Level.ERROR); | |
| } | |
| _mainScheduler = mainScheduler; | |
| } |
3 participants
POH transports
Async cache improvements