Repository navigation
fix(transport): the responder closes idle inbound links and counts every departure (#853, main port of v40.0.10) - #854
Conversation
…nk departure is counted (#853) Main port of v40.0.10 (971a605 + dc5b296), with two design changes. - Link direction from LinkEstablished { is_initiator }; inbound_links / outbound_links gauges; EdgeLinkInfo.direction in the UDL and the regenerated bindings. - Responder-side idle reap on the pool tick (inbound_link_idle_bound, default 120 s, 0 disables), node-authoritative teardown; busy: link_in_flight, inbound transfers, outbound ships, sender-side resource progress, LXMF serve activity, the attribution handshake. - {inbound,outbound}_link_closed_by_reason with one token per way a link leaves, and reconcile_vanished_links pruning links the node no longer holds as `vanished`. Changed from the release line: - The LXMF serve exemption is BOUNDED: a link is busy for one inbound idle bound after its last LXMF activity (a request, its response handed to the link), then the normal rule applies. A sender-side resource still making progress (e.g. a large LXMF response) is busy too. Witness (f): fails with the exemption made for-life and with it removed. - hold_link_open / release_link_hold removed: no A/V caller on main, and #813 tracks link ownership through LinkPlane::Av + AvSink. - Main has no P0 telemetry (#820) yet, so the new metrics are in EdgeMetrics / EdgeMetricsBundle only; their PyO3 / UniFFI flattening arrives with #820's port. Gates: fmt; clippy --all-targets -D warnings on CI's three sets; lib filtered 819/853/link/reap/metrics/observability 84 passed (84 under ffi-uniffi); 6 integration files 57 passed; links_ffi + peer_mutation_ffi + witness under ffi-uniffi 30 passed; bindings regen matches; 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5FDihqVKTSXyNYs2CLnrK
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cb17f4081
ℹ️ 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".
| inbound_link_idle_expired( | ||
| now_ms, | ||
| *at, | ||
| last_inbound.get(id).copied(), | ||
| self.node.get_remote_identity(id).is_some(), |
There was a problem hiding this comment.
When a peer-opened link has received no inbound frame for the bound but is still carrying periodic outbound reverse-path traffic, this predicate continues aging solely from last_inbound. The busy sets only defer reaping while a resource send is currently in progress, and Channel-first sends are not represented there at all, so the next tick can close an actively used link immediately after or even during an outbound send. For an initiator-only/NATed peer, the fallback outbound dial is unavailable, leaving subsequent pushes undeliverable until that peer transmits and redials; successful outbound resource and Channel activity should therefore refresh the link's idle timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38bf07f. A successful outbound send on a link now restarts its idle clock. That covers a Channel-first frame fully sent (the scoped lease and both reverse-path arms) and a Resource whose completion future resolved in ship_resource_on_link. Each stamps link_last_outbound_at, which is kept separate from link_last_inbound_at so the #353 reverse-path selector still reads only inbound liveness. The idle predicate starts the clock at max(last inbound, last outbound, establishment). Witness a_link_this_node_keeps_sending_on_is_not_reaped_854: B sends one frame and then only listens, and A keeps answering on that link via send_on_reply_path_only for three bounds. The link stays open, and it is reaped only after A stops. With the stamp disabled, A's reply path is gone under its own sends (Unreachable: no live reply path).
| let due: Vec<LinkId> = candidates | ||
| .into_iter() | ||
| .filter(|id| !busy.contains(id)) | ||
| .filter(|id| { |
There was a problem hiding this comment.
When the peer has already closed an idle link but its LinkClosed event is still queued behind this timer arm, reconciliation deliberately marks the link as a suspect and waits for the grace period, yet this selection still treats it as due because the local mirrors remain and there is no node.link_is_established(id) check. link_teardown_as then removes the direction record and counts idle_expired; when the queued event is finally handled it cannot count the real peer_closed reason. This defeats the new grace period and corrupts departure telemetry, so links the node already considers gone must be left for the event or vanished reconciliation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38bf07f. Idle selection now filters on node.link_is_established. A link the node already closed is left to its queued LinkClosed (counted under the real reason), or after VANISH_GRACE to reconciliation's vanished. It is never counted as our idle_expired. Witness the_idle_reap_skips_links_the_node_no_longer_holds_854: an inbound link in edge's books, far past the bound but not held by the node, survives three passes with idle_expired = 0, then counts once as vanished. Without the filter, the second pass tears it down as idle_expired.
| forget_link_direction( | ||
| &self.link_direction, | ||
| self.metrics.get(), | ||
| self.metrics_source, | ||
| &link_id, | ||
| cause, | ||
| ); |
There was a problem hiding this comment.
If a locally initiated or reaper-initiated close has its LinkClosed event dropped during control-plane saturation, this eagerly removes the direction record and the function subsequently removes the link from established_links, so reconcile_vanished_links can no longer discover it. The local teardown path does not remove link_to_peer_key_id, link_last_inbound_at, or inbound_resource_active; those are cleared only by the now-lost event's LinkBook::forget, causing the per-link maps to grow across repeated reaps. Either perform the complete LinkBook cleanup eagerly or retain one of the reconciliation records until the close event or grace-period cleanup runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38bf07f by doing the complete cleanup eagerly. link_teardown_as counts the close under its cause before close_link (so the following LinkClosed adds nothing), then runs the full LinkBook::forget right away. That covers mirror, established_at, attribution, last-inbound and last-outbound stamps, dialed dest + identity pool, in-flight, inbound transfer marks and plane, plus the LXMF entry, pool stamp and scoped pool. A lost LinkClosed leaves nothing behind. Witness a_local_teardown_forgets_the_link_everywhere_854: every per-link map is empty after a teardown, and the close is counted once as local_teardown. With the old partial teardown, attribution, the stamps and the transfer mark remain.
| let established_at = self.link_established_at.lock().await.clone(); | ||
| let last_inbound = self.link_last_inbound_at.lock().await.clone(); | ||
| let now_ms = u64::try_from(chrono::Utc::now().timestamp_millis().max(0)).unwrap_or(0); |
There was a problem hiding this comment.
Because the reaper runs inside the same listen loop that consumes NodeEvents, these snapshots can be stale whenever the timer branch wins while an inbound frame or ResourceTransferStarted/ResourceProgress event is already queued. The frame has reached the node, but neither last_inbound nor the busy maps has been updated yet, so a link that is actively receiving at the idle boundary can be selected and closed before its activity event is handled. Under event backlog this is not limited to an exact timer tie; the reaper should consult authoritative node activity or defer candidates with pending receive activity rather than relying only on event-loop mirrors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 38bf07f, using deferral rather than node state. leviculum 0.27's std ReticulumNode has no per-link last-inbound: link_stats carries none, and core's last_inbound_secs also counts keepalives, which would keep every link alive. So a link is closed only by the second consecutive pass that finds it idle with an unchanged (last inbound, last outbound) snapshot. A frame or ResourceProgress queued behind the timer arm is drained by the loop before the next tick, and its stamp or busy mark makes the link no longer due. Witness a_link_is_reaped_only_on_the_second_idle_observation_854 drives the passes itself. The first idle pass only notes the link, a frame landing between passes saves it, and two idle unchanged passes close it. With a single observation, the first pass closes it.
…nner (#853) inbound_exits_are_instrumented_or_marked failed the v40.0.10 tag run on the bare `return;` after the swallow_link_closed test seam in the LinkClosed arm. The seam is empty in production and the swallowed close is still counted as `vanished` by reconciliation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5FDihqVKTSXyNYs2CLnrK
1. Outbound use restarts the idle clock. A successful outbound send on a link (a Channel-first frame fully sent on the scoped lease and both reverse-path arms; a Resource whose completion future resolved) stamps link_last_outbound_at, kept apart from link_last_inbound_at so the #353 reverse-path selector still reads only inbound liveness. The idle clock is max(last inbound, last outbound, establishment). An initiator-only peer's link this node keeps answering on is no longer reaped under it. 2. Idle selection skips links the node no longer holds, so a queued LinkClosed is never pre-empted and misfiled as idle_expired; those wait for their event or reconciliation's `vanished`. 3. A local teardown counts the close under its cause, closes at the node, then runs the full LinkBook::forget eagerly (plus LXMF, pool stamp, scoped pool): a lost LinkClosed leaks no attribution, stamps or transfer marks. 4. Two consecutive idle observations. leviculum 0.27's std handle does not surface a link's last-inbound (link_stats has none; core's last_inbound_secs counts keepalives), so a link is closed only by the second pass that finds it idle with the same (last inbound, last outbound) snapshot; activity queued behind the timer lands between. Witnesses, each failing before its fix (mutation runs, file restored): - (g) a_link_this_node_keeps_sending_on_is_not_reaped_854: without the outbound stamp, A's reply path is gone under its own sends. - the_idle_reap_skips_links_the_node_no_longer_holds_854: without the node filter, idle_expired is counted for a link the node had closed. - a_local_teardown_forgets_the_link_everywhere_854: without the eager forget, attribution/stamps/transfer marks remain. - (h) a_link_is_reaped_only_on_the_second_idle_observation_854: with one observation, the first pass closes the link. Gates: fmt; clippy --all-targets -D warnings on CI's three sets; lib filtered 819/853/854/link/reap/metrics/observability/inbound_exits 87 passed (87 under ffi-uniffi); 6 integration files 59 passed; links_ffi + peer_mutation_ffi + witness under ffi-uniffi 32 passed; 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5FDihqVKTSXyNYs2CLnrK
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… speak (#853) From the Datum link-storm trace (storm-v4006): ~890 `link identified` and ~825 PeerClosed per 10 min from one v40.0.6 initiator, and no responder line at the canonical's RUST_LOG=info. - No PoolFull close on release: release_link_after_transfer no longer trims the pool to the idle cap, and the reaper applies no cap either; pooled lanes die only by the idle bound. pool_full is retired (always 0, kept for its series); idle_cap is accepted but not enforced. - LinkClaim: a pool lane's transfer claim is an RAII guard released on Drop, so a send cancelled mid-ship (the responder's 60 s reply timeout) no longer retires its lane. - A pooled lane closing ChannelExhausted or Stale logs a throttled WARN with running counts (the starved-responder signature). - Responder round lifecycle at INFO (started; both completion paths with duration_ms; reply failures stay WARN), with peer, kind and the arrival link; counter responder_rounds_total{kind, outcome}. - responder_link_up_seconds{stage: bundle_gate, announce_intake, owner_binding} from LinkEstablished on a peer-dialled link, and responder_link_up_total{ok, bundle_refused, timeout, link_gone}. - EdgeMetrics only on main (accessors responder_rounds_total() / responder_link_up_seconds() / responder_link_up_total()); the bundle and both bindings carry them when #848 lands. FixedHistogram / HistogramSnapshot come over verbatim from the release line. Witnesses: a_burst_closes_nothing_on_release_and_the_bound_retires_it_853 (14 concurrent sends, cap 1), a_cancelled_send_gives_its_lane_back_853, each_responder_round_outcome_increments_its_token_once_853, a_responder_link_up_lands_in_a_bucket_and_counts_its_outcome_853. Fail-before: with the release trim + reaper cap restored the burst witness fails (12 lanes closed pool_full); with LinkClaim's Drop a no-op the cancel witness fails (the lane stays claimed). Gates: fmt; clippy --all-targets -D warnings on CI's three sets; lib filtered 819/853/854/link/reap/metrics/observability/inbound_exits 89 passed (89 under ffi-uniffi); 6 integration files 60 passed (link_pool_819 3 runs green); links_ffi + peer_mutation_ffi + witness under ffi-uniffi 32 passed; 0 failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5FDihqVKTSXyNYs2CLnrK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d58e06f6e2
ℹ️ 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".
| let candidates = self | ||
| .link_direction | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) | ||
| .inbound_ids(); |
There was a problem hiding this comment.
When control-plane saturation drops a LinkEstablished event, this link is never inserted into link_direction, so it can never enter the inbound reaper's candidate set. The ControlPlaneOverflow arm explicitly acknowledges that establishment events may be missed, but later LinkIdentified/traffic does not reconstruct the direction, and reconciliation only examines the same direction map plus established_links, which the missed event also populates. Consequently, a peer-opened link in this scenario can remain live and uncounted indefinitely—the exact growth this reaper is intended to stop—so recovery needs to discover authoritative node links or establish bookkeeping from subsequent per-link events.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #859 (1419d93). New recover_unknown_link runs on a link's next per-link event (LinkIdentified, or any frame through attribute_and_deliver). If the node holds the link but edge has no direction record for it, the link is recorded INBOUND unless this node dialled it (dialed_link_dest is written at every own dial), added to the established mirror, given now as its establishment so the idle clock starts there, and has its plane classified. It is counted in recovered_links_total. Witness a_link_whose_establish_event_was_lost_is_recovered_and_reaped_854: a test seam drops the next inbound LinkEstablished; B's frame then rebuilds the bookkeeping, and the link is counted and reaped once idle. Without the recovery the witness fails. Limit: leviculum 0.27 still can't list its links, so a link that never carries another event stays invisible until it closes.
| let outcome = send_fragments_on_channel(&self.node, &link_id, &fragments).await; | ||
| if outcome.complete() { | ||
| // CIRISEdge#853 (Codex on #854) — a sent frame is a use of the link. | ||
| note_outbound_use(&self.link_last_outbound_at, link_id); |
There was a problem hiding this comment.
With a configured short inbound idle bound, a reverse-path Channel send can begin after the first idle observation and still be awaiting pacing when the next reaper tick runs; Channel transmission can take up to the 15-second frame budget, while the tick can be as short as 250 ms. This timestamp is written only after the whole frame succeeds, and unlike the Resource path no busy guard covers the await, so the unchanged second observation can close the link mid-send. Fresh evidence beyond the earlier fix is that the final code still places note_outbound_use after send_fragments_on_channel(...).await in both reverse-path Channel arms; mark the link busy for the duration of those awaits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #859 (1419d93). Every Channel-first send that rides a link goes through send_channel_marked, which holds an OutboundShipGuard (the Resource path's busy mark) for the whole await. That covers both reverse-path arms and the scoped lease. A reply waiting on the Channel window is therefore busy to the inbound reaper, not only once it has completed. Witness a_reply_waiting_on_the_channel_holds_its_link_854: the first idle pass notes the link, then a reply is held 3 s inside the send by a test delay. The second pass runs mid-send and closes nothing, and the reply completes. With the guard removed, the second pass closes the link.
| // CIRISEdge#853 — the first application frame on a link a peer dialled | ||
| // ends its link-up (a no-op for a link not being timed). | ||
| if let Some(m) = ctx.metrics { | ||
| m.responder_link_up_end(link_id.into_bytes(), crate::observability::LINK_UP_OK); |
There was a problem hiding this comment.
For an owner-binding push, this ends and removes the pending link-up timer before the frame reaches OwnerBindingCarveOut::admit_frame, where the newly added LINK_UP_STAGE_OWNER_BINDING observation is recorded. Because responder_link_up_stage ignores links absent from the pending map, every successfully consumed owner binding produces no owner-binding latency sample at all. Defer the ok end until downstream link-up stages have been recorded, or otherwise preserve the start time through owner-binding admission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #859 (1419d93). The transport no longer ends the link-up on the first frame. note_link_up_frame runs at the sink and ends it ok only for an ATTRIBUTED frame. An unattributed frame, which may be the sibling's owner-binding push, leaves it open. After recording LINK_UP_STAGE_OWNER_BINDING, the carve-out ends it ok itself. An unattributed frame that is not an owner binding leaves the link-up to time out. Witness an_unattributed_frame_leaves_the_link_up_for_the_owner_binding_stage_854 fails when the frame ends the link-up regardless of attribution.
| if link.is_some() { | ||
| *self | ||
| .last_inbound_link | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner) = link; |
There was a problem hiding this comment.
When an existing responder coordinator later receives a frame over HTTPS or another non-Reticulum transport, link is None, but this conditional leaves the previous Reticulum link ID stored. All subsequent responder lifecycle logs therefore attribute those rounds to a stale link rather than the documented -, which is especially misleading during transport failover investigations. Assign the option unconditionally so None clears the prior link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| tracing::debug!(peer = %peer, ?kind, "responder driver ending (channel closed)"); | ||
| break; | ||
| }; | ||
| let began = *round_began.get_or_insert_with(std::time::Instant::now); |
There was a problem hiding this comment.
If an initiator abandons an incomplete round and sends a frame with a new round ID, drive_round_step_framed resets the responder session and rebinds it, but this local timer remains anchored to the abandoned round. When the replacement round completes, its duration_ms therefore includes all time spent waiting in the prior round, corrupting the new lifecycle diagnostic precisely in packet-loss and retry scenarios. Detect the round-ID change before stepping and restart round_began when the coordinator rebinds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #859 (1419d93). New responder_round_clock restarts the round timer whenever a frame names a round id different from the one the timer is anchored to, i.e. when the coordinator rebinds to a new round. Same-round frames and legacy (round 0) frames leave it alone. Witness the_round_clock_restarts_when_the_responder_rebinds_854: after 45 s spent in an abandoned round, the replacement round's start is its own first frame. Without the restart the witness fails.
| .reusable_dialed_link | ||
| .lock() | ||
| .await | ||
| .get(&dest) | ||
| .is_some_and(|pool| pool.contains(&link_id)), |
There was a problem hiding this comment.
When a pooled scoped link closes with ChannelExhausted or Stale, this predicate checks only reusable_dialed_link, while scoped lanes are stored in reusable_scoped_link. Such a close is therefore treated as non-pooled and never reaches note_starved_pool_close, suppressing the warning and running counters for one of the two pool families even though the new diagnostic is documented for pooled lanes generally. Check both pools before forgetting the link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #859 (1419d93). The pooled check is now link_is_pooled, which scans both reusable_dialed_link and reusable_scoped_link, so a scoped lane closing ChannelExhausted/Stale reaches note_starved_pool_close. Witness a_scoped_lane_counts_as_pooled_854 fails when only the identity pool is consulted.
1 participant
Main port of v40.0.10 (release-40.0.10 dc5b296), cherry-picked without the release-only files. Closes #853.
Field read. The canonical on 0.5.224 (edge v40.0.8) grew ~95 inbound links per 5 min with the pool gauges at zero: #819's reap is initiator-only, and about a fifth of inbound links are never closed by older peers' pools. No events were lost and there is no implicit cap (
max_linksunset; the 1,024 mirror envelope only logs).Change.
LinkEstablished { is_initiator }, so an inbound link is known before it is identified;directiononlink_list()(UDL regenerated).inbound_idle_bound, default 120 s, 0 disables), exempting links with a transfer in either direction (incl. sender-side Resource progress), LXMF serve activity within one bound, and the 30 s identify window. Closes go through the node's authoritative link state.inbound_links/outbound_linksgauges andinbound_/outbound_link_closed_by_reasonwith every departure token: idle_expired, pool_idle_expired, pool_full, local_teardown, normal, peer_closed, timeout, stale, invalid_proof, channel_exhausted, blackholed, other, vanished.node.link_is_established, counting links the node dropped without an event asvanished(2 s grace so a queuedLinkClosedisn't misfiled).hold_link_openis dropped (no caller; feat(transport): the A/V spine on the live node (#805 item 4, #720) #813 tracks A/V links throughLinkPlane::Av).Not projected to PyO3/UniFFI yet on main: the P0 bundle machinery (#848) isn't here, so the new gauges and reason families live in
EdgeMetrics/EdgeMetricsBundleuntil #848 lands.Conflict to carry, whichever of this and #813 merges second: #813 routes A/V frames to
deliver_av_frame, bypassingattribute_and_deliver, the only writer oflink_last_inbound_at. A peer-opened A/V link carrying media would read as idle and be reaped 120 s into a call. Fix: stamplink_last_inbound_atindeliver_av_frame(preferred) or treatLinkPlane::Avas busy.Witness
tests/inbound_reap_853.rs(two loopback nodes): (a) a quiet dialed link is reaped and the dialer self-heals on its next send; (b) a link mid-receive survives; (c) bound 0 disables; (d) gauges by direction; (e) a silent close is countedvanished; (f) the LXMF bound. Mutation runs: (a)(b)(e) fail with the reap + reconciliation removed; (f) fails both with the exemption for-life and removed.Gates: fmt; clippy
--all-targets -D warningson CI's three sets; lib filtered 84; integration 57; FFI 30; 0 failed.🤖 Generated with Claude Code
https://claude.ai/code/session_01C5FDihqVKTSXyNYs2CLnrK