Skip to content

fix(mock): emit server event after app ready so onServer runs#5969

Open
elrrrrrrr wants to merge 1 commit into
nextfrom
fix/mock-emit-server-after-ready
Open

fix(mock): emit server event after app ready so onServer runs#5969
elrrrrrrr wants to merge 1 commit into
nextfrom
fix/mock-emit-server-after-ready

Conversation

@elrrrrrrr

@elrrrrrrr elrrrrrrr commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

What's the problem this PR solves?

In single-process mock mode (mm.app()), egg core's onServer() never runs, so the http server gets no clientError handler (and graceful shutdown / server timeout / websocket wiring is skipped too). Symptom: a malformed request that triggers clientError is not logged.

Root cause

@eggjs/mock creates the http server and emits server inside createServer(), which runs before await app.ready(). But egg core registers its server listener inside Application.load() (which runs during app.ready()), and onServer reads loaded config. So the event is emitted before the listener exists (and before config is loaded) → it's lost → onServer never runs.

egg3 (worked) — config loaded synchronously, listener registered in the constructor

new App() ─┬─ loader.load()  →  CONFIG loaded         ┐ all in the
           └─ bindEvents()   →  once('server') listener┘ constructor (sync)
mock.createServer() ──emit('server')──▶ onServer ✅  (config ready, listener ready)

egg4 (broken) — async loader moved listener registration into load()

new App()            → constructor does nothing
mock.createServer()  → emit('server') ✗ no listener yet, config not loaded  ← LOST
await app.ready()
   └─ super.load()   → CONFIG loaded + app.js/plugins register on('server')
   └─ bindEvents()   → registers once('server')  ← too late
=> onServer never runs ❌  (clientError logging etc. broken)

This PR — emit once, after ready

mock.createServer()  → only creates + caches the server, sets app.server (NO emit)
await app.ready()
   └─ super.load()   → CONFIG loaded + plugins register on('server')
   └─ bindEvents()   → registers once('server') + onServer
app.emit('server')   → the only emit → onServer ✅ + plugins ✅   (all fire exactly once)

The fix

Stop emitting server inside createServer() (it now only creates & caches the server, so app.server stays available during boot exactly as before), and emit it once, after await app.ready() in both the single (lib/app.ts) and parallel (lib/parallel/app.ts) app workers:

createServer(app);           // create + cache server only
await app.ready();
if (app.server) {
  app.emit('server', app.server);   // listener registered + config loaded → onServer runs
}

