#268: download refuses instead of returning None — C-79's untreated sibling

[Polichinel]

views-crafdapi found this in our code on the first un_crafd delivery attempt, and it is C-79 with the method name changed.

The defect

self._dsm.download_prediction(file_id).to_dict().get("data", {}).get("file_bytes", None)

When data is present and null, the {} default never applies — the key exists — so the next .get raises AttributeError: 'NoneType' object has no attribute 'get'. It fires from inside a dict comprehension over pinned ids in TargetLease.load, three frames from the port, naming neither the file_id nor the fact that a download had failed. views-crafdapi spent an evening ruling out an OOM kill that turned out to be a different pid three minutes later.

The method was byte-identical in both partners, so the FAO leg carries it too. It has simply not fired there yet.

Why nothing caught it

tests/test_store_port.py was written for C-79, with five parametrised tests across both partners — one of them named test_an_unrecognised_result_is_refused_rather_than_assumed_good. It mentioned download zero times.

And contract/store_metadata.py already writes .get("data", {}) or {} — the guard this lacked, one file away, never applied. C-79's own resolution note had been the specification all along: "an unrecognised result should be refused and named, not adapted to silently." It just never got applied a second time.

The fix

Refuse anything that is not non-empty bytes, naming the file_id, that a download failed, and the types actually received. Empty bytes are refused with the rest — no shard, sidecar or manifest is ever zero-length, so b"" is a failed download wearing a valid type, and returning it only moves the same crash to the parser. This protects all three call sites, not just the one that crashed; two of them are manifest reads.

Mutation-proven on three: restoring the original one-liner fails 18 of the module's tests; accepting empty bytes fails exactly 2 (one per partner); dropping the file_id from the message fails exactly 2.

It also moved, because a guard said so

The refusal pushed managers/ to 469 lines against epic #148's 450 bound. That budget's own instruction is to move something out rather than raise the number, so _ContractStorePort — which is not the manager — is now {partner}/store_port.py. 388 lines, 62 of headroom.

The port stopped naming DatastoreModule in its constructor on the way out. A DIP seam whose stated purpose is that nothing downstream sees the client's types should not name one — and a new module mentioning views_pipeline_core would have widened C-40's blast radius past the two files test_views_pipeline_core_is_confined_to_the_partner_managers pins. The contract is the four methods, and it now says so.

Registered

• C-99 — the defect, resolved here.
• C-100 — what reading the whole port turned up: file_metadata has no caller in the package. latest_file_id runs three times, download three times, upload from the sink; the fourth method is reachable only from a test, with contract/store_metadata.py dead behind it. Not deleted here — that is a decision belonging with the second store ([Scoping] Second prediction store / partner — sample-bearing, multi-target, cm+pgm with reconciliation #97), not with a download bug.

Verification

ruff clean; 446 passed, 1 skipped, 39 xfailed.

Closes #268.