Repository navigation
fix(agent): run interrupted software list requests again on restart - #4388
jarhodes314 wants to merge 3 commits into
Conversation
A software_list request waiting in the software manager queue when the agent exits, e.g. after a self-update, was lost, and the restarted agent kept the command at executing forever. On resume, a software_list command awaiting its builtin operation is now moved back to the step that triggers that operation, so the request is sent again to the software manager. Other builtin operations are left unchanged, as re-running a software update is not safe. The software manager no longer reports as failed a software list request it was running when the agent stopped. That failure completed the command before the result of the request sent again on resume was published. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: James Rhodes <jarhodes314@gmail.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
| /// Return the builtin operation triggered just before the current state of a command, | ||
| /// along with the state triggering this builtin operation | ||
| fn builtin_operation_trigger_state( | ||
| &self, | ||
| command_state: &GenericCommandState, | ||
| ) -> Option<(&OperationName, &StateName)> { | ||
| let operation_name = command_state.operation()?; | ||
| let version = command_state.workflow_version()?; | ||
| let entry = self.commands.entry(&command_state.topic.name)?; | ||
| let workflow = self | ||
| .workflows | ||
| .get(&(entry.entity_type, operation_name.as_str().into()))? | ||
| .get(version) | ||
| .ok()?; | ||
| workflow | ||
| .states | ||
| .iter() | ||
| .find_map(|(state, action)| match action { | ||
| OperationAction::BuiltInOperation(operation, handlers) | ||
| if handlers.on_exec.status == command_state.status => | ||
| { | ||
| Some((operation, state)) | ||
| } | ||
| _ => None, | ||
| }) | ||
| } |
There was a problem hiding this comment.
issue: What if in a custom workflow there's more than 1 possible state that triggered command we're awaiting?
For example:
operation = "software_list"
[init]
script = "/etc/tedge/operations/pick-action.sh"
on_stdout = ["list_first", "update_first"]
[list_first]
operation = "builtin:software_list"
on_exec = "executing"
[update_first]
operation = "builtin:software_update"
on_exec = "executing"
[executing]
action = "await-operation-completion"
on_success = "successful"
[successful]
action = "cleanup"
[failed]
action = "cleanup"From my understanding, because we find and return the first state that matches (which starts a builtin operation and has on_exec to current state), and iteration order of the hashmap is unstable, when actually running the workflow, we could possibly enter executing via update_first, but then on restart execution state could be moved back to list_first. (though i'm not 100%, I haven't actually tried that)
Default sotfware_list workflow will not suffer from this, but I think this change seems to brake such (IMO valid) custom workflows. I see possible ways forward:
- [AI recommended] for the purposes of this PR, add check if there is only 1 possible previous state we could transition from, and if not, fall back to the previous behaviour
- consider such workflows, using one state to await multiple different operations, invalid
- or, if we still want this retry to work in custom workflows, I think there's no other way but to actually record at runtime which states we go through and use that runtime information to go back to the previous state, which would be much more complex change but potentially reusable for other scenarios
There was a problem hiding this comment.
I think this is a reasonably rare case. IMO the core behaviour we're trying to fix here is "the agent drives the operation to a terminal state". In my mind, this means either resuming operations or marking them as failed if we can't do that (e.g. with this case where there's an ambiguity). @albinsuresh, do you have thoughts here?
Recording the state transitions the workflow took would be neat and I can see how that would be useful (we could feasibly show a user the trace of an operation somewhat like home assistant does for automations). I don't think that's a thing this PR wants to go into though.
There was a problem hiding this comment.
[AI] The exact workflow above is rejected when the agent loads it. Roughly what tedge-agent does on loading a workflow file:
load(workflow_file)
for each state in workflow
if state.action is builtin:<op> and <op> != workflow.operation
error "The `builtin:<op>` cannot be invoked from `<workflow.operation>`, but only from `<op>`"
register a placeholder workflow for <workflow.operation>
that fails every command with "Invalid operation workflow definition ..."
I ran tedge-agent with it to confirm: a software_list command goes init → executing → failed with that reason, so it never reaches the resume code. There's now a test that loads your workflow and checks it's rejected.
Your point still found a real case though: the awaiting state can be shared with a sub-operation step (operation = "some_custom_op", on_exec = "executing"). That step persists the command at executing before publishing the sub-command, so if the agent stops in between, resume saw no sub-command and moved the command back to the builtin:software_list step, running a software list the command never asked for. Resume now only re-runs the builtin when every step leading to the awaiting state is builtin:software_list, and otherwise leaves the command as it was before this PR (test: command_awaiting_a_state_shared_by_a_builtin_and_a_sub_operation_is_resumed_unchanged). Several builtin:software_list steps sharing the state are still re-run: any of them sends the same request and waits in the same state, which another test checks.
On failing commands we can't resume: in that ambiguous case resume can't tell a lost sub-command from one that is about to arrive, as the sub-command init is published retained with QoS 1 and is only processed after resume. Failing it there would fail commands that would have completed. The place to guarantee a terminal state is the timeout_second/on_timeout of await-operation-completion, which the docs describe but the agent doesn't implement yet (TODO: Register a timeout event in the workflow actor). I'll raise that as a follow-up rather than grow this PR.
There was a problem hiding this comment.
[AI] Follow-up raised as #4397: await-operation-completion ignores timeout_second and on_timeout.
…start Signed-off-by: James Rhodes <jarhodes314@gmail.com>
…start Signed-off-by: James Rhodes <jarhodes314@gmail.com>
GitHub Actions
Waiting
Proposed changes
When tedge-agent exits while
software_listrequests are waiting in the software manager's in-memory queue (e.g. theRestartRequiredexit after a self-update), those requests are lost. The restarted agent resumes them atexecuting, anawait-operation-completionstep waiting for a builtin operation that will never answer, so they never complete. With #4361, the c8y mapper skips newsoftware_listrequests while one is pending, so Cumulocity is left with a stale software list.The software manager change is needed alongside the resume change: for the request the software manager was running when the agent stopped, its "cancelled due to unexpected agent restart" failure completed the command first, then the result of the re-sent request was published on top of a failed (possibly already cleared) command.
Tests:
tests/tedge_agent/software_list_resume(no cloud credentials needed): a test plugin makeslistslow, onesoftware_listrequest is being run by the software manager while a second is queued, thentedge-agentis restarted. Both must reachsuccessfuland none may be published asfailed. On main, both requests stay atexecuting. With only the resume change,robot-runningis published asfailedand then with itscurrentSoftwareList. With this PR, it passes.software_list_interrupted_by_an_agent_restart_is_run_again_on_resume: the workflow actor is aborted after sending the request and respawned on the same state dir; the request must reach the software manager again. Fails on main.software_list_interrupted_by_a_restart_is_not_reported_as_failed: the first message from the software manager on startup must beSoftwareCommandMetadata, not a failedSoftwareListCommand. Fails on main.software_list_of_a_legacy_builtin_workflow_interrupted_by_an_agent_restart_is_run_again_on_resume: guards that legacyaction = "builtin"workflows are still re-run once the failure is dropped.Types of changes
Paste Link to the issue
Needed by #4361, whose
Tedge Self Updatetests fail because of this bug.Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments
Merge Danger
Door: two-way
No persisted format changes; reverting restores the previous behaviour.
Blast Radius: software_list
A
software_listinterrupted by an agent restart is now re-run on every restart rather than failed once, so during an agent crash loop such a request shows as executing until the loop ends, rather than failed.Limits
Only
software_listis re-run, since re-running asoftware_updateis not safe. Custom workflows usingbuiltin:software_listare covered too, as the trigger step is looked up in the workflow. Workflows using the legacyaction = "builtin"form were already re-run on resume, as their command board stays atscheduled. Child devices running an older agent still have the bug.Alternative considered
A mapper-side alternative (have the c8y mapper clear pending
software_listrequests when the agent restarts) was rejected: it clears commands that haven't reached a terminal state, which breaks the command contract, and the agent still publishes the late result of a cleared command, bringing the cleared topic back.🤖 Generated with Claude Code