icons::USE_NERD_FONTS is unguarded process-global render state — one future test away from nondeterministic snapshot failures

[JDonaghy]

src/icons.rs:12 holds render state in a process-global mutable:

static USE_NERD_FONTS: AtomicBool = AtomicBool::new(true);

pub fn set_nerd_fonts(val: bool) { USE_NERD_FONTS.store(val, Ordering::Relaxed); }

Icon::s() / Icon::c() read it on every call to choose between the nerd glyph and the ASCII fallback, so it directly determines rendered output — and rendered width, since the two variants are not the same width.

It is written from several production paths, all driven by settings.use_nerd_fonts:

• src/tui_main/mod.rs:675
• src/tui_main/shell_app.rs:187
• src/core/settings.rs:1087, :1599 (the :set nerdfonts / :set nonerdfonts command path)
• src/gtk/mod.rs:1395

Why this matters now

No test currently calls set_nerd_fonts, so this is latent, not live — verified during the #615 investigation. But a Rust test binary runs its tests in parallel threads in one process, so a single future test that flips this flag would non-deterministically corrupt every snapshot test running concurrently, with failures that depend on scheduling and therefore on core count. That is a nightmare to diagnose.

We just spent a multi-round investigation on #615, whose failure mode was exactly this shape: a render that depends on process-wide ambient state, passing everywhere locally and failing only in CI. #615 turned out to be a different mechanism (~/.config/vimcode/session.json leaking into Engine::new()), and USE_NERD_FONTS was audited and cleared as a candidate — but the audit is what surfaced it as a landmine.

Proposal

Move the flag off process-global state and onto something scoped — per-Engine, per-Backend, or thread-local. src/render.rs:788 already does b.set_nerd_fonts(engine.settings.use_nerd_fonts) on the backend, and src/render.rs:16653 has a comment asserting that resolving icons is "the backend's job (Backend::set_nerd_fonts)". So the intended design may already be backend-scoped and the global is vestigial — worth checking whether the icons:: global can simply be deleted in favour of the backend path.

If the global must stay for now, at minimum add a test-only guard (a mutex or #[serial]) so no test can flip it while snapshot tests run.

Not urgent

Nothing is broken today. This is a "fix it before it costs a week" issue — file it, size it, schedule it. Found during the #615 investigation.

[JDonaghy]

Coordinator Assignment

Machine: precision
Repo: vimcode
Files: (unspecified)
Do not touch: (nothing else in flight)

Briefing

Issue #618: icons::USE_NERD_FONTS is unguarded process-global render state — one future test away from nondeterministic snapshot failures

src/icons.rs:12 holds render state in a process-global mutable:

static USE_NERD_FONTS: AtomicBool = AtomicBool::new(true);

pub fn set_nerd_fonts(val: bool) { USE_NERD_FONTS.store(val, Ordering::Relaxed); }

Icon::s() / Icon::c() read it on every call to choose between the nerd glyph and the ASCII fallback, so it directly determines rendered output — and rendered width, since the two variants are not the same width.

It is written from several production paths, all driven by settings.use_nerd_fonts:

• src/tui_main/mod.rs:675
• src/tui_main/shell_app.rs:187
• src/core/settings.rs:1087, :1599 (the :set nerdfonts / :set nonerdfonts command path)
• src/gtk/mod.rs:1395

Why this matters now

No test currently calls set_nerd_fonts, so this is latent, not live — verified during the #615 investigation. But a Rust test binary runs its tests in parallel threads in one process, so a single future test that flips this flag would non-deterministically corrupt every snapshot test running concurrently, with failures that depend on scheduling and therefore on core count. That is a nightmare to diagnose.

We just spent a multi-round investigation on #615, whose failure mode was exactly this shape: a render that depends on process-wide ambient state, passing everywhere locally and failing only in CI. #615 turned out to be a different mechanism (~/.config/vimcode/session.json leaking into Engine::new()), and USE_NERD_FONTS was audited and cleared as a candidate — but the audit is what surfaced it as a landmine.

Proposal

Move the flag off process-global state and onto something scoped — per-Engine, per-Backend, or thread-local. src/render.rs:788 already does b.set_nerd_fonts(engine.settings.use_nerd_fonts) on the backend, and src/render.rs:16653 has a comment asserting that resolving icons is "the backend's job (Backend::set_nerd_fonts)". So the intended design may already be backend-scoped and the global is vestigial — worth checking whether the icons:: global can simply be deleted in favour of the backend path.

If the global must stay for now, at minimum add a test-only guard (a mutex or #[serial]) so no test can flip it while snapshot tests run.

Not urgent

Nothing is broken today. This is a "fix it before it costs a week" issue — file it, size it, schedule it. Found during the #615 investigation.

[JDonaghy]

Coordinator: Assignment Complete

Machine: precision
Status: done
Exit code: 0
Duration: 5m 58s
Log: /home/john/.coord/logs/d70dc9963c5b.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: precision
Status: done
Exit code: 0
Duration: 4m 41s
Log: /home/john/.coord/logs/16cd18854cdb.log

[JDonaghy]

Review Complete — ✅ Approved

Reviewer findings could not be posted directly to PR #644 (gh pr review was rejected — likely a self-review restriction). Findings are reproduced here.

Blocking findings

None.

Non-blocking concerns

• The whole fix rests on the invariant "the thread that calls set_nerd_fonts is always the same thread that later calls Icon::s()/Icon::c()." I verified this holds today (grepped every thread::spawn site in src/core/** — dap.rs, buffers.rs, ext_panel.rs, lsp_ops.rs, plugins.rs, search.rs, source_control.rs, lsp.rs, lsp_manager.rs — none of them touch icons::, Icon::, nerd_fonts_enabled(), or resolved_icon(); the one non-flag icon usage in ext_panel.rs:813 reads .nerd directly and bypasses the flag entirely). But this invariant is now a silent production correctness requirement, not just a test-isolation nicety, and nothing enforces it — a future background-thread render path would silently fall back to the thread's default (true) instead of the configured setting, with no panic/warning to catch it. Worth a one-line comment on set_nerd_fonts/nerd_fonts_enabled (already partially present) flagging this as a "must be called on the render thread" contract, or a debug-only assertion, so a future violation fails loudly rather than silently mis-rendering. Not blocking since the claim is correct as of this diff.
• The issue's "Proposal" section suggested checking whether the icons:: global could be deleted outright in favor of the already-existing backend-scoped Backend::set_nerd_fonts path (src/render.rs:788). This PR keeps the global (now thread-local) rather than investigating removal. That's an explicitly optional ask in the issue text, and the chosen fix is one of the three concrete options the issue itself proposed, so this isn't a scope violation — just noting the "can it be deleted" question is still open for a follow-up if desired.

Nits

• None.