Ask the database once which directories a delete shares

[laffer1]

mport delete looks hung on packages with many directories. It isn't deadlocked — it is burning CPU in a quadratic loop. Deleting py311-botocore on a normal desktop install took 10m40s.

Cause

is_safe_to_delete_dir() ran one query per directory asset:

SELECT count(*) FROM assets WHERE pkg!=? AND type IN (...) AND data=?

assets carries a single index, assets_pkg (pkg). The pkg!=? inequality cannot use it and nothing indexes data, so each call is a full scan:

sqlite> EXPLAIN QUERY PLAN SELECT count(*) from assets where pkg!='py311-botocore' and type in (...) and data='...';
`--SCAN assets

944,774 rows / 170 MB on the test machine, ~0.8 s per call. py311-botocore has 864 directory assets → ~11 minutes, which matches the 10m40s measured. gdb on the live process confirmed it, every sample landing in sqlite3_step under is_safe_to_delete_dir, with procstat -f showing only master.db open (no package files — it was not hashing).

Projected for other installed packages: rust 1886 dirs ≈ 25 min, boost-libs 1339 ≈ 18 min, llvm22 701 ≈ 9 min.

Killing the apparently-hung process makes it worse: file unlinking happens before the transaction that unregisters the package, so an interrupted delete leaves the package registered with its files gone.

Fix

Ask once instead of N times. The loop only needs the set of this package's directories that another package also owns:

SELECT DISTINCT data FROM assets
 WHERE pkg!=?1 AND type IN (...)
   AND data IN (SELECT data FROM assets WHERE pkg=?1 AND type IN (...))

The inner side uses assets_pkg, so one scan covers the whole delete rather than one per directory. is_safe_to_delete_dir() keeps its other guards (root, prefix, system mtree dirs) and ends in a bsearch() over the sorted result, preserving the old exact-match data=? semantics.

No new index and no schema migration. An index on assets(data) would also work but costs +71 MB on a 170 MB database; the partial-index variant is +1.4 MB but SQLite only matches it when the query's IN list is written in the identical order, which is a trap for the next person to edit the query.

Memory is bounded by the package's own directory count, not the size of assets: 4 paths for rust, at most ~100 KiB even if every directory it owned turned out to be shared.

Behavior change

A failure building the set now aborts the delete before anything is removed. Previously a failed query in is_safe_to_delete_dir() returned false, silently leaving directories behind partway through a delete that otherwise continued.

Verification

package	dir assets	before	after
py312-botocore	920	10m40s*	1.6 s
py312-jmespath	7	—	0.77 s

* measured on py311-botocore (864 dir assets) before the change.

Correctness checked both directions on py312-jmespath, 3 of whose 7 directories are shared with other packages:

• removed: site-packages/jmespath, .../__pycache__, .../jmespath-0.10.0-py3.12.egg-info, share/licenses/py312-jmespath-0.10.0_1
• kept: share/licenses, lib/python3.12, lib/python3.12/site-packages
• no spurious Directory in use by another package? warnings

Build is clean at -Werror -Wall -W. precommit_c_sanity.sh passes (cppcheck clean, clang-format no changes).

run_splint_on_staged.sh could not run: it aborts preprocessing on mport_lua.h because lua's lauxlib.h/luaconf.h are not on its search path. Confirmed pre-existing by running splint against the unmodified main version of the same file — identical failure. So the splint pass did not actually cover this commit.

Summary by Sourcery

Speed up package deletion by batching shared-directory ownership checks before removing package files.

Bug Fixes:

• Prevent package deletion from spending minutes performing one full database scan per directory asset.
• Abort deletion before removing files when the shared-directory lookup fails, instead of continuing with a partial delete.

Enhancements:

• Determine shared directories with one database query per package and use an in-memory set for fast directory ownership checks while preserving existing deletion safeguards.

[sourcery-ai]

Reviewer's Guide

The delete path now queries database ownership once, caches shared directory paths in a sorted in-memory set, and performs fast exact lookups while retaining existing safety checks; query or allocation failures abort before removal.

Sequence diagram for cached shared-directory checks during package deletion

sequenceDiagram
    participant Delete as mport_delete_primative
    participant DB as SQLite assets
    participant Set as shared_dir_set
    participant FS as Filesystem

    Delete->>DB: build_shared_dir_set()
    DB-->>Delete: shared directory data rows
    Delete->>Set: qsort(paths)
    loop each directory asset
        Delete->>Set: bsearch(asset_path)
        alt directory is not shared
            Delete->>FS: mport_rmdir(file)
        else directory is shared
            Delete-->>Delete: retain directory
        end
    end
    Delete->>Set: free_shared_dir_set()

Loading

File-Level Changes

Change	Details	Files
Replaced per-directory full-table ownership checks with one precomputed shared-directory set per delete.	Runs a single DISTINCT query matching directories owned by both the target and another package. Collects and sorts returned paths, then uses exact-match bsearch lookups during deletion. Bounds memory to the target package’s directory count and avoids schema/index changes.	libmport/delete_primative.c
Integrated shared-directory set lifecycle and failure handling into the delete workflow.	Builds the set before filesystem removal begins and frees it on normal and error exits. Aborts the delete if set construction fails, changing prior partial-delete behavior. Preserves existing root, prefix, system-directory, and directory-type safety guards.	libmport/delete_primative.c

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[github-actions]

Claude finished @laffer1's task in 2m 37s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (read diff, changed file, CLAUDE.md/AGENTS.md)
• Understand request (code review of the delete-shared-dir-lookup PR)
• Review correctness, memory safety, error propagation, SQL injection, path traversal, consistency
• Post review findings

Review

This is a well-diagnosed fix for a genuine O(n²) hotspot (per-directory full-table scan → one query per delete). I traced the full allocation/error lifecycle and the SQL construction; I did not find correctness or memory-safety bugs.

Correctness

• build_shared_dir_set() (libmport/delete_primative.c:532) preserves the old exact-match semantics: it computes the set of pack's directory-type assets (ASSET_DIR, ASSET_DIRRM, ASSET_DIRRMTRY, ASSET_DIR_OWNER_MODE, ASSET_AUTODIR) that are also owned by some other package with any of those same five types — identical to what the old per-row count(*) ... pkg!=? and data=? query tested, just computed once instead of N times.
• is_safe_to_delete_dir() (:616) now does a bsearch() on asset_path (the raw data column value), which is the same field the old query filtered on — the comparison basis didn't shift to the resolved file path.
• Query at :532 uses %Q for pack->name (sqlite3 printf escaping/quoting) and %d only for internal enum constants — no SQL injection, consistent with existing patterns in this file (e.g. line ~155, ~195).

Memory safety

• Growth uses reallocarray() (already used elsewhere, query.c:591) with a local grown temp, so a failed realloc doesn't clobber/lose the original paths pointer — no leak on OOM.
• All three failure paths inside the loop (sqlite3_step error, reallocarray failure, strdup failure) funnel through the single error: label at :532's function body, which finalizes the statement and frees exactly the count strings that were successfully allocated, then the array itself — no double-free, no leak, no use of an uninitialized slot.
• set->paths/set->count are zeroed before the first fallible operation, so a prepare failure (RETURN_CURRENT_ERROR before goto error is reachable) leaves the struct in a state where free_shared_dir_set() would be a safe no-op even if called — though the caller correctly skips calling it in that case since nothing was allocated yet.
• Call site in mport_delete_primative(): build_shared_dir_set() failure at :208 correctly finalizes the outer stmt (prepared at line 194) before returning — no leak of that statement. The only other early-return inside the asset loop (:225, on sqlite3_step error) also frees shared_dirs. I checked every break/case in the loop body (lines 240–430); none of the other paths return early, so sqlite3_finalize(stmt) + free_shared_dir_set() at :433-434 cover the remaining exits.
• cmp_dir_path (:518) and its use in both qsort (:588) and bsearch (:647) are type-consistent (char ** elements throughout).

Error propagation / behavior change

• The PR body flags that a failed set-build now aborts the delete before any files are removed, versus the old code where a failed per-directory query silently treated that directory as "not safe to delete" and kept going. That's an accurate description of the diff and, given this file's existing "roll back a failed delete" direction (see recent commits 883cbed, 11aa048), consistent with the codebase's trend toward failing loudly rather than leaving partial state.

Splint annotations

• New declarations use /*@notnull@*/, /*@out@*/, and /*@only@*/ appropriately for the DB-facing allocator and out-param (:80-89), matching AGENTS.md's guidance for security-sensitive code.

Minor, non-blocking observations

• struct shared_dir_set is file-local (not in mport_private.h), appropriately scoped since only this translation unit uses it.
• No new index/schema change, as described — the tradeoff analysis in the PR body (partial index size vs. an assets(data) index) is reasonable and out of scope for this fix anyway.

Validation

• I was not able to run make/compile the tree or execute precommit_c_sanity.sh in this sandboxed environment — Bash invocations here (including make, gcc --version, git fetch) required approval that wasn't available non-interactively. This is a tooling limitation of this review session, not a finding about the PR. The PR description states the build is clean at -Werror -Wall -W and precommit_c_sanity.sh passes, with run_splint_on_staged.sh blocked by a pre-existing, unrelated lauxlib.h search-path issue (confirmed against unmodified main too) — I have no way to independently re-verify those claims here, but the reasoning given is credible and the pre-existing-failure check (running splint against main) is good practice.

Conclusion: No correctness, memory-safety, SQL-injection, or error-propagation issues found. The fix is sound and matches its stated goal (single index-backed query instead of one scan per directory), with memory usage properly bounded and cleaned up on all paths I traced.
· Branch: delete-shared-dir-lookup

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook