[Biquaternions]

This PR originally adda an unsupported config option that reintroduces a vanilla bug where effects might not get consumed, primarily used for raid farms to reduce the consumption of bad omen potions.

Notice how in vanilla the effect Raid Omen is applied on top of Bad Omen without consuming the latter. In contrast, Paper consumes Bad Omen and only Raid Omen remains (which is probably the intended behavior by Mojang).

Vanilla behavior (tested in singleplayer):

Screencast_20261025_174525.webm

Paper behavior:

Screencast_20261025_174347.webm

Test world:

World:
world.zip

Coordinates:
-1547 72 -1013

Spark report for Paper, all vanilla configs applied

https://spark.lucko.me/ee4z8eks7S

Technical Details

This behavior was introduced by SPIGOT-540 to fix an entity tick exception (and a server crash) in version 1.8, see 51693d4
Mojang has since introduced a try/catch block to ignore the ConcurrentModificationException to address the issue in SPIGOT-540 which in turn has caused the new behavior in vanilla, but also making SPIGOT-540 not really required anymore in a setup that prioritizes vanilla behavior.

The ConcurrentModificationException is not actually triggered by an asynchronous modification, which explains why it can be easily re-created with a timed minecart. What happens is the following:

1. LivingEntity#tickEffects creates an iterator for the activeEffects map.
2. LivingEntity#tickEffects iterates over minecraft:bad_omen and executes MobEffectInstance#tickServer.
3. BadOmenEffect#applyEffectTick executes applyEffectTick which adds minecraft:raid_omen to the activeEffects map.
4. Because the activeEffects map was modified during the lifetime of the iterator, when LivingEntity#tickEffects executes HashIterator#remove, it will throw a CME, even if the operation was singlethreaded.

This is a log created by printing a stacktrace when adding minecraft:raid_omen and forcing the try/catch to throw instead of ignore the exception:
https://mclo.gs/QWUWmU9

After looking at all the calls of addEffect and removeEffect, there are no concurrent calls for effect removal, but there's a single one for effect additions, specifically for spiders. This can happen if a datapack adds spiders as a possible spawn during world generation, and there's already a check in place to prevent an asynchronous call of the effect change event, and the entire addEffect has already been running asynchronously despite SPIGOT-540 due to the entity not having ticked yet at this point and therefore tickingEffects remaining as false.

Plugins are also already forbidden from adding or removing effects asynchronously due to the AsyncCatcher, so even plugins can't really trigger a CME after this removal.

[roccodev]

The config name is a bit misleading, no? When it's set to true, which is the old paper behavior, the concurrent change is not prevented (the effect gets removed). It's vanilla that's preventing the concurrent change by bailing out of the loop (what a weird hack may I add)

Actually that's not true for bad omen, they return false so the iterator takes care of it. How does it not throw an exception though, when they're adding raid omen during it? The only case I imagine is that the effect is the last in the iteration, I think you should test with more than 1 effect

[Biquaternions]

Good call about the config name!

I've been looking into this a little bit deeper and I think I know why the CME happens and why add/remove don't throw:
There's no actual concurrent access, which explains why this can be re-created consistently with just a timed minecart, according to this log, BadOmenMobEffect is ticked, adds a new entry to effect map, which causes the iterator (which already exists) to no longer match the modcount and trigger the CME.

See:
https://mclo.gs/QWUWmU9

So, the final fix is probably a bit cleaner and won't require a config option, I'll be updating it in a while. There is a conccurent access that can happen during world generation apparently, so I'll have to add some different handling for that.

[Warriorrrr]

Dropping a fix that was working as it should to intentionally re-introduce a bug is really not it for me. I still think that if you want to use this bug, it'd be best to create a plugin for it instead of us dropping fixes or adding unsupported settings.

[Biquaternions]

I would normally have the same opinion, however in this case the fix in SPIGOT-540 was originally implemented to address a server crash, which has already been addressed by Mojang, so there's two fixes in place for the exact same issue.

I personally prefer the approach taken by Spigot, since is cleaner and produces the expected behavior Mojang probably wanted, however since the original issue is already handled, I think it is possible to prioritize vanilla behavior on this one until Mojang decides to fix it themselves.

Alternatively, since this can be considered an exploit, having it as an unsupported setting I think is a valid approach, specially given that is only a single line of diff, see:
5201fd7

[electronicboy]

Mojang doesn't deal with plugins mutating state, them wrapping that in a CME catch is a bodge, not a fix, the behavior is flawed and undeterministic

[Biquaternions]

True, although plugins shouldn't be able to mutate the effect map asynchronously to trigger a CME, and effects are not data-driven currently to introduce a synchronous CME.

Still I don't discard the possibility via other means, I'll revert this to the old approach of adding a config option, given the primary use case for this would be a vanilla setup, which is unlikely to have such plugins.