Skip to content

test(daemon): import fresh daemon-init modules one at a time - #34085

Merged
nwparker merged 1 commit into
mainfrom
nwparker/daemon-init-fresh-import-order
Oct 26, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/daemon-init-fresh-import-order

Conversation

@nwparker

@nwparker nwparker commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 0 0 0 0
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​5 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1

ELI5

One daemon start-up unit test failed on every CI run that put it in test shard 4/5, and with today's file list that's every PR that doesn't add or remove a test file. This makes the test's setup import its modules in a fixed order, which makes it pass there.

What Changed

Before: importFreshDaemonInit (the shared setup for the daemon-init tests) called vi.resetModules() and then started four dynamic imports at once with Promise.all. On CI shard 4, "re-binds listeners after the first daemon provider is installed" failed 4 out of 4 times across two unrelated PRs (#33102, #33839): setLocalPtyProvider was called 0 times, even though init returned normally.

After: the four modules (daemon-provider-init, -restart, -state, daemon-protocol-cleanup) are imported one after another. Each one evaluates against the freshly reset, fully mocked module graph in a fixed order. Shard 4 passed on this branch (run 40404219946, job 121859327924, file confirmed in shard 4/5). The assertions are unchanged.

Why

  • Cause: a race in Vitest's mock registration. The first dynamic import runs resolveMocks(), which takes the queued vi.mock ids and clears the queue before it awaits their resolution. The module fetch only waits for mocks while that queue is non-empty. So when the imports run concurrently, the other three fetch their dependencies while the mocks are still unregistered. daemon-provider-state can then bind the real ../ipc/pty/provider/registry, so installDaemonProvider runs but the mock records 0 calls. Only the first test in the file is exposed, because the queue is only filled before the first import, and whether it fails depends on how slow that round trip is (CI shard load).
  • Why this approach: the first sequential import waits for every mock to be registered before any other module is fetched, so this removes the race rather than shifting timing. Moving the file into its own isolated run would only hide the symptom and add test infrastructure. The import graph has no cycles, so the order of exports and side effects is unchanged.
  • Local runs: I couldn't reproduce the CI symptom locally. On a heavily loaded machine the first cold import can take more than 30s and hit the test timeout in either order, which is a separate local artifact.

Linked Issue

N/A (main break: shard-4 unit failure blocking PRs).

Visual Proof

N/A: test-only change.

Testing

  • CI shard 4/5 on this branch: daemon-init-provider-installation.test.ts passes (16 tests).

  • pnpm tc:node and oxlint pass.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Test-only. There's no runtime, SSH, mobile or wire impact.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@nwparker nwparker changed the title test(daemon): import fresh daemon-init modules sequentially (draft, shard-4 investigation) [DO NOT MERGE] test(daemon): import fresh daemon-init modules sequentially (shard-4 investigation) Oct 26, 2026
@nwparker
nwparker marked this pull request as ready for review October 26, 2026 21:20
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 2 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7ecf878f-bd9b-490c-b7a7-49f9242708da
Commits

Reviewing files that changed from the base of the PR and between a93e31a and 15318e8.

Files selected for processing: 1
  • src/main/daemon/daemon-init-fresh-import.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nwparker
nwparker force-pushed the nwparker/daemon-init-fresh-import-order branch from df971b1 to ab8b570 Compare October 26, 2026 21:47
@nwparker nwparker changed the title [DO NOT MERGE] test(daemon): import fresh daemon-init modules sequentially (shard-4 investigation) test(daemon): import fresh daemon-init modules one at a time Oct 26, 2026
Parallel dynamic imports right after vi.resetModules made the cold first test in
daemon-init-provider-installation order-dependent: it failed on every CI run that
placed the file in unit shard 4.
@nwparker
nwparker force-pushed the nwparker/daemon-init-fresh-import-order branch from ab8b570 to 15318e8 Compare October 26, 2026 21:53
@nwparker
nwparker merged commit 6423536 into main Oct 26, 2026
40 checks passed
Jinwoo-H pushed a commit to thefrikidude/orca that referenced this pull request Oct 27, 2026
…blyai#34085)

Parallel dynamic imports right after vi.resetModules made the cold first test in
daemon-init-provider-installation order-dependent: it failed on every CI run that
placed the file in unit shard 4.
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.

1 participant