[sapols]

Change Summary

Overview

Today, when an upload's PUT to S3 loses its connection before S3 responds, urllib3 resends it automatically. IMAP-Science-Operations-Center/imap-data-access#373 removes that resend, because resending an upload blindly has side effects. On its own, that change would turn a dropped upload connection into a failed job: upload_products would re-raise the error, and the job would exit with code 1, which Batch wouldn't retry.

This PR restores the retry one level up. upload_products now handles a dropped connection like a 503 SlowDown: it reruns the whole upload(), for up to 3 attempts in all, then exits with RETRY_EXIT_CODE so Batch reruns the job. Unlike resending the PUT, rerunning the upload checks first: the upload API returns 409 FileAlreadyExists if S3 already has the file, which upload_products already skips.

It works with imap-data-access 1.3.0 too, and should merge before the new release: the processing images pip install imap_processing from git, so they get the newest imap-data-access rather than the one in poetry.lock.

Unsurprisingly, I had Claude's help writing/understanding this fix. It seems like the right move to me—I don't want my IMAP-Science-Operations-Center/imap-data-access#373 to be the reason jobs start failing—but I welcome feedback on this approach.

File changes

• imap_processing/cli.py: retry upload errors containing "Connection aborted", and show the error in the retry warning.
• imap_processing/tests/test_cli.py: parametrize the 503 test (now test_post_processing_upload_retryable_error) over SlowDown and a dropped connection, and add a test for a retry that finds the file already stored.

Testing

All 60 tests in test_cli.py pass, and the new cases fail without the fix. I also ran upload_products end to end against both imap-data-access versions, mocking only urllib3's connection pool.

[lacoak21]

This looks good to me! We should probably also do this for the download api call right? Although I just checked and seems like we dont have any retry handling in place for that yet.

I just added a new Issue

[tmplummer]

This looks fine. One question I have about exiting with RETRY_EXIT_CODE is will that play nice with Dagster? I don't know all the details about Dagster's interface with batch. Will it care about a failed batch job and retry. I think the answer is that dagster doesn't care, but worth checking.

[lacoak21]

The exit code should work with dagster because it only will fail or succeed when the batch job fails or succeeds. A batch job's status is only ever set to failed or succeeded when all of its retries are up.