Skip to content

Python: fix: bound pending policy approvals - #7996

Open
Patel Namraa (Namraa310806) wants to merge 7 commits into
microsoft:mainfrom
Namraa310806:fix/7890-bound-pending-approvals
Open

Python: fix: bound pending policy approvals#7996
Patel Namraa (Namraa310806) wants to merge 7 commits into
microsoft:mainfrom
Namraa310806:fix/7890-bound-pending-approvals

Conversation

@Namraa310806

Copy link
Copy Markdown
Contributor

Motivation & Context

PolicyEnforcementFunctionMiddleware can retain pending policy-approval entries indefinitely when approval_on_violation=True and an approval is never consumed.

The pending approvals are stored in _pending_policy_approvals. When applications reuse a middleware instance across many interactions, unconsumed approvals can accumulate over time, causing unbounded memory growth.

This PR fixes the lifecycle of those pending approvals while preserving the existing approval-binding and fail-closed security behavior.

Fixes #7890

Description & Review Guide

  • What are the major changes?

    • Bound _pending_policy_approvals with a configurable max_pending_approvals limit, defaulting to 1000.
    • Evict the oldest pending approval using FIFO ordering when the configured limit is exceeded.
    • Remove stale pending approvals when an approval for the same call_id is encountered from a different session.
    • Preserve the existing session, call ID, function/argument, security-label, and violation matching checks from the approval-binding implementation.
    • Add regression coverage for FIFO eviction, configured limits, session-mismatch cleanup, evicted approvals failing closed, and concurrent bursts of unique approval requests.
  • What is the impact of these changes?

    • Prevents _pending_policy_approvals from growing without bound under repeated unconsumed policy violations.
    • The default limit bounds retained pending approval state while still allowing applications to configure the limit.
    • Evicted or stale approvals cannot authorize tool execution; they fail closed and require a new approval request.
    • Existing valid approve-and-replay behavior remains unchanged.
  • What do you want reviewers to focus on?

    • Whether the default max_pending_approvals=1000 is an appropriate bound for the experimental FIDES policy-approval flow.
    • Whether session-mismatch cleanup is the appropriate lifecycle point for removing approvals that can no longer match.
    • Whether the FIFO eviction behavior and fail-closed semantics are correct when the pending-approval limit is reached.
    • Whether the regression tests adequately cover the memory-growth and security-sensitive edge cases.

Related Issue

Fixes #7890

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Sep 1, 2026
@github-actions github-actions Bot changed the title fix: bound pending policy approvals Python: fix: bound pending policy approvals Sep 1, 2026
@Namraa310806

Copy link
Copy Markdown
Contributor Author

Note on Related Work

A draft PR (#7893) was already opened for this issue, and I reviewed it while working on #7890. Since the draft had not yet progressed to an active PR while the issue remained unresolved in the main codebase, I opened this PR with a more concise approach focused on directly bounding the pending approval state and preserving the existing approval-binding behavior.

The intent is not to duplicate or compete with the existing work, but to make sure the underlying bug is fixed with a focused and maintainable approach. I’m happy to adjust or consolidate the implementation based on maintainer feedback.

Comment on lines +1939 to +1954
call_id = self._get_call_id(context)
if call_id:
# If bounded, evict oldest entry when adding a new unique call_id would exceed limit.
# Do not evict when updating an existing call_id (re-request scenario).
if (
self._max_pending_approvals is not None
and call_id not in self._pending_policy_approvals
and len(self._pending_policy_approvals) >= self._max_pending_approvals
):
# Evict oldest (first) entry
oldest_call_id = next(iter(self._pending_policy_approvals))
del self._pending_policy_approvals[oldest_call_id]
logger.debug(
f"Evicted oldest pending approval '{oldest_call_id}' to maintain limit of {self._max_pending_approvals}"
)
self._pending_policy_approvals[call_id] = self._pending_record(context, violations)

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.

Should the bounded map stay keyed by approval_id rather than provider call_id? Framework calls can carry a distinct function_call_occurrence_id; _matches_pending_approval looks up that occurrence-aware ID at security.py:1868 and _consume_pending_approval removes it at security.py:1908, but this path inserts by call_id. Once the crash above is fixed, valid occurrence-aware approvals will still miss the stored record and be re-requested instead of executing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. You're right — the pending-approval map needs to use the same canonical identity as the lookup and consume paths.

I've updated the insertion path to use _get_approval_id(context) instead of the provider call_id, while keeping call_id as part of the existing approval-binding validation.

I also added a regression test covering the function_call_occurrence_id case to verify the full request → store → approve → match → consume lifecycle.

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.

Could we restore approval_id = self._get_approval_id(context) before building the request? _request_policy_violation_approval now defines only call_id, but passes approval_id at security.py:1971, so every violating call with approval_on_violation=True raises NameError before MiddlewareTermination can return the approval request.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you're right. I had missed restoring the approval_id = self._get_approval_id(context) assignment in _request_policy_violation_approval().

I've restored it and verified that the approval request is now created with the same approval_id used for storing, matching, and consuming the pending approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: Unbounded memory growth in PolicyEnforcementFunctionMiddleware pending approvals

3 participants