video: a completed generation is discarded when the mp4 download hits ECONNRESET — _fetch_mp4 has no retry #895

Activity

1. ffroliva commented on Sep 22, 2026

   

   ffroliva

   on Sep 22, 2026

   Owner

   More actions

   Assessment of #895: CONFIRMED-BUG (confidence 9/10)

   Confirmed, and still present on develop at v0.79.0 — migrated_composer.py:2798 is a single page.request.get with no retry and no except, so a Playwright transport error takes down the whole command after the clip is finished and billed. Your reading of api/_retry.py is right too: it is imported by client.py, ui_automation.py and ui_automation_video.py, and by neither migrated download site. grep -rn ECONNRESET src/ returns nothing — no code path in the project recognises a socket-level failure at all. And max_redirects=0 stays exactly as it is; thank you for pre-empting that.

   Two things your report surfaced that go further than it claimed:

   • The same statement exists a second time, at migrated_recover.py:165 — which is the GET behind gflow data download (services/media_recovery.py:101 → recover_clip). So the documented way out of a stranded asset can be stranded by the identical error. The retry needs to be one shared helper called from both sites, not a patch in _fetch_mp4 alone.
   • The exception is Playwright's own Error, not a gflow error class, which is exactly why the message is bare: no gflow error class means no remediation_hint, hence Unexpected error … retryable: False. Your fallback ask stands as a separate gap — nothing in src/ currently points a stranded user at the recovery command.

   A third, smaller one: _fetch_mp4 loops over (record.video_url, record.poster_url), and a raised exception — as opposed to a bad status — aborts the loop, so the poster URL is never tried either.

   On your "Related" note — you were looking at the right thing on the wrong version

   gflow data download <media_id> did ship, in v0.79.0 (2026-09-18). v0.73.2 predates it, which is why gflow data --help did not list it. Your premise correction is accepted and the recovery path is real — please upgrade. It will not fix this bug, but it gives you back the clips you have already lost.

   The media id is recorded before the download is attempted: on_started fires as soon as the submit reply names it (migrated_composer.py:2495-2498), and the recorder upserts the asset row right there (data/recorder.py:788). So every clip stranded this way is already in your catalog:

   gflow data list videos      # the rows with local_path: null
   gflow data download <media_id>
   

   That costs no credits. One caveat, per the second finding above: the recovery GET has the same missing retry, so it may take a second attempt until this is fixed.

   Next step

   The fix is a short bounded retry on transport errors at both call sites, keeping max_redirects=0. It touches a transport, so it goes through our pre-implementation review first. Your point about the signed URL's grace window (#871) is the binding constraint on the design — the backoff will be prompt (a few attempts, sub-second) rather than generous, exactly as you suggested.

   Separately, the failure is deterministically testable without waiting for another organic ECONNRESET — a fake request context that raises once, plus route.abort("connectionreset") for the integration layer — so this will land with a regression test rather than a hopeful patch.

   Thank you for the traceback, the content-length header, and especially for recording the apps_spark_2 false lead. That last note saved the next reader a detour.
