fix: apply routing config on proxy reuse path (defense-in-depth)#149
fix: apply routing config on proxy reuse path (defense-in-depth)#149kagura-agent wants to merge 6 commits intoBlockRunAI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two control-plane endpoints ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller / startProxy
participant Proxy as Proxy Server
participant Router as In-memory Router Config
Caller->>Proxy: startProxy(port, routingConfig?)
alt proxy already running (reuse)
Caller->>Proxy: POST /__update-routing { routingConfig }
Proxy->>Proxy: parse JSON body
Proxy->>Router: mergeRoutingConfig(routingConfig) (mutate in-memory)
Proxy-->>Caller: 200 { updated: true } / 200 { updated: false } / 400 { error: ... }
else new proxy instance
Proxy->>Router: initialize routerOpts.config (defaults or provided)
Proxy-->>Caller: proxy started
end
Caller->>Proxy: GET /__routing-config
Proxy-->>Caller: current routerOpts.config (JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/proxy.ts (1)
1398-1416: Consider adding a timeout to the routing config update fetch.The fetch call to update routing config on the existing proxy has no timeout. If the proxy is in a degraded state, this could hang indefinitely, blocking the
startProxycall.♻️ Proposed fix to add timeout
// If a routing config was provided, push it to the running proxy so it takes effect if (options.routingConfig) { + const updateController = new AbortController(); + const updateTimeout = setTimeout(() => updateController.abort(), HEALTH_CHECK_TIMEOUT_MS); try { const updateRes = await fetch(`${baseUrl}/__update-routing`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ routingConfig: options.routingConfig }), + signal: updateController.signal, }); + clearTimeout(updateTimeout); if (!updateRes.ok) { console.warn( `[ClawRouter] Failed to update routing config on existing proxy: ${updateRes.status}`, ); } } catch (err) { + clearTimeout(updateTimeout); console.warn( `[ClawRouter] Could not update routing config on existing proxy: ${err instanceof Error ? err.message : String(err)}`, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 1398 - 1416, The fetch that posts to `${baseUrl}/__update-routing` when options.routingConfig is present can hang; wrap the request in an AbortController with a short configurable timeout (e.g., few seconds), pass controller.signal to fetch, and clear the timeout on completion; update the catch to handle aborts gracefully (recognize AbortError) while preserving the existing warning path for non-ok responses in the same block that uses updateRes, modifying only the request invocation and error handling around the fetch in the startProxy/routing update section that references options.routingConfig and baseUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/proxy.ts`:
- Around line 1568-1572: The handler currently always returns { updated: true }
even when no routing config was provided; change the logic around
body.routingConfig and mergeRoutingConfig so you only update routerOpts.config
and return { updated: true } when body.routingConfig is present and non-empty
(e.g., truthy and for objects check Object.keys(body.routingConfig).length > 0),
otherwise do not modify routerOpts.config and return a clear response such as
res.writeHead(200, ...) / res.end(JSON.stringify({ updated: false })) (or a 400
error if you prefer validation), making sure the code paths around
mergeRoutingConfig, routerOpts.config, res.writeHead and res.end reflect this
conditional behavior.
---
Nitpick comments:
In `@src/proxy.ts`:
- Around line 1398-1416: The fetch that posts to `${baseUrl}/__update-routing`
when options.routingConfig is present can hang; wrap the request in an
AbortController with a short configurable timeout (e.g., few seconds), pass
controller.signal to fetch, and clear the timeout on completion; update the
catch to handle aborts gracefully (recognize AbortError) while preserving the
existing warning path for non-ok responses in the same block that uses
updateRes, modifying only the request invocation and error handling around the
fetch in the startProxy/routing update section that references
options.routingConfig and baseUrl.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9bc2734a-c52e-48f0-b4cf-f37c0cf58683
📒 Files selected for processing (2)
src/proxy.routing-config-reuse.test.tssrc/proxy.ts
kagura-agent
force-pushed
the
fix/routing-config-ignored
branch
from
46c1b55 to
36ac46a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/proxy.ts (1)
1600-1618: Missing timeout on routing config update fetch.The fetch to
/__update-routinglacks a timeout. If the existing proxy is in a degraded state, this could hang indefinitely. Consider usingAbortSignal.timeout()similar to howcheckExistingProxyusesHEALTH_CHECK_TIMEOUT_MS.⏱️ Proposed fix to add timeout
// If a routing config was provided, push it to the running proxy so it takes effect if (options.routingConfig) { try { const updateRes = await fetch(`${baseUrl}/__update-routing`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ routingConfig: options.routingConfig }), + signal: AbortSignal.timeout(HEALTH_CHECK_TIMEOUT_MS), }); if (!updateRes.ok) { console.warn( `[ClawRouter] Failed to update routing config on existing proxy: ${updateRes.status}`, ); } } catch (err) { console.warn( `[ClawRouter] Could not update routing config on existing proxy: ${err instanceof Error ? err.message : String(err)}`, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 1600 - 1618, The fetch to update routing config can hang because it has no timeout; update the block that posts to `${baseUrl}/__update-routing` (the code using options.routingConfig and the updateRes handling) to use an AbortSignal with a timeout (reuse HEALTH_CHECK_TIMEOUT_MS and AbortSignal.timeout() like checkExistingProxy does), pass signal into fetch, and ensure you abort/handle the timeout error in the existing try/catch so the existing console.warn logs the error message when the request times out.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/proxy.ts`:
- Around line 1600-1618: The fetch to update routing config can hang because it
has no timeout; update the block that posts to `${baseUrl}/__update-routing`
(the code using options.routingConfig and the updateRes handling) to use an
AbortSignal with a timeout (reuse HEALTH_CHECK_TIMEOUT_MS and
AbortSignal.timeout() like checkExistingProxy does), pass signal into fetch, and
ensure you abort/handle the timeout error in the existing try/catch so the
existing console.warn logs the error message when the request times out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60d8008a-935c-44f5-82b6-b4b81a861ee0
📒 Files selected for processing (2)
src/proxy.routing-config-reuse.test.tssrc/proxy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/proxy.routing-config-reuse.test.ts
|
@coderabbitai Valid point — fixed in e6c4181. The endpoint now returns |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Thanks for digging into this! After looking closely I think this PR fixes a real bug, but it's a different bug than #147 — and the actual #147 case was just fixed via a different path in v0.12.142. What v0.12.142 fixes (the actual #147 case)In the single-process OpenClaw plugin scenario that @jeegankones reported, the second // src/index.ts (pre-v0.12.142)
if (proxyAlreadyStarted) {
api.logger.info("Proxy already started by earlier register() call — skipping");
return; // ← bails here, never reaches startProxy() / reuse path
}So v0.12.142 fixes that by deferring the first What this PR does fix
So this is still worth merging as defense-in-depth — but I'd suggest:
Want me to (a) merge as-is with an updated description, (b) wait for you to revise, or (c) close in favor of the v0.12.142 approach? Either way, thanks for the careful diff and the tests. |
|
Thanks for the thorough breakdown @1bcMax! You're right — the actual #147 flow never hits the reuse path, so the framing was off. I'll go with (b): revise the description to frame it as a defense-in-depth fix for the Will push the update shortly. |
When register() is called twice (common in OpenClaw), the second call with user config would hit the proxy reuse path and skip mergeRoutingConfig(). Custom routing config from openclaw.json was silently ignored. Fix: Add /__update-routing POST endpoint to the running proxy server. When the reuse path detects an existing proxy and options.routingConfig is provided, POST the config to update the running instance. Also adds /__routing-config GET endpoint for debugging.
kagura-agent
changed the title
kagura-agent
force-pushed
the
fix/routing-config-ignored
branch
from
dcd7fbc to
a343b8e
Compare
|
Fixed lint error — removed unused |
|
Closing — #147 was fully fixed in v0.12.142 via the deferred-register-startup approach in index.ts. The The hot-update endpoint here is interesting work and would be useful if we ever support live config reloading via a separate channel (e.g. CLI command), but the current scenarios are covered by the v0.12.142 approach. Happy to revisit if/when we have a use case for runtime routing-config updates. |
2 participants
Summary
Defense-in-depth fix for the
startProxy()reuse path: when a secondregister()call hits an existing proxy viacheckExistingProxy(),routingConfigwas silently dropped becausemergeRoutingConfigwas never called on that path.Note: This is a different bug than #147 (which was fixed in v0.12.142 via the single-process plugin path). This PR addresses the multi-process reuse scenario where
startProxy()returns an existing proxy URL.What changed
/__update-routinginternal endpoint on the proxy server — accepts POST with routing config and callsmergeRoutingConfigon the running instancestartProxy()reuse path sends routing config to the running proxy whenroutingConfigis present/__routing-configdebug endpoint for inspection{ updated: false }when noroutingConfigprovided (callers can distinguish no-op vs actual update)Limitation
Config applied this way is additive — it cannot remove overrides set by the first caller. This is inherent to the reuse model.
Tests