Handle BOM-prefixed EcoHarmonogram responses in v3#6984
Conversation
markvp
left a comment
There was a problem hiding this comment.
Thanks for this, and for the thorough diagnosis and tests. The EcoHarmonogram parsing fix is spot on: I confirmed it's a genuine v3 regression. On master the client used requests with response.encoding = "utf-8-sig", which handled the BOM; the pipeline migration moved it onto the curl_cffi session, whose .json() parses raw bytes and chokes on that same payload. Decoding utf-8-sig explicitly is the right fix, and I like that the error message doesn't leak the response body. This half is a must-have for 3.0.
Could I ask you to split the PR, though? The transient-recovery hardening (the SourceShell.fetch -> bool change, UpdateFailed on first refresh, and the YAML "advance only when all sources succeed" behaviour) isn't actually v3-specific: the same "a failed fetch still marks the day as fetched" behaviour is present on master today, so it's a long-standing robustness improvement rather than a v3 regression. It also changes coordinator and YAML fetch semantics that deserve their own review, and it would widen the blast radius of the release branch.
So my suggestion:
- Keep this PR to the EcoHarmonogram fix, targeting
release/3.0.0. - Move the recovery hardening to a separate PR against 3.1 (it's an additive change, so a minor). Given the v2 counterpart in #6983, that one may even be a candidate for a 2.x patch.
Happy to help line up the 3.1 PR once this one is scoped down. Thanks again.
ab7a5e4 to
3fe4aee
Compare
|
Applied the requested scope split. This branch is now rebased on the current |
markvp
left a comment
There was a problem hiding this comment.
Thanks for splitting this down to just the EcoHarmonogram fix — exactly the 3.0 scope. The utf-8-sig BOM decode plus the single retry reads cleanly, the regression test is now wired into pytest.ini so it actually runs, and CI is green. Good for 3.0.
Summary
The integration-wide transient-fetch recovery previously included here has been removed at maintainer request. That work is preserved separately for a future v3.1 PR; this PR now contains only the provider-specific parser fix suitable for
release/3.0.0.Validation
Related to #6981 and the v3 architecture in #6562.