2. ffroliva commented on Sep 22, 2026

   

   ffroliva

   on Sep 22, 2026

   Owner

   More actions

   Design review done — one correction I owe you, and one question

   The retry design went through our pre-implementation council (five reviewers, independently). Verdict CAUTION: the bug and the call sites are confirmed, but the mechanism I sketched in my last comment was wrong in two ways. Both corrections are in your favour — the fix gets smaller.

   1. I was wrong about the grace window, and it was my own reasoning, not yours

   I said the backoff would be "prompt rather than generous" because of #871's grace window. That misreads the tree. RESULT_URL_GRACE_S = 20.0 (migrated_composer.py:317) and SIGNED_URL_WAIT_S = 45.0 (migrated_recover.py:54) are waits for the signed URL to appear in a batchexecute frame — not TTLs on the URL itself. #871 was a URL that was never observed, not one that expired. Nothing anywhere in the tree measures when one of these links actually dies.

   The outcome happens to survive — the backoff will be sub-second — but for a different and better-evidenced reason, below.

   2. api/_retry.py already retries Playwright transport errors

   Its predicate is (NetworkError, RateLimitError, *retryable_engine_errors()) (_retry.py:136-141), and _engine.retryable_engine_errors() returns playwright.async_api.Error — which is exactly what your read ECONNRESET surfaces as. Its docstring says so outright: "so a TCP reset / DNS hiccup / connect timeout mid-attempt is retried rather than surfaced raw to callers." The labs arm already wraps its media download in it (client.py:2034-2040); the migrated arm is the outlier.

   So this is convergence on an existing pattern, not new machinery — and the grep ECONNRESET returning nothing, which we both read as "no retry anywhere", actually also means nothing needs to string-match it. Matching on the message would have been a new and worse coupling.

   3. Playwright retries this natively, and that is probably the whole fix

   APIRequestContext.get takes max_retries. Reading the shipped driver (coreBundle.js, _sendRequestWithRetries): it retries only e.code === "ECONNRESET", never on status codes, with a 250 ms → 500 ms backoff — and crucially the backoff is charged to the same call's timeout budget, so three attempts stay inside one 180 s window rather than stretching to nine minutes.

   That matters more than it sounds: our download holds a lock shared with every other generation on the client, so a multiplied timeout would stall unrelated work.

   The question — this is the one thing blocking the design

   Did you actually observe ECONNABORTED, or was it in the suggested-fix list as a likely sibling?

   It matters. Node reports socket hang up with code: 'ECONNRESET', so the native retry already covers two of the three conditions you named. If ECONNABORTED was a reasonable guess rather than something in a bundle, the fix is a one-argument change with no new code at all. If you have it in an incident bundle, we add a thin wrapper around the existing retry helper — still small, but worth knowing which.

   If you still have the bundles: error.class and the first line of error.detail for each of the two failures is enough. No need to attach anything.

   Also being fixed in the same change

   • A third call site you couldn't have seen from your traceback: ui_automation_video.py:1255 has the same unretried download on the labs route.
   • The bare error itself. playwright.async_api.Error isn't one of our error classes, which is why you got Unexpected error … retryable: False with no guidance. It becomes NetworkError (exit 6), which our exit-code table already documents as "Network failure persisted across 3 attempts" — and which is correctly marked retryable.
   • The MCP side is worse than the CLI side and gets fixed by the same typing: today an agent calling gflow_download_media gets "Unexpected Error; details were logged server-side" with no remediation field at all, and the queued gflow_generate_video path returns detail: "sha256:<hash>".
   • An expired signed link currently answers 403 and then advises "retry with a simpler prompt text" — on a path with no prompt. Same wrong-advice class we removed in A retired labs tRPC route (404 "Flow RPCs have been deprecated") tells the user to simplify a prompt that does not exist #875.

   One thing your report surfaced that is bigger than this issue

   Tracing the data layer for this: when the download fails, the catalog keeps assets.status = "pending" forever — the only writer that moves it runs after a successful download, and update_asset_status has no callers at all. So the clip is generated and billed, and the catalog says pending, while the operation row says failed. A reader cannot distinguish "never generated" from "generated, download failed".

   And gflow data download doesn't reconcile it — it adds the file row and leaves both stale.

   That is being filed separately: gflow records "we downloaded it" but never records "Flow generated it", even though Flow tells us so explicitly one line before the download is attempted. Your issue is the symptom that exposed it. Thank you — it's the more valuable half.
3. ffroliva

   added 2 commits that reference this issue on Sep 22, 2026

   fix(worker): keep the media id on a failed queue row (#895)

   ...

   e19e213

   fix(video): retry the signed-media download and type its failure (#895)…

   ...

   142c4d9
4. ffroliva

   closed this as completedin #899on Sep 22, 2026
5. ffroliva commented on Sep 22, 2026

   

   ffroliva

   on Sep 22, 2026

   Owner

   More actions

   Fixed in v0.79.1 — on PyPI now

   pip install --upgrade gflow-cli     # or: uv tool upgrade gflow-cli / gflow update
   

   Thank you for this report. The traceback, the GCS content-length header and the note about the apps_spark_2 false lead all did real work — the last one in particular saved the next reader a detour, and I said so in the code.

   What changed

   The transfer now survives a dropped connection. Not with a hand-rolled loop: Playwright's APIRequestContext already takes max_retries, and it matches on the driver's ECONNRESET and never on an HTTP status. Your instinct that the retry should be prompt was right, and it turns out the driver already agrees — 250 ms → 500 ms — and, importantly, its backoff is charged to the same timeout budget rather than restarting it. That last part is what made it safe here: our download holds a lock shared with every other generation, so a per-attempt timeout would have turned one 180 s wait into nine minutes.

   Both properties were measured against a real TCP RST with an A/B control, not taken from the docs — docs/superpowers/spikes/2026-09-22-playwright-max-retries-econnreset.md. The control arm, with retries disabled, reproduces your exact error string on localhost.

   The failure is now typed. playwright.async_api.Error isn't one of our error classes, which is precisely why you got Unexpected error … retryable: False with no guidance. It's NetworkError now — exit 6, correctly marked retryable — and it names the media id and the free recovery command, so nobody re-generates a clip they already paid for. max_redirects=0 is untouched, as you asked.

   Three call sites, not one. Yours was the one in the traceback. The second is gflow data download's own transfer — the escape hatch could have been stranded by the same fault it exists to rescue. The third is the labs download path.

   The MCP side was worse and is fixed too. An agent calling gflow_download_media used to get "Unexpected Error; details were logged server-side" with no remediation field at all, and the queued gflow_generate_video path returned detail: "sha256:<hash>". The failed queue row also kept losing the media id entirely — so the only copy an agent had was a UUID inside an English sentence recommending a shell command it can't run.

   Your "Related" note was right, and about the wrong version. gflow data download <media_id> shipped in v0.79.0; you were on v0.73.2, which predates it. Upgrading gets you both the fix and the recovery. Your previously-lost clips are still recoverable — the media id is recorded before the download is attempted, so they're in your catalog:

   gflow data list videos      # the rows with local_path: null
   gflow data download <media_id>
   

   That costs no credits.

   Two honest caveats

   1. We could not reproduce your fault. A 20× probe against a real stranded clip was 20/20 clean on this machine — against your 2-in-6. Different path (recovery vs generation) and, more likely, a different network. So the reset looks environment-specific rather than something Flow does to everyone; the VPN / corporate-proxy / AV-inspection class is the usual cause for large transfers on Windows, and the new error text names it.

   This means the fix rests on the controlled measurement above and on your evidence, not on a local repro. If it still bites you on v0.79.1, that is genuinely useful information and I'd rather know.

   2. The question from my last comment is still open, and it now matters more.

   Did you actually observe ECONNABORTED, or was it in the suggested-fix list as a likely sibling?

   Node reports socket hang up with code: 'ECONNRESET', so the native retry covers two of the three conditions you named. ECONNABORTED it does not cover. If you have it in an incident bundle, v0.79.1 will not fully fix your case and a follow-up is owed — a thin wrapper around our existing retry helper, which is a small change. error.class plus the first line of error.detail from either failure is enough; nothing needs attaching.

   Closing this since the reported ECONNRESET path is fixed and released, but please reopen or ping here if it recurs — I'll file the follow-up immediately rather than asking you to re-litigate it.

   Also spun out of your report

   Tracing the data layer for this turned up something bigger: the catalog records "we downloaded it" but never records "Flow generated it", even though Flow says so explicitly one line before the download is attempted. So after a failure like yours the asset row reads pending forever while the operation row reads failed — a reader can't tell "never generated" from "generated, download failed". Tracked as #896 / #897 / #898, and a separate one (#900) where a database timeout can report a billed, successful generation as failed.

   None of that was on the radar before your issue. Thanks again.