This:

  • emits server exactly once, after the listener is registered and config is loaded (no double-emit, no premature onServer);
  • keeps createServer before ready (so app.server is available during boot — the intentional ordering from More security tips #98 is preserved);
  • mirrors @eggjs/cluster, which also emits server after the app is ready (the behaviour More security tips #98 originally referenced).

Alternatives considered (and rejected)

  • Register the server listener in the constructor (restore egg3 timing): ❌ crashes — onServer reads this.config, which isn't loaded until app.ready(), so firing it at the pre-ready emit throws. This is exactly why egg4 moved the registration into load().
  • Move createServer() to after app.ready(): ❌ shifts app-boot microtask timing and turns unrelated boot-error fixtures (e.g. plugins/multipart, cluster-client-error) into unhandled errors → CI red. Keeping createServer before ready and only relocating the emit avoids this.
  • Re-emit server after ready (keep the early emit too): works, but emits twice; the early emit can double-fire server listeners registered before ready. Removing the early emit is cleaner.

Test

Added a regression guard in plugins/mock/test/app.test.ts: after app.ready(), assert app.server has a clientError listener (i.e. onServer ran). It fails before this change (count 0) and passes after.

Validation

Verified against a real egg4 framework (chair, @eggjs/mock@7.0.2-beta.9): a should log error when 400 test (raw malformed request → expect log) failed because clientError wasn't wired. With this change it passes deterministically.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 18, 2026 09:54
@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

createServer() no longer emits the 'server' event at creation time. Instead, both MockApplicationWorker._init and MockParallelApplication._init now emit the event after await app.ready() completes, ensuring Egg's listener (registered during load/ready) is in place before the event fires. A new regression test confirms the clientError listener is attached post-ready.

Changes

Server Event Emission Timing Fix

Layer / File(s) Summary
Remove server event from createServer() and document post-ready requirement
plugins/mock/src/lib/mock_http_server.ts
createServer() no longer calls app.emit('server', server) after creating the HTTP server. Comments explain that Egg registers the listener during Application.load()/app.ready(), so the event must be emitted by the caller after that point.
Add post-ready server event emission in both application variants
plugins/mock/src/lib/app.ts, plugins/mock/src/lib/parallel/app.ts
Both MockApplicationWorker._init and MockParallelApplication._init now conditionally emit 'server' after await app.ready() (when app.server exists), ensuring the listener registered by Egg during ready is in place before the event fires.
Regression test for listener attachment
plugins/mock/test/app.test.ts
New test case creates an app with cache: false, awaits ready(), asserts app.server is set, and confirms listenerCount('clientError') > 0 to verify the listener was successfully attached.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Suggested reviewers

  • killagu

Poem

🐇 The server event hops with care,
Not born when created in the air.
But waits for ready() to conclude,
So listeners are waxed and brewed.
A timing dance, both old and new,
This bunny's precision rings true! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main fix: emitting the server event after app ready so onServer runs, which directly addresses the root cause described in the PR objectives.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mock-emit-server-after-ready

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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 and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 18, 2026

Copy link
Copy Markdown

Deploying egg with  Cloudflare Pages  Cloudflare Pages

Latest commit: 464503c
Status: ✅  Deploy successful!
Preview URL: https://8f3a0a2a.egg-cci.pages.dev
Branch Preview URL: https://fix-mock-emit-server-after-r.egg-cci.pages.dev

View logs

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adjusts the initialization order in MockApplicationWorker to await app.ready() before invoking createServer(app). This ensures that the server event is not emitted before egg core registers its listener, preventing the loss of critical event bindings like clientError. A new test case has been added to verify that the clientError listener is successfully attached after initialization. No review comments were provided, so there is no additional feedback to address.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.53%. Comparing base (0333c73) to head (464503c).

Files with missing lines Patch % Lines
plugins/mock/src/lib/parallel/app.ts 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #5969      +/-   ##
==========================================
+ Coverage   85.50%   85.53%   +0.02%     
==========================================
  Files         669      669              
  Lines       19825    19828       +3     
  Branches     3917     3919       +2     
==========================================
+ Hits        16952    16960       +8     
+ Misses       2481     2477       -4     
+ Partials      392      391       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a lifecycle ordering bug in @eggjs/mock single-process mode where the mock HTTP server was created (and the server event emitted) before Egg core registers its once('server', ...) listener during app.ready(), causing Application.onServer() not to run and leaving clientError (and related server wiring) uninitialized.

Changes:

  • Move mock HTTP server creation (createServer(app)) to run after await app.ready() in single-process mock initialization.
  • Add a regression test asserting that clientError is wired on app.server after app.ready().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
plugins/mock/src/lib/app.ts Reorders initialization so createServer() runs after app.ready() to avoid losing the server event and ensure onServer() executes.
plugins/mock/test/app.test.ts Adds a regression test verifying app.server has a clientError listener after readiness.

@fengmk2

fengmk2 commented Jun 18, 2026

Copy link
Copy Markdown
Member

@codex review

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@plugins/mock/test/app.test.ts`:
- Around line 90-101: The test function creates an app and performs assertions,
but if any assertion fails, the app.close() cleanup call is skipped, causing
state leakage. Restructure the code by moving the await app.ready() call and all
assertions into a try block, then place await app.close() in a finally block to
ensure cleanup always executes regardless of assertion failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9fd68324-40d7-48ab-b479-260dbfa8188f

📥 Commits

Reviewing files that changed from the base of the PR and between 0333c73 and 68a72fd.

📒 Files selected for processing (2)
  • plugins/mock/src/lib/app.ts
  • plugins/mock/test/app.test.ts

Comment thread plugins/mock/test/app.test.ts
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 18, 2026

Copy link
Copy Markdown

Deploying egg-v3 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 464503c
Status: ✅  Deploy successful!
Preview URL: https://b3b2c844.egg-v3.pages.dev
Branch Preview URL: https://fix-mock-emit-server-after-r.egg-v3.pages.dev

View logs

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 68a72fdf13

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@elrrrrrrr elrrrrrrr force-pushed the fix/mock-emit-server-after-ready branch from 68a72fd to 0cc716b Compare June 18, 2026 10:26
@elrrrrrrr elrrrrrrr changed the title fix(mock): create mock http server after app ready so the server event isn't lost fix(mock): re-emit server event after app ready so onServer runs Jun 18, 2026
@elrrrrrrr elrrrrrrr force-pushed the fix/mock-emit-server-after-ready branch from 0cc716b to ec8ccfb Compare June 18, 2026 11:14
Copilot AI review requested due to automatic review settings June 18, 2026 11:14
@elrrrrrrr elrrrrrrr changed the title fix(mock): re-emit server event after app ready so onServer runs fix(mock): emit server event after app ready so onServer runs Jun 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment on lines +83 to +88
// @eggjs/mock emits the `server` event before `app.ready()`, while egg core
// registers its `once("server", ...)` listener inside Application.load()
// (during app.ready()). egg core re-emits `server` after the listener is
// registered so onServer still runs and wires up the `clientError` handler
// (plus graceful shutdown / server timeout / websocket). Regression guard:
// assert the clientError listener is wired after ready.
Comment thread plugins/mock/src/lib/parallel/app.ts
@eggjs/mock created the http server and emitted the `server` event inside
`createServer()`, which runs BEFORE `app.ready()`. But egg core registers its
`once('server', server => this.onServer(server))` listener inside
`Application.load()` (during `app.ready()`), and `onServer` reads loaded config.
So the event was emitted before the listener existed (and before config was
loaded), and `onServer` — which wires up clientError logging, graceful shutdown,
server timeout and websocket support — never ran in single-process mock mode.

Move the `server` emit out of `createServer()` (it now only creates & caches the
server, so `app.server` stays available during boot as before) and emit it once
after `await app.ready()` in both the single and parallel app workers. This
mirrors @eggjs/cluster, which also emits `server` after the app is ready, and
keeps the event emitted exactly once.

Added a regression test asserting `app.server` has a `clientError` listener
after ready.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@elrrrrrrr elrrrrrrr force-pushed the fix/mock-emit-server-after-ready branch from ec8ccfb to 464503c Compare June 18, 2026 11:18
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