fix: resolve 404 log error for non-latest task tries in multi-host worker environments

[oboki]

In environments with multiple workers (e.g., CeleryExecutor), task logs for previous tries (i.e., not the latest try_number) may fail to load with the following error:

Could not read served logs: 404 Client Error: NOT FOUND for url: http://worker-2:8793/log/dag_id=tutorial_dag/run_id=manual__2025-05-04T12:54:25.273420+00:00/task_id=extract/attempt=5.log

As shown in the screenshots below, attempt=5 was actually executed on worker-1, but the Web UI incorrectly tries to fetch the logs from worker-2:

This happens because _get_log_retrieval_url generates the log URL based on TaskInstance.hostname, which only stores the hostname of the latest execution attempt. It does not keep track of the history of previous tries.

To fix this, I updated the logic to use the TaskInstanceHistory model, just as the "Details" tab does, so the correct hostname is used for each specific try_number.

With this change, logs for previous attempts load correctly as expected.

[jason810496]

Nice catch! Thanks for the PR, but I think we can fix on API side instead of file_task_handler side.

[bugraoz93]

Thanks a lot for the PR! Looks good. It would be great to include a test case in the unit tests where ti is not None but try_number is different than the ti.try_number to test the additional condition

[jason810496]

Nice! Looks good to me, but it seems like there is side effect for mocking session in the test, which leads to dead lock for SQLite.

[pierrejeambrun]

Nice looks good overall just a few tests to fix.

[bugraoz93]

Some of the tests are still failing. Could you please check them?

[bugraoz93]

A more general thing about routes and services: There are similar cases where we have methods inside route files. We already have some level of distinction between them. How about moving them to services? What do you think?
cc: @pierrejeambrun @rawwar @jason810496 @shubhamraj-git @ephraimbuddy

[jason810496]

A more general thing about routes and services: There are similar cases where we have methods inside route files. We already have some level of distinction between them. How about moving them to services? What do you think? cc: @pierrejeambrun @rawwar @jason810496 @shubhamraj-git @ephraimbuddy

Looks good to me to move the query helper function to the airflow-core/src/airflow/api_fastapi/core_api/services module level. (I think airflow-core/src/airflow/api_fastapi/common/db would also be a suitable location.)

Just to double-check, do you mean to move the _find_task_instance_for_try_number function directly into the services module and not introduce a new service class, right?

Since this helper simply wraps a query as a function, and the existing service classes (e.g., for connections, pools, variables) are designed for bulk operations, creating a new class might not be necessary here.

[pierrejeambrun]

Nits regarding discussion, but LGTM.

[bugraoz93]

A more general thing about routes and services: There are similar cases where we have methods inside route files. We already have some level of distinction between them. How about moving them to services? What do you think? cc: @pierrejeambrun @rawwar @jason810496 @shubhamraj-git @ephraimbuddy

Looks good to me to move the query helper function to the airflow-core/src/airflow/api_fastapi/core_api/services module level. (I think airflow-core/src/airflow/api_fastapi/common/db would also be a suitable location.)

Just to double-check, do you mean to move the _find_task_instance_for_try_number function directly into the services module and not introduce a new service class, right?

Since this helper simply wraps a query as a function, and the existing service classes (e.g., for connections, pools, variables) are designed for bulk operations, creating a new class might not be necessary here.

Yes, I agree on the second look, airflow-core/src/airflow/api_fastapi/common/db is more suitable. I intend to decouple these methods from routes. Even with collapse all in some routes, there are a lot of endpoints and making the code less readable.

Yes, it can be on the root level. For sure, it depends on the use case if we look at it in general. We aren't doing full object-oriented programming in general, it is mixed with Functional mostly due to the nature of Python. Unless we use service classes as a singleton across all routes, it can only add overhead of creating the object rather than calling the method. I don't have a strong opinion on that since the entire stack is using a mixed approach and makes sense in most cases :)

[bugraoz93]

I will take a look moving other methods from routes to proper places soon

[jason810496]

The test seems flaky, update with latest main.

[oboki]

The test seems flaky, update with latest main.

Finally, all tests have passed! Thanks @jason810496. I really appreciate your support.

[github-actions]

Backport failed to create: v3-0-test. View the failure log Run details

Status	Branch	Result
❌	v3-0-test

You can attempt to backport this manually by running:

cherry_picker 0b0ff5d v3-0-test

This should apply the commit to the v3-0-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

[jason810496]

Manual packport to v3-10-test due to conflicts: #50833

[dstandish]

This PR has some kind of bug.

When you clear a TI you see two log files with try == 1

[dstandish]

Reverting here #51135