Comprehensive break down + improvements Unowned shared state in Timer and InvUtils causes cross-module bugs, with proposed fixes

[sebastiaana8620-a11y]

Describe the feature

Hi, and some context first

(disocord name: sebastiaan09060)
I am Sebastiaan, I build software for a living. I have been using Meteor for a while and wanted to give something back that is more useful than a bug report, so I did a systematic pass over the module layer instead of just reporting the one thing that annoyed me.

I went through all 196 modules in systems/modules, one at a time, and also read TrouserStreak (82 modules) and the Wurst Meteor addon (61 modules), since both call into Meteor's utility layer directly and I wanted to see how far Meteor's internals actually reach.

First, credit where it is due: the mixin discipline in this codebase is cleaner than a lot of professional Java I have worked with, and the module abstraction is genuinely well designed. What follows is not a list of complaints, it is three things where I think a small change to a shared component would close a whole family of downstream bugs at once.

Every claim below has been checked against the source, with file and line references, and I have included a concrete fix for each. Happy to open PRs for any of them.

The findings in short

1. NoSlow and FastClimb can permanently strand Timer's override, leaving the entire client at a modified game speed with no in-game recovery. Three of the five modules that touch the override already handle this correctly, these two do not.

2. InvUtils.previousSlot is a single shared public static field that all 57 slot-swapping call sites contend for, so one module's swap silently destroys another's pending restore.

3. ReflectInit.init uses return where continue is meant, so one addon can abort initialisation for every addon after it.

The first two are the interesting ones because they are the same underlying shape: shared mutable state with no ownership. Fixing the ownership model fixes many symptoms at once.

Problem 1: Timer's override can be permanently stranded

What the code does

Timer holds the override as a single field on the module singleton:

// systems/modules/world/Timer.java:26-39
public static final double OFF = 1;
private double override = 1;

public double getMultiplier() {
    return override != OFF ? override : (isActive() ? multiplier.get() : OFF);
}

public void setOverride(double override) {
    this.override = override;
}

The important detail is the order of that ternary in getMultiplier(). If override != OFF, the override is returned regardless of whether the Timer module itself is enabled. So a stale override cannot be cleared by turning Timer off, and it cannot be cleared from the GUI at all.

Five modules call setOverride. Three of them clean up after themselves:

// Speed.java:108-110, LongJump.java:132-134, Burrow.java:157-159
@Override
public void onDeactivate() {
    Modules.get().get(Timer.class).setOverride(Timer.OFF);
}

NoSlow and FastClimb do not. Both have an onActivate but no onDeactivate, and both only reset the override from inside a tick handler:

// NoSlow.java:162-172
@EventHandler
private void onPreTick(TickEvent.Pre event) {
    if (web.get() == WebMode.Timer) {
        if (mc.level.getBlockState(mc.player.blockPosition()).getBlock() == Blocks.COBWEB && !mc.player.onGround()) {
            resetTimer = false;
            Modules.get().get(Timer.class).setOverride(webTimer.get());
        } else if (!resetTimer) {
            Modules.get().get(Timer.class).setOverride(Timer.OFF);
            resetTimer = true;
        }
    }
}

FastClimb.java:63-73 has the identical shape for its climbing check.

Why that strands the value

The reset only happens on a later tick, in the else branch, once the condition has become false. But when a module is deactivated its event handlers are unsubscribed, so that later tick never arrives.

Reproduction:

1. Enable NoSlow with web mode set to Timer.
2. Stand in a cobweb and off the ground, so setOverride(webTimer) runs and resetTimer is false.
3. Disable NoSlow while still in that state.
4. The handler is now unsubscribed, so the else branch never runs. override keeps the web value for the rest of the session.
5. Because getMultiplier() checks override != OFF before anything else, the whole game stays at that speed and toggling Timer on and off does not help.

FastClimb is the same: disable it mid-climb and the override is stranded.

The fix, minimal version

Add the same onDeactivate the other three modules already have, to both NoSlow and FastClimb:

@Override
public void onDeactivate() {
    Modules.get().get(Timer.class).setOverride(Timer.OFF);
    resetTimer = true;
}

That resolves the reported bug and is consistent with the existing pattern in Speed, LongJump and Burrow.

The fix I would actually suggest

The minimal fix leaves the design able to reproduce this. Any future module that forgets onDeactivate reintroduces it, and there is no way to notice until a user is stuck at 10x speed. Two options, both small:

Option A, ownership. Make the override only apply while its setter is still active:

public class Timer extends Module {
    public static final double OFF = 1;

    private Module owner;
    private double override = OFF;

    public double getMultiplier() {
        // an override from a module that is no longer active is ignored
        if (owner != null && !owner.isActive()) {
            owner = null;
            override = OFF;
        }
        return override != OFF ? override : (isActive() ? multiplier.get() : OFF);
    }

    public void setOverride(Module owner, double override) {
        // do not let a second module silently steal an active override
        if (this.owner != null && this.owner != owner && this.owner.isActive()) return;
        this.owner = override == OFF ? null : owner;
        this.override = override;
    }
}

