field notes · CrowdStrike/falconpy · issue #1508

The Anatomy of an Intermittent 500

Manny Castillo · Senior SDET · August 2026

 

14 tests, no credentials required, 100% of added lines covered

ShareLinkedIncopy link

An SDK reported a 500 that never came from the API. The message was a Python exception, the status code was manufactured, and the response header that support needed to research the failure had been thrown away. The real fault was one line in the error handler, on the only path that never gets exercised when things are going well. Budget ~12 minutes.

1. A 500 with no trace ID

falconpy#1508 is a short, unusually well written report. Calls to UserManagement.grant_user_role_ids() were intermittently returning this:

{
  "body": {
    "errors": [
      {"message": "'bytes' object has no attribute 'get'", "code": 500}
    ]
  }
}

That message is a Python AttributeError, not an API response. Somewhere inside the SDK, code called .get() on a bytes object, the exception was caught by a broad handler, and the string was packed into an error envelope that looks exactly like a real server error.

The 500 is a constant. SDKError._code is 500, and the catch-all raises it without passing a code through, so whatever the upstream actually answered, the caller is handed a 500. A 502 from a gateway, a 503 from a load balancer and a 429 from a rate limiter all arrive as the same indistinguishable number.

The reporter’s real complaint was not the crash. It was that a manufactured response carries no X‑Cs‑TraceId, so CrowdStrike support had no way to look up what happened upstream. The bug destroyed the evidence needed to diagnose it.

2. Read the neighbouring patch first

There was already an open pull request touching response handling. Before writing a line, I read it.

PR #1428 fixes a different issue and edits the same function I would need to change, calc_content_return(). The tempting conclusion is that someone already fixed this. The useful move is to measure. I checked the branch out into a git worktree and ran my reproduction against it:

Two of four, and incidentally, as a side effect of adding a content-type branch rather than by touching the failing line. The root cause survives that patch untouched.

That table is worth more than the fix itself. It turns “these might overlap” into a specific, checkable claim, which is the difference between a competing patch and a complementary one.

3. One line, two ways to reach it

Every response in FalconPy funnels through one function that turns a requests.Response into the SDK’s documented dictionary. At the end of it sits an error logging block:

# Catch and log API response errors
try:
    if resp.status_code >= 400:
        _message = None
        _errors = returned.get("body", {}).get("errors", [])

returned is only a dictionary on the JSON and text/plain paths. On the binary path it is raw bytes, either resp.content assigned directly when the body is empty, or via Result.full_return, which returns bytes(self.resources) rather than a dictionary whenever the body is binary.

So the trigger is a two part condition, and both parts have to hold at once: status ≥ 400, so the error block runs at all, and a body that is not JSON or text/plain, so returned is bytes when it gets there.

Note the asymmetry, because it is the whole reason this was hard to chase. The trigger is any status at or above 400, but the symptom is always precisely 500. Six different upstream failures collapse into one error message, and the one number that would have told you which failure you hit is the number that gets overwritten.

That intersection is also why it is intermittent. It is not the endpoint misbehaving. It is a gateway, load balancer or WAF in front of the API answering on its behalf with an HTML page or an empty body, which happens on no schedule anyone controls.

4. Trace it yourself

Pick a status code and a response shape. The panel shows which branch of the normaliser runs, what type returned holds when it reaches the error check, and what the caller finally gets, before and after the patch.

Three things are worth poking at. Keep the gateway HTML page selected and cycle through 403, 429, 500, 502 and 503. The left column reports 500 every time, while the right column preserves what actually happened. That flattening is the bug’s real cost.

Then set the status to 200 with a binary payload. Both columns return raw bytes, unchanged. That is the file download contract the SDK documents, and any fix that breaks it is worse than the bug. Finally, try text/plain with a non-JSON message. Neither column crashes, but neither returns anything useful either. That is issue #1154, the one PR #1428 exists to fix.

5. Fixing it where the type actually varies

The obvious patch is to coerce every non-JSON body into an error envelope. That is also the patch that breaks RTR file downloads, sensor installers and report exports, all of which return bytes on purpose.

The safety argument turns out to be positional. The failing line already lives inside if resp.status_code >= 400. A successful download returns 2xx and never enters that block. So the guard goes there, and no operation level “is this endpoint binary?” metadata is needed, which is fortunate, because the SDK does not cleanly expose any.

     if resp.status_code >= 400:
+        if not isinstance(returned, dict):
+            # An error was returned as content we could not parse as JSON,
+            # leaving us with a binary payload. Normalize it to the standard
+            # error format so the status code and the response headers - which
+            # carry the trace ID needed to research the failure - are retained
+            # instead of being discarded by an unhandled exception. (Issue #1508)
+            returned = Result()(status_code=resp.status_code,
+                                headers=resp.headers,
+                                body=build_error_body_from_payload(returned, resp.status_code)
+                                )
         _message = None
         _errors = returned.get("body", {}).get("errors", [])

