fix(serve): allow server config with stop method to auto-start#26748
fix(serve): allow server config with stop method to auto-start#26748
Conversation
|
Updated 11:53 PM PT - Feb 4th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26748That installs a local version of the PR into your bun-26748 --bun |
WalkthroughChanged server-config detection to check for the presence of Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/regression/issue/26747.test.ts`:
- Around line 35-36: The test currently uses a hard setTimeout (const timeout =
setTimeout(() => proc.kill(), 3000)) to terminate the server; replace this with
an await-based approach that waits for the expected startup condition/output and
then closes the process (e.g., await the success log or use a Promise.race with
a deadline promise) or, if the timeout is truly required, add an explicit
comment explaining why a fixed timeout and proc.kill() are necessary; update
references to timeout and proc.kill() accordingly and ensure any created timer
is cleared when the process exits.
- Around line 10-55: The first test's Bun.spawn instance (proc) is cleaned up
manually with try/finally and proc.kill(); change it to use the same RAII
pattern as the other test by declaring the spawned process with "await using
proc = Bun.spawn(...)" so Bun will auto-cleanup; move the timeout setup/clear
inside that scope (or use a finally to clear it) and remove the manual
proc.kill() in the finally block so the test uses automatic disposal for the
proc created by Bun.spawn.
test/regression/issue/26747.test.ts
Outdated
| // Set a timeout to kill the server after checking output | ||
| const timeout = setTimeout(() => proc.kill(), 3000); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid setTimeout in tests; prefer awaiting a condition.
As per coding guidelines, tests should not use setTimeout. Instead, consider using a pattern that awaits the expected condition or uses a deadline-based approach.
However, given that this test needs to verify server startup and then terminate a long-running process, the current approach may be acceptable. Consider documenting why the timeout is necessary here.
Alternative approach using race pattern
- // Set a timeout to kill the server after checking output
- const timeout = setTimeout(() => proc.kill(), 3000);
-
try {
// Wait for first bit of stdout to verify server started
const reader = proc.stdout.getReader();
- const { value } = await reader.read();
+ const { value } = await Promise.race([
+ reader.read(),
+ new Promise((_, reject) =>
+ Bun.sleep(3000).then(() => reject(new Error("Timeout waiting for server start")))
+ ),
+ ]);
reader.releaseLock();🤖 Prompt for AI Agents
In `@test/regression/issue/26747.test.ts` around lines 35 - 36, The test currently
uses a hard setTimeout (const timeout = setTimeout(() => proc.kill(), 3000)) to
terminate the server; replace this with an await-based approach that waits for
the expected startup condition/output and then closes the process (e.g., await
the success log or use a Promise.race with a deadline promise) or, if the
timeout is truly required, add an explicit comment explaining why a fixed
timeout and proc.kill() are necessary; update references to timeout and
proc.kill() accordingly and ensure any created timer is cleared when the process
exits.
test/regression/issue/26747.test.ts
Outdated
| }); | ||
|
|
||
| // Set a timeout to kill the server after checking output | ||
| const timeout = setTimeout(() => proc.kill(), 3000); |
There was a problem hiding this comment.
CLAUDE.md violation: Tests should not use setTimeout.
From CLAUDE.md:
CRITICAL: Do not write flaky tests. Do not use
setTimeoutin tests. Instead,awaitthe condition to be met.
Instead of using a timeout to kill the server, consider awaiting a specific condition (e.g., waiting for the first stdout chunk with a deadline pattern using Promise.race).
See also:
Lines 118 to 120 in 2b9f0c0
test/regression/issue/26747.test.ts
Outdated
| `, | ||
| }); | ||
|
|
||
| const proc = Bun.spawn({ |
There was a problem hiding this comment.
CLAUDE.md violation: Prefer await using for spawned processes to ensure proper cleanup.
From test/CLAUDE.md:
Generally,
await usingorusingis a good idea to ensure proper resource cleanup. This works in most Bun APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc.
Note: The second test on line 74 correctly uses await using proc.
| const proc = Bun.spawn({ | |
| await using proc = Bun.spawn({ |
2b9f0c0 to
7b144a2
Compare
|
Updated the test to address review feedback:
Both tests now use consistent RAII patterns for process cleanup. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26747.test.ts`:
- Around line 25-31: The comment above the setTimeout block is inaccurate: it
says "unref it so the process can exit" but the code calls process.exit(0)
inside setTimeout; update the comment to accurately describe that the test
forces process termination after 100ms rather than using server.unref(),
referencing the setTimeout(() => { process.exit(0); }, 100) snippet so future
readers know the process is being forcibly exited.
test/regression/issue/26747.test.ts
Outdated
| // This allows us to check the startup message without the server blocking forever | ||
| setTimeout(() => { | ||
| // The server is already started by the entry point wrapper | ||
| // We can safely exit after checking | ||
| process.exit(0); | ||
| }, 100); |
There was a problem hiding this comment.
CLAUDE.md violation: This test uses setTimeout in the fixture code, which violates the CLAUDE.md rule:
CRITICAL: Do not write flaky tests. Do not use
setTimeoutin tests. Instead,awaitthe condition to be met.
The 100ms delay creates a timing-dependent test that could be flaky. Instead, consider having the test:
- Read stdout incrementally and look for "Started"
- Kill the process once the expected output is found
This avoids the arbitrary timing assumption. Example approach:
| // This allows us to check the startup message without the server blocking forever | |
| setTimeout(() => { | |
| // The server is already started by the entry point wrapper | |
| // We can safely exit after checking | |
| process.exit(0); | |
| }, 100); | |
| // After the server starts, unref it so the process can exit | |
| // This allows us to check the startup message without the server blocking forever | |
| process.exit(0); // Exit immediately after startup | |
| `, |
Or, modify the test to stream stdout and kill the process once "Started" is detected rather than relying on the fixture to exit itself.
The fix for #26142 incorrectly used `typeof def.stop !== 'function'` to detect Server instances returned by Bun.serve(). This caused server config objects with a custom `stop` method (e.g., Elysia apps) to not auto-start. Changed the detection to use `typeof def.reload !== 'function'` instead, since `reload` is specific to Server instances and unlikely to be on user config objects. Fixes #26747 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7b144a2 to
95c759a
Compare
Summary
stopmethod would not auto-starttypeof def.stop !== 'function'totypeof def.reload !== 'function'The previous fix for #26142 used the presence of a
stopmethod to detect Server instances returned byBun.serve(). However, user config objects can legitimately have astopmethod (e.g., Elysia apps), causing them to not auto-start.Server instances have a
reloadmethod that config objects typically don't have, making it a better detection heuristic.Test plan
test/regression/issue/26747.test.tstest/regression/issue/26142.test.tsstill passesFixes #26747
🤖 Generated with Claude Code