From ef22774b6b2cfa551dfb04051792d515d118374d Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Tue, 14 Oct 2025 14:47:54 +0200 Subject: [PATCH 1/7] Enhance memory management and error handling in attachment processing - Refactor content handling in AWSAttachmentsService to ensure cleanup after uploads. - Improve error handling in malwareScanner by streaming files directly to the scanner. - Optimize memory usage in basic.js and plugin.js by clearing content references post-upload. --- lib/aws-s3.js | 60 ++++++++++++++++++++++++++----------------- lib/basic.js | 21 +++++++++++++-- lib/malwareScanner.js | 38 +++++++++++++-------------- lib/plugin.js | 12 +++++++-- 4 files changed, 84 insertions(+), 47 deletions(-) diff --git a/lib/aws-s3.js b/lib/aws-s3.js index 8c43e90e..e0342328 100644 --- a/lib/aws-s3.js +++ b/lib/aws-s3.js @@ -217,7 +217,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { ) } - const { content = _content, ...metadata } = data + let { content = _content, ...metadata } = data const Key = metadata.url if (!Key) { @@ -254,17 +254,24 @@ module.exports = class AWSAttachmentsService extends require("./basic") { params: input, }) - const stored = super.put(attachments, metadata, null, isDraftEnabled) - await Promise.all([stored, multipartUpload.done()]) + try { + const stored = super.put(attachments, metadata, null, isDraftEnabled) + await Promise.all([stored, multipartUpload.done()]) - const duration = Date.now() - startTime - logConfig.debug('File upload to S3 completed successfully', { - filename: metadata.filename, - fileId: metadata.ID, - bucket: this.bucket, - key: Key, - duration - }) + const duration = Date.now() - startTime + logConfig.debug('File upload to S3 completed successfully', { + filename: metadata.filename, + fileId: metadata.ID, + bucket: this.bucket, + key: Key, + duration + }) + } finally { + // Cleanup: explicitly clear content reference to help GC + content = null + data.content = null + input.Body = null + } // Initiate malware scan if configured if (this.kind === 's3') { @@ -454,22 +461,29 @@ module.exports = class AWSAttachmentsService extends require("./basic") { client: this.client, params: input, }) - await Promise.all([multipartUpload.done()]) - const keys = { ID: targetID } + try { + await Promise.all([multipartUpload.done()]) - const scanRequestJob = cds.spawn(async () => { - await scanRequest(req.target, keys, req) - }) + const keys = { ID: targetID } - scanRequestJob.on('error', async (err) => { - logConfig.withSuggestion('error', - 'Failed to initiate malware scan for attachment', err, - 'Check malware scanner configuration and connectivity', - { keys, errorMessage: err.message }) - }) + const scanRequestJob = cds.spawn(async () => { + await scanRequest(req.target, keys, req) + }) - logConfig.debug(`[S3 Upload] Uploaded file using updateContentHandler for ${req.target.name}`) + scanRequestJob.on('error', async (err) => { + logConfig.withSuggestion('error', + 'Failed to initiate malware scan for attachment', err, + 'Check malware scanner configuration and connectivity', + { keys, errorMessage: err.message }) + }) + + logConfig.debug(`[S3 Upload] Uploaded file using updateContentHandler for ${req.target.name}`) + } finally { + // Cleanup: explicitly clear content reference to help GC + req.data.content = null + input.Body = null + } } } else if (req?.data?.note) { const key = { ID: targetID } diff --git a/lib/basic.js b/lib/basic.js index cfe584c4..7ffbebf3 100644 --- a/lib/basic.js +++ b/lib/basic.js @@ -51,6 +51,13 @@ module.exports = class AttachmentsService extends cds.Service { 'Check database connectivity and attachment entity configuration', { attachmentEntity: attachments.name, recordCount: data.length, errorMessage: error.message }) throw error + } finally { + // Cleanup: clear content reference after upload to minimize memory usage + data.forEach((d) => { + if (d.content) { + d.content = null + } + }) } } @@ -139,14 +146,24 @@ module.exports = class AttachmentsService extends cds.Service { * Handles non-draft attachment updates by uploading content to the database * @param {Express.Request} req - The request object * @param {cds.Entity} attachment - Attachments entity definition - * @returns + * @returns */ async nonDraftHandler(req, attachment) { if (req?.content?.url?.endsWith("/content")) { const attachmentID = req.content.url.match(attachmentIDRegex)[1] const data = { ID: attachmentID, content: req.content } const isDraftEnabled = false - return this.put(attachment, [data], null, isDraftEnabled) + try { + return await this.put(attachment, [data], null, isDraftEnabled) + } finally { + // Cleanup: clear content reference after upload to minimize memory usage + if (data.content) { + data.content = null + } + if (req.content) { + req.content = null + } + } } } diff --git a/lib/malwareScanner.js b/lib/malwareScanner.js index cf2e19be..2f93b63b 100644 --- a/lib/malwareScanner.js +++ b/lib/malwareScanner.js @@ -80,14 +80,10 @@ async function scanRequest(Attachments, key, req) { logConfig.debug('Fetching file content for scanning', { fileId: key.ID }) const contentStream = await AttachmentsSrv.get(currEntity, key) - let fileContent - try { - logConfig.verbose('Converting file stream to string for scanning', { fileId: key.ID }) - fileContent = await streamToString(contentStream) - } catch (err) { + if (!contentStream) { logConfig.withSuggestion('error', - 'Cannot read file content for malware scanning', err, - 'Check file integrity and storage accessibility', + 'Cannot fetch file content for malware scanning', null, + 'Check file exists and storage accessibility', { fileId: key.ID }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) return @@ -97,19 +93,20 @@ async function scanRequest(Attachments, key, req) { const scanStartTime = Date.now() try { - logConfig.debug('Sending file to Malware Scanning Service', { + logConfig.debug('Streaming file to Malware Scanning Service', { fileId: key.ID, - scannerUri: credentials.uri, - contentLength: fileContent.length + scannerUri: credentials.uri }) + // Stream the file directly to the scanner without loading into memory response = await fetch(`https://${credentials.uri}/scan`, { method: "POST", headers: { Authorization: "Basic " + Buffer.from(`${credentials.username}:${credentials.password}`, "binary").toString("base64"), }, - body: fileContent, + body: contentStream, + duplex: 'half' // Required for streaming request bodies }) } catch (error) { @@ -119,6 +116,11 @@ async function scanRequest(Attachments, key, req) { 'Check malware scanner service binding and network connectivity', { fileId: key.ID, scanDuration, scannerUri: credentials?.uri }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) + + // Cleanup: destroy stream on error + if (contentStream && typeof contentStream.destroy === 'function') { + contentStream.destroy() + } return } @@ -151,6 +153,11 @@ async function scanRequest(Attachments, key, req) { 'Check malware scanner service health and response format', { fileId: key.ID, scanDuration, httpStatus: response?.status }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) + } finally { + // Cleanup: destroy stream after scan completes + if (contentStream && typeof contentStream.destroy === 'function') { + contentStream.destroy() + } } } @@ -206,15 +213,6 @@ function getCredentials() { } } -function streamToString(stream) { - const chunks = [] - return new Promise((resolve, reject) => { - stream.on('data', (chunk) => chunks.push(Buffer.from(chunk))) - stream.on('error', (err) => reject(err)) - stream.on('end', () => resolve(Buffer.concat(chunks).toString('utf8'))) - }) -} - module.exports = { scanRequest } diff --git a/lib/plugin.js b/lib/plugin.js index 05af06b5..4cadbec3 100644 --- a/lib/plugin.js +++ b/lib/plugin.js @@ -118,13 +118,21 @@ cds.once("served", async function registerPluginHandlers() { * Reads the attachment content if requested * @param {[cds.Entity]} param0 * @param {import('@sap/cds').Request} req - The request object - * @returns + * @returns */ async function readAttachment([attachment], req) { if (req._.readAfterWrite || !req?.req?.url?.endsWith("/content") || !attachment || attachment?.content) return let keys = { ID: req.req.url.match(attachmentIDRegex)[1] } let { target } = req - attachment.content = await AttachmentsSrv.get(target, keys) //Dependency -> sending req object for usage in SDM plugin + const content = await AttachmentsSrv.get(target, keys) + attachment.content = content + + // Cleanup: Set up response cleanup to release content reference after it's sent + if (req.res && typeof req.res.once === 'function') { + req.res.once('finish', () => { + attachment.content = null + }) + } } }) From 725dfa9e47492b6a8bb948b0becdf3782d589661 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Wed, 15 Oct 2025 14:02:48 +0200 Subject: [PATCH 2/7] Optimize memory usage by updating cleanup comments in S3 upload methods --- lib/aws-s3.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/aws-s3.js b/lib/aws-s3.js index e0342328..e7e21326 100644 --- a/lib/aws-s3.js +++ b/lib/aws-s3.js @@ -267,7 +267,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { duration }) } finally { - // Cleanup: explicitly clear content reference to help GC + // clear content reference after upload to minimize memory usage content = null data.content = null input.Body = null @@ -480,7 +480,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { logConfig.debug(`[S3 Upload] Uploaded file using updateContentHandler for ${req.target.name}`) } finally { - // Cleanup: explicitly clear content reference to help GC + // clear content reference after upload to minimize memory usage req.data.content = null input.Body = null } From a2a7ba77913b06e148c2b39261088dff2052e396 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Wed, 15 Oct 2025 14:09:14 +0200 Subject: [PATCH 3/7] Remove unnecessary cleanup code for response content in attachment handling --- lib/plugin.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/plugin.js b/lib/plugin.js index 4cadbec3..099a31ae 100644 --- a/lib/plugin.js +++ b/lib/plugin.js @@ -126,13 +126,6 @@ cds.once("served", async function registerPluginHandlers() { let { target } = req const content = await AttachmentsSrv.get(target, keys) attachment.content = content - - // Cleanup: Set up response cleanup to release content reference after it's sent - if (req.res && typeof req.res.once === 'function') { - req.res.once('finish', () => { - attachment.content = null - }) - } } }) From a18b48cd60d9541593c41cfe5beea6c2ec1ecff8 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Wed, 15 Oct 2025 15:34:10 +0200 Subject: [PATCH 4/7] Refactor attachment handling to streamline memory management and error handling --- lib/aws-s3.js | 59 +++++++++++++++++-------------------------- lib/basic.js | 23 +++-------------- lib/malwareScanner.js | 11 ++------ 3 files changed, 28 insertions(+), 65 deletions(-) diff --git a/lib/aws-s3.js b/lib/aws-s3.js index e7e21326..27a35484 100644 --- a/lib/aws-s3.js +++ b/lib/aws-s3.js @@ -217,7 +217,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { ) } - let { content = _content, ...metadata } = data + const { content = _content, ...metadata } = data const Key = metadata.url if (!Key) { @@ -254,24 +254,17 @@ module.exports = class AWSAttachmentsService extends require("./basic") { params: input, }) - try { - const stored = super.put(attachments, metadata, null, isDraftEnabled) - await Promise.all([stored, multipartUpload.done()]) + const stored = super.put(attachments, metadata, null, isDraftEnabled) + await Promise.all([stored, multipartUpload.done()]) - const duration = Date.now() - startTime - logConfig.debug('File upload to S3 completed successfully', { - filename: metadata.filename, - fileId: metadata.ID, - bucket: this.bucket, - key: Key, - duration - }) - } finally { - // clear content reference after upload to minimize memory usage - content = null - data.content = null - input.Body = null - } + const duration = Date.now() - startTime + logConfig.debug('File upload to S3 completed successfully', { + filename: metadata.filename, + fileId: metadata.ID, + bucket: this.bucket, + key: Key, + duration + }) // Initiate malware scan if configured if (this.kind === 's3') { @@ -462,28 +455,22 @@ module.exports = class AWSAttachmentsService extends require("./basic") { params: input, }) - try { - await Promise.all([multipartUpload.done()]) + await Promise.all([multipartUpload.done()]) - const keys = { ID: targetID } + const keys = { ID: targetID } - const scanRequestJob = cds.spawn(async () => { - await scanRequest(req.target, keys, req) - }) + const scanRequestJob = cds.spawn(async () => { + await scanRequest(req.target, keys, req) + }) - scanRequestJob.on('error', async (err) => { - logConfig.withSuggestion('error', - 'Failed to initiate malware scan for attachment', err, - 'Check malware scanner configuration and connectivity', - { keys, errorMessage: err.message }) - }) + scanRequestJob.on('error', async (err) => { + logConfig.withSuggestion('error', + 'Failed to initiate malware scan for attachment', err, + 'Check malware scanner configuration and connectivity', + { keys, errorMessage: err.message }) + }) - logConfig.debug(`[S3 Upload] Uploaded file using updateContentHandler for ${req.target.name}`) - } finally { - // clear content reference after upload to minimize memory usage - req.data.content = null - input.Body = null - } + logConfig.debug(`[S3 Upload] Uploaded file using updateContentHandler for ${req.target.name}`) } } else if (req?.data?.note) { const key = { ID: targetID } diff --git a/lib/basic.js b/lib/basic.js index 7ffbebf3..60f7be99 100644 --- a/lib/basic.js +++ b/lib/basic.js @@ -51,13 +51,6 @@ module.exports = class AttachmentsService extends cds.Service { 'Check database connectivity and attachment entity configuration', { attachmentEntity: attachments.name, recordCount: data.length, errorMessage: error.message }) throw error - } finally { - // Cleanup: clear content reference after upload to minimize memory usage - data.forEach((d) => { - if (d.content) { - d.content = null - } - }) } } @@ -146,24 +139,14 @@ module.exports = class AttachmentsService extends cds.Service { * Handles non-draft attachment updates by uploading content to the database * @param {Express.Request} req - The request object * @param {cds.Entity} attachment - Attachments entity definition - * @returns + * @returns {Promise} - Result of the upsert operation */ async nonDraftHandler(req, attachment) { if (req?.content?.url?.endsWith("/content")) { const attachmentID = req.content.url.match(attachmentIDRegex)[1] const data = { ID: attachmentID, content: req.content } const isDraftEnabled = false - try { - return await this.put(attachment, [data], null, isDraftEnabled) - } finally { - // Cleanup: clear content reference after upload to minimize memory usage - if (data.content) { - data.content = null - } - if (req.content) { - req.content = null - } - } + return this.put(attachment, [data], null, isDraftEnabled) } } @@ -212,7 +195,7 @@ module.exports = class AttachmentsService extends cds.Service { * @param {cds.Entity} Attachments - Attachments entity definition * @param {string} key - The key of the attachment to update * @param {*} data - The data to update the attachment with - * @returns + * @returns {Promise} - Result of the update operation */ async update(Attachments, key, data) { logConfig.debug("Updating attachment for", { diff --git a/lib/malwareScanner.js b/lib/malwareScanner.js index 2f93b63b..408a6a85 100644 --- a/lib/malwareScanner.js +++ b/lib/malwareScanner.js @@ -116,11 +116,7 @@ async function scanRequest(Attachments, key, req) { 'Check malware scanner service binding and network connectivity', { fileId: key.ID, scanDuration, scannerUri: credentials?.uri }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) - - // Cleanup: destroy stream on error - if (contentStream && typeof contentStream.destroy === 'function') { - contentStream.destroy() - } + contentStream?.destroy() return } @@ -154,10 +150,7 @@ async function scanRequest(Attachments, key, req) { { fileId: key.ID, scanDuration, httpStatus: response?.status }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) } finally { - // Cleanup: destroy stream after scan completes - if (contentStream && typeof contentStream.destroy === 'function') { - contentStream.destroy() - } + contentStream?.destroy() } } From 96ff04a64e730e87d77ac8af22426ccbf5f6f629 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Thu, 16 Oct 2025 09:44:37 +0200 Subject: [PATCH 5/7] Improve S3 file handling and malware scanning by refining logging messages and ensuring proper stream cleanup --- lib/aws-s3.js | 6 +++--- lib/malwareScanner.js | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/aws-s3.js b/lib/aws-s3.js index 27a35484..9a735f5a 100644 --- a/lib/aws-s3.js +++ b/lib/aws-s3.js @@ -255,6 +255,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { }) const stored = super.put(attachments, metadata, null, isDraftEnabled) + await Promise.all([stored, multipartUpload.done()]) const duration = Date.now() - startTime @@ -335,7 +336,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { const Key = response.url - logConfig.debug('Downloading file from S3', { + logConfig.debug('Streaming file from S3', { bucket: this.bucket, key: Key }) @@ -348,7 +349,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { ) const duration = Date.now() - startTime - logConfig.debug('File download from S3 completed successfully', { + logConfig.debug('File streamed from S3 successfully', { fileId: keys.ID, bucket: this.bucket, key: Key, @@ -454,7 +455,6 @@ module.exports = class AWSAttachmentsService extends require("./basic") { client: this.client, params: input, }) - await Promise.all([multipartUpload.done()]) const keys = { ID: targetID } diff --git a/lib/malwareScanner.js b/lib/malwareScanner.js index 408a6a85..2bccd331 100644 --- a/lib/malwareScanner.js +++ b/lib/malwareScanner.js @@ -116,8 +116,13 @@ async function scanRequest(Attachments, key, req) { 'Check malware scanner service binding and network connectivity', { fileId: key.ID, scanDuration, scannerUri: credentials?.uri }) await updateStatus(AttachmentsSrv, key, "Failed", currEntity, draftEntity, activeEntity) + + // Cleanup: destroy stream on error contentStream?.destroy() return + } finally { + // Cleanup: destroy stream after request completion + contentStream?.destroy() } try { From 35c81143a79b01863c25ce960e27bea0038f0330 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Thu, 16 Oct 2025 09:54:08 +0200 Subject: [PATCH 6/7] Remove unnecessary Promise.all wrapping for multipart upload completion --- lib/aws-s3.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/aws-s3.js b/lib/aws-s3.js index 9a735f5a..6bd2327a 100644 --- a/lib/aws-s3.js +++ b/lib/aws-s3.js @@ -455,7 +455,7 @@ module.exports = class AWSAttachmentsService extends require("./basic") { client: this.client, params: input, }) - await Promise.all([multipartUpload.done()]) + await multipartUpload.done() const keys = { ID: targetID } From ad942059a2e22765ca4275be98dd73aeae1f10e8 Mon Sep 17 00:00:00 2001 From: Simon Kobler Date: Thu, 16 Oct 2025 10:29:24 +0200 Subject: [PATCH 7/7] Simplify attachment content retrieval by directly assigning the result of AttachmentsSrv.get --- lib/plugin.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/plugin.js b/lib/plugin.js index 099a31ae..df380b9e 100644 --- a/lib/plugin.js +++ b/lib/plugin.js @@ -124,8 +124,7 @@ cds.once("served", async function registerPluginHandlers() { if (req._.readAfterWrite || !req?.req?.url?.endsWith("/content") || !attachment || attachment?.content) return let keys = { ID: req.req.url.match(attachmentIDRegex)[1] } let { target } = req - const content = await AttachmentsSrv.get(target, keys) - attachment.content = content + attachment.content = await AttachmentsSrv.get(target, keys) } })