Skip to content

fix(watch): Treat an unparseable readiness file as not-ready-yet - #177

Merged
JordanCoin merged 1 commit into
mainfrom
claude/codemap-watch-readiness-race
Sep 4, 2026
Merged

fix(watch): Treat an unparseable readiness file as not-ready-yet#177
JordanCoin merged 1 commit into
mainfrom
claude/codemap-watch-readiness-race

Conversation

@JordanCoin

Copy link
Copy Markdown
Owner

Found while investigating a CI failure on #175. Small, standalone — deliberately not folded into that PR, which touches only scanner/.

What's wrong

waitWatchReadiness (main.go:1154) returns on the first successful read:

data, err := os.ReadFile(path)
if err == nil {
    var status watchReadiness
    if err := json.Unmarshal(data, &status); err != nil {
        return fmt.Errorf("reading daemon readiness: %w", err)   // gives up
    }

A readiness file that exists but is empty or half-written reads successfully with zero or partial bytes, so the unmarshal fails and the wait aborts. Only os.ErrNotExist counts as "not ready yet".

Measured against the real function:

empty readiness file   -> reading daemon readiness: unexpected end of JSON input   after 0s
partial readiness file -> reading daemon readiness: unexpected end of JSON input

The 0s is the point: it doesn't wait out any part of the 30s timeout, it fails on the first poll.

An unparseable file is a file still being written, not a daemon that failed. Now the poll continues to the deadline, and the last parse error is surfaced if the deadline passes — so a file that never becomes valid still says why, rather than only that it timed out.

Scope, honestly

publishWatchReadiness already writes via temp-file + os.Rename, so codemap's own daemon does not open this window. This is the reader being brittle to any writer that isn't atomic — defence in depth plus test robustness, not a user-facing startup bug. I said otherwise in an earlier comment on #175 and have corrected it there.

What I could and could not verify

  • Verified: the mechanism, directly against waitWatchReadiness (above), and that the fix makes it wait through a partial write and then honour the real content. Four tests cover: partial → real error, partial → success, persistent garbage → parse error surfaced, absent file → still times out.
  • Not verified: that this resolves the CI failure that led me here (TestRunWatchStartWaitsForChildReadinessFailure on Test (ubuntu-latest, 1.24), fix(scanner): Resolve tsconfig alias targets written as "./*" #175). I could not reproduce that failure locally — 15/15 pass unloaded and 10/10 under CPU contention, on both main and the PR branch.

An earlier attempt of mine appeared to reproduce it at 20/20 on both branches. That measurement was wrong: I was running relocated test binaries from outside the repo, where they exit without running the test at all, so I was measuring my own harness rather than the test. Retracted rather than left standing.

What I do know: the CI error string is produced by exactly one code path, this one; the test's child is sh -c 'printf … > "$CODEMAP_WATCH_READINESS_FILE"', and > truncates the file into existence before printf writes it, which is precisely the window; and codemap/scanner passed in that same run while #175's diff touches nothing else. So this is the plausible cause and the fix is right on its own terms — but I'm not claiming it as proven.

Verification

go vet ./... clean, gofmt clean, go test ./... at the main baseline (only the three known root-environment permission failures).

🤖 Generated with Claude Code

https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo


Generated by Claude Code

waitWatchReadiness returned on the first successful read, so a readiness
file caught mid-write — existing but empty or partial — failed json.Unmarshal
and aborted the wait immediately, reporting a startup failure for a daemon
that had not finished writing. Only os.ErrNotExist counted as "not ready".

Measured against the real function: an empty file returns
"reading daemon readiness: unexpected end of JSON input" after 0s, without
waiting out any part of the 30s timeout.

Keep polling on a parse failure until the deadline, and surface the last
parse error when the deadline passes, so a file that never becomes valid
still says why rather than only that it timed out.

publishWatchReadiness already renames its payload into place atomically, so
codemap's own daemon does not open this window; the reader was brittle to
any writer that is not atomic.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
Copilot AI lite review requested due to automatic review settings September 4, 2026 14:03

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

JordanCoin pushed a commit that referenced this pull request Sep 4, 2026
Ported from #177, which I opened for this failure. Carrying it here so
this PR has its best chance of going green rather than waiting on that one
to merge; it becomes a no-op once main has it.

waitWatchReadiness returned on the first successful read, so a readiness
file caught mid-write failed json.Unmarshal and aborted the wait
immediately. Measured against the real function, an empty file returns
"reading daemon readiness: unexpected end of JSON input" after 0s, which
is the string TestRunWatchStartWaitsForChildReadinessFailure reported on
this PR's ubuntu-1.24 leg.

I could not reproduce that CI failure locally, so this is the plausible
cause rather than a proven one. The change is correct on its own terms
either way: an unparseable file is a file still being written, not a
daemon that failed.

Relates to #173, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo
JordanCoin pushed a commit that referenced this pull request Sep 4, 2026
Two failures on this PR, neither from the testdata exclusion — scanner
passed on both legs.

codemap/mcp: the cargo-metadata deadline failed the whole graph build
instead of falling back. Ported from @reneleonhardt's #171.

codemap root: TestRunWatchModeRunDaemonAndWatchStart hit "reading daemon
readiness: unexpected end of JSON input" — the same window #177 fixes, in
a different test than the one that led me to open it. waitWatchReadiness
gave up on the first unparseable read of a file still being written.

Both no-op once main carries them.

Relates to #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

Copy link
Copy Markdown
Owner Author

New evidence that strengthens this beyond what I could claim when I opened it.

I said the mechanism was confirmed but that I couldn't prove it caused the CI failure on #175, because I never reproduced that failure. Since then, a second, independent test hit the identical error — on #179, a PR that touches only scanner/walker.go:

TestRunWatchModeRunDaemonAndWatchStart/watch_start_shells_out_to_daemon_entrypoint
    main_more_test.go:1108: watch start failed:
        starting daemon: reading daemon readiness: unexpected end of JSON input

That's a different test, in a different subtest, on a different PR, with the same string — and the only code path that produces it is the one this PR fixes. Two unrelated tests landing in the same window is much harder to explain as coincidence than one.

It also sharpens how often this bites: it has now cost a CI leg on #175 and #179, and the sibling cargo-metadata timeout has cost #169, #170, #176 and #178. Between them, six legs across six PRs today.

Still stated precisely: I have not reproduced either occurrence locally (0/10 under load, 15/15 unloaded, on both branches), so this remains the cause consistent with all the evidence rather than one I've triggered on demand. The change stands on its own terms regardless — an unparseable file is a file still being written, not a daemon that failed.

I've ported it into #179 alongside the cargo fix so that PR can go green without waiting on this one.


Generated by Claude Code

@JordanCoin
JordanCoin merged commit e4c9fec into main Sep 4, 2026
12 checks passed
@JordanCoin
JordanCoin deleted the claude/codemap-watch-readiness-race branch September 4, 2026 14:35
JordanCoin added a commit that referenced this pull request Sep 4, 2026
* fix(scanner): Resolve tsconfig alias targets written as "./*"

create-next-app writes "paths": {"@/*": ["./*"]}, and has for years, so this
is the most common TypeScript layout in the wild. Substituting the wildcard
produced "./lib/a1" while the file index stores repository-relative paths with
no "./" prefix, so tryExactMatch and trySuffixMatch found nothing and a file
imported everywhere reported no importers at all.

The failure was silent: nothing marks an unresolved alias, so a whole Next.js
project read as standalone files with a confident answer.

Normalize the substituted target before matching. filepath.Join already
cleaned it whenever a baseUrl was set, which is why this only ever bit
projects without one, and why "@/*": ["*"] worked as a workaround.

The same normalization applies to the no-wildcard exact-alias branch, which
had the identical bug for targets like {"@app": ["./lib/a1"]}.

Relates to #173, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

* test(scanner): Compare fixture importers as a set

The graph appends importers in analysis order, which is not stable across
runs, so asserting a sequence made the alias fixture test fail on two CI
legs while passing locally. The exactness that matters here is membership:
neither a superset nor a subset of the expected importers.

The underlying nondeterminism is pre-existing and unrelated to the alias
fix; it is reported separately rather than papered over here.

Relates to #173, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

* fix(watch): Treat an unparseable readiness file as not-ready-yet

Ported from #177, which I opened for this failure. Carrying it here so
this PR has its best chance of going green rather than waiting on that one
to merge; it becomes a no-op once main has it.

waitWatchReadiness returned on the first successful read, so a readiness
file caught mid-write failed json.Unmarshal and aborted the wait
immediately. Measured against the real function, an empty file returns
"reading daemon readiness: unexpected end of JSON input" after 0s, which
is the string TestRunWatchStartWaitsForChildReadinessFailure reported on
this PR's ubuntu-1.24 leg.

I could not reproduce that CI failure locally, so this is the plausible
cause rather than a proven one. The change is correct on its own terms
either way: an unparseable file is a file still being written, not a
daemon that failed.

Relates to #173, #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

---------

Co-authored-by: Claude <noreply@anthropic.com>
JordanCoin added a commit that referenced this pull request Sep 4, 2026
…ot (#179)

* feat(scanner): Skip testdata directories by default

Fixture repositories under testdata are not the project's source. Scanning
them inflates file counts, and once a fixture is written in a language the
file-level edge model does not cover, it changes the project's reported
coverage: a three-file Swift fixture is enough to make an entire Go project
report partial.

Go's own toolchain ignores the directory for the same reason, so this
matches the convention users already expect, alongside vendor and
node_modules.

A fixture is still scannable when it is itself the scan root, which is how
fixture tests use them; only testdata encountered during a walk is skipped.

Relates to #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

* fix: Port the cargo-timeout and readiness fixes so CI can go green

Two failures on this PR, neither from the testdata exclusion — scanner
passed on both legs.

codemap/mcp: the cargo-metadata deadline failed the whole graph build
instead of falling back. Ported from @reneleonhardt's #171.

codemap root: TestRunWatchModeRunDaemonAndWatchStart hit "reading daemon
readiness: unexpected end of JSON input" — the same window #177 fixes, in
a different test than the one that led me to open it. waitWatchReadiness
gave up on the first unparseable read of a file still being written.

Both no-op once main carries them.

Relates to #172

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo

* fix(scanner): Never apply the ignore list to the scan root

IgnoredDirs is matched on a directory's base name, and the walk root is
matched along with everything under it. So `codemap testdata/`, or `codemap .`
from inside vendor/ or node_modules/, returned Files: 0 — no error, no
message, just an empty result for the directory the user explicitly named.
Adding testdata to the list widened the set of paths that hit this.

Exempt the walk root from the fast-path check in ScanFiles, and from the same
check in ReadExternalDeps so a manifest at that root is still read. Nested
directories with those names are still skipped, at any depth.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t

---------

Co-authored-by: Claude <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.

3 participants