feat(0.22.0): queue polling parity — exponential curve + log tail + auto-cancel - #202
Closed
ottomansky wants to merge 5 commits into
Closed
feat(0.22.0): queue polling parity — exponential curve + log tail + auto-cancel#202ottomansky wants to merge 5 commits into
ottomansky wants to merge 5 commits into
Conversation
…uto-cancel
Close FIIA P0-3: `kbagent job run --wait` now matches the cadence and
semantics of the official keboola-as-code Go CLI that FIIA currently
depends on, so FIIA can drop its custom Queue-polling code.
Resolution order / behaviour changes:
* `wait_for_queue_job` polls on an exponential curve (2s x 30 -> 5s x
48 -> 15s) instead of the fixed 1s interval. `--poll-strategy fixed`
keeps the legacy cadence for tests and very short jobs.
* On FAILED / WARNING / TERMINATED terminal states, fetch the last
--log-tail-lines events (default 200, 0 disables, max 5000) via the
Storage Events API `GET /v2/storage/events?runId=...` and attach
them as `logTail` on the JSON result or `details.logTail` on errors.
The Queue API has no /jobs/{id}/events route despite the name;
verified 404 against queue.europe-west3.gcp.keboola.com.
* If the local --timeout elapses before the remote finishes, issue
`kill_job` against the Queue, poll briefly for terminal state, and
raise `JOB_TIMEOUT_TERMINATED` (exit 7) with the cancelled job +
logTail in `details`. If the kill itself fails, surface the legacy
`QUEUE_JOB_TIMEOUT` (exit 4, retryable=True) so scripts can tell
"we killed it" from "local gave up, remote may still run".
Architecture:
* `KeboolaApiError.details: dict` carries structured context without
touching the stable top-level JSON envelope; ErrorResponse gains
an optional `details` field, emitted only when non-empty.
* Reuses existing `kill_job` (POST /jobs/{id}/kill) -- did not add a
duplicate `terminate_job` method; the real endpoint is `/kill`.
Tests:
* tests/test_client.py: 13 new unit tests — curve math, fixed fallback,
strategy validation, deadline-trim, QUEUE_JOB_FAILED/TIMEOUT paths,
fetch_job_events list/dict/limit/unknown-shape.
* tests/test_services.py: 10 new unit tests — poll_strategy threading,
log-tail attach on warning/terminated, timeout-terminate + fallback,
log-tail fetch failure swallowed, negative/invalid-arg validation.
* tests/test_cli.py: 6 new tests — flag forwarding, click.Choice reject,
range-check reject, exit-7 JOB_TIMEOUT_TERMINATED, exit-4 fallback.
* tests/test_e2e.py: 4 new live tests against a real Keboola project
— guaranteed-fail python job produces non-empty logTail; --timeout
< runtime produces exit 7 + remote kill confirmed; --poll-strategy
fixed reaches completion; direct fetch_job_events on a finished job.
Pre-PR 3-agent review loop: wire-contract (queue vs storage endpoint,
poll curve cadence, exit-code mapping), silent-failure audit (broad
except, falsy-check skips, exception-mutation pattern), test-coverage
enumeration (per-branch). No critical/high findings; observability
improvements applied (debug logs on unhandled wait codes, terminate
None-return disambiguation, empty-runId guard).
…sort + deadline cap
Round 1 of the post-PR review loop surfaced two actionable findings; this
commit addresses them plus a handful of medium-severity robustness items.
CRITICAL — exception chaining / no mutation:
_handle_wait_error previously mutated the caught exception's .details
dict in-place via _attach_log_tail and re-raised the same instance.
That violates Python's exception chaining guarantee (no `from exc`)
and risks logTail contamination if an upstream harness ever stored
and re-raised the same KeboolaApiError. Replace with:
- a new _enrich_with_tail helper that returns a fresh details dict
without touching the original
- `raise KeboolaApiError(..., details=...) from exc` in every
specialised path (QUEUE_JOB_FAILED, QUEUE_JOB_TIMEOUT + kill ok,
QUEUE_JOB_TIMEOUT + kill failed fallback)
- bare `raise` for the fall-through path (observability log already
in place from the pre-PR review)
_attach_log_tail is gone.
HIGH — defensive ordering in _safe_fetch_log_tail:
Storage Events API returns newest -> oldest today, but our code trusted
that ordering via a comment alone. Sort explicitly by `created` DESC
with missing-field events sinking to the bottom, so a future API
ordering change cannot silently invert the surfaced tail.
MEDIUM — deadline overshoot in _terminate_and_wait:
The polling loop always slept a full second after each GET, even when
the grace deadline had only milliseconds left. Cap the sleep at
`min(1.0, max(0.0, deadline - monotonic()))` so the grace period is
honoured precisely. Added `remaining = deadline - monotonic()` check
at the top of each iteration with explicit break on <= 0.
MEDIUM — test robustness:
* TestWaitForQueueJob monotonic mocks now use a clamped callable
(last-value after exhaustion) instead of a bare `iter([...])`, so a
refactor adding observability calls produces a sensible reading
instead of an uninformative StopIteration.
* New test_run_job_unhandled_wait_code_bubbles_up_unchanged locks the
observability fall-through for error codes we don't specialise.
* New test_run_job_failure_exception_chaining_does_not_mutate_original
proves the CRITICAL fix: original exception.details stays empty,
the re-raised instance is new, and `__cause__` is set.
* New TestSafeFetchLogTailDefensiveSort covers the defensive sort with
both happy-path and missing-`created` edge cases.
* E2E class now derives its alias from `request.node.name` so
pytest-xdist parallel runs can't collide on a shared project alias.
All 46 PR4 unit/CLI tests pass. 4/4 E2E tests pass live against project
1143 in 56s.
Round 2 of the post-PR review loop surfaced user-facing polish items:
HIGH — help text jargon:
`kbagent job run --help` referenced "FIIA" and "keboola-as-code"; neither
is meaningful to external users. Replaced with plain-English descriptions
that explain what the curve actually does (starts fast, relaxes toward
15s) and when `fixed` is useful. Same cleanup in the command docstring
and plugin references.
HIGH — JSON envelope shapes not documented:
AGENT_CONTEXT listed the new exit codes but didn't show the JSON shapes
consumers need to match with jq. Added a "--json response shapes by
exit code" section with the four distinct envelopes (exit 0 success,
exit 1 QUEUE_JOB_FAILED, exit 4 QUEUE_JOB_TIMEOUT, exit 7
JOB_TIMEOUT_TERMINATED) plus a recommended jq pattern that picks up
the tail regardless of success/failure.
MEDIUM — hint signature:
`kbagent --hint client job run` was generating
`fetch_job_events(job_id=...)` but the real client method takes
`run_id` (Storage Events endpoint is runId-keyed). Updated the hint to
emit `fetch_job_events(run_id=str(job.get("runId") or job["id"]), ...)`
so the generated Python code compiles against our client.
MEDIUM — automation guidance:
Called out that `--log-tail-lines 0` is recommended for automation
pipelines to keep the JSON envelope compact, both in the Typer help and
the plugin reference.
MEDIUM — concurrent-kill idempotency:
Documented in `_terminate_and_wait`'s docstring that concurrent kill
attempts (user firing `job terminate` while `job run --wait` is
auto-cancelling the same job) are safe because the Queue's
"not in killable states" 400 response falls through the existing
post-kill GET to confirm terminal state.
No behavioural changes. Same 46/46 unit tests pass; same 4/4 E2E tests
pass live against project 1143 in 59s. SKILL.md regenerated (no diff).
Round-2 review consolidation:
* Fix-verification agent: all round-1 fixes verified correct, no
regressions detected.
* API-ergonomics agent: 2 HIGH (jargon + JSON shapes, both addressed
here), 2 MEDIUM (hint sig + automation default, addressed).
* Integration agent: 0 critical/high, 2 MEDIUM -- double-kill
idempotency (documented), error-code enum for PR9 (intentionally
deferred, that's PR9's scope).
ottomansky
marked this pull request as ready for review
April 22, 2026 20:59
padak
previously approved these changes
Apr 22, 2026
padak
dismissed
their stale review
April 22, 2026 21:41
Replacing with English version for broader team visibility.
padak
approved these changes
Apr 22, 2026
padak
left a comment
Member
There was a problem hiding this comment.
Approve — merge-ready
Solid feature with real value (FIIA P0-3 parity with the Go CLI). All three review rounds addressed in commits. No blockers.
Nice-to-have (follow-up)
- Magic number:
src/keboola_agent_cli/services/job_service.py:150, 156hastime.sleep(min(1.0, ...))with a hardcoded1.0. Per CONTRIBUTING.md this should live inconstants.pyasJOB_TERMINATE_POLL_INTERVAL = 1.0. - SKILL.md: Run
make skill-genlocally —--poll-strategyand--log-tail-linesshould appear in the decision table. The CI freshness check would catch it, but belt + braces. - Dead code:
src/keboola_agent_cli/commands/job.py:380-383branchstatus == "error"in human-mode rendering is effectively unreachable (the service always raisesQUEUE_JOB_FAILED). Either remove it, or add a comment "defensive for future service-layer changes".
Positives
- The
JOB_POLL_CURVEcurve (2s×30 → 5s×48 → 15s) is elegant; the_iter_poll_intervalsgenerator is well tested. - Extending
KeboolaApiError.details: dict+ErrorResponsewithexclude_none=Truekeeps the.error.details.*envelope stable. - The new exit code 7 (
JOB_TIMEOUT_TERMINATED) vs. existing 4 (QUEUE_JOB_TIMEOUT, retryable) nicely distinguishes the reason. - Test matrix 406+232+200+293 = 1131 test lines, all passing.
Per CONTRIBUTING.md, magic numbers belong in constants.py. Replace the hardcoded 1.0 sleep cadence in _terminate_and_wait's grace-window loop with JOB_TERMINATE_POLL_INTERVAL so the cadence is documented and centrally adjustable.
…ring The service layer always raises QUEUE_JOB_FAILED for failed jobs, so the elif status == 'error' branch inside job_run's human-mode rendering was unreachable. Remove it and collapse the trailing else into a flatter if/elif/else, keeping the warning branch's log-tail attached. A short inline comment preserves the invariant for future readers in case the service-layer contract changes.
Contributor
Author
|
Addressed the magic-number + dead-code nice-to-haves:
Re: SKILL.md — ran Targeted tests: 69/69 pass. |
padak
added a commit
that referenced
this pull request
Apr 23, 2026
…cel (#202) Exit code 7 (JOB_TIMEOUT_TERMINATED) distinguishes local-timeout-killed from queue-side failure (exit 4). Log tail N events attached to logTail on failure or dict on WARNING/TERMINATED. Legacy 1s fixed polling preserved behind --poll-strategy fixed for tests and very short jobs. Polling curve: 2s x 30 -> 5s x 48 -> 15s forever. KeboolaClient.wait_for_queue_job gains poll_strategy kwarg. KeboolaClient.fetch_job_events wraps Storage API /v2/storage/events?runId=. JobService._handle_wait_error centralises failure enrichment. Max's review fixes: existing error codes reused (no new literal strings).
Member
|
Integrated into Full integration PR coming as release/0.22.0 -> main. |
padak
added a commit
that referenced
this pull request
Apr 23, 2026
- sync-workflow.md: new "Adopting an existing kbc Go CLI checkout" section for sync init --adopt-existing (#201). - gotchas.md: new entries for exit code 7 (JOB_TIMEOUT_TERMINATED), --poll-strategy fixed, --log-tail-lines N, logTail response semantics (#202); new section for --deny-writes / --deny-destructive (#203); new section for sync init --adopt-existing (#201). - workspace-workflow.md: new "Orphan detection + garbage collection" section for workspace list --orphaned + workspace gc (#204). - commands-reference.md: sync init --adopt-existing flag documented. - README.md: "What it does" table + "All commands" block updated with the 15 new commands shipped in 0.22.0 (flow CRUD, config metadata, workspace GC, storage describe, project pin + firewall, queue polling parity).
This was referenced Apr 23, 2026
ottomansky
pushed a commit
to ottomansky/keboola-agent-cli
that referenced
this pull request
Apr 30, 2026
…tion Three PRs' changelog entries were dropped during the git merge --squash conflict resolution of release/0.22.0: - keboola#201 ErrorCode enum + sync init --adopt-existing (3 entries) - keboola#202 queue polling parity (5 entries) - keboola#205 storage describe-bucket/table/column/batch (6 entries) Without this fix, 'kbagent changelog' on installed 0.22.0 would show only 18 of the 32 features shipped in this release. Adding the missing 14 entries before tagging v0.22.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close FIIA P0-3:
kbagent job run --waitnow matches the polling contract FIIA currently depends on in the official keboola-as-code Go CLI. After this merges FIIA can drop its custom Queue-polling code and depend on kbagent forjob run+ log tail + timeout-cancel.Summary
--waitnow walks an exponential curve (2s × 30 → 5s × 48 → 15s forever) via newJOB_POLL_CURVE. Legacy fixed 1s behaviour preserved behind--poll-strategy fixedfor tests and very short jobs.--log-tail-lines N) and surfaces them aslogTailon the JSON result ordetails.logTailon errors.kill_jobagainst the remote and exits 7 (EXIT_JOB_TIMEOUT_TERMINATED) with the cancelled job dict + logTail attached. If the kill itself fails, surfacesQUEUE_JOB_TIMEOUT(exit 4, retryable=True) so scripts can distinguish "we killed it" from "local gave up, remote may still run".{job, logTail}on timeout-terminate and{logTail}on queue failures. JSON envelope addserror.detailsonly when non-empty.Resolution order / behaviour
successresolvedVariableValuesIderrorQUEUE_JOB_FAILEDdetails.logTailwarning/terminatedlogTailon result--timeout+ kill okJOB_TIMEOUT_TERMINATEDdetails.job+details.logTail--timeout+ kill failsQUEUE_JOB_TIMEOUTdetails.logTailArchitecture
KeboolaClient.wait_for_queue_job(poll_strategy=...)dispatches through a new_iter_poll_intervals()generator. Deadline check trims the last sleep so we never overshoot by one interval.ValueErroron invalid strategy (programming error; service layer validates first).KeboolaClient.fetch_job_events(run_id, limit)wraps Storage APIGET /v2/storage/events?runId=...— verified live. The Queue API has no/jobs/{id}/eventsroute despite the name (my first attempt returned 404, caught by E2E before PR).JobService._handle_wait_error()centralises failure enrichment; reuses existingkill_jobrather than adding a duplicateterminate_job._safe_fetch_log_tail()/_terminate_and_wait()never raise — log-tail and kill are convenience surfaces; a secondary failure must not mask the primary error.Pre-PR review loop
Three Explore agents in parallel, orthogonal focuses:
/jobs/{id}/eventsmistake before E2E and before PR._terminate_and_waitreturnsNonefrom three paths without distinguishing them, (b) empty-stringrunIdswallow, (c)_handle_wait_errorfall-through lacked a debug log. All addressed withlogger.debug(...)disambiguation calls. No HIGH/CRITICAL.fetch_job_eventswithlimit<=0,wait_for_queue_jobwith non-dictresult,_terminate_and_waitkill-throws + GET-returns-non-terminal race, empty-string runId). No tests passing for the wrong reason.Test plan
uv run ruff check src/ tests/— cleanuv run ruff format --check src/ tests/— cleantest_log_tail_surfaced_on_queue_job_failed— deterministic pythonraise→QUEUE_JOB_FAILED+ non-emptydetails.logTailtest_timeout_triggers_remote_kill_and_exits_seven— 120s sleep +--timeout 8→ exit 7, remotestatus ∈ {terminated, cancelled, terminating}test_fixed_poll_strategy_reaches_completion— 3s job +--poll-strategy fixed→ exit 0, status=successtest_fetch_job_events_returns_list_on_real_job— direct client call against/v2/storage/events?runId=...Files touched
src/keboola_agent_cli/constants.py—JOB_POLL_CURVE,VALID_POLL_STRATEGIES,DEFAULT_POLL_STRATEGY,DEFAULT_LOG_TAIL_LINES,MAX_LOG_TAIL_LINES,JOB_TERMINATE_GRACE_SECONDS,EXIT_JOB_TIMEOUT_TERMINATED.src/keboola_agent_cli/client.py—_iter_poll_intervals,wait_for_queue_job(poll_strategy),fetch_job_events.src/keboola_agent_cli/services/job_service.py—_safe_fetch_log_tail,_attach_log_tail,_terminate_and_wait, extendedrun_job, new_handle_wait_error.src/keboola_agent_cli/commands/job.py—--poll-strategy,--log-tail-lines,_render_log_tail,formatter.error(details=...).src/keboola_agent_cli/commands/_helpers.py—map_error_to_exit_codegainsQUEUE_JOB_TIMEOUT → 4andJOB_TIMEOUT_TERMINATED → 7.src/keboola_agent_cli/errors.py—KeboolaApiError.details.src/keboola_agent_cli/models.py+output.py—ErrorResponse.details(omitted when empty).src/keboola_agent_cli/hints/definitions/job.py— hint step for log-tail fetch + cadence note.src/keboola_agent_cli/commands/context.py,CLAUDE.md, plugincommands-reference.md— doc updates for the new flags / exit codes.pyproject.toml,plugin.json,changelog.py— 0.21.1 → 0.22.0.tests/test_client.py(+214),tests/test_services.py(+298),tests/test_cli.py(+200),tests/test_e2e.py(+289).Breaking changes
--waitchanges from fixed 1s to the exponential curve. Scripts that depended on tight 1s polling can opt back in with--poll-strategy fixed. Documented in the changelog.QUEUE_JOB_TIMEOUTnow maps to exit code 4 (was 1). Scripts that distinguished by exit code should update; the error code itself is unchanged.JOB_TIMEOUT_TERMINATEDis a new error code + new exit 7. Pre-0.22.0 versions of kbagent never produced this.