Skip to content

fix: race condition - URL not available before relay connection - #437

Open
lucksus wants to merge 2 commits into
mainfrom
fix/url-race-condition
Open

fix: race condition - URL not available before relay connection#437
lucksus wants to merge 2 commits into
mainfrom
fix/url-race-condition

Conversation

@lucksus

@lucksus lucksus commented Jan 15, 2026

Copy link
Copy Markdown
Collaborator

Waiting for the relay connection before completing initialization

While debugging ad4m integration with new Iroh transport, I saw calls to TxImp::url returning none because they happened to early in some cases. This wasn't the root cause of our issues. I don't know if it's fine without this fix in all cases, but it makes the initialization more deterministic and adds debug logs.

Summary by CodeRabbit

  • Bug Fixes
    • Transport initialization now waits (up to 30s) for the first listening URL to become available, failing with a clear timeout if not.
    • Added one-time signaling and improved logging around first URL readiness and subsequent address updates.

✏️ Tip: You can customize this high-level summary in your review settings.

@lucksus
lucksus requested a review from jost-s January 15, 2026 14:37
@cocogitto-bot

cocogitto-bot Bot commented Jan 15, 2026

Copy link
Copy Markdown

✔️ b075b1d - Conventional commits check succeeded.

@coderabbitai

coderabbitai Bot commented Jan 15, 2026

Copy link
Copy Markdown

Walkthrough

Adds a one-shot signal for the first discovered local URL during IrohTransport setup; watch_addr_task emits the signal on first URL discovery. IrohTransport::create waits up to 30 seconds for that signal and errors on timeout; logging around URL events was expanded.

Changes

Cohort / File(s) Summary
URL readiness signaling and initialization sync
crates/transport_iroh/src/lib.rs
Add a oneshot channel to signal first local URL readiness; watch_addr_task accepts an optional url_ready_tx and sends on first URL discovery; IrohTransport::create waits up to 30s for the signal and errors on timeout; atomic local-URL update and additional logging for URL changes and readiness events.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • veeso
  • matthme
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly aligns with the main change: fixing a race condition where URLs weren't available before relay connection initialization.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b075b1d and 0fcceb4.

📒 Files selected for processing (1)
  • crates/transport_iroh/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/transport_iroh/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cloudflare Pages

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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 and usage tips.

@github-actions

github-actions Bot commented Jan 15, 2026

Copy link
Copy Markdown

The following will be added to the changelog


[0.4.0-dev.3] - 2026-01-15

Bug Fixes

  • Abort orphaned tasks in error case
  • Race condition - wait for the relay connection before completing initialization

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b809bc and b075b1d.

📒 Files selected for processing (1)
  • crates/transport_iroh/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/transport_iroh/src/lib.rs (3)
crates/api/src/url.rs (2)
  • as_ref (74-76)
  • addr (214-221)
crates/api/src/transport.rs (1)
  • url (398-398)
crates/transport_iroh/src/url.rs (1)
  • get_url_with_first_relay (5-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
crates/transport_iroh/src/lib.rs (3)

221-230: LGTM!

The oneshot channel pattern is correctly implemented to signal URL readiness. The sender is properly passed to the spawned task, and the receiver will be awaited with a timeout.


275-284: LGTM!

The function signature change is well-designed. Using Option<...> maintains flexibility, and the mutable binding enables the .take() pattern for one-time signaling.


286-318: LGTM!

The logic correctly identifies the first URL by checking guard.is_none() before the update. The .take() pattern ensures one-time signaling, and releasing the write lock before signaling is good practice to avoid holding locks during async operations.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread crates/transport_iroh/src/lib.rs
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jan 15, 2026

Copy link
Copy Markdown

Deploying kitsune2 with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0fcceb4
Status: ✅  Deploy successful!
Preview URL: https://91089171.kitsune2.pages.dev
Branch Preview URL: https://fix-url-race-condition.kitsune2.pages.dev

View logs

@jost-s

jost-s commented Jan 15, 2026

Copy link
Copy Markdown
Contributor

This could be a useful change. I would simplify the main connection task and pass in the local URL instead of an option.

But in the larger picture, this is something that kitsune should handle better. When a local URL is received from the relay, new_listening_address is called. I think kitsune shouldn't act before it doesn't have a listening address. Then this issue wouldn't occur.

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.

2 participants