Skip to content

test(cli): let the forged sibling-env start bind its own port - #7740

Merged
lidge-jun merged 2 commits into
devfrom
fix/windows-sibling-start-port-race
Oct 26, 2026
Merged

lidge-jun merged 2 commits into
devfrom
fix/windows-sibling-start-port-race

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 26, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fixes the only failure in dev Cross-platform CI run 40183909309 (windows 9/9, job 121140589969): a direct start cannot claim sibling status with only a forged port env timed out with "timed out waiting for sibling runtime record" while the child ocx start was still alive.

Root cause is a port race in the test, not a product regression. freeLoopbackPort() releases its probe socket before the child is spawned, and the child is hard-pinned with --port. If another process on the runner (Windows shards run four Bun pools) takes that port in between, the pinned start waits in port reclaim for up to 60 s (src/cli/index.ts port selection → reclaimListenPort, then a further 5 s availability wait) and only publishes its PID/runtime record after binding (src/cli/start-ownership-publication.ts). The 45 s CI deadline (watchdogMs(10_000) on win32) expires first, leaving exactly the observed state.

The forged-env case checks sibling admission, so it does not need a predetermined listener port. It now starts without --port and binds its own port through the fixture's config.port: 0. startSibling accepts an unpinned start and takes the bound port from the runtime record. Every pinned caller keeps the exact port-equality check, and the PID, readiness PID, ready status and absent siblingOfPort assertions are unchanged. No timeout was raised and no retry was added.

When the record never appears, the error now includes the requested port, the child PID, the last record read, and stdout/stderr tails, so a future stall can be diagnosed from the CI log.

Verification

Evidence for the root cause, from a separate investigation at dev 8ebe46b with the pinned Bun 1.4.4:

  • Holding the selected child port: at 45.01 s exitCode=null runtime=null pidFile=false. After the port was released at 48 s, a matching record appeared at 50.15 s. With the port held permanently, the child logged "still busy after prefer-retry; refusing ephemeral fallback" at 67 s and exited 1 at 72 s.
  • Free port, forged marker: record at 3.4 s. An unpinned start while the old port was still held published a different valid port in 4.5 s.
  • Ruled out: the forged OCX_SIBLING_OF_PORT probe blocking. honorSiblingMarker strips both env fields and consumeSiblingHandoff returns false immediately without a nonce.

On this branch (Windows, Bun 1.4.4):

  • bun test --isolate tests/cli/cli-start-journal-order.test.ts: 6 pass / 0 fail (forged case 4.2 s)
  • bun run typecheck: pass
  • bun run privacy:scan: pass
  • git diff --check: clean

The same release-then-pin pattern exists at other pinned call sites in this file and in sibling-home-client-sync, ocx-launcher-runtime and cli-start-auxiliary-bind. Those cases assert on a specific port, so they are left unchanged here.

NOT RUN: full bun run test (test-only change to one file); Windows Cross-platform CI on this head (manual dispatch after merge, per CI policy).

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (Test-only; none needed.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Tests
    • Expanded CLI startup coverage for explicitly selected and configured ports. Tests verify that reported ports are valid and readiness checks use the port reported at runtime. They also check that failures to receive a runtime record include launch and output details and stop a child process that is still running. The forged-sibling test now uses its configured port.

freeLoopbackPort() releases its probe before the pinned child binds; a port taken in between makes the pinned start wait out port reclaim past the 45 s Windows deadline with no runtime record (dev run 40183909309). The forged-env case checks sibling admission, not a port number, so it now binds through config.port 0. Timeouts report the child's output and last record.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 26, 2026 04:50
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T04:51:58.044312Z fa7eda4 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Oct 26, 2026
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between fa7eda4 and 0df8551.

Files selected for processing: 1
  • tests/cli/cli-start-journal-order.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The CLI startup tests allow a sibling process to use its configured port. They validate the runtime-record port, use it for readiness checks, and include process and output details when waiting for the record fails.

Changes: Sibling process startup

Layer / File(s) Summary
Runtime port selection and readiness
tests/cli/cli-start-journal-order.test.ts
startSibling accepts an optional port and validates the runtime-record port. Readiness checks use the reported port. If waiting for the record fails, the helper kills a running child and includes launch details and bounded output tails in the error. The forged-port test lets the child use its configured port.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: Minimal · up to 0df85

The sibling-port test change is mergeable; no concrete current-head risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title accurately describes the main change in tests/cli/cli-start-journal-order.test.ts: the forged sibling environment starts without a pinned port and binds to its configured port.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…safe

Log only allowlisted runtime-record fields (the record carries the per-process attestationSecret), rethrow an early-exit error unchanged since its stderr is already consumed, and catch synchronous Response construction failures in the bounded tail reader.
@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer dev integration record (release 2.93 dev-merge loop), per the MAINTAINERS.md maintainer-integration exception for dev. This is maintainer integration, not an approving review.

  • Exact head: 0df8551; test-only change to tests/cli/cli-start-journal-order.test.ts.
  • Required CI on the exact head: PR CI run 40271854920 (ci, gates, test 1/4–4/4: success), enforce-target rerun 40272116150: success, hygiene 40272116141: success.
  • Windows-sensitive: Cross-platform CI lane=all dispatched on the branch at this head, run 40271853253 — windows 1/9–9/9, macos 1/3–3/3, macos control, macos widget + bundle, test 1/4–4/4, ci: success.
  • Independent technical review (separate agent): round 1 FIX_THEN_MERGE (2 medium: runtime record attestationSecret in the timeout diagnostic; consumed-stream error masking an early exit), both fixed in 0df8551; round 2 MERGE, 0 blocking, with probes confirming the secret is absent, the early-exit message is preserved and stream reads are bounded.
  • Root-cause evidence and the original failure (dev run 40183909309, windows 9/9) are in the PR description. The failed job's rerun at the same SHA passed, and dev lane=all 40271254814 at 8e18e60 was green before this change; this PR removes the port race from the forged-env case rather than relying on it not recurring.

@lidge-jun
lidge-jun merged commit 415078c into dev Oct 26, 2026
65 of 67 checks passed
@lidge-jun
lidge-jun deleted the fix/windows-sibling-start-port-race branch October 26, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant