fix: settle startup failures and clean up owned server resources - #5727
fix: settle startup failures and clean up owned server resources#5727OskarEichler wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 78371cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. WalkthroughThe change updates Merge Risk: 🟡 Moderate · up to The PR improves startup error handling and resource cleanup, but direct startup and shutdown can still race so a listener may become active after shutdown reports completion, while some failed setup paths may not clean up fully and plugin bind failures may not retry. This is a bounded lifecycle correctness risk that should be explicitly reviewed or addressed before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35f010e7-3a6a-46a2-a936-6042883334c7
📒 Files selected for processing (3)
.changeset/server-startup-cleanup.mdlib/Server.jslib/servers/WebsocketServer.js
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e4d39fa-1683-45cb-a9ad-3187eb2b9e63
📒 Files selected for processing (2)
lib/Server.jstest/e2e/api.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Fixes
Compatibility
No API/engine/dependency change. Startup errors now reach awaited start()/callback error handling; callers must handle rejection. Shutdown may wait for already-started initialization before resolving. Unrelated HTTP listeners remain untouched. Bonjour log text reflects explicit protocol selection.
Verification
Real loopback HTTP/WebSocket checks cover owned listener registration/removal, shared/separate server modes and connections. Bounded occupied TCP/IPC controls verify promise rejection without uncaught exceptions. Real webpack compiler hooks and a real pending HTTP listen verify startup/shutdown ordering and closed sockets. All 17 existing plugin API tests pass in the frozen full run. Combined server controls: 14 pass; middleware/reporting/startup controls: 15 pass. Build and all lint/type checks pass.
Audit scope
This is a focused, independently based change from a broader source review at f804962. The combined frozen-source run executed 1,002 tests: 951 passed, 43 failed, one cancelled, seven skipped. Failures were traced to the separately proposed overlay DOM snapshots, reconnect-disabled test expectation and IPv6 host-test assumptions; it is not represented as a green full suite. Relevant focused results are listed above. No checked-in tests/specs/snapshots were added or modified. Hosted CI and the full OS/Node matrix remain pending.
Summary by CodeRabbit