Skip to content

fix(tui): report Runtime Chat owner-lock IO faults truthfully - #5736

Merged
Hmbown merged 2 commits into
mainfrom
fix/relay-lock-error-message-20260830
Aug 30, 2026
Merged

fix(tui): report Runtime Chat owner-lock IO faults truthfully#5736
Hmbown merged 2 commits into
mainfrom
fix/relay-lock-error-message-20260830

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Refs #5735.

The RuntimeChatRelayHost::open lock path mapped every acquire failure to "another process owns this session", discarding the underlying io error — which is why the flaky Safety-gate failure said nothing actionable. Only WouldBlock is genuine contention; other failures now report themselves.

Verified: cargo test -p codewhale-tui --lib -- runtime_chat_relay16 passed / 0 failed, including the genuine-contention test (message unchanged for that path).

This is the diagnosis-enabling half of #5735; the flake itself stays open until a run reports the real errno.

No-Issue: partial fix for tracked flake #5735 — does not close it.

The open path mapped every lock-acquire failure to 'another process owns
this session', discarding the underlying io error. Only WouldBlock is
genuine contention; anything else (filesystem fault, permission error) is
now reported as itself. The next occurrence of the #5735 flake will say
what actually failed instead of a blanket ownership claim.

Verified: cargo test -p codewhale-tui --lib -- runtime_chat_relay —
16 passed, 0 failed (incl. the genuine-contention test, message unchanged).

Refs #5735

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 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-08-30T13:19:58.013249Z 1b73d77 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.

@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 3m 4s —— View job


Reviewing PR #5736

  • Gather context (PR body, diff)
  • Read changed file crates/tui/src/runtime_chat_relay.rs
  • Review diff for correctness / safety / reuse / tests
  • Post review feedback

Summary

Good, narrowly-scoped fix. RelayScopeLock::acquire returns anyhow::Result<Self>, and anyhow::Error::downcast_ref correctly walks through the .context(...) wrapper added around the flock/LockFile call (crates/tui/src/runtime_chat_relay.rs:1565-1568) back to the original std::io::Error, so the WouldBlock check at runtime_chat_relay.rs:263-265 genuinely distinguishes real lock contention from other IO faults (permission denied, symlink refusal, non-regular-file, etc.) on the Unix path. Non-contention failures now surface the real cause via {error:#} instead of a misleading "another process owns this session" message — this directly matches the PR's stated goal and the linked #5735 flake symptom.

Findings

  1. Possible platform gap — Windows lock contention may misclassify (runtime_chat_relay.rs:263-272, feeds from the LockFile failure path at runtime_chat_relay.rs:1570-1578). The WouldBlock check assumes the OS error from a failed lock maps to ErrorKind::WouldBlock. That mapping is reliable for the POSIX EWOULDBLOCK/EAGAIN case from flock, but on Windows a LockFile failure (ERROR_LOCK_VIOLATION/ERROR_SHARING_VIOLATION) is not one of the OS codes the standard library classifies as WouldBlock — it typically decodes to Other/Uncategorized. If that's correct, genuine lock contention on Windows would now fall into the new "IO fault" branch instead of the friendly ownership message, a behavior change on a platform this code explicitly supports (#[cfg(windows)]). Worth double-checking on a Windows target, or handling the Windows error code explicitly alongside WouldBlock.
    Fix this →

  2. Test coverage gap for the new branch. The diff adds no test that exercises the non-WouldBlock path. The existing test (scoped_state_is_exclusive_account_bound_and_restart_stable, runtime_chat_relay.rs:2007-2046) only asserts RelayScopeLock::acquire(...).is_err() on double-acquire — it never checks message text, and there's no test at all for a genuine IO fault (e.g., permission denied, or the lock path being a directory) going through RuntimeChatRelayHost::open. A regression in the downcast/WouldBlock logic (including finding Fix cargo fmt formatting issues #1) wouldn't be caught by the current suite.
    Fix this →

No provider/model-routing code is touched by this change, and no reuse/duplication or security issues (secrets, exec, input validation) were introduced — the change is a straightforward error-mapping fix confined to one function.

Note: I was not able to run cargo test/cargo build in this sandbox (Bash approval was required and unavailable for those commands), so I relied on static analysis of the diff and anyhow's documented context-chain downcasting semantics rather than executing the test suite myself. The PR body's reported cargo test -p codewhale-tui --lib -- runtime_chat_relay result (16 passed / 0 failed) was not independently re-verified here.

@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: 1b73d7700b

ℹ️ 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 on lines +263 to +265
let contention = error
.downcast_ref::<std::io::Error>()
.is_some_and(|io| io.kind() == std::io::ErrorKind::WouldBlock);

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 Treat Windows lock violations as contention

On Windows, a contended LockFile reports raw OS error 33 (ERROR_LOCK_VIOLATION), rather than ErrorKind::WouldBlock, so a second Codewhale process now receives the new local-I/O-fault message instead of the established ownership message. The analogous process lock in crates/tui/src/runtime_threads.rs:3037-3040 explicitly classifies raw errors 32 and 33 as contention; apply the same platform-specific classification here.

Useful? React with 👍 / 👎.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@claude

claude Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 0s —— View job


Reviewing PR #5736 (re-review after new commit 0f3c2f6)

  • Gather context (PR body, diff, prior review comments)
  • Check whether the Windows lock-contention gap was addressed
  • Read current diff vs origin/main
  • Post review feedback

Summary

The new commit (0f3c2f6) is rustfmt-only — no functional change since the previous review. The core fix (crates/tui/src/runtime_chat_relay.rs:256-272) is still sound for the Unix path: flock's EWOULDBLOCK/EAGAIN reliably decodes to std::io::ErrorKind::WouldBlock, so genuine contention still gets the friendly ownership message there, and other IO faults now surface their real cause instead of being papered over — directly fixing the diagnosis gap for #5735.

Outstanding issue (not yet fixed)

Windows lock contention will be misclassified as an IO fault (crates/tui/src/runtime_chat_relay.rs:263-265, sourced from the LockFile failure at runtime_chat_relay.rs:1570-1578). A contended LockFile call reports raw OS error 33 (ERROR_LOCK_VIOLATION, sometimes 32/ERROR_SHARING_VIOLATION), which the standard library does not map to ErrorKind::WouldBlock — it decodes as Other/Uncategorized. The downcast_ref::<io::Error>().is_some_and(|io| io.kind() == WouldBlock) check at line 263-265 will therefore be false for real Windows contention, and a second Codewhale process will now see "Runtime Chat could not take its owner lock: ..." instead of the established "Another Codewhale process already owns this Runtime Chat account session." message.

This exact class of bug already has a fix in this codebase: crates/tui/src/runtime_threads.rs:3037-3040 explicitly checks matches!(error.raw_os_error(), Some(32 | 33)) alongside the Unix WouldBlock check for the analogous process lock. The same platform-specific classification should be applied here so Windows behavior isn't regressed by this PR.

Fix this →

Minor: still no regression test for the new IO-fault branch

As noted previously, there's still no test exercising a non-contention lock failure through RuntimeChatRelayHost::open (e.g. lock path being a directory, or permission denied on Unix) to assert the new "Runtime Chat could not take its owner lock: ..." message and confirm the WouldBlock message is unchanged for genuine contention. Not blocking given the PR's stated scope as a partial fix for #5735, but would guard against regressing the branch this PR just added.

Other notes

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