diff --git a/ChangeLog.md b/ChangeLog.md index 900ec37a2..b819264a3 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -26,6 +26,7 @@ Blob: - Validate proposed blob and container lease IDs during acquire and change lease operations, returning `InvalidHeaderValue` for malformed GUID values to match Azure Storage. (issue #2367) - Fixed service- and container-level Filter Blobs requests failing when the optional `where` query parameter is omitted. - Fixed blob operations hanging when a client disconnects before the operation queue processes the request. (issue #2575) +- Fixed a blob download that races a `Put Block List` on the same blob returning the new ETag with the old content. The default Loki metadata store now returns a snapshot of the blob's metadata for a download, so the ETag, properties, and block list in one response always come from the same blob version. (issue #2845) - Implement `PutBlobFromUrl` (`Put Blob From URL`), which previously returned 501. The source is fetched over loopback, as `PutBlockFromURL` already does, so that SAS authentication and the `x-ms-source-if-*` conditions are enforced by the existing download path. Standard blob properties are copied from the source unless `x-ms-copy-source-blob-properties` is false, request blob content headers override them either way, request metadata replaces the source's rather than adding to it, and `x-ms-copy-source-tag-option: COPY` reads the source's tags over that same authorized path. An `x-ms-source-content-md5`, `x-ms-blob-content-md5`, `Content-MD5`, or `x-ms-content-crc64` header is checked against the copied content, and the response reports the MD5 and CRC64 of that content. A SAS needs Create or Write to create the blob, Write to overwrite it, and Tag as well when the request sets tags with `x-ms-tags` or copies the source's. As with `CopyBlobFromURL`, only sources on the same Azurite instance are supported. Queue: diff --git a/src/blob/handlers/AppendBlobHandler.ts b/src/blob/handlers/AppendBlobHandler.ts index 20a7627c2..d4c0da2e5 100644 --- a/src/blob/handlers/AppendBlobHandler.ts +++ b/src/blob/handlers/AppendBlobHandler.ts @@ -131,7 +131,10 @@ export default class AppendBlobHandler extends BaseHandler accountName, containerName, blobName, - {} + {}, + undefined, + undefined, + false // This write precheck does not need a copy of the block list. ); if (blob.properties.blobType !== Models.BlobType.AppendBlob) { diff --git a/src/blob/handlers/PageBlobHandler.ts b/src/blob/handlers/PageBlobHandler.ts index 16e8e0230..b07648387 100644 --- a/src/blob/handlers/PageBlobHandler.ts +++ b/src/blob/handlers/PageBlobHandler.ts @@ -194,7 +194,9 @@ export default class PageBlobHandler extends BaseHandler containerName, blobName, {}, - options.leaseAccessConditions + options.leaseAccessConditions, + undefined, + false // This write precheck does not need a copy of the page ranges. ); if (blob.properties.blobType !== Models.BlobType.PageBlob) { @@ -314,7 +316,9 @@ export default class PageBlobHandler extends BaseHandler containerName, blobName, {}, - options.leaseAccessConditions + options.leaseAccessConditions, + undefined, + false // This write precheck does not need a copy of the page ranges. ); if (blob.properties.blobType !== Models.BlobType.PageBlob) { diff --git a/src/blob/persistence/IBlobMetadataStore.ts b/src/blob/persistence/IBlobMetadataStore.ts index 7510486ee..0197c7463 100644 --- a/src/blob/persistence/IBlobMetadataStore.ts +++ b/src/blob/persistence/IBlobMetadataStore.ts @@ -615,6 +615,8 @@ export interface IBlobMetadataStore /** * Gets a blob item from metadata store by account name, container name and blob name. * Will return block list or page list as well for downloading. + * Write prechecks may disable the snapshot to avoid copying content metadata. + * Without a snapshot, the result may reference live metadata and must not be mutated. * * @param {Context} context * @param {string} account @@ -623,6 +625,7 @@ export interface IBlobMetadataStore * @param {BlobSelector} selector Snapshot or version to target; empty targets the current blob * @param {Models.LeaseAccessConditions} [leaseAccessConditions] Optional. Will validate lease if provided * @param {Models.ModifiedAccessConditions} [modifiedAccessConditions] + * @param {boolean} [returnSnapshot=true] Isolate the result from subsequent writes. * @returns {Promise} * @memberof IBlobMetadataStore */ @@ -633,7 +636,8 @@ export interface IBlobMetadataStore blob: string, selector: BlobSelector, leaseAccessConditions?: Models.LeaseAccessConditions, - modifiedAccessConditions?: Models.ModifiedAccessConditions + modifiedAccessConditions?: Models.ModifiedAccessConditions, + returnSnapshot?: boolean ): Promise; /** diff --git a/src/blob/persistence/LokiBlobMetadataStore.ts b/src/blob/persistence/LokiBlobMetadataStore.ts index f6d0eb540..23cefcb40 100644 --- a/src/blob/persistence/LokiBlobMetadataStore.ts +++ b/src/blob/persistence/LokiBlobMetadataStore.ts @@ -1497,7 +1497,8 @@ export default class LokiBlobMetadataStore blob: string, selector: BlobSelector = {}, leaseAccessConditions?: Models.LeaseAccessConditions, - modifiedAccessConditions?: Models.ModifiedAccessConditions + modifiedAccessConditions?: Models.ModifiedAccessConditions, + returnSnapshot: boolean = true ): Promise { const snapshot = selector.snapshot ?? ""; const versionId = selector.versionId ?? ""; @@ -1523,7 +1524,11 @@ export default class LokiBlobMetadataStore context ); - return doc; + // Downloads need a snapshot. The handlers await between reading the block + // list and the ETag, and commitBlockList mutates this live Loki document in + // place, so a shared reference can pair a new ETag with old content. + // Write prechecks can skip the copy because persistence re-fetches the blob. + return returnSnapshot ? structuredClone(doc) : doc; } /** diff --git a/src/blob/persistence/SqlBlobMetadataStore.ts b/src/blob/persistence/SqlBlobMetadataStore.ts index a6cfc5b00..e554cd09e 100644 --- a/src/blob/persistence/SqlBlobMetadataStore.ts +++ b/src/blob/persistence/SqlBlobMetadataStore.ts @@ -1190,8 +1190,10 @@ export default class SqlBlobMetadataStore implements IBlobMetadataStore { blob: string, selector: BlobSelector = {}, leaseAccessConditions?: Models.LeaseAccessConditions, - modifiedAccessConditions?: Models.ModifiedAccessConditions + modifiedAccessConditions?: Models.ModifiedAccessConditions, + returnSnapshot: boolean = true ): Promise { + // SQL queries already return independent models, regardless of returnSnapshot. const snapshot = selector.snapshot ?? ""; this.assertNoVersionId(context, selector.versionId); return this.sequelize.transaction(async (t) => { diff --git a/tests/blob/handlers/AppendBlobHandler.test.ts b/tests/blob/handlers/AppendBlobHandler.test.ts index 4dc2de6d7..2817931ff 100644 --- a/tests/blob/handlers/AppendBlobHandler.test.ts +++ b/tests/blob/handlers/AppendBlobHandler.test.ts @@ -61,7 +61,10 @@ describe("AppendBlobHandler", () => { blobCtx.account, blobCtx.container, blobCtx.blob, - deepEqual({}) + deepEqual({}), + undefined, + undefined, + false ) ).thenResolve({ name: blobCtx.blob, diff --git a/tests/blob/unit/lokiBlobMetadataStore.unit.test.ts b/tests/blob/unit/lokiBlobMetadataStore.unit.test.ts new file mode 100644 index 000000000..167894270 --- /dev/null +++ b/tests/blob/unit/lokiBlobMetadataStore.unit.test.ts @@ -0,0 +1,147 @@ +import * as assert from "assert"; + +import Context from "../../../src/blob/generated/Context"; +import * as Models from "../../../src/blob/generated/artifacts/models"; +import { + BlobModel, + BlockModel +} from "../../../src/blob/persistence/IBlobMetadataStore"; +import LokiBlobMetadataStore from "../../../src/blob/persistence/LokiBlobMetadataStore"; + +const context = { contextId: "test", startTime: new Date() } as Context; +const account = "devstoreaccount1"; +const container = "container"; +const blobName = "blob"; + +describe("LokiBlobMetadataStore @loki", () => { + let store: LokiBlobMetadataStore; + + beforeEach(async () => { + store = new LokiBlobMetadataStore("lokiBlobMetadataStore.unit.test", true); + await store.init(); + await store.createContainer(context, { + accountName: account, + name: container, + properties: { + etag: "container-etag", + lastModified: new Date() + } + }); + }); + + afterEach(async () => { + await store.close(); + }); + + async function commitVersion(blockName: string, etag: string, size: number) { + const block: BlockModel = { + accountName: account, + containerName: container, + blobName, + isCommitted: false, + name: blockName, + size, + persistency: { id: `extent-${blockName}`, offset: 0, count: size } + }; + await store.stageBlock(context, block); + + const date = new Date(); + const blob: BlobModel = { + accountName: account, + containerName: container, + name: blobName, + snapshot: "", + properties: { + creationTime: date, + lastModified: date, + etag, + blobType: Models.BlobType.BlockBlob + }, + isCommitted: true + }; + await store.commitBlockList(context, blob, [ + { blockName, blockCommitType: "Latest" } + ]); + } + + it("downloadBlob returns a snapshot that a later commitBlockList does not change", async () => { + await commitVersion("block-v1", "etag-v1", 10); + + // A download handler holds this model across awaits while it streams the + // body. A commit in that window must not change the version it describes. + const downloaded = await store.downloadBlob( + context, + account, + container, + blobName + ); + + await commitVersion("block-v2", "etag-v2", 20); + + assert.strictEqual(downloaded.properties.etag, "etag-v1"); + assert.strictEqual(downloaded.properties.contentLength, 10); + assert.deepStrictEqual( + (downloaded.committedBlocksInOrder || []).map((block) => block.name), + ["block-v1"] + ); + + const current = await store.downloadBlob( + context, + account, + container, + blobName + ); + assert.strictEqual(current.properties.etag, "etag-v2"); + assert.strictEqual(current.properties.contentLength, 20); + assert.deepStrictEqual( + (current.committedBlocksInOrder || []).map((block) => block.name), + ["block-v2"] + ); + }); + + for (const blobType of [Models.BlobType.AppendBlob, Models.BlobType.PageBlob]) { + it(`${blobType} write prechecks do not traverse content metadata`, async () => { + let extentReads = 0; + const persistency = { + get id() { + extentReads++; + return "extent"; + }, + offset: 0, + count: 512 + }; + await store.createBlob(context, { + accountName: account, + containerName: container, + name: blobName, + snapshot: "", + properties: { + blobType, + etag: "etag", + lastModified: new Date(), + contentLength: 512 + }, + isCommitted: true, + ...(blobType === Models.BlobType.AppendBlob + ? { committedBlocksInOrder: [{ name: "", size: 512, persistency }] } + : { pageRangesInOrder: [{ start: 0, end: 511, persistency }] }) + }); + extentReads = 0; + + const blob = await store.downloadBlob( + context, + account, + container, + blobName, + undefined, + undefined, + undefined, + false + ); + + assert.strictEqual(blob.properties.blobType, blobType); + assert.strictEqual(blob.properties.contentLength, 512); + assert.strictEqual(extentReads, 0); + }); + } +});