Skip to content

perf(tools): defer tool cache version check to background refresh#36

Merged
ChangeHow merged 1 commit into
mainfrom
perf/async-tool-cache
May 13, 2026
Merged

perf(tools): defer tool cache version check to background refresh#36
ChangeHow merged 1 commit into
mainfrom
perf/async-tool-cache

Conversation

@ChangeHow
Copy link
Copy Markdown
Owner

What

  • Defer tool cache version check to a background subshell so shell startup stays fast
  • Cache is now sourced immediately if it exists; stale detection (older than 24h) runs asynchronously via () { ... } &!
  • Added fallback slow path when cache is missing

Tests

  • Isolated FNM_DIR and XDG_DATA_HOME in frontend.test.js to prevent host fnm installation from interfering with sandboxed tests

Checklist

  • Code changes are covered by tests
  • Existing tests pass

@ChangeHow ChangeHow merged commit cb04f48 into main May 13, 2026
1 check passed
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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; }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant