Skip to content

_ContractStorePort.download fails open where upload refuses — C-79's untreated sibling #268

Description

@Polichinel

The defect

_ContractStorePort.download chains .get() onto an unvalidated store result:

def download(self, file_id: str) -> bytes:
    return (
        self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None)
    )

When the store returns a result whose data is None, .get("data", {}) yields None
the default never applies, because the key is present — and the next .get raises
AttributeError: 'NoneType' object has no attribute 'get'.

This is C-79's untreated sibling: the same class, the same file, the same fail-open
polarity, in the method next door.

This is not hypothetical — it cost an evening

First un_crafd delivery attempt, 2026-08-13 (views-crafdapi#44, epic #40). The run
resolved its inbound contract and its historical leg cleanly, then died three minutes into
the shard download:

17:43:56  Contract inbound resolved: run rusty_bucket_forecasting_20260727_095355 (3 targets leased)
17:43:57  historical delivery coverage: 64742 distinct cells, 28421738 rows
17:46:59  ERROR - Error during postprocessor run: 'NoneType' object has no attribute 'get'
  File ".../contract/wire/sink.py", line 81, in deliver_run
    frame, headers = lease.load()
  File ".../contract/wire/source_selection.py", line 69, in <dictcomp>
    name: self.store.download(file_id)
  File ".../crafd/managers/crafd.py", line 40, in download
    self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None)
AttributeError: 'NoneType' object has no attribute 'get'

What the operator can learn from that message: nothing. It does not say which file_id
failed, that a download failed at all, or why. We spent real time ruling out an OOM kill
(there was one in dmesg, three minutes later, on a different pid) before establishing that
the actual cause was a download returning a result the port did not check. A named refusal
would have ended that in one line.

The proximate trigger was memory pressure — see the companion issue — but the trigger is not
the point. Any failed download produces the same unreadable crash: a yanked file, an
expired key, a rate limit, a network blip.

upload already refuses; download does not

The other method on the same class carries a long comment explaining exactly why it must not
do this, and the fix that C-79 landed on 2026-08-05:

    # **Refuse unless success is explicitly True** (register C-79). The earlier
    # `if success is False` failed OPEN: a result that was None, or lacked the
    # attribute, or carried a non-bool, sailed through as though the upload had worked.
    #
    # The old `to_dict()` fallback is gone with it: dead on the real path, and an
    # unrecognised result should be refused and named, not adapted to silently.
    success = getattr(result, "success", None)
    if success is not True:
        ...raise, naming what it actually got...

"An unrecognised result should be refused and named, not adapted to silently."
download adapts silently — it returns None on any shape it does not recognise, and lets
the caller trip over it later. C-79's own resolution note is the specification for this fix;
it was simply never applied to the second method.

Both partners carry it

download is byte-identical in unfao/managers/unfao.py and crafd/managers/crafd.py
exactly as C-79 recorded for upload. Verified at origin/development (25ca25c). The FAO
delivery has the same defect on the same leg; it has not fired there yet.

The fix already has a pattern and a home

tests/test_store_port.py exists and was written for C-79: five parametrised tests across
both partners, including test_an_unrecognised_result_is_refused_rather_than_assumed_good
and test_the_refusal_names_what_it_actually_got. None of them covers download.

So this is not new machinery — it is the same polarity change and the same test shapes,
applied to the method that was missed. The fake-store fixture is already there.

C-79's resolution also recorded "the standing excuse never applied here" — the port takes a
store object and calls methods on it, needing neither Appwrite env nor a path manager. That
remains true for download.

Acceptance

  • A download that returns an unrecognised result shape is refused and named
    naming the file_id — not chained into an AttributeError.
  • A download that reports a failure is refused, carrying the store's own error.
  • Both partners covered (unfao and crafd), as test_store_port.py already does for upload.
  • Mutation-proven: reverting the polarity fails the new tests.
  • The register entry cross-references C-79 as the sibling that was fixed first.

Cross-references

Filed from the views-crafdapi seat. No change was made in this repo. Per this repo's own
convention (tests/test_doc_accuracy.py"The FILE is the claim; the line number is not"),
this issue names symbols; the traceback line numbers above are quoted evidence from a run,
not a location claim.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions