Skip to content

fix(lsp): mark late-auxiliary coverage at notify time, not ceiling expiry (refs #3482) - #3486

Merged
apmantza merged 1 commit into
apmantza:masterfrom
drakeo338:claude/3482-late-aux-mark-notify-time
Sep 26, 2026
Merged

apmantza merged 1 commit into
apmantza:masterfrom
drakeo338:claude/3482-late-aux-mark-notify-time

Conversation

@drakeo338

@drakeo338 drakeo338 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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 gets startedAt, 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

- LSPService.touchFile (clients/lsp/index.ts:6034)
  + markPendingAuxiliaryCoverage(..., startedAt)

Tests

New test #3482: marks the collect-later pair at the touch's own notify time in 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(" clients finds one producer.

Observability

No new failure path; no record added.

…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.
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for your first pull request! A maintainer will review it soon. Please make sure CI passes and the PR template is filled out.

@sonarqubecloud

Copy link
Copy Markdown

@apmantza apmantza left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 startedAt reverted to Date.now(), #3482: marks the collect-later pair at the touch's own notify time… fails with AssertionError: expected 2800 to be less than 50, as quoted. It passes with the fix.
  • One unrelated failure. service-aux-grace.test.ts has 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

@apmantza
apmantza enabled auto-merge September 25, 2026 23:56
@apmantza
apmantza merged commit 9803ed3 into apmantza:master Sep 26, 2026
50 of 51 checks passed
apmantza pushed a commit that referenced this pull request Sep 26, 2026
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
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.

2 participants