test(live): stabilize close callback synchronization - #1145
Conversation
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. |
|
@codex review pls |
|
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 91 mixed files remain; 0 existing customizations changed. Compared 91 existing customizations unchanged
51 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 37350518845 --repo openai/openai-java \
--name castiron-custom-code-37350518845-1 --dir /tmp/castiron-custom-code-37350518845-1
git apply --stat /tmp/castiron-custom-code-37350518845-1/custom-code.patch
cat /tmp/castiron-custom-code-37350518845-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin c50d94b56e76317888b7018fd51c6704c89865b5 2eddb49d20bda4056af094dff552bd456ee1c1b2
python3 scripts/castiron/custom_code_report.py report \
--base c50d94b56e76317888b7018fd51c6704c89865b5 \
--head 2eddb49d20bda4056af094dff552bd456ee1c1b2 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-2eddb49d20bd
cat /tmp/castiron-custom-code-2eddb49d20bd/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 2eddb49d20bda4056af094dff552bd456ee1c1b2. No actionable findings.
The callback now waits until the test releases it, even if CI pauses the test thread. Running push() on a worker lets the test proceed when the timer is delivering the callback. The test still checks that close() waits and that the final transcript arrives before it returns. Cleanup always releases the callback. These changes fit the existing test structure without changing production code.
Source review only. Hosted build, lint, tests, compatibility, and Castiron checks pass on this commit. The delay probe, repeated runs, and early-return mutation are author-reported; I did not run them locally.
2 participants
Summary
Keep the transcript close test's callback blocked until the test explicitly releases it. Its previous 10-second timeout could expire while the test thread was delayed, causing a correct
close()to return and fail the assertion that it was still waiting. This reproduces the failure that removed #1144 from the merge queue.Run
push()on a worker too: it can wait for the timer-dispatched callback. The test still checks thatclose()blocks before release and delivers the final transcript before returning, with bounded worker waits and unconditional callback release during cleanup.Reproduced the original failure by pausing the test thread for 11 seconds after callback entry; the revised test passes the same schedule. It also catches a temporary mutation that makes
close()return early, passes 100 consecutive executions, and passes all 37 transcript tests. The delay and mutation are local probes, not committed changes. No production behavior changes.