fix(client): raise on an already-failed Storage job instead of returning it

[martinsifra]

Issue: DMD-1898

What

_wait_for_storage_job (client/_core.py) had two terminal-state checks — an early
return before the poll loop and a second check inside it — and they had drifted:

	status == "success"	status == "error"
initial body (early return)	returns	returns ← bug
polled body (in loop)	returns	raises STORAGE_JOB_FAILED

So a Storage API fast fail (terminal straight away, never waiting) reached the caller
as a normal return value. All 19 call sites of the helper either return the job or
job.get("results", {}) — and an error job has no results — so the failure surfaced as a
silent empty success. Call sites: storage_tables.py 16x (import/export/delete/snapshot/…),
branches.py 2x (create/delete dev branch), workspaces.py 1x. Enqueued with POST 11x,
DELETE 7x and PUT 1x (change_sharing_type).

Blast radius. Every one of those operations could report exit 0 with nothing done. The
size of the real-world exposure depends on when the API fails fast, which is not something
this PR establishes — e.g. storage create-table --if-not-exists keys its idempotency off
catching STORAGE_JOB_FAILED, and that path is live-validated in the 0.84.2 changelog
("third create → original STORAGE_JOB_FAILED envelope"), so the duplicate-name error
evidently arrives polled and that flag works today. The bug class is real and the fix is
unconditional; treat any specific "this command was broken" claim as unproven unless the
error is known to arrive terminal on the first response.

How

Restructured to check-then-fetch: one terminal-state check at the top of the loop, so
the caller's initial body and every polled body traverse identical code and the class of bug
(two checks that can drift) is no longer expressible.

This is the shape the two sibling pollers in the same package already use —
wait_for_queue_job (client/queue.py) and wait_for_query_job (client/query.py). The
storage poller was the odd one out precisely because it receives the first job dict from the
caller instead of fetching it, and grew a second check for that case.

Preserved verbatim, so no existing behaviour moves: sleep-before-poll ordering, the deadline
check before the sleep (an exhausted budget never costs a poll interval), both error
messages, both status codes, both retryable flags. Poll counts are identical to the old
loop for every budget, including max_wait=0 and max_wait < one poll interval.

Also fixed, and unlike the fast-fail this one needs no hypothesis: the failure-message
extraction assumed error is a dict. {"error": null} or {"error": "some text"} turned a
cleanly-failed Storage job into an AttributeError traceback instead of STORAGE_JOB_FAILED
— on the polled path that has always worked, no fast fail required (992f995,
_storage_job_error_message; a string error's text is now used as the message, unusable
shapes fall back to the generic text). Smaller blast radius than the headline bug, but
directly evidenced — the repo has already paid for this shape once, when the Metastore
answered {"error": 422}.

Commits

1. test(client) — new TestWaitForStorageJob. The poller is reached by 19 call sites
   but had no tests of its own: its success fast path was covered only incidentally (in
   test_storage_truncate), and the terminal-error fast path and the timeout path were not
   covered at all. Four contracts pinned; test_already_failed_body_raises lands as
   xfail(strict=True) — red against the unfixed code, which is what proves it catches the
   bug.
2. fix(client) — the restructure, plus removal of that marker. strict=True makes the
   removal mandatory: a strict xfail that starts passing fails the suite, so the marker
   cannot be forgotten. (A non-strict xfail would XPASS silently and then never fail CI
   again in either direction — effectively a disabled test.)

xfail_strict is deliberately not set in pyproject.toml; strict lives on the marker,
since making it a repo-wide default is a convention change that should not ride along in a
bugfix branch.

Testing

• make check green: 5755 passed, 12 skipped as of 5e1d6bd (5750 + 1 xfailed after commit 1).
• Test fallout of the fix: zero. All 19 occurrences of "status": "error" in tests/
  were checked — for the storage poller the error always arrives in the polled response
  (test_client.py, import job), one is the Queue poller (a different code path), one is
  job-listing fixture data. Nothing encoded the old broken behaviour.
• No E2E: reproducing a fast fail against the live API is not reliably arrangeable, and the
  --if-not-exists path already goes through KeboolaApiError, which the fix guarantees.

Merge order

Please merge before #556. That PR carries a local guard in merge_requests.merge() for
this exact blind spot; once this lands, the guard is dead code and #556 drops it on rebase
(checklist posted there).

🤖 Generated with Claude Code

---

Review follow-ups

• e719d5a — the job: arg docstring said "from POST/DELETE"; change_sharing_type
  (storage_tables.py:310) enqueues with PUT. Pre-existing inaccuracy, carried over in
  8ecf99d and caught by review. Now names all three verbs.
• 09876bf — second review round: opens 0.84.3 (pyproject + make version-sync +
  changelog bullet + gotchas.md tagged (since v0.84.3)). v0.84.2 is tagged and
  published at main's HEAD, so there was no in-progress key to append to, and neither
  changelog-check nor version-check would have caught the omission. Also pins the
  "poll counts unchanged" claim with a sleep recorder (verified it now fails where it
  previously only ran a second slower), adds the sub-interval-budget and polled-success
  cases, narrows the sibling-poller wording from a parity claim to a shape claim, and
  dedups _mk_client.
• 992f995 — Devin review: tolerate a non-dict error on a failed Storage job (see the
  bolded paragraph in How — the best-evidenced defect in this PR).
• 5e1d6bd — pushed by @padak (disclosed in his second review): _OMIT sentinel so the
  explicit {"error": null} body is actually constructed by the fallback test, plus the
  missing blank line before the new gotchas.md heading. Reviewed and verified on pull:
  both claims check out (None was silently collapsing "absent" and "null" into one case).
• af9a790 — corrected the call-site count in the test class docstring: 19 on main
  (16/2/1), not 20. The 20 holds only on the [DMD-1833] Merge requests — Part 1, Layer 3 (client) #556 branch, which adds merge(). The
  earlier "15x" for storage_tables.py came from a grep that required the HTTP verb on
  the same line as _request(, so it missed change_sharing_type's multi-line call.

The commit message of 8ecf99d still says "all 20 call sites"; leaving it, since
rewriting pushed history would orphan the review thread on it.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[martinsifra]

Thanks — I verified all six independently rather than taking them on trust, and all six hold. Addressed in 09876bf.

Including the one I had argued against. On point 2 I claimed a regression would go red via assert httpx_mock.get_requests() == []. I measured it: with the deadline check moved after the sleep, break still precedes the fetch, so no request is made — the test passes, 1.08 s instead of 0.21 s. Exactly as you said, and my reasoning was wrong.

#	Verdict	Evidence I gathered
1	Correct, and it is the convention	The bump is carried by the substantive PR in all 12 preceding releases (0.84.2 ← #594/#597, 0.84.1 ← #589, 0.84.0 ← auth login-password). v0.84.2 is a published non-prerelease release tagged at b47a3c4 = main's HEAD, so no in-progress key existed; #596/#598 merely appended to 0.84.2 while it was unreleased. Confirmed changelog-check only audits that released versions have entries, and version-check only compares plugin.json/marketplace.json/uv.lock to pyproject — neither would catch it.
2	Correct, including the part I disputed	Measured above.
3	Correct	The class pinned initial-success, initial-error, timeout, polled-error — polled-success was missing.
4	Correct	Exactly two _mk_client definitions in the whole suite, 62 lines apart, byte-identical. Not an arbitrary pick from many.
5	Correct	_core.py handles only success/error; wait_for_queue_job keys off isFinished and caps sleep at min(interval, remaining). Shape matches, behaviour does not.
6	Correct	The 0.84.2 changelog records --if-not-exists as live-validated ("third create → original STORAGE_JOB_FAILED envelope"), so that error arrives polled and the flag works today. My motivation was overstated.

What changed:

• 0.84.3 opened — pyproject.toml + make version-sync (plugin.json, marketplace.json, uv.lock), a Fix: bullet in changelog.py, and a gotchas.md entry tagged (since v0.84.3) covering the behaviour change, the "scripts that treated an empty result as success" migration note, and the success/error-only narrowness as a known limitation.
• Poll counts pinned — test_timeout_raises_storage_job_timeout records sleeps and asserts sleeps == []; verified it now fails with assert [1.0] == [] under the regression. Added test_budget_below_one_interval_still_polls_once (0.5 s budget → exactly one poll, one-interval overshoot preserved) for the other half of the claim.
• test_polled_success_returns_the_polled_body — processing → success → results, replacing the incidental coverage.
• Docstring narrowed — no longer reads as a parity claim; it now names both divergences and says they predate the restructure. Whether Storage jobs can end in a state other than success/error is a genuine open question, but it is pre-existing behaviour, so I left it as documented narrowness rather than widening the fix.
• _mk_client — one module-level helper, both classes use it.
• PR body softened on the --if-not-exists motivation.

make check: 5753 passed, 12 skipped, version in sync, all 45 stable releases have changelog entries.

Two notes on scope. 8ecf99d's commit message still carries the overstated --if-not-exists wording; I am not rewriting pushed history that has a review thread on it, and it is squashed away on merge — the squash body is drafted with the corrected framing. And since this PR now opens 0.84.3, #556 will append to that key rather than open its own (0.85.0 is reserved for the mcp_tool removal).

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/test_client.py:3365

• This test currently relies on the implementation not making any HTTP requests; if it regresses and starts polling, the unmatched httpx_mock request will raise httpx.TimeoutException and BaseHttpClient._do_request() will retry with real time.sleep backoff, making the failure slow and potentially confusing. Consider patching time.sleep to _noop_sleep in this test too so a regression fails fast.

    def test_already_successful_body_returns_without_polling(self, httpx_mock) -> None:
        """A terminal-success initial body is returned as-is, with no HTTP call."""
        with _mk_client() as client:
            job = client._wait_for_storage_job({"id": 1, "status": "success", "results": {"x": 1}})

[devin-ai-integration]

Devin Review found 1 potential issue.

[devin-ai-integration]

🔍 Error extraction still assumes error is a dict

job.get("error", {}).get("message", ...) raises AttributeError if the Storage API returns error as a plain string (the Query-Service poller in this package explicitly handles both shapes — see _extract_query_error tests in tests/test_client.py). Pre-existing, but the fix now makes this path reachable from the initial response body too, so a fast-fail whose error is a string would surface as an unhandled AttributeError rather than STORAGE_JOB_FAILED. Worth a one-line tolerance check while the code is being touched.

(Refers to lines 237-243)

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[martinsifra]

Confirmed and fixed in 992f995 — thanks, this one was real, not theoretical.

Reproduced the exact failure first:

>>> {'error': 'plain text boom'}.get('error', {}).get('message', 'Storage job failed')
AttributeError: 'str' object has no attribute 'get'

And your framing of the reachability is right: pre-existing, but the restructure means the terminal check now also sees the caller's initial response body, so the shape can arrive from one more direction than before.

Worth adding to the case you made — the field is demonstrably not reliably a dict in this repo's own history, not just in principle. Three precedents:

• queue.py:250 isinstance-guards its own result before .get("message").
• _extract_query_job_error (_transfer.py:152) is a documented helper for "strings, dicts and unknown shapes".
• The Metastore once answered {"error": 422} — an int — and the CLI rendered API error 422: 422. That cost a released bugfix, which is why BaseHttpClient._raise_api_error now accepts error only when it is a non-empty string.

_core.py was the last place reading it bare.

One deliberate deviation from the nearest precedent: the queue poller falls back to generic text when result is not a dict, which discards a string error's content. I followed _extract_query_job_error instead and use a string error as the message — it is the more useful of the two, and losing the operator's only diagnostic text to a type check would be its own small bug. Extracted as _storage_job_error_message with the reasoning in its docstring so nobody "simplifies" it back.

A dict with no usable message, an int, a list, or a missing field all fall back to "Storage job failed" rather than rendering None or a raw repr. Two tests cover it (the string shape, plus five unusable variants), and the 0.84.3 changelog bullet now mentions the hardening.

make check: 5755 passed, 12 skipped.

[padak]

Review — verified locally, no blocking findings

Checked out 992f995 in a clean worktree and reproduced every claim in the description rather than reading them. Summary: the bug is real, the fix is behaviour-preserving apart from the one intended change, and the new tests genuinely catch it.

What I verified (not just read)

1. The new tests are red against the unfixed poller. Restored _core.py from origin/main on top of the PR's test file:

FAILED tests/test_client.py::TestWaitForStorageJob::test_already_failed_body_raises
FAILED tests/test_client.py::TestWaitForStorageJob::test_string_error_field_still_raises_with_its_text
FAILED tests/test_client.py::TestWaitForStorageJob::test_unusable_error_field_falls_back_to_generic_message
3 failed, 5 passed

So the fast-fail contract is pinned by a test that actually fails without the fix — the xfail(strict=True) → removal dance in commits 1/2 did its job.

2. "Poll counts are identical" holds exhaustively, not anecdotally. Extracted both loops (old + new) against a deterministic fake clock and swept max_wait ∈ {0, 0.001, 0.5, 1.0, 1.5, 2.0, 3.0, 60, 600} × six status sequences (terminal-success-first, terminal-error-first, waiting-forever, waiting→success, waiting→error, unknown-terminal-status):

scenario	old (outcome, polls, sleeps)	new
terminal success first	return, 0, 0	return, 0, 0
terminal error first	return, 0, 0	raise_failed, 0, 0 ← the intended change
waiting forever, max_wait=0	timeout, 0, 0	timeout, 0, 0
waiting forever, max_wait=0.5	timeout, 1, 1	timeout, 1, 1
waiting forever, max_wait=60	timeout, 60, 60	timeout, 60, 60
waiting→error, max_wait=0.5	raise_failed, 1, 1	raise_failed, 1, 1
unknown terminal status, max_wait=60	timeout, 60, 60	timeout, 60, 60

9 differences across the whole sweep, all of them the status: error initial body. Nothing else moves — including the sub-interval overshoot and the max_wait=0 no-sleep case, which the two new timing tests pin.

3. make check green on this branch, macOS / py3.12: 5755 passed, 12 skipped, 154 deselected (the description's 5751 predates 992f995). CI is green on all four jobs too.

4. The bug class is confined to this poller. Read the two siblings: wait_for_queue_job (client/queue.py) and wait_for_query_job (client/query.py) both fetch before checking and neither accepts a caller-supplied initial body, so neither can grow the second check that drifted here. Nothing else in client/ needs the same treatment.

5. All 19 call sites re-counted on main — storage_tables.py 16×, branches.py 2×, workspaces.py 1×. Every one of them is return job.get("results", {}), return job, or a bare wait, so the "empty success" mechanism in the description is exactly right: an error job has no results, so {} came back as a success payload.

The strongest part of this PR is underadvertised

992f995 (_storage_job_error_message) is missing from the "Review follow-ups" list in the description and only shows up in changelog.py. It deserves top billing, because unlike the fast-fail it fixes something reachable today, on the already-working polled path:

# before
error_msg = job.get("error", {}).get("message", "Storage job failed")

{"error": null} → None.get(...) → AttributeError; {"error": "some text"} → str.get → AttributeError. Either shape turns a cleanly-failed Storage job into a traceback, no fast fail required. That is a smaller blast radius than the headline bug but a much better-evidenced one — the description currently hedges the whole PR on the fast-fail hypothesis ("treat any specific 'this command was broken' claim as unproven", which is the right call) while sitting on a defect that needs no hypothesis at all. Worth a line in What.

Findings

🟢 nit — tests/test_client.py, test_unusable_error_field_falls_back_to_generic_message. None doubles as the "omit the key" sentinel:

for error_field in ({}, {"message": ""}, 422, None, ["boom"]):
    job: dict[str, Any] = {"id": 1, "status": "error"}
    if error_field is not None:
        job["error"] = error_field

so an explicit {"error": None} — one of exactly two shapes that made the old code raise AttributeError, and the one this loop looks like it is covering — is never constructed. The other ("error": "text") has its own test; this one has none. A distinct sentinel (_OMIT = object()) buys the missing case for one line.

🟢 nit — gotchas.md:3636. The new ## A Storage job that failed instantly… heading has no blank line before it; it sits flush against the previous section's last list item. 125 of the 127 ## headings in the file have one (the other exception is pre-existing at :1655). CommonMark still renders it as a heading, so this is consistency only.

🟢 nit — description drift. make check line says 5751; the follow-ups list stops at af9a790. Both fixed by one edit if you touch the description for the point above.

Optional, explicitly out of scope for a behaviour-preserving bugfix — follow-up candidates

• The failure message carries no job id. wait_for_queue_job raises f"Queue job {job_id} failed: {msg}"; this one raises the bare API message, so a failed Storage job gives the user nothing to look up. The timeout branch right below it does include job_id. Prefixing would not disturb storage_service.py's "already has the same display name" in exc.message substring match, but it is a message change and this PR deliberately preserved messages verbatim — better as its own change.
• Unknown terminal statuses still burn the full budget. Documented honestly in both the docstring and the gotcha: anything that is not success/error polls until STORAGE_JOB_TIMEOUT with retryable: true, i.e. a permanent failure advertised as retryable. Pre-existing, correctly left alone here.

Verdict

Approve-quality from my side. The restructure makes the drift unexpressible rather than merely fixed, the docstring explains why for the next reader, the tests are the kind that fail for the right reason, and the version/plugin/changelog surfaces (pyproject → plugin.json → marketplace.json → uv.lock → changelog.py → gotchas.md tagged (since v0.84.3)) are all in step — 09876bf catching the missing 0.84.3 key is the sort of thing neither changelog-check nor version-check would have flagged, since v0.84.2 was already tagged at main's HEAD.

Merge-order note from the description confirmed as sensible: land this before #556 so its local guard in merge_requests.merge() drops out on rebase.

Automated review — three cosmetic nits, nothing blocking. Not an approval; a human still holds the merge gate.

[padak]

To set expectations on the review above: all three findings are cosmetic (🟢), nothing blocks. The substance — the fix itself, the equivalence claim and the tests — I verified by reproduction, not by reading, and it all holds.

I'll keep watching this branch. Push whenever you're ready: I'll re-review the delta, and once there's nothing left worth raising, I'll approve rather than leave another round of comments.

No need to address the nits if you'd rather not — say so and I'll approve as it stands.

[padak]

No outstanding review findings — and a disclosure: I pushed the nits myself

Rather than leave you a second round of comments over three cosmetic things, I fixed two of them on your branch: 5e1d6bd. Revert it without discussion if you'd rather own them.

What's in it (no behaviour, no production code):

• test_unusable_error_field_falls_back_to_generic_message used None as its "leave the key out" sentinel, so the loop that looks like it covers {"error": None} never actually built that body. It matters more than the others: {"error": None} is one of exactly two shapes that made the pre-992f995 extraction raise AttributeError — verified in isolation, job.get("error", {}).get("message", …) gives 'NoneType' object has no attribute 'get' — and the other one ({"error": "text"}) already has its own test. Absence now has an _OMIT sentinel, so null is a real case.
• The new gotcha section got the blank line before its ## heading that 125 of the file's other 127 headings have. (The remaining exception at gotchas.md:1655 is pre-existing; left alone.)

make check green on the pushed head: 5755 passed, 12 skipped.

Left for you — I tried to correct it and my tooling refused to rewrite another author's PR description, which is fair enough: the Testing section still says 5751 passed (5755 as of 5e1d6bd), and the follow-ups list still stops at af9a790, so 992f995 appears nowhere in the description. That last one is worth a line in What, not just the changelog — it is the one defect in this PR that needs no hypothesis at all. The fast-fail bug is correctly hedged as unproven, while {"error": null} / {"error": "text"} turn a cleanly-failed Storage job into a traceback on the polled path that has always worked. Smaller blast radius, much better evidence.

What I verified by reproduction, not by reading

1. The tests are red against the unfixed poller. _core.py restored from origin/main under your test file: 3 failed, 5 passed — test_already_failed_body_raises among them. The xfail(strict=True) → removal sequencing did its job.
2. "Poll counts are identical" holds exhaustively. Both loops against a deterministic fake clock, max_wait ∈ {0, 0.001, 0.5, 1, 1.5, 2, 3, 60, 600} × 6 status sequences: 9 differences in the whole sweep, every one of them the status: error initial body. Sub-interval overshoot and the max_wait=0 no-sleep case included.
3. make check green locally (macOS/py3.12) at both 992f995 and 5e1d6bd; CI green on all four jobs.
4. The bug class is confined to this poller. wait_for_queue_job and wait_for_query_job both fetch before checking and neither takes a caller-supplied initial body, so neither can grow the second check that drifted here.
5. 19 call sites re-counted on main (16/2/1), all return job / return job.get("results", {}) / bare wait — the "empty success" mechanism is exactly as described.

Nothing left on my side worth raising against 1–5, which cover your work. 5e1d6bd is mine and I am explicitly not reviewing my own commit — give it a glance before merging. The formal approval comes from a human hand, not from this pass.

Merge order per your note: land this before #556 so its local guard in merge_requests.merge() drops out on rebase.

[martinsifra]

@padak Ready for the approval pass — everything from both rounds is resolved:

• 5e1d6bd reviewed on pull, keeping it. Both claims verified: my None was indeed collapsing "absent" and "null" into one case (so {"error": null} — an AttributeError shape on the old extraction — was never actually built), and 126 of 127 gotchas.md headings have the blank line. One footnote for the record: the old expression raised AttributeError on four shapes, not two — 422, None, ["boom"] and "text", i.e. anything but a dict or an absent key. Your fix covers three of the four in the fallback loop and the string shape has its own test, so the coverage conclusion only gets stronger.
• Description updated (the part your tooling rightly refused to touch): Testing now says 5755 as of 5e1d6bd; 992f995 has top billing in How as the no-hypothesis defect, exactly per your framing; the follow-ups list now includes 992f995 and your 5e1d6bd with disclosure.
• The two out-of-scope candidates (job id in the failure message, unknown-terminal-status burning the budget as retryable: true) — agreed as follow-ups, not this PR.

Nothing else queued from my side. After the approval this squash-merges (subject from the PR title; body drafted), then #556 rebases and drops its local merge() guard.

[padak]

Your correction is right and mine was wrong — checked it rather than take it on trust:

error field	old job.get("error", {}).get("message", …)
absent	'Storage job failed'
{} / {"message": …}	fine
None	AttributeError: 'NoneType' object has no attribute 'get'
"text"	AttributeError: 'str' object …
422	AttributeError: 'int' object …
["boom"]	AttributeError: 'list' object …

Four shapes, not two — the rule is "anything that is neither a dict nor absent", and I generalised from the two the docstring happened to name instead of enumerating. Worth correcting in the record since I put a number in a review; it doesn't move the conclusion, it strengthens it — three of the four ride the fallback loop and the string has its own test, so _storage_job_error_message is exercised across the whole non-dict space rather than at two points in it.

Description reads right now: 5755 … as of 5e1d6bd, 992f995 with top billing in How, both follow-ups listed with the disclosure on mine.

Nothing outstanding from me. The approval comes from @padak's own hand — my tooling declines to cast it, which is the correct call for a repo whose merge gate is one approving review.

[padak]

Approved — with a disclosure: I pushed the nits myself

Rather than leave you a second round of comments over three cosmetic things, I fixed two of them on your branch: 5e1d6bd. Revert it without discussion if you'd rather own them.

What's in it (no behaviour, no production code):

• test_unusable_error_field_falls_back_to_generic_message used None as its "leave the key out" sentinel, so the loop that looks like it covers {"error": None} never actually built that body. It matters more than the others: {"error": None} raised AttributeError on the pre-992f995 extraction — verified in isolation, job.get("error", {}).get("message", …) gives 'NoneType' object has no attribute 'get'. Absence now has an _OMIT sentinel, so null is a real case. (Per your correction, which I re-checked and agree with: the old expression raised on four shapes — None, "text", 422, ["boom"], i.e. anything neither dict nor absent — so the fallback loop now exercises three of the four and the string has its own test.)
• The new gotcha section got the blank line before its ## heading that 125 of the file's other 127 headings have. (The remaining exception at gotchas.md:1655 is pre-existing; left alone.)

make check green on the pushed head: 5755 passed, 12 skipped.

Description: resolved on your side — Testing reads 5755 … as of 5e1d6bd, 992f995 has top billing in How, and both follow-ups are listed with the disclosure on mine. That was the third nit and it is closed.

What I verified by reproduction, not by reading

1. The tests are red against the unfixed poller. _core.py restored from origin/main under your test file: 3 failed, 5 passed — test_already_failed_body_raises among them. The xfail(strict=True) → removal sequencing did its job.
2. "Poll counts are identical" holds exhaustively. Both loops against a deterministic fake clock, max_wait ∈ {0, 0.001, 0.5, 1, 1.5, 2, 3, 60, 600} × 6 status sequences: 9 differences in the whole sweep, every one of them the status: error initial body. Sub-interval overshoot and the max_wait=0 no-sleep case included.
3. make check green locally (macOS/py3.12) at both 992f995 and 5e1d6bd; CI green on all four jobs.
4. The bug class is confined to this poller. wait_for_queue_job and wait_for_query_job both fetch before checking and neither takes a caller-supplied initial body, so neither can grow the second check that drifted here.
5. 19 call sites re-counted on main (16/2/1), all return job / return job.get("results", {}) / bare wait — the "empty success" mechanism is exactly as described.

Approving on the strength of 1–5, which cover your work. 5e1d6bd is mine and I am explicitly not reviewing my own commit — give it a glance before merging.

Merge order per your note: land this before #556 so its local guard in merge_requests.merge() drops out on rebase.