fix(acp): cancellation, session cleanup, proxy EOF, and session dir fixes - #658
Merged
Sertaç Özercan (sozercan) merged 4 commits intoSep 23, 2026
Merged
Conversation
Flushing an HTTP/1 response can close the incoming request body before the upstream transport makes its final EOF check after sending the body. Remember the EOF in the bounded reader and keep returning it so a streaming response no longer disconnects the forwarded prompt.
MkdirAll applies the supervisor's private umask, so the session base directory could lose the mode that lets child UIDs traverse to their own private trees. Chmod the verified directory handle to the intended mode after confirming it is the same real directory.
A failed process stop observation is not a cleanup proof. Let a later Delete call observe the same stopped process again while callers that already joined an earlier attempt keep that attempt's result, and send the courtesy cancel only on the first deletion.
Revoke provider and MCP authority immediately, then hold only locally caused cancellation errors until the exact prompt settles or the bounded wait ends. Preserve independent failures, completed results, and separate remote cleanup proof. Add real-process cancellation API regressions and sanitized lifecycle diagnostics.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Copilot started reviewing on behalf of
Sertaç Özercan (sozercan)
September 23, 2026 06:29
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-cutting cancellation and cleanup concurrency changes warrant final human review despite extensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
This PR hardens ACP cancellation, cleanup, filesystem isolation, and provider request streaming.
Changes:
- Preserves cancellation provenance across provider and MCP proxy revocation.
- Enables cleanup retries and correct session-directory permissions.
- Preserves request EOF during streamed proxy responses and adds regression coverage.
| File | Description |
|---|---|
workers/acp/supervisor/provider_proxy.go |
Holds locally cancelled proxy errors until prompt settlement. |
workers/acp/supervisor/provider_proxy_cancellation_test.go |
Tests provider cancellation races and capacity bounds. |
workers/acp/supervisor/prompt_handlers.go |
Associates prompt settlement waits with cancellation revocation. |
workers/acp/supervisor/prompt_gate_cancellation.go |
Defines cancellation-cause handling. |
workers/acp/supervisor/prompt_cancellation_integration_test.go |
Tests end-to-end provider cancellation ordering. |
workers/acp/supervisor/mcp_proxy.go |
Preserves MCP cancellation provenance and definitive results. |
workers/acp/supervisor/mcp_proxy_cancellation_test.go |
Tests MCP cancellation and bounded admission. |
workers/acp/supervisor/mcp_cancellation_integration_test.go |
Tests MCP cancellation lifecycle and cleanup. |
workers/acp/supervisor/mcp_broker_client.go |
Propagates local context cancellation distinctly. |
workers/acp/supervisor/mcp_broker_client_test.go |
Covers cancellation and independent broker outcomes. |
internal/acp/session.go |
Adds settlement waiting and retryable deletion observations. |
internal/acp/session_settlement_wait_test.go |
Tests bounded, prompt-specific settlement waits. |
internal/acp/session_delete_retry_test.go |
Tests cleanup retries and waiter result isolation. |
internal/acp/runtime_env.go |
Applies intended mode through a verified directory handle. |
internal/acp/runtime_env_unix_test.go |
Tests directory modes under a private umask. |
internal/acp/runtime_env_test.go |
Verifies rejected symlinks remain unchanged. |
internal/acp/runtime_env_linux_test.go |
Tests cross-UID session isolation. |
cmd/orka-provider-auth-proxy/request_body_test.go |
Covers terminal EOF and request limits. |
cmd/orka-provider-auth-proxy/proxy.go |
Remembers terminal EOF after request-body closure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sertaç Özercan (sozercan)
added this pull request to stack #661
September 23, 2026 06:50
Sertaç Özercan (sozercan)
deleted the
fix/acp-cancellation-and-cleanup
branch
September 23, 2026 15:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four independent fixes found during live validation of the human-approval work, split out of #589 so they can land on their own. Each is one commit.
MkdirAllapplies the supervisor's private umask, so child UIDs could lose traversal into their own private trees. The verified directory handle is chmod'ed to the intended mode.Deletemay observe the same stopped process again, callers already joined to an earlier attempt keep that attempt's result, and the courtesy cancel is sent only on the first deletion.The cancellation regression test targets a read-only brokered tool here; #589 re-points it at an approval-required tool once the controller owns approval decisions.
Stack: this PR →
feat/foundry-boot-recovery→ #589.test/e2e-cleanup-evidenceis independent.Validation:
make lint-fix,make test, and the focusedinternal/acp,workers/acp/supervisor, andcmd/orka-provider-auth-proxysuites.