Fix: preserve modelOverride in agent handler (#5369) by CodeReclaimers · Pull Request #8398 · openclaw/openclaw

CodeReclaimers · 2026-02-04T00:30:25Z

Summary

Fixes the intermittent race condition where sub-agent model overrides were silently ignored (~3% failure rate).

When sessions_spawn sets a model override via sessions.patch, the agent handler could read stale cached data and overwrite the fresh modelOverride when writing back to the session store
The fix moves session entry construction inside the updateSessionStore mutator, ensuring it always uses fresh store data (loaded with skipCache: true)

Root Cause

The agent handler was:

Reading session entry early via loadSessionEntry() (could return cached/stale data)
Building nextEntry with modelOverride: entry?.modelOverride (potentially undefined)
Calling updateSessionStore() which correctly re-reads with skipCache: true
But the mutator ignored the fresh store data and just wrote store[key] = nextEntry

This meant freshly-written modelOverride values from sessions.patch could be overwritten.

Changes

src/gateway/server-methods/agent.ts: Build session entry inside updateSessionStore using fresh store data
src/gateway/server-methods/agent.test.ts: Add 4 comprehensive tests for the fix

Test plan

Existing tests updated and passing
New test: issue #5369: agent handler preserves fresh modelOverride from store
New test: issue #5369: request params override fresh store values for label/spawnedBy
New test: issue #5369: creates new entry when store has no existing entry
New test: issue #5369: preserves all important fields from fresh store
Full test suite passes (5,043 tests)

Fixes #5369

🤖 Generated with Claude Code

Greptile Overview

Greptile Summary

Fixes a race where the agent handler could overwrite freshly-patched session fields (notably modelOverride) by moving session entry construction into the updateSessionStore mutator so it always uses the fresh store snapshot.

Adds targeted tests in src/gateway/server-methods/agent.test.ts to simulate stale loadSessionEntry() data vs fresh store data and assert that request params override where intended while preserving store-only fields.

Confidence Score: 4/5

This PR is largely safe to merge; the core race fix is sound and well-covered by tests.
The change localizes session entry construction to the fresh store snapshot inside updateSessionStore, which directly addresses the described overwrite race. Main remaining concern is that the send-policy precheck still relies on a potentially stale cached entry, which could be incorrect if sessions.patch updates policy-relevant fields. Tests are deterministic and cover the modelOverride scenario well.
src/gateway/server-methods/agent.ts (send-policy check freshness); src/gateway/server-methods/agent.test.ts (timer cleanup robustness).

_{(2/5) Greptile learns from your feedback when you react with thumbs up/down!}

When spawning sub-agents via sessions_spawn, the model parameter was intermittently ignored due to a race condition. The agent handler would read session data early (potentially from cache), build nextEntry, then overwrite the session store - clobbering any modelOverride that sessions.patch had just written. The fix moves session entry construction inside the updateSessionStore mutator, ensuring it always uses fresh store data (loaded with skipCache: true). This eliminates the race condition entirely. Changes: - Build session entry inside updateSessionStore using fresh store data - Add fallback path for when storePath is not available - Add comprehensive tests for the fix and edge cases Fixes openclaw#5369 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

greptile-apps

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-04T00:31:41Z

Additional Comments (2)

src/gateway/server-methods/agent.ts
[P1] Send-policy check can still use stale cached entry

The pre-flight resolveSendPolicy({ entry, channel: entry?.channel, chatType: entry?.chatType }) at agent.ts:238-245 still uses the potentially stale loadSessionEntry() result. If sessions.patch updates policy-relevant fields (e.g., sendPolicy, channel, chatType) between the cached read and the updateSessionStore(skipCache: true) write, this handler can incorrectly allow/deny sends based on stale data even though the store has a newer policy. Consider resolving policy from the fresh store entry inside updateSessionStore (or doing a fresh read for policy) when storePath is present.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/gateway/server-methods/agent.ts
Line: 238:245

Comment:
[P1] **Send-policy check can still use stale cached entry**

The pre-flight `resolveSendPolicy({ entry, channel: entry?.channel, chatType: entry?.chatType })` at `agent.ts:238-245` still uses the potentially stale `loadSessionEntry()` result. If `sessions.patch` updates policy-relevant fields (e.g., `sendPolicy`, `channel`, `chatType`) between the cached read and the `updateSessionStore(skipCache: true)` write, this handler can incorrectly allow/deny sends based on stale data even though the store has a newer policy. Consider resolving policy from the fresh store entry inside `updateSessionStore` (or doing a fresh read for policy) when `storePath` is present.

How can I resolve this? If you propose a fix, please make it concise.

src/gateway/server-methods/agent.test.ts
[P3] Fake timers aren’t restored if the test fails early

In agent.test.ts:380-382, vi.useRealTimers() is called at the end of the test body. If an assertion throws before that line, subsequent tests can run under fake timers. Using a try/finally (or afterEach(() => vi.useRealTimers())) would make the suite more robust.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/gateway/server-methods/agent.test.ts
Line: 380:382

Comment:
[P3] **Fake timers aren’t restored if the test fails early**

In `agent.test.ts:380-382`, `vi.useRealTimers()` is called at the end of the test body. If an assertion throws before that line, subsequent tests can run under fake timers. Using a `try/finally` (or `afterEach(() => vi.useRealTimers())`) would make the suite more robust.

How can I resolve this? If you propose a fix, please make it concise.

P1: Move send policy check inside updateSessionStore mutator to use fresh store data, avoiding potential stale policy decisions when sessions.patch updates the entry between initial read and write. P3: Add afterEach hook to restore real timers in agent tests, ensuring cleanup even if tests fail mid-execution with fake timers enabled. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

CodeReclaimers · 2026-02-04T00:52:23Z

Follow-up: Addressed greptile-apps review comments

Pushed commit 1d222a14a with the following fixes:

P1: Fresh data for send policy check

Moved the resolveSendPolicy() check inside the updateSessionStore mutator to use fresh store data instead of potentially stale cached entry. This prevents a similar race condition where sessions.patch could update the send policy between initial read and write.

The mutator now returns a discriminated union { denied: true } or { denied: false, entry } to handle the policy check result while maintaining type safety.

P3: Fake timers cleanup

Added afterEach(() => vi.useRealTimers()) to the test describe block to ensure fake timers are always restored, even if a test fails mid-execution with fake timers enabled. This is a best practice to prevent test pollution.

All 5,043 tests pass with these changes.

Resolve conflicts integrating main's legacy key pruning and new features with the openclaw#5369 race condition fix that moves session entry construction inside the updateSessionStore mutator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

openclaw-barnacle · 2026-02-17T17:44:48Z