bug: infrahubctl load reports failed import tasks as successes, hiding an HTTP 500 from update_optional_relationships

[ogenstad]

Summary

LineDelimitedJSONImporter.execute_batches never unpacks the pair its batch yields, so its entire error-handling branch is unreachable and a failed import task is recorded as a success.

Fixing that shows the failures it has been hiding: update_optional_relationships gets an HTTP 500 from the server during the export/import integration tests. Both halves are described below, because the second is only visible once the first is fixed.

Everything needed to reproduce and fix this is inline. Nothing here depends on reading another branch.

1. Failed import tasks are reported as successes

InfrahubBatch.execute() is an async generator yielding (node, result):

# infrahub_sdk/batch.py
async def execute(self) -> AsyncGenerator:
    ...
    for completed_task in asyncio.as_completed(tasks):
        node, result = await completed_task
        if isinstance(result, Exception) and not self.return_exceptions:
            raise result
        yield node, result          # <- a 2-tuple

execute_batches binds that pair to a single name:

# infrahub_sdk/transfer/importer/json.py, in execute_batches
async for result in batch.execute():        # result is (node, value), not value
    if self.console:
        progress.update(progress_task, advance=1)
    if isinstance(result, Exception):       # a tuple is never an Exception -> always False
        ...                                 # entire error branch unreachable
    else:
        results.append(result[1])           # the [1] is the tell: it knew it had a pair

The importer builds its batches with return_exceptions=True (await self.client.create_batch(return_exceptions=True)), so a failed task does not raise: the batch yields (node, exc). That pair goes down the else and the exception object is appended to results as though it were an imported node.

Effects on infrahubctl load:

• failed tasks print nothing, since the exceptions list stays empty and "N failures" is never reached
• --continue-on-error has nothing to report and is effectively a no-op
• without --continue-on-error, raise result never fires, so it does not stop on the first failure
• exception objects are returned among the imported results

Every other batch.execute() call site in the SDK unpacks the pair (async for _, response in ... in client.py, exporter/json.py, task/manager.py), so this looks like a slip rather than intent.

Reproducing without any other branch

Against stable as it is today:

import asyncio
from io import StringIO

from rich.console import Console

from infrahub_sdk import InfrahubClient
from infrahub_sdk.batch import InfrahubBatch
from infrahub_sdk.exceptions import GraphQLError
from infrahub_sdk.transfer.importer.json import LineDelimitedJSONImporter
from infrahub_sdk.transfer.schema_sorter import InfrahubSchemaTopologicalSorter


async def main() -> None:
    console = Console(record=True, file=StringIO(), width=200)

    async def failing_task() -> None:
        raise GraphQLError(errors=[{"message": "boom", "path": ["TestPersonCreate"]}])

    batch = InfrahubBatch(return_exceptions=True)
    batch.add(task=failing_task)

    importer = LineDelimitedJSONImporter(
        client=InfrahubClient(),
        topological_sorter=InfrahubSchemaTopologicalSorter(),
        continue_on_error=True,
        console=console,
    )
    results = await importer.execute_batches([batch])
    print("results:", results)
    print("output:", repr(console.file.getvalue()))


asyncio.run(main())

Observed:

results: [GraphQLError("An error occurred while executing the GraphQL Query None, [{'message': 'boom', 'path': ['TestPersonCreate']}]")]
output: ''

The exception is returned as a result and nothing is printed. Expected: an empty results, and 1 failures plus boom on the console.

Fix

In execute_batches:

-            async for result in batch.execute():
+            async for _, result in batch.execute():
                 if self.console:
                     progress.update(progress_task, advance=1)
                 if isinstance(result, Exception):
@@
                     exceptions.append(error_str)
                 else:
-                    results.append(result[1])
+                    results.append(result)

The success path is unchanged: result[1] and the unpacked result are the same value. The only difference is that exceptions reach the branch that was always meant to handle them.

One related defensive change is already on stable and needs no action: the error branch reads err.get("message", err) rather than err["message"], so an error entry without a message key cannot raise a KeyError out of the branch whose job is to keep going. It currently guards code that cannot run, and becomes live with the fix above.

Suggested regression tests

New file tests/unit/sdk/test_json_importer.py:

"""How `execute_batches` reports a failed task when it was told to keep going."""

from __future__ import annotations

from io import StringIO

import pytest
from rich.console import Console

from infrahub_sdk import InfrahubClient
from infrahub_sdk.batch import InfrahubBatch
from infrahub_sdk.exceptions import GraphQLError
from infrahub_sdk.transfer.importer.json import LineDelimitedJSONImporter
from infrahub_sdk.transfer.schema_sorter import InfrahubSchemaTopologicalSorter
from tests.helpers.cli import remove_ansi_color


def recording_console() -> Console:
    return Console(record=True, file=StringIO(), width=200)


def importer(console: Console) -> LineDelimitedJSONImporter:
    return LineDelimitedJSONImporter(
        client=InfrahubClient(),
        topological_sorter=InfrahubSchemaTopologicalSorter(),
        continue_on_error=True,
        console=console,
    )


def batch_raising(exc: Exception) -> InfrahubBatch:
    async def failing_task() -> None:
        raise exc

    batch = InfrahubBatch(return_exceptions=True)
    batch.add(task=failing_task)
    return batch


async def test_an_error_entry_without_a_message_does_not_stop_the_import() -> None:
    """The entry is rendered whole rather than raising a KeyError out of the keep-going branch."""
    console = recording_console()
    exc = GraphQLError(errors=[{"path": ["TestPersonCreate"], "extensions": {"code": "UNDEFINED_ERROR"}}])

    results = await importer(console=console).execute_batches([batch_raising(exc)])

    output = remove_ansi_color(console.file.getvalue())  # type: ignore[attr-defined]
    assert results == []
    assert "1 failures" in output
    assert "TestPersonCreate" in output, "the entry the server sent still reaches the user"


async def test_an_error_entry_with_a_message_is_reported_by_that_message() -> None:
    console = recording_console()
    exc = GraphQLError(errors=[{"message": "boom", "path": ["TestPersonCreate"]}])

    await importer(console=console).execute_batches([batch_raising(exc)])

    assert "boom" in remove_ansi_color(console.file.getvalue())  # type: ignore[attr-defined]


async def test_a_failure_still_raises_when_not_continuing_on_error() -> None:
    console = recording_console()
    importer_stopping = LineDelimitedJSONImporter(
        client=InfrahubClient(),
        topological_sorter=InfrahubSchemaTopologicalSorter(),
        continue_on_error=False,
        console=console,
    )
    exc = GraphQLError(errors=[{"path": ["TestPersonCreate"]}])

    with pytest.raises(GraphQLError, match="An error occurred while executing the GraphQL Query"):
        await importer_stopping.execute_batches([batch_raising(exc)])

All three fail before the fix and pass after it.

Age

Present since the initial commit 796a962 (2024-09-12), confirmed with:

git log -S 'async for result in batch.execute()' -- infrahub_sdk/transfer/importer/json.py

2. The failure it has been hiding: HTTP 500 from update_optional_relationships

With the unpacking fixed, six tests/integration/test_export_import.py tests fail:

TestSchemaExportImportBase::test_step02_import_no_schema
TestSchemaExportImportBase::test_step03_export_initial_dataset
TestSchemaExportImportBase::test_step04_import_initial_dataset
TestSchemaExportImportBase::test_step05_import_initial_dataset_with_existing_data
TestSchemaExportImportManyRelationships::test_step02_import_initial_dataset
TestSchemaExportImportManyRelationships::test_step03_import_initial_dataset_with_existing_data

All share one chain:

tests/integration/test_export_import.py:127  await importer.import_data(...)
infrahub_sdk/transfer/importer/json.py:109   import_data
infrahub_sdk/transfer/importer/json.py:157   update_optional_relationships
infrahub_sdk/transfer/importer/json.py       raise result    (in execute_batches)

The underlying error is an HTTP 500 from POST /graphql/main whose body is the plain string Internal Server Error rather than a GraphQL envelope, which the SDK surfaces as:

infrahub_sdk.exceptions.base.JsonDecodeError: Unable to decode response as JSON data from
http://localhost:<port>/graphql/main. Server response: Internal Server Error

These tests do not pass --continue-on-error, so the importer raises rather than continuing. A secondary symptom in the same run is FileAlreadyExistsError: .../nodes.json already exists, which follows from the earlier steps aborting partway.

Not yet diagnosed. The 500 is a server-side crash while adding optional relationships, and the response body carries no detail, so it needs the Infrahub server log from that run to go further. It may belong in opsmill/infrahub rather than here.

This means the export/import integration tests have been passing for roughly two years while the import was partly failing.

Evidence

• Failing CI run, with the full traceback for all six tests: https://github.com/opsmill/infrahub-sdk-python/actions/runs/35078408040/job/104736495596
• Found while reviewing PR feat: unify and re-root the not-found classes under GraphQLError #1363, which touched the same function for an unrelated reason. Both changes were reverted there so an error-catalogue PR would not sit red behind a server bug. PR feat: unify and re-root the not-found classes under GraphQLError #1363 is not a prerequisite for this issue and nothing here needs to be read out of it.

Suggested order

1. Land the unpacking fix with the unit tests above.
2. Expect the six integration tests to go red, and treat that as the real bug surfacing rather than a regression.
3. Diagnose the 500 with the server log, and split it out to opsmill/infrahub if the fault is server-side.

Step 1 on its own turns CI red, so the two want to be sequenced together, or the six integration tests marked xfail with a link to this issue in between.