Skip to content

Add WhenUpdatesCompleted to Foundry MemoryProvider to await async memory extraction - #653

Open
PratikDhanave (PratikDhanave) wants to merge 4 commits into
microsoft:mainfrom
PratikDhanaveFork:foundry-memory-when-updates-completed
Open

Add WhenUpdatesCompleted to Foundry MemoryProvider to await async memory extraction#653
PratikDhanave (PratikDhanave) wants to merge 4 commits into
microsoft:mainfrom
PratikDhanaveFork:foundry-memory-when-updates-completed

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

What

Adds WhenUpdatesCompleted(ctx, pollingInterval) to the Foundry MemoryProvider so callers can await asynchronous memory extraction before searching.

store() previously discarded the poller returned by BeginUpdateMemories, making memory updates fire-and-forget with no way to know when they land. This retains the most recent poller on the provider (guarded by a sync.Mutex) and drives it to completion on demand via runtime.Poller.PollUntilDone. store() stays non-blocking, so only opt-in callers pay the polling cost.

  • Returns nil when no update is pending or the update completed successfully.
  • Returns a non-nil error when the update failed.
  • Clears the pending update on success so repeated calls are no-ops.

Why (parity)

Memory extraction on Foundry is a long-running operation. The .NET/Python agent-framework Foundry memory providers submit the update and expose a way to await the extraction result (the LRO poller / update-result endpoint) so a subsequent SearchMemories sees the freshly written memories. The Go port kept the fire-and-forget submit but never surfaced the completion handle, so tests and callers had no deterministic way to sequence store -> search. This closes that gap while keeping the hot path unchanged.

Not a duplicate of #627/#619/#617.

How tested

go build ./..., go vet ./provider/foundryprovider/..., go test ./provider/foundryprovider/... all pass. New tests in memory_test.go use the existing recordingTransport to:

  • script BeginUpdateMemories (202 + Operation-Location) then a status sequence queued -> completed, asserting WhenUpdatesCompleted returns nil and clears the pending update (a second call issues no further requests);
  • script a failed status, asserting a non-nil error;
  • assert the no-pending case returns nil immediately without any request.

Open design questions

  • API shape: method exposes pollingInterval and relies on the caller's ctx for the overall deadline. Should it instead take a full runtime.PollUntilDoneOptions, or default the interval?
  • Scope: only the single most-recent update is tracked. Concurrent/overlapping updates keep only the latest poller. Is per-scope tracking wanted, or is latest-wins sufficient?
  • Terminal states: PollUntilDone treats completed as success and failed as error. Foundry also has a superseded terminal state (an update replaced by a newer one); the generic poller does not treat it as terminal. If awaiting a superseded update matters, a follow-up could poll GetUpdateResult directly and treat superseded as success. Flagging as a follow-up rather than expanding scope here.

@PratikDhanave
PratikDhanave (PratikDhanave) force-pushed the foundry-memory-when-updates-completed branch from e4cc68d to b43f47a Compare July 23, 2026 15:41
@github-actions

This comment has been minimized.

@PratikDhanave
PratikDhanave (PratikDhanave) force-pushed the foundry-memory-when-updates-completed branch from b43f47a to eab5828 Compare July 24, 2026 01:40
@github-actions github-actions Bot added public-api-change Pull Request changes public APIs parity-approved Go API consistency review found no parity issues labels Jul 24, 2026
@github-actions

This comment has been minimized.

Foundry extracts memories asynchronously after a run, so store() is
fire-and-forget and returns before memories are persisted. Retain the
long-running poller returned by BeginUpdateMemories (guarded by a mutex)
and expose WhenUpdatesCompleted to await the most recent update. This
lets callers and tests observe stored memories before the next search
without making the store path block.
@PratikDhanave
PratikDhanave (PratikDhanave) force-pushed the foundry-memory-when-updates-completed branch from eab5828 to 442ca39 Compare July 24, 2026 09:35
@github-actions github-actions Bot removed the parity-approved Go API consistency review found no parity issues label Jul 24, 2026
# Conflicts:
#	provider/foundryprovider/memory_test.go
@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by Go API Consistency Review Agent · sonnet46 · 31.2 AIC · ⌖ 5.89 AIC · ⊞ 5.7K

