fix: retry asset downloads on 5xx and recover them across runs

An asset download is two hops — mint a signed URL, then fetch the bytes — and only the first went through _make_request. The second was a bare session.request with no 5xx handling, which lost two images in the 2026-10-20 09:00 run: one to a 500 the retried first hop had already recovered from, one to a 503. Both re-downloaded cleanly an hour later (106 KB and 1.3 MB), so the bytes were there all along. A third asset timed out and aborted its whole conversation. curl_cffi's RequestException subclasses OSError, so the un-wrapped timeout slipped past resolve_media's except ProviderError into the export loop's except OSError and was reported as "File write failed for conversation 6a945155" — a conversation that had already downloaded ~20 images. Nothing was wrong with any file write. The retry loop is factored out of _make_request into _send_with_retries, which returns the raw response so a bytes caller gets the same handling as a JSON one. Hop 2 goes through it: 5xx retried, Retry-After honoured, timeouts surfacing as ProviderError for resolve_media to placeholder. The hop is now logged, which it never was. Assets get their own ladder — an immediate retry, then ~2/4/8/16s with jitter kept, since a storage incident fails many assets at once — bounded by a per-asset wall-clock budget, because the timeout case costs 40s a go. Beyond the retry: - Transient failures (5xx, timeout, network) are recorded as pending_media so a later run re-fetches just that asset; capped at 3 runs. 404/403 stay permanent — settled 2026-08-17 for 19 assets. - A circuit breaker stops downloads after 5 consecutive transient failures, recording the rest as retryable. Permanent failures never trip it; any success resets the streak. - Media failures now appear in the set ids. The run that lost two assets pushed a clean "OK". Keeps FUTURE.md's per-conversation cache reset correctly closed. The README split (FUTURE.md §2) stays otial enough to justify breaking every anchor link. 37 new tests (414 passing). Verified live: both lost assets recovered, doctor green, a read-only chatgpt dns. The cross-run retry and breaker are unit-tested only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/cwa7qnGp Claude-Session: https://claude.ai/code/session_012gCWSknsWvhLnMFwa7qnGp
2026-10-20 10:32:20 -04:00
parent 805689c649
commit 8c3e00a520
12 changed files with 990 additions and 63 deletions
@@ -100,7 +100,7 @@ class TestResolveMedia:

        n = resolve_media(conv, provider, tmp_path, "provider/project/year", "images", report)

-        assert n == 1
+        assert n.downloaded == 1
        assert report.media_downloaded == 1
        assert block["local_path"] == "media/file_img1.png"
        rendered = render_blocks_to_markdown([block])
@@ -117,7 +117,7 @@ class TestResolveMedia:
        report = LossReport()

        n = resolve_media(conv, provider, tmp_path, "provider/project/year", "images", report)
-        assert n == 0
+        assert n.downloaded == 0
        assert "local_path" not in block
        assert provider.calls == []

@@ -129,7 +129,7 @@ class TestResolveMedia:
        report = LossReport()

        n = resolve_media(conv, provider, tmp_path, "provider/project/year", "all", report)
-        assert n == 1
+        assert n.downloaded == 1
        assert block["local_path"] == "media/file_audio1.wav"
        rendered = render_blocks_to_markdown([block])
        assert "media/file_audio1.wav" in rendered and rendered.startswith("> 📎")
@@ -139,7 +139,9 @@ class TestResolveMedia:
        block = make_image_placeholder(ref="sediment://file_x", source="user_upload")
        conv = _conv_with([block])
        report = LossReport()
-        assert resolve_media(conv, provider, tmp_path, "provider/project/year", "off", report) == 0
+        assert resolve_media(
+            conv, provider, tmp_path, "provider/project/year", "off", report
+        ).downloaded == 0
        assert provider.calls == []

    def test_idempotent_uses_disk(self, tmp_path):
@@ -164,7 +166,7 @@ class TestResolveMedia:
        report = LossReport()

        n = resolve_media(conv, provider, tmp_path, "provider/project/year", "images", report)
-        assert n == 0
+        assert n.downloaded == 0
        assert "local_path" not in block
        assert report.media_failed["expired-or-missing"] == 1
        # Still renders as a placeholder, not a broken image link
@@ -192,7 +194,9 @@ class TestResolveMedia:
            pass
        block = make_image_placeholder(ref="sediment://file_x", source="user_upload")
        conv = _conv_with([block])
-        assert resolve_media(conv, NoDownload(), tmp_path, "provider/project/year", "images", LossReport()) == 0
+        assert resolve_media(
+            conv, NoDownload(), tmp_path, "provider/project/year", "images", LossReport()
+        ).downloaded == 0


 # ---------------------------------------------------------------------------
@@ -262,3 +266,156 @@ class TestUploadMediaAndRewrite:
        new_body, res_map = upload_media_and_rewrite(body, tmp_path, client, {})
        assert new_body == body
        assert res_map == {}
+
+
+# ---------------------------------------------------------------------------
+# Transient vs permanent failures, and the cross-run retry they feed.
+#
+# Measured 2026-10-20 in one scheduled run: a signed URL returned 500 on one
+# asset and 503 on another. Both are recoverable, yet the conversation was
+# marked exported, so the assets were lost for good. Permanent failures must
+# stay out of the retry set — 19 assets in this archive 403/404 because their
+# bytes were lost in a 2026-07-14 Library migration, and retrying those every
+# morning would be noise that also trips the circuit breaker.
+# ---------------------------------------------------------------------------
+
+
+class TestTransientFailureRetry:
+    def _run(self, failure, tmp_path, circuit=None):
+        from src.media import resolve_media
+
+        ref = "sediment://file_x"
+        provider = _FakeProvider(fail_refs={ref: RuntimeError(failure)})
+        conv = _conv_with([make_image_placeholder(ref, "user_upload")])
+        report = LossReport()
+        outcome = resolve_media(
+            conv, provider, tmp_path, "provider/project/year", "images", report,
+            circuit=circuit,
+        )
+        return outcome, report, conv
+
+    def test_server_error_is_retryable(self, tmp_path):
+        outcome, report, conv = self._run("Signed URL: HTTP 500 after 5 retries", tmp_path)
+        assert outcome.failed == 1
+        assert outcome.retryable_refs == ["sediment://file_x"]
+        assert report.media_failed["download-error"] == 1
+        # Placeholder survives, so the ref is still in the Markdown.
+        assert "local_path" not in conv["messages"][0]["blocks"][0]
+
+    def test_service_unavailable_is_retryable(self, tmp_path):
+        outcome, _, _ = self._run("Signed URL: HTTP 503 after 5 retries", tmp_path)
+        assert outcome.retryable_refs == ["sediment://file_x"]
+
+    def test_timeout_is_retryable(self, tmp_path):
+        outcome, _, _ = self._run(
+            "Network error after 5 retries: Operation timed out", tmp_path
+        )
+        assert outcome.retryable_refs == ["sediment://file_x"]
+
+    def test_deleted_asset_is_not_retryable(self, tmp_path):
+        outcome, report, _ = self._run(
+            "Asset no longer exists — HTTP 404 'File not found'", tmp_path
+        )
+        assert outcome.failed == 1
+        assert outcome.retryable_refs == []
+        assert report.media_failed["expired-or-missing"] == 1
+
+    def test_forbidden_asset_is_not_retryable(self, tmp_path):
+        outcome, report, _ = self._run("HTTP 403 — detail: Forbidden", tmp_path)
+        assert outcome.retryable_refs == []
+        assert report.media_failed["forbidden"] == 1
+
+    def test_success_reports_nothing_retryable(self, tmp_path):
+        from src.media import resolve_media
+
+        ref = "sediment://file_x"
+        provider = _FakeProvider(assets={ref: (b"png", "image/png", None)})
+        conv = _conv_with([make_image_placeholder(ref, "user_upload")])
+        outcome = resolve_media(
+            conv, provider, tmp_path, "provider/project/year", "images", LossReport()
+        )
+        assert (outcome.downloaded, outcome.failed, outcome.retryable_refs) == (1, 0, [])
+
+
+class TestMediaCircuit:
+    def test_trips_after_consecutive_transient_failures(self):
+        from src.media import MediaCircuit
+
+        c = MediaCircuit(threshold=3)
+        for _ in range(2):
+            c.record_failure("download-error")
+        assert not c.tripped
+        c.record_failure("download-error")
+        assert c.tripped
+
+    def test_success_resets_the_streak(self):
+        from src.media import MediaCircuit
+
+        c = MediaCircuit(threshold=3)
+        c.record_failure("download-error")
+        c.record_failure("download-error")
+        c.record_success()
+        c.record_failure("download-error")
+        assert not c.tripped, "scattered failures must not trip the breaker"
+
+    def test_permanent_failures_never_trip_it(self):
+        """A deleted upload says nothing about the provider's health."""
+        from src.media import MediaCircuit
+
+        c = MediaCircuit(threshold=2)
+        for _ in range(10):
+            c.record_failure("expired-or-missing")
+            c.record_failure("forbidden")
+        assert not c.tripped
+
+    def test_open_circuit_skips_downloads_but_keeps_refs(self, tmp_path):
+        from src.media import CIRCUIT_OPEN_BUCKET, MediaCircuit, resolve_media
+
+        ref = "sediment://file_y"
+        provider = _FakeProvider(assets={ref: (b"png", "image/png", None)})
+        conv = _conv_with([make_image_placeholder(ref, "user_upload")])
+        circuit = MediaCircuit(threshold=1)
+        circuit.record_failure("download-error")
+        assert circuit.tripped
+
+        report = LossReport()
+        outcome = resolve_media(
+            conv, provider, tmp_path, "provider/project/year", "images", report,
+            circuit=circuit,
+        )
+        assert provider.calls == [], "must not touch the network while open"
+        assert outcome.downloaded == 0
+        assert outcome.retryable_refs == [ref], "skipped assets must not be abandoned"
+        assert report.media_failed[CIRCUIT_OPEN_BUCKET] == 1
+        assert circuit.skipped == 1
+
+    def test_already_downloaded_asset_served_while_open(self, tmp_path):
+        """An open circuit must not stop on-disk media being linked."""
+        from src.media import MediaCircuit, resolve_media
+
+        ref = "sediment://file_z"
+        conv = _conv_with([make_image_placeholder(ref, "user_upload")])
+        # Derive the directory the way the exporter does rather than
+        # hardcoding it — the layout is "<provider>/<project>.<year>/".
+        from src.utils import build_export_path, generate_filename
+
+        media_dir = build_export_path(
+            tmp_path,
+            "chatgpt",
+            None,
+            "2026-05-20T00:00:00+00:00",
+            generate_filename("Has Media", "conv-1", "2026-05-20T00:00:00+00:00"),
+            "provider/project/year",
+        ).parent / "media"
+        media_dir.mkdir(parents=True)
+        (media_dir / "file_z.png").write_bytes(b"already here")
+
+        circuit = MediaCircuit(threshold=1)
+        circuit.record_failure("download-error")
+        provider = _FakeProvider()
+        resolve_media(
+            conv, provider, tmp_path, "provider/project/year", "images", LossReport(),
+            circuit=circuit,
+        )
+        assert provider.calls == []
+        assert conv["messages"][0]["blocks"][0]["local_path"] == "media/file_z.png"