Repository navigation
fix(minimald): publish the box's address before launching its activate hooks - #2072
Conversation
…e hooks FinalizeSession launched the box to run its on_activate hooks before register_hostname published the box's loopback address and registered its name. The hook launch is the box's first attach, and an own-address box that declares static ingress refuses to attach without a published address (NET-010, NET-121), so every own_ip session with --ingress and an activate hook failed with "no published address handed for box <name>". Register (and publish) first, keeping the first finalize's bounded wait for the range verdict, then run the hooks, then promote the record to Active. A finalize that fails after the publish leaves it on the Materializing record; the client's activate cleanup destroys the session, and destroy withdraws the publish, the name, and any grant. Adds a test seam recording what each own-address launch reads as the published address, and tests that the activate-hook launch finds the hand already published and that a failed hook's publish is withdrawn by the destroy. Closes #2070 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. 📝 WalkthroughWalkthroughFinalization now registers the hostname before launching activation hooks. Linux-only tests cover the published address observed at launch, successful activation, and cleanup after an activation hook fails. ChangesOwn-address Finalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change is mergeable after normal Linux CI checks; no concrete blocking issue was established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
A rabbit watched the address appear, Comment |
|
Approved: no findings at Nothing to do. Clobber: none: |
Closes #2070
Cause
SessionActor::finalize(crates/minimald/src/session.rs) launched the box to run itson_activatehooks (launch_host_for_hooks(LaunchPhase::Activating)) beforeregister_hostname(&record, true)published the box's loopback address and registered its*.min.internalname. That hook launch is the box's first attach. For an own-address box that declares static ingress, the attach (finish_own_ip_attachinnet/gvproxy_network.rs) readsOwnAddressReporter::published_address(), finds none, and fails withno published address handed for box <name>by design (NET-010 / NET-121). So every--network own_ip --ingress …session with at least one activate hook failed atFinalizeSession. Sessions without activate hooks never launch during finalize, so they registered first and worked. Introduced by #1798.Fix
Move
register_hostname(&record, true)ahead of the activate-hook block infinalize, after the patch/hook-script marker checks, the package check, and patch materialization. The first finalize keepswait_for_verdict = true, so it still waits (bounded) for the range verdict. The record is promoted toActiveafter the hooks, as before.register_hostnamereads only the record's id, name, network mode, policy, and box addresses, none of which theActivewrite changes. The comment that said "The box's first finalize: the one registration that may wait for the range verdict" moved with the call and now explains the ordering.Error path: if the hook launch or a hook fails (or the record write fails), finalize returns
Errwith the publish standing on aMaterializingrecord. The CLI's activate cleanup then sendsDestroySession, whosederegister_hostname(true)unpublishes, forgets, releases any grant, and deregisters the name.owns_hostname_routeis keyed on the in-memorySessionInner::Active, which aMaterializingsession already has, so the deregister runs. If the client dies before it can destroy, the leftoverMaterializingrecord is reaped at the next daemon start, and grants for sessions missing from the store get swept. A retried finalize registers again, which is idempotent per session id.Tests
launch_publish_seam(#[cfg(all(test, target_os = "linux"))]). For every own-address mock launch, it records what the production attach'sOwnAddressReporter::published_address()would return at launch time. The mock launcher attaches no network, so this is how a test sees the order.an_activate_hook_launch_finds_the_box_s_address_already_published: a handed own-ip box with declared ingress and anon_activatehook finalizes. Its hook launch found the hand already published (this assertion fails on the old ordering, which recordedNone), the hook ran, the record isActive, and the name answers at the hand.a_failed_activate_hook_s_publish_is_withdrawn_by_the_destroy: the same box with a failing hook fails finalize and is not promoted. AfterDestroySession, the name is NXDOMAIN, nothing routes, andpublished_own_addressisNone.Verification
cargo fmt --all: clean.minimalddoes not build natively.just test/just fixscope to-p minvmd -p sessionsthere and would not touch this change.just test-crosscould not run: the Docker daemon is not running andcrossis not installed. No musl target is installed for acargo check --target …-linux-musleither. The Linux CI lanes are the first compile and test of this change. Please watchci-linux-native-successbefore marking it ready.just lint-prosedoes not apply.🤖 Generated with Claude Code
Summary by CodeRabbit