diff --git a/CHANGELOG.md b/CHANGELOG.md index c5f3855bd..5f1c23cae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A clip whose download connection dropped is no longer recorded as never generated + (#896, #898).** When Flow reports a video done but the signed-media connection drops + on every retry, the catalog now marks the asset `MEDIA_GENERATION_STATUS_SUCCESSFUL` + rather than leaving it `pending` forever. The operation's `error_type` is + `media-download` (still exit 6), so `gflow data list errors` tells a lost transfer from + a failed generation. `gflow data list videos` gains a `STATUS` column and a `status` + field in `--json`. An HTTP error answer on the signed URL still leaves the asset + `pending`. **Changed:** that failure's Problem Details `type` is now + `…/errors/media-download` (a subclass of the `network` error, same exit code). - **A generation flow.google.com refuses is now reported as that refusal, by name (#906, #873).** Flow answers a refused submit with HTTP 200 and a `batchexecute` error envelope carrying Google's reason (`PUBLIC_ERROR_UNUSUAL_ACTIVITY`), but gflow's parser discarded any frame diff --git a/docs/DATA_LAYER.md b/docs/DATA_LAYER.md index e4cac2443..fd8a5c0c0 100644 --- a/docs/DATA_LAYER.md +++ b/docs/DATA_LAYER.md @@ -206,6 +206,11 @@ async worker (which mirrors its `generation_queue.error_json` failure into `count>1` request fires the callback per output — no sibling row is stranded); failures before `on_started`, and all image failures, INSERT a fresh row with full request metadata. +- A download whose connection drops on every retry AFTER Flow reported the clip done + (`MediaDownloadError`, exit 6, `error_type=media-download`, #896) also marks that + clip's asset `MEDIA_GENERATION_STATUS_SUCCESSFUL`: the operation failed, the + generation did not, and `gflow data download ` recovers it for free. An + HTTP error answer on the signed URL (e.g. an expired link) still leaves it `pending`. - A poll that COMPLETES with a Flow-reported failure (`succeeded=false`) is recorded as `failed` with `error_type=generation-failed` and the `failure_reasons` as detail — not as a success. @@ -334,6 +339,10 @@ By default, `images` and `videos` aggregate rows by Flow media ID. If an asset has multiple local copies (e.g. re-downloaded to different paths), they appear as a single row with a `COPIES` count and the path of the latest copy. +`videos` also shows the asset `STATUS` (`status` in `--json`): `pending` until Flow +reports the clip done, then `MEDIA_GENERATION_STATUS_SUCCESSFUL` — including when only +the download failed (#896). Images are recorded `ready`. + TTY stdout → Rich table; pipe or `--json` → JSONL. Default sort: newest first. Exit code 16 on data-store errors (same `DataStoreError` family as `gflow data media`). A **missing or freshly-created** DB is NOT a data-store error — `data list` auto-creates the schema via `DataStore.open()` and returns exit 0 with an empty result (see [#88](https://github.com/ffroliva/gflow-cli/issues/88)). > **`data list profiles` vs `gflow auth list`:** `data list profiles` shows profiles that have **recorded generations** in the catalog; `gflow auth list` shows profiles that have ever **logged in** via `gflow auth login`. A profile that logged in but never generated anything will appear in `auth list` but not in `data list profiles`. @@ -537,7 +546,8 @@ Distinct from the above (which are errors OF the data layer), the failures — the last segment of each exception's `problem_type` URI. Common values: `waf-rejection` (reCAPTCHA/WAF refusal — labs HTTP 403, or a flow.google.com `PUBLIC_ERROR_UNUSUAL_ACTIVITY` envelope), `content-policy`, `auth-expired`, `transport-timeout`, `wire-format`, `rate-limit`, `ui-mode-unavailable` -(cohort pin), `media-attribution`. Query them with `gflow data list errors`. +(cohort pin), `media-attribution`, `media-download` (the clip was generated; only +the transfer failed — recover it with `gflow data download`). Query them with `gflow data list errors`. --- diff --git a/src/gflow_cli/api/transports/_common.py b/src/gflow_cli/api/transports/_common.py index 4685c98e8..561eab7d5 100644 --- a/src/gflow_cli/api/transports/_common.py +++ b/src/gflow_cli/api/transports/_common.py @@ -30,6 +30,7 @@ FlowApiError, FlowAppError, FlowHostMigratedError, + MediaDownloadError, NetworkError, RateLimitError, WafRejectionError, @@ -678,7 +679,8 @@ class that is not a :class:`GFlowError`, which renders as *"Unexpected error … error_class=type(exc).__name__, route=route, ) - raise NetworkError( + raise MediaDownloadError( + media_id=media_id, detail=( f"the signed media URL for {media_id} dropped the connection on all " f"{_DOWNLOAD_ATTEMPTS} attempts ({type(exc).__name__})" diff --git a/src/gflow_cli/cli_data.py b/src/gflow_cli/cli_data.py index 833c96542..0c5545313 100644 --- a/src/gflow_cli/cli_data.py +++ b/src/gflow_cli/cli_data.py @@ -168,6 +168,7 @@ def _emit_videos_table(rows: list[VideoRow]) -> None: "MODEL", "DURATION", "CREATED", + "STATUS", "COPIES", "LOCAL_PATH", ): @@ -182,6 +183,7 @@ def _emit_videos_table(rows: list[VideoRow]) -> None: r.model, f"{r.duration:g}s" if r.duration is not None else "", r.created_at.strftime("%Y-%m-%d %H:%M"), + escape(r.status or ""), str(r.copy_count), r.local_path or "", ) diff --git a/src/gflow_cli/data/queries.py b/src/gflow_cli/data/queries.py index b7c42e58c..9a16b6994 100644 --- a/src/gflow_cli/data/queries.py +++ b/src/gflow_cli/data/queries.py @@ -285,6 +285,7 @@ class VideoRow: created_at: datetime local_path: str | None copy_count: int = 1 + status: str | None = None # Aggregated: one row per asset, local_files collapsed into a subquery. @@ -297,6 +298,7 @@ class VideoRow: a.aspect_ratio AS aspect, a.model AS model, a.duration_seconds AS duration, + a.status AS status, a.created_at AS created_at, COALESCE(lfa.copy_count, 0) AS copy_count, lfa.latest_path AS local_path @@ -329,6 +331,7 @@ class VideoRow: a.aspect_ratio AS aspect, a.model AS model, a.duration_seconds AS duration, + a.status AS status, a.created_at AS created_at, 1 AS copy_count, lf.path AS local_path @@ -387,6 +390,7 @@ def list_videos( created_at=datetime.fromisoformat(str(r["created_at"])), local_path=str(r["local_path"]) if r["local_path"] is not None else None, copy_count=int(r["copy_count"]), + status=str(r["status"]) if r["status"] is not None else None, ) for r in rows ] diff --git a/src/gflow_cli/data/recorder.py b/src/gflow_cli/data/recorder.py index 77e2759e6..05811448f 100644 --- a/src/gflow_cli/data/recorder.py +++ b/src/gflow_cli/data/recorder.py @@ -28,7 +28,7 @@ ) from gflow_cli.data.repository import DataRepository from gflow_cli.data.store import DataStore -from gflow_cli.errors import GFlowError, MediaAttributionError +from gflow_cli.errors import GFlowError, MediaAttributionError, MediaDownloadError from gflow_cli.observability import exception_message_hash if TYPE_CHECKING: @@ -1045,7 +1045,6 @@ def record_failed_operation( completed_at = _now_utc_iso() repo = self.repository repo.upsert_profile(profile_name, profile_dir) - recorded = False for media_id in flow_media_ids: op = repo.get_operation_for_output_asset(profile_name, media_id, mode) @@ -1058,6 +1057,14 @@ def record_failed_operation( recorded = True elif op.status == OperationStatus.FAILED: recorded = True # already terminal — do not duplicate + if isinstance(exc, MediaDownloadError) and exc.media_id: + # #896: Flow said DONE and billed; only the transfer failed. Without this the + # asset reads "pending" forever and nothing distinguishes it from a failed + # generation. Same vocabulary record_completed_video writes. Runs after the + # operation write, which matters more if a busy store fails one of the two. + repo.update_asset_status( + profile_name, exc.media_id, "MEDIA_GENERATION_STATUS_SUCCESSFUL" + ) if recorded: return diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index 02b1ce27d..083de4730 100644 --- a/src/gflow_cli/errors.py +++ b/src/gflow_cli/errors.py @@ -41,6 +41,7 @@ "ReferenceNotFoundError", "MentionIndexUnavailableError", "ModelModeIncompatibilityError", + "MediaDownloadError", "NetworkError", "OwnerEvidence", "ProblemDetails", @@ -361,6 +362,22 @@ class NetworkError(FlowApiError): _default_remediation = "Check connectivity and try again." +class MediaDownloadError(NetworkError): + """The signed-media GET failed AFTER Flow reported the generation done (#896). + + Raised only by ``get_signed_media``, so catching it means the clip exists and was + billed — the recorder uses it to mark the asset generated instead of leaving it + ``pending`` forever. Exits 6 like its parent. + """ + + problem_type = "https://gflow-cli.dev/errors/media-download" + title = "Generated media could not be downloaded" + + def __init__(self, *args: Any, media_id: str = "", **kwargs: Any) -> None: + super().__init__(*args, **kwargs) + self.media_id = media_id + + class WireFormatError(FlowApiError): """Carries discovery fields so ``grep error_class=WireFormatError`` in structured logs reveals what was unexpected, enabling new error class diff --git a/tests/api/transports/test_signed_media_retry.py b/tests/api/transports/test_signed_media_retry.py index 4565476e6..d90ff3ab9 100644 --- a/tests/api/transports/test_signed_media_retry.py +++ b/tests/api/transports/test_signed_media_retry.py @@ -137,6 +137,16 @@ async def test_it_raises_network_error(self) -> None: with pytest.raises(NetworkError): await MigratedComposer._fetch_mp4(page, _record()) # pyright: ignore[reportPrivateUsage, reportArgumentType] + async def test_it_names_the_generated_clip_for_the_recorder(self) -> None: + """#896: the recorder marks THIS media id generated only because the error says + which clip it was. Reverting to a bare NetworkError silently restores 'pending'.""" + from gflow_cli.errors import MediaDownloadError + + page = FakePage(PlaywrightError()) + with pytest.raises(MediaDownloadError) as caught: + await MigratedComposer._fetch_mp4(page, _record()) # pyright: ignore[reportPrivateUsage, reportArgumentType] + assert caught.value.media_id == MEDIA_ID + async def test_it_exits_6_and_is_marked_retryable(self) -> None: page = FakePage(PlaywrightError()) with pytest.raises(NetworkError) as caught: diff --git a/tests/data/test_failure_recording.py b/tests/data/test_failure_recording.py index 37e5f474c..3ca909f04 100644 --- a/tests/data/test_failure_recording.py +++ b/tests/data/test_failure_recording.py @@ -17,7 +17,12 @@ from gflow_cli.data.redaction import ERROR_DETAIL_MAX_CHARS, redact_error_detail from gflow_cli.data.repository import DataRepository from gflow_cli.data.store import DataStore -from gflow_cli.errors import ContentPolicyError, DataStoreError, WafRejectionError +from gflow_cli.errors import ( + ContentPolicyError, + DataStoreError, + MediaDownloadError, + WafRejectionError, +) def _fetch_operations(store: DataStore) -> list[dict[str, object]]: @@ -418,3 +423,52 @@ def test_record_completed_video_records_failed_when_not_succeeded(tmp_path: Path assert op["status"] == OperationStatus.FAILED.value assert op["error_type"] == "generation-failed" assert op["error_detail"] == "PUBLIC_ERROR_UNSAFE_GENERATION" + + +# --------------------------------------------------------------------------- +# #896 — a download that fails AFTER Flow reported the clip done +# --------------------------------------------------------------------------- + + +def _asset_status(store: DataStore, media_id: str) -> str: + (status,) = store.conn.execute( + "SELECT status FROM assets WHERE flow_media_id = ?", (media_id,) + ).fetchone() + return status + + +def test_failed_download_records_the_clip_as_generated(tmp_path: Path) -> None: + """Flow said DONE and billed; only the transfer died. The asset must say so, + and the operation must name the download — not the generation — as what failed.""" + with DataStore.open(tmp_path / "gflow.db") as store: + recorder = OperationRecorder(DataRepository(store), prompt_mode="store") + req = _started_video(recorder, tmp_path) + recorder.record_failed_operation( + profile_name="default", + profile_dir=tmp_path / "p", + command="video t2v", + mode=OperationKind.T2V, + exc=MediaDownloadError(detail="reset", media_id="media-1"), + request=req, + flow_media_ids=["media-1"], + ) + assert _asset_status(store, "media-1") == "MEDIA_GENERATION_STATUS_SUCCESSFUL" + (op,) = _fetch_operations(store) + assert op["status"] == OperationStatus.FAILED.value + assert op["error_type"] == "media-download" + + +def test_failed_generation_leaves_the_clip_pending(tmp_path: Path) -> None: + with DataStore.open(tmp_path / "gflow.db") as store: + recorder = OperationRecorder(DataRepository(store), prompt_mode="store") + req = _started_video(recorder, tmp_path) + recorder.record_failed_operation( + profile_name="default", + profile_dir=tmp_path / "p", + command="video t2v", + mode=OperationKind.T2V, + exc=WafRejectionError("poll blocked", status=403), + request=req, + flow_media_ids=["media-1"], + ) + assert _asset_status(store, "media-1") == "pending" diff --git a/tests/test_cli_data.py b/tests/test_cli_data.py index 819b7db52..556d2b819 100644 --- a/tests/test_cli_data.py +++ b/tests/test_cli_data.py @@ -133,6 +133,7 @@ def test_data_list_videos_json(seeded_db: Path) -> None: assert result.exit_code == 0 rows = [json.loads(ln) for ln in result.output.splitlines() if ln.strip()] assert len(rows) == 2 + assert all(row["status"] == "ready" for row in rows) # #896/#898: status is visible def test_data_list_videos_filter_no_match(seeded_db: Path) -> None: diff --git a/tests/test_data_queries.py b/tests/test_data_queries.py index 52ee5979e..84b2afd7b 100644 --- a/tests/test_data_queries.py +++ b/tests/test_data_queries.py @@ -376,6 +376,14 @@ def test_list_videos_carries_duration(seeded: Path) -> None: assert rows[0].duration > 0 # float, e.g. 6.0 +@pytest.mark.parametrize("all_copies", [False, True]) +def test_list_videos_carries_asset_status(seeded: Path, all_copies: bool) -> None: + """#896/#898: without the column, 'generated but not downloaded' and 'never + generated' read the same.""" + rows = list_videos(db_path=seeded, profile="alice", limit=1, offset=0, all_copies=all_copies) + assert rows[0].status == "ready" + + # ─── list_profiles ──────────────────────────────────────────────────────────── diff --git a/website/docs/DATA_LAYER.md b/website/docs/DATA_LAYER.md index c9dda8f21..842917650 100644 --- a/website/docs/DATA_LAYER.md +++ b/website/docs/DATA_LAYER.md @@ -206,6 +206,11 @@ async worker (which mirrors its `generation_queue.error_json` failure into `count>1` request fires the callback per output — no sibling row is stranded); failures before `on_started`, and all image failures, INSERT a fresh row with full request metadata. +- A download whose connection drops on every retry AFTER Flow reported the clip done + (`MediaDownloadError`, exit 6, `error_type=media-download`, #896) also marks that + clip's asset `MEDIA_GENERATION_STATUS_SUCCESSFUL`: the operation failed, the + generation did not, and `gflow data download ` recovers it for free. An + HTTP error answer on the signed URL (e.g. an expired link) still leaves it `pending`. - A poll that COMPLETES with a Flow-reported failure (`succeeded=false`) is recorded as `failed` with `error_type=generation-failed` and the `failure_reasons` as detail — not as a success. @@ -334,6 +339,10 @@ By default, `images` and `videos` aggregate rows by Flow media ID. If an asset has multiple local copies (e.g. re-downloaded to different paths), they appear as a single row with a `COPIES` count and the path of the latest copy. +`videos` also shows the asset `STATUS` (`status` in `--json`): `pending` until Flow +reports the clip done, then `MEDIA_GENERATION_STATUS_SUCCESSFUL` — including when only +the download failed (#896). Images are recorded `ready`. + TTY stdout → Rich table; pipe or `--json` → JSONL. Default sort: newest first. Exit code 16 on data-store errors (same `DataStoreError` family as `gflow data media`). A **missing or freshly-created** DB is NOT a data-store error — `data list` auto-creates the schema via `DataStore.open()` and returns exit 0 with an empty result (see [#88](https://github.com/ffroliva/gflow-cli/issues/88)). > **`data list profiles` vs `gflow auth list`:** `data list profiles` shows profiles that have **recorded generations** in the catalog; `gflow auth list` shows profiles that have ever **logged in** via `gflow auth login`. A profile that logged in but never generated anything will appear in `auth list` but not in `data list profiles`. @@ -537,7 +546,8 @@ Distinct from the above (which are errors OF the data layer), the failures — the last segment of each exception's `problem_type` URI. Common values: `waf-rejection` (reCAPTCHA/WAF refusal — labs HTTP 403, or a flow.google.com `PUBLIC_ERROR_UNUSUAL_ACTIVITY` envelope), `content-policy`, `auth-expired`, `transport-timeout`, `wire-format`, `rate-limit`, `ui-mode-unavailable` -(cohort pin), `media-attribution`. Query them with `gflow data list errors`. +(cohort pin), `media-attribution`, `media-download` (the clip was generated; only +the transfer failed — recover it with `gflow data download`). Query them with `gflow data list errors`. ---