fix(task): add request timeout to devFetch so a stalled dev server rejects instead of hanging

[spokodev]

Fixes #4292

Problem

devFetch in src/task.ts (used by runTask() and listTasks()) wraps http.request() without a timeout. The _pidIsRunning guard only checks that the dev server process is alive via kill(pid, 0) — it does not verify the socket is accepting or answering requests. When the worker is alive but stalled (or restarting and not yet listening on the unix socket), the returned promise never settles, so nitro task run in a CI pipeline hangs until a job-level kill with no diagnostic.

Fix

Set the timeout option on http.request() (default 30s) and destroy the request with a descriptive error on the timeout event so the existing error → reject path surfaces it to the caller. TaskRunnerOptions gains an optional timeout so callers (e.g. CI scripts) can set a tighter limit.

Test

test/unit/task.test.ts spins an HTTP server on a unix socket that accepts connections but never responds, points nitro.dev.json at it with a live pid, and asserts listTasks({ cwd, timeout: 200 }) rejects with the timeout error. On main the promise never settles and the test fails by timing out; with the fix it rejects in ~200ms.

pnpm vitest run test/unit/ — the new test passes; the 4 pre-existing cloudflare preset failures reproduce identically on a clean checkout of main and are unrelated. pnpm fmt and pnpm typecheck clean.

[vercel]

@spokodev is attempting to deploy a commit to the Nitro Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds request timeout support to the internal devFetch helper to prevent indefinite hangs. A new timeout option is added to TaskRunnerOptions, wired through _getTasksContext into http.request options, and a timeout handler destroys stalled connections with a meaningful error. A unit test validates the timeout behavior.

Changes

Request timeout support for devFetch

Layer / File(s)	Summary
Timeout configuration contract src/types/runtime/task.ts	TaskRunnerOptions now includes an optional timeout property (in milliseconds, default 30,000), documenting the dev-server request timeout for users to customize.
Timeout implementation in devFetch src/task.ts	_getTasksContext derives a timeout from opts with a 30,000 ms default, passes it to http.request options, and registers a request.on("timeout") handler that destroys the connection with an error to prevent indefinite waits on stalled sockets.
Timeout test validation test/unit/task.test.ts	Unit test verifies that listTasks with a configured timeout (200 ms) rejects with a timeout error instead of hanging when the dev server socket is stalled, including cleanup of the temporary test directory and server.

---

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

Possibly related PRs

• nitrojs/nitro#3268: Introduced the bare http.request devFetch implementation in src/task.ts; this PR adds the timeout handling that was missing from that initial refactor.

🚥 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 PR title follows conventional commits format with 'fix(task):' prefix and clearly describes the main change: adding timeout support to prevent stalled servers from hanging.
Description check	✅ Passed	The PR description is comprehensive and directly related to the changeset, detailing the problem, fix, and test approach with reference to issue #4292.
Linked Issues check	✅ Passed	The PR implementation fully addresses all objectives from issue #4292: adds timeout to devFetch, defaults to 30s, exposes TaskRunnerOptions.timeout for override, covers runTask/listTasks, and includes a test demonstrating timeout rejection.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the linked issue #4292: timeout implementation in task.ts, type definition in types/runtime/task.ts, and a focused unit test in test/unit/task.test.ts with no unrelated modifications.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

test/unit/task.test.ts (1)

17-39: 💤 Low value

Consider reversing cleanup order for robustness.

The test correctly validates the timeout behavior. However, the cleanup order could be improved: currently rm runs before server.close(), but it's safer to close the server before removing the directory containing the socket file. While force: true handles busy files on most platforms, explicitly closing the server first is clearer and more robust.

♻️ Suggested cleanup order

   const cwd = await mkdtemp(join(tmpdir(), "nitro-task-test-"));
-  cleanups.push(() => rm(cwd, { recursive: true, force: true }));

   // A worker socket that accepts connections but never responds, like a
   // stalled dev server whose pid is still alive.
   const socketPath = join(cwd, "worker.sock");
   const server = http.createServer(() => {});
   cleanups.push(() => new Promise((resolve) => server.close(() => resolve())));
+  cleanups.push(() => rm(cwd, { recursive: true, force: true }));
   await new Promise<void>((resolve) => server.listen(socketPath, resolve));

🤖 Prompt for 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.

In `@test/unit/task.test.ts` around lines 17 - 39, The cleanup order should close
the test HTTP server before removing the temporary directory to avoid deleting
the socket file while it's still in use; modify the cleanup registrations around
the created server and rm so that the server.close cleanup (using
server.close()) is pushed/registered before the rm cleanup (the rm(cwd, {
recursive: true, force: true }) call), or combine them into a single cleanup
that first awaits server.close() then calls rm, ensuring the server is closed
prior to directory removal; update references in the test to use the cleanups
array, server, server.close, and rm accordingly.

🤖 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.

Nitpick comments:
In `@test/unit/task.test.ts`:
- Around line 17-39: The cleanup order should close the test HTTP server before
removing the temporary directory to avoid deleting the socket file while it's
still in use; modify the cleanup registrations around the created server and rm
so that the server.close cleanup (using server.close()) is pushed/registered
before the rm cleanup (the rm(cwd, { recursive: true, force: true }) call), or
combine them into a single cleanup that first awaits server.close() then calls
rm, ensuring the server is closed prior to directory removal; update references
in the test to use the cleanups array, server, server.close, and rm accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ab738c7d-2216-45f6-a2db-c277adb851ae

📥 Commits

Reviewing files that changed from the base of the PR and between 7765bcb and 34a6ea1.

📒 Files selected for processing (3)

• src/task.ts
• src/types/runtime/task.ts
• test/unit/task.test.ts