Skip to content

ADFA-5414 + ADFA-5416: cleanup and guard-order fixes in the tree-sitter query path - #1778

Open
davidschachterADFA wants to merge 2 commits into
task/ADFA-5401-treesitter-close-racefrom
task/ADFA-5414-query-cursor-cleanup
Open

ADFA-5414 + ADFA-5416: cleanup and guard-order fixes in the tree-sitter query path#1778
davidschachterADFA wants to merge 2 commits into
task/ADFA-5401-treesitter-close-racefrom
task/ADFA-5414-query-cursor-cleanup

Conversation

@davidschachterADFA

@davidschachterADFA davidschachterADFA commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

ADFA-5414 + ADFA-5416: cleanup and guard-order fixes in the tree-sitter query path

Stacked on #1776. Base is task/ADFA-5401-treesitter-close-race, not stage, because both files are reformatted by that PR's ratchet commits — basing on stage would duplicate those reformats and conflict. Merge #1776 first, or retarget this to stage once it lands.

Two independent fixes found while reviewing #1776. Both are small; they share a branch because they are in the same two files and the same review context.

ADFA-5414 — clean up on both exits from the query cursor loop (68ed840)

doSafeExecQueryCursor had two ways out and only one cleaned up. The check after action(match) called onClosedOrEdited() and recycled the match; the loop condition could also become false, and that path exited straight out — the caller's compensating action never ran, and the match it was holding never went back to the pool.

That matters because onClosedOrEdited is how a caller discards a partial result:

Caller onClosedOrEdited Consequence of the silent exit
updateCodeBlocks { blocks.clear() } commits a half-built list — the editor draws a truncated set of fold/indent guides instead of none
TsScopedVariables.init { captures.clear() } commits a torn scope tree, mis-resolving local-variable highlighting

The loop is now driven by match != null — normal exhaustion, which must not trigger onClosedOrEdited — with the condition re-checked at the top as well as after the action. Both failure exits run onClosedOrEdited() and recycle; the whileTrue exit recycles without it, since a caller stopping on purpose is not an error.

Pre-existing, but #1776 widened it: adding query.canAccess() to matchCondition means a freed query now reaches this exit too, not just an edited node.

Also corrects the diagnostic beside it, which printed node.hasErrors() under a node.hasChanges label — matchCondition tests hasChanges, so the log pointed at the wrong thing precisely when someone would be reading it.

Not addressed: the early return result path still skips the node.recycle() below the loop. Separate leak, and a separate decision about whether a returned result may reference the node.

ADFA-5416 — check canAccess() before reading patternCount (aa973a8)

updateCodeBlocks() guarded on blocksQuery.patternCount == 0 || !blocksQuery.canAccess() || …, but getPatternCount() calls checkAccess() internally — so on a closed query the first operand throws before the guard beside it can return. The exception left updateStyles() and was swallowed as "AnalyzeWorker crashed", dropping the whole style update rather than skipping just the code-blocks step. Swapped, matching TsBracketPairs, which already had the operands this way round.

Testing

  • On device (Pixel 6 Pro, Android 17): syntax highlighting renders and updates normally — this loop is what feeds it — with no AnalyzeWorker crashed, no Cannot access native object, no SIGSEGV.
  • :editor-api:, :editor-treesitter: and :editor: compile; spotlessApply clean.
  • No unit test: editor-api has no test source set for this helper, and the behaviour is a teardown race. The argument is the control flow, which is small enough to read.

🤖 Generated with Claude Code

https://claude.ai/code/session_011Crj29d6Q2DGjioWPxtuSR

davidschachterADFA and others added 2 commits September 2, 2026 18:18
doSafeExecQueryCursor had two ways out and only one of them cleaned up.
The check after action(match) called onClosedOrEdited() and recycled the
match before breaking; the loop condition itself could also become false,
and that path exited straight out - the caller's compensating action never
ran and the match it was holding never went back to the pool.

That matters because onClosedOrEdited is how a caller discards a partial
result. updateCodeBlocks passes { blocks.clear() }, so an exit through the
loop condition committed a half-built list and the editor drew a truncated
set of fold and indent guides instead of none. TsScopedVariables passes
{ captures.clear() } and would commit a torn scope tree.

The loop is now driven by `match != null` - normal exhaustion, which must
not trigger onClosedOrEdited - with the condition re-checked at the top as
well as after the action. Both failure exits run onClosedOrEdited() and
recycle; the whileTrue exit recycles without it, since the caller stopping
on purpose is not an error.

Pre-existing, but ADFA-5401 widened it by adding query.canAccess() to
matchCondition, so a freed query now reaches this exit as well as an
edited node.

Also corrects the debug diagnostic beside it, which printed
node.hasErrors() under a "node.hasChanges" label - matchCondition tests
hasChanges, so the log pointed at the wrong thing exactly when someone
would be reading it.

Not addressed here: the early `return result` path still skips the
node.recycle() below the loop. That is a separate leak and a separate
decision about whether a returned result may reference the node.

Verified on device (Pixel 6 Pro, Android 17): syntax highlighting renders
and updates normally - this loop is what feeds it - with no
"AnalyzeWorker crashed", no "Cannot access native object" and no SIGSEGV.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Crj29d6Q2DGjioWPxtuSR
updateCodeBlocks() guarded on

    blocksQuery.patternCount == 0 || !blocksQuery.canAccess() || ...

but TSQuery.getPatternCount() calls checkAccess() internally, so on a
closed query the first operand throws IllegalStateException before the
guard beside it can return. The exception left updateStyles() and was
swallowed by processNextMessage()'s catch as "AnalyzeWorker crashed",
dropping the whole style update rather than skipping just the code-blocks
step the guard existed to skip.

Swapped, matching TsBracketPairs, which already had the operands this way
round.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Crj29d6Q2DGjioWPxtuSR

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

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