Skip to content

Fix: Make dev-proxy build timeout configurable and distinct from failures - #474

Closed
abhijeetnardele24-hash wants to merge 2 commits into
debugmcp:mainfrom
abhijeetnardele24-hash:fix/dev-proxy-timeout
Closed

Fix: Make dev-proxy build timeout configurable and distinct from failures#474
abhijeetnardele24-hash wants to merge 2 commits into
debugmcp:mainfrom
abhijeetnardele24-hash:fix/dev-proxy-timeout

Conversation

@abhijeetnardele24-hash

Copy link
Copy Markdown
Contributor

Closes #472. Adds DEV_PROXY_BUILD_TIMEOUT_MS and distinguishes SIGTERM/killed timeouts from real build failures in dev-proxy.

@debugmcpdev debugmcpdev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks — the core change here is good and matches what #472 asked for: DEV_PROXY_BUILD_TIMEOUT_MS alongside the other proxy env knobs, and err.killed || err.signal === 'SIGTERM' to stop reporting a timed-out build as Build failed:. The timeout message text ("the build may still have succeeded, re-run manually to confirm") is exactly the guidance an agent needs at that moment. I'd like to merge this once the branch is cleaned up:

Must fix

  1. Rebase so this PR contains only the dev-proxy change. The branch currently stacks on your #473 commit (that issue is now fixed via #476, so those files will conflict) and carries the regenerated pnpm-lock.yaml — which removes the repo's security overrides block (vite/qs/esbuild/hono/… floors) and downgrades esbuild — plus placeholder text in pnpm-workspace.yaml. Neither file should appear in the diff at all:
    git fetch upstream main
    git checkout -b fix/dev-proxy-timeout-v2 upstream/main
    git checkout fix/dev-proxy-timeout -- tools/dev-proxy/dev-proxy.mjs
    # then re-apply only the timeout hunks if the file picked up other edits
    
    (Or interactively rebase and drop everything except the tools/dev-proxy/dev-proxy.mjs hunks.)
  2. Restore the deleted comment block above the catch (the one explaining why stderr is sanitized, referencing issue #154). It documents a real constraint — raw build stderr must not reach tool responses — and the constraint still applies to both of your new error paths.

Should fix

  1. Guard against a malformed env var: parseInt(...) yields NaN for e.g. DEV_PROXY_BUILD_TIMEOUT_MS=abc, and execSync treats a NaN timeout as no timeout. Something like:
    const parsed = parseInt(process.env.DEV_PROXY_BUILD_TIMEOUT_MS || '', 10);
    const BUILD_TIMEOUT_MS = Number.isFinite(parsed) && parsed > 0 ? parsed : 120000;
  2. Document the new knob in the dev-proxy Configuration list in CLAUDE.md (it enumerates DEV_PROXY_PORT, DEV_PROXY_BUILD_CMD, …).

Optional#472 also suggested raising the default when the build command is a docker build; fine to leave that out and let docker users set the env var, as long as the knob is documented.

No tests exist for tools/dev-proxy/ today, so I won't ask for one — I'll verify the timeout path manually before merge. Happy to answer questions; once the branch is just the dev-proxy diff this should go in quickly.

@debugmcpdev

Copy link
Copy Markdown
Collaborator

Closing in favor of #487, per your own note there — thanks for cutting the clean unstacked branch. One heads-up for future branches: this one's pnpm-lock.yaml was regenerated by a different pnpm major, which dropped the security overrides block (and pnpm-workspace.yaml picked up a placeholder allowBuilds entry). #479 pins packageManager to pnpm@10.33.0 so after it lands, pnpm install will use the right version automatically via corepack.

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.

dev-proxy: hardcoded 120s build timeout makes docker rebuilds report a successful build as 'Build failed'

2 participants