Repository navigation
fix(lsp): mark late-auxiliary coverage at notify time, not ceiling expiry (refs #3482) - #3486
Conversation
…piry (refs apmantza#3482) The turn-end drain uses markPendingAuxiliaryCoverage's timestamp as the freshness baseline for a collect-later auxiliary pair, checking a file's mtime against it to decide whether a later scan result still describes the current revision. The mark was stamped with Date.now() at the point where the aux-grace ceiling wait gives up, which is up to auxGraceMs (2000ms by default) after the touch's own notify actually went out. An edit landing inside that window gets an mtime earlier than the mark, so the drain reads the file as unmodified since the mark and delivers the scanner's findings for the file's prior revision as if they were fresh. Pass startedAt (stamped at touchFile's own entry, before any spawn or wait work) instead. It's the same anchor apmantza#2324's napi-fallback staleness check already uses two lines above for an identical concern, and it is the earliest instant this touch could possibly predate, which is what this baseline needs to be conservative in the right direction. This covers the EditDuringGrace scenario from apmantza#3482. The issue's other scenario, ReTouchRemark, needs two more parts per the issue's own TLA+ model (binding the mark to the scanner's outstanding-publish count, and preserving the baseline across a stale re-arm) that touch shared refresh logic in runtime-turn.ts and have an edge case the model itself flags as uncovered (a scanner that publishes more than once per scan). That's a maintainer design call, not a mechanical fix, so it's left as a follow-up rather than bundled in here.
|
Thanks for your first pull request! A maintainer will review it soon. Please make sure CI passes and the PR template is filled out. |
|
apmantza
left a comment
There was a problem hiding this comment.
Thanks, this is part 1 of #3482's fix and it checks out.
Verified locally at 894fa93:
- The new test is red without the fix. With
startedAtreverted toDate.now(),#3482: marks the collect-later pair at the touch's own notify time…fails withAssertionError: expected 2800 to be less than 50, as quoted. It passes with the fix. - One unrelated failure.
service-aux-grace.test.tshas one other failure here:partitions real lens_diagnostics results by delivering server id (#2776)times out at 5 s. It fails the same way on master's code on this machine (which was under load), so it is not this PR's. CI decides.
Direction of the change. startedAt is taken before any spawn or handshake, so on a cold touch the baseline can be well before the real notify. That errs safely: an earlier baseline only makes the turn-end mtime gate stricter (more "changed since mark" re-arms), and never lets an older revision's findings through. It also matches the napiFallbackCoveredSince(filePath, startedAt) anchor just above.
The rest of #3482. Refs is right. ReTouchRemark still needs the backlog binding, and the latent stale re-arm refresh needs keeping the baseline. Those are in progress on our side and will build on this change rather than duplicate it.
Generated by Claude Code
Parts 2 and 3 of #3482 (part 1, the mark at the touch's entry, is #3486): - the LSP client counts stored publications per path, capped at the sends of the document's current open lifetime (opengrep's rule-load empty publish), and counts a receipt superseded by the resync clear or a newer receipt; the mark records the scanner's unpublished sends and the turn-end drain waits for that many more publications; - a stale verdict re-arms with the same baseline; - a dead or replaced client reads as no binding (WeakRef). Residual (hence refs): a surplus publish that lands while a send is still outstanding counts toward that send, so an older scan's late answer can still meet the count. An opengrep-aware rebaseline needs the client to handle semgrep/rulesRefreshed, which nothing in clients/ does today. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GpuMYnAosBztLRYHDcGN3W
Why
An edit made during the aux-grace wait could make the turn-end drain deliver a scanner's findings for the previous revision (EditDuringGrace in #3482).
Summary
markPendingAuxiliaryCoverage got
Date.now()after the grace wait, up to 2 s after the notify, so that edit looked unmodified to the mtime gate. It now getsstartedAt, the same anchor #2324 uses just above.Notes for the reviewer
Refs #3482, not Closes: ReTouchRemark (parts 2 and 3 of the issue's fix) is left for you, since the issue flags an edge in part 2.
Change outline
Tests
New test
#3482: marks the collect-later pair at the touch's own notify timein service-aux-grace.test.ts. Without the fix:AssertionError: expected 2800 to be less than 50. Lint and build:dist pass.Test assessment
One test added; no existing test changed or made redundant.
Blast radius
One call site; not a hot path.
Class sweep
Class of size 1:
grep -rn "markPendingAuxiliaryCoverage(" clientsfinds one producer.Observability
No new failure path; no record added.