Skip to content

Stabilize Windows PTY and login test timing (VibeTV #336) - #20

Draft
marcus7989 wants to merge 2 commits into
vibetvfrom
codex/windows-flakes
Draft

marcus7989 wants to merge 2 commits into
vibetvfrom
codex/windows-flakes

Conversation

@marcus7989

@marcus7989 marcus7989 commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

[K-Marcus] Windows PTY/login fixtures raced PowerShell startup against test budgets. Use the native test executable for the real PTY descendant, and reap login fixtures before wait_for_login(..., Duration::ZERO). The fixture wait is now bounded to 60 seconds: a stalled child is terminated and produces a clear timeout error, addressing Paul's counter-review. A new test verifies timeout and child termination.

Only #[cfg(test)] code in rust/src/managed_process.rs and rust/src/providers/claude/accounts/login.rs changes. Product code, the 15-second descendant marker wait and 10-second shutdown/cleanup limits are unchanged. Live descendant/Job Object assertions, isolated account identity, failed exit code 1 and its error message remain checked.

Validation for exact PR head 762beabcb4d817e07de7e807d6cb5b6124279e21 together with prerequisite #21 c8e11559ed78df845a1ee732e511025b458d1320:

Paul's counter-review was on ba43f0d and had no blocker; his unbounded-wait note is fixed here. Codex review requested on this new head, still pending. Keep Draft; no merge approval. Actual post-integration full-suite rerun is prepared and remains required after #21/#20 are integrated. No devices, merge, release or .16 pin changes.

Additional repository-required native Desktop-shell unit suite on unchanged cumulative source ef4aace55a59ed61fc66ab9b7d341129b82afa69: run 40908176267, 431 passed / 0 failed / 0 ignored / 0 filtered. Combined backend, CLI and Desktop-shell total: 2,481 PASS / 0 FAIL / 1 pre-existing ignored, same exact cumulative source. Both Rust manifests are verified; no app/device/customer-flow test is claimed.

The current-head Codex request has now had at least 20 minutes without a bot review, inline finding or bot reaction. Review remains pending; no automated no-findings or merge readiness is claimed.

@DreamyTalesPAN DreamyTalesPAN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counter-review of #20 at head ba43f0d2113aeb6ded6490399eff729fd770bd4c (read-only review, nothing built or pushed on my side).

Verdict: correct, test-only, no hiding of real failures. No blocker. Two non-blocking notes.

  • Product code untouched. In rust/src/managed_process.rs all hunks are inside mod tests (opens at line 829, hunks start at 1056). In login.rs the test module opens at 377, the hunk is at 582. Production prefixes are unchanged in both files.
  • Timeouts not loosened. The 15 s marker wait and the 10 s shutdown/cleanup limits stay as they were. The login test no longer races a 10 s budget against PowerShell startup; it reaps the fixture first and then calls wait_for_login(..., Duration::ZERO). That is valid because wait_for_login calls try_wait() before it checks the timeout, so an already-reaped child takes the exit-status path, not the timeout path.
  • Assertions are stronger, not weaker. The failed fixture must now exit with code Some(1) and the error must contain exit code 1 (matches LoginFailure::CliExit). The running-process timeout/cancel path is still covered by the untouched cancelled_and_timed_out_logins_reap_child.
  • PTY containment test still tests the real thing. It still spawns a real descendant, checks it is alive, runs the real Job Object shutdown(10 s) and asserts the descendant dies. Using the native test executable instead of two PowerShell starts removes only the startup cost. The new fixture test is a no-op unless CODEXBAR_MANAGED_DESCENDANT_MARKER is set, so it is harmless in a normal suite run. The new early-exit assert and the richer timeout message are an improvement.
  • Non-blocking 1: child.wait() in the login test has no deadline, so a hung PowerShell would hang the test until the job timeout instead of failing with a message. Acceptable for a test-only change, but worth knowing.
  • Non-blocking 2 (evidence): this PR has no CI run on its own head (no checks reported). The runtime evidence is from VibeTV nesszer#336, where the same patch was applied to .16, and the PR text says so honestly. The files are byte-identical on the .15 base. The full suite stays red on restart_uses_manifest_gui_and_leaves_standalone_cli_alone until #21 lands, which fits your merge order #21 → #20. After #21 is merged, please check one full-suite run on the vibetv result.

Not checked: a real-Windows run on my side; I relied on the linked runs.

@marcus7989

Copy link
Copy Markdown
Owner Author

[K-Marcus] @codex review

W307 addressed Paul's unbounded child.wait() note from his ba43f0d counter-review. Exact new head 762beabcb4d817e07de7e807d6cb5b6124279e21 replaces both blocking fixture waits with try_wait() polling and a 60-second fixture-startup deadline. A stalled fixture is terminated and returns a clear TimedOut error. The new stalled-fixture test checks deadline failure and that the killed child can be reaped within five seconds.

This budget applies only before wait_for_login(..., Duration::ZERO); production code and the existing 10/15-second cleanup/marker deadlines remain unchanged. Exit-zero, isolated credential identity, exact failed exit code 1 and exit code 1 error assertions remain. The earlier 17-second startup-delay proof still fits the fixture budget. Format/whitespace and byte-identical production-prefix checks passed.

Preparing native full library + CLI CI on an isolated #21 + this head source, plus the four-PR cumulative source. These are pre-merge test candidates, not target-branch merges. After #21 is actually integrated, one full suite on the actual vibetv result remains required; no device or merge action by W307.

@marcus7989

Copy link
Copy Markdown
Owner Author

[K-Marcus] W307 native full suite GREEN on isolated #21 + current #20 source e749083ad573fed8815c204c60e2a5824af944a0: 2,038 library + 1 CLI passed, 0 failed, 1 pre-existing ignored, 0 filtered. Run/job. All test files exactly match #21 c8e11559ed78df845a1ee732e511025b458d1320 and #20 762beabcb4d817e07de7e807d6cb5b6124279e21; production prefixes are unchanged. The stalled-fixture deadline and child-termination test passes. This does not fabricate a green run at standalone #20, where #21 is absent.

The post-integration suite is prepared: verify actual vibetv SHA, integrated #21/#20 with unchanged heads and identical file blobs, then dispatch the same full-suite workflow. No actual target merge or device action. Paul's old-head unbounded-wait note is fixed; Codex current-head request remains pending; 0 review threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants