feat(lockfile): graduate lockfiles from experimental#7929
Conversation
Summary of ChangesHello @jdx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and maintainability of the lockfile implementation. It addresses critical areas such as error handling for file operations and mutexes, introduces automatic cleanup for unreferenced resources, and improves diagnostic capabilities, paving the way for the lockfile feature to move out of experimental status. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the lockfile implementation for production readiness by improving error handling, adding automatic cleanup, and strengthening test coverage.
Changes:
- Improved mutex handling to recover from poisoned states instead of panicking
- Enhanced error differentiation between missing files (expected) and parse errors (warnings)
- Added automatic cleanup of unreferenced conda packages during lockfile updates
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Err(e) = file::remove_file(path) { | ||
| // Only warn if the file actually exists - ENOENT is fine | ||
| if path.exists() { | ||
| warn!( | ||
| "failed to remove empty lockfile {}: {}", | ||
| display_path(path), | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: checking path.exists() after remove_file fails can give incorrect results if another process deletes the file between the failed remove and the exists check. Instead, check the error kind directly: if e.kind() != std::io::ErrorKind::NotFound.
| // Use unwrap_or_else to recover from poisoned mutex (thread panicked while holding lock) | ||
| let mut cache = ALL_LOCKFILES_CACHE | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); |
There was a problem hiding this comment.
Recovering from a poisoned mutex may lead to inconsistent cache state if a panic occurred while modifying the cache. Consider logging a warning when recovering from a poisoned mutex to aid debugging, similar to: warn!(\"Recovered from poisoned ALL_LOCKFILES_CACHE mutex\").
| // Use unwrap_or_else to recover from poisoned mutex (thread panicked while holding lock) | ||
| let mut cache = SINGLE_LOCKFILE_CACHE | ||
| .lock() | ||
| .unwrap_or_else(|e| e.into_inner()); |
There was a problem hiding this comment.
Recovering from a poisoned mutex may lead to inconsistent cache state if a panic occurred while modifying the cache. Consider logging a warning when recovering from a poisoned mutex to aid debugging, similar to: warn!(\"Recovered from poisoned SINGLE_LOCKFILE_CACHE mutex\").
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements to the lockfile implementation, moving it closer to production readiness. The changes, including mutex poisoning safety, enhanced error logging, and automatic cleanup of unreferenced conda packages, are well-executed and significantly enhance the feature's robustness. I've offered a couple of suggestions to further refine the error handling and improve the efficiency of the new cleanup logic. Overall, this is a strong contribution.
| if let Err(e) = file::remove_file(path) { | ||
| // Only warn if the file actually exists - ENOENT is fine | ||
| if path.exists() { | ||
| warn!( | ||
| "failed to remove empty lockfile {}: {}", | ||
| display_path(path), | ||
| e | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation to check if a warning should be logged for a failed file removal has a potential race condition. It first tries to remove the file, and on failure, it checks if the file exists. If another process removes the file between these two operations, a legitimate error (e.g., permission denied) could be silently ignored.
A more robust approach is to inspect the error itself to see if it's a NotFound error, which is what you're trying to suppress. This avoids the race condition and is more idiomatic. You're already using this pattern in handle_lockfile_read_error.
| if let Err(e) = file::remove_file(path) { | |
| // Only warn if the file actually exists - ENOENT is fine | |
| if path.exists() { | |
| warn!( | |
| "failed to remove empty lockfile {}: {}", | |
| display_path(path), | |
| e | |
| ); | |
| } | |
| } | |
| if let Err(e) = file::remove_file(path) { | |
| // Only warn if the error is not "file not found" | |
| if !e.downcast_ref::<std::io::Error>().is_some_and(|io| io.kind() == std::io::ErrorKind::NotFound) { | |
| warn!( | |
| "failed to remove empty lockfile {}: {}", | |
| display_path(path), | |
| e | |
| ); | |
| } | |
| } |
| // Remove unreferenced packages | ||
| for (platform, packages) in &mut self.conda_packages { | ||
| let referenced_for_platform = referenced.get(platform); | ||
| packages.retain(|basename, _| { | ||
| referenced_for_platform | ||
| .map(|refs| refs.contains(basename)) | ||
| .unwrap_or(false) | ||
| }); | ||
| } | ||
|
|
||
| // Remove empty platform entries | ||
| self.conda_packages | ||
| .retain(|_, packages| !packages.is_empty()); |
There was a problem hiding this comment.
The logic to remove unreferenced conda packages and then remove empty platform entries can be made more efficient and concise by combining them into a single retain operation on self.conda_packages. This avoids iterating over the conda_packages map twice.
| // Remove unreferenced packages | |
| for (platform, packages) in &mut self.conda_packages { | |
| let referenced_for_platform = referenced.get(platform); | |
| packages.retain(|basename, _| { | |
| referenced_for_platform | |
| .map(|refs| refs.contains(basename)) | |
| .unwrap_or(false) | |
| }); | |
| } | |
| // Remove empty platform entries | |
| self.conda_packages | |
| .retain(|_, packages| !packages.is_empty()); | |
| // Remove unreferenced packages and empty platform entries | |
| self.conda_packages.retain(|platform, packages| { | |
| if let Some(referenced_for_platform) = referenced.get(platform) { | |
| packages.retain(|basename, _| referenced_for_platform.contains(basename)); | |
| !packages.is_empty() | |
| } else { | |
| false // remove the whole platform entry if it's not in `referenced` | |
| } | |
| }); |
- Fix mutex poisoning safety with unwrap_or_else - Add error logging for empty lockfile deletion - Add corruption detection in lockfile read errors - Add cleanup of unreferenced conda packages - Fix atomic writes and cache invalidation - Remove experimental requirement from all code paths and docs Lockfiles are now enabled by default and no longer require experimental=true. This provides reproducible tool installations with version pinning and checksum verification. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
3a07dbf to
b99d23e
Compare
|
bugbot run |
Hyperfine Performance
|
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.1.12 x -- echo |
19.5 ± 0.3 | 18.9 | 20.7 | 1.00 |
mise x -- echo |
19.7 ± 0.4 | 19.1 | 23.8 | 1.01 ± 0.02 |
mise env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.1.12 env |
19.2 ± 0.6 | 18.4 | 24.9 | 1.00 |
mise env |
19.4 ± 0.4 | 18.5 | 21.0 | 1.01 ± 0.04 |
mise hook-env
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.1.12 hook-env |
20.0 ± 0.6 | 19.1 | 25.2 | 1.00 |
mise hook-env |
20.1 ± 1.0 | 19.3 | 37.5 | 1.01 ± 0.06 |
mise ls
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|---|---|---|---|---|
mise-2026.1.12 ls |
17.7 ± 0.3 | 17.0 | 18.9 | 1.00 |
mise ls |
17.8 ± 0.3 | 17.0 | 19.1 | 1.01 ± 0.03 |
xtasks/test/perf
| Command | mise-2026.1.12 | mise | Variance |
|---|---|---|---|
| install (cached) | 110ms | 111ms | +0% |
| ls (cached) | 70ms | 69ms | +1% |
| bin-paths (cached) | 73ms | 73ms | +0% |
| task-ls (cached) | 2489ms | 2487ms | +0% |
### 🚀 Features - **(edit)** add interactive config editor (`mise edit`) by @jdx in [#7930](#7930) - **(lockfile)** graduate lockfiles from experimental by @jdx in [#7929](#7929) - **(task)** add support for usage values in task confirm dialog by @roele in [#7924](#7924) - **(task)** improve source freshness checking with edge case handling by @jdx in [#7932](#7932) ### 🐛 Bug Fixes - **(activate)** preserve ordering of paths appended after mise activate by @jdx in [#7919](#7919) - **(install)** sort failed installations for deterministic error output by @jdx in [#7936](#7936) - **(lockfile)** preserve URL and prefer sha256 when merging platform info by @jdx in [#7923](#7923) - **(lockfile)** add atomic writes and cache invalidation by @jdx in [#7927](#7927) - **(templates)** use sha256 for hash filter instead of blake3 by @jdx in [#7925](#7925) - **(upgrade)** respect tracked configs when pruning old versions by @jdx in [#7926](#7926) ### 🚜 Refactor - **(progress)** migrate from indicatif to clx by @jdx in [#7928](#7928) ### 📚 Documentation - improve clarity on uvx and pipx dependencies by @ygormutti in [#7878](#7878) ### ⚡ Performance - **(install)** use Kahn's algorithm for dependency scheduling by @jdx in [#7933](#7933) - use Aho-Corasick for efficient redaction by @jdx in [#7931](#7931) ### 🧪 Testing - remove flaky test_http_version_list test by @jdx in [#7934](#7934) ### Chore - use github backend instead of ubi in mise.lock by @jdx in [#7922](#7922) ### New Contributors - @ygormutti made their first contribution in [#7878](#7878)
Lockfiles were graduated from experimental in v2026.2.0 (#7929), but the `mise lock` command still had `ensure_experimental("lock")` gating it. This removes that check and cleans up MISE_EXPERIMENTAL=1 from all lockfile e2e tests and the docs reference. Closes #8009 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
## Summary
- Remove `settings.ensure_experimental("lock")` check from `mise lock`
command — lockfiles were graduated from experimental in v2026.2.0
(#7929) but this gate was missed
- Update docs in `tips-and-tricks.md` to no longer reference
experimental mode for lockfiles
- Remove unnecessary `MISE_EXPERIMENTAL=1` from all 15 lockfile e2e
tests
Fixes #8009
## Test plan
- [ ] `mise lock` works without `experimental=true` setting
- [ ] Existing lockfile e2e tests pass without `MISE_EXPERIMENTAL=1`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Removes a feature gate and updates docs/tests; behavior change is
limited to allowing `mise lock` to run without experimental settings.
>
> **Overview**
> **Lockfiles are no longer gated behind experimental mode.** The `mise
lock` CLI command now runs without calling
`settings.ensure_experimental("lock")`.
>
> Docs and e2e coverage were updated accordingly: the lockfile tip now
refers to lockfiles being enabled (not experimental mode), and all
lockfile-related e2e tests drop the `MISE_EXPERIMENTAL=1` env var while
continuing to run with `MISE_LOCKFILE=1`.
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
5273aac. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
## Summary This PR makes lockfiles production-ready by removing the experimental requirement and addressing all identified issues for production use: **Production readiness fixes:** - Fix mutex poisoning safety with `unwrap_or_else(|e| e.into_inner())` to recover gracefully from panicked threads - Add error logging for empty lockfile deletion failures - Add corruption detection in lockfile read errors with helpful warning messages - Add cleanup of unreferenced conda packages to prevent lockfile growth **Remove experimental requirement:** - Remove `&& settings.experimental` checks from all lockfile-related code paths - Update documentation to remove experimental badge and notes - Lockfiles are now enabled by default with `lockfile = true` ## Test plan - [ ] Existing lockfile tests pass - [ ] `mise install` works with lockfiles enabled - [ ] `mise lock` generates lockfiles correctly - [ ] Verify lockfile checksums work without experimental flag - [ ] Check documentation renders correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Medium risk because it changes gating behavior so lockfile resolution/checksum generation now runs whenever `lockfile` is enabled, affecting installs across multiple backends and potentially surfacing stricter behavior in existing workflows. > > **Overview** > **Lockfiles are now treated as production-ready**: all lockfile-related code paths no longer require `experimental`, so checksum generation/verification and locked-version resolution run when `lockfile` is enabled. > > **Improves lockfile robustness** by recovering from poisoned lockfile caches, warning on non-`NotFound` lockfile read failures as possible corruption, and logging failures when deleting an empty lockfile. Also adds cleanup of unreferenced shared conda packages when updating lockfiles, and updates docs/settings notes to remove the experimental badge/requirements (including for `locked`). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5b205a8. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
### 🚀 Features - **(edit)** add interactive config editor (`mise edit`) by @jdx in [jdx#7930](jdx#7930) - **(lockfile)** graduate lockfiles from experimental by @jdx in [jdx#7929](jdx#7929) - **(task)** add support for usage values in task confirm dialog by @roele in [jdx#7924](jdx#7924) - **(task)** improve source freshness checking with edge case handling by @jdx in [jdx#7932](jdx#7932) ### 🐛 Bug Fixes - **(activate)** preserve ordering of paths appended after mise activate by @jdx in [jdx#7919](jdx#7919) - **(install)** sort failed installations for deterministic error output by @jdx in [jdx#7936](jdx#7936) - **(lockfile)** preserve URL and prefer sha256 when merging platform info by @jdx in [jdx#7923](jdx#7923) - **(lockfile)** add atomic writes and cache invalidation by @jdx in [jdx#7927](jdx#7927) - **(templates)** use sha256 for hash filter instead of blake3 by @jdx in [jdx#7925](jdx#7925) - **(upgrade)** respect tracked configs when pruning old versions by @jdx in [jdx#7926](jdx#7926) ### 🚜 Refactor - **(progress)** migrate from indicatif to clx by @jdx in [jdx#7928](jdx#7928) ### 📚 Documentation - improve clarity on uvx and pipx dependencies by @ygormutti in [jdx#7878](jdx#7878) ### ⚡ Performance - **(install)** use Kahn's algorithm for dependency scheduling by @jdx in [jdx#7933](jdx#7933) - use Aho-Corasick for efficient redaction by @jdx in [jdx#7931](jdx#7931) ### 🧪 Testing - remove flaky test_http_version_list test by @jdx in [jdx#7934](jdx#7934) ### Chore - use github backend instead of ubi in mise.lock by @jdx in [jdx#7922](jdx#7922) ### New Contributors - @ygormutti made their first contribution in [jdx#7878](jdx#7878)
## Summary
- Remove `settings.ensure_experimental("lock")` check from `mise lock`
command — lockfiles were graduated from experimental in v2026.2.0
(jdx#7929) but this gate was missed
- Update docs in `tips-and-tricks.md` to no longer reference
experimental mode for lockfiles
- Remove unnecessary `MISE_EXPERIMENTAL=1` from all 15 lockfile e2e
tests
Fixes jdx#8009
## Test plan
- [ ] `mise lock` works without `experimental=true` setting
- [ ] Existing lockfile e2e tests pass without `MISE_EXPERIMENTAL=1`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Low Risk**
> Removes a feature gate and updates docs/tests; behavior change is
limited to allowing `mise lock` to run without experimental settings.
>
> **Overview**
> **Lockfiles are no longer gated behind experimental mode.** The `mise
lock` CLI command now runs without calling
`settings.ensure_experimental("lock")`.
>
> Docs and e2e coverage were updated accordingly: the lockfile tip now
refers to lockfiles being enabled (not experimental mode), and all
lockfile-related e2e tests drop the `MISE_EXPERIMENTAL=1` env var while
continuing to run with `MISE_LOCKFILE=1`.
>
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
5273aac. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
… mode (#8500) ## Summary - Adds `prefer_offline` guard for `"latest"` versions in `resolve_version()`, matching the existing guard for fully-qualified versions added in #7976 - Prevents `mise env`, `hook-env`, `activate`, and `exec` from making network calls when resolving `"latest"` versions — avoiding hangs with private npm registries ## Context After lockfiles graduated from experimental in v2026.2.0 (#7929), `mise env --json` with `npm:pkg = "latest"` and no lockfile entry falls through to a network fetch that `prefer_offline` mode was intended to prevent. If the private registry holds the TCP connection open (waiting for credentials), mise hangs indefinitely. PR #7976 fixed this for fully-qualified versions (e.g. `"2.3.2"`) but the `"latest"` path was not covered. This one-liner closes that gap. See #8499 for the full root cause analysis. Fixes #8499 ## Test plan - [ ] `mise env --json` with `npm:pkg = "latest"` and no registry auth no longer makes network calls or hangs - [ ] Existing behavior unchanged when `prefer_offline` is not set (e.g. `mise install` still resolves latest from registry) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Small conditional change to version resolution logic that only affects `prefer_offline` behavior for `latest`; low risk but could change expectations for users relying on remote latest lookup in that mode. > > **Overview** > Prevents network access when resolving `"latest"` in `ToolVersion::resolve_version` while `prefer_offline` is enabled, aligning `latest` behavior with the existing prefer-offline skip for fully-qualified versions. > > This adds a guard so `hook-env`/`activate`/`exec`/`env` won’t call `latest_version_with_opts` unless explicitly doing a latest-version lookup (`opts.latest_versions`), avoiding hangs against registries that keep connections open. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 1ecf269. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
This PR makes lockfiles production-ready by removing the experimental requirement and addressing all identified issues for production use:
Production readiness fixes:
unwrap_or_else(|e| e.into_inner())to recover gracefully from panicked threadsRemove experimental requirement:
&& settings.experimentalchecks from all lockfile-related code pathslockfile = trueTest plan
mise installworks with lockfiles enabledmise lockgenerates lockfiles correctly🤖 Generated with Claude Code
Note
Medium Risk
Medium risk because it changes gating behavior so lockfile resolution/checksum generation now runs whenever
lockfileis enabled, affecting installs across multiple backends and potentially surfacing stricter behavior in existing workflows.Overview
Lockfiles are now treated as production-ready: all lockfile-related code paths no longer require
experimental, so checksum generation/verification and locked-version resolution run whenlockfileis enabled.Improves lockfile robustness by recovering from poisoned lockfile caches, warning on non-
NotFoundlockfile read failures as possible corruption, and logging failures when deleting an empty lockfile. Also adds cleanup of unreferenced shared conda packages when updating lockfiles, and updates docs/settings notes to remove the experimental badge/requirements (including forlocked).Written by Cursor Bugbot for commit 5b205a8. This will update automatically on new commits. Configure here.