Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Implements a host-owned controlled-session supervisor that orchestrates controller/workload startup, request handling, termination, output finalization, result delivery/ack, and cleanup, aligning the Docker runtime adapter with supervised lifecycle ownership.
Changes:
- Add
RunControlledSessionV1and supporting supervisor implementation to drive the controlled-session lifecycle machine end-to-end. - Harden Docker workload PTY handling by switching lifecycle operations to container-ID targeting and adding explicit
Started()/Cleanup()semantics. - Extend the lifecycle machine to represent startup failures where workload output finalization is still pending, plus add unit + Docker integration evidence and update design docs.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/dockerdeploy/testdata/session_channel_helper/main.go | Adds a “supervise” mode to exercise the supervisor lifecycle over the session channel. |
| internal/dockerdeploy/controlled_session_workload_pty.go | Tracks container ID and adds Started()/Cleanup() for supervised teardown correctness. |
| internal/dockerdeploy/controlled_session_workload_pty_test.go | Updates PTY adapter tests for container-ID operations and cleanup idempotence. |
| internal/dockerdeploy/controlled_session_supervisor.go | New supervisor that owns the controlled-session lifecycle from prepare through delivery-tail cleanup. |
| internal/dockerdeploy/controlled_session_supervisor_test.go | New unit tests covering normal lifecycle, cancellation, runtime-loss, and deadlines. |
| internal/dockerdeploy/controlled_session_supervisor_integration_test.go | New Docker integration test validating end-to-end supervised behavior and cleanup. |
| internal/dockerdeploy/controlled_session_controller.go | Reuses shared Docker container-ID parsing helper. |
| internal/dockerdeploy/controlled_session_controller_test.go | Updates container-ID parsing test to the renamed shared helper. |
| internal/controlledsession/lifecycle.go | Adds startup-failure modeling for “pending workload output finalization” cases. |
| internal/controlledsession/lifecycle_test.go | Adds coverage for startup failure waiting on partially-started workload output finalization. |
| docs/CONTROLLED_SESSION_DESIGN.md | Updates design documentation to reflect the implemented host supervisor. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func (process *fakeControlledSessionProcessV1) ForceStop(context.Context) error { | ||
| process.forceStopped = true | ||
| return nil | ||
| } |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
![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: 88709615e2
ℹ️ 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".
| select { | ||
| case result := <-supervisor.workloadResult: | ||
| supervisor.workloadObserved = &result | ||
| supervisor.observeWorkloadResult(result) | ||
| case result := <-supervisor.controllerResult: | ||
| supervisor.controllerObserved = &result | ||
| supervisor.observeControllerLoss("controller process exited", result.err) | ||
| case <-supervisor.bridge.RequestsDone(): |
There was a problem hiding this comment.
When the Docker attachment encounters a read error or output delivery fails while the workload remains running, SessionIOBridgeV1.OutputDone() closes, but this active-state wait never observes it. If the controller connection and workload otherwise remain alive, the session can therefore continue indefinitely after losing its only output stream, potentially blocking the workload on PTY backpressure and consuming resources until an unrelated exit or host cancellation occurs. The supervisor should treat output-pump termination as a lifecycle failure and begin teardown rather than waiting only for workload, controller, request-stream, or context events.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41e0f3203
ℹ️ 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".
| supervisor.sendTerminating() | ||
| supervisor.stopAndObserveWorkload() |
There was a problem hiding this comment.
When a controller input request is blocked by PTY backpressure, handleRequest remains in WriteInput with the bridge's request context and holds the workload's operationMu. This teardown path does not call StopRequests until cleanupDeliveryTail, so RequestGracefulStop blocks acquiring the same mutex before it can observe its own deadline; host cancellation, workload exit, or controller termination can therefore hang indefinitely without reaching forced stop or cleanup. Stop the request pump as soon as termination begins so the in-flight write is canceled before workload teardown.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6416ae1db
ℹ️ 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".
| _, observeErr := supervisor.observe(controlledsession.ObservationV1{ | ||
| Kind: controlledsession.ObservationWorkloadOutputsFinalizedV1, | ||
| WorkloadOutputFinalizationStatus: &finalization.Status, | ||
| }) |
There was a problem hiding this comment.
When another cause such as host cancellation, controller termination, or workload exit moves the machine out of active before the PTY pump fails, waitForTermination stops selecting OutputDone. This path then records only the failed output-finalization status and, unlike observeOutputTermination, never applies ObservationRuntimeObservationLostV1; consequently the terminal result incorrectly reports runtime observation as maintained despite a PTY read or closure failure during teardown. Classify the pump's terminal failure here before recording output finalization while preserving the already-latched cause.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 215c80816e
ℹ️ 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".
| if err := supervisor.prepare(startupCtx); err != nil { | ||
| return supervisor.finishStartupFailure(err) |
There was a problem hiding this comment.
When the admitted caller cancels while channel, controller, or workload startup is in progress, prepare returns the context error here, but this path records only ObservationStartupFailureV1. The terminal result therefore reports startup-failure instead of host-cancel, even though the lifecycle machine supports latching a pre-activation host cancellation before recording the subsequent startup/output facts. Check ctx.Err() and observe host cancellation before finishing the startup failure.
Useful? React with 👍 / 👎.
| if err != nil && canceledByHost && errors.Is(err, context.Canceled) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
When termination races a resize blocked in Docker HTTP I/O, host cancellation closes the resize connection, but resizeDockerContainerPTYV1 can return a wrapped write/read socket error rather than context.Canceled; this condition then terminates request dispatch, and waitForControllerFinalization records the still-live controller as lost so it cannot send complete or acknowledge the result. Fresh evidence beyond the earlier blocked-input case is the production resize adapter's raw network-error returns in docker_pty_attach.go. Since canceledByHost already proves lifecycle-owned cancellation, do not require the handler error itself to match context.Canceled.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01b3781416
ℹ️ 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".
| _, observeErr := supervisor.observe(controlledsession.ObservationV1{ | ||
| Kind: controlledsession.ObservationWorkloadOutputsFinalizedV1, | ||
| WorkloadOutputFinalizationStatus: &finalization.Status, | ||
| }) |
There was a problem hiding this comment.
When a controller sends complete in the interval between this observation and sendLifecycleEvent, completeOutputFinalizationLocked has already cleared waitingOutputs and opened waitingFinalize, so ApplyRequest accepts the request even though the controller has not yet received workload_outputs_finalized. This lets an eager or adversarial controller report successful finalization without processing the final output status, contrary to the protocol ordering; gate complete until publication of that event has resolved.
Useful? React with 👍 / 👎.
Stack created with Sapling. Best reviewed with ReviewStack.