This makes the bug structurally impossible rather than fixed in two places, and it also fixes a second problem the current code has: if NoSlow and Speed both want the override at once, whichever writes last wins and the other silently loses its effect with no indication.

Option B, central release. If you would rather not change the setOverride signature, release the override centrally when any module is disabled, in whatever code path handles module deactivation:

// wherever a module is toggled off
Modules.get().get(Timer.class).releaseIfOwnedBy(module);

Option A is the one I would pick, because it also handles the contention case. I am happy to write either.

Problem 2: InvUtils.previousSlot is unowned shared state

What the code does

// utils/player/InvUtils.java:27
public static int previousSlot = -1;

// utils/player/InvUtils.java:149-165
public static boolean swap(int slot, boolean swapBack) {
    if (slot == SlotUtils.OFFHAND) return true;
    if (slot < 0 || slot > 8) return false;
    if (swapBack && previousSlot == -1) previousSlot = mc.player.getInventory().getSelectedSlot();
    else if (!swapBack) previousSlot = -1;

    mc.player.getInventory().setSelectedSlot(slot);
    ((IMultiPlayerGameMode) mc.gameMode).meteor$syncSelected();
    return true;
}

public static boolean swapBack() {
    if (previousSlot == -1) return false;

    boolean return_ = swap(previousSlot, false);
    previousSlot = -1;
    return return_;
}

There is one previousSlot for the entire client, it is public so addons write to it directly as well, and there are 57 call sites in Meteor alone.

The two ways this breaks

Case 1, a restore gets destroyed. swap(slot, false) sets previousSlot = -1 unconditionally.

