Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude-plugin/marketplace.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"plugins": [
{
"name": "kbagent",
"version": "0.84.2",
"version": "0.84.3",
"source": "./plugins/kbagent",
"description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces",
"category": "development"
Expand Down
2 changes: 1 addition & 1 deletion plugins/kbagent/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "kbagent",
"version": "0.84.2",
"version": "0.84.3",
"description": "AI-friendly interface to Keboola Connection projects — explore configs, jobs, lineage, call MCP tools, manage dev branches, and debug SQL in workspaces",
"author": {
"name": "Keboola",
Expand Down
37 changes: 37 additions & 0 deletions plugins/kbagent/skills/kbagent/references/gotchas.md
Original file line number Diff line number Diff line change
Expand Up @@ -3633,6 +3633,43 @@ Four things a coding agent will otherwise get wrong:
does not accept a Storage API token -- it is the still-open primary ask of
issue #594. Do not imply this command covers billing/invoice history; tell
the user it is out of reach from the CLI today.

## A Storage job that failed instantly used to report success (since v0.84.3)

Every async Storage operation -- table import/export, `create-table`,
`delete-table`, `truncate-table`, snapshot create/restore, dev-branch
create/delete, bucket share, workspace table load -- enqueues a Storage job and
waits on the same shared poller. Before v0.84.3 that poller checked for a
terminal state in **two** places, and they had drifted: the check on each
*polled* body raised `STORAGE_JOB_FAILED` on `status: error`, but the check on
the *initial* response body (the one the enqueuing POST/PUT/DELETE returns)
handed the job straight back to the caller.

So when the Storage API failed **fast** -- answering with an already-terminal
error instead of a queued `waiting` job -- the failure was swallowed. Every call
site returns either that job or `job.get("results", {})`, and an error job
carries no `results`, so the operation came back as an **empty success**: exit
code 0, no error output, nothing actually done. Jobs that failed after at least
one poll always raised correctly, which is why this never showed up in normal
use -- only fast failures were affected.

- **Since v0.84.3** the terminal state is evaluated once, at the top of the poll
loop, so the initial body and every polled body travel identical code. A fast
failure now raises `STORAGE_JOB_FAILED` like any other failed job.
- **If you have a script that treated an empty result as success**, it will now
see the error it should always have seen. That is the intended change; check
for `if not result:` style guards written against the old behaviour.
- **Nothing else moved**: wait budgets (`STORAGE_JOB_MAX_WAIT` 60 s,
`IMPORT_JOB_MAX_WAIT` / `EXPORT_JOB_MAX_WAIT` 600 s), poll cadence, the
up-to-one-interval deadline overshoot, error messages and exit codes are
byte-identical.
- **Known narrowness, unchanged by the fix**: the poller recognises only
`success` and `error`. Any other terminal status the Storage API might report
would exhaust the whole budget and surface as `STORAGE_JOB_TIMEOUT`
(`retryable: true`) rather than a failure -- so a wait that times out suspiciously
fast against a small budget is worth reading as "unknown status", not "still
running".

## `config update --set 'state...'` is now a hard error, not a silent no-op (since v0.84.2)

Before v0.84.2, `--set` on `config update` / `config row-update` applied every
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "keboola-cli"
version = "0.84.2"
version = "0.84.3"
description = "AI-friendly CLI for managing Keboola projects"
readme = "README.md"
requires-python = ">=3.12"
Expand Down
25 changes: 25 additions & 0 deletions src/keboola_agent_cli/changelog.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,31 @@

# Ordered newest-first. Each value is a list of brief one-line descriptions.
CHANGELOG: dict[str, list[str]] = {
"0.84.3": [
"Fix: a Storage job that failed instantly no longer reports success. "
"`_wait_for_storage_job`, the poller every async Storage operation waits on, checked "
"for a terminal state in two places -- once on the response body the caller hands in, "
"once on each polled body -- and the two had drifted: the polled check raised "
"`STORAGE_JOB_FAILED` on `status: error`, the initial check handed the job straight "
"back. So whenever the Storage API failed FAST (answering with an already-terminal "
"error instead of a queued `waiting` job) the failure was swallowed. All 19 call "
"sites in `client/` either return that job or its `results` -- and an error job "
"carries no `results` -- so a failed table import, dev-branch create/delete, "
"snapshot, bucket share or table export came back as an empty success: exit 0, no "
"error, nothing done. Only jobs that failed after at least one poll ever raised. "
"The poller now evaluates the terminal state ONCE, at the top of the loop, so the "
"caller's body and every polled body travel identical code and the two checks cannot "
"drift apart again. Wait budgets, poll cadence, overshoot, error messages and exit "
"codes are unchanged -- the only behaviour that moves is that a fast failure now "
"raises instead of returning. Scripts that treated an empty result as success will "
"now see the error they should always have seen. Also adds the poller's first tests "
"(`TestWaitForStorageJob`): it had none of its own, and neither the fast-fail path "
"nor the timeout path was covered anywhere. Hardened the failure message extraction "
"at the same time: a job whose `error` field is a plain string (or an int, as the "
"Metastore once returned) used to raise `AttributeError` -- a traceback instead of a "
"clean `STORAGE_JOB_FAILED` exit -- and a string error's text is now used as the "
"message instead of being discarded.",
],
"0.84.2": [
"New: `kbagent config clone` duplicates a configuration WHOLE (closes #587). "
"`--project P --component-id C --config-id ID --name N [--target-project P2] "
Expand Down
68 changes: 58 additions & 10 deletions src/keboola_agent_cli/client/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,32 @@
from ..stream_client import StreamClient


def _storage_job_error_message(job: dict[str, Any]) -> str:
"""Best-effort human message out of a failed Storage job's ``error`` field.

Written tolerantly on purpose. An API ``error`` field is not reliably a
dict in this codebase's experience: the Metastore once answered with an
int (``{"error": 422}``), which is why ``BaseHttpClient._raise_api_error``
accepts ``error`` only when it is a non-empty string; the Queue poller
guards its own ``result`` with ``isinstance``; and
``_extract_query_job_error`` handles strings, dicts and unknown shapes.
Assuming a dict here would turn a failed job into an ``AttributeError``
traceback instead of ``STORAGE_JOB_FAILED`` -- and since the terminal
check now also sees the caller's initial response body, that shape would
arrive from one more direction than before.
"""
error = job.get("error")
if isinstance(error, dict):
message = error.get("message")
# A dict without a usable message falls through to the generic text
# rather than rendering "None" or a raw dict repr at the user.
if isinstance(message, str) and message:
return message
elif isinstance(error, str) and error:
return error
return "Storage job failed"


class _CoreClient(BaseHttpClient):
"""Shared plumbing base for the Keboola client mixins."""

Expand Down Expand Up @@ -195,8 +221,30 @@ def _wait_for_storage_job(
) -> dict[str, Any]:
"""Poll a Storage API job until it reaches a terminal state.

The terminal state is evaluated in ONE place for the caller's initial
body and for every polled body alike -- the loop checks before it
fetches. Keep it that way: this used to be two checks (an early return
before the loop plus a second check inside it) and they drifted, so an
already-terminal ERROR initial body was returned as-is instead of
raising. Every call site either returns the job or its ``results``, so
that surfaced as a silent empty success.

The check-then-fetch *shape* matches the sibling pollers
``wait_for_queue_job`` / ``wait_for_query_job``; the behaviour does not,
and deliberately so -- this is not a parity claim. Two differences worth
knowing: this poller recognises only ``success`` and ``error``, so any
other terminal status the Storage API might report would exhaust the
whole budget and surface as ``STORAGE_JOB_TIMEOUT`` (the queue poller
keys off ``isFinished`` and ends on any terminal state), and the sleep
here is not capped to the remaining budget, so a wait overshoots its
deadline by up to one poll interval. Both predate this restructure.

Args:
job: Initial job response from POST/DELETE.
job: Initial job response from the request that enqueued the job
(POST, PUT or DELETE -- e.g. ``change_sharing_type`` enqueues
with PUT). May already be terminal (the Storage API can fail
fast, never returning ``waiting``), in which case no request
is made at all.
max_wait: Maximum seconds to wait (default: STORAGE_JOB_MAX_WAIT).

Returns:
Expand All @@ -206,25 +254,25 @@ def _wait_for_storage_job(
KeboolaApiError: If the job fails or times out.
"""
job_id = job.get("id")
if job.get("status") in ("success", "error"):
return job

deadline = time.monotonic() + max_wait
while time.monotonic() < deadline:
time.sleep(STORAGE_JOB_POLL_INTERVAL)
response = self._request("GET", f"/v2/storage/jobs/{job_id}")
job = response.json()
while True:
status = job.get("status")
if status == "success":
return job
if status == "error":
error_msg = job.get("error", {}).get("message", "Storage job failed")
raise KeboolaApiError(
message=error_msg,
message=_storage_job_error_message(job),
status_code=500,
error_code=ErrorCode.STORAGE_JOB_FAILED,
retryable=False,
)
Comment on lines 266 to 268

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 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)

Open in Devin Review

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Checked before sleeping, so an exhausted budget never costs a
# poll interval.
if time.monotonic() >= deadline:
break
time.sleep(STORAGE_JOB_POLL_INTERVAL)
job = self._request("GET", f"/v2/storage/jobs/{job_id}").json()

raise KeboolaApiError(
message=f"Storage job {job_id} did not complete within {max_wait}s",
status_code=504,
Expand Down
Loading