o/devicemgmtstate: remove sequence entry after unsequenced message is processed - #17371
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents unbounded growth of devicemgmtstate persisted state by deleting Sequences[baseID] entries once an unsequenced request-message has fully completed processing, since unsequenced messages don’t carry Applied progress that needs to be preserved.
Changes:
- Update
removeRequestMessageto delete theSequencesmap entry when removing the last unsequenced message for a base ID. - Add/adjust unit tests to cover both sequenced (preserve
Applied) and unsequenced (delete sequence entry) behaviors. - Minor test refactors/renames to clarify intent around sequenced vs unsequenced paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| overlord/devicemgmtstate/devicemgmtmgr.go | Deletes stale Sequences entries after unsequenced messages are processed. |
| overlord/devicemgmtstate/devicemgmtmgr_test.go | Adds coverage asserting sequences are preserved for sequenced messages and removed for unsequenced messages. |
| // Unsequenced messages have no Applied progress to carry forward. | ||
| if msg.SeqNum == 0 && len(seq.Messages) == 0 { | ||
| delete(ms.Sequences, msg.BaseID) | ||
| } |
There was a problem hiding this comment.
Good catch, but the suggestion doesn't make sense here. To make it concrete, say we have mesg-1 and mesg-2 already in flight. Both parse to BaseID = "mesg", so they share Sequences["mesg"] and the sequence "mesg" is in SequenceLRU.
Now mesg (unsequenced) arrives with the same BaseID. Since its SeqNum = 0, enqueueRequestMessages skips the LRU update, and that's fine since "mesg" is already in the LRU from the sequenced messages, & it'd mess with the sequence LRU order. When mesg completes and removeRequestMessage is called, we shouldn't drop SequenceLRU since the sequence is still in progress and we want to know the true LRU ordering.
That said, "you" are pointing at a real underlying issue: a base ID shared between sequenced and unsequenced messages leads to odd behavior, especially during dispatch. Tracking that here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17371 +/- ##
==========================================
+ Coverage 78.81% 78.88% +0.06%
==========================================
Files 1405 1397 -8
Lines 197076 196895 -181
Branches 2498 2498
==========================================
- Hits 155334 155325 -9
+ Misses 32379 32217 -162
+ Partials 9363 9353 -10
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:
|
|
Wed Jul 29 14:02:41 UTC 2026 Failures:Preparing:
Executing:
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)
|
st3v3nmw
added
the
Auto rerun spread
|
Spread test failures are unrelated to these changes. Copilot summary:
|
Once an unsequenced message completes the pipeline, its entry in
Sequenceswas being left behind.removeRequestMessagekept entries intact to preserveAppliedprogress for sequenced messages, but unsequenced messages carry no such progress. Over time, these stale entries accumulate in state.removeRequestMessagenow deletes the sequence entry when the processed message is unsequenced and no other messages remain under that base ID.