-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(env): improve hook-env watch_files tracking and early-exits #8716
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
c7c5e57
fix(env): track env plugin watch_files in session and env cache
rpendleton 7429dea
fix(env): prevent stale PREV_SESSION watch_files from leaking into ne…
rpendleton 10283f8
test(env): add e2e test for plugin watch_files
rpendleton e1fc77c
fix(env): correct misleading comment about env_results_cached
rpendleton 208d46a
fix(env): rename tool_watch_files to env_watch_files for clarity
rpendleton f263f5a
test(env): speed up test_env_plugin_watch_files e2e test
rpendleton 7db5aa3
fix(env): prevent missing mise.lock from destabilizing hook-env
rpendleton 06840da
fix(env): align hook-env session timestamp with fast path
rpendleton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Test that after a full hook-env rerun triggered by a config-search path change, | ||
| # the next prompt can take the fast-path instead of repeatedly falling back | ||
| # because ancestor directory mtimes remain newer than the session timestamp. | ||
|
|
||
| export MISE_ENV_CACHE=0 | ||
| export MISE_TRACE=1 | ||
|
|
||
| cat >mise.toml <<'EOF' | ||
| [env] | ||
| FOO = "bar" | ||
| EOF | ||
|
|
||
| eval "$(mise activate bash 2>/dev/null)" | ||
|
|
||
| # Creating a config file in a config-search directory should force a full hook-env run. | ||
| sleep 1 | ||
| mkdir -p .config/mise | ||
| cat >.config/mise/config.toml <<'EOF' | ||
| [env] | ||
| BAR = "baz" | ||
| EOF | ||
| output=$(mise hook-env -s bash 2>/dev/null) | ||
| if [[ $output == *"__MISE_SESSION"* ]]; then | ||
| ok "config-search path change bypasses fast-path once" | ||
| else | ||
| fail "config-search path change should bypass fast-path but got: '$output'" | ||
| fi | ||
| eval "$output" | ||
|
|
||
| # The rerun should update the session timestamp so the next prompt can fast-path. | ||
| # Fast-path exits before logger init, so both stdout and stderr should be empty. | ||
| output=$(mise hook-env -s bash 2>&1) | ||
| if [[ -z $output ]]; then | ||
| ok "fast-path works after config-search rerun" | ||
| else | ||
| fail "expected fast-path (no output) after config-search rerun but got: '$output'" | ||
| fi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Test that optional mise.lock files do not prevent hook-env from stabilizing, | ||
| # while lockfile creation/removal still invalidates the session. | ||
|
|
||
| export MISE_ENV_CACHE=1 | ||
| export __MISE_ENV_CACHE_KEY="dGVzdGtleXRlc3RrZXl0ZXN0a2V5dGVzdGtleXRlc3Q=" | ||
|
|
||
| cat >mise.toml <<'EOF' | ||
| [env] | ||
| FOO = "bar" | ||
| EOF | ||
|
|
||
| eval "$(mise activate bash)" | ||
|
|
||
| # With no lockfile present, hook-env should be stable. | ||
| output=$(mise hook-env -s bash) | ||
| if [[ -z $output ]]; then | ||
| ok "no lockfile does not destabilize hook-env" | ||
| else | ||
| fail "no lockfile should not destabilize hook-env but got: '$output'" | ||
| fi | ||
|
|
||
| # Creating a lockfile should invalidate once. | ||
| sleep 1 | ||
| touch mise.lock | ||
| output=$(mise hook-env -s bash) | ||
| if [[ $output == *"__MISE_SESSION"* ]]; then | ||
| ok "creating mise.lock invalidates session" | ||
| else | ||
| fail "creating mise.lock should invalidate session but got: '$output'" | ||
| fi | ||
| eval "$output" | ||
|
|
||
| # Should stabilize after picking up the new lockfile. | ||
| output=$(mise hook-env -s bash) | ||
| if [[ -z $output ]]; then | ||
| ok "hook-env stabilizes after mise.lock creation" | ||
| else | ||
| fail "hook-env should stabilize after mise.lock creation but got: '$output'" | ||
| fi | ||
|
|
||
| # Removing the lockfile should also invalidate once. | ||
| sleep 1 | ||
| rm mise.lock | ||
| output=$(mise hook-env -s bash) | ||
| if [[ $output == *"__MISE_SESSION"* ]]; then | ||
| ok "removing mise.lock invalidates session" | ||
| else | ||
| fail "removing mise.lock should invalidate session but got: '$output'" | ||
| fi | ||
| eval "$output" | ||
|
|
||
| # And then stabilize again. | ||
| output=$(mise hook-env -s bash) | ||
| if [[ -z $output ]]; then | ||
| ok "hook-env stabilizes after mise.lock removal" | ||
| else | ||
| fail "hook-env should stabilize after mise.lock removal but got: '$output'" | ||
| fi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # Test that env plugin watch_files are tracked in the session and env cache, | ||
| # so that modifying a watched file triggers hook-env to re-evaluate. | ||
| # | ||
| # See: https://github.com/jdx/mise/discussions/8603 | ||
| # | ||
| # There are four meaningfully different code paths depending on: | ||
| # - tools=false: plugin runs in config.load_env() via NonToolsOnly; | ||
| # watch_files flow through config.watch_files() into the slow-path check. | ||
| # - tools=true: plugin runs in toolset.load_post_env() via ToolsOnly; | ||
| # watch_files come back as env_watch_files from env_with_path_and_split() | ||
| # and the slow-path relies on PREV_SESSION.watch_files to detect changes. | ||
| # - cache=off: watch_files computed fresh each time. | ||
| # - cache=on: watch_files stored in CachedEnv; cache must invalidate on change. | ||
| # | ||
| # We test all four combinations to ensure full coverage. | ||
|
rpendleton marked this conversation as resolved.
|
||
|
|
||
| export __MISE_ENV_CACHE_KEY="dGVzdGtleXRlc3RrZXl0ZXN0a2V5dGVzdGtleXRlc3Q=" | ||
|
|
||
| setup_plugin() { | ||
| local plugin_name=$1 | ||
| local data_file=$2 | ||
| local env_var=$3 | ||
|
|
||
| local plugin_dir="$MISE_DATA_DIR/plugins/$plugin_name" | ||
| mkdir -p "$plugin_dir/hooks" | ||
|
|
||
| cat >"$plugin_dir/metadata.lua" <<-EOFMETA | ||
| PLUGIN = {} | ||
| PLUGIN.name = "$plugin_name" | ||
| PLUGIN.version = "1.0.0" | ||
| PLUGIN.homepage = "https://example.com" | ||
| PLUGIN.license = "MIT" | ||
| PLUGIN.description = "Test plugin for watch_files tracking" | ||
| PLUGIN.minRuntimeVersion = "0.3.0" | ||
| EOFMETA | ||
|
|
||
| cat >"$plugin_dir/hooks/mise_env.lua" <<-EOFHOOK | ||
| function PLUGIN:MiseEnv(ctx) | ||
| local f = io.open("$data_file", "r") | ||
| local value = f:read("*all"):gsub("%s+$", "") | ||
| f:close() | ||
| return { | ||
| env = {{key = "$env_var", value = value}}, | ||
| cacheable = true, | ||
| watch_files = {"$data_file"} | ||
| } | ||
| end | ||
| EOFHOOK | ||
| } | ||
|
|
||
| # Assert that hook-env produces no output (fast-path). | ||
| assert_hook_env_is_stable() { | ||
| local msg=$1 | ||
| output=$(mise hook-env -s bash) | ||
| if [[ -z $output ]]; then | ||
| ok "$msg" | ||
| else | ||
| fail "$msg but got: '$output'" | ||
| fi | ||
| } | ||
|
|
||
| # Runs a test scenario with both plugins active: activate, verify initial | ||
| # values, modify one watched file at a time, verify hook-env detects the | ||
| # change and updates the value. | ||
| run_watch_files_test() { | ||
| local label=$1 | ||
|
|
||
| unset TEST_WATCH_NONTOOL TEST_WATCH_TOOL | ||
| unset __MISE_SESSION __MISE_DIFF | ||
| rm -rf "$MISE_STATE_DIR/env-cache" "$MISE_CACHE_DIR" | ||
|
|
||
| # Reset data files | ||
| echo "initial_nontool" >"$DATA_FILE_A" | ||
| echo "initial_tool" >"$DATA_FILE_B" | ||
|
|
||
| # Activate runs hook-env and establishes the initial session. | ||
| eval "$(mise activate bash)" | ||
|
|
||
| # Verify initial values | ||
| if [[ $TEST_WATCH_NONTOOL == "initial_nontool" ]]; then | ||
| ok "$label: nontool initial value set" | ||
| else | ||
| fail "$label: expected TEST_WATCH_NONTOOL=initial_nontool, got '$TEST_WATCH_NONTOOL'" | ||
| fi | ||
| if [[ $TEST_WATCH_TOOL == "initial_tool" ]]; then | ||
| ok "$label: tool initial value set" | ||
| else | ||
| fail "$label: expected TEST_WATCH_TOOL=initial_tool, got '$TEST_WATCH_TOOL'" | ||
| fi | ||
|
|
||
| # Fast-path should work (nothing changed) | ||
| assert_hook_env_is_stable "$label: fast-path works when nothing changed" | ||
|
|
||
| # Modify the nontool watched file (tools=false plugin) | ||
| sleep 1 | ||
| echo "updated_nontool" >"$DATA_FILE_A" | ||
|
|
||
| # hook-env should detect the change | ||
| output=$(mise hook-env -s bash) | ||
| if [[ $output == *"__MISE_SESSION"* ]]; then | ||
| ok "$label: nontool watch_files change bypasses fast-path" | ||
| else | ||
| fail "$label: nontool watch_files change should bypass fast-path but got: '$output'" | ||
| fi | ||
|
|
||
| # Eval and verify updated value | ||
| eval "$output" | ||
| if [[ $TEST_WATCH_NONTOOL == "updated_nontool" ]]; then | ||
| ok "$label: nontool updated value picked up" | ||
| else | ||
| fail "$label: expected TEST_WATCH_NONTOOL=updated_nontool, got '$TEST_WATCH_NONTOOL'" | ||
| fi | ||
|
|
||
| # Fast-path should stabilise | ||
| assert_hook_env_is_stable "$label: fast-path works after nontool update" | ||
|
|
||
| # Modify the tool watched file (tools=true plugin) | ||
| sleep 1 | ||
| echo "updated_tool" >"$DATA_FILE_B" | ||
|
|
||
| # hook-env should detect the change | ||
| output=$(mise hook-env -s bash) | ||
| if [[ $output == *"__MISE_SESSION"* ]]; then | ||
| ok "$label: tool watch_files change bypasses fast-path" | ||
| else | ||
| fail "$label: tool watch_files change should bypass fast-path but got: '$output'" | ||
| fi | ||
|
|
||
| # Eval and verify updated value | ||
| eval "$output" | ||
| if [[ $TEST_WATCH_TOOL == "updated_tool" ]]; then | ||
| ok "$label: tool updated value picked up" | ||
| else | ||
| fail "$label: expected TEST_WATCH_TOOL=updated_tool, got '$TEST_WATCH_TOOL'" | ||
| fi | ||
|
|
||
| # Fast-path should stabilise | ||
| assert_hook_env_is_stable "$label: fast-path works after tool update" | ||
| } | ||
|
|
||
| # --- Setup plugins and data files --- | ||
|
|
||
| DATA_FILE_A="$MISE_DATA_DIR/watch_test_data_a" | ||
| DATA_FILE_B="$MISE_DATA_DIR/watch_test_data_b" | ||
| setup_plugin "test-watch-nontool" "$DATA_FILE_A" "TEST_WATCH_NONTOOL" | ||
| setup_plugin "test-watch-tool" "$DATA_FILE_B" "TEST_WATCH_TOOL" | ||
|
|
||
| # --- Shared config: both plugin types active --- | ||
|
|
||
| cat >"$MISE_CONFIG_DIR/config.toml" <<'EOF' | ||
| [env] | ||
| _.test-watch-nontool = { tools = false } | ||
| _.test-watch-tool = { tools = true } | ||
| EOF | ||
|
|
||
| # --- Test 1: cache=off --- | ||
|
|
||
| export MISE_ENV_CACHE=0 | ||
| run_watch_files_test "cache=off" | ||
|
|
||
| # --- Test 2: cache=on --- | ||
|
|
||
| export MISE_ENV_CACHE=1 | ||
| run_watch_files_test "cache=on" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
why is this marked as slow?
Uh oh!
There was an error while loading. Please reload this page.
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.
That's partially related to #8716 (comment), though even when considering the sleeps, I'm not sure why it's taking long enough to pass the 20 second warning. I'll take a look.
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.
ideally we don't need to mark it as slow so the test will actually run on this PR
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.
I think I can improve this a bit.
With debug builds,
mise activatetakes ~2.5s andmise hook-envtakes ~1.3s. We also have the one second sleeps for file modification detection. Since we're testing four scenarios, (2.5 + 1.3 + 1.0) * 4 = 19.2s, and that's before we consider anything else (like macOS Gatekeeper randomly verifying the executable, since a build is performed right before the test).If I configure both
tools=trueandtools=falseplugins in the same config and test them simultaneously in each scenario, I can avoid running activate as many times and likely get it under the 20s. I'm also not sure if I even need to run activate more than once.I'll work on that now.
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.
ah hmm, I didn't realize things were so slow. I think maybe what we could do is loosen the rules on what is considered slow to more like 1 minute. It's really to avoid things that take multiple minutes like compiling ruby.
I think it would also be fine to just have a larger test that just activates once. We do that often.
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.
I'm having some troubles with my test being a bit flaky, but I think it may be interacting with a pre-existing
mise.lockissue that’s reproducible onmain:should_exit_early_fast()so fast-path trace logs are visible.export MISE_TRACE=1cdinto a directory with amise.tomlbut nomise.lockhook-env, and observe thatshould_exit_early()keeps running and returningfalsebecausemise.lockis treated as a watched file that was "deleted", even though it never existed.touch mise.tomlhook-envnow exits early as expected andmise.lockis no longer present in the resolved watch files.I don’t yet understand why touching
mise.tomlchanges whethermise.lockappears in the watch set, but the current behavior seems wrong regardless: a missing optionalmise.lockshould not keephook-envfrom stabilizing.My test seems to hit this sometimes as well, which may explain the flakiness. I haven’t pinned down exactly what makes it appear or disappear yet, but once
mise.lockis no longer treated as a required existing watched file, I suspect the test will become stable too.As a potential fix, I tried ignoring
mise.lockif it didn't exist. That improved things so that when youcdinto a directory, the fast check allows for an early exit on subsequent prompts. However, if I thentouch mise.lock, it starts falling back to the slow check again.I'm seeing if I can identify what's causing this to happen. It's maybe a bit out-of-scope at this point, but I don't want to introduce a flaky test.
Uh oh!
There was an error while loading. Please reload this page.
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.
Alright, I think I have that
mise.lockissue resolved. I tried to fix it in a separate PR but then that e2e test ran into the same problem being fixed in this PR, so I figured I'd just group the fixes together in this PR. Let me know if there are any concerns about the size of the PR though.