Skip to content

fix(core): handle processor replacement rejections - #1795

Open
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/tracing-processor-replacement-rejections
Open

fix(core): handle processor replacement rejections#1795
sylvesterkaczmarek wants to merge 3 commits into
openai:mainfrom
sylvesterkaczmarek:fix/tracing-processor-replacement-rejections

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

Handle rejected shutdown promises when replacing tracing processors.

MultiTracingProcessor.setProcessors() is synchronous but invokes each old processor's async shutdown() without observing its returned promise. If a custom processor rejects during shutdown, the rejection can surface as unhandled while the replacement processor list has already been installed.

Keep the synchronous replacement contract, attach a rejection handler to each old-processor shutdown, and report failures through the existing redaction-aware tracing error logger. Replacement processors continue to be installed immediately.

Test plan

  • replaces a processor whose shutdown() returns a rejected promise
  • verifies replacement remains synchronous/non-throwing
  • verifies the rejected shutdown is observed and logged
  • verifies the replacement processor is installed and receives subsequent trace events
  • verified the branch is exactly one commit ahead of current upstream main
  • full repository verification is left to GitHub Actions

Issue number

N/A. Found while auditing tracing processor replacement lifecycle.

Checks

  • I've added new tests (if relevant)
  • Documentation change is not required; public replacement semantics are unchanged
  • I've run pnpm test and pnpm test:examples
    • (If you made a major change) I've run pnpm test:integration
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR
  • I've added a changeset using pnpm changeset to indicate my changes

@changeset-bot

changeset-bot Bot commented Sep 1, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 47b1a17

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@openai/agents-core Patch
@openai/agents-extensions Patch
@openai/agents-openai Patch
@openai/agents-realtime Patch
@openai/agents Patch
realtime-react-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T23:45:13.150366Z 1e53f76 Manual request
🔒 Security Review Completed 2026-09-07T23:47:43.730181Z 1e53f76 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The shutdown-rejection path is worth fixing, and the catch belongs at this fire-and-forget boundary. However, handledRejection() already attaches a rejection handler before the SDK receives the Promise, so the current test cannot demonstrate that processor replacement prevents an unhandled rejection.

Please replace that fixture with an actually rejecting processor and verify through the replacement path that the SDK handles the rejection and the replacement remains usable. Keep the implementation scoped to the existing asynchronous processor contract.

Copy link
Copy Markdown
Contributor Author

Addressed the review on the latest branch head. The replacement test now uses an actually rejecting shutdown() promise, exercises it through MultiTracingProcessor.setProcessors(), and verifies the replacement remains installed and usable. The pre-handled rejection fixture is gone.

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor Author

Updated the regression per your review: the replaced processor now returns a genuine rejected shutdown promise with no pre-attached handler. The replacement path contains it, logs the failure, and the replacement remains usable. Focused test passes.

@seratch

seratch commented Sep 7, 2026

Copy link
Copy Markdown
Member

@codex review again

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 1e53f764f4

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: 1e53f764f4

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for updating the regression test. The genuine rejected shutdown promise addresses my previous feedback, and the runtime fix looks sound.

One existing fixture still needs updating: in packages/agents-core/test/tracing.test.ts, the test "should call all processors shutdown when setting new processors" assigns bare vi.fn() shutdown mocks. These return undefined, so the new .catch() call throws; coverage CI confirms this failure.

Please make both shutdown mocks return resolved promises, for example with vi.fn(async () => {}), and rerun the checks. This corrects the fixture to match the existing Promise<void> contract. No additional runtime fallback is needed.

Copy link
Copy Markdown
Contributor Author

Addressed in 47b1a170. The two shutdown spies in should call all processors shutdown when setting new processors now return resolved promises, matching the existing Promise<void> contract. Full packages/agents-core/test/tracing.test.ts passes 79/79, Prettier and git diff --check pass, and the pre-commit TruffleHog scan found no secrets.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants