-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Projects] v2 update LRO poller for memory ops #36703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR replaces the generic LRO poller implementation with a custom, domain-specific MemoryStoreUpdatePoller for memory store update operations. The custom poller provides enhanced functionality specific to memory operations, including tracking updateId, updateStatus, and supersededBy properties, along with specialized handling for the "superseded" terminal status unique to memory updates.
Key Changes:
- Implements a custom
MemoryStoreUpdatePollerusing@azure/core-lro'screateHttpPollerwith domain-specific state management - Updates the
updateMemoriesoperation to return the new custom poller type instead of a genericPollerLike - Standardizes environment variable naming from
AZURE_AI_MODEL_DEPLOYMENT_NAMEtoMODEL_DEPLOYMENT_NAMEacross all samples - Adds new sample files demonstrating memory search tool usage with agents
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/ai/ai-projects/src/api/memoryStores/memoryStoreUpdatePoller.ts | New file implementing the custom MemoryStoreUpdatePoller with specialized state handling for memory update operations including "superseded" status |
| sdk/ai/ai-projects/src/api/memoryStores/operations.ts | Removes the generic deserializer function and updates updateMemories to use the new custom poller |
| sdk/ai/ai-projects/src/restorePollerHelpers.ts | Adds logic to restore the custom memory update poller from serialized state using the "memoryUpdate" kind discriminator |
| sdk/ai/ai-projects/src/api/memoryStores/index.ts | Exports the new poller types for public API surface |
| sdk/ai/ai-projects/src/index.ts | Re-exports the new poller types at the package root |
| sdk/ai/ai-projects/src/classic/memoryStores/index.ts | Updates type signature to use the custom poller type |
| sdk/ai/ai-projects/review/ai-projects-node.api.md | Documents the API surface changes including the new poller types and updated return type |
| sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentMemorySearch.ts | New sample demonstrating memory search tool with agent conversations |
| sdk/ai/ai-projects/samples/v2-beta/javascript/agents/tools/agentMemorySearch.js | JavaScript version of the memory search sample |
| sdk/ai/ai-projects/samples-dev/agents/tools/agentMemorySearch.ts | Development sample for memory search functionality |
| sdk/ai/ai-projects/samples/v2-beta/typescript/sample.env | Adds new environment variables for memory chat and embedding models, and TripAdvisor connection |
| sdk/ai/ai-projects/sample.env | Root sample environment file updated with new variables |
| Multiple sample files | Standardizes environment variable from AZURE_AI_MODEL_DEPLOYMENT_NAME to MODEL_DEPLOYMENT_NAME |
| sdk/ai/ai-projects/samples/v2-beta/typescript/README.md | Adds documentation entry for the new memory search sample |
| sdk/ai/ai-projects/samples/v2-beta/javascript/README.md | Adds documentation entry for the JavaScript memory search sample |
| sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentBingGrounding.ts | Fixes comment formatting (backticks to quotes) and updates environment variable name |
| sdk/ai/ai-projects/samples/v2-beta/javascript/agents/tools/agentBingGrounding.js | JavaScript version of the comment formatting fix and variable update |
| export function createMemoryStoreUpdatePoller( | ||
| client: Client, | ||
| expectedStatuses: string[], | ||
| getInitialResponse?: () => PromiseLike<PathUncheckedResponse>, | ||
| options: CreateMemoryStoreUpdatePollerOptions = {}, | ||
| ): MemoryStoreUpdatePoller { |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The createMemoryStoreUpdatePoller function is missing JSDoc documentation. As this is an exported function that creates the custom poller, it should document its parameters, return value, and usage examples following Azure SDK guidelines for public APIs.
| function buildRunningOperation( | ||
| client: Client, | ||
| expectedStatuses: string[], | ||
| getInitialResponse?: () => PromiseLike<PathUncheckedResponse>, |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buildRunningOperation function doesn't receive the user-provided options.abortSignal, which is needed for proper abort handling in sendPollRequest. Looking at the reference implementation in pollingHelpers.ts (lines 53-94), the abort signal from options is captured in the closure and used in the sendPollRequest method.
Without this, user abort signals won't properly cancel ongoing poll requests. The function signature should include options: CreateMemoryStoreUpdatePollerOptions as a parameter, and the sendPollRequest should handle options.abortSignal similar to the reference implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| if (parsed.status === "failed") { | ||
| state.status = "failed"; | ||
| if (!state.error) { | ||
| state.error = parsed.error | ||
| ? new Error(parsed.error.message ?? "Memory update failed") | ||
| : new Error("Memory update failed"); | ||
| } | ||
| return; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the status is "failed", the code only sets state.error if it doesn't already exist (if (!state.error)). However, it doesn't set state.result in the failure case. According to the LRO pattern, when an operation fails, the state should still be properly initialized. If a consumer expects state.result to always be defined when accessing it, this could cause issues.
Consider initializing state.result with a default value even in the failure case for consistency, or document that result will be undefined on failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LRO pattern says result should be populated only if status=completed, error should only be populated if status=failed. In this case, superseded_by should only be populated if status=superseded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| if (terminalUpdateStatuses.includes(parsed.status)) { | ||
| state.status = "succeeded"; | ||
| state.result = parsed.result ?? | ||
| state.result ?? { | ||
| memory_operations: [], | ||
| usage: createDefaultUsage(), | ||
| }; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check terminalUpdateStatuses.includes(parsed.status) will incorrectly mark "failed" status as succeeded. The terminalUpdateStatuses array includes "failed" (line 43), but the code already handles "failed" in the previous conditional (lines 111-118). This means if the check on line 111 is somehow bypassed or if there's a logic error, a "failed" status could incorrectly be treated as "succeeded".
The terminalUpdateStatuses array should only include non-error terminal statuses: ["completed", "superseded"].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| // @public (undocumented) | ||
| export type MemoryStoreUpdateOperationState = OperationState_2<MemoryStoreUpdateResult> & { | ||
| updateId?: string; | ||
| updateStatus?: MemoryStoreUpdateStatus; | ||
| supersededBy?: string; | ||
| }; | ||
|
|
||
| // @public (undocumented) | ||
| export type MemoryStoreUpdatePoller = PollerLike<MemoryStoreUpdateOperationState, MemoryStoreUpdateResult> & { | ||
| readonly updateId?: string; | ||
| readonly updateStatus?: MemoryStoreUpdateStatus; | ||
| readonly supersededBy?: string; | ||
| }; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new public API types MemoryStoreUpdateOperationState and MemoryStoreUpdatePoller are marked as (undocumented) in the API extractor output. According to Azure SDK guidelines, all public APIs must be documented. These types need JSDoc comments in the source code (memoryStoreUpdatePoller.ts) to appear properly documented in the API surface.
| sendPollRequest: async (path: string, pollOptions?: { abortSignal?: AbortSignalLike }) => { | ||
| function abortListener(): void { | ||
| pollAbortController.abort(); | ||
| } | ||
| const abortSignal = pollAbortController.signal; | ||
| if (pollOptions?.abortSignal?.aborted) { | ||
| pollAbortController.abort(); | ||
| } else if (!abortSignal.aborted) { | ||
| pollOptions?.abortSignal?.addEventListener("abort", abortListener, { | ||
| once: true, | ||
| }); | ||
| } |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abort signal handling in sendPollRequest is incomplete compared to the reference implementation in pollingHelpers.ts. The reference implementation (lines 67-83) handles both the user-provided options.abortSignal and the poll-specific pollOptions?.abortSignal, while this implementation only handles pollOptions?.abortSignal. This means user abort signals from the initial createMemoryStoreUpdatePoller options won't be properly propagated to poll requests.
Suggestion: Add handling for the user-provided abort signal similar to the reference implementation:
const abortSignal = pollAbortController.signal;
if (options?.abortSignal?.aborted) {
pollAbortController.abort();
} else if (pollOptions?.abortSignal?.aborted) {
pollAbortController.abort();
} else if (!abortSignal.aborted) {
options?.abortSignal?.addEventListener("abort", abortListener, {
once: true,
});
pollOptions?.abortSignal?.addEventListener("abort", abortListener, {
once: true,
});
}And in the finally block, remove both listeners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| export type MemoryStoreUpdateOperationState = OperationState<MemoryStoreUpdateResult> & { | ||
| updateId?: string; | ||
| updateStatus?: MemoryStoreUpdateStatus; | ||
| supersededBy?: string; | ||
| }; | ||
|
|
||
| export type MemoryStoreUpdatePoller = PollerLike< | ||
| MemoryStoreUpdateOperationState, | ||
| MemoryStoreUpdateResult | ||
| > & { | ||
| readonly updateId?: string; | ||
| readonly updateStatus?: MemoryStoreUpdateStatus; | ||
| readonly supersededBy?: string; | ||
| }; |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public API types MemoryStoreUpdateOperationState and MemoryStoreUpdatePoller are missing JSDoc documentation comments. According to Azure SDK guidelines, all public APIs should have clear documentation explaining their purpose and usage. This is especially important for these types as they introduce new extension properties (updateId, updateStatus, supersededBy) beyond the standard OperationState.
| export interface CreateMemoryStoreUpdatePollerOptions { | ||
| updateIntervalInMs?: number; | ||
| abortSignal?: AbortSignalLike; | ||
| restoreFrom?: string; | ||
| } |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CreateMemoryStoreUpdatePollerOptions interface is missing JSDoc documentation. This should document the purpose of each option, especially restoreFrom which is used for resuming a poller from a serialized state.
| } | ||
|
|
||
| // @public (undocumented) | ||
| export type MemoryStoreUpdateOperationState = OperationState_2<MemoryStoreUpdateResult> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we override existing status property instead of defining a new updateStatus?
(Surprised by OperationState_2 name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateId and status are both required, if that is consistent with OperationState_2 design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we override existing status property instead of defining a new updateStatus?
(Surprised by OperationState_2 name)
The reason for the OperationState name collision is that another "OperationState" is apparently being emitted from our TypeSpec, and that local OperationState roughly corresponds to the OperationStatus enum in core-lro/poller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we override existing status property instead of defining a new updateStatus?
(Surprised by OperationState_2 name)
Are the responses from the service constrained to the OperationStatus strings? I'm seeing "queued" and "superseded" as possible return statuses, but those don't exist in the OperationStatus enums here
sdk/ai/ai-projects/src/api/memoryStores/memoryStoreUpdateMemoriesPoller.ts
Outdated
Show resolved
Hide resolved
sdk/ai/ai-projects/src/api/memoryStores/memoryStoreUpdateMemoriesPoller.ts
Outdated
Show resolved
Hide resolved
sdk/ai/ai-projects/src/api/memoryStores/memoryStoreUpdateMemoriesPoller.ts
Outdated
Show resolved
Hide resolved
| if (parsed.status === "failed") { | ||
| state.status = "failed"; | ||
| if (!state.error) { | ||
| state.error = parsed.error | ||
| ? new Error(parsed.error.message ?? "Memory update failed") | ||
| : new Error("Memory update failed"); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LRO pattern says result should be populated only if status=completed, error should only be populated if status=failed. In this case, superseded_by should only be populated if status=superseded.
| } | ||
|
|
||
| if (terminalUpdateStatuses.includes(parsed.status)) { | ||
| state.status = "succeeded"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminal is the same as succeeded. It just means that we need to stop polling. Do we have to remap status to failed/succeeded/running? Or can we return the original values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing "superseded" or "queued" as status values for OperationStatus in the core-lro/poller. What do you suggest?
sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentMemorySearch.ts
Outdated
Show resolved
Hide resolved
sdk/ai/ai-projects/samples/v2-beta/typescript/src/agents/tools/agentMemorySearch.ts
Outdated
Show resolved
Hide resolved
The `buildRunningOperation` function wasn't receiving the user-provided
`options.abortSignal`, preventing proper cancellation of ongoing poll
requests.
**Changes:**
- Pass `options` parameter to `buildRunningOperation` to capture
user-provided abort signal
- Update `sendPollRequest` to listen to both `options.abortSignal` and
`pollOptions?.abortSignal`
- Add event listeners for both signals with proper cleanup in finally
block
This aligns with the reference implementation in `pollingHelpers.ts`
lines 53-94.
```typescript
// Before: only pollOptions abort signal was handled
sendPollRequest: async (path: string, pollOptions?: { abortSignal?: AbortSignalLike }) => {
if (pollOptions?.abortSignal?.aborted) {
pollAbortController.abort();
}
// ...
}
// After: both user-provided and poll-specific signals are handled
sendPollRequest: async (path: string, pollOptions?: { abortSignal?: AbortSignalLike }) => {
if (options?.abortSignal?.aborted) {
pollAbortController.abort();
} else if (pollOptions?.abortSignal?.aborted) {
pollAbortController.abort();
} else if (!abortSignal.aborted) {
options?.abortSignal?.addEventListener("abort", abortListener, { once: true });
pollOptions?.abortSignal?.addEventListener("abort", abortListener, { once: true });
}
// ...
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: glharper <[email protected]>
The `terminalUpdateStatuses` array included "failed", which could cause
failed operations to be incorrectly marked as succeeded if the explicit
failure check was bypassed.
## Change
Removed "failed" from `terminalUpdateStatuses` in
`memoryStoreUpdatePoller.ts`:
```typescript
// Before
const terminalUpdateStatuses: MemoryStoreUpdateStatus[] = ["completed", "superseded", "failed"];
// After
const terminalUpdateStatuses: MemoryStoreUpdateStatus[] = ["completed", "superseded"];
```
## Logic Flow
The `applyUpdateState` function handles statuses in this order:
1. **Lines 111-118**: Explicit "failed" check → sets `state.status =
"failed"`
2. **Lines 121-128**: `terminalUpdateStatuses` check → sets
`state.status = "succeeded"`
3. **Line 131**: Default → sets `state.status = "running"`
With "failed" in `terminalUpdateStatuses`, any logic error allowing
"failed" to reach line 121 would incorrectly mark it as succeeded. This
fix ensures only non-error terminal statuses ("completed", "superseded")
are treated as success.
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/Azure/azure-sdk-for-js/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: glharper <[email protected]>
…36710) The `sendPollRequest` function in `memoryStoreUpdatePoller.ts` only handled poll-specific abort signals (`pollOptions?.abortSignal`), ignoring user-provided abort signals from `createMemoryStoreUpdatePoller` options. This prevented proper cancellation propagation. **Changes** - Pass `options` parameter to `buildRunningOperation` to access user-provided abort signal - Check and listen to both `options.abortSignal` and `pollOptions?.abortSignal` in `sendPollRequest` - Remove both listeners in finally block to prevent memory leaks Implementation now matches the reference pattern in `pollingHelpers.ts` (lines 67-90). <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: glharper <[email protected]>
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists