Sitelet https://github.com/Stephenson-Software/trace-client-python/pull/7
Skip to content

Keep the sender thread alive if _send ever raises - #7

Merged
dmccoystephenson merged 1 commit into
mainfrom
trace/guard-sender-thread
Oct 3, 2026
Merged

Keep the sender thread alive if _send ever raises#7
dmccoystephenson merged 1 commit into
mainfrom
trace/guard-sender-thread

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Sep 30, 2026

Copy link
Copy Markdown
Member

Summary

  • TraceClient._drain now wraps each self._send(body) call in try / except Exception: # noqa: BLE001, logging [trace] sender failed on ... at DEBUG and continuing the loop. The sender thread's survival no longer depends on every future edit to _send keeping all raising code inside its own try.
  • A regression test patches _send to raise once, then asserts that threading.excepthook is never called, that a later report is still delivered, and that the failure is logged at DEBUG only.
  • No behavior change for any input known today: _send already catches everything it can raise. Nothing Java-visible changed (wire format, opt-out values, disabled_reason strings and defaults are untouched), so no matching change is needed in trace-client-java.
  • No documentation change is needed. The README's "Never raises" row and the class docstring already describe this promise; this change makes the code hold to it more robustly.

Closes #5

Test plan

  • python3 -m unittest -v: Ran 29 tests, OK (local interpreter)
  • Stash-and-run: with the _drain change stashed, test_the_sender_thread_survives_an_unexpected_error_from_send FAILS; with it restored, the test PASSES
  • CI Build matrix test (3.8), test (3.10), test (3.12) green

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson @claude
Keep the sender thread alive if _send ever raises
94952e6 
_drain now wraps each _send call in a try that logs at DEBUG and moves on,
so the thread's survival no longer depends on every future edit to _send
keeping all raising code inside its own try. No behavior change today.

Closes #5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

dmccoystephenson commented Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Self-review rubric (the rubric below is scored against the PR head after CI went green):

  • Scope: PASS. Only trace_client/trace_client.py (_drain, +4/-1) and tests/test_trace_client.py (one new test) are modified, and both are required by Sender thread in _drain has no outer guard around _send #5.
  • Tests-new: PASS. No new public name was added. The new branch in _drain is exercised by test_the_sender_thread_survives_an_unexpected_error_from_send.
  • Tests-fix: PASS. This was confirmed empirically with stash-and-run: with the _drain change stashed, the new test FAILED (thread died, nothing delivered); with the change restored, it PASSED.
  • Sibling structure: PASS. No new files. The test follows the existing test_a_base_url_without_a_scheme_is_logged_not_a_dead_thread pattern (patched threading.excepthook, assertions on self.log).
  • Sibling renames: PASS. Nothing was renamed.
  • Docs: PASS. The README "Never raises" row and the class docstring already state the promise, and no documented behavior changed. The Phase 7 table was re-checked: usage example, promises table, opt-out list, wire format, Building, headers and pyproject.toml are all unaffected.
  • Issue resolution: PASS. Sender thread in _drain has no outer guard around _send #5 asks for the _send call in _drain to be wrapped in try/except Exception # noqa: BLE001 with a DEBUG log, and for a regression test that patches _send to raise once, asserts threading.excepthook is not called and asserts a later report is delivered. All of that is present.
  • CI: PASS. test (3.8), test (3.10) and test (3.12) are green on the head.
  • Stdlib-only: PASS. No import lines were added, and pyproject.toml is untouched.
  • Py3.8 floor: PASS. test (3.8) is green. The backslash-continued multi-context with in the test is valid on 3.8.
  • Never-raises / never-blocks: PASS. The new handler lives on the sender thread and only logs. report() and close() are unchanged.
  • Log level: PASS. The only added _LOG call is _LOG.debug("[trace] sender failed on %s: %s", ...).
  • Version agreement: PASS. No version string changed (0.3.0 everywhere).
  • Exports: PASS. No public names were added.
  • Java parity noted: PASS. The PR body states that nothing Java-visible changed.

Observations for the reviewer (judgment calls, not blocking):

  • trace_client/trace_client.py:203: the handler catches Exception, not BaseException. A SystemExit/KeyboardInterrupt raised inside _send would still end the thread. This matches the except Exception convention used everywhere else in the file and the scope of Sender thread in _drain has no outer guard around _send #5.
  • trace_client/trace_client.py:204: the log line includes the full request body, as the existing could not deliver %s line in _send already does. It is DEBUG only, so it stays consistent with current behavior.

Result: all rubric items pass with CI green. Merging needs a human, since this dispatch is not authorized to merge.

This review comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit e595128 into main Oct 3, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sender thread in _drain has no outer guard around _send

1 participant

@dmccoystephenson