Skip to content

fix(client): close the notification channel on drop and de-flake the parked-death tests - #22

Merged
btspoony merged 2 commits into
mainfrom
fix/pr-review-followups-client-tests
Sep 14, 2026
Merged

btspoony merged 2 commits into
mainfrom
fix/pr-review-followups-client-tests

Conversation

@btspoony

Copy link
Copy Markdown
Member

What

Two related fixes in the client notification path, both from the 2026-09-14 review of #18 (residual pr-review-followups-2026-09-14-4 + finding TEST-01):

  1. Production: Drop for HarnessClient now closes the notification channel the same way the EOF path and close()'s teardown do.
  2. Tests: the parked-death tests stop asserting a best-effort value, gain the coverage they were missing, and lose the artifacts that made them misleading.

Why

Drop (nit DEBT-02). src/client/close_ladder.rs aborted both tasks on drop but never took the notification producer. Since #18 the sole broadcast::Sender lives behind the shared Arc, so a subscription held across a drop-without-close() woke only when the aborted read task happened to be dropped, and its error was built from state with no diagnostics — even though kill_on_drop had just killed the runtime.

Flake (TEST-01). tests/client_lifecycle.rs asserted message.contains("exit code: 0"). That value comes only from the single best-effort poll in src/client/read_loop.rs:71-81 (Weak::upgrade → try_lock → try_wait), whose own comment says the child "may already have been waited on — skip silently either way". When the poll missed, the test failed on a healthy tree and the panic text accused the just-fixed bug ("the parked-recv-on-death bug is back").

Coverage (TEST-02/03/06/07, DX-01/02, DOCS-01). The parked path never asserted the stderr tail, guarded the diagnostic ordering only implicitly, used a 300 ms window where the file's convention is 1 s, asserted two things that cannot fail, left a manual-verification eprintln!, duplicated the shared run_prefix handshake inline, and carried a unit-test doc that claimed read-loop coverage it does not have. The NotificationsProducer comment claimed subscriptions clone the Sender — the inverse of the invariant the fix rests on, and a comment a maintainer could "fix" the code to match.

Changes

  • src/client/close_ladder.rs — Drop applies *lock(&self.notifications) = None; after the aborts, with a comment; the existing teardown tests hold.
  • src/client/core.rs — corrected NotificationsProducer comment; added the drop-wake unit test.
  • src/client/subscription.rs — test doc reworded (no claim of read-loop coverage).
  • tests/client_lifecycle.rs — exit-code assertions removed with the best-effort relationship documented in each test's doc comment; stderr emitted and the tail asserted on the parked path; parking window widened to 1 s; four vacuous assertions removed (the hang panics remain); eprintln! removed; inline handshake replaced with run_prefix; new end-to-end test for the post-death contracts (second close() is a no-op, a fresh subscription is born-failed).
  • .changes/unreleased/fix-parked-recv-hang-on-runtime-death.md — claim kept honest: the closed error always carries the reason and the captured stderr tail, and carries the exit code when the EOF poll reaped the child (best-effort). One new fragment for the Drop change.

Verification

  • Discrimination for the Drop change: the new unit test fails when the take is removed (0/1 pass, panic at the assertion), and passes when restored — the change is load-bearing.
  • Repeats: 10× each of the four parked-death/relevant tests plus the two new tests and the untouched spontaneous_death_surfaces_exit_code_and_stderr_tail control → 70 runs, 70 passes (35.4 s wall).
  • git diff origin/main...HEAD -- src/client/read_loop.rs → empty; recv's body unchanged (the subscription.rs hunk is inside mod tests).
  • rustfmt applied; no compiler warnings introduced.

Risk

The production change is two lines and only affects the drop-without-close() path; close() still takes the producer first, so a second take is a no-op. No change to the wake mechanism, the broadcast channel, or the error shape.

…without close()

`Drop` now takes the notification producer, the same `Option::take` the read
loop's EOF tail and `close()`'s `finish_teardown` perform, so a subscription
held across a drop-without-close wakes with `Error::TransportClosed`
deterministically instead of only whenever the aborted read task happens to be
dropped.

Test follow-ups on the parked-recv/death suite (from the deep review of #16-#19):

- Drop the `exit code: 0` assertions: the read loop attaches the code from a
  single best-effort poll of the child, so a healthy build may legitimately
  omit it. Document that instead of asserting it.
- Widen the parked scripts to the file's 1 s park convention and the outer
  bounds to 5 s, so the parked window is reliable and a genuine hang still
  fails loudly.
- Replace the constant-string closed-reason assertions with the behavioural
  one: the captured stderr tail must reach the parked `recv()` / `Session::run`.
- Delete the vacuous `elapsed < 2 s` checks (reaching the `TransportClosed`
  arm already proves the outer bound was met) and the leftover
  `eprintln!("CORRECT: …")` verification print.
- Reuse `run_prefix("m-1")` for the `Session::run` script instead of an inlined
  handshake copy.
- Add end-to-end coverage for the post-death teardown contracts: a
  subscription created after a spontaneous EOF death is born-failed, and a
  second `close()` is a no-op; plus a unit test for the `Drop` wake.

Docs: correct the `NotificationsProducer` ownership comment (subscriptions keep
a `Receiver`, never a `Sender`) and the subscription test doc (it owns its
channel, so the client-side drops are covered elsewhere).
@cursor

cursor Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Small change limited to drop-without-close teardown; idempotent with existing EOF/close paths and covered by new and updated tests.

Overview
HarnessClient drop without close() now clears the shared notification producer (same as EOF teardown and close()), so a parked NotificationSubscription::recv gets Error::TransportClosed right away instead of hanging until the aborted read task is torn down.

Tests and docs align with that contract: parked-death / Session::run regressions stop asserting a best-effort exit code (source of flakes), assert the stderr tail instead, use a 1 s park window, add coverage for post-death born-failed subscriptions and idempotent second close(), plus a unit test for drop-waking parked recv. Comments clarify subscriptions never hold a broadcast Sender.

Reviewed by Cursor Bugbot for commit de200aa. Configure here.

…citations

The closed-reason substring is not best-effort: every `TransportClosed`
branch of `NotificationSubscription::recv` builds its error from the same
constant, and `Session::run` propagates the error unchanged. The assertion
removed alongside the unsound exit-code one was therefore sound, and is
restored in the parked-path arms of both end-to-end tests.

The exit-code assertions stay removed (the read loop attaches the code from
a single best-effort poll of the child, whose emission is conditional), as
do the `elapsed < 2s` checks, which only restated the outer timeout that
gates the arm.

Hard-coded `read_loop.rs` line numbers in the test comments are replaced by
symbol references ("the bounded stderr drain", "the best-effort exit poll",
"the producer take") so the comments cannot rot.

The drop-without-close changelog fragment no longer describes the SDK's own
test methodology: the consumer-relevant half (the diagnostics carried by the
error) folds into the remaining bullet.
@btspoony
btspoony merged commit 82aee8d into main Sep 14, 2026
6 checks passed
@btspoony
btspoony deleted the fix/pr-review-followups-client-tests branch September 14, 2026 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant