code-mode: extend test coverage to lock in cell lifecycle#28468
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03eec933ea
ℹ️ 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".
bc4a140 to
80f8653
Compare
1a0b289 to
8df3577
Compare
| }; | ||
| if handle | ||
| .termination_requested | ||
| .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) |
There was a problem hiding this comment.
nit: if we're not actually protecting any data with synchronization points between these atomic ops, might as well use relaxed everywhere
| let code = r#"// @exec: {"yield_time_ms": 100} | ||
| text("phase 1"); | ||
| let code = r#"// @exec: {"yield_time_ms": 0, "max_output_tokens": 16} | ||
| for (let index = 0; index < 512; index++) { |
There was a problem hiding this comment.
nit: this seems like not many iterations, could this flake if timing gets weird in a cloud vm that's running tests?
There was a problem hiding this comment.
Bumped this to a much larger cycle, proved failure without and readded, agree that this is kinda "magic", there is a deterministic test to verify the behavior in the code-mode tests, but this tries to capture it at the suite level too.
8df3577 to
eac98a0
Compare
This PR establishes the intended behavior as an executable contract before a refactor of the cell runtime begins. It also fixes cases where a second observer or termination request could replace an existing response channel and leave the original caller unresolved.
Behavior codified