Conversation
There was a problem hiding this comment.
Pull request overview
Introduces a concurrency-safe, host-owned lifecycle state machine for controlled sessions, formalizing transitions from preparing → active → terminating → terminated while ensuring the first accepted termination cause is latched and preserved across later observations/requests.
Changes:
- Added
MachineV1lifecycle state machine with validated observations/requests and deterministic cause latching. - Added unit tests covering concurrency races, authorization immutability, finalization timeout behavior, and idempotence guarantees.
- Updated controlled session design documentation to reflect the expanded set of termination causes (including controller-requested termination).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/controlledsession/lifecycle.go | Adds the V1 lifecycle state machine, observation handling, request handling, and snapshot/transition helpers. |
| internal/controlledsession/lifecycle_test.go | Adds coverage for lifecycle races, termination cause latching, authorization enforcement, and finalization/timeout behaviors. |
| docs/CONTROLLED_SESSION_DESIGN.md | Updates lifecycle/termination-cause documentation to match the new design semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transition.After = machine.state | ||
| transition.Cause = machine.cause | ||
| transition.AwaitingControllerFinalization = machine.waitingFinalize | ||
| transition.RequestAccepted = true | ||
| return transition, nil |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
580cec9 to
97d53e0
Compare
|
@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: d13fcaa72f
ℹ️ 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 := validateCauseObservationV1(observation); err != nil { | ||
| return transition, err | ||
| } | ||
| machine.latchLocked(CauseRuntimeObservationLostV1, &transition) |
There was a problem hiding this comment.
When runtime observation is lost after the output barrier has set waitingFinalize, this call only invokes latchLocked; because a cause is already latched, that function returns without clearing the wait. ObservationFinishedV1 then remains rejected until the controller completes or the finalization timer expires, even though loss of authoritative runtime observation requires immediate session failure and closure. Host cancellation and a repeated controller termination have the same abortive-finalization problem, so these paths should end the wait while preserving the original cause.
Useful? React with 👍 / 👎.
| return transition, fmt.Errorf("%w: %v", ErrObservationRejected, err) | ||
| } | ||
| machine.workloadOutputs = *observation.WorkloadOutputFinalizationStatus | ||
| machine.waitingFinalize = machine.controller.Kind == ControllerFinalizationActiveV1 |
There was a problem hiding this comment.
For a valid authorization that omits OperationCompleteV1, an active controller still enters waitingFinalize after workload output is finalized. Every complete request is then rejected by the authorization check, while ObservationFinishedV1 is rejected because the wait remains active, forcing every such session to consume the full timeout and report finalization-timeout rather than not-completed. Only arm this wait when the controller was granted the operation needed to satisfy it.
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: f43d62d7d3
ℹ️ 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 := validateWorkloadOutputFinalizationStatusV1(*observation.WorkloadOutputFinalizationStatus); err != nil { | ||
| return transition, fmt.Errorf("%w: %v", ErrObservationRejected, err) | ||
| } | ||
| machine.workloadOutputs = *observation.WorkloadOutputFinalizationStatus |
There was a problem hiding this comment.
When runtime-observation-lost is latched before the output barrier, this branch still accepts WorkloadOutputFinalizationDrainedV1 and later allows that status into the terminal result. Loss of authoritative Docker observation makes complete delivery unverifiable, so the design requires the recording/output finalization to fail; otherwise a Docker outage can produce a result falsely claiming that every byte was delivered. Reject drained for this cause when the loss preceded finalization.
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: 94d763adc9
ℹ️ 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 machine.cause == CauseRuntimeObservationLostV1 && observation.WorkloadOutputFinalizationStatus.Kind == WorkloadOutputFinalizationDrainedV1 { | ||
| return transition, fmt.Errorf("%w: runtime observation loss requires failed workload output finalization", ErrObservationRejected) |
There was a problem hiding this comment.
When host cancellation, controller termination, or workload exit has already latched the first cause, a subsequent ObservationRuntimeObservationLostV1 before the output barrier leaves machine.cause unchanged, so this guard accepts drained. That reports every byte as delivered even though docs/CONTROLLED_SESSION_DESIGN.md lines 501-506 require failed finalization whenever a runtime error makes complete delivery unverifiable, irrespective of which termination cause won the race. The new cause-based guard is fresh evidence that the runtime-loss check still misses this distinct ordering; track whether runtime observation was lost separately from the immutable 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: 93b736e2d4
ℹ️ 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".
| machine.runtimeObservationLost = true | ||
| machine.latchLocked(CauseRuntimeObservationLostV1, &transition) |
There was a problem hiding this comment.
When Docker observation is lost after workload_outputs_finalized(drained) and the controller's complete request but before ObservationFinishedV1, this only records the boolean and attempts to latch an already-latched cause. The earlier drained-output guard has already run and no finalization wait remains, so ObservationFinishedV1 can still accept a result containing exit code 0, drained, completed, and successful cleanup, falsely presenting a valid session despite docs/CONTROLLED_SESSION_DESIGN.md:1093-1096 requiring observation loss to fail the recording and prevent the session from being accepted as valid. Fresh evidence beyond the prior wait and pre-barrier reports is this post-complete ordering, which those remedies do not cover.
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: b862992fe3
ℹ️ 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 := validateRuntimeObservationStatusV1(result.RuntimeObservationStatus); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When a terminated frame contains CauseRuntimeObservationLostV1 together with RuntimeObservationMaintainedV1 or drained workload output, every per-field check here succeeds. That combination cannot be produced by this lifecycle—this cause must be latched before the output barrier, where runtime loss requires failed output finalization—so a direct ResultV1 decoded through ReadEventV1 can be accepted with impossible success fields. Add cross-field validation for the runtime-loss cause.
Useful? React with 👍 / 👎.
| Cause TerminationCauseV1 `json:"cause"` | ||
| WorkloadStatus ProcessStatusV1 `json:"workload_status"` | ||
| WorkloadOutputFinalizationStatus WorkloadOutputFinalizationStatusV1 `json:"workload_output_finalization_status"` | ||
| RuntimeObservationStatus RuntimeObservationStatusV1 `json:"runtime_observation_status"` |
There was a problem hiding this comment.
This required terminal-result field changes the user-visible controlled-session wire contract and its documented semantics, but the commit adds no fragment under .changes/unreleased/. Without a fragment, the protocol change will be omitted from generated release notes; add an Added or Changed Changie fragment.
AGENTS.md reference: AGENTS.md:L9-L13
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: 9e8814155a
ℹ️ 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 := validateRuntimeObservationStatusV1(result.RuntimeObservationStatus); err != nil { | ||
| return err | ||
| } | ||
| if result.Cause == CauseRuntimeObservationLostV1 { |
There was a problem hiding this comment.
In the ReadEventV1 path, this cross-field validation only covers runtime-observation loss, so a directly decoded result with CauseControllerLostV1 plus ControllerFinalizationCompletedV1 (or CauseStartupFailureV1 without ControllerFinalizationStartupFailedV1) is accepted. Those combinations cannot be produced by MachineV1: controller loss records lost, startup failure records startup-failed, and validateControllerFinishLocked prevents either status from being rewritten. Validate these cause/status pairs here as well so malformed terminal frames cannot represent impossible lifecycle outcomes.
Useful? React with 👍 / 👎.
Add the concurrency-safe host-owned lifecycle machine for preparing, active, terminating, and terminated sessions. Latch the first accepted termination cause, enforce granted controller requests, preserve explicit completion across later disconnects, permit bounded controller finalization after application exit, and prevent terminal status reports from rewriting observed outcomes. Reject startup failures after controller activation. Ensure host cancellation, runtime-observation loss, and controller-requested termination end a pending controller-finalization wait without rewriting the already-latched cause. Add lifecycle race, authorization immutability, disconnect, timeout, idempotence, abortive-finalization, startup-boundary, and hostile-result coverage. Clarify controller-requested termination as a distinct design cause.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! 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". |
Add the concurrency-safe host-owned lifecycle machine for preparing, active, terminating, and terminated sessions. Latch the first accepted termination cause, enforce granted controller requests, preserve explicit completion across later disconnects, permit bounded controller finalization after application exit, and prevent terminal status reports from rewriting observed outcomes.
Reject startup failures after controller activation. Preserve an active bounded controller-finalization wait across repeated controller termination, host cancellation, and runtime-observation-loss signals without rewriting the already-latched cause. Skip that wait when the controller was not granted the
completeoperation, allowing the terminal result to recordnot-completedimmediately.When runtime-observation loss occurs before workload output finalization, track it independently from the first latched termination cause, reject a later
drainedclaim, and require explicit failed finalization because complete delivery is no longer verifiable.Add lifecycle race, authorization immutability, disconnect, timeout, idempotence, bounded-finalization, runtime-loss output integrity, startup-boundary, no-complete authorization, and hostile-result coverage. Clarify controller-requested termination as a distinct design cause.
Stack created with Sapling. Best reviewed with ReviewStack.