Skip to content

fix(native): remove deadlock detection - #3166

Merged
kixelated merged 1 commit into
mainfrom
codex/remove-deadlock-detection
Aug 29, 2026
Merged

fix(native): remove deadlock detection#3166
kixelated merged 1 commit into
mainfrom
codex/remove-deadlock-detection

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

  • remove the debug-only deadlock checker and its process-lifetime thread
  • replace the remaining low-frequency parking_lot::Mutex with std::sync::Mutex, removing moq-native’s direct parking_lot dependency
  • stop enabling parking_lot/deadlock_detection across release builds; Cargo feature unification compiled its per-lock tracking even though #[cfg(debug_assertions)] prevented the checker from running

Fixes #3125.

Public API changes

None.

Test plan

  • nix develop --command just fix
  • nix develop --command just check
  • nix develop --command just test (3264 passed, 1 skipped)
  • inspected cargo tree --locked -p moq-relay -e features -i parking_lot_core to confirm only parking_lot_core/default remains enabled

Cross-package sync is not applicable because this changes neither a public API nor a wire format.

(Written by Codex)

Co-authored-by: Codex <codex@openai.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 29, 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-29T04:32:43.081704Z a595872 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.

@kixelated
kixelated enabled auto-merge (squash) August 29, 2026 04:30

@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: a595872233

ℹ️ 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 rs/moq-native/Cargo.toml
@@ -54,7 +54,6 @@ moq-net = { workspace = true }
# congestion configs. Version matched to the copy iroh pulls in.
noq-proto = { version = "1", default-features = false, optional = true }
notify = { version = "8", optional = true }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Add a regression check for the disabled feature

This bug fix has no automated check that parking_lot/deadlock_detection remains absent from the unified release feature graph. A later dependency change can silently re-enable the per-lock tracking that this commit removes while all behavioral tests still pass, so encode the inspected cargo tree invariant in a regression check that fails when the feature returns. (Written by GPT-5.6 Sol)

AGENTS.md reference: AGENTS.md:L141-L141

Useful? React with 👍 / 👎.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: da4ab485-0326-4bb3-9880-1f3834ecf05a

📥 Commits

Reviewing files that changed from the base of the PR and between 85c9f48 and a595872.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • rs/moq-native/Cargo.toml
  • rs/moq-native/src/accept.rs
  • rs/moq-native/src/log.rs
💤 Files with no reviewable changes (2)
  • rs/moq-native/src/log.rs
  • rs/moq-native/Cargo.toml

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


Walkthrough

The native module removes the parking_lot dependency and its deadlock detection feature. The accept health state changes to std::sync::Mutex, with lock results unwrapped at each access. The logging initialization no longer starts the debug-only deadlock detector, and the detector implementation is removed.

Merge Risk: 🔵 Low · up to a5958

The change removes deadlock-detection overhead and switches native health-state locking to the standard library without changing public APIs or wire formats. It is mergeable with owner awareness because a panic while holding the health-state lock could poison it and cause later lock attempts to panic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The reviewable changes satisfy the code-related objectives in issue #3125 by removing deadlock detection and the direct parking_lot dependency. Cargo.lock regeneration and removal of the backtrace dep… Review Cargo.lock outside the filtered file set. Confirm it was regenerated and that the parking_lot deadlock_detection and backtrace dependency edges are removed as required by issue #3125.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: removing native deadlock detection.
Description check ✅ Passed The description directly explains the deadlock checker removal, mutex replacement, dependency change, testing, and linked issue.
Out of Scope Changes check ✅ Passed All reviewed changes support the linked issue. The mutex replacement removes the remaining dependency usage, and the log changes remove the unused deadlock checker and its thread.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files.
Full details: Linked Issues check

Explanation

The reviewable changes satisfy the code-related objectives in issue #3125 by removing deadlock detection and the direct parking_lot dependency. Cargo.lock regeneration and removal of the backtrace dependency edge cannot be verified because Cargo.lock is excluded by the !**/*.lock path filter.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch codex/remove-deadlock-detection

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.

@kixelated
kixelated merged commit eb5776e into main Aug 29, 2026
5 checks passed
@kixelated
kixelated deleted the codex/remove-deadlock-detection branch August 29, 2026 04:37
This was referenced Aug 29, 2026
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.

moq-tokio: parking_lot deadlock_detection is compiled into every release build

1 participant