Repository navigation
👻 fix: Guard Redis Stream Resume Against Disposed HITL Graph - #14258
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR hardens the Redis Streams resume path for HITL pauses (notably ask_user_question / tool approvals) by preventing crashes when a locally cached StandardGraph has been disposed, and by ensuring reconstructed resume content includes the correct ask_user_question args.
Changes:
- Add a defensive “read cached graph” wrapper in
RedisJobStoreso disposed cached graphs don’t crash resume; stale cache entries are dropped and the code falls back to Redis chunk reconstruction. - In
GenerationJobManager.getResumeState, stamp missingask_user_questiontool-call args onto reconstructed content from the authoritative pending question payload. - Add a Redis-backed integration regression test covering both the disposed-graph crash and args-stamping on reconstructed resume state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/api/src/stream/implementations/RedisJobStore.ts | Adds a guarded cached-graph read path to avoid resume crashes and force reconstruction when the cached graph is disposed. |
| packages/api/src/stream/GenerationJobManager.ts | Stamps missing ask_user_question args into reconstructed aggregated content during resume state generation. |
| packages/api/src/stream/tests/hitlResumeRedis.stream_integration.spec.ts | Adds Redis Streams integration regression coverage for the crash and for args stamping on reconstruction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (err) { | ||
| logger.debug( | ||
| `[RedisJobStore] Cached graph for ${streamId} is unusable (likely disposed); falling back to reconstruction:`, | ||
| err instanceof Error ? err.message : err, | ||
| ); | ||
| this.localGraphCache.delete(streamId); | ||
| return null; | ||
| } |
| describe('HITL ask_user_question resume (Redis Streams)', () => { | ||
| let ioredisClient: Redis | Cluster | null = null; |
| afterAll(async () => { | ||
| if (ioredisClient) { | ||
| const keys = await ioredisClient.keys(`HitlResumeRedisTest*`); | ||
| const streamKeys = await ioredisClient.keys(`stream:*`); | ||
| await Promise.all([...keys, ...streamKeys].map((k) => ioredisClient!.del(k))); | ||
| await ioredisClient.quit(); | ||
| } | ||
| }); |
| console.warn('no redis'); | ||
| return; | ||
| } | ||
| const { manager } = await makeManager(); |
| await manager.emitChunk(streamId, runStep); | ||
| await manager.emitChunk(streamId, argDelta); | ||
| await new Promise((r) => setTimeout(r, 50)); |
| * @param streamId - The stream identifier | ||
| * @returns Content parts array or null if not found | ||
| */ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35e9daea4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| beforeAll(async () => { | ||
| process.env.USE_REDIS = process.env.USE_REDIS ?? 'true'; | ||
| process.env.USE_REDIS_CLUSTER = process.env.USE_REDIS_CLUSTER ?? 'false'; | ||
| process.env.REDIS_URI = process.env.REDIS_URI ?? 'redis://127.0.0.1:6379'; | ||
| process.env.REDIS_KEY_PREFIX = 'HitlResumeRedisTest'; |
There was a problem hiding this comment.
When the stream integration suite is run via test:cache-integration:stream, Jest runs these specs --runInBand, so this file can be followed by another stream integration spec in the same worker. These process.env mutations are never restored (unlike the neighboring Redis stream specs), so a later import of ~/cache/redisClients can be initialized with USE_REDIS/REDIS_KEY_PREFIX=HitlResumeRedisTest from this test, making the suite order-dependent and potentially pointing later tests at the wrong prefixed keys or a quit client. Please snapshot and restore process.env in afterAll.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1d1923a — added originalEnv = { ...process.env } snapshot in beforeAll and restore in afterAll, matching the neighboring Redis stream specs.
| @@ -897,7 +926,7 @@ export class RedisJobStore implements IJobStore { | |||
| if (graphRef) { | |||
| const graph = graphRef.deref(); | |||
| if (graph) { | |||
| const localParts = graph.getContentParts(); | |||
| const localParts = this.readCachedGraph(streamId, graph, (g) => g.getContentParts()); | |||
There was a problem hiding this comment.
When this fallback is hit for a disposed paused ask_user_question graph, callers other than getResumeState() still receive the reconstructed tool call with empty args. One concrete path is the allSubscribersLeft callback in GenerationJobManager calling jobStore.getContentParts() directly and api/server/controllers/agents/request.js persisting that raw aggregatedContent; after a user disconnects from a paused Redis stream, that partial save can overwrite the already-stamped unfinished response with a blank ask card. The stamping needs to cover direct getContentParts() consumers or that disconnect path needs to use the stamped resume content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the getResumeState stamp only covered one consumer and missed the allSubscribersLeft disconnect-persist path, and it's a separate concern from the crash this PR fixes. I've dropped the arg-stamp entirely in 1d1923a to keep the PR scoped to the disposed-graph crash. The reconstructed-empty-ask-args cosmetic (blank record card on reconnect/reload) is pre-existing for the cross-replica reconstruction path and will get its own fix that covers all getContentParts() consumers.
| } as unknown as ServerSentEvent; | ||
| await manager.emitChunk(streamId, runStep); | ||
| await manager.emitChunk(streamId, argDelta); | ||
| await new Promise((r) => setTimeout(r, 50)); |
There was a problem hiding this comment.
This delay is compensating for GenerationJobManager.emitChunk() persisting Redis chunks via fire-and-forget appendChunk(), but production callers do not wait before approvals.pause() or an immediate status/reload. In the disposed-graph path added here, getResumeState() can therefore reconstruct from Redis before the pre-pause on_run_step chunk has been written, returning empty/partial aggregatedContent instead of the ask card; the fix should make the relevant chunk writes durable before relying on reconstruction rather than depending on timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed in 1d1923a — that test (and its sleep) went away with the arg-stamp. The remaining disposed-graph test writes its fallback chunk via store.appendChunk() directly (which awaits the Lua eval), so there's no fire-and-forget race to paper over.
When a run pauses for `ask_user_question` (or tool approval), the paused turn's client is disposed and `disposeClient` (api/server/cleanup.js `graphPropsToClean`) runs `clearHeavyState()` and then NULLS the graph's internal arrays (`messages`, `contentData`). `RedisJobStore` still holds a `WeakRef` to that graph in `localGraphCache`, so on resume/reconnect `getContentParts()` / `getRunSteps()` deref the disposed graph and call `graph.getContentParts()` (`this.messages.slice()`) / `graph.getRunSteps()` (`[...this.contentData]`), throwing "Cannot read properties of null (reading 'slice')" / "this.contentData is not iterable" and aborting the resume. Redis-only: InMemoryJobStore reads `state.contentParts` / `graph?.contentData ?? []` directly and never calls the throwing SDK getters, which is why `USE_REDIS_STREAMS=false` works. RedisJobStore now reads the cached graph through `readCachedGraph()`, which tolerates a disposed graph: it swallows the deref error, drops the stale cache entry, and lets the caller fall back to durable chunk reconstruction instead of crashing. The true root cause (the unguarded null deref) is in `@librechat/agents` `StandardGraph.getContentParts()` / `getRunSteps()` and is addressed there separately; this is the host-side defensive guard. Fixes #14247. Addresses Bug 3 of #14253.
35e9dae to
1d1923a
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danny-avila
changed the title
…at-AI#14258) When a run pauses for `ask_user_question` (or tool approval), the paused turn's client is disposed and `disposeClient` (api/server/cleanup.js `graphPropsToClean`) runs `clearHeavyState()` and then NULLS the graph's internal arrays (`messages`, `contentData`). `RedisJobStore` still holds a `WeakRef` to that graph in `localGraphCache`, so on resume/reconnect `getContentParts()` / `getRunSteps()` deref the disposed graph and call `graph.getContentParts()` (`this.messages.slice()`) / `graph.getRunSteps()` (`[...this.contentData]`), throwing "Cannot read properties of null (reading 'slice')" / "this.contentData is not iterable" and aborting the resume. Redis-only: InMemoryJobStore reads `state.contentParts` / `graph?.contentData ?? []` directly and never calls the throwing SDK getters, which is why `USE_REDIS_STREAMS=false` works. RedisJobStore now reads the cached graph through `readCachedGraph()`, which tolerates a disposed graph: it swallows the deref error, drops the stale cache entry, and lets the caller fall back to durable chunk reconstruction instead of crashing. The true root cause (the unguarded null deref) is in `@librechat/agents` `StandardGraph.getContentParts()` / `getRunSteps()` and is addressed there separately; this is the host-side defensive guard. Fixes LibreChat-AI#14247. Addresses Bug 3 of LibreChat-AI#14253.
…at-AI#14258) When a run pauses for `ask_user_question` (or tool approval), the paused turn's client is disposed and `disposeClient` (api/server/cleanup.js `graphPropsToClean`) runs `clearHeavyState()` and then NULLS the graph's internal arrays (`messages`, `contentData`). `RedisJobStore` still holds a `WeakRef` to that graph in `localGraphCache`, so on resume/reconnect `getContentParts()` / `getRunSteps()` deref the disposed graph and call `graph.getContentParts()` (`this.messages.slice()`) / `graph.getRunSteps()` (`[...this.contentData]`), throwing "Cannot read properties of null (reading 'slice')" / "this.contentData is not iterable" and aborting the resume. Redis-only: InMemoryJobStore reads `state.contentParts` / `graph?.contentData ?? []` directly and never calls the throwing SDK getters, which is why `USE_REDIS_STREAMS=false` works. RedisJobStore now reads the cached graph through `readCachedGraph()`, which tolerates a disposed graph: it swallows the deref error, drops the stale cache entry, and lets the caller fall back to durable chunk reconstruction instead of crashing. The true root cause (the unguarded null deref) is in `@librechat/agents` `StandardGraph.getContentParts()` / `getRunSteps()` and is addressed there separately; this is the host-side defensive guard. Fixes LibreChat-AI#14247. Addresses Bug 3 of LibreChat-AI#14253.
Summary
Fixes the
Cannot read properties of null (reading 'slice')crash when resuming anask_user_question(or tool-approval) pause withUSE_REDIS_STREAMS=true. Reported in #14247; this is also Bug 3 of #14253.Root cause
When a run pauses for HITL, the paused turn's client is disposed.
disposeClient(api/server/cleanup.js,graphPropsToClean) runsclearHeavyState()and then nulls the graph's internal arrays (messages,contentData) for GC.RedisJobStorestill holds aWeakRefto that same graph object inlocalGraphCache. On resume/reconnect its fast path derefs the (now disposed) graph and calls the SDK getters:getContentParts()→StandardGraph.getContentParts()→this.messages.slice(this.startIndex)→reading 'slice' of nullgetRunSteps()→StandardGraph.getRunSteps()→[...this.contentData]→this.contentData is not iterableThis is Redis-only:
InMemoryJobStore.getContentParts()returns the storedstate.contentPartsarray, and itsgetRunSteps()readsgraph?.contentData ?? []— neither calls the throwing SDK getters. That's whyUSE_REDIS_STREAMS=falseworks.Fix (host-side defensive guard)
RedisJobStore: reads the cached graph through a newreadCachedGraph()helper that tolerates a disposed graph — it swallows the deref error, drops the stale cache entry, and lets the caller fall back to durable chunk reconstruction instead of aborting the resume.getResumeState: once the fallback path is taken, the reconstructedask_user_questiontool_call carries emptyargs(the streamed arg deltas are name-less and dropped by the aggregator, and no completion event fires). In-memory mode stamps those args at pause time; this mirrors that by stamping the reconstructed part'sargsfrom the authoritative pending question, so a reconstructed / cross-replica resume state shows the question instead of a blank record card. Idempotent — parts that already have args are untouched, so in-memory content is unaffected.The true root cause (the unguarded null deref) lives in
@librechat/agentsStandardGraph.getContentParts()/getRunSteps()and is being addressed there separately; this PR is the host-side guard so LibreChat is safe on the current SDK.Testing
packages/api/src/stream/__tests__/hitlResumeRedis.stream_integration.spec.ts(real Redis):getContentParts/getRunSteps; both fall back to reconstruction. Verified the test reproduces the exactreading 'slice' of nullwhen the guard is removed.getResumeStatestamps the reconstructed ask tool_call args from the pending question.All 207 existing stream tests pass;
tscclean.Notes on #14253
#14253 reports two further custom-endpoint (modelSpec ephemeral agent) resume failures — Bug 1 (
missing_modelfrom objectthinkingbreakingparseCompactConvoon resume replay) and Bug 2 (silent no-op resume frommodelLabel-derived agent id drift). Those are in the resume-replay path (restoreResumeContext) and are not covered here; still under investigation.