fix(gateway): add local dispatch for cron tool calls#7543
fix(gateway): add local dispatch for cron tool calls#7543hclsys wants to merge 3 commits intoopenclaw:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const localDispatcher = opts.allowLocal ? getLocalGatewayDispatcher() : null; | ||
| const canDispatchLocally = | ||
| Boolean(localDispatcher) && !opts.config && !urlOverride && !remoteUrl && !isRemoteMode; | ||
| if (canDispatchLocally) { |
There was a problem hiding this comment.
[P1] Local dispatch is bypassed whenever opts.config is provided, which can accidentally reintroduce WS self-contention
canDispatchLocally requires !opts.config (src/gateway/call.ts:193-196). Callers that pass a config object explicitly (common in tests/embedders) will never take the local dispatcher path even if allowLocal: true, no URL override, and gateway is local. That makes allowLocal behavior depend on an unrelated calling detail and can break the intended optimization/fix in those contexts. If the intention is “don’t dispatch locally when we’re overriding config loading”, consider checking a narrower condition (e.g., whether URL/remote mode is forced) rather than opts.config presence.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/call.ts
Line: 193:196
Comment:
[P1] Local dispatch is bypassed whenever `opts.config` is provided, which can accidentally reintroduce WS self-contention
`canDispatchLocally` requires `!opts.config` (`src/gateway/call.ts:193-196`). Callers that pass a config object explicitly (common in tests/embedders) will *never* take the local dispatcher path even if `allowLocal: true`, no URL override, and gateway is local. That makes `allowLocal` behavior depend on an unrelated calling detail and can break the intended optimization/fix in those contexts. If the intention is “don’t dispatch locally when we’re overriding config loading”, consider checking a narrower condition (e.g., whether URL/remote mode is forced) rather than `opts.config` presence.
How can I resolve this? If you propose a fix, please make it concise.
src/gateway/server-ws-runtime.ts
Outdated
| setLocalGatewayDispatcher(async (opts) => { | ||
| const req = { | ||
| type: "req", |
There was a problem hiding this comment.
[P1] Local dispatcher is never cleared, so it can retain stale handlers/context across server lifecycle
attachGatewayWsHandlers unconditionally calls setLocalGatewayDispatcher(...) (src/gateway/server-ws-runtime.ts:55-57) but there’s no corresponding cleanup when the WS server shuts down or handlers are detached. In processes that restart/recreate the gateway server in-process (tests, hot-reload, multi-instance), this global singleton can keep pointing at old params.context/extraHandlers, leading to requests being handled by the wrong instance or using freed resources.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-ws-runtime.ts
Line: 55:57
Comment:
[P1] Local dispatcher is never cleared, so it can retain stale handlers/context across server lifecycle
`attachGatewayWsHandlers` unconditionally calls `setLocalGatewayDispatcher(...)` (`src/gateway/server-ws-runtime.ts:55-57`) but there’s no corresponding cleanup when the WS server shuts down or handlers are detached. In processes that restart/recreate the gateway server in-process (tests, hot-reload, multi-instance), this global singleton can keep pointing at old `params.context`/`extraHandlers`, leading to requests being handled by the wrong instance or using freed resources.
How can I resolve this? If you propose a fix, please make it concise.| return (await Promise.race([ | ||
| dispatchPromise, | ||
| new Promise<never>((_, reject) => | ||
| setTimeout(() => reject(new Error(formatTimeoutError())), timeoutMs), | ||
| ), | ||
| ])) as T; |
There was a problem hiding this comment.
[P2] Local-dispatch timeout uses setTimeout without cancellation, which can keep timers alive unnecessarily
In the local dispatcher path you race against new Promise(... setTimeout ...) (src/gateway/call.ts:211-216), but there’s no clearTimeout when dispatchPromise wins. This isn’t functionally incorrect, but it leaves an extra timer scheduled per call, which can add noise/overhead in high-frequency cron usage. Consider storing the timer id and clearing it in a .finally/after Promise.race settles.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/call.ts
Line: 211:216
Comment:
[P2] Local-dispatch timeout uses `setTimeout` without cancellation, which can keep timers alive unnecessarily
In the local dispatcher path you race against `new Promise(... setTimeout ...)` (`src/gateway/call.ts:211-216`), but there’s no `clearTimeout` when `dispatchPromise` wins. This isn’t functionally incorrect, but it leaves an extra timer scheduled per call, which can add noise/overhead in high-frequency cron usage. Consider storing the timer id and clearing it in a `.finally`/after `Promise.race` settles.
How can I resolve this? If you propose a fix, please make it concise.
openclaw-barnacle
bot
added
gateway
Address TypeScript error TS2322: Type 'string' is not assignable to type '"req"'. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Closing — this was superseded by upstream changes. Happy to resubmit a focused fix if the issue resurfaces. |
2 participants
Fixes #6508\n\n- add in-process gateway dispatcher for allowed local tool calls\n- use allowLocal for cron tool gateway calls to avoid WS self-contention\n- add local dispatch test coverage\n\n## Test plan\n- pnpm vitest src/gateway/call.test.ts\n- pnpm vitest src/agents/tools/cron-tool.test.ts\n- pnpm format\n
Greptile Overview
Greptile Summary
This PR adds an in-process “local dispatcher” path for gateway tool calls so certain calls (notably cron) can be handled without looping back through the gateway’s own WebSocket client, avoiding self-contention. Concretely:
callGatewaynow optionally short-circuits to a registered local dispatcher whenallowLocalis enabled and the gateway target is local.handleGatewayRequestdirectly.allowLocal: true, and tests cover the local-dispatch path.Overall this fits cleanly into existing gateway call infrastructure by keeping the dispatcher optional and controlled by
allowLocalwhile preserving existing URL/config resolution for remote/override scenarios.Confidence Score: 3/5
opts.config, which can makeallowLocalunreliable depending on how call sites are structured.Context used:
dashboard- CLAUDE.md (source)dashboard- AGENTS.md (source)