fix(load): retry append loads instead of running them at most once

[anoop-narang]

What

append is excluded from retries today:

retryable=(mode != "append"),

The stated reason is that append is the only non-idempotent mode — if the server commits the load but the response is lost, a retry re-appends the same rows.

That is not how the API behaves. It keys a receipt on upload_id, and a re-POST of the same id replays the committed result instead of applying the load a second time. The invariant that makes a retry safe is re-sending the same upload, not the mode.

ManagedDatabaseClient stages once, in upload_parquet, outside the operation it retries — so the invariant already holds for every mode. This drops the exclusion and says so in the comment.

Why it matters

The destination serialises writes per table and refuses rather than queues, so concurrent writers to one table are answered 409 RESOURCE_LOCKED with a Retry-After. Waiting is the whole strategy for that error.

An append had no budget to wait with — one attempt, no backoff, regardless of the max_retries the caller configured. The shape that most needs patience was the one shape that had none.

Also here, because the above needs them

Classify a 409 by its error.code, not by the status alone. CONFLICT becomes terminal — the request cannot succeed as posted, so the current behaviour spends the entire retry budget arriving at the same answer. RESOURCE_LOCKED stays transient. A 409 carrying no error envelope is classified exactly as before.

HotdataError now carries status_code, code and retry_after_seconds. The message is flattened and truncated to 500 chars for readability, so it could not be used as a discriminator without substring-matching prose.

Honour Retry-After, and jitter the backoff. Retry-After is taken as a floor on the ramp, capped like the ramp so a mistaken header cannot park an attempt for an hour. Jitter of up to +50% is added on top and never subtracted, so a stated Retry-After is not undercut.

Jitter matters here specifically: the callers that collide on a table are the ones that started together, so an unjittered ramp has them retry in lockstep and collide again on every attempt. The cap applies to the ramp and deliberately not to the jitter above it — clamping the total would land every late attempt on exactly _MAX_BACKOFF_SECONDS and re-correlate the waits that most need spreading.

This lengthens a 20-attempt budget from 285s to roughly 316-405s.

Not covered

HotdataClient.load_managed_table(file=...) uploads inside the call, so a retry there would mint a new upload id, find no receipt, and duplicate. It has no retry wrapper and is unchanged.

Tests

165 pass, up from 161.

• test_append_load_runs_at_most_once is replaced by test_append_load_retries_like_every_other_mode.
• test_a_retried_load_re_sends_the_same_upload_id pins the invariant the safety actually rests on — a refactor moving the upload inside the retry would otherwise break it silently.
• New coverage for RESOURCE_LOCKED vs CONFLICT classification, a 409 with no error envelope, Retry-After parsing (including a header cased the way the API sends it, and an HTTP-date form that must be ignored rather than raise), and the delay's floor / cap / jitter behaviour.

ruff check is clean on every file touched. One pre-existing E501 in tests/test_request_timeout.py is left alone, along with the repo's existing format drift.

[claude]

nit: (not blocking) CONFLICT → terminal is applied by classify_sdk_error to every endpoint, but the reasoning below it is load-specific ("an upload already consumed…, a receipt naming a different target, an incompatible column type").

The case I'd want checked before merging is ManagedDatabaseClient.ensure_managed_database (managed_client.py:92-104): the operation resolves, and on KeyError creates. If two writers race and the loser's create is answered 409 CONFLICT (duplicate description / already exists), the old behaviour retried, the retry's resolve_managed_database found the database the winner had just made, and ensure_managed_database returned normally. With CONFLICT terminal that race now surfaces as a hard failure.

If the API only ever emits CONFLICT from the load path, this is a non-issue — but the classifier can't tell, so it's worth confirming which endpoints use that code.

[anoop-narang]

Checked this, and I don't think the race is reachable — but it was the right thing to ask, and the objection to how the rule is written stands regardless.

On the specific case: a managed database's description is not unique. Two concurrent create_managed_database calls with the same description both succeed and yield two databases; neither is answered CONFLICT. So in the ensure_managed_database sequence the loser's create doesn't 409, and there is no self-healing retry to lose.

The CONFLICTs reachable near that path are an alias/effective-name collision when attaching a catalog to a database that already has one under that name, and a guard against bulk-deleting a batch whose databases hold data. Neither is reachable from creating a fresh database.

Both of those are also genuinely terminal — retrying either reaches the same answer — so terminal is the right classification for them too, which is the general property the rule needs rather than a load-specific one.

You're right that the reasoning under the branch is written load-specifically while the rule is global. The examples are load examples because that's the path this PR is about; the rule itself only claims "the request cannot succeed as posted", which is what CONFLICT means everywhere it's emitted. If a future endpoint starts using CONFLICT for something retryable, this classifier is where it'd bite, and the comment should be the thing that warns them. Happy to reword it that way if you'd rather it not read as load-only.

[claude]

Reviewed the full diff. No blocking issues; three non-blocking inline comments.

Note on scope of verification: the change rests on a server-side property this repo cannot demonstrate — that a re-POST of the same upload_id replays a receipt rather than re-applying the load. The client-side half of the invariant does hold as described (upload_parquet stages in its own _request_with_retry, outside the one wrapping the load), and test_a_retried_load_re_sends_the_same_upload_id pins it. The server half is taken on the author's word. CI was still in progress when this review started, so I have not seen the test results.

[claude]

Cycle 2: the two nits from the last round are addressed — test_a_lock_refusal_is_retried_and_waits_the_header_out / ..._falls_back_to_the_ramp now pin the Retry-After → time.sleep wiring by recording the argument, and the README and test_retry_policy docstring both scope the non-idempotency claim to the transport. Nothing new blocking; the remaining thread (CONFLICT → terminal vs. a raced ensure_managed_database create) was and stays non-blocking.