perf(tools): defer tool cache version check to background refresh#36
Conversation
There was a problem hiding this comment.
Code Review
This pull request optimizes Zsh tool loading by implementing a fast-path that sources cached initializations immediately while deferring version checks to a background process. It also improves test isolation in the frontend suite by sandboxing environment variables. Feedback focuses on preventing race conditions during concurrent cache updates by using unique temporary filenames and optimizing the background refresh logic to update cache timestamps even when versions match, avoiding redundant checks.
| cached_version=$(<"$version_file") | ||
| [[ "$current_version" == "$cached_version" ]] && return 0 | ||
|
|
||
| if eval "$init_command" >| "$tmp_file" 2>/dev/null; then |
There was a problem hiding this comment.
There is a potential race condition here. If multiple shell instances (e.g., opening multiple terminal tabs at once) start simultaneously and find the cache stale, they will all attempt to write to the same ${cache_file}.tmp path. This can lead to cache corruption if one process moves the file while another is still writing to it. Consider including the shell's PID ($$) in the temporary filename definition (at line 30) to ensure uniqueness across concurrent processes.
| binary_path=$(command -v "$binary_name") || return 0 | ||
| current_version=$("$binary_path" --version 2>/dev/null | head -1) || current_version="unknown" | ||
| cached_version=$(<"$version_file") | ||
| [[ "$current_version" == "$cached_version" ]] && return 0 |
There was a problem hiding this comment.
When the binary version matches the cached version, the cache file's modification time remains unchanged. This causes the find command at line 42 to continue identifying the cache as 'stale' (older than 24 hours) on every subsequent shell startup. Updating the timestamp of the cache file when the version matches prevents these unnecessary background checks for another 24 hours.
[[ "$current_version" == "$cached_version" ]] && { command touch "$cache_file"; return 0; }
What
() { ... } &!Tests
FNM_DIRandXDG_DATA_HOMEinfrontend.test.jsto prevent host fnm installation from interfering with sandboxed testsChecklist