DBP: Restructure AgentManager and scheduler and add tests #2777

THISISDINOSAUR · 2024-05-14T12:30:32Z

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207175016813449/f
Tech Design URL:
CC:

Description:

Move all userNotification related calls and anything else unrelated to scheduling up a layer to the agent manager
Removes the existing scheduler, and moves the ActivityScheduler management to a new class
Make a variety of changes to scheduler behavior, such as running all the time (as long as theirs profile data), changing scheduler frequency to 20 minutes, removes status publishing
Fixes us not deleting the login item when the user deleted their data
As part of that, removes the dataDeleted IPC method, since it's no longer needed
Reworks the agent manager and associated dependancies so it can be unit tested
Adds those unit tests.

Pixels are still outstanding
But everything else should be done now (integrating with Pete's latest PR etc)

Steps to test this PR:

Generally test that DBP works, but not too thoroughly since we will do that for the whole feature branch at once.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…rokerProtectionQueueMode`. Add `DataBrokerDebugCommand` and associated protocol. Add initial set of tests.

…/cancellation, add tests & mocks

…cationService

github-actions · 2024-05-14T12:32:30Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 7a753dc

THISISDINOSAUR · 2024-05-14T12:32:51Z

DuckDuckGo/DBP/DataBrokerProtectionLoginItemInterface.swift

@@ -63,12 +68,6 @@ extension DefaultDataBrokerProtectionLoginItemInterface: DataBrokerProtectionLog
        }


This is the pixels that are pending

THISISDINOSAUR · 2024-05-14T12:33:27Z

...BrokerProtection/Sources/DataBrokerProtection/DebugUI/DataBrokerRunCustomJSONViewModel.swift

-/*
- I wasn't able to import this mock from the background agent project, so I had to re-use it here.
- */
-private final class PrivacyConfigurationManagingMock: PrivacyConfigurationManaging {


Since various things moved to the DBP package, we can remove this :D

THISISDINOSAUR · 2024-05-14T12:45:52Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

+
+    public func runQueuedOperations(showWebView: Bool) {
+        // TODO
+        //scheduler.runQueuedOperations(showWebView: showWebView)


pending integration with outstanding PR

aataraxiaa

Nice one, looks great overall @THISISDINOSAUR. Left a few very minor suggestions and a question.

aataraxiaa · 2024-05-14T13:51:32Z

DuckDuckGo/DBP/DataBrokerProtectionLoginItemInterface.swift

-    func disableLoginItem()
-    func enableLoginItem()
+protocol DataBrokerProtectionLoginItemInterface: DataBrokerProtectionAppToAgentInterface {
+    func dataDeleted()


aataraxiaa · 2024-05-14T13:58:02Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

+import BrowserServicesKit
+import PixelKit
+
+// This is to avoid exposing all the dependancies outside of the DBP package


Nice solution 👍🏼

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

...Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionBackgroundActivityScheduler.swift

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

…nd-scheduler # Conflicts: # DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionAgentManager.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerOperation.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionScheduler.swift # LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionQueueManagerTests.swift # LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionQueueModeTests.swift # LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/Mocks.swift

THISISDINOSAUR · 2024-05-14T18:09:39Z

@aataraxiaa btw I've updated to integrate with your latest, and addressed all comments except for this one: #2777 (comment) (waiting to see what Juan says)
So that leaves only pixels outstanding

aataraxiaa

Latest changes look good @THISISDINOSAUR. Added a comment about strange behaviour I saw this morning when testing.

aataraxiaa · 2024-05-15T08:25:56Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionAgentManager.swift

+
+extension DataBrokerProtectionAgentManager: DataBrokerProtectionAgentAppEvents {
+
+    public func profileSaved() {


Was just testing this again this morning, and something strange was happening in that this method was never called on a profile save action. I stepped through the chain of calls and it looked like the profileSaved method of the login item was being called twice. I removed all background agents that were running, and made sure I was attached to the background process, but that didn’t solve it. Didn’t look further as I must be doing something strange. Maybe you could give it a try and let me know.

jotaemepereira

@THISISDINOSAUR I’m seeing some issues when scanning. If you follow the steps you will see it:

Start a scan
Edit the profile while the scan is running (add a new name, for example)
Something is happening there where the profileSaved in the DataBrokerProtectionAgentManager is not being called, causing the new scans to never start.

It leaves the app in a weird state, because the scans were cancelled for some reason, but they never started again.

THISISDINOSAUR · 2024-05-15T13:56:38Z

@THISISDINOSAUR I’m seeing some issues when scanning. If you follow the steps you will see it:

Start a scan

Edit the profile while the scan is running (add a new name, for example)

Something is happening there where the profileSaved in the DataBrokerProtectionAgentManager is not being called, causing the new scans to never start.

It leaves the app in a weird state, because the scans were cancelled for some reason, but they never started again.

I think this is the same issue Pete ran into and was introduced in the merge, I'm not sure what's going in yet

THISISDINOSAUR · 2024-05-15T15:37:44Z

I figured it out, it actually happened when I added the tests, AgentManager wasn't being persisted in memory 🙄
I've also now added the pixels.
I've also added delay before each xpc call to make sure the login item had chance to start. I'm unsure if this is necessary, it depends on if the API is blocking until the login item is launched

aataraxiaa

Recent changes LGTM!

jotaemepereira

Worked for me 🎉

Bunn

LGTM. Tested these scenarios:

Scan from scratch
Updating user profile after scan ended
Updating user profile mid scan

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207338264132623/f Tech Design URL: https://app.asana.com/0/481882893211075/1207174884557414/f CC: **Description**: This PR has the effect of mergeing four different PRs into main: #2777 #2758 #2757 #2743 This covers significant changes to the XPC interface and how the main app uses it, the background manager, the scheduler, and the processor. See individual PRs for details. There's no code changes here that haven't been reviewed separately as part of those PRs, so it's up to you how you want to review it code wise. The more important step at this stage is manual testing. **Steps to test this PR**: 1. Because this is a fairly significant set of changes, we need to be as thorough as we can in testing DBP generally works and really try to break it. Because of this, I'm keen that anyone testing things of their own ways to break it before trying the steps below. 2. Edit/add profile data, and before scans finish, edit it again. Scans should start afresh, and you should see an interrupted pixel fire. Check that you don't get a scans completed notification 3. Edit/add profile data, and before scans finish, close the app, and reopen it. The same set of manual scans should still continue, and a blocked pixel should fire. 4. With profile data already existing but initial scans having finished, close the app and reopen it. Scheduled scans should run. 5. With profile data already existing, and with the app closed, launch the background agent, scheduled scans should run 6. Edit/add profile data, put the laptop to sleep/lock it/restart it, check that scans continue as expected. 7. Check that the background agent activity lines up with the UI, I.e. the progress bar and when we sent the scans completed notification is accurate  --- ###### Internal references: [Pull Request Review Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f) [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943) [Pull Request Documentation](https://app.asana.com/0/1202500774821704/1204012835277482/f) --------- Co-authored-by: Pete Smith <[email protected]>

aataraxiaa and others added 24 commits May 10, 2024 11:04

Rebase merge

8457fae

Rebase merge

7f6234a

Rebase merge

d4386f4

Rebase merge

bd76677

Refactor DataBrokerOperation dependency definition, usage

4c555b1

Update Builder test post-refactor

926b53e

Rebase merge

2170a1d

Initial implementation of DataBrokerProtectionQueueManager & `DataB…

f3468a6

…rokerProtectionQueueMode`. Add `DataBrokerDebugCommand` and associated protocol. Add initial set of tests.

Post-rebase manual fixes

35aef1c

Rebase merge: Scheduler

699ff01

Add tests & mocks

13c1ae1

Remove debug command enum, protocol, and usage

6f19c1e

Correct log message

76dabb9

Post-rebase updates/fixes

6c4a23b

PR Feedback Part 1

5de0f3c

PR Feedback: Fire pixels in queue manager, clear errors on completion…

ec93899

…/cancellation, add tests & mocks

PR Feedback: Rename provide method

74b6cc2

PR Feedback: Avoid side-effects, rename errorCollection, and add test

a1283af

Swiftlint

a995cb3

Merge agentManager and scheduler

f7909d6

Refactor AgentManager to enable testability

e72e05b

Make IPC server testable

be40496

Add mocks for tests and rename MockUserNotification to MockUserNotifi…

e227ed6

…cationService

Add AgentManager tests

b19d9e3

THISISDINOSAUR changed the base branch from feature/pir-stabilization to pete/queue-manager-and-state May 14, 2024 12:31

THISISDINOSAUR commented May 14, 2024

View reviewed changes

Update agent manager log statements

0698622

THISISDINOSAUR commented May 14, 2024

View reviewed changes

THISISDINOSAUR requested a review from aataraxiaa May 14, 2024 12:48

aataraxiaa approved these changes May 14, 2024

View reviewed changes

Base automatically changed from pete/queue-manager-and-state to feature/pir-stabilization May 14, 2024 14:22

THISISDINOSAUR added 6 commits May 14, 2024 17:12

Integrate new and renamed debug commands

99ea6aa

Add debug running status string back

901bf4f

Update references to manual and restore debug run state

2b5e863

Fix another reference to manual

e2f29a5

Move scheduler config into config struct

1bb4a1f

jotaemepereira self-requested a review May 14, 2024 19:00

aataraxiaa reviewed May 15, 2024

View reviewed changes

jotaemepereira requested changes May 15, 2024

View reviewed changes

THISISDINOSAUR added 5 commits May 15, 2024 15:41

Fix agent manager not being persisted in memory

eddab4b

Fix not calling app launched

a00aeea

Add interrupted error to queueManager

028d561

Add delay to profile saved call to make sure agent has started

24a43ae

Add pixels

c1c2ebd

THISISDINOSAUR added 2 commits May 15, 2024 17:02

Fix interrupt error

9b5940a

Add interrupt to unit tests

7a753dc

aataraxiaa approved these changes May 15, 2024

View reviewed changes

jotaemepereira self-requested a review May 15, 2024 18:46

jotaemepereira approved these changes May 15, 2024

View reviewed changes

Bunn approved these changes May 15, 2024

View reviewed changes

THISISDINOSAUR merged commit 1ddb5a4 into feature/pir-stabilization May 15, 2024
22 of 23 checks passed

THISISDINOSAUR deleted the elle/dbp-agentmanager-and-scheduler branch May 15, 2024 21:04

THISISDINOSAUR mentioned this pull request May 16, 2024

Merge pir stabilization feature branch into main #2789

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBP: Restructure AgentManager and scheduler and add tests #2777

DBP: Restructure AgentManager and scheduler and add tests #2777

THISISDINOSAUR commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

THISISDINOSAUR May 14, 2024

THISISDINOSAUR May 14, 2024

THISISDINOSAUR May 14, 2024

aataraxiaa left a comment

aataraxiaa May 14, 2024

aataraxiaa May 14, 2024

THISISDINOSAUR commented May 14, 2024

aataraxiaa left a comment

aataraxiaa May 15, 2024 •

edited

Loading

jotaemepereira left a comment

THISISDINOSAUR commented May 15, 2024

THISISDINOSAUR commented May 15, 2024

aataraxiaa left a comment

jotaemepereira left a comment

Bunn left a comment

		@@ -63,12 +68,6 @@ extension DefaultDataBrokerProtectionLoginItemInterface: DataBrokerProtectionLog
		}


		extension DataBrokerProtectionAgentManager: DataBrokerProtectionAgentAppEvents {

		public func profileSaved() {

DBP: Restructure AgentManager and scheduler and add tests #2777

DBP: Restructure AgentManager and scheduler and add tests #2777

Conversation

THISISDINOSAUR commented May 14, 2024 • edited Loading

Internal references:

github-actions bot commented May 14, 2024 • edited Loading

THISISDINOSAUR May 14, 2024

Choose a reason for hiding this comment

THISISDINOSAUR May 14, 2024

Choose a reason for hiding this comment

THISISDINOSAUR May 14, 2024

Choose a reason for hiding this comment

aataraxiaa left a comment

Choose a reason for hiding this comment

aataraxiaa May 14, 2024

Choose a reason for hiding this comment

aataraxiaa May 14, 2024

Choose a reason for hiding this comment

THISISDINOSAUR commented May 14, 2024

aataraxiaa left a comment

Choose a reason for hiding this comment

aataraxiaa May 15, 2024 • edited Loading

Choose a reason for hiding this comment

jotaemepereira left a comment

Choose a reason for hiding this comment

THISISDINOSAUR commented May 15, 2024

THISISDINOSAUR commented May 15, 2024

aataraxiaa left a comment

Choose a reason for hiding this comment

jotaemepereira left a comment

Choose a reason for hiding this comment

Bunn left a comment

Choose a reason for hiding this comment

THISISDINOSAUR commented May 14, 2024 •

edited

Loading

github-actions bot commented May 14, 2024 •

edited

Loading

aataraxiaa May 15, 2024 •

edited

Loading