Skip to content

Send generated functions to the line that declared them - #110

Merged
JesseHerrick merged 17 commits into
mainfrom
beam-debug-info-definition
Oct 3, 2026
Merged

JesseHerrick merged 17 commits into
mainfrom
beam-debug-info-definition

Conversation

@JesseHerrick

@JesseHerrick JesseHerrick commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

Closes #108. Supersedes #102, which is folded in here. Go-to-definition for DSL-generated functions such as Ash code interfaces. It does not special-case any framework. The Ash side is ash-project/ash#2971, which is merged and will ship in the next Ash release after v3.33.11. With released Ash, a code interface goes to its define line too, from the generic declaring-call search described below.

Summary

A function that a macro generates has no source definition, so go-to-definition fell back to the top of its module. The compiled module's Dbgi chunk records the location of each def:

  • Its :line is the line in the module's own file that was compiled when the def was made. Usually this is the macro call or the line where a before-compile hook ran, but a generator can expand the def at any line.
  • A file: {path, line} entry comes from @file or from quote location: :keep. When path is the module's own source, this line is where the code asked for the function. When path is another file, it is the generator's own implementation, and :line is used instead.

Released Ash records line: 1, file: {"deps/ash/lib/ash/code_interface.ex", N} for every generated def. After ash#2971, Ash expands each generated def at its define line, so the debug info is line: <define line>, file: {"deps/ash/lib/ash/code_interface.ex", N}. The foreign-file rule above picks up :line. Ash did not use @file, because the function body's lines still come from code_interface.ex, so stacktraces would show the user's file with Ash's line numbers. Their approach keeps stacktraces unchanged.

