From c1d8e09bc00b51da30d487d412a9b5e5b2f6a896 Mon Sep 17 00:00:00 2001 From: Nick Tessier <22119573+nick4598@users.noreply.github.com> Date: Fri, 14 Jun 2024 10:13:18 -0400 Subject: [PATCH] Allow a callback to be passed to cleanDeletedBlocks making it interruptible. (#6842) Co-authored-by: Ben Polinsky <78756012+ben-polinsky@users.noreply.github.com> Co-authored-by: imodeljs-admin <38288322+imodeljs-admin@users.noreply.github.com> Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com> --- common/api/core-backend.api.md | 4 +- ...ite-cleanup-callback_2024-06-13-16-10.json | 10 ++ ...ite-cleanup-callback_2024-06-13-16-10.json | 10 ++ common/config/rush/pnpm-lock.yaml | 8 +- core/backend/package.json | 2 +- core/backend/src/CloudSqlite.ts | 73 +++++++++++--- .../backend/src/integration/AzuriteTest.ts | 20 ++++ .../src/integration/CloudSqlite.test.ts | 95 ++++++++++++++++++- .../imodeljs-test-app/app/build.gradle | 2 +- .../project.pbxproj | 2 +- .../project.pbxproj | 2 +- utils/workspace-editor/src/WorkspaceEditor.ts | 2 +- 12 files changed, 204 insertions(+), 26 deletions(-) create mode 100644 common/changes/@itwin/core-backend/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json create mode 100644 common/changes/@itwin/workspace-editor/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json diff --git a/common/api/core-backend.api.md b/common/api/core-backend.api.md index 253b14076c2b..01e8117e4d5e 100644 --- a/common/api/core-backend.api.md +++ b/common/api/core-backend.api.md @@ -953,10 +953,13 @@ export namespace CloudSqlite { name: string; rootDir: string; } + export function cleanDeletedBlocks(container: CloudContainer, options: CleanDeletedBlocksOptions): Promise; // (undocumented) export interface CleanDeletedBlocksOptions { debugLogging?: boolean; + findOrphanedBlocks?: boolean; nSeconds?: number; + onProgress?: (nDeleted: number, nTotalToDelete: number) => number; } export interface CloudCache { // @internal @@ -986,7 +989,6 @@ export namespace CloudSqlite { // (undocumented) readonly cache?: CloudCache; checkForChanges(): void; - cleanDeletedBlocks(options?: CleanDeletedBlocksOptions): Promise; clearWriteLock(): void; connect(cache: CloudCache): void; get containerId(): string; diff --git a/common/changes/@itwin/core-backend/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json b/common/changes/@itwin/core-backend/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json new file mode 100644 index 000000000000..0d1595d2d83b --- /dev/null +++ b/common/changes/@itwin/core-backend/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@itwin/core-backend", + "comment": "Make cleanDeletedBlocks interruptible, and remove function from CloudContainer object. Add it to CloudSqlite namespace", + "type": "none" + } + ], + "packageName": "@itwin/core-backend" +} \ No newline at end of file diff --git a/common/changes/@itwin/workspace-editor/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json b/common/changes/@itwin/workspace-editor/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json new file mode 100644 index 000000000000..d6b27ff22252 --- /dev/null +++ b/common/changes/@itwin/workspace-editor/nick-cloudsqlite-cleanup-callback_2024-06-13-16-10.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@itwin/workspace-editor", + "comment": "", + "type": "none" + } + ], + "packageName": "@itwin/workspace-editor" +} \ No newline at end of file diff --git a/common/config/rush/pnpm-lock.yaml b/common/config/rush/pnpm-lock.yaml index 97a409fe64aa..33081f50515c 100644 --- a/common/config/rush/pnpm-lock.yaml +++ b/common/config/rush/pnpm-lock.yaml @@ -11,7 +11,7 @@ importers: ../../core/backend: specifiers: - '@bentley/imodeljs-native': 4.8.2 + '@bentley/imodeljs-native': 4.8.4 '@itwin/build-tools': workspace:* '@itwin/cloud-agnostic-core': ^2.1.0 '@itwin/core-bentley': workspace:* @@ -62,7 +62,7 @@ importers: webpack: ^5.76.0 ws: ^7.5.3 dependencies: - '@bentley/imodeljs-native': 4.8.2 + '@bentley/imodeljs-native': 4.8.4 '@itwin/cloud-agnostic-core': 2.1.0_scz6qrwecfbbxg4vskopkl3a7u '@itwin/core-telemetry': link:../telemetry '@itwin/object-storage-azure': 2.2.2_scz6qrwecfbbxg4vskopkl3a7u @@ -3792,8 +3792,8 @@ packages: resolution: {integrity: sha512-IIs1wDcY2oZ8tJ3EZRw0U51M+0ZL3MvwoDYYmhUXaa9/UZqpFoOyLBGaxjirQteWXqTIMm3mFvmC+Nbn1ok4Iw==} dev: false - /@bentley/imodeljs-native/4.8.2: - resolution: {integrity: sha512-V5O/9zGvMUu6Xnk+T/rGGOA5qiEtTE8Ih/FhbrKd0Yu8tcFuWkguGNGdETJK/Tstf/rKJAcRRKbLTPiV45BUhA==} + /@bentley/imodeljs-native/4.8.4: + resolution: {integrity: sha512-ndy5I2BZH8tnS4QI/Nv4vrEmiSTfNrDObhMjjMDqOnPGK61y+5R77/kATrQhaCF3fTnpHD8AtBIpME4AOdzuqA==} requiresBuild: true dev: false diff --git a/core/backend/package.json b/core/backend/package.json index 2d81eda5463e..610fe7f4691f 100644 --- a/core/backend/package.json +++ b/core/backend/package.json @@ -98,7 +98,7 @@ "webpack": "^5.76.0" }, "dependencies": { - "@bentley/imodeljs-native": "4.8.2", + "@bentley/imodeljs-native": "4.8.4", "@itwin/cloud-agnostic-core": "^2.1.0", "@itwin/core-telemetry": "workspace:*", "@itwin/object-storage-azure": "^2.2.2", diff --git a/core/backend/src/CloudSqlite.ts b/core/backend/src/CloudSqlite.ts index 28637bd88222..0a7e57be8ddf 100644 --- a/core/backend/src/CloudSqlite.ts +++ b/core/backend/src/CloudSqlite.ts @@ -360,8 +360,23 @@ export namespace CloudSqlite { * a 404 error. Default is 1 hour. */ nSeconds?: number; - /** if enabled, outputs verbose logs about the cleanup process. These would include outputting blocks which are determined as eligible for deletion. */ + /** if enabled, outputs verbose logs about the cleanup process. Output includes blocks determined eligible for deletion. + * @default false + */ debugLogging?: boolean; + /** If true, iterates over all blobs in the cloud container to add blocks that are 'orphaned' to the delete list in the manifest. + * Orphaned blocks are created when a client abruptly halts, is disconnected, or encounters an error while uploading a change. + * If false, the search for 'orphaned' blocks is skipped and only any blocks which are already on the delete list are deleted. + * @default true + */ + findOrphanedBlocks?: boolean; + /** + * a user-supplied progress function called during the cleanup operation. While the search for orphaned blocks occurs, nDeleted will be 0 and nTotalToDelete will be 1. + * Once the search is complete and orphaned blocks begin being deleted, nDeleted will be the number of blocks deleted and nTotalToDelete will be the total number of blocks to delete. + * If the return value is 1, the job will be cancelled and progress will be saved. If one or more blocks have already been deleted, then a new manifest file is uploaded saving the progress of the delete job. + * Return any other non-0 value to cancel the job without saving progress. + */ + onProgress?: (nDeleted: number, nTotalToDelete: number) => number; } /** @@ -498,15 +513,6 @@ export namespace CloudSqlite { */ uploadChanges(): Promise; - /** - * Clean any unused deleted blocks from cloud storage. When a database is written, a subset of its blocks are replaced - * by new versions, sometimes leaving the originals unused. In this case, they are not deleted immediately. - * Instead, they are scheduled for deletion at some later time. Calling this method deletes all blocks in the cloud container - * for which the scheduled deletion time has passed. - * @param options options which influence the behavior of cleanDeletedBlocks. @see CleanDeletedBlocksOptions - */ - cleanDeletedBlocks(options?: CleanDeletedBlocksOptions): Promise; - /** * Create a copy of an existing database within this CloudContainer with a new name. * @note CloudSqlite uses copy-on-write semantics for this operation. That is, this method merely makes a @@ -515,8 +521,8 @@ export namespace CloudSqlite { */ copyDatabase(dbName: string, toAlias: string): Promise; - /** Remove a database from this CloudContainer. - * @see cleanDeletedBlocks + /** Remove a database from this CloudContainer. Unused blocks are moved to the delete list in the manifest. + * @see [[CloudSqlite.cleanDeletedBlocks]] to actually delete the blocks from the delete list. */ deleteDatabase(dbName: string): Promise; @@ -576,6 +582,46 @@ export namespace CloudSqlite { promise: Promise; } + /** + * Clean any unused deleted blocks from cloud storage. Unused deleted blocks can accumulate in cloud storage in a couple of ways: + * 1) When a database is updated, a subset of its blocks are replaced by new versions, sometimes leaving the originals unused. + * 2) A database is deleted with [[CloudContainer.deleteDatabase]] + * In both cases, the blocks are not deleted immediately. Instead, they are scheduled for deletion at some later time. + * Calling this method deletes all blocks in the cloud container for which the scheduled deletion time has passed. + * @param container the CloudContainer to be cleaned. Must be connected and hold the write lock. + * @param options options for the cleanup operation. @see CloudSqlite.CleanDeletedBlocksOptions + */ + export async function cleanDeletedBlocks(container: CloudContainer, options: CleanDeletedBlocksOptions): Promise { + let timer: NodeJS.Timeout | undefined; + try { + const cleanJob = new NativeLibrary.nativeLib.CancellableCloudSqliteJob("cleanup", container, options); + let total = 0; + const onProgress = options?.onProgress; + if (onProgress) { + timer = setInterval(async () => { // set an interval timer to show progress every 250ms + const progress = cleanJob.getProgress(); + total = progress.total; + const result = onProgress(progress.loaded, progress.total); + if (result === 1) + cleanJob.stopAndSaveProgress(); + else if (result !== 0) + cleanJob.cancelTransfer(); + }, 250); + } + await cleanJob.promise; + onProgress?.(total, total); // make sure we call progress func one last time when download completes + container.checkForChanges(); // re-read the manifest so the number of garbage blocks is updated. + } catch (err: any) { + if (err.message === "cancelled") + err.errorNumber = BriefcaseStatus.DownloadCancelled; + + throw err; + } finally { + if (timer) + clearInterval(timer); + } + } + /** @internal */ export async function transferDb(direction: TransferDirection, container: CloudContainer, props: TransferDbProps) { if (direction === "download") @@ -583,7 +629,7 @@ export namespace CloudSqlite { let timer: NodeJS.Timeout | undefined; try { - const transfer = new NativeLibrary.nativeLib.CloudDbTransfer(direction, container, props); + const transfer = new NativeLibrary.nativeLib.CancellableCloudSqliteJob(direction, container, props); let total = 0; const onProgress = props.onProgress; if (onProgress) { @@ -604,6 +650,7 @@ export namespace CloudSqlite { } finally { if (timer) clearInterval(timer); + } } diff --git a/full-stack-tests/backend/src/integration/AzuriteTest.ts b/full-stack-tests/backend/src/integration/AzuriteTest.ts index c50e5b3f334c..c29c488afc12 100644 --- a/full-stack-tests/backend/src/integration/AzuriteTest.ts +++ b/full-stack-tests/backend/src/integration/AzuriteTest.ts @@ -10,6 +10,7 @@ import * as azureBlob from "@azure/storage-blob"; import { BlobContainer, CloudSqlite, IModelHost, SettingsContainer } from "@itwin/core-backend"; import { AccessToken, Guid } from "@itwin/core-bentley"; import { LocalDirName, LocalFileName } from "@itwin/core-common"; +import * as crypto from "crypto"; // spell:ignore imodelid itwinid mkdirs devstoreaccount racwdl @@ -33,6 +34,25 @@ export namespace AzuriteTest { export namespace Sqlite { export type TestContainer = CloudSqlite.CloudContainer; + export const uploadDummyBlock = async (container: CloudSqlite.CloudContainer, blockNameSize: number): Promise => { + const generateRandomHexString = (length: number) => { + const bytes = crypto.randomBytes(length); + const hexString = bytes.toString("hex"); + return hexString.toUpperCase(); + }; + const azClient = createAzClient(container.containerId); + const blockName = `${generateRandomHexString(blockNameSize)}.bcv`; + const blobClient = azClient.getBlockBlobClient(blockName); + await blobClient.uploadData(Buffer.alloc(0)); + return blockName; + }; + + export const checkBlockExists = async (container: CloudSqlite.CloudContainer, blockName: string): Promise => { + const azClient = createAzClient(container.containerId); + const blobClient = azClient.getBlockBlobClient(blockName); + return blobClient.exists(); + }; + export const setSasToken = async (container: CloudSqlite.CloudContainer, accessLevel: BlobContainer.RequestAccessLevel) => { container.accessToken = await CloudSqlite.requestToken({ baseUri, containerId: container.containerId, accessLevel }); }; diff --git a/full-stack-tests/backend/src/integration/CloudSqlite.test.ts b/full-stack-tests/backend/src/integration/CloudSqlite.test.ts index 45dfb8198337..43b05c0c8f72 100644 --- a/full-stack-tests/backend/src/integration/CloudSqlite.test.ts +++ b/full-stack-tests/backend/src/integration/CloudSqlite.test.ts @@ -8,7 +8,7 @@ import * as chaiAsPromised from "chai-as-promised"; import { existsSync, removeSync } from "fs-extra"; import { join } from "path"; import * as sinon from "sinon"; -import { BlobContainer, BriefcaseDb, CloudSqlite, IModelHost, KnownLocations, PropertyStore, SnapshotDb, SQLiteDb } from "@itwin/core-backend"; +import { BlobContainer, BriefcaseDb, CloudSqlite, IModelHost, IModelJsFs, KnownLocations, PropertyStore, SnapshotDb, SQLiteDb } from "@itwin/core-backend"; import { KnownTestLocations } from "@itwin/core-backend/lib/cjs/test"; import { assert, BeDuration, DbResult, Guid, GuidString, OpenMode } from "@itwin/core-bentley"; import { AzuriteTest } from "./AzuriteTest"; @@ -335,14 +335,15 @@ describe("CloudSqlite", () => { await azSqlite.setSasToken(contain1, "read"); // don't ask for delete permission contain1.connect(caches[1]); await CloudSqlite.withWriteLock({ user: user1, container: contain1 }, async () => { - await expect(contain1.cleanDeletedBlocks()).eventually.rejectedWith("not authorized").property("errorNumber", 403); + // need nSeconds 0 or the blocks of the database we just deleted won't be deleted. + await expect(CloudSqlite.cleanDeletedBlocks(contain1, { nSeconds: 0 })).eventually.rejectedWith("delete block failed (403)"); }); contain1.disconnect(); await azSqlite.setSasToken(contain1, "admin"); // now ask for delete permission contain1.connect(caches[1]); - await CloudSqlite.withWriteLock({ user: user1, container: contain1 }, async () => contain1.cleanDeletedBlocks()); + await CloudSqlite.withWriteLock({ user: user1, container: contain1 }, async () => CloudSqlite.cleanDeletedBlocks(contain1, {nSeconds: 0})); expect(contain1.garbageBlocks).equals(0); // should successfully purge // should be connected @@ -445,6 +446,94 @@ describe("CloudSqlite", () => { newCache2.destroy(); }); + it("should be able to interrupt cleanDeletedBlocks operation", async () => { + const cache = azSqlite.makeCache("clean-blocks-cache"); + const container = testContainers[0]; + + const dbName = "testBimForCleaningBlocks"; + + const pathToCopy = join(KnownLocations.tmpdir, `${dbName}`); + + if (IModelJsFs.existsSync(pathToCopy)) + IModelJsFs.removeSync(pathToCopy); + const sqliteDb = new SQLiteDb(); + sqliteDb.createDb(pathToCopy, undefined, { rawSQLite: true }); + sqliteDb.executeSQL("CREATE TABLE TestData(id INTEGER PRIMARY KEY,val BLOB)"); + // Insert 16mb so we have some data to delete. + sqliteDb.executeSQL(`INSERT INTO TestData(id,val) VALUES (1, randomblob(1024*1024*16))`); + + sqliteDb.saveChanges(); + sqliteDb.closeDb(); + await azSqlite.setSasToken(container, "write"); // get a write token to be able to upload. + await azSqlite.uploadFile(container, cache, dbName, pathToCopy); + + container.connect(cache); + const dbs = container.queryDatabases(); + expect(dbs.length).to.be.greaterThanOrEqual(1); + const db = container.queryDatabase(dbName); + expect(db !== undefined).to.be.true; + + expect(container.garbageBlocks).to.be.equal(0); + container.acquireWriteLock("testuser"); + await container.deleteDatabase(dbName); + await container.uploadChanges(); + expect(container.queryDatabase(dbName)).to.be.undefined; + + expect(container.garbageBlocks).to.be.greaterThan(0); + const garbageBlocksPrev = container.garbageBlocks; + + // cleanDeletedBlocks defaults to an nSeconds of 3600, so we expect to keep our garbage blocks, because they are less than 3600 seconds old. + await CloudSqlite.cleanDeletedBlocks(container, {}); + expect(container.garbageBlocks).to.be.equal(garbageBlocksPrev); + + // Upload dummy block to simulate an orphaned block. + const blockName = await azSqlite.uploadDummyBlock(container, 24); + // findOrphanedBlocks is false, so we expect to keep our dummy block. + await CloudSqlite.cleanDeletedBlocks(container, {findOrphanedBlocks: false}); + await expect(azSqlite.checkBlockExists(container, blockName)).to.eventually.become(true); + // findOrphanedBlocks is true, so we expect to remove our dummy block. + await CloudSqlite.cleanDeletedBlocks(container, {findOrphanedBlocks: true}); + await expect(azSqlite.checkBlockExists(container, blockName)).to.eventually.become(false); + + expect(container.garbageBlocks).to.be.equal(garbageBlocksPrev); + + const onProgress = sinon.stub(); + onProgress.onFirstCall().returns(2); + + // Faking the interval setup in cleanDeletedBlocks. + const clock = sinon.useFakeTimers({toFake: ["setInterval"], shouldAdvanceTime: true, advanceTimeDelta: 1}); + let resolved = false; + CloudSqlite.cleanDeletedBlocks(container, {nSeconds: 0, findOrphanedBlocks: true, onProgress}).then(() => { + resolved = true; + }).catch(() => { + resolved = true; + }); + + while (!resolved) { + await clock.tickAsync(250); + await new Promise((resolve) => clock.setTimeout(resolve, 1)); + } + container.checkForChanges(); + + expect(onProgress.called).to.be.true; + // We aborted our cleanup, so expect no progress to be shown. + expect(container.garbageBlocks).to.be.equal(garbageBlocksPrev); + + resolved = false; + clock.reset(); + clock.restore(); + + onProgress.reset(); + onProgress.returns(0); + // One final clean that we don't interrupt ( because we always return 0 from onProgress) + await CloudSqlite.cleanDeletedBlocks(container, {nSeconds: 0, onProgress}); + container.checkForChanges(); + expect(container.garbageBlocks).to.be.equal(0); + + container.releaseWriteLock(); + container.disconnect({detach: true}); + }); + /** make sure that the auto-refresh for container tokens happens every hour */ it("Auto refresh container tokens", async () => { const contain1 = testContainers[0]; diff --git a/test-apps/display-test-app/android/imodeljs-test-app/app/build.gradle b/test-apps/display-test-app/android/imodeljs-test-app/app/build.gradle index 8112fa054435..9ea9b2483600 100644 --- a/test-apps/display-test-app/android/imodeljs-test-app/app/build.gradle +++ b/test-apps/display-test-app/android/imodeljs-test-app/app/build.gradle @@ -41,7 +41,7 @@ dependencies { implementation 'com.google.android.material:material:1.7.0' implementation 'androidx.constraintlayout:constraintlayout:2.1.4' implementation 'androidx.navigation:navigation-ui:2.5.3' - implementation 'com.github.itwin:mobile-native-android:4.8.2' + implementation 'com.github.itwin:mobile-native-android:4.8.4' implementation 'androidx.webkit:webkit:1.5.0' } diff --git a/test-apps/display-test-app/ios/imodeljs-test-app/imodeljs-test-app.xcodeproj/project.pbxproj b/test-apps/display-test-app/ios/imodeljs-test-app/imodeljs-test-app.xcodeproj/project.pbxproj index 91a46c783332..e1cfe6e1f2dc 100644 --- a/test-apps/display-test-app/ios/imodeljs-test-app/imodeljs-test-app.xcodeproj/project.pbxproj +++ b/test-apps/display-test-app/ios/imodeljs-test-app/imodeljs-test-app.xcodeproj/project.pbxproj @@ -453,7 +453,7 @@ repositoryURL = "https://github.com/iTwin/mobile-native-ios"; requirement = { kind = exactVersion; - version = 4.8.2; + version = 4.8.4; }; }; /* End XCRemoteSwiftPackageReference section */ diff --git a/tools/internal/ios/core-test-runner/core-test-runner.xcodeproj/project.pbxproj b/tools/internal/ios/core-test-runner/core-test-runner.xcodeproj/project.pbxproj index 470da8fa42bb..153eb312996f 100644 --- a/tools/internal/ios/core-test-runner/core-test-runner.xcodeproj/project.pbxproj +++ b/tools/internal/ios/core-test-runner/core-test-runner.xcodeproj/project.pbxproj @@ -552,7 +552,7 @@ repositoryURL = "https://github.com/iTwin/mobile-native-ios"; requirement = { kind = exactVersion; - version = 4.8.2; + version = 4.8.4; }; }; /* End XCRemoteSwiftPackageReference section */ diff --git a/utils/workspace-editor/src/WorkspaceEditor.ts b/utils/workspace-editor/src/WorkspaceEditor.ts index c8f77f7e5eb6..666fbe61198a 100644 --- a/utils/workspace-editor/src/WorkspaceEditor.ts +++ b/utils/workspace-editor/src/WorkspaceEditor.ts @@ -458,7 +458,7 @@ async function initializeWorkspace(args: InitializeOpts) { async function purgeWorkspace(args: EditorOpts) { const container = getCloudContainer(args); const nGarbage = container.garbageBlocks; - await CloudSqlite.withWriteLock({ ...args, container }, async () => container.cleanDeletedBlocks()); + await CloudSqlite.withWriteLock({ ...args, container }, async () => CloudSqlite.cleanDeletedBlocks(container, {})); container.checkForChanges(); // re-read manifest to get current garbage count showMessage(`purged ${sayContainer(args)}. ${nGarbage - container.garbageBlocks} garbage blocks cleaned`); }