fix: handle GridFS write errors to prevent silent file loss - #6487
Conversation
DataBaseHelper.saveFile/saveFileWithId, BaseEntity._createFile and
PolicyImportExport._createFile mint the GridFS file _id up front and resolve on
end() regardless of the write outcome, with no stream 'error' handler. An async
GridFS write failure is therefore swallowed: the promise hangs (the end callback
never fires on error) or leaves a phantom fileId that later surfaces as
"FileNotFound" with lost document content.
Fix all four write sites to:
- register on('error', reject) so async write failures reject instead of
hanging / crashing (unhandled 'error') / leaving a phantom id,
- resolve with the id only after a successful finish (single-settle guard),
- reject on null/undefined content.
Adds unit tests covering reject-on-stream-error, reject-on-null,
resolve-on-success and empty-buffer-allowed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Volodymyr Shvets <volodymyr.shvets@climission.com>
There was a problem hiding this comment.
While the set of changes itself looks good, I would recommend to refactor these changes to avoid duplication, leaving each call site as a one-liner.
1. common/src/helpers/db-helper.ts
Add GridFSBucketWriteStream to the mongodb import (line 5):
import { Db, GridFSBucket, GridFSBucketWriteStream } from 'mongodb';Add the shared helper (e.g. just after connectGridFS, before saveFile):
/**
* Write content to a GridFS upload stream and resolve with the file id only
* after a successful finish. Registers an 'error' handler so async write
* failures reject instead of hanging, crashing, or leaving a phantom id.
* Rejects on null/undefined content (empty buffers/strings are valid).
* @param content file content
* @param label context label for error messages
* @param openStream opens the target upload stream
* @returns file Id
*/
public static writeToGridFS(
content: string | Buffer,
label: string,
openStream: () => GridFSBucketWriteStream
): Promise<ObjectId> {
return new Promise<ObjectId>((resolve, reject) => {
try {
if (content === null || content === undefined) {
reject(new Error(`GridFS write (${label}): content is null/undefined`));
return;
}
const stream = openStream();
const fileId = stream.id;
let settled = false;
// resolve only after a successful finish; surface async write errors instead of leaving a phantom id
const done = (err?: any) => {
if (settled) { return; }
settled = true;
if (err) { reject(err); } else { resolve(fileId); }
};
stream.on('error', done);
stream.write(content);
stream.end(() => done());
} catch (error) {
reject(error);
}
});
}Replace saveFile and saveFileWithId bodies:
public static async saveFile(uuid: string, buffer: Buffer): Promise<ObjectId> {
return DataBaseHelper.writeToGridFS(buffer, uuid, () => DataBaseHelper.gridFS.openUploadStream(uuid));
} public static async saveFileWithId(id: ObjectId, filename: string, buffer: Buffer): Promise<ObjectId> {
return DataBaseHelper.writeToGridFS(buffer, filename, () => DataBaseHelper.gridFS.openUploadStreamWithId(id, filename));
}2. common/src/models/base-entity.ts
protected _createFile(json: string | Buffer, name: string): Promise<ObjectId> {
const fileName = `${name}_${this._id?.toString()}_${GenerateUUIDv4()}`;
return DataBaseHelper.writeToGridFS(json, name, () => DataBaseHelper.gridFS.openUploadStream(fileName));
}3. common/src/import-export/policy.ts
private static _createFile(json: string | Buffer, fileName: string): Promise<ObjectId> {
return DataBaseHelper.writeToGridFS(json, fileName, () => DataBaseHelper.gridFS.openUploadStream(fileName));
}Test coverage
Because all four sites now route through writeToGridFS, the existing saveFile/saveFileWithId regression tests transitively exercise the same code path used by both _createFile methods, closing the coverage gap (BaseEntity._createFile and PolicyImportExport._createFile carry identical logic but aren't tested). If you'd rather assert them directly, you'll be able now unit-test DataBaseHelper.writeToGridFS in isolation.
…dFS helper Addresses review: the stream-error/phantom-id handling was duplicated at four call sites. DataBaseHelper.writeToGridFS now owns it and takes an openStream callback, so saveFile, saveFileWithId, BaseEntity._createFile and PolicyImportExport._createFile are each a one-liner. Behaviour is unchanged except that the two saveFile* null-content messages now use the same `GridFS write (<label>): content is null/undefined` wording as the _createFile sites. saveFileWithId still resolves the passed id (openUploadStreamWithId sets stream.id to it) and now writes via write()+end() rather than end(buffer, cb). Tests: the existing saveFile/saveFileWithId regression tests pass untouched and now transitively cover both _createFile methods. Added direct writeToGridFS coverage, including that a throwing openStream rejects instead of throwing synchronously. 13 passing in that file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
saveFileWithId previously surfaced failures via end(buffer, (err) => done(err)). Collapsing it to end(() => done()) dropped that signal for a stream that reports an error through the callback without emitting 'error'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Description
Fixes silent GridFS write loss.
DataBaseHelper.saveFile/saveFileWithId,BaseEntity._createFile, andPolicyImportExport._createFilemint the file_idup front and resolve onend()regardless of the write outcome, with no stream'error'handler. An async GridFS write failure is therefore swallowed:endcallback never fires when the stream errors), or'error'event crashes the process, orfileIdis recorded with no bytes written, surfacing later asFileNotFoundand lost document content (schema / VC / policy-action documents).Because
BaseEntity._createFileis used by every file-backed entity's@BeforeCreate/@BeforeUpdatehook, this affects ~20 entities (vc-document,vp-document,schema,policy,tool,policy-action, etc.).Changes
All four write sites now:
fileStream.on('error', reject)so async write failures reject instead of hanging / crashing / leaving a phantom id;'error'andendcan't double-settle);null/undefinedcontent (empty buffers remain valid — 0-length file).Files:
common/src/models/base-entity.ts—_createFilecommon/src/helpers/db-helper.ts—saveFile,saveFileWithIdcommon/src/import-export/policy.ts—_createFileTests
common/tests/unit-tests/gridfs-save-error-handling.test.mjs(mocksDataBaseHelper.gridFS):'error'— the core regressionnull/undefinedcommonbuilds clean; 7/7 tests pass.Related issue(s)
Silent GridFS write failures leading to
FileNotFound/ lost document content under load.