Fix handling of transient locks during upgrade

We currently have a problem in the handling of locks in the `upgrade
<app>` command when a transitive dependency switches supervision trees.

If we start with:

      A
     / \
    B1  C
        |
        D
        |
        E

And upgrade to:

      A
     / \
    B2  C
    |   |
    E   D

by calling `rebar3 upgrade b`, we would expect the ugprade command to do
so directly. However we are currently left with the following lock file:

      A
     / \
    B2  C
        |
        D
        |
        E

This is _not_ critical since on the next run, the lock file is fixed
automatically through an install-deps sequence. However, this is a
jarring state in which to leave a project.

The reason is that we proceed the following way to handle an upgrade:

1. pick the name of the chosen app, and unlock it if it's at the level 0
2. grab a list of all its current children (and their own) and unlock
   them
3. do the same for all the other unlocked dependencies
4. pass this new lock internal format to the get-deps provider in
   upgrade mode
5. take the result and save it to the lock file.

What's interesting is that right now this yields invalid locks at
step 3, because we have removed the lock on B1, but not on E
(which we don't yet know is going to be a new child of the next
B version until we fetch it)

In step 4, we fetch dependencies, and in there we compare the
pre-upgrade locks and fetch structure with those expected in the current
lock state, and rebuild everything.

The fix in this commit adds a check there where if the app is in the
current lock set at a level deeper than the current one, the lock is
forced to be refreshed as if it were not there. This lock would get
refreshed on a blank run anyway since the current lock set would start
empty (rather than pre-populated by `upgrade`), and the level-order
traversal ensures that the locks remain safe in all cases.

Fixes #2030

2 files changed, 71 insertions, 4 deletions

diff --git a/src/rebar_prv_install_deps.erl b/src/rebar_prv_install_deps.erl
index 068c4c8..0187b4f 100644
--- a/src/rebar_prv_install_deps.erl
+++ b/src/rebar_prv_install_deps.erl
@@ -206,10 +206,23 @@ maybe_lock(Profile, AppInfo, Seen, State, Level) ->
                 default ->
                     case sets:is_element(Name, Seen) of
                         false ->
+                            %% Check whether the currently existing lock is
+                            %% deeper than the current one (which can happen
+                            %% during an upgrade). If the current app is
+                            %% shallower than the existing lock, replace the
+                            %% existing lock. This prevents weird transient
+                            %% lock-tree states (which would self-heal on a
+                            %% later run) after a `rebar3 upgrade <app>'
+                            %% command when a deep dep switches lineages for
+                            %% another newer parent.
                             Locks = rebar_state:lock(State),
-                            case lists:any(fun(App) -> rebar_app_info:name(App) =:= Name end, Locks) of
-                                true ->
+                            case find_app_and_level_by_name(Locks, Name) of
+                                {ok, _App, LockLvl} when LockLvl =< Level ->
                                     {sets:add_element(Name, Seen), State};
+                                {ok, App, _LockLvl} ->
+                                    LockedApp = rebar_app_info:dep_level(AppInfo, Level),
+                                    {sets:add_element(Name, Seen),
+                                     rebar_state:lock(State, [LockedApp | Locks -- [App]])};
                                 false ->
                                     {sets:add_element(Name, Seen),
                                      rebar_state:lock(State, rebar_app_info:dep_level(AppInfo, Level))}
@@ -426,3 +439,12 @@ not_needs_compile(App) ->
     not(rebar_app_info:is_checkout(App))
         andalso rebar_app_info:valid(App)
           andalso rebar_app_info:has_all_artifacts(App) =:= true.
+
+find_app_and_level_by_name([], _) ->
+    false;
+find_app_and_level_by_name([App|Apps], Name) ->
+    case rebar_app_info:name(App) of
+        Name -> {ok, App, rebar_app_info:dep_level(App)};
+        _ -> find_app_and_level_by_name(Apps, Name)
+    end.
+
diff --git a/test/rebar_upgrade_SUITE.erl b/test/rebar_upgrade_SUITE.erl
index c55456c..54ff18c 100644
--- a/test/rebar_upgrade_SUITE.erl
+++ b/test/rebar_upgrade_SUITE.erl
@@ -12,7 +12,7 @@ groups() ->
                 tree_a, tree_b, tree_c, tree_c2, tree_cj, tree_ac, tree_all,
                 delete_d, promote, stable_lock, fwd_lock,
                 compile_upgrade_parity, umbrella_config,
-                profiles, profiles_exclusion]},
+                profiles, profiles_exclusion, tree_migration]},
      {git, [], [{group, all}]},
      {pkg, [], [{group, all}]}].
 
@@ -512,7 +512,15 @@ upgrades(profiles_exclusion) ->
      ["A","B","C","E","F","H"],
      {"A", [{"A","1.0.0"}, "D", {"E","3.0.0"},
             {"B","2.0.0"}, {"F","2.0.0"}, "G",
-            {"C","1.0.0"}, {"H","4.0.0"}, "I"]}}.
+            {"C","1.0.0"}, {"H","4.0.0"}, "I"]}};
+upgrades(tree_migration) ->
+    {[{"B", "1.0.0", []},
+      {"C", "1.0.0", [{"D","1.0.0",[{"E", "1.0.0", []}]}]}],
+     [{"B", "2.0.0", [{"E","1.0.0",[]}]},
+      {"C", "1.0.0", [{"D","1.0.0",[]}]}],
+     ["B"],
+     {"B", [{"A","1.0.0"}, "D", {"E","1.0.0"},
+            {"B","2.0.0"}]}}.
 
 %% TODO: add a test that verifies that unlocking files and then
 %% running the upgrade code is enough to properly upgrade things.
@@ -702,6 +710,43 @@ profiles(Config) ->
 
 profiles_exclusion(Config) -> profiles(Config).
 
+tree_migration(Config) ->
+    apply(?config(mock, Config), []),
+    ConfigPath = ?config(rebarconfig, Config),
+    {ok, RebarConfig} = file:consult(ConfigPath),
+    %% Install dependencies before re-mocking for an upgrade
+    rebar_test_utils:run_and_check(Config, RebarConfig, ["lock"], {ok, []}),
+    {App, _Unlocks} = ?config(expected, Config),
+
+    meck:new(rebar_prv_upgrade, [passthrough]),
+    meck:expect(rebar_prv_upgrade, do, fun(S) ->
+                                               apply(?config(mock_update, Config), []),
+                                               meck:passthrough([S])
+                                       end),
+    NewRebarConf = rebar_test_utils:create_config(filename:dirname(ConfigPath),
+                                                  [{deps, ?config(next_top_deps, Config)}]),
+    {ok, NewRebarConfig} = file:consult(NewRebarConf),
+    {ok, NewState} = rebar_test_utils:run_and_check(
+        Config, NewRebarConfig, ["upgrade", App], return
+    ),
+    meck:unload(rebar_prv_upgrade),
+    %% Check that the internal state properly has E with a lock-level
+    %% of 1.
+    Locks = rebar_state:lock(NewState),
+    [Locked] = [X || X <- Locks, rebar_app_info:name(X) =:= <<"E">>],
+    ?assertEqual(1, rebar_app_info:dep_level(Locked)),
+    %% Check that the lockfile on disk agrees
+    AppDir = ?config(apps, Config),
+    Lockfile = filename:join([AppDir, "rebar.lock"]),
+    case file:consult(Lockfile) of
+        {ok, [{_Vsn, Prop}|_]} -> % packages
+            ?assertMatch({<<"E">>, _, 1}, lists:keyfind(<<"E">>, 1, Prop));
+        {ok, [Prop]} -> % git source
+            ?assertMatch({<<"E">>, _, 1}, lists:keyfind(<<"E">>, 1, Prop))
+    end,
+    ok.
+
+
 run(Config) ->
     apply(?config(mock, Config), []),
     ConfigPath = ?config(rebarconfig, Config),