Conversation
Use state information in connection task handlers, instead of the in-memory repository. The information returned by the interface repository may not correctly represent the actual connection state, if the task handlers were to be retried or exit early. The changes from this patch are a prerequisite for supporting retries of task handlers in scenarios where an operation on a snap becomes temporarily impossible to execute, e.g. due to snap locks being held by other processes. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A disabled connection peer can have removed security profiles unintentionally recreated.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Updates disconnect-related handlers to use durable connection state, improving retry safety.
Changes:
- Derives affected snaps from persisted connections.
- Tolerates already-disconnected repository entries during retries.
- Adds retry regression tests and clarifies repository/state documentation.
| File | Description |
|---|---|
overlord/ifacestate/handlers.go |
Makes disconnect and undo handling retry-tolerant. |
overlord/ifacestate/ifacestate_test.go |
Adds retry and stale-connection tests. |
overlord/ifacestate/ifacemgr.go |
Documents repository ephemerality. |
overlord/ifacestate/helpers.go |
Clarifies persisted connection states. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…t handlers fixup! overlord/ifacestate: use state as source of truth in disconnect handlers Fix no-op repo.Plug assertions in TestDoRemoveAlreadyDisconnectedInRepo and TestDoRemoveIgnoresStaleUndesiredConn: consumer's plug is named "plug", not "slot", so the lookup was nil regardless of whether the snap had actually been removed from the repository. Caught by Copilot PR review on canonical#17692.
…t handlers fixup! overlord/ifacestate: use state as source of truth in disconnect handlers Assert security is actually re-applied in TestUndoConnectAlreadyDisconnectedInRepo, matching the sibling TestUndoConnect. Previously only conns cleanup and task status were checked, so an implementation that returned early from the tolerated NotConnectedError branch (skipping the re-apply-security step) would still have passed. Caught by Copilot PR review on canonical#17692.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17692 +/- ##
==========================================
+ Coverage 78.36% 79.11% +0.74%
==========================================
Files 1415 1422 +7
Lines 200459 207768 +7309
Branches 2503 2503
==========================================
+ Hits 157084 164369 +7285
- Misses 33950 33970 +20
- Partials 9425 9429 +4
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:
|
| task.Errorf("skipping security profiles setup for snap %q when handling snap %q: %v", affectedInstanceName, affectingSnap, err) | ||
| continue | ||
| } | ||
| if !snapst.Active { |
Member
There was a problem hiding this comment.
This might miss some cases where snapst.PendingSecurity is non-nil. Possibly something like:
if !snapst.Active &&
(snapst.PendingSecurity == nil || snapst.PendingSecurity.SideInfo == nil) {
continue
}
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Inactive peers can have security profiles regenerated from the wrong revision instead of their pending-security state.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
|
Wed Sep 23 06:05:07 UTC 2026 Test Predictor AnalysisPreparing
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)
|
…t handlers fixup! overlord/ifacestate: use state as source of truth in disconnect handlers Drop TestDoRemoveStillUpdatesMidRefreshPeer: it was a strict subset of TestDoRemoveMidRefreshPeerUsesPendingRevision (both assert the peer is not skipped; only the latter also asserts the correct pending revision is used, and with Current == PendingSecurity.SideInfo it could not have caught a wrong-revision regression anyway). TestDoRemoveMidRefreshPeerUsesPendingRevision now establishes PendingSecurity via the real OnSnapLinkageChanged hook first (matching production wiring) before pinning it forward to a distinct revision, so it covers both properties in one, higher-fidelity test.
Contributor
Author
|
The PR had a bit of scope creep during as reviews as preexisting issues with components and security profiles for mid-refresh snaps. All of this has been addressed. |
This branch has not been deployed
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.
Use state information in connection task handlers, instead of the in-memory repository. The information returned by the interface repository may not correctly represent the actual connection state, if the task handlers were to be retried or exit early.
The changes from this patch are a prerequisite for supporting retries of task handlers in scenarios where an operation on a snap becomes temporarily impossible to execute, e.g. due to snap locks being held by other processes.
Cherry picked from #17587
Related: SNAPDENG-37471