o/devicemgmtstate: add task to queue response messages - #17254
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for the queue-mgmt-response task in overlord/devicemgmtstate, completing the message-processing pipeline by generating signed response-message assertions and queueing them for transmission on the next Store message exchange.
Changes:
- Implemented
queue-mgmt-responsetask logic to build response bodies (from completed subsystem changes or pre-set rejection/authorization outcomes), sign them, and store them inReadyResponses. - Added state helper to remove processed request messages from a sequence while preserving sequence progress.
- Expanded unit tests to cover successful response queuing, idempotency, retry behavior while subsystem changes are incomplete, and signing/error scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| overlord/devicemgmtstate/devicemgmtmgr.go | Implements response queuing/signing, adds request-message removal helper, and wires the new task into dispatch chains. |
| overlord/devicemgmtstate/devicemgmtmgr_test.go | Adds coverage for queue-mgmt-response and updates existing tests to account for the new task in message chains. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17254 +/- ##
==========================================
- Coverage 78.86% 78.85% -0.02%
==========================================
Files 1401 1401
Lines 195954 196081 +127
Branches 2466 2466
==========================================
+ Hits 154543 154620 +77
- Misses 32138 32174 +36
- Partials 9273 9287 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| }, nil | ||
| }) | ||
|
|
||
| s.runner.AddHandler("queue-mgmt-response", func(*state.Task, *tomb.Tomb) error { return nil }, nil) |
There was a problem hiding this comment.
The no-op s.runner.AddHandler("queue-mgmt-response", ...) overrides in the pre-existing tests are there because those tests assert on intermediate state (that a message was dispatched, validated, or applied correctly) before doQueueResponse runs and removes it from the sequence. Now that doQueueResponse is implemented and registered by the manager, .settle runs it to completion, which removes the RequestMessage from state and breaks the assertions on ms.Sequences[...].Messages.
The no-op handler suppresses the real task keeping tests focused & their assertions meaningful.
There was a problem hiding this comment.
Just pinging @pedronis to bring it to his attention. I think this makes sense, it enables testing intermediate state and those tests still run most of the change, just not the response/removal bit.
420d0ee to
21896f3
Compare
|
Wed Jul 1 13:53:01 UTC 2026 No spread failures reported |
4ffb3fe to
b66865c
Compare
b66865c to
d7e0cf0
Compare
|
Mon Jul 13 17:54:51 UTC 2026 Failures:Preparing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list of the below tests you wish to run (unskip plus test list must be valid yaml)
|
miguelpires
left a comment
miguelpires
left a comment
There was a problem hiding this comment.
qq about some error handling code paths
| s.st.Unlock() | ||
| err := s.mgr.DoQueueResponse(t, &tomb.Tomb{}) | ||
| s.st.Lock() | ||
| c.Assert(err, ErrorMatches, `cannot find subsystem change "16384"`) |
There was a problem hiding this comment.
Just to clarify, if this happens we're basically stuck right? This sequence will just stick around until it gets pushed out of the state due to a global limit
There was a problem hiding this comment.
@miguelpires, yes. I have a TODO in doQueueResponse to handle this after this PR & maybe #17273 land:
// TODO: rejecting sequences currently happens in 2 ways:
// 1. doDispatchMessage can evict the sequence immediately if it's rejected early.
// 2. If it errors elsewhere (in validate, apply, or queue-response), we end
// up not advancing Applied, which means we accumulate messages until we
// hit the sequence cap.
// Refactor sequence rejection to always evict immediately and determine the
// right behavior for internal errors.
I can do it in this PR but I think it makes sense to land this first because I'll have to refactor the dispatching code that already does rejection and make sure all rejection/error paths in all tasks/handlers reject the sequences & messages immediately instead of waiting for the cap.
There was a problem hiding this comment.
Yes, definitely lets land this first. We can deal with that in its own PR. I was just thinking about the devicemgmtmgr's behaviour on error and wanted to check my understanding. Thanks
| queueTask := ti.queue["mesg-1"] | ||
| c.Assert(queueTask, NotNil) | ||
| c.Check(queueTask.Status(), Equals, state.ErrorStatus) | ||
| c.Check(strings.Join(queueTask.Log(), "\n"), testutil.Contains, "cannot sign response message: device key not found") |
There was a problem hiding this comment.
Same with this; if we get this error, the change fails and the sequence is stuck?
| // up not advancing Applied, which means we accumulate messages until we | ||
| // hit the sequence cap. | ||
| // Refactor sequence rejection to always evict immediately and determine the | ||
| // right behavior for internal errors. |
There was a problem hiding this comment.
we definitely need to see what to do for internal errors?
There was a problem hiding this comment.
also the comment makes it sound like it applies only to sequences, but reasonable behavior for internal error is also important for not-sequenced messages
There was a problem hiding this comment.
we definitely need to see what to do for internal errors?
Yes. Since tasks are idempotent, a failed message will not be re-dispatched or re-applied on the next change, but the request message remains in state until doQueueResponse completes, so such failures leave it hanging indefinitely.
also the comment makes it sound like it applies only to sequences, but reasonable behavior for internal error is also important for not-sequenced messages
I've fixed it to be clearer.
|
|
||
| change := m.state.Change(msg.ApplyChangeID) | ||
| if change == nil { | ||
| return fmt.Errorf("cannot find subsystem change %q", msg.ApplyChangeID) |
There was a problem hiding this comment.
this seems also an internal error, in which scenario would this happen?
There was a problem hiding this comment.
I've marked it as internal error: .... The branch is a defensive guard in the rare case the change is not in state anymore. For instance, if a ready subsystem change is pruned before this task fires. I'm looking at the pruning code to see if this is even possible.. say if the computer is shut down mid-device-management-change and then booted after a week.
| s.st.Unlock() | ||
| err := s.mgr.DoQueueResponse(t, &tomb.Tomb{}) | ||
| s.st.Lock() | ||
| c.Assert(err, FitsTypeOf, &state.Retry{}) |
There was a problem hiding this comment.
would be probably be good to then set the change to Done and check run again in this one test
ae71431 to
cff4064
Compare
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
thanks, a local LLM review made this comment, seems more material possibly for a follow up?
Missing test coverage for completed/replayed messages after removal. The branch now removes processed requests from Sequences, but duplicate coverage only exercises duplicates while the request is still pending: devicemgmtmgr_test.go:544-567. I’d add tests for a duplicate sequenced message with Applied already past it, and for an unsequenced duplicate after its response was queued/sent. If the store contract guarantees those cannot happen after LastReceivedToken, the test can document that assumption at the exchange boundary.
st3v3nmw
added
the
Auto rerun spread
I'll follow up with some tests to improve the coverage. Thanks! |
|
Spread failures are unrelated to these changes, Copilot summary: https://github.com/canonical/snapd/actions/runs/29240595071/job/86889028970: The job ran spread tests on https://github.com/canonical/snapd/actions/runs/29240595071/job/86889028802: The failing job runs the spread test |
* o/devicemgmtstate: add task to queue response messages * o/devicemgmtstate: test that a task chain error does not affect other messages * o/devicemgmtstate: set error message for unknown message kind in setMessageResponseFromChange * o/devicemgmtstate: do not advance Applied on non-success response * o/devicemgmtstate: add reuseable noopTask in tests * o/devicemgmtstate: improve error messages * o/devicemgmtstate: fixes after review
This PR implements the
queue-mgmt-responsetask. Once a message has been applied (or rejected before reaching the subsystem), this task builds a signed response-message and queues it for transmission on the next store exchange.For messages with a subsystem change, the task retries until the change completes, then calls the handler's
ResultFromChangeto produce the response body. For messages that were rejected or not authorized before dispatch, the response is built directly fromResponseStatus&ResponseBodyin theRequestMessage.Split from #16248, replaces #16656.