canary-load: keep going on any exception, fail at the end if any collected

[bosconi]

Motivation

test/canary-load is the 23-hour validator the qa-canary pipeline runs against the shared sandbox every night. Its chunk loop had three exception handlers, and anything they did not recognize propagated out of the loop. That did two things: it ended the run early, and, because the rethrow of collected failures sits after the loop, it dropped every failure collected before the abort from the annotation and the analytics DB. Two runs in the last week ended that way:

• #986 aborted at 4h54m on AssertionError: <Response [502]> upstream server not available, the balancer's answer while environmentd restarted for the v26.40.1 cutover. http_sql_query asserted status_code == 200, and a bare AssertionError matched no handler.
• #982 aborted at 7h on Temporary failure in name resolution from the agent's resolver. That arrives as a requests.ConnectionError, which the connection handler catches, but the message was not in CONNECTION_ERROR_STRINGS, so it re-raised.

Description

Three commits, one per cause:

1. Add Temporary failure in name resolution to CONNECTION_ERROR_STRINGS, so the DNS blip restarts the chunk like the other transport failures.
2. In http_sql_query, retry a 5xx a few times with a short sleep, then turn any non-200 into a FailedTestExecutionError instead of asserting. The chunk loop collects it like a SQL failure.
3. Collapse the three handlers into one except Exception that classifies via a new failure_messages helper: connection errors restart the chunk and stay out of the verdict, testdrive errors yield one entry per error as before, and any other exception is collected with its traceback as details. The run always reaches the end of its runtime and goes red if anything was collected.

One behavior change beyond the two failure cases: a psycopg OperationalError whose message is not a connection error used to re-raise and now is collected. Connection failures are still stdout-only and still do not fail the build; that is unchanged.

Verification

No test file covers test/canary-load and the workflow needs the canary credentials, so verification was a local harness against the module: http_sql_query with requests.post mocked to return 502/502/200, 502 forever, 403 once, and 200; and failure_messages plus the handler body against a multi-error FailedTestExecutionError mixing a connection error, a real error and the DistinctBy exclusion, a CommandFailureCausedUIError, psycopg OperationalError with and without a connection string, the DNS ConnectionError, the bare AssertionError from #986, and a KeyError. Each routed as described above. ruff, black and pyright are clean on the file.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KamXNLq4ze5CGpai1mGmrF

[def-]

QA LLM Review

1. MEDIUM -- connection-error classification never sees the transport text for testdrive failures

test/canary-load/mzcompose.py:228

is_connection_error is applied only to message, but every testdrive-sourced failure arrives with the constant message Executing - failed! and the real error text in details. So the whole connection-error path is inert for the testdrive half of the test (the DELETE, the 8 validation SELECTs), and a transport blip there is collected as a hard failure that turns the nightly red. This predates the PR, but it is the classifier this diff rewrites, and it defeats the stated goal for the dominant failure path.

Details

Composition._read_testdrive_junit_errors (misc/python/materialize/mzcompose/composition.py:1077) builds TestFailureDetails(message=f"Executing {location} failed!", details=failure_text); for c.testdrive(dedent(...)) the location is stdin, so the message is literally Executing - failed!. failure_messages faithfully forwards that pair, and the handler classifies on the first element only.

Observed in the qa-canary history: builds 913, 929 and 935 each went red with

Executing - failed!
20:1: preparing query failed: connection closed

connection closed is in CONNECTION_ERROR_STRINGS; had it been matched, those three runs would have logged a connection failure and restarted the chunk. The same blind spot makes the Non-positive multiplicity in DistinctBy suppression inert on the testdrive path (it still works on the HTTP path, where message is the raw SQL error). Note the escape hatch that still works: when the junit read yields nothing, the CommandFailureCausedUIError branch classifies on stdout+stderr, so only the FailedTestExecutionError branch is affected.

Cheapest fix, matching the semantics the HTTP path already has:

                    for message, details in failure_messages(e):
                        haystack = message if details is None else f"{message}\n{details}"
                        # TODO(def-): Remove when database-issues#6825 is fixed
                        if "Non-positive multiplicity in DistinctBy" in haystack:
                            continue
                        ...
                        if is_connection_error(haystack):

2. LOW -- collected testdrive errors lose their iteration/chunk attribution and location

test/canary-load/mzcompose.py:237

The handler now rebuilds a fresh TestFailureDetails(message=message, details=details) instead of appending the original object, dropping test_case_name_override, location and line_number. Those fields are what put iteration 753 of chunk 41 in workflow_default in the annotation heading today; after this change every collected error is attributed to the workflow as a whole.

Details

_read_testdrive_junit_errors sets test_case_name_override=self.current_test_case_name_override, which workflow_default keeps current via c.override_current_testcase_name(f"iteration {i} of chunk {count_chunk} ..."), and append_to_junit_suite (misc/python/materialize/cli/mzcompose.py:936) uses error.test_case_name_override or error.location_as_file_name() or test_case_key for the junit test case name. Dropping both leaves test_case_key. For a 23-hour run where the interesting signal is often when it broke (chunk start vs. deep into a chunk), that is the most useful part of the annotation.

Having failure_messages return list[TestFailureDetails] instead of list[tuple[str, str | None]] fixes this and gives the handler the details that finding 1 needs, in one change: the FailedTestExecutionError branch returns e.errors unchanged, and the other two branches construct one entry each.