fix(codex): yield app-server notification projection#82333
Conversation
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: no. high-confidence live reproduction was run in this review. The linked issue gives concrete production liveness logs and current source confirms the queued notification projection path, so this is source-reproducible rather than fully reproduced here. Real behavior proof Next step before merge Security Review detailsBest possible solution: Land a narrow Codex app-server notification-yield fix after maintainer review and live or diagnostic proof that account and MCP status notifications no longer stall gateway liveness. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was run in this review. The linked issue gives concrete production liveness logs and current source confirms the queued notification projection path, so this is source-reproducible rather than fully reproduced here. Is this the best way to solve the issue? Yes, this is a narrow maintainable direction for the reported queue starvation: keep pre-turn capture synchronous and yield before projector work. The missing piece is real runtime proof that the liveness symptom improves under the reported Codex/MCP or cron path. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 0ad3d25fb7cd. |
* fix(codex): yield app-server notification projection * docs(changelog): note codex notification yield fix
* fix(codex): yield app-server notification projection * docs(changelog): note codex notification yield fix
* fix(codex): yield app-server notification projection * docs(changelog): note codex notification yield fix
* fix(codex): yield app-server notification projection * docs(changelog): note codex notification yield fix
Summary
Verification
Behavior addressed: Codex app-server account and MCP status notifications no longer project synchronously through the embedded-run notification queue while still preserving pre-turn rate-limit capture.
Real environment tested: Local targeted Codex app-server harness tests in this checkout.
Exact steps or command run after this patch: node scripts/run-vitest.mjs extensions/codex/src/app-server/run-attempt.test.ts -t "yields a macrotask|preserves Codex usage-limit reset details"; node scripts/run-oxlint.mjs extensions/codex/src/app-server/run-attempt.ts extensions/codex/src/app-server/run-attempt.test.ts
Evidence after fix: The targeted Vitest command passed 2 tests with 119 skipped; oxlint reported 0 warnings and 0 errors.
Observed result after fix: Notification projection yields to the event loop after synchronous queue bookkeeping, and the usage-limit test preserves rate-limit reset details when turn/start fails.
What was not tested: Live Codex OAuth/MCP gateway run, Telegram delivery, AWS health checks, and a production cron-triggered embedded_run.
Fixes #81936