tests: pin the DIMT tombstone refusal by its dead ifindex; harden dplane_tid() (BLO-38883)

[kkroo]

Follow-up to #128 (BLO-38883): Ally's three suggestions there, plus a harness race I hit while verifying them. Test-only.

Changes (tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py)

The refusal is asserted whole, ifindex included.

• During the held window, the tombstone keeps the dead link's index, so its FAIL_INSTALL and REMOVE_FAIL carry that index.
• If the hold had lapsed and the owner's ADD had replayed, a live entry would refuse the stranger with the same codes, but with 0 (still creating) or the new link's index.
• Asserting the index makes the tombstone's provenance visible at the assertion. The trailing _prove_dellink_unread() was the only other thing that told the two apart, so it goes. That also takes a vtysh round trip out of the 30 s hold.

_run_client(). _held_delete() and _direct_answer() share it instead of each carrying the same popen / communicate / kill / communicate block.

dplane_tid() no longer lets stderr into its result. This is the race found while verifying.

1. router.run() returns stderr with stdout.
2. A zebra thread that exits between the task glob and its comm read makes cat print an error after the TID.
3. _dplane_in_fd() then splices "<tid>\ncat: ..." into its for fd in /proc/<tid>/fd/* loop.
4. The newline ends the for list, and bash dies with syntax error near unexpected token cat:'` before any hold is armed.

It hit once on devbox under load. Locally, the old loop run over a task directory whose comm is gone reproduces that exact message, and the new loop returns the bare TID. Each read now drops its own stderr, and anything but one numeric TID is "not found", which every caller already reports with pytest.fail.

Verification

Devbox, Ubuntu 24.04 CI image built from #127's head (its zebra is master's), with this branch's zebra_dimt_tunnel dir mounted over the image's copy. Load average about 60.

Run	Result
module, master zebra	21 passed, 1 skipped
parked test, zebra without the owner check at the top of zebra_dimt_tunnel_park()	FAILS: a non-owner add against the tombstone was not refused before its barrier: []
parked test, zebra whose park refusal carries ifindex 0 (what a replayed live entry answers with)	FAILS at the new assertion: {'tunnel_id': 20, 'ifindex': 0, 'result': 1} != {'tunnel_id': 20, 'ifindex': 8, 'result': 1}
parked test ×3, instrumented copy: hold left after the non-owner block	28.3 s, 28.5 s, 29.0 s of 30 s

The ifindex-0 row is the new assertion's negative control. Before this change, that mutant passed the whole test: the old assertion ignored the ifindex, and the trailing window proof checks the hold, not the refusal.

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-38883

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b23088a

Test-only, tightly scoped, and the ifindex-0 mutant in the PR body is a real negative control — it is what makes the new assertion worth more than the window proof it replaces. One finding, in the dplane_tid() half.

Critical Issues (0)

Important Issues (1)

• [code/gstack] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:883 — the new "" sentinel is unguarded at two of its four call sites, where it silently degrades a hold instead of failing it.
  • dplane_tid()'s docstring states "Anything but one numeric TID is 'not found', which every caller reports." Two callers do not report it. hold_dplane_sendmsg (:623) and hold_dplane_in_recvmsg (:738) both read trace_fds=_route_netlink_fds(router, worker) if worker else None / _dplane_in_fd(router, worker) if worker else None, then hand that to _hold_dplane_syscalls(), which re-resolves the TID itself (:757) and only fails if that second resolve is also empty.
  • So a first resolve that misses and a second that succeeds — e.g. /var/run/frr/zebra.pid momentarily unreadable across a zebra respawn, or any stray token in the loop output — arms the hold with trace_fds=None. _hold_dplane_syscalls() has no warning on that path (:768 is a bare if trace_fds:), so the run proceeds unnarrowed, which this file's own docstring at :611-614 identifies as the BLO-28405 mis-aim that synced 2 of 36 CI runs on an ethtool probe.
  • Before this change the same transient produced a loud failure: a garbage TID reached _route_netlink_fds() / _dplane_in_fd(), whose pytest.fail guards (:460, :698) fired. The hardening is right, but it converted a loud harness failure into a silent wrong-aim at these two sites. The 2>/dev/null additions make the trigger rarer than it was; they do not remove the path.
  • Fix: resolve once and fail at the call site, or thread the resolved TID through. Either kills the double-resolve at the same time:

    worker = dplane_tid(router)
    if not worker:
        pytest.fail("zebra_dplane worker not found -- cannot aim the hold")

    and drop the if worker else None fallback. Please also correct the docstring's "every caller reports" claim to match whatever the code ends up doing — as written it tells the next reader the sentinel is handled everywhere.

Suggestions (2)

• [errors] test_zebra_dimt_tunnel.py:1538 — proc.communicate(timeout=5) after proc.kill() can itself raise TimeoutExpired, leaving the client unreaped and replacing the real assertion failure with a timeout traceback. Pre-existing in both old copies, but consolidating into _run_client() makes it a one-place fix: wrap the post-kill communicate in its own try, or use proc.wait(timeout=5) and accept empty output.
• [types] test_zebra_dimt_tunnel.py:1539 — _run_client() returns (stdout, stderr, rc) while both callers return (..., rc, stderr). The two call sites unpack correctly today; the swap across one function boundary is the kind of thing a third caller gets wrong silently, since all three are strings/ints. Returning (stdout, rc, stderr) would match the surrounding convention.

Strengths

• The ifindex assertion is a genuine strengthening, not a rewrite: asserting the refusal dict whole pins the tombstone's provenance at the assertion itself, and it now runs on both loop iterations rather than once at the end — strictly more coverage than the trailing _prove_dellink_unread() it replaces. The two earlier window proofs (:1711, :1729) are untouched, so the docstring's "before the re-add and again once the re-add's barrier is in hand" still holds verbatim.
• The ifindex-0 mutant run is the right evidence to have gone and collected: it demonstrates the old assertion passed a mutant the new one catches, which is the only thing that justifies trading the window proof away.
• dplane_tid()'s docstring records the concrete failure chain (thread exits → cat error spliced into _dplane_in_fd()'s for list → bash syntax error near cat:'). That is why-not-what, and it is the detail that would otherwise cost the next reader an hour.
• _run_client() removes a genuinely duplicated popen/communicate/kill block without changing either caller's contract — _text() placement, .strip() on stderr, and returncode semantics all verified identical to the originals.

Recommended Action

1. Address the Important finding this cycle — two lines at each of :623 and :738, plus the docstring correction.
2. Consider the Suggestions opportunistically.

[kkroo]

Lease: penstock drive (session a78aa249) until 04:00Z. I'm fixing the Important finding from the b23088a9 review: the unguarded dplane_tid() sentinel at the hold_dplane_sendmsg and hold_dplane_in_recvmsg call sites. A disposition follows.

🤖 Generated with Claude Code

[kkroo]

Pushed 8a915ce9, which fixes the Important finding from the b23088a9 review.

• One resolve per hold. hold_dplane_sendmsg() and hold_dplane_in_recvmsg() no longer look up the TID themselves. They pass the FD resolver as trace_fds_for=_route_netlink_fds / _dplane_in_fd. _hold_dplane_syscalls() resolves the TID once, pytest.fails when it is empty, and applies the resolver to that same TID. The -p target and the trace-fds narrowing can no longer come from two different resolves, so the silent unnarrowed arm (BLO-28405) is gone.
• Docstring. Both remaining dplane_tid() callers, inject_netlink_syscall_failure() and _hold_dplane_syscalls(), now pytest.fail on the "" sentinel. That makes "which every caller reports" true, so the docstring is left as written. A comment at _hold_dplane_syscalls() records why the resolve is centralized.
• Suggestions, not taken. The post-kill communicate timeout and the _run_client() tuple order are not addressed in this cycle.
• Verification. py_compile passes. The topotest needs a privileged FRR build, so CI runs it.

Lease released.

@allyblockcast please re-review at 8a915ce9.

🤖 Generated with Claude Code

[kkroo]

Withdrawn. I posted this before seeing that the penstock drive session (a78aa249) had already fixed the same Important finding in 8a915ce, under a lease that it has since released. 6326512deb was never pushed, so the PR head is 8a915ce, and that is what Ally should review.

8a915ce also closes the double resolve. _hold_dplane_syscalls() resolves the TID once and pytest.fails on "", and the holds pass in an fd resolver instead of a TID. My version did the same thing with a helper, so I'm dropping it. The two suggestions (_run_client() tuple order, reaping a client that outlives kill()) are still open. Both are non-blocking; I'll put them in a separate PR if they're wanted.

I'm running the same two devbox checks against 8a915ce and will post the results here.

[kkroo]

Devbox results at 8a915ce, the current head. Ubuntu 24.04 CI image (master zebra), with this head's zebra_dimt_tunnel mounted over the image's copy:

Run	Result
zebra_dimt_tunnel module	21 passed, 1 skipped
parked test, instrumented so the first dplane_tid() call returns "" and later calls resolve (the miss-then-hit case from the Important finding)	FAILS: zebra_dplane worker not found -- cannot hold the dplane worker

So the miss now fails the hold loudly, where b23088a armed it with trace_fds=None.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8a915ce

The Important finding is fixed, and fixed structurally rather than by adding a second guard. Passing the resolver instead of the resolved FD set means the narrowing and the -p target are no longer two values that should agree — they are one TID used twice, so the unnarrowed arm is unreachable rather than merely unlikely.

Prior Findings Dispositioned (1)

• prior:b23088a important 1 — fixed — tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:764 — trace_fds = trace_fds_for(router, worker) if trace_fds_for else None applies the resolver to the TID resolved one line earlier at :759 and guarded at :760-763, so there is no second resolve to disagree with the -p target at :778. Both hold sites now pass a resolver rather than a resolved set (:622 trace_fds_for=_route_netlink_fds, :736 trace_fds_for=_dplane_in_fd) and neither calls dplane_tid() any more. dplane_tid() has exactly two callers left — :499 and :759 — and both pytest.fail on the "" sentinel (:500-503, :760-763), so the docstring's "which every caller reports" at :876 is now true as written and correctly left alone.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [errors] test_zebra_dimt_tunnel.py:886 — return tids[0] if len(tids) == 1 and tids[0].isdigit() else "" collapses "no zebra_dplane thread" and "more than one" into the same "", and both callers render it as worker not found. If FRR ever runs multiple dplane threads the failure will name the wrong cause. _dplane_in_fd() at :694-699 takes the opposite approach for the identical none-vs-several split — found {}: {} — and that asymmetry is the only reason this is worth a line. Returning the count, or widening the two caller messages to say "not uniquely resolved", would match the convention the file already sets.

Strengths

• Resolving once inside _hold_dplane_syscalls() and passing trace_fds_for as a callable is the stronger of the two fixes offered in the prior review. Threading a resolved TID through would have left two parameters that a future caller could desynchronize; a resolver applied to the locally-resolved worker cannot be aimed at a different TID without editing :764 itself. The comment at :754-757 records that intent and names BLO-28405, so the next reader learns why the indirection exists rather than reading it as ceremony.
• Moving the FD scan inside _hold_dplane_syscalls() also puts it after require_strace() (:758 before :764), which is an unadvertised improvement: previously hold_dplane_sendmsg() ran _route_netlink_fds() before the strace check, so on a host with TOPOTESTS_ALLOW_MISSING_STRACE set and no strace installed the FD scan's pytest.fail (:461) could beat tracing_unavailable()'s pytest.skip (:50). The opt-out now behaves as its docstring at :40-48 promises.
• dplane_tid()'s rejection is total, not partial: a non-numeric pid short-circuits at :879, .split() at :885 means any stray token spliced into the loop output makes len(tids) != 1, and the glob failing to expand yields no output at all because [ "" = zebra_dplane ] short-circuits before basename. Every one of those lands on a guarded pytest.fail rather than on a malformed shell command.
• The ifindex the new tombstone assertion pins at :1760 is the right one on both paths. The retry loop's only non-exception exit is the break at :1692, which is gated on removed["ifindex"] == ifindex, and the retry path re-binds ifindex at :1713 from the re-created link — so the value reaching the refusal assertion is always the index of the link whose RTM_DELLINK is still held, matching the two _prove_dellink_unread() calls at :1714 and :1732 that remain.
• The three-key refusal dict matches the shape asserted everywhere else in the file (:1105, :1232, :1690, :1927), so asserting it whole is consistent rather than a new convention, and _prove_dellink_unread() is still exercised at its two window-proof sites — the removed third call was redundant with the assertion that replaced it.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.