Repository navigation
Choria transport fixes and cleanup - #349
Merged
Merged
Conversation
bastelfreak
approved these changes
Oct 28, 2026
nmburgan
force-pushed
the
choria-transport-fixes
branch
from
November 2, 2026 17:55
53efca7 to
fde56e0
Compare
cleanup_tmpdir checked File.basename for the bolt-choria- prefix, which does not split a backslash path on Linux, so a Windows target's tmpdir was refused and left behind. The check now splits on either separator. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
The gem replaces an options timeout of exactly 5, its own default, with the DDL timeout plus the discovery timeout, so an rpc-timeout of 5 was never applied. The timeout now goes through the accessor after the build, where it is set correctly. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
The rescue around shell.kill never fired, because rpc_request rescues for itself, so a kill that failed was silent. The failures are now read from the response and logged. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
batch_task_with discovered a target that stayed silent in the pre-warm discovery again per target and waited out the timeout again. It now reports such a target as an error once and runs the task on the rest. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
rpc_request files a reply with status code 1 under the responded targets, as it does for download, so a target whose run_no_wait answered that way was polled for a task that never ran, or, when its reply came first, its missing task_id failed the whole group as missing-task-id. Such a reply is now an error under bolt/choria-task-start-failed, the way the download step already handles its own status 1 replies, and the task_id comes from a target that started. rpc_request returns the status messages as well, so the messages for a refused start and a failed download end with what the agent said. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
Every batch method fired node_start for its capable targets after prepare_targets had already fired it for the incapable ones, from 3 places. prepare_targets now fires it for every target before discovery, and the callers lose their copies. Also, we no longer really need emit_results to take fire_node_start, so that is removed. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
A reply with status code 1 is filed under the responded targets, and the two callers that need to know picked it out of a per-target map of status codes and another of status messages. rpc_request now returns aborted, the status message per target that answered 1, and the maps go. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
After RPC_FAILURE_RETRIES failed polls in a row, poll_with_retries reported a flag that both callers turned into the same error. It now puts a bolt/choria-poll-failed error naming the poll into the completed results for each pending target, and the callers only handle the deadline. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
The helper took the reply data and looked the code up under a symbol or a string key. Each caller knows which key its reply uses, so it passes the code and the helper only supplies the default of 1 with a warning. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
shell_list mixed the targets whose process ended with those whose poll failed in one hash that the caller split again by looking for an error key. It now returns them as 2 hashes, and wait_for_shell_results fetches output for the finished ones and keeps the failures as they are. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
run_script and upload_and_start_task each uploaded a file, made it executable and emitted the failures, and each built the command that runs it with an interpreter in its own way. stage_executable and executable_command do both once, and powershell_env_statements builds the $env assignments that prepend_env_vars and build_task_command both needed. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
batch_task_with recorded a tmpdir for cleanup only after upload_and_start_task returned, so an interrupt during an upload left the directory on the targets. The caller now generates the path and records it before the uploads start. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
- The rescue in batch_connected? that nothing could reach, since rpc_request rescues for itself - The fallback on a task's file list - The empty-pending guards and the fetched-output fallback in shell.rb - The keyword defaults of the output helper - The verbose and progress settings on the client, which the gem's defaults already give - The shell_run label fallback - The Array() on targets that every caller passes as an array - The ssl-cert and ssl-key checks that config validation already enforces and the copies and defaults around them - The cache check that 5 places spelled out is discovered?(target) - Prepare_targets reuses undiscovered_error for a target that did not answer discovery Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
The per-target options bullet called the first-target read inherent to the batch model. It now names the options a batch shares, says the first target's values are used, and gives the reason batching by option values was not done. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
The provided_features comment said the list drives task selection, but select_implementation overrides the Base method that reads it; the list serves apply_prep. The context parameter of poll_with_retries also names the poll failure error now, not only log lines. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
Examples for behavior the branch changes that had none: unsafe environment variable names refused at both call sites, upload and chmod failures for 1 of 2 targets on the script and the task path, a script run on a Windows target with no chmod, a task run with nothing pending that sends no poll and does not sleep, the tmpdir generated in run_task_via_shell used for every step and removed, and the tmpdir removed when build_task_command raises after the uploads. The upload_file_content comment and the developer doc said a file can be roughly 750 KB under the 1 MB NATS limit. The shell agent's DDL caps a command at 10,240 characters, so a file can be about 7.5 KB on POSIX and under 3 KB on Windows. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
…rror rpc_request filed every statuscode 1 reply under the responded targets, because bolt_tasks answers 1 for a task that ran and failed with the data still valid. The shell and rpcutil agents never answer 1 with usable data, and an action policy that denies the caller answers 1 with "You are not authorized to call this agent or action", so a denial was reported as a missing handle, exit code 1, a handle not found, no data or no response to discovery, with the reason in a log line only. A statuscode 1 reply is now an RPC error with the agent's status message unless the caller passes accept_aborted: true, which the 3 bolt_tasks calls and the ping behind batch_connected? do. The agent inventory failure log line moves from debug to warn and replaces the "No response" line that repeated it. Signed-off-by: nmburgan <13688219+nmburgan@users.noreply.github.com> Assisted-by: Claude Fable 5.1 <noreply@anthropic.com>
nmburgan
force-pushed
the
choria-transport-fixes
branch
from
November 2, 2026 17:55
fde56e0 to
4a09d5c
Compare
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
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.
This PR includes a bunch of fixes and cleanup for the existing Choria transport code in preparation for the file transfer (upload/download file) changes coming next. Review each commit individually.