Module A: swap(toolSlot, true)     -> previousSlot = 0   (A intends to restore to 0)
Module B: swap(weaponSlot, false)  -> previousSlot = -1  (A's restore data is gone)
Module A: swapBack()               -> previousSlot == -1, returns false, no restore

Module A never returns the player to slot 0. The user is left holding whatever A swapped to. This is the mechanism behind the reports of AutoTool not swapping back when KillAura is also running.

Case 2, a restore goes to the wrong slot. swap(slot, true) only records when previousSlot == -1.

Player on slot 3.
Module A: swap(5, true)   -> previousSlot = 3
Module B: swap(7, true)   -> previousSlot already set, so B records nothing
Module B: swapBack()      -> restores to 3, which was A's target, not B's
                          -> previousSlot = -1
Module A: swapBack()      -> returns false, A's restore is lost

Both modules end up wrong from one interleaving. Because the field is public static and addons touch it directly, this is not solvable inside individual modules, it has to be fixed in InvUtils.

The fix: hand out a token instead of sharing a field

The property you want is that a restore only applies if the caller still owns the swap, and that nesting restores to the outermost original slot rather than an intermediate one.

public final class InvUtils {
    /** Opaque handle returned by swap(). Only the owner can restore. */
    public static final class SwapToken {
        private final int restoreSlot;
        private boolean released;
        private SwapToken(int restoreSlot) { this.restoreSlot = restoreSlot; }
    }

    private static final ArrayDeque<SwapToken> swapStack = new ArrayDeque<>();

    /**
     * Swap to a slot and get a token to restore with.
     * Returns null if the swap could not be performed.
     */
    public static SwapToken swapTracked(int slot) {
        if (slot == SlotUtils.OFFHAND) return null;
        if (slot < 0 || slot > 8) return null;

        // nested swaps inherit the outermost restore target, so unwinding
        // always lands back where the player actually started
        int restore = swapStack.isEmpty()
            ? mc.player.getInventory().getSelectedSlot()
            : swapStack.peek().restoreSlot;

        SwapToken token = new SwapToken(restore);
        swapStack.push(token);
        setSlot(slot);
        return token;
    }

    /** Restore the swap this token owns. Safe to call in any order, and safe to call twice. */
    public static boolean swapBack(SwapToken token) {
        if (token == null || token.released) return false;
        token.released = true;

        // out-of-order release: another module swapped after us and still holds
        // the hand, so drop our claim without yanking the slot from under it
        if (swapStack.peek() != token) {
            swapStack.remove(token);
            return false;
        }

        swapStack.pop();
        // only the last outstanding swap actually restores
        if (swapStack.isEmpty()) setSlot(token.restoreSlot);
        return true;
    }

    private static void setSlot(int slot) {
        mc.player.getInventory().setSelectedSlot(slot);
        ((IMultiPlayerGameMode) mc.gameMode).meteor$syncSelected();
    }
}

Why each piece is there:

• Inheriting restoreSlot is what makes nesting correct. In case 2 above, both A and B now restore to slot 3, which is where the player actually was.
• Only restoring when the stack empties stops an inner module yanking the hand away from an outer one that is still mid-action.
• The released flag makes double calls harmless, which matters because module deactivation paths often call cleanup more than once.
• Removing a non-top token instead of ignoring it prevents a leak when modules are disabled in an order different from the order they swapped.

Migration, so this does not break the addon ecosystem

This is the part I would want to get right, because I found 99 call sites across 21 files in TrouserStreak and the Wurst addon touching InvUtils and Timer directly. Breaking the old API outright would break a lot of third-party code at once.

The old API can sit on top of the new one, so nothing breaks on day one:

@Deprecated // use swapTracked() / swapBack(SwapToken)
public static int previousSlot = -1;

private static SwapToken legacyToken;

@Deprecated // use swapTracked(slot)
public static boolean swap(int slot, boolean swapBack) {
    if (swapBack) {
        if (legacyToken == null) legacyToken = swapTracked(slot);
        else setSlot(slot);
        return legacyToken != null;
    }
    // non-restoring swap: release any legacy claim, then move
    if (legacyToken != null) { swapBack(legacyToken); legacyToken = null; }
    if (slot == SlotUtils.OFFHAND) return true;
    if (slot < 0 || slot > 8) return false;
    setSlot(slot);
    return true;
}

@Deprecated // use swapBack(SwapToken)
public static boolean swapBack() {
    if (legacyToken == null) return false;
    boolean ok = swapBack(legacyToken);
    legacyToken = null;
    return ok;
}

Legacy callers keep working with today's behaviour, migrated callers get correctness, and the two can coexist while addons catch up. Deprecation warnings then do the documentation work for you.

If you would rather not carry the shim, the alternative is a breaking change in a major release with a note to addon authors. Your call, and I would follow whichever you prefer.

Problem 3: ReflectInit aborts all remaining addons instead of skipping one

What the code does

// utils/ReflectInit.java:41-53
public static void init(Class<? extends Annotation> annotation) {
    for (Reflections reflection : reflections) {
        Set<Method> initTasks = reflection.getMethodsAnnotatedWith(annotation);
        if (initTasks == null) return;
        ...
    }
}

reflections holds one entry per addon. The return on line 44 exits the whole method, so if any single addon's lookup yields null, every addon after it in iteration order is never initialised. Nothing is logged, so the user sees some addons silently not working, and which ones depends on ordering.

continue is what the surrounding logic implies.

Being honest about severity

I should flag that in practice Reflections.getMethodsAnnotatedWith returns an empty set rather than null, so I could not construct a case where this actually fires today. Treat this as a latent bug and a robustness fix rather than an active one. The control flow is still wrong, and it is the kind of thing that becomes a confusing bug report the day a Reflections version or a classloader edge case changes that behaviour.

The fix

public static void init(Class<? extends Annotation> annotation) {
    for (Reflections reflection : reflections) {
        Set<Method> initTasks = reflection.getMethodsAnnotatedWith(annotation);
        if (initTasks == null || initTasks.isEmpty()) {
            MeteorClient.LOG.warn("ReflectInit: no @{} tasks found for {}, skipping",
                annotation.getSimpleName(), reflection);
            continue;
        }
        ...
    }
}

Two changes: continue instead of return, and a log line so a silent skip becomes a visible one. The log line is the part that actually saves debugging time later.

Other things I noticed but have not verified properly

I am separating these out deliberately. I have not confirmed them to the same standard as the three above, so please treat them as leads rather than reports. I did not want to pad a bug report with things I am not sure about.

• No static analysis in CI. Error Prone would catch discarded return values on immutable types, which is an easy class of bug to introduce with Minecraft's Vec3/Vec3d API where every operation returns a new instance rather than mutating. Worth a look at the places where a .add(...) or .multiply(...) result might not be assigned.

• onActivate and onDeactivate re-fire on world change. That means state a module builds at activation can accumulate across server hops, and anything world-scoped that a module persists can end up associated with the wrong server. A dedicated onWorldChanged hook would separate "the user toggled me" from "the world went away", which are currently the same signal.

• Silent catches in the settings layer. There are catch (Exception) blocks in the settings code that swallow the exception. If a config fails to deserialise, the user gets a silently reset setting instead of an error. Logging the setting name and the cause would make corrupted-config reports diagnosable.

If any of these are interesting I am happy to dig in properly and open separate issues with the same level of detail as above.

Closing

Thanks for the work on this project, and sorry for the length. I would rather give you something you can act on directly than a vague report.

I am happy to open PRs for problems 1, 2 and 3. For problem 2 I would want to agree the API shape with you before writing it, since it touches every calling module and the addon ecosystem, so let me know if the token approach is a direction you would accept or if you would rather solve it differently.

Before submitting a suggestion

• This feature doesn't already exist in the client. (I have checked every module and their settings on the latest dev build)

• This wasn't already suggested. (I have searched suggestions on GitHub and read the FAQ)

[machiecodes]

Thanks for taking the time to look into all this! We appreciate your desire to help, but there are several things that could be better approached here:

• Each of these 3 issues should be their own separate issue, this makes tracking fixes or discussion about them more organized
• While detail is certainly appreciated the length here is a bit long even when split, try including links to the source instead of inserting blocks here

And lastly that this seems to be written with AI assistance. We don't have a blanket "no AI" rule, but we prefer issues and PR descriptions to be written by hand because it demonstrates that you've put thought into what you're reporting and the solutions you propose.

If you're interested in contributing, we would welcome you opening these as separate issues in your own words. The timer issue could also be a relatively simple first PR :)