fix(ebpf): detach uprobe links for dead inodes to stop bpf_link fd leak

[vaderyang]

Problem

The eBPF SSL-uprobe capture source leaks bpf_link file descriptors. Each
attached target inode holds a handful of bpf_link fds (one per uprobe:
ssl_write + ssl_read_enter + ssl_read_exit) for the entire process
lifetime, even after every process backing that inode has exited.

Root cause is in h-capture/src/ebpf/source.rs: the static-target attach path
called program.attach(...) and discarded the returned link id. In aya 0.13 the
link (and its fd) stays owned inside the program's link map until it is
explicitly detached, so dropping the id does not release anything.
rescan_targets keeps discovering new inodes as npm-style auto-updates rotate
the on-disk binary and as monitored sessions come and go, but nothing ever
detached probes for inodes whose processes were all gone.

The attachments — and their fds — therefore grew monotonically (the
ebpf_uprobes_attached gauge only ever climbed, far past the live working set),
eventually crossing the process fd soft limit and starving the HTTP API
listener's accept(), so the API stopped responding.

Fix

• Track the (program, link_id) pairs per attached inode (InodeLinks).
• On each rescan, compute the set of inodes still backed by an on-disk path or a
  running target process, and detach the probes for any inode no longer live.
  Detaching drops the link and closes its fd.
• Prune seen to the live set so a reused inode number can be re-attached.
• Partial-attach failures now unwind their own links instead of orphaning fds.

Net effect: the health gauge falls on detach instead of ratcheting up, and the
fd footprint tracks the live monitored set rather than growing without bound.

Dynamic libssl probes are intentionally left attached for the process
lifetime (a fixed, startup-only set — not a growth source).

Defense-in-depth

scripts/staging/heron.service now sets LimitNOFILE=65536 (it previously
inherited the systemd default of 1024). The prod unit is provisioned on the host
and is not tracked here — it needs the same change applied there.

Verification

• Type-checked the exact aya-0.13.1 API usage introduced here (attach returning
  the link id, &mut Program -> &mut UProbe, detach(link_id), the per-inode
  link-tracking collections) against the real aya 0.13.1 crate on Linux —
  cargo check clean.
• The full --features ebpf build + load soak runs on merge via staging-soak
  (the BPF toolchain isn't available on the dev machine).

Post-deploy runtime check

After rollout, confirm the gauge oscillates around the active set instead of
climbing, and that bpf_link fds fall as sessions exit:

• ebpf_uprobes_attached should track the live target set, not increase forever.
• bpf_link entries under the process fd table should drop when monitored
  sessions end.

[github-actions]

The existing tests cover exe_link_has_basename, target_has_source, and resolve_target_offsets — pure helpers. There are no tests for the new GC/detach logic (rescan_targets's dead-inode path, detach_links, or attach_at's partial-failure unwind). rescan_targets takes &mut Ebpf which is hard to construct in a unit test without a loaded BPF program, so this is understandable, but the GC logic is the core of the fix and has no test coverage. Worth a Suggestion.

I have enough to write the review.

Summary

PR #172 fixes a real bpf_link fd leak in the eBPF uprobe capture source: program.attach() returned a UProbeLinkId that was previously discarded, so aya kept the link (and its bpf_link fd) alive in the program's link map for the whole process lifetime. With npm-style auto-update inode rotation, attached inodes grew monotonically and eventually exhausted the process fd limit, starving the API listener's accept(). The fix tracks (program_name, link_id) pairs per inode and detaches them when the inode is no longer backed by an on-disk path or a running process; partial-attach failures unwind their links; seen is pruned so a reused inode number can be re-attached. A second commit sets LimitNOFILE=65536 on the staging systemd unit as defense-in-depth. The aya 0.13 API surface (UProbe::attach → Result<UProbeLinkId>, UProbe::detach(link_id) → calls LinkMap::remove → link.detach() → closes the fd) checks out, and CI passed. Recommendation: APPROVE — the fix is correct, well-documented, and the only nits are non-blocking.

Blocking

(none)

Suggestions

• server/h-capture/src/ebpf/source.rs:786-812 — seen.insert(ino) happens before attach_at, so a partial-attach failure (e.g., ssl_write attaches, ssl_read_enter fails with a transient perf_event_open EAGAIN) marks the inode as seen but not attached. The unwound links are correctly detached (no fd leak — that part is new and good), but the inode is never re-attempted on subsequent rescans until it becomes dead and is pruned by GC. This is pre-existing behavior (the original code also inserted-then-attempted), so not a regression — but now that attach_at distinguishes "no offsets resolved" (deterministic) from "partial kernel failure" (transient), the transient case could retry instead of being permanently skipped. Consider: on Err(e) from attach_at, seen.remove(&ino) so the next rescan retries.
• server/h-capture/src/ebpf/source.rs:816-830 — The dead-inode GC detaches any inode not in the live set, where live is built from inode_of returning Some this pass. inode_of calls std::fs::metadata, which can transiently fail on a live inode during a /proc race (process executing, /proc/<pid>/exe link momentarily unreadable). On such a pass the inode is absent from live, GC detaches it, and the next pass re-attaches it — a detach/re-attach churn that briefly drops probes for that inode (events missed in the window) and cycles bpf_link fds. For a process that forks/exits rapidly this could repeat. Consider requiring two consecutive "not live" passes before detaching, or treating a metadata error (inode_of returns None but the path exists) differently from "path gone." Low-severity since the common case is genuinely-dead inodes from auto-update rotation.
• server/h-capture/src/ebpf/source.rs:819-823 — The GC collects dead by iterating attached.keys() and filtering against live, then removes them in a second loop. This is fine for the expected cardinality (a handful of target inodes), but seen.retain(|ino| live.contains(ino) || attached.contains_key(ino)) (line 833) is O(|seen| × |live|) since both are HashSets of (u64,u64) — that's a hash lookup per element, so actually O(|seen|). Fine. No change needed; just confirming the complexity is bounded.
• server/h-capture/src/ebpf/source.rs:464-477 — detach_links silently continues on a vanished program or wrong-type program. For the dead-inode GC this is correct (best-effort), but for the partial-attach unwind in attach_at (line 713), a silent skip could leave a bpf_link fd orphaned if the program object was somehow replaced between attach and unwind. In practice the program set is fixed for the process lifetime (loaded once at startup, line 126-134), so this can't happen — but a tracing::warn! on the continue paths would surface it if it ever did.
• server/h-capture/src/ebpf/source.rs (tests) — No test coverage for the new GC behavior. The pure helpers (exe_link_has_basename, target_has_source, resolve_target_offsets) are tested, but detach_links, attach_at's partial-failure unwind, and rescan_targets's dead-inode pruning are untested. rescan_targets takes &mut Ebpf which is hard to construct without a loaded BPF program, so unit-testing it would require a trait seam — understandable that it's not done. If a fn dead_inodes(attached: &HashMap<K,V>, live: &HashSet<K>) -> Vec<K> helper were extracted from the GC loop, that pure step could be tested directly without an Ebpf handle.
• scripts/staging/heron.service:35 — LimitNOFILE=65536 is reasonable, but consider whether the prod unit (provisioned out-of-band per the commit message) needs the same value documented somewhere tracked. The commit message notes "needs the same LimitNOFILE applied there" but there's no follow-up task or issue referenced. Worth confirming the prod provisioning playbook picks this up.

Questions

• server/h-capture/src/ebpf/source.rs:788 — seen.insert(ino) before attach_at means a partial-attach failure permanently marks the inode as attempted. Is this intentional for the transient-failure case, or should Err from attach_at undo the seen.insert? The pre-existing behavior was the same, so this isn't a regression — but the PR's new partial-unind logic makes the "transient" vs "deterministic" distinction visible, so it might be worth revisiting.
• scripts/staging/heron.service — The commit message says "The prod unit is provisioned on the host (not tracked here) and needs the same LimitNOFILE applied there." Is there a tracked follow-up to apply this to prod, or is it a manual ops step that could be forgotten? The staging soak gate won't catch a prod-only fd-exhaustion regression since staging already has the higher limit.

Verified

• aya 0.13 API surface: read /home/ci/.cargo/registry/src/.../aya-0.13.1/src/programs/uprobe.rs — UProbe::attach returns Result<UProbeLinkId, ProgramError> (line 84), UProbe::detach(link_id: UProbeLinkId) takes by value (line 103). LinkMap::remove (in links.rs:81-86) calls .detach() on the removed link, which closes the bpf_link fd. The fix's premise ("aya owns the link until detach/take_link") is correct.
• Caller compatibility: searched all attached_inodes references (lines 186, 191, 193, 205, 257, 261, 267, 273) — all use .is_empty() or .len(), which work on both HashSet and HashMap. No caller depends on the old HashSet shape.
• Metric gauge: Metric::EbpfUprobesAttached is kind: Gauge (h-common/src/internal_metrics.rs:157), so .set() is the correct operation. The PR correctly switched from implicit ratcheting to explicit .set(live_count).
• Leakage check: grepped scripts/staging/heron.service for RFC1918 ranges, CGNAT, plaintext credentials, and BEGIN ... PRIVATE KEY — no matches. Paths (/opt/heron/...) are generic install paths, not machine-specific. Commit message refers to "the staging VM" / "the prod host" generically per CLAUDE.md rule #5.
• seen.retain correctness: traced the GC loop — dead is computed and removed from attached before seen.retain, so attached.contains_key(ino) in the retain only matches live (still-attached) inodes. Dead inodes are correctly pruned from both attached and seen, allowing a reused inode number to be re-attempted.
• Partial-attach unwind: std::mem::take(&mut links) in attach_at (line 713) empties links and passes ownership to detach_links; the now-empty links is dropped on return Err(e) with no effect. No double-free or use-after-free.

---

🤖 Reviewed by the review bot • workflow run