isinstance(value, dict) asks whether a value is of a type, and returns a boolean. It is the right question here for a reason worth stating: the existing code was already defensive. returned.get("body", ) supplies a default. But that default protects against a missing key, not a wrong type. The call never happens, because bytes has no .get to call. Defensive looking code that defends against the wrong thing.

It is also deliberately not a try/except AttributeError. That would swallow genuine attribute errors raised by deeper code, recreating in miniature the exact catch-all problem that made this bug so hard to find. The type check asks precisely the question that matters and leaves every other failure mode free to surface.

Why Result()(...) and not Result(...)

Those look like the same thing, and both of them work. The difference is what comes out the other side. The constructor sends the body through _parse_body(), which rebuilds it from parsed components and injects a meta key:

body we built   -> ['errors', 'resources']
Result(...)     -> ['errors', 'meta', 'resources']
Result()(...)   -> ['errors', 'resources']

The call form is a legacy passthrough that assembles the dictionary directly and hands back exactly what it was given. That matters for one reason: generate_error_result(), the SDK’s own error generator twenty lines below, produces precisely that shape. Matching it means an error manufactured by this patch is indistinguishable from every other error the SDK generates, and no synthetic meta appears where the API never sent one.

The helper cannot throw

if isinstance(payload, bytes):
    message = payload.decode("utf-8", errors="replace").strip()
else:
    message = str(payload).strip()
if not message:
    message = "No content was received for this request."
if len(message) > MAX_ERROR_PAYLOAD_LENGTH:
    message = f"{message[:MAX_ERROR_PAYLOAD_LENGTH]}..."

return {"errors": [{"code": status_code, "message": message}], "resources": []}

errors="replace" is doing real work. A WAF page is not guaranteed to be valid UTF‑8, and a bare .decode() would raise UnicodeDecodeError, putting us right back where we started, throwing an exception from inside exception handling. The else branch means any type at all is handled, so this function is total: there is no input for which it raises.

The truncation matters more than it looks. Without a cap, a multi-megabyte HTML error page gets embedded whole into an error message that may then be logged, serialised and shipped somewhere.

6. Proving it without an API key

FalconPy’s test suite talks to the live Falcon API. 143 of its 158 test files need credentials, and most fail at collection time without them. My first instinct was to go get a trial account. That instinct was wrong, and the repo says why. Every unit testing workflow is gated on this:

if: |
  github.event_name == 'push' ||
  (github.event_name == 'pull_request' &&
   github.event.pull_request.head.repo.full_name == github.repository)

That last clause skips pull requests from forks. Every outside contribution is a fork PR, so the live test suite will not run on it, no matter whose credentials exist where. PR #1428 confirms it: cross repository, zero checks reported.

Which reframes the question entirely. The job is not to get credentials. It is to write tests a maintainer can run in a second and believe. The bug lives in shared plumbing, so it mocks cleanly: 14 tests built on real requests.Response objects rather than mocks, which exercises the actual case-insensitive header handling that production hits.

Then the check that makes the rest of them mean anything. I removed the guard and reran. Eight failures, every one an AttributeError on the same line. A test that passes both with and without your fix is testing nothing.

And the thing the reporter actually asked for, end to end through the exact call from the issue:

{'status_code': 502,
 'headers': {'Content-Type': 'text/html',
             'X-Cs-Traceid': 'trace-abc-123'},
 'body': {'errors': [{'code': 502,
                      'message': '<html>...502 Bad Gateway...</html>'}],
          'resources': []}}

A real 502 instead of a fabricated 500, the gateway’s own text as the message, and the trace ID intact, which was the whole point.

7. What generalises

Read the adjacent patch, then measure it. “Might be related” is a hypothesis. A worktree and a reproduction script turn it into a table, and the table is what makes a competing PR non-competing.

Error handlers are the least tested code you own. This line only executed when something had already gone wrong upstream, so it was never exercised in normal operation, and its own failure looked like the upstream failure it was trying to report. That class of bug hides indefinitely.

Catch-all handlers convert bugs into believable lies. except Exception turned an AttributeError into a well formed 500 that looked exactly like the API answering. Users escalated to support, support had no trace ID, and the loop closed with nobody able to see the real cause.

Verify the negative case. Removing the fix and watching the tests fail takes thirty seconds and is the only evidence your tests are attached to your change.

Read the CI config before optimising for it. An afternoon of chasing credentials would have bought nothing, because the workflow that needed them was never going to run. Ten lines of YAML said so.