Merge pir stabilization feature branch into main #2789

THISISDINOSAUR · 2024-05-16T17:09:31Z

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:

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.
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
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.
With profile data already existing but initial scans having finished, close the app and reopen it. Scheduled scans should run.
With profile data already existing, and with the app closed, launch the background agent, scheduled scans should run
Edit/add profile data, put the laptop to sleep/lock it/restart it, check that scans continue as expected.
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
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Task/Issue URL:https://app.asana.com/0/1199230911884351/1207175016813446/f **Description**:This PR includes all changes relating to [this tech design sub-task](https://app.asana.com/0/0/1207199754528648/f). Specifically:

Task/Issue URL: https://app.asana.com/0/0/1207231867963535/f Tech Design URL: https://app.asana.com/0/0/1207198317317333/f CC: **Description**: 1. Moves the existing XPC interface ("DataBrokerProtectionScheduler") from the scheduler into the BackgroundAgentManager 2. Renames said manager to AgentManager 3. Renames the XPC interface to DataBrokerProtectionAgentInterface 4. Completely changes the composition of the interface, separating it out into two different protocols: DataBrokerProtectionAgentAppEvents, DataBrokerProtectionAgentDebugCommands 5. As those protocols imply, changes the XPC interface methods to be events based, to move decision making from the main app to the agent. Except for the debug commands, which are now clearly separated as for debugging purposes only. 6. Removes two of the many XPC layers since they were operating solely as a passthrough (i.e. boilerplate for nothing) 7. Uses protocol composition to further cut down on the repetition in those layers 8. Changes LoginItemScheduler to rather than be a class, a protocol that inherits the AgentInterface 9. Removes existing completions from the XPC interface since they were unused 10. Replaces them with new completions that are solely for reporting the reliability of XPC It still needs the pixels for #9, including clean up of the old pixels **Steps to test this PR**: 1. Test DBP still works, particularly removing the login item, entirely and starting fresh (since when testing at one point I managed to break that) 2. Otherwise don't test too much since this PR is not intended to stand alone/be merged into main as is.  --- ###### 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)

Task/Issue URL: https://app.asana.com/0/0/1207231867963533/f Tech Design URL: https://app.asana.com/0/0/1207199754528649/f **Description**: This PR introduces `DataBrokerProtectionQueueManager` to manage state and logic relating to the `OperationQueue` which runs DBP operations.

Task/Issue URL: https://app.asana.com/0/1199230911884351/1207175016813449/f Tech Design URL: CC: **Description**: 1. Move all userNotification related calls and anything else unrelated to scheduling up a layer to the agent manager 2. Removes the existing scheduler, and moves the ActivityScheduler management to a new class 3. 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 4. Fixes us not deleting the login item when the user deleted their data 5. As part of that, removes the dataDeleted IPC method, since it's no longer needed 5. Reworks the agent manager and associated dependancies so it can be unit tested 6. 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**: 1. 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](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 <[email protected]>

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerJobRunner.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerOperation.swift

THISISDINOSAUR · 2024-05-17T11:08:54Z

@jotaemepereira can you pay particular attention to if the sleep stuff is still as expected? The merge was tricky because of the file renames, git wanted to do some very incorrect things

github-actions · 2024-05-17T11:09:50Z

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

Generated by 🚫 dangerJS against f645b59

aataraxiaa

Overall looks good. Nothing stands out after main merge. Most scenarios work as expected. Still figuring out best way to test some pixels and scheduled scans, but others might have ideas on how to best verify these. Specifically:

Whats the best way to test a background scan is started when we launch the app? I tried attaching to the agent process via Xcode, then launching the app, but this results in a SIGTERM.
How can we test agent pixels are fired in the above scanario? If we can’t attach before launch, they won’t appear in Xcode’s console.

Either way I’ll continue to look into testing, but wanted to add my initial feedback now.

aataraxiaa · 2024-05-17T11:31:01Z

...kages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerOperation.swift

-                await onError(error: DataBrokerProtectionError.emailError(error as? EmailError))
-            }
+// swiftlint:disable explicit_non_final_class
+class DataBrokerOperation: Operation {


Nothing here stands out as incorrect after the main merge @THISISDINOSAUR 👍

aataraxiaa · 2024-05-17T11:49:39Z

LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Operations/DataBrokerJob.swift

+import UserScript
+import Common
+
+protocol DataBrokerJob: CCFCommunicationDelegate {


Nothing stands out here as incorrect, but I think (?) @jotaemepereira made changes here which were merged from main so would be good for him to have a look.

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

aataraxiaa · 2024-05-17T16:36:39Z

Overall looks good. Nothing stands out after main merge. Most scenarios work as expected. Still figuring out best way to test some pixels and scheduled scans, but others might have ideas on how to best verify these. Specifically:

Whats the best way to test a background scan is started when we launch the app? I tried attaching to the agent process via Xcode, then launching the app, but this results in a SIGTERM.

How can we test agent pixels are fired in the above scanario? If we can’t attach before launch, they won’t appear in Xcode’s console.

Either way I’ll continue to look into testing, but wanted to add my initial feedback now.

I was able to confirm background scans ran after app launch using a combination of the Debug DB viewer and Activity Monitor. On launch opt-outs from previous scan were run. 👍🏼

# Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGoDBPBackgroundAgent/DataBrokerProtectionBackgroundManager.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/DebugUI/DataBrokerRunCustomJSONViewModel.swift # LocalPackages/DataBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionScheduler.swift # LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/Mocks.swift

aataraxiaa · 2024-05-17T16:40:43Z

Tested quite a bit today with various scans and all looks good. Only thing I wasn’t sure how to verify was the interrupted pixel fire, but maybe another reviewer here could verify that. 🚀

THISISDINOSAUR · 2024-05-17T16:41:35Z

@jotaemepereira can you pay particular attention to if the sleep stuff is still as expected? The merge was tricky because of the file renames, git wanted to do some very incorrect things

@Bunn same thing re the subscription stuff

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

aataraxiaa

LGTM, nice find @Bunn 🚀

Bunn

LGTM

jotaemepereira

@THISISDINOSAUR While testing manual scans, I realized the flag that we pass to the StageCalculator is now wrong, which causes some of the pixels to not be triggered when they should be.

THISISDINOSAUR · 2024-05-20T18:03:29Z

@THISISDINOSAUR While testing manual scans, I realized the flag that we pass to the StageCalculator is now wrong, which causes some of the pixels to not be triggered when they should be.

Whereabouts? Is it the isImmediateOperation flag?

jotaemepereira

I tried again, and it is working. I do not know what happened yesterday and why the flag was false, though 🤷🏼‍♂️

# By Elle Sullivan (8) and others # Via Elle Sullivan (3) and GitHub (2) * main: (26 commits) Autofill engagement KPIs for pixel reporting (#2806) autofill: don't prefix autofill email pixels with `m.mac.` (#2808) Bump BSK (#2807) Make profile selector optional (#2811) Add History to iOS (updated UI and rollout) (#2770) New autofill save & update password prompt pixels for alignment with iOS (#2801) Add mylife data broker (#2786) Increase test timeout (#2810) Subscription refactoring, BSK update (#2809) Check for entitlement in DBP agent (#2802) move permanent survey card to first position (#2804) Subscription refactoring (#2764) Bump version to 1.89.0 (191) Merge pir stabilization feature branch into main (#2789) Update age params for multiple brokers (#2800) Fix top level navigation blocks (#2792) Adding to the Dock automatically (#2722) Fix lint error Make activity only call completion once all work has actually finished Fix activity scheduler not calling completion ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

aataraxiaa and others added 4 commits May 8, 2024 16:59

THISISDINOSAUR requested review from Bunn, jotaemepereira and aataraxiaa May 16, 2024 17:18

aataraxiaa approved these changes May 17, 2024

View reviewed changes

THISISDINOSAUR added 2 commits May 17, 2024 17:40

Fix swiftlint warning

f4269b5

Fix new swift lint warning

b8c4e4f

Bunn reviewed May 20, 2024

View reviewed changes

...Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionBackgroundActivityScheduler.swift Outdated Show resolved Hide resolved

Fix activity scheduler not calling completion

03978c2

Bunn reviewed May 20, 2024

View reviewed changes

...Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionBackgroundActivityScheduler.swift Outdated Show resolved Hide resolved

Make activity only call completion once all work has actually finished

4c801c7

aataraxiaa approved these changes May 20, 2024

View reviewed changes

Fix lint error

f645b59

Bunn approved these changes May 20, 2024

View reviewed changes

jotaemepereira requested changes May 20, 2024

View reviewed changes

jotaemepereira approved these changes May 21, 2024

View reviewed changes

THISISDINOSAUR merged commit 8dd682c into main May 22, 2024
22 checks passed

THISISDINOSAUR deleted the feature/pir-stabilization branch May 22, 2024 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge pir stabilization feature branch into main #2789

Merge pir stabilization feature branch into main #2789

THISISDINOSAUR commented May 16, 2024 •

edited

Loading

THISISDINOSAUR commented May 17, 2024

github-actions bot commented May 17, 2024 •

edited

Loading

aataraxiaa left a comment •

edited

Loading

aataraxiaa May 17, 2024

aataraxiaa May 17, 2024

aataraxiaa commented May 17, 2024

aataraxiaa commented May 17, 2024

THISISDINOSAUR commented May 17, 2024

aataraxiaa left a comment

Bunn left a comment

jotaemepereira left a comment

THISISDINOSAUR commented May 20, 2024 •

edited

Loading

jotaemepereira left a comment

Merge pir stabilization feature branch into main #2789

Merge pir stabilization feature branch into main #2789

Conversation

THISISDINOSAUR commented May 16, 2024 • edited Loading

Internal references:

THISISDINOSAUR commented May 17, 2024

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

aataraxiaa left a comment • edited Loading

Choose a reason for hiding this comment

aataraxiaa May 17, 2024

Choose a reason for hiding this comment

aataraxiaa May 17, 2024

Choose a reason for hiding this comment

aataraxiaa commented May 17, 2024

aataraxiaa commented May 17, 2024

THISISDINOSAUR commented May 17, 2024

aataraxiaa left a comment

Choose a reason for hiding this comment

Bunn left a comment

Choose a reason for hiding this comment

jotaemepereira left a comment

Choose a reason for hiding this comment

THISISDINOSAUR commented May 20, 2024 • edited Loading

jotaemepereira left a comment

Choose a reason for hiding this comment

THISISDINOSAUR commented May 16, 2024 •

edited

Loading

github-actions bot commented May 17, 2024 •

edited

Loading

aataraxiaa left a comment •

edited

Loading

THISISDINOSAUR commented May 20, 2024 •

edited

Loading