Skip to content

test: make daemon disconnect checks independent of startup speed - #508

Merged
bompus merged 2 commits into
fork/consolidatedfrom
test/windows-lifecycle-ordering
Oct 15, 2026
Merged

bompus merged 2 commits into
fork/consolidatedfrom
test/windows-lifecycle-ordering

Conversation

@bompus

@bompus bompus commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

The early-disconnect regression test now reaches proxy attachment before its short idle deadline can fire, and prints daemon socket/client/shutdown state if the process stays alive.

A test-only preload skips the initial startup idle arm; the real timer still starts when the last registered client disconnects. The test retains its raw connection closing before client hello, eight-second exit requirement and writer-lock cleanup assertion. Its outer deadline now covers the sequential startup waits. The separate idle-exit tests retain their original startup behavior.

Real watcher-startup cases now have explicit 30-second outer deadlines rather than Vitest’s five-second default. These include initialization and child startup before the unchanged readiness deadlines. File-level temp cleanup reuses the existing bounded removal helper after test teardown. No production daemon behavior changes; the earlier Windows post-disconnect exit failure remains unexplained.

Validation: both changed suites passed 84/84 tests on Node 24/Linux after the final preload-path and deadline changes. TypeScript/assets/viewer build, fixture syntax, agent-doc guard and whitespace checks passed. The test floor guard flags the exit assertion’s move into a diagnostic variable; the same call, deadline and true assertion remain. The follow-up deadline correction passed all 60 runtime-control tests on Node 24/Linux. The first cross-platform CI run passed Linux and macOS. Windows passed the disconnect regression but failed two remaining watcher cases at Vitest’s 5,000 ms default (5,036 ms and 5,020 ms). Those cases and siblings with the same startup path now use the explicit outer deadline. A new Windows/macOS/Linux lifecycle run is required before landing.

README “About this fork” rows checked: tests only; no feature, language, runtime support or measured-number row changes. Related earlier lifecycle work: #398 and #399.

Summary by CodeRabbit

  • Tests
    • Added checks for daemon shutdown after a client disconnects, with diagnostic logging to help investigate failures.
    • Extended timeouts for daemon and runtime-control tests to accommodate longer-running test environments.
    • Temporary test directories are now cleaned up asynchronously after saved environment settings are restored.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository YAML (base), Organization UI (inherited)
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 51ae4033-9292-4b64-9178-c008fe38e3e9
📥 Commits

Reviewing files that changed from the base of the PR and between 039df77 and 2f1b42f.

📒 Files selected for processing: 1
  • __tests__/runtime-control.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

The test suite now awaits temporary-directory cleanup, adds daemon lifecycle instrumentation to the disconnect-before-client-hello test, and extends that test and six runtime-control test timeouts.

Changes: Test lifecycle and daemon checks

Layer / File(s) Summary
Instrument and verify daemon disconnect
__tests__/fixtures/daemon-disconnect-order.cjs, __tests__/mcp-daemon.test.ts
The test preloads a fixture that adjusts daemon idle-timer calls and logs lifecycle state. It checks that the daemon exits and retains the idle-timeout and writer-lock checks.
Update test cleanup and timeout handling
__tests__/file-tmpdir.setup.ts, __tests__/runtime-control.test.ts
The cleanup hook awaits temporary-directory removal. Six runtime-control tests use 30-second timeouts.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 2f1b4

This test-only change preserves the daemon shutdown checks, awaits temporary-directory cleanup, and keeps hangs and incorrect results failing within bounded deadlines. No concrete merge-blocking risk is established.

### Architecture Summary

Architecture risk: 🔵 Low · up to 2f1b4

The change affects 1 system.

Changed systems: __tests__

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — tests (service) was modified; 4 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in tests/file-tmpdir.setup.ts: Adds the rmTempDir import used by the cleanup hook.
  • observed — Modified behavior in tests/file-tmpdir.setup.ts: Changes afterAll to an async callback and replaces synchronous fs.rmSync cleanup with an awaited rmTempDir(fileDir) call; environment-variable restoration remains unchanged.
  • observed — Modified behavior in tests/fixtures/daemon-disconnect-order.cjs: When CODEGRAPH_DAEMON_INTERNAL is '1', the fixture patches Daemon.prototype.armIdleTimer to skip its first call per daemon and delegate later calls to the original method.
  • observed — Modified behavior in tests/fixtures/daemon-disconnect-order.cjs: Under the same environment condition, wrappers log daemon state after connections and client drops, log socket closure on the next immediate turn, and log before delegating to the original stop method.
Pre-merge checks: Passed 6
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Suppressions Explained Passed The pull request changes four test files only. The added lines contain no lint, type-check, compiler, or configuration suppression directives such as eslint-disable, @ts-expect-error, @ts-ignore, #[al…
User-Visible Changes Documented Passed The pull-request diff changes only four files under __tests__/. It adds test timing, cleanup, preload, and diagnostics logic. It does not add, remove, or rename a CLI command or flag, MCP tool or ar…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: making daemon disconnect lifecycle checks independent of startup speed. It is concise and specific.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 4 files

View guided diff | Turn on auto-fix | Re-trigger cubic

@bompus
bompus merged commit a5f1429 into fork/consolidated Oct 15, 2026
5 checks passed
@bompus
bompus deleted the test/windows-lifecycle-ordering branch October 15, 2026 22:37
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