Skip to content

fix: downgrade BrokenPipeError ping log from ERROR to WARNING #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
palantir-valiot wants to merge 1 commit into
base: main
Choose a base branch
from

fix: log BrokenPipeError/OSError in _ping_pong at WARNING instead of …

28655e4
Select commit
Loading
Failed to load commit list.
Open

fix: downgrade BrokenPipeError ping log from ERROR to WARNING #54

fix: log BrokenPipeError/OSError in _ping_pong at WARNING instead of …
28655e4
Select commit
Loading
Failed to load commit list.
Palantir - Valiot / Pulse PR Reviewer completed May 1, 2026 in 1m 46s

Reviewed (1 comment)

Review

Functional Change

The downgrade of BrokenPipeError (and OSError) ping logs from ERROR to WARNING is correct and appropriate. It aligns with the existing _unsubscribe pattern and correctly reduces alert noise for an expected, self-healing failure path. Other unexpected exceptions continue to log at ERROR, which is the right behavior.

Issue Found

Severity Finding
Minor Docstring bracket imbalance — the "fix" on line 7 adds an extra ], changing ]] to ]]]. The original was already balanced (3 [ and 3 ]), so the change introduces a syntax error in the docstring.

Minor Note

Catching OSError in _ping_pong is slightly broader than the existing _unsubscribe method (which catches only BrokenPipeError). In practice this is unlikely to cause issues since self._conn.send() is not expected to raise non-connection OSError subclasses, but it is a minor inconsistency with the existing pattern cited in the PR description.

Recommendation

Revert the docstring bracket change (remove the extra ]) before merging.

View more details on Palantir - Valiot