Skip to content

Drop stale analyses when the index becomes ready - #252

Open
alganet wants to merge 1 commit into
jorgsowa:mainfrom
alganet:fix/stale-analysis-at-index-ready
Open

alganet wants to merge 1 commit into
jorgsowa:mainfrom
alganet:fix/stale-analysis-at-index-ready

Conversation

@alganet

@alganet alganet commented Aug 31, 2026

Copy link
Copy Markdown

A file opened while the workspace scan is still running is analyzed against whatever has been indexed by then, so it can report a symbol as missing purely because the scan had not yet reached the file declaring it. compute_open_file_diagnostics knows that and holds UndefinedClass and UndefinedFunction back until is_index_ready() flips, and republish_after_index_ready sends the fuller set once it has.

Holding them back is only half of it. The analysis cache is keyed on (source, decl_version) and a finishing scan changes neither, so the republish re-renders the same memo through an open gate -- and publishes exactly the stale findings the gate existed to hide. They stay through pull requests as well as pushes, until somebody types a character in the file, which changes the source, misses the cache, and produces the right answer.

This is the case 9149c80 left. That commit fixed the same staleness for didChangeWatchedFiles by calling note_new_file_declarations for every CREATED/CHANGED file; the initial scan reaches the store through mirror_text/ingest_from_doc and calls neither. Issue #242's third acceptance criterion -- "Full semantic diagnostics are available after indexing completes" -- is not met until both paths invalidate.

So mark_index_ready bumps decl_version on its way through. That is what the counter is for: evict_analysis_all already carries the same meaning for a PHP-version change, and "the workspace is now complete" is the same claim about the same cache. Bumping once at the transition also answers the objection recorded in ae51285, which is why this was not fixed there: bumping per scanned file would thrash the cache during a large scan, and one bump at readiness costs one analysis per open file, once per session.

The new test is the deterministic sibling of
stale_cached_analysis_not_invalidated_by_new_dependency_file, with the scan's mirror_text-and-nothing-else in place of that path's explicit note_new_file_declarations.

A file opened while the workspace scan is still running is analyzed
against whatever has been indexed by then, so it can report a symbol as
missing purely because the scan had not yet reached the file declaring
it. `compute_open_file_diagnostics` knows that and holds UndefinedClass
and UndefinedFunction back until `is_index_ready()` flips, and
`republish_after_index_ready` sends the fuller set once it has.

Holding them back is only half of it. The analysis cache is keyed on
`(source, decl_version)` and a finishing scan changes neither, so the
republish re-renders the same memo through an open gate -- and publishes
exactly the stale findings the gate existed to hide. They stay through
pull requests as well as pushes, until somebody types a character in the
file, which changes the source, misses the cache, and produces the right
answer.

This is the case 9149c80 left. That commit fixed the same staleness for
`didChangeWatchedFiles` by calling `note_new_file_declarations` for every
CREATED/CHANGED file; the initial scan reaches the store through
`mirror_text`/`ingest_from_doc` and calls neither. Issue jorgsowa#242's third
acceptance criterion -- "Full semantic diagnostics are available after
indexing completes" -- is not met until both paths invalidate.

So `mark_index_ready` bumps `decl_version` on its way through. That is
what the counter is for: `evict_analysis_all` already carries the same
meaning for a PHP-version change, and "the workspace is now complete" is
the same claim about the same cache. Bumping once at the transition also
answers the objection recorded in ae51285, which is why this was not
fixed there: bumping per scanned file would thrash the cache during a
large scan, and one bump at readiness costs one analysis per open file,
once per session.

The new test is the deterministic sibling of
`stale_cached_analysis_not_invalidated_by_new_dependency_file`, with the
scan's `mirror_text`-and-nothing-else in place of that path's explicit
`note_new_file_declarations`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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