-
-
Notifications
You must be signed in to change notification settings - Fork 769
perf(hook-env): add fast-path to skip initialization when nothing changed #7073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
b1a9171
8ddd8df
ecd9921
dc82d49
484ec54
7834f94
dd1008e
f7738fc
355b9aa
bfe3e1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Test that hook-env fast-path correctly detects when nothing has changed | ||
| # and skips expensive initialization | ||
|
|
||
| # Activate mise to set up the session | ||
| eval "$(mise activate bash)" | ||
|
|
||
| # Run hook-env once to establish a session | ||
| mise hook-env -s bash | ||
|
|
||
| # Verify fast-path works when nothing has changed | ||
| # The fast-path should produce no output (empty response) | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| if [[ -z $output || $output == *"early-exit"* ]]; then | ||
| ok "fast-path works when nothing changed" | ||
| else | ||
| fail "fast-path did not work: got output '$output'" | ||
| fi | ||
|
|
||
| # Test 1: Creating a new config file should NOT trigger fast-path | ||
| echo '[tools]' >mise.toml | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| # The output should contain environment variable exports (not be empty) | ||
| # because the new config file should be detected | ||
| rm mise.toml | ||
| ok "new config file detection test completed" | ||
|
|
||
| # Test 2: Test that changing directories prevents fast-path | ||
| # First reset the session | ||
| eval "$(mise activate bash)" | ||
| mise hook-env -s bash | ||
|
|
||
| # Create a subdirectory and cd into it | ||
| mkdir -p subdir | ||
| pushd subdir >/dev/null | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| popd >/dev/null | ||
| rmdir subdir | ||
| ok "directory change detection test completed" | ||
|
|
||
| # Test 3: Creating config in parent's .config/mise should be detected | ||
| mkdir -p ../.config/mise | ||
| eval "$(mise activate bash)" | ||
| mise hook-env -s bash | ||
| # Wait a moment to ensure mtime difference | ||
| sleep 0.1 | ||
| echo '[tools]' >../.config/mise/config.toml | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| rm -rf ../.config | ||
| ok "parent .config/mise config file detection test completed" | ||
|
|
||
| # Test 4: Fast-path should work again after session is re-established | ||
| eval "$(mise activate bash)" | ||
| mise hook-env -s bash | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| if [[ -z $output || $output == *"early-exit"* ]]; then | ||
| ok "fast-path works after session re-established" | ||
| else | ||
| fail "fast-path did not work after re-establishment: got output '$output'" | ||
| fi |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ use crate::task::TaskOutput; | |||||||||||||||||||||||||||||
| use crate::ui::{self, ctrlc}; | ||||||||||||||||||||||||||||||
| use crate::{Result, backend}; | ||||||||||||||||||||||||||||||
| use crate::{cli::args::ToolArg, path::PathExt}; | ||||||||||||||||||||||||||||||
| use crate::{logger, migrate, shims}; | ||||||||||||||||||||||||||||||
| use crate::{hook_env as hook_env_module, logger, migrate, shims}; | ||||||||||||||||||||||||||||||
| use clap::{ArgAction, CommandFactory, Parser, Subcommand}; | ||||||||||||||||||||||||||||||
| use eyre::bail; | ||||||||||||||||||||||||||||||
| use std::path::PathBuf; | ||||||||||||||||||||||||||||||
|
|
@@ -433,6 +433,11 @@ impl Cli { | |||||||||||||||||||||||||||||
| if *crate::env::MISE_TOOL_STUB && args.len() >= 2 { | ||||||||||||||||||||||||||||||
| tool_stub::short_circuit_stub(&args[2..]).await?; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| // Fast-path for hook-env: exit early if nothing has changed | ||||||||||||||||||||||||||||||
| // This avoids expensive backend::load_tools() and config loading | ||||||||||||||||||||||||||||||
| if hook_env_module::should_exit_early_fast() { | ||||||||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| measure!("logger", { logger::init() }); | ||||||||||||||||||||||||||||||
| check_working_directory(); | ||||||||||||||||||||||||||||||
|
Comment on lines
+436
to
442
|
||||||||||||||||||||||||||||||
| // Fast-path for hook-env: exit early if nothing has changed | |
| // This avoids expensive backend::load_tools() and config loading | |
| if hook_env_module::should_exit_early_fast() { | |
| return Ok(()); | |
| } | |
| measure!("logger", { logger::init() }); | |
| check_working_directory(); | |
| check_working_directory(); | |
| // Fast-path for hook-env: exit early if nothing has changed | |
| // This avoids expensive backend::load_tools() and config loading | |
| if hook_env_module::should_exit_early_fast() { | |
| return Ok(()); | |
| } | |
| measure!("logger", { logger::init() }); |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,7 +19,7 @@ use crate::env::PATH_KEY; | |||||||||||||
| use crate::env_diff::{EnvDiffOperation, EnvDiffPatches, EnvMap}; | ||||||||||||||
| use crate::hash::hash_to_str; | ||||||||||||||
| use crate::shell::Shell; | ||||||||||||||
| use crate::{dirs, env, hooks, watch_files}; | ||||||||||||||
| use crate::{dirs, env, file, hooks, watch_files}; | ||||||||||||||
|
|
||||||||||||||
| pub static PREV_SESSION: Lazy<HookEnvSession> = Lazy::new(|| { | ||||||||||||||
| env::var("__MISE_SESSION") | ||||||||||||||
|
|
@@ -59,28 +59,120 @@ impl From<PathBuf> for WatchFilePattern { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// this function will early-exit the application if hook-env is being | ||||||||||||||
| /// called and it does not need to be | ||||||||||||||
| pub fn should_exit_early( | ||||||||||||||
| watch_files: impl IntoIterator<Item = WatchFilePattern>, | ||||||||||||||
| reason: Option<HookReason>, | ||||||||||||||
| ) -> bool { | ||||||||||||||
| /// Fast-path early exit check that can be called BEFORE loading config/tools. | ||||||||||||||
| /// This checks basic conditions using only the previous session data. | ||||||||||||||
| /// Returns true if we can definitely skip hook-env, false if we need to continue. | ||||||||||||||
| pub fn should_exit_early_fast() -> bool { | ||||||||||||||
| let args = env::ARGS.read().unwrap(); | ||||||||||||||
| if args.len() < 2 || args[1] != "hook-env" { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
| // Can't exit early if no previous session | ||||||||||||||
| if PREV_SESSION.loaded_configs.is_empty() { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
| // Can't exit early if --force flag is present | ||||||||||||||
| if args.iter().any(|a| a == "--force" || a == "-f") { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
| // Check if running from precmd for the first time | ||||||||||||||
| if args | ||||||||||||||
| .iter() | ||||||||||||||
| .any(|a| a == "--reason=precmd" || a.starts_with("--reason=precmd")) | ||||||||||||||
| && !*env::__MISE_ZSH_PRECMD_RUN | ||||||||||||||
| { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Fast-path fails to detect space-separated reason argumentThe check for |
||||||||||||||
| // Can't exit early if directory changed | ||||||||||||||
| if dir_change().is_some() { | ||||||||||||||
| return false; | ||||||||||||||
| } | ||||||||||||||
| // Can't exit early if MISE_ env vars changed | ||||||||||||||
|
||||||||||||||
| // Can't exit early if MISE_ env vars changed | |
| // Can't exit early if MISE_ env vars changed | |
| // Only environment variables with the `MISE_` prefix are hashed and compared by | |
| // `have_mise_env_vars_been_modified()`. Non-`MISE_` variables that might affect | |
| // resolution are intentionally excluded from this check. This assumes that only | |
| // `MISE_` variables influence the fast path's correctness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Fast-path continues when modification time unavailable
The fast-path check silently continues when metadata.modified() fails, potentially skipping necessary reloads. If modification time retrieval fails (due to filesystem limitations, permissions, or unsupported platforms), the code assumes no changes occurred and continues with the fast-path. This could cause stale environment state when config files or directories were actually modified but their mtimes couldn't be read. The safe behavior would be to bypass the fast-path when modification times are unavailable.
Additional Locations (2)
Copilot
AI
Nov 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If metadata() fails for reasons other than non-existence (e.g., permissions), this logic continues without invalidating the fast path, potentially skipping necessary re-initialization. To be safe, any failure to read metadata should cause a full run, not an early exit.
| } else if !config_path.exists() { | |
| } else { |
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Fast-path misses deleted data directory
The fast-path only checks dirs::DATA modification time when it exists, but skips the check entirely if it doesn't exist. If dirs::DATA existed in the previous session but is now deleted, the fast-path won't detect this change and may incorrectly exit early, leaving stale tool paths in the environment. The check should return false when dirs::DATA is missing to trigger a full environment update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Importing
hook_envashook_env_moduleis unconventional and can be confusing. Prefer a directuse crate::hook_env;and callhook_env::should_exit_early_fast()for clarity.