Self-conflicts treated as actual conflicts at upgrade

[DasSkelett]

Background

CKAN Version:
v1.25.1

KSP Version:
v1.4.2.2110

Operating System:
Windows10 64bit

Have you made any manual changes to your GameData folder (i.e., not via CKAN)?
No

Problem

What steps did you take in CKAN?
a) Marked all mods to update via Add available updates-button and clicked apply
b) Marked all mods manually (not via button)

What did you expect to happen?
a) Mods to update show in changset tab
b) Apply changes-button gets activated

What happened instead?
a) Mods not showing in changeset tab, clicking apply does nothing
b) Apply changes-button doesn't get activated

Screenshots:
a) Selecting all via button:

then clicking apply (no changes listed):

b) Selecting manually (button won't get activated):

Thoughts

This might either be because scatterer has its new version listed as conflict:

or the only change since v1.25.0 affecting anything in that area I know would be #2425
Scratch that, I can't imagine how refreshing the row could cause this.

[DasSkelett]

So uninstalling and reinstalling worked perfectly.

[HebaruSan]

Confirmed that the weird self-conflict is a factor. I installed v0.0329_for_1.4.2 and clicked Refresh and Add available updates, then saw the empty changeset problem as reported. Then I restarted after manually editing my registry.json to remove the conflict of Scatterer with Scatterer, and now it's not empty:

(Not sure why the sunflare and config packages aren't listed there, since they're also checked for upgrade.)

The self-conflict was added in KSP-CKAN/NetKAN#4541, but there's no explanation other than fixing #1870, which has a discussion that seems to be outdated now about SVE-HighResolution including Scatterer somehow. Current StockVisualEnhancements recommends Scatterer itself with no funny business. I think maybe one of these had "provides": [ "Scatterer" ] at some point?

[DasSkelett]

Maybe a solution would be ignoring self-conflicts on updates?
That feels a bit dirty though...

[HebaruSan]

Well, first I'd like to get a better understanding of exactly how and why it breaks and whether this is in fact a recent problem. It's possible that this is highlighting some logic error deep within the relationship resolver, the fixing of which could improve general robustness. Or it's possible that the upgrade logic is tripping over itself somewhere along the line as it attempts to reconcile removing and installing the same identifier in one step.

[HebaruSan]

Confirmed the speculation about provides: KSP-CKAN/NetKAN#4809, KSP-CKAN/NetKAN#5414

We probably can and should remove the self-conflict now, but preferably after fixing the problems it causes in the upgrade logic.

[HebaruSan]

This exception is being caught:

CKAN/GUI/Main.cs

Lines 604 to 610 in 29c8025

	catch (InconsistentKraken)
	{
	// Need to be recomputed due to ComputeChangeSetFromModList possibly changing it with too many provides handling.
	user_change_set = mainModList.ComputeUserChangeSet();
	new_conflicts = MainModList.ComputeConflictsFromModList(registry, user_change_set, CurrentInstance.VersionCriteria());
	full_change_set = null;
	}

[HebaruSan]

And this is the content of that exception:

[HebaruSan]

That's coming from here:

CKAN/Core/Relationships/SanityChecker.cs

Lines 43 to 49 in 29c8025

	// Conflicts are more difficult. Mods are allowed to conflict with themselves.
	// So we walk all our mod conflicts, find what (if anything) provide those
	// conflicts, and return false if it's not the module we're examining.
	foreach (var kvp in FindConflicting(modules, dlls?.ToHashSet(), dlc))
	{
	errors.Add($"{kvp.Key} conflicts with {kvp.Value}");
	}

So it looks like a regression of the "Mods are allowed to conflict with themselves" behavior.

[HebaruSan]

OK, it looks like what's happening is that SanityChecker is being asked to validate a hypothetical situation in which updates are installed without removing the old versions. And it always worked that way, but prior to #2339, the exception was avoided because conflicts were done only in terms of identifiers without regard to versions, so the old version of a mod was always considered not to conflict with the newer version simply because their identifiers were the same. We have Scatterer 1 and Scatterer 2 sitting next to each other, and now our "self conflict" check is version-specific, so they are counted as conflicting with one another (but not themselves). This loop skipped "ourselves" as it said, but also other versions of ourselves!:

CKAN/Core/Relationships/SanityChecker.cs

Lines 71 to 78 in b75a450

	// If something does conflict with us, and it's not ourselves, that's a fail.
	foreach (string provider in providers[conflict.name])
	{
	if (provider != mod.identifier)
	{
	errors.Add(string.Format("{0} conflicts with {1}.", mod.identifier, provider));
	}
	}

In effect, the weirdness of this aspect of upgrading was masked by #2325, and fixing that issue brought it forward. I see two ways to fix it:

1. Make the equivalent logic in SanityChecker work only based on identifiers again, so a mod list with multiple versions of the same mod would be considered consistent; or
2. Revise the upgrade logic to prepare a more complete hypothetical scenario in which the old versions are removed before passing to SanityChecker

The first option would be simpler and safer, but the second would be cleaner in terms of how it "should" work.

[politas]

It sounds like we need a more robust model of a "change" that involves "(remove [a,b,c,..]) and (add [x,y,x,...])" and stop trying to consider "upgrade" as a unique change type.

[Sunesha]

Experienced as reported in first post. Only seen this concerning the mods "Scatterer" and "VesselMover Continued".

[HebaruSan]

Thanks @Sunesha, VesselMoverContinued fits the profile in this issue (conflicts with itself, but via a provides property rather than directly), and it seems to be fixed by the change in #2430.