Implement Queue Manager for State & Interruption Management #2757

aataraxiaa · 2024-05-08T17:06:31Z

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. The mains changes includes are:

Add DataBrokerProtectionQueueManager and related tests, mocks
Add DataBrokerProtectionQueueMode and related tests, mocks
Deletes DataBrokerProtectionProcessor
Creates protocols for dependencies which are passed to the queue manager, to enable mocking and testing
Re-adds an error delegate to communicate from DataBrokerOperation to the queue manager. Uses this for all access to operations errors (the old processor had a delegate also, but also explicitly looped over errors stored in operations)
Updates DataBrokerOperationDependencies, adding the config
Removes runAllOperations and allow subsequent implementations of it

Note 1: As this PR replaces the DataBrokerProtectionProcessor type completely, it’s best to review the new DataBrokerProtectionQueueManager type while having the existing processor open in another window. This will allow for comparison of implementation, and help highlight what has explicitly changed

Note 2: To ensure the logic is correct re: queue state management, I believe it’s best to focus reviewing efforts on DataBrokerProtectionQueueManager and DataBrokerProtectionQueueMode, and their related tests. The tests describe what I understand to be the expected behavior.

Steps to test this PR:

Test 1

Run an immediate (i.e manual) initial scan
Check progress is made

Test 2

Run a second immediate scan, interrupting the first scan
Check progress is made

Test 3

Edit lines 106-107 of DataBrokerProtectionScheduler to cause the scheduler to start soon after a manual scan, i.e

static let interval: TimeInterval = 3 * 60 // 3 minutes
static let tolerance: TimeInterval = 1 // 1 minute

Run an immediate scan
Wait until a schedule scan attempts to start (breakpoint at DefaultDataBrokerProtectionQueueManager line 103)
Ensure the scheduled scan does not interrupt the immediate scan

<!—
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be marked with the DO NOT MERGE label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the Pending Product Review label.

If at any point it isn't actively being worked on/ready for review/otherwise moving forward (besides the above PR/PFR exception) strongly consider closing it (or not opening it in the first place). If you decide not to close it, make sure it's labelled to make it clear the PRs state and comment with more information.
—>

—

Internal references:

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

github-actions · 2024-05-08T17:08:27Z

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

Generated by 🚫 dangerJS against b3a6f1b

aataraxiaa · 2024-05-09T14:43:03Z

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

                                                                                shouldRunNextStep: { [weak self] in
                    guard let self = self else { return false }
                    return !self.isCancelled
                })

-                if let sleepInterval = brokerTimeInterval {


brokerTimeInterval was optional in this type, but was always passed in as a parameter. Optionality was unnecessary.

aataraxiaa · 2024-05-09T14:47:09Z

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

+         mismatchCalculator: MismatchCalculator,
+         brokerUpdater: DataBrokerProtectionBrokerUpdater?)
+
+    func startImmediateOperationsIfPermitted(showWebView: Bool,


The naming here IMO better reflects the difference between scan types - immediate or schedule (i.e both time based).

We've had some confusion about the language we use here in the past, so it would be good to align on it more broadly

Good point. I am happy to make broader renaming changes as part of this work (e.g there are a lot of places we reference …manualScan and runQueuedOperations). Proposal:

Old Naming

...manualScan (e.g startManualScan, isManualScan, etc.)

runQueuedOperations

New Naming Proposal 1

...immediateOperations (e.g startImmediateOperations, isImmediateOperation, etc.)

startScheduledOperations

New Naming Proposal 2

…manualOperations (e.g startManualOperations, isManualOperation, etc.)

startAutomaticOperations

My preference is for Proposal 1, but I’d be happy with either (or other suggestions). The goal is to correctly capture the intent, and also align the two method names to be relative to each other.

What do you think @THISISDINOSAUR @Bunn ?

How about #1 but with jobs instead of operations? I think we should make "job" the chosen word for the actual thing we care about, and "operation" an implementation detail.

I’m unsure if we should use Job here, as we recently renamed the old DataBrokerOperation to be DataBrokerJob (and then have ScanJob..etc.). We also have related types like WebJobRunner. However, the intended relationship between the top-level Operation and lower-level jobs is a bit unclear to me. @Bunn , do you have any thoughts on Operation and Job naming?

I'd prefer # 1 as it is. They are going to run the actual 'NS'Operation, so I think the naming makes sense. If it were startScheduledJobs, for example, it also makes sense to me. However, DataBrokerOperation has jobs, and what the manager does is prepare DataBrokerOperation to run the jobs. In my opinion, there's no need to call it jobs.

All that said, both would be semantically correct in my opinion, so I have no strong preference, but a slight preference for # 1.

Ok, cool, thanks all. I’ll go with proposal 1 as it is. I’ll rename in all places in this PR, but contain the renaming to one commit.

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

aataraxiaa · 2024-05-09T14:48:24Z

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

+        }
+
+        operationQueue.addBarrierBlock { [weak self] in
+            let errorCollection = self?.errorCollection()


We no longer capture the operations and loop over them to get errors, but rather only rely on delegation from the DataBrokerOperation

I'm slightly weary of getting the state management wrong there, but it's the only way to do it correctly really for the interrupting logic

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

THISISDINOSAUR

Looks great overall!

...erProtection/Sources/DataBrokerProtection/Operations/DataBrokerProtectionBrokerUpdater.swift

...rces/DataBrokerProtection/Operations/ParentChildRelationship/MismatchCalculatorUseCase.swift

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

THISISDINOSAUR · 2024-05-10T17:33:40Z

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

+         mismatchCalculator: MismatchCalculator,
+         brokerUpdater: DataBrokerProtectionBrokerUpdater?)
+
+    func startImmediateOperationsIfPermitted(showWebView: Bool,


We've had some confusion about the language we use here in the past, so it would be good to align on it more broadly

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

THISISDINOSAUR · 2024-05-10T17:49:50Z

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

+        }
+
+        operationQueue.addBarrierBlock { [weak self] in
+            let errorCollection = self?.errorCollection()


I'm slightly weary of getting the state management wrong there, but it's the only way to do it correctly really for the interrupting logic

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

...BrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionQueueManagerTests.swift

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

…/cancellation, add tests & mocks

aataraxiaa · 2024-05-13T13:57:04Z

Most feedback addressed @THISISDINOSAUR. Only remaining feedback is re: naming of Operation/Job, and I’ll wait for more input from @Bunn here.

Bunn · 2024-05-13T14:39:46Z

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

+
+        firePixels(operationDependencies: operationDependencies)
+
+        addOperations(withType: type,


The operations are always added without a priority date. This means that every time the scheduler runs, it will run all operations, ignoring the preferredRunDate for those operations.

The startScheduledOperationsIfPermitted function should send Date() as the priority date so the system can correctly run operations using the preferredRunDate.

Awesome, great find @Bunn. I’ll push a fix.

Is it possible to meaningfully unit test for this issue?

I’ve added to the existing tests to ensure that the priorityDate is passed when running scheduled operations. I think further testing could be done lower down when we check dates and decide to run or not etc.

Bunn

Interruption looks good, but there's a bug where we are ignoring the scheduler preferredRunDate and every time it runs, it runs all operations

aataraxiaa · 2024-05-13T15:21:19Z

@THISISDINOSAUR & @Bunn - I’ve implemented all feedback. See the latest commits for relevant changes.

Bunn · 2024-05-13T16:53:33Z

...ataBrokerProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionQueueModeTests.swift

+        XCTAssertNil(result)
+    }
+
+    func testWhenModeIsScheduled_thenPriorityDateIsNotNil() throws {


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 commented May 9, 2024

View reviewed changes

aataraxiaa changed the title ~~Pete/queue manager and state~~ Implement Queue Manager for State & Interruption Management May 9, 2024

aataraxiaa requested a review from THISISDINOSAUR May 9, 2024 15:20

aataraxiaa marked this pull request as ready for review May 9, 2024 15:20

aataraxiaa requested a review from Bunn May 9, 2024 18:40

aataraxiaa added 14 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

aataraxiaa force-pushed the pete/queue-manager-and-state branch from 665b47e to 6c4a23b Compare May 10, 2024 10:53

THISISDINOSAUR reviewed May 10, 2024

View reviewed changes

aataraxiaa added 5 commits May 12, 2024 11:48

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

Bunn reviewed May 13, 2024

View reviewed changes

Bunn requested changes May 13, 2024

View reviewed changes

aataraxiaa added 2 commits May 13, 2024 16:08

PR Feedback: Fix priorityDate bug, add debug command, add tests

8c08a26

PR Discussion: Rename ...manualScan and runQueuedOperations

b3a6f1b

Bunn reviewed May 13, 2024

View reviewed changes

aataraxiaa requested a review from Bunn May 13, 2024 17:25

Bunn approved these changes May 13, 2024

View reviewed changes

THISISDINOSAUR approved these changes May 14, 2024

View reviewed changes

aataraxiaa merged commit e4c8e30 into feature/pir-stabilization May 14, 2024
22 of 23 checks passed

aataraxiaa deleted the pete/queue-manager-and-state branch May 14, 2024 14:22

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

Implement Queue Manager for State & Interruption Management #2757

Implement Queue Manager for State & Interruption Management #2757

aataraxiaa commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

aataraxiaa May 9, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 10, 2024

aataraxiaa May 13, 2024

THISISDINOSAUR May 13, 2024

aataraxiaa May 13, 2024

Bunn May 13, 2024

aataraxiaa May 13, 2024

aataraxiaa May 9, 2024

THISISDINOSAUR May 10, 2024

THISISDINOSAUR left a comment

THISISDINOSAUR May 10, 2024

THISISDINOSAUR May 10, 2024

aataraxiaa commented May 13, 2024

Bunn May 13, 2024 •

edited

Loading

aataraxiaa May 13, 2024

THISISDINOSAUR May 13, 2024

aataraxiaa May 13, 2024

Bunn left a comment

aataraxiaa commented May 13, 2024 •

edited

Loading

Bunn May 13, 2024


		firePixels(operationDependencies: operationDependencies)

		addOperations(withType: type,

Implement Queue Manager for State & Interruption Management #2757

Implement Queue Manager for State & Interruption Management #2757

Conversation

aataraxiaa commented May 8, 2024 • edited Loading

Test 1

Test 2

Test 3

Internal references:

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Old Naming

New Naming Proposal 1

New Naming Proposal 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aataraxiaa commented May 13, 2024

Bunn May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bunn left a comment

Choose a reason for hiding this comment

aataraxiaa commented May 13, 2024 • edited Loading

Choose a reason for hiding this comment

aataraxiaa commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

Bunn May 13, 2024 •

edited

Loading

aataraxiaa commented May 13, 2024 •

edited

Loading