Skip to content

fix: stop Codex app-server EOF handler spin - #668

Merged
Octane0411 merged 1 commit into
Octane0411:mainfrom
TangHuaiZhe:fix/codex-app-server-eof-cleanup
Sep 2, 2026
Merged

fix: stop Codex app-server EOF handler spin#668
Octane0411 merged 1 commit into
Octane0411:mainfrom
TangHuaiZhe:fix/codex-app-server-eof-cleanup

Conversation

@TangHuaiZhe

@TangHuaiZhe TangHuaiZhe commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Unregister Codex app-server stdout and stderr readability handlers when their pipes reach EOF.
  • Clear both output handlers before terminating the subprocess during an explicit stop.
  • Add lifecycle regression tests for EOF and explicit shutdown.

Root cause

CodexAppServerClient left each FileHandle.readabilityHandler installed after availableData returned empty data at EOF. On macOS, an EOF descriptor remains readable, so Foundation repeatedly invoked both the stdout and stderr handlers after the app-server process exited or its pipes closed. Those two hot dispatch sources could keep roughly two CPU cores busy. The existing stop() path also terminated the process without unregistering the handlers, allowing the same spin during normal disconnects.

Verification

  • swift test --disable-sandbox --scratch-path /Users/tanghuaizhe/Dev/open-vibe-island/.build --filter CodexAppServer — 7 tests passed.
  • swift test --disable-sandbox --scratch-path /Users/tanghuaizhe/Dev/open-vibe-island/.build — 370 tests passed.
  • git diff --check — passed.

Summary by CodeRabbit

  • Bug Fixes

    • Improved cleanup of application output streams when the connection ends or the client stops.
    • Prevented lingering output handlers after end-of-file, supporting more reliable shutdown behavior.
  • Tests

    • Added coverage for output-handler cleanup during normal stream closure and client shutdown.

@coderabbitai

coderabbitai Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b343f32-864e-40a7-b2f8-7ba1a7159632

📥 Commits

Reviewing files that changed from the base of the PR and between 38972f1 and c2e9e2d.

📒 Files selected for processing (2)
  • Sources/OpenIslandCore/CodexAppServer.swift
  • Tests/OpenIslandCoreTests/CodexAppServerLifecycleTests.swift

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

CodexAppServerClient now tracks stdout and stderr read handles, removes readability handlers at EOF, and clears handlers during stop(). Lifecycle tests cover asynchronous EOF cleanup and immediate stop cleanup.

Changes

Codex output handler lifecycle

Layer / File(s) Summary
Output handler setup and lifecycle cleanup
Sources/OpenIslandCore/CodexAppServer.swift, Tests/OpenIslandCoreTests/CodexAppServerLifecycleTests.swift
The client stores stdout and stderr read handles. start() configures both handlers through configureOutputHandlers(stdout:stderr:). Each handler clears itself at EOF. stop() clears both handlers and read handles. Tests validate EOF and stop cleanup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to c2e9e

This localized change unregisters app-server output handlers at EOF and during shutdown, with lifecycle tests added; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing CPU spin from the Codex app-server EOF handler.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Octane0411
Octane0411 merged commit 762accc into Octane0411:main Sep 2, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants