Fix Kiro usage command pipe hangs#1535
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 1:30 AM ET / 05:30 UTC. Summary Reproducibility: yes. at source level. Current main still reaches waitUntilExit after termination and then readDataToEndOfFile on stdout/stderr, matching the linked detached-child and ignored-SIGTERM reproduction shapes. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the bounded Kiro runner after redacted focused fake-cli or Swift test output and CI confirm the inherited-pipe, ignored-SIGTERM, cancellation, and escaped-descendant paths pass without live Kiro credentials. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main still reaches waitUntilExit after termination and then readDataToEndOfFile on stdout/stderr, matching the linked detached-child and ignored-SIGTERM reproduction shapes. Is this the best way to solve the issue? Yes, the branch follows the narrow maintainable direction by reusing bounded pipe capture and process cleanup while preserving Kiro validation semantics. The remaining blocker is proof and completed CI, not a clear code defect in the latest diff. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fc8513228c5f. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb315dcc3
ℹ️ 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".
| didTerminateForIdle = true | ||
| break | ||
| } | ||
| try await Task.sleep(for: .milliseconds(100)) |
There was a problem hiding this comment.
When a provider refresh is superseded or its waiter is cancelled, UsageStore cancels the refresh task; with this new cancellable sleep, that cancellation throws out of runCommand while kiro-cli is still running. Because there is no cancellation handler or defer that clears the pipe handlers and terminates the process before rethrowing, a hung or slow kiro-cli can be left running in the background after the UI has abandoned the refresh.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Follow-up pushed in What changed:
Local proof: I also tried |
steipete
force-pushed
the
kiran/fix-kiro-bounded-command-runner
branch
from
fba48ab to
ad14534
Compare
clawsweeper
Bot
added
P2
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8976fa0656
ℹ️ 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".
| async let stdoutData = stdoutCapture.finish(timeout: .seconds(1)) | ||
| async let stderrData = stderrCapture.finish(timeout: .seconds(1)) |
There was a problem hiding this comment.
When kiro-cli exits after printing complete usage but leaves a helper holding stdout/stderr open, process.isRunning is already false so the termination block above is skipped; these bounded finish calls then just stop reading after one second and return the snapshot. In that inherited-pipe scenario a hung or long-lived helper survives the refresh, so repeated refreshes can leak CLI child processes instead of only avoiding the UI hang.
Useful? React with 👍 / 👎.
Launch Kiro CLI commands in a dedicated pre-exec process group and clean up inherited-pipe holders and residual helpers using start-time-verified identities. Add bounded termination/draining, portable pre-reap process tracking, Linux descriptor isolation, and focused lifecycle regressions. Follow-up to #1533 and #1535.
2 participants
Summary
Fixes #1533.
Kiro usage refresh could still wait forever after the direct
kiro-cliprocess exited or ignored termination, because the runner used an unboundedwaitUntilExit()plus final EOF reads on stdout/stderr. This keeps the existing activity-based behavior, but bounds process cleanup and stops relying on EOF from inherited pipes.The regression uses a fake
kiro-clithat prints valid usage, starts a child that keeps stdout open, and exits.fetch()now returns from the parent output instead of waiting for the child.Proof
swiftc -swift-version 6 -parse Sources/CodexBarCore/Providers/Kiro/KiroStatusProbe.swift Tests/CodexBarTests/KiroStatusProbeTests.swiftI also attempted the focused SwiftPM test locally, but SwiftPM hung while downloading Sparkle's binary artifact before it reached compilation. The added regression should run under normal CI package resolution.