system/nxpkg: simplify lifecycle cleanup

Use shared cleanup paths and keep the command wrappers small.

Assisted-by: OpenAI Codex:gpt-5.6-sol
Signed-off-by: aviralgarg05 <gargaviral99@gmail.com>

Author: aviralgarg05

system/nxpkg/pkg_install.c

@@ -69,8 +69,7 @@ static int pkg_install_acquire_lock(FAR const char *name, FAR char *path,
  * Name: pkg_install_acquire_installed_lock
  *
  * Description:
- *   Acquire the global installed-database lock.  This serializes the
- *   read-modify-write sequence used by install, uninstall, and rollback.
+ *   Serialize installed database updates.
  *
  ****************************************************************************/
 
@@ -134,12 +133,7 @@ static int pkg_install_prune_oldest_version(
   size_t victim = entry->version_count;
   size_t i;
 
-  /* Versions are appended in install order, so the lowest index that
-   * isn't the active ("current") or rollback ("previous") version is the
-   * oldest one safe to drop.  Without this, a package updated more than
-   * PKG_INSTALLED_VERSIONS_MAX times becomes permanently un-installable
-   * (pkg_install_add_version would just fail forever).
-   */
+  /* Keep the current and rollback versions when pruning. */
 
   for (i = 0; i < entry->version_count; i++)
     {
@@ -156,16 +150,7 @@ static int pkg_install_prune_oldest_version(
       return -E2BIG;
     }
 
-  /* Deleting the pruned version's on-disk directory here, before this
-   * in-memory db update is even durably saved, left a real inconsistency
-   * window: if pkg_metadata_save_installed() subsequently failed (full
-   * SD card, I/O error), the payload was already gone but the last
-   * successfully-saved instpkg.jsn could still list that version as
-   * installed.  Hand the victim's version string back to the caller
-   * instead, so it can defer the actual directory removal until after
-   * the save succeeds - mirroring how this file already treats the
-   * installed db as authoritative everywhere else.
-   */
+  /* Let the caller delete this version after committing the database. */
 
   snprintf(pruned_version, pruned_version_size, "%s",
            entry->versions[victim]);
@@ -426,7 +411,7 @@ int pkg_install(FAR const char *name)
           goto errout;
         }
 
-      ret = pkg_acquire_source(source, tmp, lock);
+      ret = pkg_acquire_source(source, tmp);
       if (ret < 0)
         {
           pkg_error("acquire source failed: %d", ret);
@@ -583,12 +568,7 @@ int pkg_install(FAR const char *name)
       goto errout;
     }
 
-  /* Only remove the pruned version's payload directory now that the db
-   * update naming it gone is durably saved - see
-   * pkg_install_prune_oldest_version()'s comment for why doing this
-   * before the save could leave a saved db entry pointing at an
-   * already-deleted version if the save had failed instead.
-   */
+  /* Remove the pruned payload after committing the database. */
 
   if (pruned_version[0] != '\0')
     {
@@ -598,10 +578,7 @@ int pkg_install(FAR const char *name)
   ret = pkg_install_write_pointers(installed, manifest->name);
   if (ret < 0)
     {
-      /* The installed database is authoritative.  The pointer files are
-       * convenience mirrors and can be reconstructed from it, so failure
-       * to refresh one must not roll back a durably committed install.
-       */
+      /* Pointer files can be rebuilt from the installed database. */
 
       pkg_error("unable to refresh current/previous pointers: %d", ret);
     }
@@ -612,11 +589,7 @@ int pkg_install(FAR const char *name)
   ret = pkg_txn_write_state(name, PKG_TXN_ACTIVATED);
   if (ret < 0)
     {
-      /* The installed database has already been committed.  Do not enter
-       * the failure cleanup path here: it could remove a payload referenced
-       * by that database.  Transaction state is recovery bookkeeping and
-       * can be cleared below.
-       */
+      /* Do not remove payloads after the database commit. */
 
       pkg_error("txn state activated failed: %d", ret);
     }
@@ -644,11 +617,7 @@ int pkg_install(FAR const char *name)
       pkg_store_remove_file(tmp);
     }
 
-  /* Reclaim whatever was staged into the version directory (payload,
-   * manifest.jsn) before this failure - otherwise every failure past
-   * this point leaves a permanently orphaned, never-activated version
-   * directory with no way to reclaim it short of "remove".
-   */
+  /* Remove a version directory created by this failed install. */
 
   if (version_dir_created)
     {
@@ -682,9 +651,7 @@ int pkg_install(FAR const char *name)
   pkg_free(lock);
   pkg_free(installed_lock);
 
-  /* Callers such as nxstore use the errno to tell a download failure from
-   * a digest mismatch or an incompatible package.
-   */
+  /* Preserve negative errno values for library callers. */
 
   return ret;
 }
@@ -733,13 +700,7 @@ int pkg_list(FAR FILE *stream)
  * Name: pkg_uninstall
  *
  * Description:
- *   Remove every installed version of "name": their version directories
- *   (payload + manifest.jsn), the current/previous pointer files, any
- *   leftover txn.tx, the entry in the shared installed-packages database,
- *   and finally the now-empty package root directory.  Refuses to run
- *   while an install/update for the same package is in flight (a live
- *   lock.lk), since removing the store out from under it would corrupt
- *   whatever it's mid-writing.
+ *   Remove a package and all of its installed versions.
  *
  ****************************************************************************/
 
@@ -804,10 +765,7 @@ int pkg_uninstall(FAR const char *name)
       goto errout_with_installed_lock;
     }
 
-  /* Drop this entry from the authoritative database before removing its
-   * payloads.  A power loss can then leave reclaimable orphan files, but
-   * never a database entry that points at a payload already deleted.
-   */
+  /* Commit removal before deleting payloads. */
 
   removed = *entry;
   index = (size_t)(entry - db->entries);
@@ -872,11 +830,7 @@ int pkg_uninstall(FAR const char *name)
  * Name: pkg_rollback
  *
  * Description:
- *   Swap "name"'s current and previous installed versions.  The swap (as
- *   opposed to just clearing "previous") lets a second rollback undo the
- *   first.  Verifies the rollback target's version directory still
- *   exists on disk before committing any state change, and refuses to
- *   run while an install/update for the same package is in flight.
+ *   Swap the current and previous installed versions.
  *
  ****************************************************************************/
 
@@ -978,10 +932,7 @@ int pkg_rollback(FAR const char *name)
       goto errout_with_installed_lock;
     }
 
-  /* The installed database is authoritative, so commit it first.  Refresh
-   * the current/previous pointer files afterwards as convenience mirrors;
-   * they can be reconstructed from the database if either write fails.
-   */
+  /* Commit the database before refreshing pointer files. */
 
   ret = pkg_metadata_save_installed(db);
   if (ret < 0)

system/nxpkg/pkg_main.c

@@ -78,20 +78,12 @@ int main(int argc, FAR char *argv[])
           return EXIT_FAILURE;
         }
 
-      /* pkg_install() returns a real negative errno on the meaningful
-       * pipeline failures (nxstore uses that directly), not just
-       * EXIT_SUCCESS/EXIT_FAILURE - normalize to a plain 0/1 shell exit
-       * status here.
-       */
+      /* Convert library errors to a shell exit status. */
 
       return pkg_install(argv[2]) == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
     }
 
-  /* "update" is a package name resolving to whatever version is latest
-   * in the local index - pkg_install() already handles the "already
-   * installed at a different version" transition transparently via
-   * pkg_install_update_installed(), so no separate code path is needed.
-   */
+  /* Installing the latest indexed version also performs an update. */
 
   if (strcmp(cmd, "update") == 0)
     {