Repository navigation
Fix env-keyed probe memo written three ways (#1888) - #1895
Mikola Lysenko (mikolalysenko) wants to merge 4 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
Red test for #1888: the Python site query memo files an answer under the old environment key even when the environment changed while the query ran, so a later ask in the old environment is served a stale interpreter answer. Assisted-by: Claude Code:claude-opus-5-5
Composer, Python and RubyGems discovery each cached a probe answer (composer global home, the Python site query, gem env) keyed on the process environment, with three copies of the key and two memo types. The Python copy kept an answer even when the environment changed while the probe ran, so a later ask could get an interpreter answer from the wrong environment. All three now use utils::process::EnvKeyedMemo and env_key, which keep an answer only if the environment is unchanged after the probe. Ruby keeps its per-environment single-flight cells on the shared key. Fixes #1888 Assisted-by: Claude Code:claude-opus-5-5
Skip the environment clone on a cache hit: the key is passed to the probe only on a miss. Drop the Python copy of the keep-rule test (the shared helper's test covers it, and SITE_QUERY_MEMO is that type), and drop test assertions that raced other tests changing the environment or working directory. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit baa250a. Configure here.
|
Ready for review (burn-down agent) at
Generated by Claude Code |
Final review briefWhat it does: It moves the env-keyed probe memo, which Composer, Python and Ruby each wrote their own way, into one shared Risk: low. The cache key, single-slot replacement, recovery from a poisoned lock and "never cache Look here
Verified: I read the full diff and compared each call site with main (cache key, invalidation, locking, error caching, Windows variable-name handling). There is no debug code or scope creep, and Changes I made: none. Open questions (non-blocking):
Auto-merge is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1888
Summary
Composer, Python and RubyGems discovery each cache one probe answer
(
composer global config home, the Python site query,gem env) keyedon the process environment and working directory. That key function
was written three times, and there were two separate single-slot memo
types. They now share
utils::process::{EnvKey, env_key, EnvKeyedMemo}.Root cause
The memo was copied per crawler instead of living in one helper, and the
copies drifted. Composer and Ruby keep an answer only if the environment
is unchanged when the probe returns. Python's
KeyedMemokept everysuccessful answer, so an answer computed while another thread changed
the environment was stored under the old key. A later ask in the old
environment then got the wrong interpreter's site-packages.
Changes
utils::process:EnvKey,env_key(), andEnvKeyedMemo<V>withComposer's rule. It has one slot and keeps an answer only when the
answer is
Someand the environment is unchanged after the run. On acache hit it no longer clones the environment.
EnvKeyedMemo<String>directly. Composer'sEnvKeyedMemo/env_keyand Python'sKeyedMemo/SiteQueryKey/site_query_keyare deleted.gem env gemdirandgem env gempathtogether and waits on concurrentaskers), but keys them on the shared
EnvKey/env_key.GemEnvKey/gem_env_keyare deleted. The issue allows this option, andit leaves Ruby's concurrency unchanged.
utils::process, and there is a new end-to-endkeep-rule test against the real process environment.
Test evidence
site_query_memo_drops_an_answer_the_environment_changed_underagainstthe old Python memo. It fails with
left: Some("ran under a moved environment") right: Some("fresh").utils::process::tests::env_keyed_memo_drops_an_answer_the_environment_changed_underruns the same scenario on the shared memo, which
SITE_QUERY_MEMOnow uses, and passes. The Python copy was removed as a duplicate.
cargo fmt --all -- --check: clean.cargo clippy --workspace --all-features -- -D warnings: clean.cargo test --workspace --all-featureson 9ff63ee: passed (exit 0).cargo test -p socket-patch-core --all-features: 6994 passed, 4 failed. The 4 failures(
copy_tree::relax_loop_must_not_traverse_symlinked_root,vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry,pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched,pypi_requirements::wire_failure_rolls_back_already_written_files) needread-only directories to block writes. This container runs as uid 0, which
ignores that. None of them touches the code changed here, and CI does not
run as root.
npm/,pypi/andgem/don't use these probes.Per-issue checklist
env_keyed_memo_reuses_only_an_identical_environment,env_keyed_memo_asks_again_after_a_failed_probe,env_keyed_memo_drops_an_answer_the_environment_changed_under,env_key_is_sorted, the Rubymemoize_gem_env_homesandgem_env_cells_are_keyed_on_the_inherited_environmenttests, and Python'ssite_query_memo_follows_the_environment./code-review high findings not fixed
already true of the Composer and Ruby memos; storing only a digest would be a
separate change.
a change that is made and undone while the probe runs (A→B→A). That gap is in
the existing Composer and Ruby rule too. The doc comment now describes exactly
what is checked.
this behavior is unchanged from main.
https://claude.ai/code/session_011f9aePz6bCHA8SMFdPu94K
Note
Medium Risk
Changes caching behavior for ecosystem package discovery probes; the Python fix prevents mis-keyed site-packages paths, while Composer/Ruby behavior should match their prior stricter rule.
Overview
Composer, Python, and Ruby discovery each memoize expensive subprocess probes (
composer global config home, the Python site-packages query,gem env) keyed on process environment and cwd. That logic lived in three copies with two different memo types; Python’sKeyedMemocould cache an answer computed while the environment changed, so a later ask in the original environment could get the wrong site-packages.This PR adds shared
utils::process::{EnvKey, env_key, EnvKeyedMemo}with one keep rule: cache onlySomeanswers when the environment is unchanged after the probe returns. Composer and Python wire their static memos to it and drop local duplicates; Ruby keeps its async per-env single-flight cells but keys them on the sharedenv_key. Memo unit tests move toprocess.rs, including a new test that the keep rule applies to the real process environment.Reviewed by Cursor Bugbot for commit baa250a. Configure here.
Generated by Claude Code