store: publish the canned ACL as a single PUT, never multipart (issue #534)

[espg]

Closes #534.

What this changes

x-amz-acl is legal on PutObject and on CreateMultipartUpload, and illegal on UploadPart — S3 answers that one:

400 Bad Request
  <Code>InvalidArgument</Code>
  <Message>The specified header is not valid in this context</Message>
  <ArgumentName>x-amz-acl</ArgumentName>

obstore attaches the issue #495 canned ACL as a client_options.default_headers entry, so it rides every request the write twin makes. Any object over obstore's 5 MiB multipart threshold therefore failed. obstore has no per-request ACL and no way to withhold a default header from one operation, so a multipart write on that handle cannot be made legal — it can only be avoided.

Both write seams now pass use_multipart=False when the target carries the ACL:

• _AclWriteObjectStore.set / set_if_not_exists — the zarr chunk path. Both are spelled out against obstore rather than delegated to a second ObjectStore adapter, because the adapter calls put_async(store, key, buf) bare and cannot express the flag. _acl_writer (an adapter held only for its .store) becomes _acl_store, the raw handle.
• put_object — the raw side-channel path. Mostly small JSON, but the inline index buffer, the temporal tabular object and a column backfill payload are bounded by nothing that keeps them under 5 MiB.

That leaves PutObject as the only request the twin is capable of making. This is deliberately a property of the handle, not a list of permitted operations: enumerating the operations once and missing UploadPart is exactly how #534 reached the fleet. In-account targets are untouched and still multipart.

_SINGLE_PUT_MAX_BYTES (5 GiB, S3's PutObject ceiling) refuses an oversize payload by key, so the day a chunk object outgrows a single PUT it surfaces as a named error rather than as a 400 partway through a fleet run. Published chunk objects run 1–17 MB today, so this is a tripwire, not a limit.

Why the ACL stays

#534 left open whether source.coop needs the canned ACL at all, and named that the thing to measure first. Their Option 3 documentation answers it: "You upload with --acl bucket-owner-full-control so Source Cooperative owns the objects", and under What not to do, "Upload without bucket-owner-full-control when using your own role." Their own example is aws s3 cp --acl bucket-owner-full-control of a large file — which multiparts, with the ACL on CreateMultipartUpload only. Per-operation is the documented shape; this PR implements the same thing by removing the operation that cannot carry it.

So the client_options threading proposed as step 1 of the issue is not in this PR: it existed to make the measurement possible, and the measurement is answered. It remains available as a follow-up escape hatch.

Evidence this is the whole bug

Read anonymously off the bucket (ListObjectsV2?fetch-owner=true needs no credentials there), so these are the published bytes, not a reconstruction:

• _oneshard_test.zarr — the deliberate single-shard test from 2026-08-25. Every object it wrote is present up to 2,098,180 B, and exactly two are missing: 18/rx_flux/c/0 and 18/rx_flux_times/c/0, the only two over 5 MiB (6.7–10.9 MB in the SERC probe). Both their zarr.json siblings landed.
• One ATL03 leaf on source.coop, 54 objects: exactly 8 over 5 MiB and exactly 8 multipart ETags ("…-2"), none below. Largest 16.9 MB.
• Ownership is already correct: objects the fleet wrote direct-IAM carry the same owner canonical ID (277a68ee…) as objects source.coop wrote itself, so the ACL is doing its job on every request S3 accepts it on.

The IAM side needs nothing — template.yaml's published-prefix grant already matches source.coop's documented policy exactly, including s3:PutObjectAcl.

Phases

Single phase. The change is one seam-pair in one module plus the tests that pin it; splitting it would be the artificial file-splitting CLAUDE.md §2 rules out.

• FakeS3 models S3's UploadPart rejection; both write seams forced to single PUT; size ceiling; tests; prose corrected.

How it was tested

tests/test_store_acl_signing.py (obstore physics) and tests/test_store_acl_seam.py (zagg's use of it) drive real handles against an in-memory S3 stand-in and assert captured wire bytes — no mocking of obstore, so an assertion about SignedHeaders is an assertion about what a fleet worker would send.

• FakeS3.do_PUT now returns the real 400 InvalidArgument for x-amz-acl on UploadPart. Without that the fleet's failure mode passed silently in the harness.
• test_a_multipart_write_carrying_the_acl_dies_on_uploadpart — CreateMultipartUpload signs the ACL, UploadPart is refused, nothing lands. Both halves, because the fix depends on both.
• test_an_oversize_chunk_is_written_as_one_signed_put — a 12 MiB zarr chunk produces zero ?uploads, zero partNumber, one signed PUT, and the bytes land. Replaces the test that asserted the opposite.
• test_an_oversize_side_channel_object_is_written_as_one_put, test_a_conditional_write_is_a_single_put_too, test_an_in_account_write_still_multiparts, test_a_payload_past_the_single_put_ceiling_is_refused_by_key.

Verified red without the fix: the two behavioural tests fail with the exact 400.

Full suite: 4915 passed, 14 skipped, plus the four below. ruff check --select=E,F,W,I --ignore=E501 src tests and ruff format --check clean; pre-commit mypy reports zero errors in the touched files.

Two failures are pre-existing on main and unrelated (confirmed by rerunning them on a stashed tree):

• test_closest_obs.py::test_an_unconvertible_max_time_offset_refuses_by_name
• test_lambda_build.py::test_function_build_succeeds — offline sandbox, No matching distribution found for zarr>=3.1.5

Two were caused by this change and fixed here: test_output.py::test_s3_path_puts_single_object and test_runner.py::test_s3_tabular_store_puts_single_object monkeypatch obstore.put with a three-positional-argument double that the new use_multipart kwarg broke. Both doubles now take **kwargs, and the test_output one asserts the flag.

Questions for review

1. put_object overrides use_multipart rather than defaulting it. A caller passing use_multipart=True for an ACL target is asking for a 400, so the refusal is total. The alternative is to raise instead of silently overriding. No caller passes it today.
2. tests/test_lambda_build.py's IAM assertions are unchanged — s3:AbortMultipartUpload and s3:ListMultipartUploadParts are now unused on the published path. Left in place: it is the policy shape source.coop's Option 3 asks for and what we sent them, and template.yaml is off limits without an issue naming it (§1). Only the comment justifying them was updated.
3. Deploy path. zagg ships in the function zip (build_function.sh copies src/zagg), not the deps layer, so getting this to the fleet is a function-zip rebuild — no new layer, and a PyPI release only if you want the version bumped. Not something this PR can or should do.

Not fixed here, still standing on #534

The multipart uploads the failed runs left in-flight on source.coop, plus the litter in §7 of MVP_HANDOFF_2026-08-25.md. Source Cooperative's docs say incomplete uploads age out on a 7-day lifecycle rule.

[espg]

🤖 from Claude (review)

This comment claims a pin the test does not deliver. Verified empirically: with the branch otherwise intact, delete use_multipart=False from set_if_not_exists only —

await obs.put_async(self._acl_store, key, buf, mode="create")

— and pytest tests/test_store_acl_seam.py tests/test_store_acl_signing.py is 21 passed. Nothing goes red.

That is the same result the comment says the test prevents: the assertions here (no POST, no partNumber, one PUT each) are satisfied by obstore's own mode != "overwrite" ⇒ non-multipart rule, so they observe exactly the implementation detail the flag exists to stop depending on. The docstring at src/zagg/store.py:367-370 makes the same claim ("no test of ours would notice changing").

Either the claim should be softened to what the test actually pins (the outcome, however obstore gets there), or the flag needs a pin that survives obstore changing its mind — e.g. asserting the kwarg at the seam, the way tests/test_output.py:240 already does for put_object (assert captured["kwargs"] == {"use_multipart": False}).

For calibration: the reachability is thin — set_if_not_exists carries zarr.json-sized payloads and the sweep lease is tiny JSON — so this is a durability-of-the-guard issue, not a live bug.

[espg]

🤖 from Claude

Fixed in e6994f1 — the finding is right, the wire framing was identical either way, so the test now asserts the argument.

test_a_conditional_write_is_a_single_put_too monkeypatches obstore.put_async with a wrapper that records kwargs and delegates to the real one, then asserts on the set_if_not_exists call:

(cond,) = [(key, kwargs) for key, kwargs in seen if key == "cond"]
assert cond[1].get("mode") == "create"
assert cond[1].get("use_multipart") is False, (
    f"set_if_not_exists left the framing to obstore: {cond[1]}"
)

The existing wire assertions (no POST, no partNumber, two PUTs, ACL signed on each) are kept alongside it — the spy delegates to the real put_async, so those still read genuinely captured bytes.

Verified the mutation the review used: with use_multipart=False deleted from set_if_not_exists alone, pytest tests/test_store_acl_seam.py tests/test_store_acl_signing.py is 1 failed, 20 passed —

E  AssertionError: set_if_not_exists left the framing to obstore: {'mode': 'create'}
E  assert None is False

— then restored, and git diff confirms store.py carries only the docstring change.

The _AclWriteObjectStore docstring at src/zagg/store.py:367-372 is corrected in the same commit. "an obstore implementation detail that no test of ours would notice changing" now reads: "an obstore implementation detail. The wire cannot tell the two apart, so tests/test_store_acl_seam.py spies on obstore.put_async and asserts the argument itself; drop it here and that test goes red."

[espg]

🤖 from Claude (review)

One object-creating path is neither on this seam nor visible to the guard that protects it. src/zagg/catalog/extract.py:229:

boto3.client("s3").upload_file(local_path, bucket, key)

tests/test_store.py::test_no_module_calls_an_obstore_write_api_directly scans for _OBSTORE_WRITE_APIS = {"put", "put_async", "open_writer", "copy", "rename"} — all obstore names. A boto3 managed transfer sails past it, and upload_file multiparts by default above 8 MB, carrying no x-amz-acl at all. So on a source.coop destination it would fail in both the #495 way (owner-less objects) and the #534 way, while the docstring above says the seam is build-enforced.

Not a regression from this PR, and _put_parquet's own docstring asserts the destination is in-account ("the extraction Lambda writes to an in-account bucket"), so today it is out of reach — the prefix is caller-supplied config, not a constant, which is the only reason it is worth naming.

Not asking for a fix in this PR. Either the docstring's "a missed site" claim gets a "(obstore sites)" qualifier, or _put_parquet is worth a line on #534's follow-up list.

[espg]

🤖 from Claude

Left by design; docstrings clarified in 6a8faae. The analysis is accurate — upload_file multiparts above 8 MB, carries no x-amz-acl, and _OBSTORE_WRITE_APIS is obstore names only — but it is pre-existing and out of this PR's scope, and as the review notes _put_parquet's own docstring asserts an in-account destination, so it is unreachable from the published path today. No behaviour changed in extract.py.

Taking the review's first option, both loose claims are now qualified.

src/zagg/store.py (put_object), after "a missed site publishes an object Source Cooperative cannot manage and says nothing about it":

That guard scans obstore call names only; a boto3 transfer is invisible to it, which is safe exactly while every such transfer stays in-account (catalog.extract._put_parquet is the one today).

src/zagg/catalog/extract.py (_put_parquet), appended to the existing in-account sentence:

That in-account destination is also why this boto3 transfer sits outside the canned-ACL seam in :mod:zagg.store (issues #522/#534): nothing here is published to an external bucket, so there is no ownership to hand over and no reason to decline upload_file's multipart.

The review's second option — a line on #534's follow-up list — is a side-effecting action I am not authorised to take on a review comment (CLAUDE.md §6), so it stays standing here for a human call.

[espg]

🤖 from Claude

Reopened and taken further in d771fdc, at espg's direction: rather than leave this as a docstring-held invariant, tests/test_store.py now has a second scanner that fails the build on a boto3 object-creating call anywhere under src/zagg/ or deployment/aws/*.py.

It scans attribute calls for upload_file, upload_fileobj, put_object, copy_object, create_multipart_upload, upload_part — attribute-only, so zagg's own bare-Name put_object does not collide, and copy is deliberately absent because numpy's .copy() is everywhere while copy_object is unambiguous. It makes no attempt to prove the receiver is a boto3 client: tracking a handle through boto3.client(...) / session.client(...) / injection is not tractable, and a false positive is a loud failure rather than a silent publish.

The two writes that exist are allowed by name with their reason, in _KNOWN_BOTO3_WRITES:

• zagg/lifecycle.py / copy_object — already carries the ACL via _copy_acl, on the same _external_target predicate the store seam uses.
• zagg/catalog/extract.py / upload_file — no ACL and none needed while the destination is in-account, with a note that output_prefix reaches run_extraction straight from the invoke event, so this entry is what to revisit if it ever points at a published bucket.

Keyed by (module, API) rather than by line, so unrelated edits do not churn it. The test asserts in both directions: a new write fails, and a stale entry fails too, so the allowlist cannot outlive the call it covers.

Verified it fires — a temporary client.put_object(...) added to telemetry.py produced a boto3 write outside the put_object seam ... ['zagg/telemetry.py:1058 put_object'], then removed. test_the_boto3_guard_catches_a_new_upload pins the same, including that .copy() is not swept up.

put_object's docstring is corrected in the same commit — it claimed a boto3 transfer was invisible to the guard, which is no longer true.

No runtime code changed, so this does not gate the release.

[espg]

🤖 from Claude

Ready for review. All phases complete, both adversarial-review findings folded, CI green on 6a8faae0 (test (3.12), test (3.13), ruff, build, build-arm64, build-x86_64).

• e6994f1 — folds the finding that test_a_conditional_write_is_a_single_put_too pinned less than it claimed. The wire framing is identical either way, so the test now spies on obstore.put_async and asserts the argument. Independently re-verified: deleting use_multipart=False from set_if_not_exists alone gives 1 failed, 20 passed.
• 6a8faae — leaves catalog.extract._put_parquet alone and names, in both docstrings, why a boto3 upload_file sits outside the ACL seam and outside the direct-write guard.

Three things left for a human call, none blocking:

1. The put_object override vs. raise question in the PR body.
2. Whether catalog.extract._put_parquet earns a line on fleet cannot write large objects to source.coop: x-amz-acl is rejected on UploadPart (400 InvalidArgument) — MVP blocker #534's follow-up list. It is safe exactly while that transfer stays in-account; pointed at a published bucket it reintroduces this bug in a shape test_no_module_calls_an_obstore_write_api_directly cannot see.
3. This does not reach the fleet on its own. zagg ships in the function zip, not the deps layer, so the fleet needs a build_function.sh rebuild and a function-code update before a GEDI rerun can be attempted. That is a deploy action and out of scope here (CLAUDE.md §1).