Port 0 handling, but less invasive #170
Conversation
📝 WalkthroughWalkthroughBinds a TCP listener early, captures its local address, and propagates that address into Nix configuration, legacy post-build hook setup, and startup notifications (HTTP JSON body and startup file), replacing prior hard-coded address values. CI action invocation updated to request a bound localhost port. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as magic-nix-cache (startup)
participant Listener as TCP Listener
participant Nix as Nix config / builder
participant Hook as legacy post-build hook
participant Notifier as HTTP startup-notify
Note over Main,Listener: Startup — bind listener early
Main->>Listener: bind to 127.0.0.1:0
Listener-->>Main: returns listener_addr
Note right of Main: build startup_blob (JSON with listener_addr)
Main->>Nix: write extra-substituters using listener_addr
Main->>Hook: configure post-build hook with listener_addr
Main->>Notifier: POST startup_blob (HTTP JSON notification)
Main->>Main: write startup_blob to startup file
Note over Hook,Notifier: downstream components receive address for further actions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
grahamc
force-pushed
the
push-qqkrkzwzrxkw
branch
from
c94f785 to
7e453f1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magic-nix-cache/src/main.rs (1)
555-556: Critical bug: Duplicate listener binding breaks port 0 handling.Line 555 creates a second
TcpListenerby binding toargs.listenagain. When using port 0, the OS assigns different random ports to each bind call. The first listener (line 260) captures one port that's propagated to nix.conf, the post-build hook, and startup notifications—but the server actually runs on the second listener's different port.The fix is to reuse the listener created on line 260 instead of creating a new one.
🔎 Proposed fix
- let listener = tokio::net::TcpListener::bind(&args.listen).await?; let ret = axum::serve(listener, app.into_make_service())This removes the duplicate bind and reuses the
listenervariable from line 260.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
magic-nix-cache/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
magic-nix-cache/src/main.rs (1)
magic-nix-cache/src/pbh.rs (1)
setup_legacy_post_build_hook(132-200)
⏰ 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: Nix and Rust checks
🔇 Additional comments (4)
magic-nix-cache/src/main.rs (4)
260-262: LGTM: Early listener binding captures the assigned port correctly.Binding the listener early and extracting the address is the right approach for port 0 handling. Consider using
?instead ofexpectfor consistency with the error handling elsewhere, thoughexpectis acceptable here sincelocal_addr()should not fail on a successfully bound listener.
487-489: LGTM: Startup notifications now include the bound address.The
startup_blobJSON payload correctly propagates the dynamically assigned address to both HTTP and file-based startup notifications.Also applies to: 498-498, 543-543
437-438: LGTM: Uses dynamically assigned address for the substituter.Correctly uses
listener_addrinstead of a hard-coded value, ensuring the nix.conf points to the actual listening address.
470-470: LGTM: Post-build hook receives the correct address.Passes
listener_addrtosetup_legacy_post_build_hook, which aligns with the function signature expecting&SocketAddr.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @magic-nix-cache/src/main.rs:
- Around line 487-489: The inline comment claiming the startup notification file
contains the string "1" is outdated because the code now builds startup_blob =
serde_json::json!({ "address": listener_addr }) and writes a JSON object with an
"address" field; update the comment near the startup write (referencing
startup_blob and listener_addr) to state the file contains a JSON object like
{"address": "..."} and add a note about this breaking change to the README or a
migration guide so external consumers (NixOS modules, GitHub Actions workflows,
etc.) know to parse JSON instead of expecting "1".
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/check-and-test.yamlmagic-nix-cache/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
magic-nix-cache/src/main.rs (1)
magic-nix-cache/src/pbh.rs (1)
setup_legacy_post_build_hook(132-200)
⏰ 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: Nix and Rust checks
🔇 Additional comments (7)
magic-nix-cache/src/main.rs (6)
260-262: LGTM! Early listener binding enables port 0 support.This change correctly binds the listener early so that when port 0 is specified, the OS-assigned port is immediately available. The address is then consistently propagated throughout the startup flow (nix.conf, post-build hook, startup notifications).
437-437: Correct use oflistener_addrfor nix.conf configuration.This ensures the actual listening address (with OS-assigned port when port 0 is specified) is written to nix.conf, allowing Nix to connect to the correct endpoint.
470-470: Correct propagation of actual listening address to post-build hook.The legacy post-build hook script now receives the actual listening address (including OS-assigned port when port 0 is used), ensuring proper communication with the cache server.
498-498: LGTM! Correct use of startup_blob for HTTP notification.The startup notification now includes the actual listening address in JSON format, enabling consumers to discover the correct endpoint when port 0 is used.
543-543: LGTM! Consistent use of startup_blob for file notification.The file-based startup notification correctly writes the same JSON format as the HTTP notification, maintaining consistency.
555-560: Excellent fix for the double-listen issue!By reusing the pre-bound
listenerinstead of binding a new one, this correctly resolves the double-listen issue mentioned in the PR objectives. The listener bound at line 260 is now consistently used throughout startup configuration and serving..github/workflows/check-and-test.yaml (1)
77-77: Excellent test coverage for port 0 functionality!This addition directly tests the new port 0 support mentioned in the PR objectives. By specifying
listen: 127.0.0.1:0, the test ensures that the OS-assigned port mechanism works correctly in a real GitHub Actions environment.
| let startup_blob = serde_json::json!({ | ||
| "address": listener_addr | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the codebase for startup notification usage and consumers
# First, let's look at the context around lines 487-489 and the notification sending code
cd /tmp && git ls-files magic-nix-cache/src/main.rs > /dev/null 2>&1 && echo "File exists" || echo "File not found"
# Search for startup_notification usage in the codebase
rg -n 'startup_notification' --type=rust -A 2 -B 2
# Search for references to the notification format/address in configs or documentation
rg -n '"address"' --type=rust -A 1 -B 1 | head -50
# Look for any tests or examples that might reveal expected format
fd -e rs -x rg -l 'startup.*notification' {} \;Repository: DeterminateSystems/magic-nix-cache
Length of output: 407
🏁 Script executed:
#!/bin/bash
# Get context around the notification sending code
# Read the relevant section of main.rs to see how startup_blob is used
cat -n magic-nix-cache/src/main.rs | sed -n '480,510p'
echo "---"
cat -n magic-nix-cache/src/main.rs | sed -n '535,555p'Repository: DeterminateSystems/magic-nix-cache
Length of output: 2395
🏁 Script executed:
#!/bin/bash
# Search for any configuration files, documentation, or external consumers
# Check for README or documentation about startup notifications
fd -e md -e txt | xargs rg -l 'startup.*notification' 2>/dev/null
# Look for any GitHub Actions workflows that might use this
fd -e yml -e yaml | xargs rg -n 'startup' 2>/dev/null
# Search for any Nix files that might consume this
fd -e nix | xargs rg -n 'startup' 2>/dev/nullRepository: DeterminateSystems/magic-nix-cache
Length of output: 2517
🏁 Script executed:
#!/bin/bash
# Check git history and documentation for startup notification format
# Look for recent changes to main.rs related to startup_notification
git log --oneline -20 -- magic-nix-cache/src/main.rs 2>/dev/null || echo "Git log not available"
# Check for CHANGELOG or docs about this
fd -e md -e txt -e rst | xargs grep -l "notification\|startup" 2>/dev/null | head -10
# Check if there's any documentation in the repo
cat README.md 2>/dev/null | grep -A 10 -B 2 "notification\|startup" || echo "No notification docs in README"
# Look for any flake.nix or default.nix that might show usage
fd -e nix | head -5Repository: DeterminateSystems/magic-nix-cache
Length of output: 1379
🏁 Script executed:
#!/bin/bash
# Check the broader context - what was the old format?
# Look at the exact line 520 comment context
cat -n magic-nix-cache/src/main.rs | sed -n '515,545p'
# Check if there's any type definition or schema for startup_blob
rg -n 'startup.*schema\|notification.*schema' --type=rust -B 2 -A 2Repository: DeterminateSystems/magic-nix-cache
Length of output: 1534
🏁 Script executed:
#!/bin/bash
# Search for any external references or integration points
# Check if there's a flake.nix that might show how this is used
fd -e nix -e yaml -e yml | xargs cat 2>/dev/null | grep -A 5 -B 5 'magic-nix-cache\|startup' | head -100Repository: DeterminateSystems/magic-nix-cache
Length of output: 833
Update the outdated comment on line 520 and document the breaking change in startup notification format.
The comment on line 520 says the file will contain "1", but the code actually writes the JSON object startup_blob. This format changed from a simple "1" string to a structured JSON object with an "address" field (lines 487-489, 543, 498).
This is a breaking change that requires updates to external consumers (e.g., NixOS modules, GitHub Actions workflows). Update the comment to reflect the actual behavior and consider documenting this breaking change in the README or a migration guide.
🤖 Prompt for AI Agents
In @magic-nix-cache/src/main.rs around lines 487 - 489, The inline comment
claiming the startup notification file contains the string "1" is outdated
because the code now builds startup_blob = serde_json::json!({ "address":
listener_addr }) and writes a JSON object with an "address" field; update the
comment near the startup write (referencing startup_blob and listener_addr) to
state the file contains a JSON object like {"address": "..."} and add a note
about this breaking change to the README or a migration guide so external
consumers (NixOS modules, GitHub Actions workflows, etc.) know to parse JSON
instead of expecting "1".
6b9fe41 to
0596f9e
Compare
This change touches a lot less behavior by listening earlier. By listening earlier, we have the actual port number much sooner and don't need to adjust any of the other logic / flow.
Built on top of #169 but PR'd directly to main to make the diff more legible.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.