Changes

  • beam.ReadDefinitionLines walks {:debug_info_v1, :elixir_erl, {:elixir_v1, map, specs}} and reads only file, relative_file, and definitions. It steps over clause bodies without allocating. Clause ASTs can nest very deep, so the ETF reader now steps over a term with a count of the terms still to skip instead of recursion: any nesting costs no stack, a count larger than the bytes left is rejected as corrupt, and the old depth guard is gone. This is also about 1.8x faster on large Dbgi chunks; Docs parsing speed is unchanged. Modules without Elixir debug info (debug_info: false, Erlang modules, no chunk) return an error and keep the fallback.
  • generatedDefinitionResultsFor uses a recorded line only when all of these conditions are true:
    • The debug info was compiled from the file being opened (same absolute path, or the relative path matches as a suffix, for a project that was moved or is reached through a symlink).
    • The line is after the module's own line. A line at or before it adds nothing to the module result.
      In all other cases, the result stays as before. A BEAM older than the source still gives its line. Dexter cannot compile the project, so a stale BEAM is the usual state while you edit, and the line from the last compile is closer than the module line. Only edits to the declaring file move it, and the next compile makes it exact. The line is not corrected for those edits: every way to do that meant guessing at the compiled text, and could send the editor to a wrong line that looked right.
  • From Resolve generated definitions to their source annotation #102: beam.Function.Line keeps the Docs chunk anno, and beam.ReadSourcePath reads :source from the CInf chunk. The Docs anno is often the same line as the Dbgi :line, but not always: Ash's code interfaces give the docs line 1. It is used when a module has no debug info, and CInf says which file the line is in.
  • From Resolve generated definitions to their source annotation #102: a generated module with no source row, such as a Spark entity module, goes to the file it was compiled from. When that path does not exist here (a BEAM built elsewhere), it is rebased onto lib/<app>, deps/<app>/lib/<app>, or deps/<app>. The app name comes from the recorded path, because Spark builds Ash's entity modules in Ash's ebin from Spark's source. If no file is found, the lexical parent stays the answer.
  • Bare-call definition and call-hierarchy preparation call it directly. Qualified definition, the references declaration, and dexter lookup get it through LookupName (shared since Unify CLI and editor semantic navigation #107). A recorded line is the function's own definition, so LookupName returns it for a strict (ExactModule) lookup too. Without a recorded line, strict and fallback lookups do not change.
  • The debug info and compile info are read on the first definition request that needs them. It is memoized on the module's generated-function cache entry, so the BEAM stamp invalidates it with everything else.

Also in this PR

  • Declaring call. When the only line the BEAM records is the module line (a @before_compile hook, or released Ash), the module's body is searched for the call whose first argument is the function's name as an atom. When several calls spell the name, as an Ash action and the code interface that runs it do (update :publish and define :publish), the macro whose calls name the most of the module's generated functions wins: define names every interface, an action names only itself. A tie keeps the module line.
  • Clauses. A function with a clause per DSL call (route :get, route :post) goes to every clause, from the line Dbgi records for each clause.
  • Past the end of the file. A recorded line the current text does not have (quote line: 99) is dropped.
  • Module names. Go-to-definition on a module that exists only as a BEAM (Module.create) goes to the file and line the compiler recorded. A module that a macro made with defmodule unquote(name) and a name it computed records no module line, so it goes to its first function's line.
  • Imports. A bare call to a generated function of an imported module resolves.
  • Defs in macro quotes. The parser no longer indexes a def inside a quote in a macro other than __using__ as a function of the macro's module. defmacro route ... quote do def handle made the index claim that the DSL defines handle/2, so a consumer that imports the DSL resolved Consumer.handle into the macro's body before the BEAM was asked. What __using__ injects and a quote in a helper function are still indexed.
  • Older Elixir. Elixir 1.17 and earlier record the module line under line, not anno; both are read. A location: :keep macro defined above its caller in the same file is no longer taken for an @file stamp.
  • Worktree moves (fix for Skip linked git worktrees nested in the project #111). git worktree move renames the directory and then rewrites its .git file in place, so a watcher can read it empty and take the worktree for a plain directory: the fsnotify backend reported every file in it, and the FSEvents backend ran a full reconcile. A .git file that names no git directory yet now makes the directory a pending top, which is checked again once git is done. This was the cause of the flaky TestFSNotifyWatcherSkipsWorktreeAddedWhileRunning on Linux; both backends now have a deterministic test for it.
  • Compressed BEAMs. A BEAM compiled with the compressed option (a gzip stream, used by some Erlang dependencies) is decompressed instead of rejected.
  • Integration tests. TestDefinition_GeneratedFunctionsFromCompiler* compile a DSL fixture of each shape with mix (with and without debug info, and with a stale source) and run in the integration job.

Performance

A cold read on real Ash modules takes 0.8–2 ms (inflate plus walk; Repro.Chat's Dbgi is 19 KB compressed and 350 KB inflated). It happens once per BEAM stamp and only on a definition miss for a generated function. The parser change below bumps IndexVersion (and the daemon contract), so each workspace rebuilds its index once after the upgrade, through the parallel full build.

Validation

  • go test ./... and make lint pass.
  • New internal/beam tests: own-file location, foreign location, plain :line, macros and private defs, stripped or Erlang debug info, truncation at every seventh byte, a clause body 200,000 ETF levels deep, and a test that compiles real modules with elixirc (skipped if Elixir is not installed) for @file, location: :keep, and plain quotes. It also checks that the Docs anno and the CInf source agree with the debug info.
  • New LSP tests: qualified and bare definitions and call hierarchy go to the recorded line, for both the @file shape and the Ash shape (:line with a generator file). LookupName returns the recorded line with and without ExactModule, and a strict lookup without a recorded line keeps the module. A stale BEAM goes to the line it recorded, and never to a line past the end of the file. A foreign location with no useful :line, and debug info from a different source file, keep the module line. The Docs anno gives the line when there is no Dbgi, and does not when there is no CInf source. A sourceless generated module goes to its rebased deps file from Dbgi or Docs, and to its lexical parent when that file is missing.
  • End to end on the repro app from Go-to-definition for Ash DSL-generated functions (code interfaces) resolves to line 1 #108 against Ash main (9fa5088, includes ash#2971), with lspprobe for definition and dexter lookup --strict for the CLI. Both give the same lines:
Probe main this branch
Chat.get_room_by_slug! chat.ex:1 chat.ex:7
Chat.create_room chat.ex:1 chat.ex:8
Repro.Chat.Room.rename (resource code_interface) room.ex:1 room.ex:19

End to end on a project with no dependencies and a DSL in the project, for macros that are not Ash (main gives line 1 of user.ex for all of them except hello):

Generator this branch
plain quote (field :email) user.ex:4
location: :keep user.ex:6
@file {file, line} user.ex:8
__using__ injection dsl.ex:5 (unchanged, the def in the quote)
nested defmodule from a macro user.ex:10
Module.create in the generator's file (no source row) dsl.ex:43

The same results hold for a copy of the project that was not recompiled, for an umbrella copied the same way (apps/<app>/lib), and for a project in a directory named with é and 日本.

🤖 Generated with Claude Code

@onnimonni

Copy link
Copy Markdown
Contributor

I tried the Ash half of this. Findings, in case they change the plan:

@file has a cost Ash probably won't take: it breaks stacktraces. @file sets the file for the whole generated function, but with location: :keep its body's line numbers still come from code_interface.ex. So a crash inside a generated function reports the user's domain file with Ash's line numbers (e.g. lib/my_app/chat.ex:1150, a line unrelated to anything), where today it correctly says deps/ash/lib/ash/code_interface.ex:1150. "Stacktraces point at the define line" only holds for the function head, not the frames users see. Minimal repro (Elixir 1.20.4):

defmodule Gen do
  defmacro gen(name, override?) do
    quote bind_quoted: [name: name, override?: override?], location: :keep do
      if override?, do: @file({"/user/domain.ex", 42})
      def unquote(name)(x) do
        r = String.to_integer(x)   # line 7 of gen.ex
        r + 1
      end
    end
  end
end
# stamped frame: {User, :stamped, 1, [file: ~c"/user/domain.ex", line: 7]}

What works with this PR as it is: keep file pointing at code_interface.ex and set only the def's :line to the define line. :line is the environment's line when def expands, so Ash can evaluate each generated def with %{env | line: define_line} (dropping the def node's own :line meta). Debug info becomes line: 18, file: {"deps/ash/lib/ash/code_interface.ex", N}, stacktraces are unchanged, and this PR's foreign-file fallback to :line picks it up without changes. With this branch plus that Ash change, lspprobe on a Phoenix app gives chat_ash.ex:18 / :16 for get_room_by_slug! / list_rooms! (main: :1, the defmodule line).

Ash issue: ash-project/ash#2970, PR: ash-project/ash#2971.

One small thing I hit while testing: this branch indexes the enclosing checkout's .dexter when run inside a linked worktree nested in the repo. #111 fixes that, so I tested the two together.

A function a macro generated has no source definition, so go-to-definition
fell back to the top of its module. The compiled module's debug info records
where each def came from: its :line, and a `file: {path, line}` entry left by
`@file` or `quote location: :keep`. When that path is the module's own source,
the line is where the code asked for the function; when it names the
generator's own file, :line is used instead.

beam.ReadDefinitionLines reads those locations from the Dbgi chunk and steps
over clause bodies without allocating. generatedDefinitionResultsFor uses a
recorded line only when the debug info was compiled from the file being
opened, the BEAM is not older than it, and the line falls after the module's
own line. Every other case keeps the module result.

Bare-call definition and call-hierarchy preparation call it directly.
Qualified definition, the references declaration, and `dexter lookup` reach
it through LookupName, which takes a recorded line even for a strict lookup,
since that line is the function's own definition.

No framework is recognized by name. Ash code interfaces (#108) resolve to
their `define` line once Ash includes ash-project/ash#2971, which expands
each generated def at that line; with released Ash they resolve to line 1 as
before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@JesseHerrick
JesseHerrick force-pushed the beam-debug-info-definition branch from 1af27b8 to 8df255d Compare September 28, 2026 21:42
Drop the stale-BEAM guard. Dexter cannot compile the project, so a BEAM
older than its source is the usual state while editing, and its line is
closer than the module line.

From #102:
- Keep the Docs chunk anno as beam.Function.Line, and use it when a module
  has no debug info. The CInf chunk's :source (beam.ReadSourcePath) says
  which file that line is in.
- Send a generated module with no source row, such as a Spark entity
  module, to the file it was compiled from, rebased onto the project's
  lib or deps when it was built elsewhere.

Move the definition-line code into generated_definition.go.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
When the source, or an unsaved buffer, has changed since the BEAM was
written, move each recorded line to the nearest call whose first argument
is the function's name as an atom (`define :list_rooms`). That is the
generic shape of a macro call that declares a name. Keep the recorded line
when it still declares the function, when no line does, or when two matches
are equally near. A current BEAM is never corrected.

Also say that the Docs anno is often, but not always, the Dbgi line: Ash's
code interfaces give the def its define line and the docs line 1.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/lsp/generated_definition.go Outdated
Comment thread internal/lsp/generated_definition.go
JesseHerrick and others added 2 commits September 29, 2026 18:28
- Follow drift before the "after the module line" check, so lines added
  above defmodule no longer drop the recorded line.
- Search to both ends of a file that got shorter than the recorded line,
  and drop a line past the end with nothing to move to.
- Treat only open buffers as unsaved edits; a file cached by another
  request is not one.
- Skip heredoc lines and module attributes when looking for a declaration.
- Prefer this checkout over the recorded path when a _build was copied
  from another checkout or worktree, and try the recorded path's tails
  under the root first, which covers umbrella apps and moved projects.
- Decode CInf charlists as Unicode codepoints, so non-ASCII paths work.
- Bound the Dbgi definitions preallocation against a corrupt count.
- Do not overwrite a newer BEAM's cache entry with an older one.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/lsp/generated_definition.go Outdated
JesseHerrick and others added 3 commits September 29, 2026 18:36
…file

- Follow a stale line only to a declaration in the body of the module
  that owns it, found with the tokenizer, so a sibling module in the same
  file that declares the same name is never the answer. A generated
  module with no source row has no owner in the generator's file, so its
  line is not corrected there.
- When a module has rows in more than one file, as two umbrella apps can,
  choose the row that shares the most trailing path components with the
  recorded source. If two rows match equally, keep the module line.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A generator can give a def any line, as `quote line: 99` does, so even a
current BEAM can record a line the file does not have. Check each line
against the current text, open buffer or disk, and fall back to the module
line when it is past the end.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A function whose only recorded line is the module line, such as one a
  @before_compile hook made, goes to the one call in its module that
  declares it by name. This also places released Ash code interfaces.
- A function with a clause per DSL call goes to every clause.
- A stale line with no name to search for is placed by the source
  definitions around it, from every module in the file; when an edit is
  between them, only a one-to-one match to `def unquote` lines counts.
- A BEAM's mtime has whole seconds, so compare staleness by the second;
  a save in the same second as the compile no longer looks stale.
- While a buffer has unsaved edits, the module line comes from the buffer.
- Go-to-definition on a module that exists only as a BEAM goes to the file
  and line the compiler recorded for it.
- A bare call to a generated function of an imported module resolves.
- Read {line, column} annotations.
- Add integration tests that compile a DSL fixture of each shape with mix,
  and run them in the integration job.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/beam/debug_info.go
Comment thread internal/lsp/generated_definition.go Outdated
Anchors that moved together are the most exact signal, so they now come
before the search for a call that spells the function's name. That search
is skipped for a function whose clauses several calls made: those calls do
not spell its name, so a call that does, such as `plug :match`, would pull
every route clause onto one line.

Also test the cases from review: route clauses next to `plug :match` in a
stale file, a multi-clause `location: :keep` generator, and lines removed
above the module in an unsaved buffer.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/lsp/generated_definition.go Outdated
A stale BEAM's line is now used as it is. Only edits to the declaring file
move it, and those files (DSL modules, schemas, routers, dependencies)
change less often than their callers; the next compile makes it exact.
Correcting it meant guessing at the text the BEAM was compiled from, and
each signal either gave up often or could send the editor to a line that
looked meaningful and was not.

What stays: a line past the end of the file is never returned, the module
line check compares lines from the same compile, and the search for the
one declaring call, which reads the current text, still places a def made
at the module line.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/lsp/generated_definition.go
Comment thread internal/lsp/generated_definition.go
JesseHerrick and others added 4 commits September 30, 2026 23:28
The module line came from the index when the BEAM had none for the
module, as for a module a macro nested in its parent. The index has the
current text, so after lines were added above defmodule it could fall
after a recorded line from the compile and drop it. Take the parent's
line from the parent's BEAM, and use the index only when no BEAM has one.

Do not search for a declaring call for a function whose clauses were made
at several lines: those calls do not spell its name, so a call that does,
such as `plug :match`, is not where they were declared.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
blankHeredocs blanked the whole line that opens a heredoc, so a call such
as `define :foo, description: """` was not found by the declaring-call
search. The opening line now keeps its text before the delimiter.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A macro that expands `defmodule unquote(name)` with a name it computed
leaves the module line at zero in the debug info, so go-to-definition on
that module's name found nothing. Its functions still record the line of
the call that made them, so the first of those is used instead.

When several calls in a module spell a generated function's name, the
search for the declaring call gave up. That is the usual shape of an Ash
resource, where an action and the code interface that runs it share a
name (`update :publish` and `define :publish`). The macro whose calls
name the most of the module's generated functions now wins, because it is
the one that generates them. A tie still keeps the module line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/beam/debug_info.go
Comment thread internal/beam/debug_info.go
A def inside a quote in a macro other than __using__ is code the macro
generates in its caller, not a function of the macro's module. The
parser indexed it as one, so `defmacro route ... quote do def handle`
made the index claim that the DSL defines handle/2, and a consumer that
imports the DSL resolved `Consumer.handle` into the macro's body before
the compiled BEAM was asked. Such defs are now skipped. What __using__
injects and a quote in a helper function are still indexed. This changes
the index, so IndexVersion and the daemon contract are bumped.

Elixir 1.17 and earlier record a module's line under the debug info
map's `line` key, not `anno`, so it was never read and recorded function
lines were compared with the index's module line instead. Both keys are
now read.

A `location: :keep` quote in a macro defined above its caller in the
same file records that file and the line inside the macro, which was
taken for an @file stamp. A recorded file line at or before the module's
own line is now ignored, and the call's line is used.

A BEAM compiled with the `compressed` option is a gzip stream around the
container, which was rejected as an invalid BEAM. It is now decompressed
with the same size limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cursor cursor 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.

Stale Bugbot comment from a previous run.

Comment thread internal/parser/parser_tokenized.go
…erms

A macro written with `, do:` has no do-block of its own, so a quote in
it (`, do: quote do` on one line, or `quote do` on the next) was never
seen as inside a macro, and its defs stayed indexed. The macro's head
line is now scanned for a quote, and a quote right after its `do:` is
recognized too.

git worktree move renames the directory, then writes its .git file again
in place, so a watcher can read the file empty. The worktree then looked
like a plain directory: the fsnotify backend reported every file in it,
and the FSEvents backend ran a full reconcile. A .git file that names no
git directory yet now makes the directory a pending top, which is checked
again once git is done. This was the cause of the flaky
TestFSNotifyWatcherSkipsWorktreeAddedWhileRunning on Linux.

Stepping over an ETF term recursed, under a depth limit that a deeply
nested clause body could exceed, which cost the whole module its debug
info. It is now a loop over a count of the terms still to skip, bounded
by the bytes left, so nesting costs no stack. It is also about 1.8x
faster on large Dbgi chunks. readAnnoLine is a loop for the same reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cursor cursor 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.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3bbd46a. Configure here.

Comment thread internal/beam/etf.go
Comment thread internal/parser/parser_tokenized.go
skipTerms counted a term among those still to step over while it read
the term's header, so a valid term at the end of the input, such as an
empty map or `[[], []]`, looked like more terms than bytes left and was
rejected. A term is now counted off before its header is read.

A macro written as `defmacro name, do: quote do: def generated` set the
quote line to its own line while its head was scanned, and the check
that follows then skipped the macro itself. Whether a def is inside a
quote is now decided before its head is scanned.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JesseHerrick
JesseHerrick merged commit f840dd2 into main Oct 3, 2026
5 checks passed
@JesseHerrick
JesseHerrick deleted the beam-debug-info-definition branch October 3, 2026 23:13
JesseHerrick added a commit that referenced this pull request Oct 4, 2026
Closes four gaps in how nested worktrees and unreadable directories are
handled (#111, and the worktree-move fix in #110). The last one is the
cause of the flaky `TestFSNotifyWatcherSkipsWorktreeAddedWhileRunning`
failures in CI.

## Summary

- **Two reads of one `.git` file could disagree (the cause of the CI
flake).** git 2.48 and later rename a worktree on `git worktree move`
and then write its `.git` file again in place (truncate, then write;
confirmed with strace on git 2.55). The watcher's walk asked "is this a
worktree?" and "is its `.git` file still being written?" with two
separate reads. The first could see the truncated file and the second
the complete one, so a worktree was neither, and every file in it was
reported as a plain directory. git 2.47 does not write the file again,
which is why it only showed up on CI's newer git.

- **A directory that the watcher cannot read was skipped silently.**
`walkDirectories` returned when `ReadDir` failed, for example when the
process had no file descriptor left (the kqueue backend uses one per
watched file). The subtree below it was not watched, not marked failed
and never retried, so the coverage report said nothing. The `Create`
handler then reported every file in it as if it were a plain directory,
also when it was a worktree that had been moved into place.
- **The index walks did not skip a worktree that git is still moving.**
`git worktree move` renames the directory and then writes its `.git`
file again in place. #110 made the watchers treat a `.git` file that
names no git directory yet as a pending worktree, but `WalkElixirFiles`
and `CollectElixirFilesParallel` did not, so a reconcile or a first
build at that moment indexed the whole worktree.

## Changes

- `parser.GitFile` and `parser.GitFileFromEntries` return one state
(none, plain, worktree, unsettled) from one read of the `.git` file. The
fsnotify walk and pending check, the FSEvents handler and its later
check, both index walks, and the runtime's removal of a reported
worktree use it. Same cost as before: no syscall for a directory without
a `.git` entry, one read for a directory with one.

- `walkDirectories` marks a directory it cannot read as failed. The
coverage report then says that the tree is not covered, and the retry
timer reads it again. A retry clears the failure only when it could read
the directory.
- The `Create` handler does not report the files of a directory that it
could not read. When coverage comes back, the runtime reconciles, and
that walk indexes the files of a plain directory and skips a worktree.
- Both index walks skip a nested directory whose `.git` file names no
git directory yet, as the watchers do.
- The watcher's directory reads can be replaced in tests (`readDir`), so
a failed read can be simulated without timing.

## Validation

- `TestWatcherDoesNotReportDirectoryItCouldNotRead`: a worktree moved
into place and a plain directory, each with a failed read, a retry that
still fails, and a retry that works. Nothing is reported from the
unreadable directory, the coverage goes degraded and then restored, and
the retry classifies the directory. With the old behavior, the test
reports the moved-in worktree's files.
- `TestWalkAndCollectSkipUnsettledGitFile`: both walks skip a directory
with an empty `.git` file. Without the change, both index it.
- `TestGitFileStates` (each state) and `TestGitFileReadsTheFileOnce`,
which serves an empty file on the first read and a complete one on the
second, as git does: one classification reads once and answers
"unsettled".
- Reproduced the CI failure in a Linux container like CI (Ubuntu 24.04,
git 2.55): `TestFSNotifyWatcherSkipsWorktreeAddedWhileRunning` failed in
2 of 20 race runs before the change. After it: 100 of 100 with git 2.55,
and 50 of 50 with git 2.47. Tracing the failing run showed the walk
deciding "not a worktree" while the `.git` file and git's records were
valid a moment later.
- The unreadable-directory fix in the first commit is a separate real
gap (proven by its own test), but it was not the cause of the CI flake,
as this PR first said.
- `go test ./...`, `go test -race` on `internal/workspace` and
`internal/parser`, ten race runs of every watcher test, and
`golangci-lint` pass. No change to the walk for directories without a
`.git` entry, so the walk costs the same.
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.

Go-to-definition for Ash DSL-generated functions (code interfaces) resolves to line 1

2 participants