diff --git a/.greptile/config.json b/.greptile/config.json new file mode 100644 index 000000000..9d6735365 --- /dev/null +++ b/.greptile/config.json @@ -0,0 +1,3 @@ +{ + "autoReview": ["open", "push"] +} diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index feca4fe13..c0b53fc1a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,6 +22,8 @@ State what the old code did before describing the new behavior. Link the reviewe The `size:*` label describes the effective diff. It is a review signal, not a merge gate. Do not set it by hand. +Greptile reviews new commits through [`.greptile/config.json`](.greptile/config.json). The dashboard's open-only trigger left follow-up commits unreviewed. This repository override adds push reviews without changing other repositories' settings. Greptile reads this setting from the PR's source branch, so older branches need the config too. For a branch without it, request a current review with `@greptileai review` after pushing. Use `@greptileai review this draft` for drafts. + ## Checks | Check | Runs on GitHub when | Local command | diff --git a/ios/HouseScan/Runtime/KeyframeStore.swift b/ios/HouseScan/Runtime/KeyframeStore.swift index c75157660..957bbc289 100644 --- a/ios/HouseScan/Runtime/KeyframeStore.swift +++ b/ios/HouseScan/Runtime/KeyframeStore.swift @@ -51,16 +51,21 @@ final class KeyframeStore { /// Bumped by `discardKeyframes`, so a write that started before it doesn't land in the list. private var epoch = 0 - /// Makes a new, empty scan folder and deletes every other one: only the current scan is kept - /// on the phone. Covers both a start over (the previous scan's folder) and launch (folders a - /// quit or crashed run left behind), since both make a new store. The folders to delete are - /// listed here, before a newer store can exist, and deleted later off the main actor - /// (`ScanFolderCleanup`), so a deletion that runs late can't take a newer scan's folder. - init() { + /// Makes a new, empty scan folder and deletes the old ones `ScanFolderCleanup` lists: it keeps + /// the two most recent completed scans (Saved scans offers them) and any folder whose bundle + /// is there but not whole. Covers both a start over (the previous scan's folder) and launch + /// (folders a quit or crashed run left behind), since both make a new store. + /// + /// The folders are listed here, before a newer store can exist, so a deletion that runs late + /// can't take a newer scan's folder. They are deleted later off the main actor, once + /// `bundleWrites` ends: the scan bundle writes started before this store, of which the + /// previous scan's may still be assembling its packet in a folder listed as never packaged. + /// Start over doesn't wait for it; only the deletion does. Nil, as at launch, waits for none. + init(deletingAfter bundleWrites: Task? = nil) { let scans = Self.scansRoot directory = scans.appending(path: UUID().uuidString, directoryHint: .isDirectory) try? FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) - Self.delete(ScanFolderCleanup(root: scans, keeping: directory.lastPathComponent)) + Self.delete(ScanFolderCleanup(root: scans, keeping: directory.lastPathComponent), after: bundleWrites) } /// Caches/Scans: one folder per scan. Saved scans lists the completed ones here. @@ -246,7 +251,8 @@ final class KeyframeStore { /// Zips the packet folder into `scan.zip`, manifest.json at the zip's root, and removes the /// folder. The zip is what "Share scan" offers; the upload sends scene.json alone. It stays on - /// the phone until the next scan unless the homeowner shares it. + /// the phone, listed in Saved scans, until two newer completed scans push it out + /// (`ScanFolderCleanup`). /// /// Streamed to disk one file at a time: a long walk's JPEGs held in memory twice (the entries /// and the archive) is what an in-memory build cost. @@ -267,11 +273,12 @@ final class KeyframeStore { try? files.removeItem(at: folder) } - /// Off the main actor. A write still in flight for a deleted folder fails, because its - /// directory is gone, and is dropped like any failed write. - nonisolated private static func delete(_ cleanup: ScanFolderCleanup) { + /// Off the main actor, after `bundleWrites`. A keyframe write still in flight for a deleted + /// folder fails, because its directory is gone, and is dropped like any failed write; the + /// bundle lists only keyframes already written. + nonisolated private static func delete(_ cleanup: ScanFolderCleanup, after bundleWrites: Task?) { Task.detached(priority: .utility) { - for (url, error) in cleanup.run() { + for (url, error) in await cleanup.run(after: { await bundleWrites?.value }) { RuntimeLog.engine.error("could not delete old scan \(url.lastPathComponent, privacy: .public): \(String(describing: error), privacy: .public)") } } diff --git a/ios/HouseScan/Runtime/ScanEngine.swift b/ios/HouseScan/Runtime/ScanEngine.swift index d43244723..6405337fb 100644 --- a/ios/HouseScan/Runtime/ScanEngine.swift +++ b/ios/HouseScan/Runtime/ScanEngine.swift @@ -2220,7 +2220,18 @@ final class ScanEngine { /// place, so it is not offered while a write is under way, and writes run one after another: /// a retry's write waits for the last one, and a write already superseded is skipped. /// `inputs` were captured with the upload's scene (`captureUpload`); this rereads nothing. + /// + /// Inputs read from an earlier store write into that store's folder, which the current + /// store's cleanup may delete; they are refused before they change what this scan offers. + /// The callers already drop a capture from before Start over; this keeps that true here. + /// + /// Start over doesn't wait for a write in flight: the next store's cleanup waits for + /// `bundleTask` before deleting the folder, so the write's bundle lands and is kept. func saveBundle(_ inputs: PacketInputs?) { + if let inputs, inputs.storeDirectory != store.directory { + RuntimeLog.engine.error("scan bundle not written: its capture belongs to a scan already started over") + return + } state.shareableScan = nil bundleSerial += 1 let serial = bundleSerial @@ -2271,7 +2282,11 @@ final class ScanEngine { meterTracking = nil meterAnchorPresence = MeterAnchorPresence() meterPlaneSource = .detectedPlane - store = KeyframeStore() + // The old scan's bundle may still be packing in the folder the new store's cleanup lists + // as never packaged. Start over doesn't wait for it; the cleanup's deletion does, for this + // chain head (every write waits for the one before it). `generation` was raised above, + // so a queued write for the old scan still skips, and a finished one isn't offered here. + store = KeyframeStore(deletingAfter: bundleTask) recorder = Self.makeRecorder(store) live?.setRecorder(recorder) motion.stop() diff --git a/ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift b/ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift index 3335f8361..927e58c00 100644 --- a/ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift +++ b/ios/HouseScanKit/Sources/HouseScanKit/Scene/ScanFolderCleanup.swift @@ -26,6 +26,13 @@ import Foundation /// can exist, and only those are deleted, whenever the deletion gets to run. Listing at deletion /// time instead let a cleanup that ran late delete a scan started after it: store A is made, /// Start over makes B, and A's cleanup then found B in the listing and deleted the scan in use. +/// +/// A folder whose bundle is still being built has no `scan.zip` yet, so it is listed as never +/// packaged. The packet is assembled in the folder first and zipped last, and Start over doesn't +/// wait for that write, so deleting on the listing alone took the folder from under it and the +/// scan never reached Saved scans. The app therefore deletes with `run(after:)`, waiting for the +/// bundle writes that were started before the listing; the recheck in `run` then keeps a folder +/// whose bundle landed meanwhile. public struct ScanFolderCleanup: Sendable { /// The bundle Share scan offers. A scan is completed when this file is a whole capture packet /// (`PacketArchiveCheck`); the file being there is not enough. @@ -132,7 +139,8 @@ public struct ScanFolderCleanup: Sendable { /// The deletion runs later than the listing, off the main actor, and a folder can change in /// between: a bundle write can start in a folder listed as never packaged, or finish in one. /// Each entry is read again and deleted only if it is still what it was when listed; one that - /// changed is left for the next cleanup. A change after this second read is not caught. + /// changed is left for the next cleanup. A change after this second read is not caught, so a + /// write still going when this runs can lose its folder: `run(after:)` waits for known writers. public func run() -> [(url: URL, error: any Error)] { let files = FileManager.default return obsolete.compactMap { url in @@ -146,4 +154,14 @@ public struct ScanFolderCleanup: Sendable { } } } + + /// `run()`, once `wait` returns. The app's `wait` is the scan bundle writes started before + /// this cleanup was listed (one chain: each write waits for the one before it), so a folder + /// listed while its packet was still being assembled is judged again only after that write + /// ends: a bundle that landed keeps the folder, and a write that failed before zipping leaves + /// it to be deleted. A `wait` that returns at once, as at launch with no write, is `run()`. + public func run(after wait: () async -> Void) async -> [(url: URL, error: any Error)] { + await wait() + return run() + } } diff --git a/ios/HouseScanKit/Tests/HouseScanKitTests/KeptScansTests.swift b/ios/HouseScanKit/Tests/HouseScanKitTests/KeptScansTests.swift index 521549436..9b21146c7 100644 --- a/ios/HouseScanKit/Tests/HouseScanKitTests/KeptScansTests.swift +++ b/ios/HouseScanKit/Tests/HouseScanKitTests/KeptScansTests.swift @@ -219,6 +219,255 @@ import Testing #expect(try Set(files.contentsOfDirectory(atPath: root.path)) == ["intact1", "intact2", "rewritten", "startsWriting", "finishesWriting"]) } + // MARK: A bundle still being written when Start over lists its folder (B44) + // + // The async tests below carry a five-minute time limit, only so that a gate never released + // fails instead of hanging; the gates and assertions decide pass or fail. In the concurrent + // package suite these tests reported up to 82.5 s (hosted run 37161498639, which had no + // limit), so a one-minute limit was too tight to rely on. + + /// A scan folder whose bundle is being written: the photos, stamp and raw streams the packet + /// is built from and the half-assembled `packet/`, but no `scan.zip` yet. + static func packing(_ name: String, in root: URL) throws -> URL { + try scan(name, in: root) + let folder = root.appending(path: name) + let files = FileManager.default + try Data("{}".utf8).write(to: folder.appending(path: "scan-stamp.json")) + try files.createDirectory(at: folder.appending(path: "streams-raw"), withIntermediateDirectories: true) + try Data([1, 2, 3]).write(to: folder.appending(path: "streams-raw/trajectory.bin")) + try files.createDirectory(at: folder.appending(path: "packet/photos"), withIntermediateDirectories: true) + try Data("photo".utf8).write(to: folder.appending(path: "packet/photos/p00001.jpg")) + return folder + } + + /// What a bundle write leaves when it ends. + enum WriteEnd: Sendable, CaseIterable { + /// The zip written to the end and `packet/` removed, as `zipPacket` does. + case bundled + /// It failed before zipping: `packet/` is left and there is no zip. + case failedBeforeZipping + /// A zip cut short is left. + case partialZip + + func apply(to folder: URL) throws { + let bundle = folder.appending(path: ScanFolderCleanup.bundleName) + switch self { + case .bundled: + try KeptScansTests.packet.write(to: bundle) + try FileManager.default.removeItem(at: folder.appending(path: "packet")) + case .failedBeforeZipping: + break + case .partialZip: + try KeptScansTests.packet.prefix(KeptScansTests.packet.count / 2).write(to: bundle) + } + } + } + + /// A bundle write the test holds open, then ends as `end` says; like the app's, it starts + /// only after the write before it (`after`). + struct HeldWrite { + let task: Task + private let release: AsyncStream.Continuation + + init(_ folder: URL, ending end: WriteEnd, after earlier: Task? = nil) { + let (gate, release) = AsyncStream.makeStream() + self.release = release + task = Task { + await earlier?.value + for await _ in gate { break } + try? end.apply(to: folder) + } + } + + func finish() { release.finish() } + } + + /// Deletes with `run(after:)`, waiting for `writes` as the app does, and says through + /// `arrived` when it has reached that wait. Returns the folders it failed to delete. + static func deleting(_ cleanup: ScanFolderCleanup, after writes: Task?, arrived: AsyncStream.Continuation) -> Task<[URL], Never> { + Task { + await cleanup.run(after: { + arrived.yield() + await writes?.value + }).map(\.url) + } + } + + /// Start over lists the previous scan's folder while its bundle is still being assembled: + /// with no zip yet it is listed as never packaged. The deletion waits for the write; a zip + /// written to the end keeps the folder, one cut short keeps it without a place (as any + /// partial bundle), and a write that failed before zipping leaves it to be deleted. + @Test(.timeLimit(.minutes(5)), arguments: WriteEnd.allCases) + func aBundleStillBeingWrittenIsJudgedOnlyAfterItsWrite(end: WriteEnd) async throws { + let root = try Self.makeRoot() + defer { try? FileManager.default.removeItem(at: root) } + let folder = try Self.packing("previous", in: root) + try Self.scan("new", in: root) + let write = HeldWrite(folder, ending: end) + + let cleanup = ScanFolderCleanup(root: root, keeping: "new") + // What B44 deleted: the folder being written is listed with the never-packaged ones. + #expect(Self.names(cleanup.obsolete) == ["previous"]) + let (arrived, arrive) = AsyncStream.makeStream() + let deleting = Self.deleting(cleanup, after: write.task, arrived: arrive) + var arrivals = arrived.makeAsyncIterator() + _ = await arrivals.next() + // The cleanup is waiting for the write, which is still assembling the packet. + #expect(FileManager.default.fileExists(atPath: folder.appending(path: "packet/photos/p00001.jpg").path)) + + write.finish() + #expect(await deleting.value.isEmpty) + let next = ScanFolderCleanup(root: root, keeping: "next") + switch end { + case .bundled: + #expect(FileManager.default.fileExists(atPath: folder.appending(path: ScanFolderCleanup.bundleName).path)) + #expect(Self.names(next.keptCompleted) == ["previous"]) + case .failedBeforeZipping: + #expect(!FileManager.default.fileExists(atPath: folder.path)) + case .partialZip: + #expect(FileManager.default.fileExists(atPath: folder.path)) + #expect(Self.names(next.incompleteBundles) == ["previous"]) + #expect(!Self.names(next.obsolete).contains("previous")) + } + } + + /// At launch there is no write to wait for: the deletion is `run()`'s, at once. + @Test func withNoWriteTheDeletionRunsAtOnce() async throws { + let root = try Self.makeRoot() + defer { try? FileManager.default.removeItem(at: root) } + try Self.scan("abandoned", in: root) + try Self.scan("run1", in: root, bundledMinutesAgo: 90) + try Self.scan("run2", in: root, bundledMinutesAgo: 30) + try Self.scan("run3", in: root, bundledMinutesAgo: 5) + try Self.scan("now", in: root) + + let none: Task? = nil + let failed = await ScanFolderCleanup(root: root, keeping: "now").run(after: { await none?.value }) + #expect(failed.isEmpty) + #expect(try Set(FileManager.default.contentsOfDirectory(atPath: root.path)) == ["now", "run2", "run3"]) + } + + /// The ends of named bundle writes, and the cleanups waiting for them. Ending a write + /// releases the cleanups waiting for it and records which it released, under the lock, so a + /// test reads which cleanup a write let go instead of inferring it from scheduling. + final class WriteEnds: @unchecked Sendable { + private let lock = NSLock() + private var ended: Set = [] + private var waiting: [(write: String, cleanup: String, resume: CheckedContinuation)] = [] + private var released: [String: [String]] = [:] + + /// Waits until `write` ends. `registered` is told once `cleanup` is on the list, so a + /// write ended after that releases it. + func wait(for write: String, as cleanup: String, registered: AsyncStream.Continuation) async { + await withCheckedContinuation { (resume: CheckedContinuation) in + lock.lock() + let already = ended.contains(write) + if already { released[write, default: []].append(cleanup) } else { waiting.append((write, cleanup, resume)) } + lock.unlock() + registered.yield() + if already { resume.resume() } + } + } + + func end(_ write: String) { + lock.lock() + ended.insert(write) + let ready = waiting.filter { $0.write == write } + waiting.removeAll { $0.write == write } + released[write, default: []] += ready.map(\.cleanup) + lock.unlock() + for waiter in ready { waiter.resume.resume() } + } + + func released(by write: String) -> [String] { + lock.lock() + defer { lock.unlock() } + return released[write] ?? [] + } + } + + /// Start over twice while the first scan's bundle is written, as the app chains them: the + /// second scan's write starts only after the first, so each cleanup waits for the newest + /// write when it listed (the chain head). The first write's end releases only the first + /// cleanup; the second cleanup waits for the second write, which is held before zipping with + /// its inputs in place, and finds them intact. The first scan's bundle is kept, the second's + /// only if its write ran (one queued for a scan already started over is skipped), and the + /// newest folders are never listed. + @Test(.timeLimit(.minutes(5)), arguments: [true, false]) + func rapidStartOversWaitForTheWholeChain(secondScanWrites: Bool) async throws { + let root = try Self.makeRoot() + defer { try? FileManager.default.removeItem(at: root) } + let files = FileManager.default + let ends = WriteEnds() + let first = try Self.packing("first", in: root) + let (firstGate, releaseFirst) = AsyncStream.makeStream() + let firstWrite = Task { + for await _ in firstGate { break } + try? WriteEnd.bundled.apply(to: first) + ends.end("first") + } + + // Start over: the second scan's folder is made and the first cleanup lists. + try Self.scan("second", in: root) + let firstCleanup = ScanFolderCleanup(root: root, keeping: "second") + #expect(Self.names(firstCleanup.obsolete) == ["first"]) + // The second scan captures its packet inputs, and its write queues behind the first. + let second = try Self.packing("second", in: root) + let secondInputs = second.appending(path: "packet/photos/p00001.jpg") + let (secondGate, releaseSecond) = AsyncStream.makeStream() + let secondWrite = Task { () -> Bool? in + await firstWrite.value + guard secondScanWrites else { + ends.end("second") + return nil + } + for await _ in secondGate { break } + // Its inputs must still be there; this write never recreates them. + let intact = FileManager.default.fileExists(atPath: secondInputs.path) + if intact { try? WriteEnd.bundled.apply(to: second) } + ends.end("second") + return intact + } + + // Start over again: the second cleanup lists both earlier folders. + try Self.scan("third", in: root) + let secondCleanup = ScanFolderCleanup(root: root, keeping: "third") + #expect(Self.names(secondCleanup.obsolete) == ["first", "second"]) + try Self.scan("fourth", in: root) + + let (registered, register) = AsyncStream.makeStream() + let deletingFirst = Task { + await firstCleanup.run(after: { await ends.wait(for: "first", as: "firstCleanup", registered: register) }).map(\.url) + } + let deletingSecond = Task { + await secondCleanup.run(after: { await ends.wait(for: "second", as: "secondCleanup", registered: register) }).map(\.url) + } + var registrations = registered.makeAsyncIterator() + _ = await registrations.next() + _ = await registrations.next() + #expect(files.fileExists(atPath: first.appending(path: "packet").path)) + + releaseFirst.finish() + await firstWrite.value + #expect(ends.released(by: "first") == ["firstCleanup"]) + #expect(await deletingFirst.value.isEmpty) + #expect(files.fileExists(atPath: first.appending(path: ScanFolderCleanup.bundleName).path)) + if secondScanWrites { + // The first write has ended and its cleanup has run; the second cleanup still waits. + #expect(files.fileExists(atPath: secondInputs.path)) + } + + releaseSecond.finish() + let wrote = await secondWrite.value + #expect(wrote == (secondScanWrites ? true : nil)) + #expect(ends.released(by: "second") == ["secondCleanup"]) + #expect(await deletingSecond.value.isEmpty) + var expected: Set = ["first", "third", "fourth"] + if secondScanWrites { expected.insert("second") } + #expect(try Set(files.contentsOfDirectory(atPath: root.path)) == expected) + #expect(files.fileExists(atPath: second.appending(path: ScanFolderCleanup.bundleName).path) == secondScanWrites) + } + /// Bundles saved at the same moment keep the order by name, newest name first, as before. @Test func equalSaveTimesFallBackToNameOrder() throws { let root = try Self.makeRoot() diff --git a/ios/README.md b/ios/README.md index 601d2da9c..788019214 100644 --- a/ios/README.md +++ b/ios/README.md @@ -98,7 +98,7 @@ Git ignores `Local.xcconfig`. Leave the team field in Xcode's Signing & Capabili **Logs.** Open Console.app on the Mac, select the iPhone, press Start and search for `subsystem:dev.housescanning.housescan`. Every screen change logs as `STATE=`; the `engine` category records the capture's decisions, the scan bundle and upload failures. The same lines show in Xcode's console while it runs the app. -**Getting a scan off the phone.** The result screen, and the screens for a failed or refused upload, have a Share scan button. It shares the scan bundle (below); AirDrop it to your Mac. It holds photos of a real home, so keep it out of git. After the app is closed, **Saved scans** in the first screen's top bar lists the completed scans the phone still keeps (the two most recent whose `scan.zip` was written to the end, as `ScanFolderCleanup` keeps them) with when each bundle was saved, its size and a Practice tag when its `scan-stamp.json` says so. A bundle cut short by a quit during its write is not listed, holds no kept place and is not deleted. Share there sends the same bundle. It copies the bundle to the app's tmp folder first, so a new scan's cleanup can't delete it while the share sheet reads it, and deletes the copy when the sheet closes; a launch deletes copies earlier runs left. A saved scan can only be shared, never reopened or continued. The code is in `UI/SavedScans/` and HouseScanKit's `SavedScans/`. +**Getting a scan off the phone.** The result screen, and the screens for a failed or refused upload, have a Share scan button. It shares the scan bundle (below); AirDrop it to your Mac. It holds photos of a real home, so keep it out of git. After the app is closed, **Saved scans** in the first screen's top bar lists the completed scans the phone still keeps (the two most recent whose `scan.zip` was written to the end, as `ScanFolderCleanup` keeps them) with when each bundle was saved, its size and a Practice tag when its `scan-stamp.json` says so. A bundle cut short by a quit during its write is not listed, holds no kept place and is not deleted. Start over doesn't wait for a bundle still being written, but the next scan's cleanup does before it deletes anything. If that write finishes the zip, the scan reaches Saved scans; if it fails before zipping, its folder is deleted, and if it leaves a partial zip, the scan isn't listed. Share there sends the same bundle. It copies the bundle to the app's tmp folder first, so a new scan's cleanup can't delete it while the share sheet reads it, and deletes the copy when the sheet closes; a launch deletes copies earlier runs left. A saved scan can only be shared, never reopened or continued. The code is in `UI/SavedScans/` and HouseScanKit's `SavedScans/`. ## Scan bundle