fix(e2e): retry serialized portable lock loser

[prekshivyas]

Outcome

Concurrent OpenClaw MCP discovery now accepts the serialized portable-host lock loser only after the competing command has committed one coherent bridge, then retries that rejected command once and requires the duplicate to be rejected as already present. This removes the deterministic Images workflow failure introduced with portable-host fencing while preserving fail-closed behavior for unknown errors.

Reason

Both OpenClaw MCP discovery jobs in Images runs 34544156685 and 34545930022 failed after one concurrent mcp add succeeded and the other exhausted the .nemoclaw-portable-host.lock retry budget. The test helper recognized only the existing Hermes restart transport signature, so it misclassified this expected serialized loser even though status proved the winner committed successfully.

Changes

• Recognize only the exact portable-host lock timeout, alongside the already-reviewed Hermes restart transport signature.
• Require post-race status verification of the committed bridge before permitting one retry of the rejected command.
• Keep already-present duplicates unchanged and reject every unknown lock or transport diagnostic.
• Add focused regression tests and document the concurrent-add retry contract.

Verification

• npx vitest run --project e2e-support test/e2e/support/mcp-bridge-reliability.test.ts — passed, 48/48 tests.
• npm run e2e:assertions:check — passed, 1,796 direct assertions across 86 files.
• npm run test:e2e-phases:check — passed, 135 tests across 88 files.
• NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli — passed.
• npm run checks:repository — passed.
• NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr — passed, including the repository-wide CLI typecheck. An initial run exhausted Node's default 4 GB local heap during that typecheck; no code or test assertion failed.
• git diff --check origin/main...HEAD and the pre-commit secret scan — passed; the diff contains no secrets, API keys, or credentials.

Review notes

This is intentionally limited to E2E reconciliation and classification. It does not change production locking behavior. Real-run evidence is the identical two-job failure in merged-main Images run 34544156685 and OpenShell PR Images run 34545930022.

---

Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability when concurrent operations encounter temporary lock contention or service restarts.
  • Preserved validation that successful operations create only one coherent result and reject duplicates.

• Tests

  • Expanded coverage for retry behavior, bridge verification, unknown failures, and unverified results.
  • Added isolated probing for concurrent-operation scenarios.

• Documentation

  • Documented the concurrent-operation reliability probe and its accepted transient failure conditions.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: e7789c3e-1210-447f-b9f7-e6c44b4fec95

📥 Commits

Reviewing files that changed from the base of the PR and between 74c628e and 37b642a.

📒 Files selected for processing (4)

• test/e2e/README.md
• test/e2e/live/mcp-bridge-reliability.ts
• test/e2e/live/mcp-bridge.test.ts
• test/e2e/support/mcp-bridge-reliability.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

The concurrent-add retry flow now recognizes portable-host lock contention and Hermes restart transport failures after committed-bridge verification. E2E and support tests use the renamed helper and cover accepted, rejected, and unverified failure cases.

Changes

Concurrent add retry

Layer / File(s)	Summary
Retry classification and helper flow test/e2e/live/mcp-bridge-reliability.ts	The helper accepts recognized portable-host lock contention and Hermes restart transport failures after verifying the committed bridge.
Integration and reliability validation test/e2e/live/mcp-bridge.test.ts, test/e2e/support/mcp-bridge-reliability.test.ts, test/e2e/README.md	Tests use the renamed helper and cover duplicate results, transient retries, unknown failures, unrelated lock failures, and pre-verification failures. The README documents the retry behavior.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: senthilr-nv, cv

Sequence Diagram(s)

sequenceDiagram
  participant ConcurrentAddTest
  participant RetryHelper as retryAfterConcurrentAddTransientFailure
  participant BridgeVerification as committed bridge verification
  ConcurrentAddTest->>RetryHelper: submit concurrent add result
  RetryHelper->>BridgeVerification: verify committed bridge
  BridgeVerification-->>RetryHelper: committed bridge status
  RetryHelper-->>ConcurrentAddTest: retry recognized transient failure or return duplicate result

Loading

Merge Risk: ⚪ Minimal · up to 37b64

The concurrent-add test flow now handles the expected lock-contention failure without expanding retry behavior beyond the verified scenario.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: retrying the serialized portable lock loser in E2E tests.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/mcp-concurrent-portable-lock-retry

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 37b642a in the fix/mcp-concurrent-p... branch remains at 96%, unchanged from commit 74c628e in the main branch.

[prekshivyas]

validation for 37b642a42365354765e280a5d589fd6d7592e31a is complete.

• Images run 34550439884 passed end to end.
• OpenClaw MCP discovery pass 1 and pass 2 both passed. These are the two jobs that failed with the exact portable-host lock-loser signature in merged-main run 34544156685 and OpenShell PR run 34545930022.
• All-agent managed runtime activation passed, as did every image build, publication, contract, and direct-start prerequisite.
• CodeRabbit reviewed the exact head with no actionable comments and minimal merge risk.
• NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr passed locally, including the full CLI typecheck.

CI run 34550439932 remains red only for current-main failures outside this four-file diff:

• shard 2: Portable host authority mutation requires the current HOME fence at runtime-provider-contract.test.ts:1131;
• shard 5: the same inherited fix(portable): unify lifecycle lock authority #11490 assertion at podman.test.ts:288;
• shard 8 initially hit the untouched test(e2e): prove staging Brev exec readiness #11301 one-second diagnostic-budget timing assertion; its one authorized same-pipeline retry passed. Shards 2 and 5 reproduced identically on that attempt and were not retried again.

The exact candidate behavior owned by this PR is green; no remaining failure is caused by the retry-classification change.