tests: name the dplane-worker failure and keep a stuck client from masking one (BLO-38883)

[kkroo]

Ally's three non-blocking suggestions on #130, all in tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py. Test-only.

dplane_tid(): none vs several (Ally, #130 review of 8a915ce, [errors]). It folded "no zebra pid", "no zebra_dplane thread" and "several zebra_dplane threads" into one "", and both callers reported that as "worker not found", so a second dplane thread would have been diagnosed as a missing one. It now pytest.fail()s itself and names which, the way _dplane_in_fd() already tells none from several (found N: [...]). Each caller passes what it was trying to do, so the two messages keep their context. The WindowNeverOpened docstring's list of pytest.fail() sites is updated to match.

_run_client(): post-kill timeout (Ally, review of b23088a, [errors]). proc.communicate(timeout=5) after proc.kill() could raise a second TimeoutExpired, which left the client unreaped and replaced the real assertion failure with a traceback. A client still alive 5 s after SIGKILL now yields empty output and returncode None, so the caller's assertion on the reply fails on what the client did.

_run_client(): return order (same review, [types]). It returned (stdout, stderr, rc) while both callers return (..., rc, stderr). It now returns (stdout, rc, stderr); both call sites updated.

Verification

On devbox, with frr master d4467836c8's CI image (24.04) and this file mounted over the module:

Run	Result
whole module	21 passed, 1 skipped (same as #130)
mutant: dplane_tid() matches no thread, hold caller	fails: zebra_dplane worker not found: zebra 298 has no zebra_dplane thread -- cannot hold the dplane worker
same mutant, injection caller	fails: ... has no zebra_dplane thread -- netlink failure injection cannot run
mutant: dplane_tid() matches every thread	fails: zebra_dplane worker not uniquely resolved: expected one zebra_dplane TID in zebra 298, found 5: ['298', '299', '300', '301', '310'] -- cannot hold the dplane worker (before this PR that case read "worker not found")

_run_client()'s kill path cannot be reached from a topotest without a client that survives SIGKILL, so it was checked offline: the functions were extracted with ast and run against fakes. Normal, killed-then-exited and unkillable clients return ('{"a":1}\n', 0, 'err'), ('partial', -9, '') and ('', None, ''). Positive control: master's _run_client() against the same unkillable fake raises TimeoutExpired.

🤖 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: dbb922f

Three well-scoped follow-ups to #130, test-only, one file. dplane_tid()'s none-vs-several split and the (stdout, rc, stderr) reorder are both clean and fully propagated — I confirmed all four call sites in the module are updated and that dplane_tid appears in no other file in the repo. One finding on the _run_client() kill path.

Critical Issues (0)

Important Issues (1)

• [errors] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:1559 — the new post-kill except makes an unkillable client indistinguishable from a dplane-worker hold, which is the attribution this PR exists to sharpen, inverted. Returning ("", None, "") sends removed=None into _held_before_delete_result() (:1592), whose precondition is "no reply to the DEL and one recvmsg entered and not returned" — and under hold_dplane_in_recvmsg() that recvmsg shape is the expected state, not a rare one. So frozen comes back non-None and the run fails with "the hold caught the worker before the DEL's result reached main" (:1726), confidently naming zebra for what was a stuck client. Pre-PR this path raised TimeoutExpired, which was ugly but named the right component; for this one path the change trades a correct loud failure for a plausible wrong one. The raw tell is already printed — rc=None where a reaped client gives an int and a SIGKILLed one gives -9 — but nothing in the message says what rc=None means, and the surrounding prose asserts the dplane explanation.
  • Keep the control flow exactly as is and make the existing stderr={!r} carry the cause, since both WindowNeverOpened messages (:1721, :1726) already print it: in the inner handler, stdout, stderr = None, "<client did not exit 5s after SIGKILL>". One line, no caller changes, and the misattributed message then contradicts itself out loud. (_text() passes str through unchanged, so .strip() is still fine.)

Suggestions (2)

• [comments] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:146 — the rewritten WindowNeverOpened docstring re-asserts "EVERY pytest.fail() in this module", but the enumeration omits two live sites: _route_netlink_fds() (:462, reached from inject_netlink_syscall_failure() when route_netlink_fds=True) and _dplane_in_fd() (:692, reached from _hold_dplane_syscalls() via trace_fds_for). Pre-existing — the old "both inject_netlink_syscall_failure() setup paths / both _hold_dplane_syscalls() paths" phrasing missed them too — and the paragraph's actual conclusion still holds, since a raises=pytest.fail.Exception marker would absorb those two as well. But this PR rewrites that exact sentence, so it is the cheap moment to make the list match. Worth noting _dplane_in_fd() is the function the new dplane_tid() docstring cites as the none-vs-several precedent.
• [errors] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:891 — if len(tids) != 1 or not tids[0].isdigit() routes a single non-numeric match into the "not uniquely resolved" message, which then reads found 1: ['cat:'] — a header about cardinality for what is a parse problem. Unreachable in practice (basename on /proc/<pid>/task/<tid> is always numeric, and the inner cat now discards its own stderr), so this is only about the message a future reader would get from the defensive branch. Splitting the non-digit case out, or softening the header to cover both, would keep the naming as precise as the rest of the function now is.

Strengths

• The none-vs-several split is the right fix and is done properly: each branch names which failure, echoes the observed value (read {!r}, found {}: {}), and threads the caller's purpose so both sites keep their context. Modelling it on _dplane_in_fd()'s existing found N: [...] keeps one convention in the module instead of inventing a second.
• Making purpose a required positional rather than defaulted means the two call sites could not silently drift into a context-free message — the compiler-equivalent enforcement for a convention that is otherwise easy to forget.
• The return-order fix is the kind of latent foot-gun worth spending a diff on: (stdout, stderr, rc) against two callers returning (..., rc, stderr) is a silent swap the day a third caller is added. Both sites updated in the same commit.
• The verification table is genuinely adversarial rather than confirmatory — mutating dplane_tid() to match no thread and to match every thread, checking both callers' messages separately, and running a positive control on master's _run_client() against the same unkillable fake to show the old code raises. The ast-extraction harness for a path no topotest can reach is a reasonable answer to "how do you test an unkillable process".
• Docstrings were updated alongside the code rather than left to rot, including the cross-reference in WindowNeverOpened.

Recommended Action

1. Address the Important finding this cycle — it is a one-line change to the inner handler and needs no caller updates.
2. Consider the two Suggestions opportunistically.

[kkroo]

Lease: kkroo drive session (Omar, 7ee27e), about 30 min, at dbb922f3. Taking Ally's 21:14Z Important (test_zebra_dimt_tunnel.py:1559, post-SIGKILL stderr). Plain push only; disposition and release follow.

[kkroo]

Lease: kkroo drive session (Omar, 7ee27e), about 30 min, at dbb922f3. Pushing a prepared change (finding fix or conflict resolution); disposition follows.

[kkroo]

Pushed 7414b1b9 on top of dbb922f3 (kkroo drive session 7ee27e). It is one commit, with no rebase and no force-push.

Important (test_zebra_dimt_tunnel.py:1559, unkillable client read as a dplane hold) — fixed the way the review suggested. Control flow is unchanged. The inner TimeoutExpired handler now sets stdout, stderr = None, "<client did not exit 5s after SIGKILL>". Both WindowNeverOpened messages (:1721, :1726) already print stderr={!r}. So when a stuck client is misattributed to the hold, the message now contradicts itself in its own text. _text() passes str through, so .strip() is unaffected. The _run_client docstring now says the stderr carries the marker, and why.

Checked:

• I ran _run_client and _text extracted from the file (via ast) against stub processes. A client that survives SIGKILL returns ("", None, "<client did not exit 5s after SIGKILL>"). A client reaped after the kill still returns ("", -9, "boom") from its real stderr.
• python3 -m py_compile is clean.
• No topotest was run here. The change is to the harness's give-up branch only.

[kkroo]

Lease released (kkroo drive session 7ee27e). Pushed 7414b1b9; disposition above.

[allyblockcast]

Ally — Consolidated PR Review

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

One line of code since the previous head, and it is exactly the remedy the open finding asked for: the control flow is untouched and no caller changed. I re-traced the new value through both _run_client() consumers at this head, including the one the finding did not name.

Prior Findings Dispositioned (1)

• prior:dbb922f important 1 — fixed — tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:1562 — the inner handler now sets stdout, stderr = None, "<client did not exit 5s after SIGKILL>", so an unkillable client is no longer indistinguishable from a dplane-worker hold. Checked at this head rather than from the patch: _text(None) returns "" (:804), so the stdout half is byte-identical to before and json.loads("") / "".splitlines() behave exactly as they did; proc.returncode is still None on this path, since the process was never reaped; and both messages that would otherwise blame zebra print the marker through their existing stderr={!r} — :1724 and :1731. The misattributing message now contradicts itself out loud, which was the point.
  • I also checked the path the finding did not cover. _direct_answer()'s caller asserts rc == 0 first (:1778) and carries stderr in its tuple, so rc=None trips that assert and surfaces the marker; the later answers assert (:1780), which omits stderr, cannot fire ahead of it. Both consumer paths are covered, not just _held_delete().

Critical Issues (0)

Important Issues (0)

Suggestions (2)

Both carried unchanged from the dbb922f review and both still open — neither blocks, and deferring them again is a reasonable call.

• [comments] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:146 — the rewritten WindowNeverOpened enumeration now names dplane_tid() and the strace attach failures, but two pytest.fail() sites remain outside it: _route_netlink_fds() (:462) and _dplane_in_fd() (:692). The paragraph's conclusion is unaffected — a raises=pytest.fail.Exception marker absorbs those two as well — so this is only about the list matching the module.
• [errors] tests/topotests/zebra_dimt_tunnel/test_zebra_dimt_tunnel.py:891 — if len(tids) != 1 or not tids[0].isdigit() still routes a single non-numeric match into the "not uniquely resolved" message, which then reads found 1: ['cat:'] — a cardinality header for what is a parse problem. Unreachable in practice, so this only affects the message a future reader would get from the defensive branch.

Strengths

• The fix took the narrow option: one assignment inside the existing handler, no change to the return shape, no caller touched. The alternative — signalling the unkillable case through the return tuple — would have rippled into _held_delete(), _direct_answer() and both of their assertion sites for no added signal.
• The docstring was extended in the same commit to say why the marker exists ("a held dplane worker also leaves the DEL unanswered, and the messages that blame it print stderr"), which records the attribution hazard rather than just the behaviour. That sentence is the thing that stops the marker being removed as noise later.
• Choosing a string the existing {!r} formatters already print, instead of adding a new field to the failure messages, means the fix needed no change at either WindowNeverOpened site.

Recommended Action

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