Skip to content

fix(antigravity): implement configurable timeout and auto-reconnect for serve#859

Open
deepziyu wants to merge 4 commits intojackwener:mainfrom
deepziyu:fix/antigravity-timeout
Open

fix(antigravity): implement configurable timeout and auto-reconnect for serve#859
deepziyu wants to merge 4 commits intojackwener:mainfrom
deepziyu:fix/antigravity-timeout

Conversation

@deepziyu
Copy link
Copy Markdown
Contributor

@deepziyu deepziyu commented Apr 7, 2026

Resolves #861

Resolves the 120s hardcoded timeout issue and CDP connection exclusion problem in \�ntigravity serve\ command. Adds --timeout\ option and \OPENCLI_ANTIGRAVITY_TIMEOUT\ support. Implements auto-reconnect logic via \�nsureConnected\ during polling.

@Astro-Han
Copy link
Copy Markdown
Contributor

I think this PR is moving in the right direction overall. Making the antigravity serve timeout configurable and trying to recover from dropped CDP sessions both make sense for a long-running local proxy.

My main concern is the new reconnect path in clis/antigravity/serve.ts, inside the catch (err) block in waitForReply(), especially the if (opts.reconnect) { ... continue; } branch. Right now it treats any polling error from isGenerating(page) or getConversationText(page) as a reconnect case. That is broader than the problem this PR is trying to solve. A lost CDP session should reconnect, but a DOM change, selector break, or logic bug should still fail directly. Otherwise the original error gets hidden and the request eventually turns into a generic timeout.

So from a design point of view, I like the direction of configurable timeout plus reconnect, but I think the reconnect policy needs a tighter boundary. I would limit retry to explicit transport or session-loss errors and rethrow everything else.

@deepziyu
Copy link
Copy Markdown
Contributor Author

deepziyu commented Apr 7, 2026

Refined the reconnection logic in waitForReply as suggested:

  1. Narrowed error scope: The catch block now specifically filters for CDP transport or session loss errors (regex: /closed|lost|not open|websocket/i). Genuine logical or execution errors will be rethrown immediately to avoid masking bugs.
  2. Added retry limit: Reconnection is now capped at a maximum of 2 attempts (maxtime = 2) per polling session.
  3. Regression Check: Verified locally that 14 overall unit test failures match the 12 baseline failures plus environment-specific noise (consistent with main branch state on this machine).

@Astro-Han
Copy link
Copy Markdown
Contributor

I checked the latest head. The reconnect change looks much better now. Restricting retries to session-loss-style errors and rethrowing other failures addresses the concern I raised earlier about masking DOM or selector bugs behind a later timeout.

I still see one separate issue with the new timeout handling. In clis/antigravity/serve.ts, OPENCLI_ANTIGRAVITY_TIMEOUT is parsed with parseInt(...) * 1000, and in src/cli.ts the --timeout value is forwarded with parseInt(opts.timeout). Invalid values like abc turn into NaN, and negative values are accepted too. That can make the proxy fail immediately or behave oddly instead of falling back cleanly. Since src/runtime.ts already has validated timeout parsing, I think this path should follow the same pattern.

@deepziyu
Copy link
Copy Markdown
Contributor Author

deepziyu commented Apr 8, 2026

Thank you for the detailed feedback, @Astro-Han!

I have standardized the timeout parsing as suggested:

  1. Refactored Runtime Utility: Exported and generalized the parsing logic in \src/runtime.ts\ into a new \parseTimeoutValue\ function that handles \NaN, \

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.

Antigravity serve 120s Timeout & CDP Connection Instability

2 participants