Skip to content

fix(test): isolate concurrent interop harness runs - #4228

Merged
kixelated merged 5 commits into
mainfrom
quest/m1/interop-flakes
Sep 27, 2026
Merged

kixelated merged 5 commits into
mainfrom
quest/m1/interop-flakes

Conversation

@kixelated

@kixelated kixelated commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Concurrent Nix shells reserved the same relay port because each kept its reservations under its private TMPDIR, so a second just test interop in another worktree saw port 4500 already serving and failed immediately. The browser pause click could hit the canvas after the controls auto-hid. Same-checkout runs also clobbered Go staging output.

Approach

  • Keep per-user port claims under /tmp/moq-test-ports-<uid> regardless of TMPDIR; refuse a reservation root that is a symlink or owned by someone else.
  • Activate the real pause button with Enter instead of a pointer click.
  • Stage Go modules inside each run directory.
  • just test harness adds regressions for separate-TMPDIR shells, pinned-port contention, symlink refusal, and canvas-covered controls; the Interop workflow runs it. The browser check is harness.browser.ts, not *.test.ts, since plain bun test runs have no Playwright Chromium.
  • Completes the interop-flakes quest.

Impact

  • Public API: none.
  • Wire: none.
  • Harness reservations remain scoped to one user; different users need disjoint ports.

Alternatives

Binding both listeners to port 0 does not keep UDP QUIC and TCP HTTP/WebSocket on the same port required by the harness URL.

An earlier revision ran two full matrices concurrently in one CI checkout. That clobbers the shared Python venv and maturin output, and doubles load on one runner, so CI runs a single matrix plus the deterministic port regression instead.

Follow-ups

  • Under heavy contention (two matrices on one machine), a browser subscriber sometimes stops progressing (python -> js, go -> js: the Pause control never appears within 30s). Not seen in a single run; worth a quest if it shows up nightly.

🤖 Generated with Claude Code

(Written by GPT-6, updated by Claude Opus 5.5)

Run one full matrix in CI instead of two concurrent ones in the same
checkout (they clobber each other's Python venv and maturin output),
keep the Playwright check out of the workspace bun test, and complete
the quest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review September 27, 2026 02:39
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 776aabde-4454-43f0-a25d-cc2ed06889ba

📥 Commits

Reviewing files that changed from the base of the PR and between 500517f and c3d439a.

📒 Files selected for processing (13)
  • .github/workflows/interop.yml
  • quest/m1/README.md
  • quest/m1/interop-flakes.md
  • test/README.md
  • test/interop/clients/js/driver.ts
  • test/interop/clients/js/harness.browser.ts
  • test/interop/clients/js/harness.ts
  • test/interop/clients/js/media.ts
  • test/interop/clients/js/tsconfig.json
  • test/interop/interop.sh
  • test/justfile
  • test/lib/harness.sh
  • test/lib/harness.test.sh

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.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 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-09-27T03:48:36.922170Z c3d439a New commits
ℹ️ 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.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Landing summary:

  • Merged main twice (workflow conflict with the new bare-fin step, then test(interop): reattach clears the stale canvas frame #4308's reattach fix).
  • CI now runs one full matrix plus just test harness. The two concurrent matrices shared one checkout and clobbered each other's Python venv and maturin output, which is what broke Interop.
  • Renamed the Playwright check to harness.browser.ts so the workspace bun test (no Chromium) skips it; that was the Test failure.
  • Completed and removed the interop-flakes quest.

Confirmed this fixes the parallel-worktree case: reservations now share /tmp/moq-test-ports-<uid> across Nix shells, so a second run walks to 4501 instead of reserving 4500 and then finding it already served.

(Written by Claude Opus 5.5)

@kixelated
kixelated enabled auto-merge (squash) September 27, 2026 03:44

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c3d439a833

ℹ️ 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".

Comment thread test/interop/interop.sh
echo "building go client (workspace moq-go via uniffi-bindgen-go)..."
local staged ffi_pkg wrapper_pkg src="$HARNESS_RUN/go-client"
if ! staged=$(bash "$WORKSPACE/go/scripts/stage.sh" 2>"$HARNESS_RUN/go-stage.log"); then
if ! staged=$(bash "$WORKSPACE/go/scripts/stage.sh" --output "$HARNESS_RUN/go-stage" 2>"$HARNESS_RUN/go-stage.log"); then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep Python staging isolated before closing the quest

When two full matrices run from the same checkout, this change isolates only Go: prepare_python still runs just py build, whose uv sync, maturin develop, and editable install concurrently rewrite the same $WORKSPACE/.venv and generated Python bindings. Those runs can therefore still clobber each other even though this commit deletes the concurrency quest; move the Python environment and generated output under HARNESS_RUN, or keep the quest open until that remaining work is tracked and tested.

AGENTS.md reference: quest/AGENTS.md:L97-L98

Useful? React with 👍 / 👎.

Comment thread test/justfile
# The browser check is not `*.test.ts`: plain `bun test` runs lack Playwright Chromium.
harness:
./lib/harness.test.sh
bun test ./interop/clients/js/harness.browser.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Install browser prerequisites in the harness recipe

In a clean checkout, just test harness reaches this command without installing workspace dependencies or Playwright Chromium, so it fails to resolve playwright or launch the configured Chromium channel. The preceding Playwright step in .github/workflows/interop.yml happens to prepare CI, but the newly advertised local recipe is not independently runnable; provision the same prerequisites here or invoke an existing setup path before the browser test.

Useful? React with 👍 / 👎.

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.

1 participant