fix(lifecycle): dotdir_plan scales linearly, and the e2e run cannot touch a real dotdir (#1202) - #1203
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideFixes two lifecycle defects: a quadratic stray-path deduplication in dotdir_plan that made uninstall appear to hang on large stores, and an unsafe e2e uninstall test that operated on the developer’s real ~/.aelfrice; adds targeted tests and changelog entry for both behaviors. Sequence diagram for aelf_uninstall using dotdir_plan with set-based deduplicationsequenceDiagram
actor User
participant cli_uninstall as cli_uninstall
participant lifecycle_dotdir_plan as lifecycle.dotdir_plan
participant lifecycle_dispose_dotdir as lifecycle.dispose_dotdir
participant filesystem as filesystem
User->>cli_uninstall: uninstall --purge --yes
cli_uninstall->>lifecycle_dotdir_plan: dotdir_plan(AELFRICE_DOTDIR)
lifecycle_dotdir_plan->>filesystem: home.iterdir()
lifecycle_dotdir_plan->>filesystem: logs_dir.iterdir()
lifecycle_dotdir_plan->>lifecycle_dotdir_plan: seen_unrecognised.add(stray)
lifecycle_dotdir_plan->>lifecycle_dotdir_plan: unrecognised.append(stray)
lifecycle_dotdir_plan-->>cli_uninstall: plan
cli_uninstall->>lifecycle_dispose_dotdir: dispose_dotdir(AELFRICE_DOTDIR, plan)
lifecycle_dispose_dotdir->>filesystem: remove install-state
lifecycle_dispose_dotdir->>filesystem: remove data (logs)
lifecycle_dispose_dotdir-->>cli_uninstall: completed
cli_uninstall-->>User: command finishes without apparent hang
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:Setr:2026-07-30T18:48:01Z] |
`s not in unrecognised` rescanned a list that grows as the loop runs, so classifying a directory cost O(n^2) path comparisons: 0.035s at 1k strays, 0.538s at 4k. `~/.aelfrice/logs/` on a long-lived store reaches five figures — 10,370 on the store this was found against — which put `dotdir_plan` past 3.5s on its own. `aelf uninstall` runs it twice, to disclose and again inside `dispose_dotdir`, so the command looked hung before printing anything. Now linear: ~2.0x per doubling, 0.094s at 16k. Classification and ordering are unchanged; the list still carries the order. The scaling test asserts the shape of the curve rather than a wall-clock budget, so it does not encode how fast the machine is — 3.8x before the fix against ~2.0x after, with the bar at 3.0x.
…ne (#1202) The module docstring claimed a hermetic tmp HOME, but the test pinned only AELFRICE_DB. `auto_install.AELFRICE_DOTDIR` is built from the real `Path.home()`, and both uninstall paths read it off the module at call time — one to list, one to delete. So `uninstall --purge --yes` here reached the developer's own ~/.aelfrice/ and would have removed its install-state sentinels and capture logs. CI never saw it: a runner's ~/.aelfrice/ is empty, so there was nothing to enumerate and nothing to lose. Locally the 5s timeout fired during the enumeration walk and stopped execution before the removal, so the quadratic bug is what kept the directory intact — fixing that alone would have made this reachable. Autouse, because --keep-db disposes install state too. A separate test asserts the fixture is in effect, and the lifecycle test now writes a stamp into the isolated dotdir and asserts --purge takes it, so the disposition is proven to run rather than no-op on an empty directory.
f10d1ba to
96f3fd6
Compare
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
Approve — and this is my regression. I wrote the quadratic loop, in the #1190 review round, addressing a Sourcery thread about stray reporting. Both findings here are correct, the second one is worse than the first, and the analysis of why they hid each other is right. I've rebased onto current Corroborated independently
On "nothing was lost"Agreed, and here is a second line of evidence for it. The part worth keeping in front of usThe two defects are not independent, and the write-up is right that the second is why the first survived. But there is a third link: the quadratic loop is the only reason the destructive one didn't fire. A faster implementation of my own change would have deleted the real dotdir on the first local test run. The performance bug was load-bearing safety, by accident. That inverts the usual severity reading. If someone had "just" fixed the slowness — which is what the symptom looks like from the outside, a hung What I'd take from it for the review processMy #1190 change was mutation-tested (I reverted the stray-reporting fix and confirmed the new test failed), CI was green, and the full local suite passed. None of that caught either defect, because:
The concrete lesson is narrow and worth writing down: "fails on main too" only rules out your branch, not your code — and when the failing frame sits in a file you touched today, that is the point to read the traceback rather than the conclusion. Smaller notes
Labelling once CI settles. |
robotrocketscience
added
the
ready-to-merge
|
[release:review:Setr:2026-07-30T18:52:08Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 96f3fd6 → |
1 participant
Closes #1202. Two defects that shipped with #1186 (PR #1190, merged earlier today). The second is why the first got through review and CI.
1.
dotdir_planwas quadraticlifecycle.py:919deduped against a list:s not in unrecognisedrescans a collection that grows as the loop runs — O(n²)PosixPath.__eq__calls over the directory.Doubling the input quadrupled the old time and roughly doubles the new one.
This is user-facing, not just a test problem.
~/.aelfrice/logs/on a long-lived store reaches five figures — 10,370 on the store this was found against — which putsdotdir_planpast 3.5s on its own, andaelf uninstallruns it twice (once to disclose, once insidedispose_dotdir). The command sat silent before printing anything.Classification and ordering are unchanged; the list still carries the order, the set only answers membership.
2. The lifecycle e2e test could delete the developer's real
~/.aelfrice/tests/regression/test_install_uninstall_e2e.pyopens with "full lifecycle in a hermetic tmp HOME" but pinned onlyAELFRICE_DB. The dotdir is not derived from anything the test controls:Both uninstall paths read it off the module at call time —
cli.py:4996to list,cli.py:5035to delete — souninstall --purge --yesin that test addressed the real home directory and would have removed its install-state sentinels, the LLM consent sentinel, and (under--purge, sinceinclude_datais True) the capture logs.Nothing was actually lost. The 5s
pytest-timeoutfired inside the quadratic walk and stopped execution beforedispose_dotdir— verified, the 10,370 logs and the manifest stamp are intact. That is worth stating plainly: defect 1 is what protected the directory from defect 2, so fixing the performance bug alone would have made the destructive path reachable. That is why both land together.CI never saw either: a runner's
~/.aelfrice/is empty, so there was nothing to enumerate (defect 1 stays fast) and nothing to delete (defect 2 is inert).I audited the rest of the suite —
git grep -l '"uninstall"' testsreturns four files, but the other three only mention the string in a command-name list. This module is the only one that invokes a disposition, so the blast radius is confined to it.Verification
lifecycle.pyto the list-based dedup fails it withdotdir_plan scaled 3.8x for 2x the input (0.0367s -> 0.1386s); the fixed version passes at ~2.0x.minof three runs, because load can only make a sample slower. Bar at 3.0x, between the measured 3.8x and 2.0x.--keep-dbdisposes install state too). One test asserts the pin is in effect, so deleting the fixture fails loudly rather than silently re-arming a destructive run; the lifecycle test writes a stamp into the isolated dotdir and asserts--purgetakes it, proving the disposition runs rather than no-ops on an empty directory.test_full_lifecycle_setup_onboard_search_uninstallwas failing on unmodifiedgithub/mainbefore this and now passes in ~1s.Summary by Sourcery
Fix dotdir lifecycle handling so uninstall runs efficiently on large stores and cannot affect a developer’s real dotdir during tests.
Bug Fixes:
Documentation:
Tests: