Add multi-device recording controls - #160
Conversation
1bcb8d1 to
01a9311
Compare
…cording # Conflicts: # Where/WhereCore/Sources/Persistence/SwiftDataStore.swift # Where/WhereCore/Sources/WhereServices.swift # Where/WhereUI/Tests/SwiftDataInspectorWiringTests.swift
…cording # Conflicts: # AGENTS.md
…cording # Conflicts: # Where/WhereCore/Sources/Persistence/StoreRemoteChangeSource.swift
| CloudKit mirroring. Debug uses `.localOnly`; exercise sync with a Release-signed | ||
| build. | ||
|
|
||
| Before shipping a schema change: |
There was a problem hiding this comment.
This should like part of a validation test script or tool that we should perhaps automate in some way.
We recently added support via the Inspector module for booting the app into different modes, maybe we add some sort of iCloud inspect/test mode to that?
…cording # Conflicts: # AGENTS.md
| .padding(.bottom, stylesheet.spacing.xxxLarge) | ||
| } | ||
|
|
||
| private var recordingTitle: LocalizedStringResource { |
There was a problem hiding this comment.
Do we still have (even, opt-in) access to the device name? If so I think we should request it instead of just doing this fallback.
There was a problem hiding this comment.
Additionally, recording the device name alongside the events, even for single event users is useful for audits should they happen; for non-nefarious users, it shows what device the event came from.
There was a problem hiding this comment.
Ah yes it's an entitlement now, yeah seems worth it https://developer.apple.com/documentation/bundleresources/entitlements/com.apple.developer.device-information.user-assigned-device-name
There was a problem hiding this comment.
Update: We'll do this later.
| manualDayCount: Int, | ||
| dismissedIssueCount: Int, | ||
| trackedRegionCount: Int, | ||
| recordingDeviceCount: Int = 0, |
There was a problem hiding this comment.
No defaults for things like this please.
| /// until the backed-up onboarding preference has been written and explicitly acknowledged. | ||
| public enum ImportPurpose: Sendable, Hashable { | ||
| case onboarding | ||
| case settings |
There was a problem hiding this comment.
I think we can likely drop the settings-based import if that helps to simplify things. I think it's entirely OK to only import from onboarding.
There was a problem hiding this comment.
(The main point of import/export is for backup and development, I don't expect many users to leverage it)
| /// recovery and terminal onboarding proof. Production bridges this to | ||
| /// `InstallationRecordingContextStoring`; tests can share an in-memory implementation across | ||
| /// recreated coordinators. | ||
| public struct ImportRecoveryPersistence: Sendable { |
There was a problem hiding this comment.
Let's convert this to a protocol instead.
| static func makeDecoder() -> JSONDecoder { | ||
| let decoder = JSONDecoder() | ||
| decoder.dateDecodingStrategy = .iso8601 | ||
| decoder.dateDecodingStrategy = .custom { decoder in |
There was a problem hiding this comment.
Given we're pre-release, I don't think we need this either/or check.
| /// is insufficient because another command can enter while an actor method is suspended. | ||
| private var isExclusive = false | ||
| private var waiters: [CheckedContinuation<Void, Never>] = [] | ||
| private var acceptsOperations = true |
There was a problem hiding this comment.
There's a lot of individual properties here; could we instead model these as an enum so unrepresentable states are impossible?
| /// is insufficient because another command can enter while an actor method is suspended. | ||
| private var isExclusive = false | ||
| private var waiters: [CheckedContinuation<Void, Never>] = [] | ||
| private var acceptsOperations = true |
There was a problem hiding this comment.
Honestly reading this file, it also feels like there's a LOT in this file and it's pretty messy. Perhaps we should also break this up into multiple controllers somehow?
|
|
||
| /// Broad hardware family used to choose an icon without persisting a | ||
| /// user-visible device name supplied by the operating system. | ||
| public enum RecordingDeviceKind: String, Codable, Sendable, Hashable { |
There was a problem hiding this comment.
We might want to add Mac here for the eventual catalyst app.
| case nickname | ||
| } | ||
|
|
||
| public init(from decoder: any Decoder) throws { |
There was a problem hiding this comment.
Do we need this custom decoder?
| nickname = try container.decodeIfPresent(String.self, forKey: .nickname) | ||
| } | ||
|
|
||
| public func encode(to encoder: any Encoder) throws { |
There was a problem hiding this comment.
Same do we need this custom encoder?
| let day = CalendarDay(from: date, in: aggregator.calendar) | ||
| try await store.perform { try await store.clearManualDay(day) } | ||
| let epochID = try await (store.dataEpoch()).id | ||
| try await store.perform(expectedDataEpochID: epochID) { |
There was a problem hiding this comment.
This seems repetitive, could we add a store.perform override to avoid needing unwrapping the epoch id at each callsite?
| /// window. Core Location can buffer callbacks before the stream consumer is installed; the | ||
| /// cutoff prevents those pre-consent / Off-period samples from becoming authorized merely | ||
| /// because they are consumed after recording turns On. | ||
| private var acceptsSamplesSince: Date? |
There was a problem hiding this comment.
Can we model these three properties in an enum to avoid allowing invalid states?
Replace the synced assignment DAG with installation-local recording consent, advisory device status, and append-only removal tombstones. Add onboarding recommendations, removed-device rejoin, backup/reset preservation, read filtering, settings UX, and end-to-end coverage.
There was a problem hiding this comment.
Along these lines, do we log / append the latest version other devices are on; so we can warn if you need to update a device, etc?
Addressed the latest six review findings and pushed each fix independently:
Validation is green: |
…cording # Conflicts: # Where/TODOs.md # Where/WhereUI/Tests/WhereSessionTrackingTests.swift
Replace the proposed coalescing-worker model with the shipped generation-token and exclusive-controller-lane design. Exercise authorized, denied, repeated, and reversed commands; retain the old race as a negative control; and prove the stale-permission branch is reachable.
I updated the Tracking Reconciliation TLA+ pilot in Verdict: Verified for these model bounds and assumptions. TLC results:
The deterministic Swift guard, The model uses TLC 1.7.4 / TLC2 2.19 with Temurin 21.0.8+9. It assumes a finite command list, eventual permission completion, and eventual completion of an admitted Core transition. It deliberately excludes reset/import lifecycle operations, device removal, persistence failure, cancellation, process termination, GPS samples, and unbounded command streams; those are not covered by this result. The source mapping, properties, configurations, assumptions, and exclusions are documented in |
1 participant
Summary
Why
A device left at home can record a stationary location that contradicts the user's actual travel. The first implementation solved that with an account-wide, append-only assignment DAG that selected Off or exactly one recorder. That was technically strong, but it made ordinary consent, CloudKit merging, backup restore, transfer, acknowledgement, and conflict UX much more complicated than the product needs.
This version keeps the useful 99%: each installation owns its own recording consent, onboarding makes a conservative recommendation, other devices expose read-only status, and a user can still permanently remove a lost, sold, or retired identity from anywhere. Apple Lost Mode or remote erase remains the security boundary for a stolen device.
Product behavior
Architecture and safety
DeviceRecordingControllerserializes this installation's local choice, physical GPS transition, profile registration, advisory check-in, removal observation, and failed-Off cleanup barrier.RecordingDeviceRemovalis an immutable global tombstone. Unreadable rows invalidate the removal read; identical CloudKit duplicates collapse across epochs, while conflicting same-ID payloads fail closed.WhereDataEpoch.resetBarrieruses the causal epoch graph to identify installations registered before an observed account reset, including concurrent-reset synthetic frontiers.InstallationRecordingContext.RecordingChoicemakes unconfirmed, Off, and On-with-cutoff mutually exclusive persisted states.LocationHistoryReaderis the shared removal-aware boundary used by reports, widgets, recent activity, issue scans, and foreground duplicate checks.Backup and compatibility
Where/Tools/upgrade-backup.rbupgrades shipped v1/v2 archives to v3 without inventing an installation or recording consentValidation
./test --all: 1,773 unit/integration tests passed./swiftformat --lint./xcstrings --lintswift run bumper config .swift run bumper test .(15 architecture tests)swift run bumper lint . --timingsgit diff --checkRollout
Before shipping, deploy the additive CloudKit schema to Production and complete the two-device validation checklist in
Where/Where/README.md. Use./Where/install --cloudkitfor that validation; an ordinary Debug install intentionally remains local-only.