return nil
}
if _, err := poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{Frequency: pollingInterval}); err != nil {
return err

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parity gap — superseded terminal state

The .NET WhenUpdatesCompletedAsync explicitly treats MemoryStoreUpdateStatus.Superseded as a successful terminal state (returns without error), mirroring Foundry service semantics where a superseded update is implicitly replaced by a newer one.

The Go implementation delegates to runtime.Poller.PollUntilDone, which only knows standard Azure LRO states and does not treat superseded as terminal. As a result, WhenUpdatesCompleted will keep polling past a superseded status until the service returns a different state or the context times out — diverging from the .NET behaviour.

The PR acknowledges this in the open design questions, but it is worth flagging explicitly since .NET has already shipped the fix:

// .NET — FoundryMemoryProvider.cs
if (status == MemoryStoreUpdateStatus.Completed || status == MemoryStoreUpdateStatus.Superseded)
    return; // treat superseded as success

Suggestion: before merging, consider either (a) using a custom PollingHandler / response classifier in the Azure Go SDK so the poller treats superseded as done, or (b) replacing PollUntilDone with a manual polling loop that mirrors .NET's WaitForUpdateAsync and explicitly handles superseded → success.

@PratikDhanave
PratikDhanave (PratikDhanave) marked this pull request as ready for review August 4, 2026 06:06
@PratikDhanave
PratikDhanave (PratikDhanave) requested a review from a team as a code owner August 4, 2026 06:06
Copilot AI lite review requested due to automatic review settings August 4, 2026 06:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an opt-in mechanism to await completion of Foundry’s asynchronous memory extraction, enabling deterministic sequencing of “store → search” without making the normal store() path blocking.

Changes:

  • Retains the most recent Foundry LRO poller returned by BeginUpdateMemories and exposes WhenUpdatesCompleted(ctx, pollingInterval) to drive it to completion.
  • Keeps store() non-blocking while allowing callers (notably tests) to wait for extraction to finish before searching.
  • Adds unit tests covering success (including “clears pending update”), failure, and “no pending update” behavior using the existing recording transport harness.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
provider/foundryprovider/memory.go Stores the latest update poller and adds WhenUpdatesCompleted to await LRO completion on demand.
provider/foundryprovider/memory_test.go Adds tests validating await-success, await-failure, and no-op behavior when nothing is pending.

# Conflicts:
#	provider/foundryprovider/memory_test.go
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added area:provider Changes files in the provider area area:provider/foundry Changes files in the provider / foundry area size:large At most 300 changed lines across at most 10 files pending-auto-risk Automatic risk classification is in progress labels Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

API Consistency Review — PR #653

Verdict: Parity mostly preserved, one actionable gap flagged inline.

What changed

  • New exported method: (*MemoryProvider).WhenUpdatesCompleted(ctx context.Context, pollingInterval time.Duration) error
  • Internal: pendingUpdate *runtime.Poller[...] field + sync.Mutex guard on MemoryProvider

Upstream equivalents found

SDK Surface Location
.NET FoundryMemoryProvider.WhenUpdatesCompletedAsync(TimeSpan? pollingInterval, CancellationToken) dotnet/src/Microsoft.Agents.AI.Foundry/Memory/FoundryMemoryProvider.cs
Python (not yet implemented — after_run is still fire-and-forget) python/packages/foundry/agent_framework_foundry/_memory_provider.py

Parity issues

1. (Inline comment) superseded terminal state not handled — medium severity

.NET's WhenUpdatesCompletedAsync explicitly treats Foundry's superseded status as a success terminal state (equivalent to completed). The Go implementation delegates to runtime.Poller.PollUntilDone, which only recognises generic Azure LRO states (Succeeded/Failed/Canceled). If Foundry returns superseded, the poller will loop until the ctx deadline, rather than returning nil as the upstream .NET behaviour dictates. See inline comment on memory.go.

2. pollingInterval is required in Go, optional with a default in .NET

.NET defaults to 5 seconds (pollingInterval ?? TimeSpan.FromSeconds(5)). The Go signature requires the caller to supply an explicit time.Duration. This is a minor usability divergence; a zero-value default (e.g. 0 meaning "use a sensible built-in default") or a documented recommendation would bring it closer to upstream.

Items that match upstream

  • Fire-and-forget store() path is unchanged ✅
  • Pending update is tracked per-provider instance, latest-wins ✅ (mirrors .NET Interlocked.Exchange` pattern)
  • On success the pending update is cleared so repeated calls are no-ops ✅
  • Returns nil immediately when no update is pending ✅
  • Returns non-nil error on a failed update ✅
  • public-api-change label already present ✅

Labels

No label changes needed — public-api-change is already applied. parity-approved is withheld pending resolution of the superseded-state gap (issue 1 above).

Generated by Go API Consistency Review Agent · sonnet46 · 31.6 AIC · ⌖ 5.27 AIC · ⊞ 6K ·

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by Go API Consistency Review Agent · sonnet46 · 31.6 AIC · ⌖ 5.27 AIC · ⊞ 6K

if poller == nil {
return nil
}
if _, err := poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{Frequency: pollingInterval}); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parity gap — superseded terminal state not handled

The .NET WhenUpdatesCompletedAsync (FoundryMemoryProvider.cs) explicitly treats superseded as a success terminal state alongside completed:

if (status == MemoryStoreUpdateStatus.Completed || status == MemoryStoreUpdateStatus.Superseded)
{
    return;
}

This Go implementation delegates to runtime.Poller.PollUntilDone, which only knows about generic Azure LRO states (Succeeded, Failed, Canceled). The Foundry-specific superseded state is not a standard Azure LRO terminal state, so the poller will likely loop until ctx is cancelled if an update is superseded by a newer one.

The PR description flags this as a known follow-up, but callers awaiting in a test or workflow may hang silently. Consider adding an explicit doc note to the godoc comment warning that a superseded update will block until the context deadline, and linking to a tracking issue.

@github-actions github-actions Bot added failed-auto-risk Automatic risk classification was inconclusive or failed and removed pending-auto-risk Automatic risk classification is in progress labels Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:provider/foundry Changes files in the provider / foundry area area:provider Changes files in the provider area failed-auto-risk Automatic risk classification was inconclusive or failed public-api-change Pull Request changes public APIs size:large At most 300 changed lines across at most 10 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants