Skip to content

Fix flush thread crashing (AttributeError) on a real server HTTP 500 - #41

Open
jaideeppyne wants to merge 1 commit into
logtail:masterfrom
jaideeppyne:fix/flusher-attributeerror-real-500
Open

Fix flush thread crashing (AttributeError) on a real server HTTP 500#41
jaideeppyne wants to merge 1 commit into
logtail:masterfrom
jaideeppyne:fix/flusher-attributeerror-real-500

Conversation

@jaideeppyne

Copy link
Copy Markdown

Summary

Fixes #25.

FlushWorker.step decides whether to log an upload failure with:

if response.status_code == 500 and getattr(response, "exception") != None:

getattr(response, "exception") is called without a default. The two things step can receive are asymmetric:

  • On a network error, Uploader.__call__ catches requests.RequestException and returns Fake500(e) — which has an .exception attribute.
  • On a genuine server HTTP 500, session.post(...) returns a real requests.Response — which has no .exception attribute.

So on a real 500, status_code == 500 is True, then getattr(response, "exception") raises AttributeError. That propagates out of step()run(), so the background flush thread dies. Every subsequent log is enqueued but never sent for the rest of the process — silent log loss. This matches the reporter's "happens occasionally" (real 500s are intermittent).

Fix

-            if response.status_code == 500 and getattr(response, "exception") != None:
+            if response.status_code == 500 and getattr(response, "exception", None) is not None:

A real Response now yields None and skips the branch; the network-error Fake500 path still logs its exception as before. (Also switched != Noneis not None to match idiom.)

Tests

Two regression tests in tests/test_flusher.py:

⚠️ Worth noting: the existing tests use mock.MagicMock(status_code=500), and MagicMock auto-vivifies .exception, which is why they never caught this — the new test deliberately uses a real Response.

Full tests/test_flusher.py suite passes (11 tests).

`FlushWorker.step` checked `getattr(response, "exception") != None` without a
default. On a network error the uploader returns a `Fake500` that carries an
`exception` attribute, but a genuine server HTTP 500 is a real
`requests.Response`, which has no such attribute. So on a real 500 the
`getattr` raised `AttributeError`, which propagated out of `step()`/`run()` and
killed the background flush thread — every subsequent log was then queued but
never sent for the life of the process.

Pass a default (`getattr(response, "exception", None) is not None`) so a real
Response skips the branch instead of crashing, while the network-error Fake500
path still logs as before.

Adds two regression tests: a real `requests.Response` 500 must not crash the
worker (fails on the old code with the exact AttributeError from logtail#25), and the
Fake500 network-error path must still log. Note: the existing tests use
`MagicMock(status_code=500)`, whose auto-vivified `.exception` masks the bug, so
the new test uses a real Response.

Fixes logtail#25.
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.

Flusher fails to log: AttributeError: 'Response' object has no attribute 'exception'

1 participant