fix(sync): prevent luarocks install tree race conditions

lua/rocks/commands.lua

@@ -7,12 +7,13 @@
 --- command	  	                     action
 ---------------------------------------------------------------------------------
 ---
---- install [rock] [version?]  	     install {rock} with optional {version}.
---- prune [rock]                     uninstall {rock} and its stale dependencies,
+--- install [rock] [version?]  	     Install {rock} with optional {version}.
+--- prune [rock]                     Uninstall {rock} and its stale dependencies,
 ---                                  and remove it from rocks.toml.
---- sync                             synchronize installed rocks with rocks.toml.
---- update                           search for updated rocks and install them.
---- edit                             edit the rocks.toml file.
+--- sync                             Synchronize installed rocks with rocks.toml.
+---                                  It may take more than one sync to prune all rocks that can be pruned.
+--- update                           Search for updated rocks and install them.
+--- edit                             Edit the rocks.toml file.
 ---
 ---@brief ]]
 ---

lua/rocks/luarocks.lua

@@ -21,20 +21,39 @@ local constants = require("rocks.constants")
 local config = require("rocks.config.internal")
 local nio = require("nio")
 
+---@class LuarocksCliOpts: SystemOpts
+---@field synchronized? boolean Whether to wait for and acquire a lock (recommended for file system IO, default: `true`)
+
+local lock = nio.control.future()
+lock.set(true) -- initialise as unlocked
+
 ---@param args string[] luarocks CLI arguments
 ---@param on_exit (function|nil) Called asynchronously when the luarocks command exits.
 ---   asynchronously. Receives SystemCompleted object, see return of SystemObj:wait().
----@param opts? SystemOpts
+---@param opts? LuarocksCliOpts
 ---@return vim.SystemObj
 ---@see vim.system
 luarocks.cli = function(args, on_exit, opts)
+    opts = opts or {}
+    opts.synchronized = opts.synchronized ~= nil and opts.synchronized or false
+    local on_exit_wrapped = on_exit and vim.schedule_wrap(on_exit)
+    if opts.synchronized then
+        lock.wait()
+        lock = nio.control.future()
+        on_exit_wrapped = vim.schedule_wrap(function(...)
+            pcall(lock.set, true)
+            if on_exit then
+                on_exit(...)
+            end
+        end)
+    end
     local luarocks_cmd = vim.list_extend({
         config.luarocks_binary,
         "--lua-version=" .. constants.LUA_VERSION,
         "--tree=" .. config.rocks_path,
         "--server='https://nvim-neorocks.github.io/rocks-binaries/'",
     }, args)
-    return vim.system(luarocks_cmd, opts, on_exit and vim.schedule_wrap(on_exit))
+    return vim.system(luarocks_cmd, opts, on_exit_wrapped)
 end
 
 ---Search luarocks.org for all packages.
@@ -46,7 +65,7 @@ luarocks.search_all = nio.create(function(callback)
     luarocks.cli({ "search", "--porcelain", "--all" }, function(obj)
         ---@cast obj vim.SystemCompleted
         future.set(obj)
-    end, { text = true })
+    end, { text = true, synchronized = false })
     ---@type vim.SystemCompleted
     local obj = future.wait()
     local result = obj.stdout

lua/rocks/operations.lua

@@ -30,10 +30,11 @@ local operations = {}
 ---@field wait fun() Wait in an async context. Does not block in a sync context
 ---@field wait_sync fun() Wait in a sync context
 
----@alias rock_table { [string]: Rock[] | string }
+---@alias rock_config_table { [string]: Rock|string }
+---@alias rock_table { [string]: Rock }
 
 ---Decode the user rocks from rocks.toml, creating a default config file if it does not exist
----@return { rocks?: rock_table, plugins?: rock_table }
+---@return { rocks?: rock_config_table, plugins?: rock_config_table }
 local function parse_user_rocks()
     local config_file = fs.read_or_create(config.config_path, constants.DEFAULT_CONFIG)
     return require("toml_edit").parse(config_file)
@@ -167,13 +168,26 @@ end)
 ---@param user_rocks? { [string]: Rock|string } loaded from rocks.toml if `nil`
 operations.sync = function(user_rocks)
     nio.run(function()
-        local has_errors = false
         local progress_handle = progress.handle.create({
             title = "Syncing",
             lsp_client = { name = constants.ROCKS_NVIM },
             percentage = 0,
         })
 
+        ---@type ProgressHandle[]
+        local error_handles = {}
+        ---@param message string
+        local function report_error(message)
+            table.insert(
+                error_handles,
+                progress.handle.create({
+                    title = "Error",
+                    lsp_client = { name = constants.ROCKS_NVIM },
+                    message = message,
+                })
+            )
+        end
+
         if user_rocks == nil then
             -- Read or create a new config file and decode it
             -- NOTE: This does not use parse_user_rocks because we decode with toml, not toml-edit
@@ -193,6 +207,7 @@ operations.sync = function(user_rocks)
                 }
             end
         end
+        ---@cast user_rocks rock_table
 
         local installed_rocks = state.installed_rocks()
 
@@ -202,143 +217,158 @@ operations.sync = function(user_rocks)
         ---@diagnostic disable-next-line: invisible
         local key_list = nio.fn.keys(vim.tbl_deep_extend("force", installed_rocks, user_rocks))
 
-        local actions = vim.empty_dict()
-        ---@cast actions (fun():any)[]
-
-        local ct = 1
-
-        local dependencies = vim.empty_dict()
-        ---@cast dependencies {[string]: RockDependency}
-
-        local to_remove_keys = vim.empty_dict()
-        ---@cast to_remove_keys string[]
-
+        local to_install = vim.empty_dict()
+        ---@cast to_install string[]
+        local to_updowngrade = vim.empty_dict()
+        ---@cast to_updowngrade string[]
+        local to_prune = vim.empty_dict()
+        ---@cast to_prune string[]
         for _, key in ipairs(key_list) do
             if user_rocks[key] and not installed_rocks[key] then
-                nio.scheduler()
-                progress_handle:report({
-                    message = ("Installing: %s"):format(key),
-                })
-                table.insert(actions, function()
-                    -- If the plugin version is a development release then we pass `dev` as the version to the install function
-                    -- as it gets converted to the `--dev` flag on there, allowing luarocks to pull the `scm-1` rockspec manifest
-                    local future
-                    if vim.startswith(user_rocks[key].version, "scm-") then
-                        future = operations.install(user_rocks[key].name, "dev")
-                    else
-                        future = operations.install(user_rocks[key].name, user_rocks[key].version)
-                    end
-                    local success, ret = pcall(future.wait)
-
-                    ct = ct + 1
-                    nio.scheduler()
-                    if not success then
-                        has_errors = true
-                        -- TODO: Keep track of failures: #55
-                        progress_handle:report({
-                            message = ("Failed to install %s: %s"):format(key, vim.inspect(ret)),
-                            percentage = math.floor(ct / #actions * 100),
-                        })
-                        return
-                    end
-                    progress_handle:report({
-                        message = ("Installed: %s"):format(key),
-                        percentage = math.floor(ct / #actions * 100),
-                    })
-                    return ret
-                end)
+                table.insert(to_install, key)
             elseif
                 user_rocks[key]
                 and installed_rocks[key]
                 and user_rocks[key].version ~= installed_rocks[key].version
             then
-                local is_downgrading = vim.version.parse(user_rocks[key].version)
-                    < vim.version.parse(installed_rocks[key].version)
+                table.insert(to_updowngrade, key)
+            elseif not user_rocks[key] and installed_rocks[key] then
+                table.insert(to_prune, key)
+            end
+        end
 
-                nio.scheduler()
-                progress_handle:report({
-                    message = is_downgrading and ("Downgrading: %s"):format(key) or ("Updating: %s"):format(key),
-                })
+        local ct = 1
+        local action_count = #to_install + #to_updowngrade + #to_prune
 
-                table.insert(actions, function()
-                    local future = operations.install(user_rocks[key].name, user_rocks[key].version)
-                    local success, ret = pcall(future.wait)
-
-                    ct = ct + 1
-                    nio.scheduler()
-                    if not success then
-                        has_errors = true
-                        progress_handle:report({
-                            message = ("Failed to downgrade %s: %s"):format(key, vim.inspect(ret)),
-                            percentage = math.floor(ct / #actions * 100),
-                        })
-                        return
-                    end
-                    progress_handle:report({
-                        message = is_downgrading and ("Downgraded: %s"):format(key) or ("Upgraded: %s"):format(key),
-                        percentage = math.floor(ct / #actions * 100),
-                    })
+        local function get_progress_percentage()
+            return math.floor(ct / action_count * 100)
+        end
 
-                    return ret
-                end)
-            elseif not user_rocks[key] and installed_rocks[key] then
-                table.insert(to_remove_keys, key)
+        for _, key in ipairs(to_install) do
+            nio.scheduler()
+            progress_handle:report({
+                message = ("Installing: %s"):format(key),
+            })
+            -- If the plugin version is a development release then we pass `dev` as the version to the install function
+            -- as it gets converted to the `--dev` flag on there, allowing luarocks to pull the `scm-1` rockspec manifest
+            local future
+            if vim.startswith(user_rocks[key].version, "scm-") then
+                future = operations.install(user_rocks[key].name, "dev")
+            else
+                future = operations.install(user_rocks[key].name, user_rocks[key].version)
             end
+            local success, ret = pcall(future.wait)
 
-            if installed_rocks[key] then
-                -- NOTE(vhyrro): It is not possible to use the vim.tbl_extend or vim.tbl_deep_extend
-                -- functions here within the async context. It simply refuses to work.
-                for k, v in pairs(state.rock_dependencies(installed_rocks[key])) do
-                    dependencies[k] = v
-                end
+            ct = ct + 1
+            nio.scheduler()
+            if not success then
+                -- TODO: Keep track of failures: #55
+                progress_handle:report({
+                    percentage = get_progress_percentage(),
+                })
+                report_error(("Failed to install %s: %s"):format(key, vim.inspect(ret)))
             end
+            progress_handle:report({
+                message = ("Installed: %s"):format(key),
+                percentage = get_progress_percentage(),
+            })
         end
+        for _, key in ipairs(to_updowngrade) do
+            local is_downgrading = vim.version.parse(user_rocks[key].version)
+                < vim.version.parse(installed_rocks[key].version)
 
-        for _, key in ipairs(to_remove_keys) do
-            local is_dependency = dependencies[key] ~= nil
-            if not is_dependency then
-                nio.scheduler()
+            nio.scheduler()
+            progress_handle:report({
+                message = is_downgrading and ("Downgrading: %s"):format(key) or ("Updating: %s"):format(key),
+            })
+
+            local future = operations.install(user_rocks[key].name, user_rocks[key].version)
+            local success, ret = pcall(future.wait)
+
+            ct = ct + 1
+            nio.scheduler()
+            if not success then
                 progress_handle:report({
-                    message = ("Removing: %s"):format(key),
+                    percentage = get_progress_percentage(),
                 })
+                report_error(
+                    is_downgrading and ("Failed to downgrade %s: %s"):format(key, vim.inspect(ret))
+                        or ("Failed to upgrade %s: %s"):format(key, vim.inspect(ret))
+                )
+            end
+            progress_handle:report({
+                message = is_downgrading and ("Downgraded: %s"):format(key) or ("Upgraded: %s"):format(key),
+                percentage = get_progress_percentage(),
+            })
+        end
 
-                table.insert(actions, function()
-                    local future = operations.remove(installed_rocks[key].name)
-                    local success, ret = pcall(future.wait)
-
-                    ct = ct + 1
-                    nio.scheduler()
-                    if not success then
-                        has_errors = true
-                        -- TODO: Keep track of failures: #55
-                        progress_handle:report({
-                            message = ("Failed to install %s: %s"):format(key, vim.inspect(ret)),
-                            percentage = math.floor(ct / #actions * 100),
-                        })
-                        return
-                    end
-                    progress_handle:report({
-                        message = ("Removed: %s"):format(key),
-                        percentage = math.floor(ct / #actions * 100),
-                    })
-                    return ret
-                end)
+        -- Determine dependencies of installed user rocks, so they can be excluded from rocks to prune
+        -- NOTE(mrcjkb): This has to be done after installation,
+        -- so that we don't prune dependencies of newly installed rocks.
+        -- TODO: This doesn't guarantee that all rocks that can be pruned will be pruned.
+        -- Typically, another sync will fix this. Maybe we should do some sort of repeat... until?
+        installed_rocks = state.installed_rocks()
+        local dependencies = vim.empty_dict()
+        ---@cast dependencies {[string]: RockDependency}
+        for _, installed_rock in pairs(installed_rocks) do
+            for k, v in pairs(state.rock_dependencies(installed_rock)) do
+                dependencies[k] = v
             end
         end
 
-        if not vim.tbl_isempty(actions) then
-            -- TODO: Error handling
-            nio.gather(actions)
-        else
+        ---@type string[]
+        local prunable_rocks = vim.iter(to_prune)
+            :filter(function(key)
+                return dependencies[key] == nil
+            end)
+            :totable()
+
+        action_count = #to_install + #to_updowngrade + #prunable_rocks
+
+        if ct == 0 and vim.tbl_isempty(prunable_rocks) then
+            nio.scheduler()
             progress_handle:report({ message = "Everything is in-sync!", percentage = 100 })
+            progress_handle:finish()
+            return
         end
-        if has_errors then
+
+        ---@diagnostic disable-next-line: invisible
+        local user_rock_names = nio.fn.keys(user_rocks)
+        -- Prune rocks sequentially, to prevent conflicts
+        for _, key in ipairs(prunable_rocks) do
+            nio.scheduler()
+            progress_handle:report({
+                message = ("Removing: %s"):format(key),
+            })
+
+            local success = operations.remove_recursive(installed_rocks[key].name, user_rock_names)
+
+            ct = ct + 1
+            nio.scheduler()
+            if not success then
+                -- TODO: Keep track of failures: #55
+                progress_handle:report({
+                    percentage = get_progress_percentage(),
+                })
+                report_error(("Failed to remove %s"):format(key))
+            else
+                progress_handle:report({
+                    message = ("Removed: %s"):format(key),
+                    percentage = get_progress_percentage(),
+                })
+            end
+        end
+
+        if not vim.tbl_isempty(error_handles) then
             progress_handle:report({
                 title = "Error",
                 message = "Sync completed with errors!",
                 percentage = 100,
             })
             progress_handle:cancel()
+            for _, error_handle in pairs(error_handles) do
+                error_handle:cancel()
+            end
         else
             progress_handle:finish()
         end
@@ -393,7 +423,6 @@ operations.update = function()
 
         if not vim.tbl_isempty(actions) then
             nio.gather(actions)
-            fs.write_file(config.config_path, "w", tostring(user_rocks))
         else
             nio.scheduler()
             progress_handle:report({ message = "Nothing to update!", percentage = 100 })