Adds transfers between stores to external attachments #1358

Spoffy · 2025-01-04T04:22:59Z

Currently a draft, pending final changes. Opened here for initial feedback.

Context

This PR follows on from #1320, and adds support for moving attachments between stores.

This allows a user to change the document's default store, then transfer all of the attachments from their current store(s) to the new default.

This includes transfers from internal (SQLite) storage to external storage, external to internal, and external to external (e.g MinIO to filesystem).

User-facing changes:

Adds an API endpoint to migrate all current attachments to the default store set for that document

Internal changes:

Adds methods to AttachmentFileManager to facilitate transfers from one storage to another.
Exposes new methods on ActiveDoc for triggering transfers and retrieving transfer status.

All of the logic behind these changes should be documented in the source code with comments - if anything is unclear, that would be great feedback.

There a few large block comments (particularly in AttachmentFileManager) that might be a good place to start when reading this code.

Related issues

#1320
#1021

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work (some tests missing)

Spoffy · 2025-01-07T01:30:41Z

app/server/lib/AttachmentStore.ts

@@ -76,6 +76,9 @@ export interface IAttachmentStore {
  // implementation and gives them control over local buffering.
  download(docPoolId: DocPoolId, fileId: FileId, outputStream: stream.Writable): Promise<void>;

+  // Remove attachment from the store
+  delete(docPoolId: DocPoolId, fileId: FileId): Promise<void>;


This isn't actually used anywhere. I've left it in for now (since it's already implemented), but we could remove it?

# Conflicts: # test/server/lib/AttachmentFileManager.ts # Conflicts: # app/server/lib/AttachmentFileManager.ts

fflorent

Great, that looks promising :).

Here are my remarks so far (I need to continue reading the code and start looking at your tests).

app/server/lib/AttachmentFileManager.ts

fflorent · 2025-01-09T15:00:33Z

app/server/lib/DocStorage.ts

   * @returns {Promise[Boolean]} True if the file got attached; false if this ident already exists.
   */
  public findOrAttachFile(
    fileIdent: string,
    fileData: Buffer | undefined,
    storageId?: string,
+    shouldUpdate: boolean = false,


I am usually not very comfortable with boolean flag params, due to their lack of expressivity.

But am I right to say the shouldUpdate may be the default? I only see the flag is kept to false only in tests.

In any case, I would suggest for adding more expressivity the following, if that makes sense to you too:

make this function private;

introducing 2 new functions named updateOrAttachFile (where you call the private function with shouldUpdate = true) and attachFileIfNew (with shouldUpdate = false);

maybe (or maybe not, that's arguable if this is now private) change shouldUpdate: boolean into { shouldUpdate = false }: {shouldUpdate: boolean } = {} or similar;

Interesting thoughts here! I thought a lot about how to split this - I don't really like the flag either, but ended up in the "tangled implementation" case of that article you linked! 🙂

I'd be happy to do it the way you suggest - make the flagged method private, and expose it as two different public methods. It absolutely feels easier to read.

I split this up fully, and separated out the implementation in a way that I think it makes it easier to read. 🙂

Let me know if that works for you!

fflorent · 2025-01-09T15:16:25Z

app/server/lib/DocStorage.ts

+      let isNewFile = true;
+
+      try {
+        // Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists.


The comment that used to be line 793 gave some reason that is still useful, I think. So I would add some more context:

Suggested change

// Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists.

// Try to insert a new record with the given ident. It'll fail UNIQUE constraint if exists.

// Even when attempting to attach a new file exclusively (and do nothing when it exists),

// it's a good idea to first check the existence of the fileIdent and if not insert the file and its data

I'm not sure if this is still relevant after the other changes made to this function? :)

fflorent · 2025-01-09T15:36:35Z

app/server/lib/DocStorage.ts

+        await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent);
+      }
+
+      return isNewFile;


The code is acceptable as it is for me, so please skip if you don't agree much about my idea.

I feel like, even it introduces a supplementary SQL query to maintain, we would increase the readability by separating the query to check the existence of the file and do the INSERT/UPDATE. Am I right to say the below code would be equivalent?

const exists = await db.run('SELECT 1 from _gristsys_Files where ident = ?', fileIdent); // Not very sure what is returned here if (exists && shouldUpdate) { await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent); } else { await db.run('INSERT INTO _gristsys_Files(ident, data, storageId) VALUES (?, ?, ?)', fileIdent, fileData, storageId); } return exists;

These look equivalent to me. I'm not sure why it was originally done by catching the unique constraint, but it looks like the only place in the code where that's done.

I'll make this change, and we'll see if @paulfitz or anyone has any opinion :)

Edit: Change is pushed

Thanks! If there exist doubts, feel free to rollback

We hit an error in one of our test cases due to this change, full writeup is below.

Long story short: The way we handle transactions means we can hit a race condition in this example, and catching the "UNIQUE" constraint violation is the simplest way of avoiding that.

I'll change this back, and add a comment explaining why we handle it this way.

Full explanation:

The SQLiteDB class' function execTransaction is built to merge all calls into a single transaction, rather than run a transaction per-call (which is how I believed it worked).

When several atttachments are being inserted simultaneously in one API call, and because Node's async behaviour is unpredictable, the individual SQL statements are effectively re-ordered at random due to multiple promises running in parallel. This means that even if the SELECT statement says "No records exist", they might be inserted before the "INSERT" actually happens, resulting in a UNIQUE constraint violation.

If we're adding two files at the same time, the resulting order of statements ended up going something like this:
SELECT 1 as fileExists FROM _gristsys_files WHERE ident = ?
SELECT 1 as fileExists FROM _gristsys_files WHERE ident = ?
INSERT INTO main._gristsys_Files (ident, data, storageId) VALUES (?, ?, ?)
INSERT INTO main._gristsys_Files (ident, data, storageId) VALUES (?, ?, ?) // Unique constraint error thrown here.

If they hypothetically had been separate transactions on separate DB connections, the second INSERT would have failed with a "database is locked" error instead - so that's actually no better here.

Hence, catching the UNIQUE constraint error is the cleanest way to avoid this.

app/server/lib/AttachmentFileManager.ts

fflorent · 2025-01-09T16:24:28Z

app/server/lib/DocStorage.ts

   */
-  public getFileInfo(fileIdent: string): Promise<FileInfo | null> {
-    return this.get('SELECT ident, storageId, data FROM _gristsys_Files WHERE ident=?', fileIdent)
+  public getFileInfo(fileIdent: string, includeData: boolean = true): Promise<FileInfo | null> {


What would you think of the following to avoid the boolean flag param:

public getFileInfo(fileIdent: string): Promise<FileInfo | null> { return this._getFileInfo(fileIdent, 'ident, storageId'); } public getFileInfoWithData(fileIdent: string): Promise<FileInfo | null> { return this._getFileInfo(fileIdent, 'ident, storageId, data'); } // ... private _getFileInfo(fileIdent: string, columns: string): Promise<FileInfo | null> { return this.get(`SELECT ${columns} FROM _gristsys_Files WHERE ident=?`, fileIdent) .... // just like what you did }

I think splitting them up is a good idea - it definitely makes it more readable.

I think I'd rather duplicate the original function, than pass columns as a string.

That's because there's no risk of bad data accidentally being loaded into the code that formats the result into a typed object (e.g, a missing column).

Additionally, because it lowers the risk of someone doing something silly (such as passing a user-provided column name) in the future.

This is done, please let me know if the refactored version is clearer :)

fflorent · 2025-01-09T16:43:24Z

app/server/lib/AttachmentFileManager.ts

+    return {
+      fileIdent,
+      isNewFile: !fileExists,
+    };
  }


This function is a bit hard to read due to all of the conditions entangled. The good news is that it is private and you have a public method to call it, so you can rework it serenely.

I feel like it would be less complex to split this function into two different ones. I made the following, but have not tested:

public async addFile( storeId: AttachmentStoreId | undefined, fileExtension: string, fileData: Buffer ): Promise<AddFileResult> { const fileIdent = await this._getFileIdentifier(fileExtension, Readable.from(fileData)); return storeId ? this._addFileToExternalStore(storeId, fileIdent, fileData) : this._addFileToLocalStore(fileIdent, fileData); } // ... private async _addFileToExternalStore( destStoreId: AttachmentStoreId, fileIdent: string, fileData: Buffer ): Promise<AddFileResult> { const destStore = await this._getStore(destStoreId); if (!destStore) { this._log.warn({ fileIdent, storeId: destStoreId }, "tried to fetch attachment from an unavailable store"); throw new StoreNotAvailableError(destStoreId); } const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false); const fileExists = fileInfoNoData !== null; if (fileExists) { // File is already stored in a different store (e.g because store has changed and no migration has happened const isFileInTargetStore = destStoreId === fileInfoNoData.storageId; // Only exit early if the file is stored elsewhere of if the file exists in the store, // otherwise we should allow users to fix any missing files // by proceeding to the normal upload logic. const fileAlreadyExistsInStore = isFileInTargetStore && await destStore.exists(this._getDocPoolId(), fileIdent); if (!isFileInTargetStore || fileAlreadyExistsInStore) { return { fileIdent, isNewFile: false, }; } } // There's a possible race condition if anything changed the record between the initial checks // in this method, and the database being updated below - any changes will be overwritten. // However, the database will always end up referencing a valid file, and the pool-based file // deletion guarantees any files in external storage will be cleaned up eventually. await this._storeFileInAttachmentStore(destStore, fileIdent, fileData); return { fileIdent, isNewFile: !fileExists, }; } private async _addFileToLocalStore( fileIdent: string, fileData: Buffer ): Promise<AddFileResult> { const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false); const fileExists = fileInfoNoData !== null; if (!fileExists) { await this._storeFileInLocalStorage(fileIdent, fileData); } return { fileIdent, isNewFile: !fileExists, }; }

I've split this into two, and there's a few duplicate comments now - but I think that's reasonable, as it's definitely a lot easier to understand the logic now.

fflorent · 2025-01-09T16:51:25Z

app/server/lib/AttachmentFileManager.ts

+      }
+    }
+
+    // There's a possible race condition if anything changed the record between the initial checks


Typo:

Suggested change

// There's a possible race condition if anything changed the record between the initial checks

// There's a possible race condition in anything changed the record between the initial checks

Not a typo here, but I'll clean up the comment to be more intelligible!

It's trying to say:

There's the potential for a race condition here, if any other code modifies the database record between the the checks at the start of this function, and this line of code. The checks could pass then, but fail if evaluated here (due to record changes).

Rephrased to this:

// A race condition can occur here, if the file's database record is modified between the // `getFileInfoNoData` call earlier in this function and now. // Any changes made after that point will be overwritten below. // However, the database will always end up referencing a valid file, and the pool-based file // deletion guarantees any files in external storage will be cleaned up eventually.

Co-authored-by: Florent <[email protected]>

This reverts commit 586d5a4.

app/server/lib/DocStorage.ts

fflorent · 2025-01-15T14:01:42Z

We have successfully run your PR with a heavy document! 🎉

Here are feedback:

⚠️ We faced an error due to this little mistake: Adds transfers between stores to external attachments #1358 (comment) :)
✅ We successfully run the POST /api/docs/:docId/attachments/transferAll endpoint
⚠️ We could not call the GET /api/docs/:docId/attachments/transferStatus endpoint, we don't know yet the cause
❓ After the transfer, we had to request a vacuum on the doc to effectively shrink it, we wonder if there are good reason for not doing that immediately at the end of the operation (the vacuum is expected to be relatively fast if we have saved much space).
ℹ️ After the vacuum, the doc shrank from 860Mb to 24Mb

And we're very glad of the progress, thank you @Spoffy! 🙏

paulfitz

Nice! @Spoffy what do you think about changing the Promise.all in ActiveDoc around here:

      const userActions: UserAction[] = await Promise.all(
        upload.files.map(file => this._prepAttachment(docSession, file)));

... and just doing that work sequentially? I think it would be hard to catch race conditions in the code, and there doesn't seem much reason to try to squeeze out some extra speed right here.

app/server/lib/ActiveDoc.ts

paulfitz · 2025-01-15T19:14:57Z

app/server/lib/AttachmentFileManager.ts

 interface AttachmentFileManagerLogInfo {
  fileIdent?: string;
  storeId?: string | null;
 }

+interface AttachmentFileInfo {
+  ident: string,


Most of codebase uses semicolon style for interfaces (I think?) unless there's a reason not to.

Also sometimes small non-exported interfaces or classes are put down below the "meat" of the file, so that they don't distract, and that might be nice here (but this is a very weak suggestion, do what you like).

paulfitz · 2025-01-15T19:17:33Z

app/server/lib/AttachmentFileManager.ts

+ * - Avoid data loss at all costs (missing files in stores, or missing file table entries)
+ * - Always be in a valid state if possible (e.g no file entries with missing attachments)
+ * - Files in stores with no file record pointing to them is acceptable (but not preferable), as
+ * they'll eventually be cleaned up when the document pool is deleted.


Thank you for adding this.

paulfitz · 2025-01-15T19:18:37Z

app/server/lib/AttachmentFileManager.ts

@@ -99,6 +132,9 @@ export class AttachmentFileManager implements IAttachmentFileManager {
    this._docPoolId = _docInfo ? getDocPoolIdFromDocInfo(_docInfo) : null;
  }

+  // This attempts to add the attachment to the given store.
+  // If the file already exists in another store, it doesn't take any action.
+  // Therefore, there isn't a guarantee that the file exists in the given store, even if this method doesn't error.


Great, thanks for adding this.

paulfitz · 2025-01-15T19:21:02Z

app/server/lib/AttachmentFileManager.ts

+    if (hasExternal) {
+      return DocAttachmentsLocationSummary.EXTERNAL;
+    }
+    return DocAttachmentsLocationSummary.INTERNAL;


This is the zero-attachment case too right? I wonder if the zero-attachment case should be external if a default external store is set. Edge case, don't know if there is scope for confusion, feels unimportant.

I think the way we'd need to do this is to add a new state "NO_FILES", return that and either:

Render it how we like in the frontend

Or translate it in ActiveDoc to either "EXTERNAL" or "INTERNAL"

I don't mind doing that (tiny bit of work) - what do you think?

paulfitz · 2025-01-15T20:10:07Z

app/server/lib/AttachmentFileManager.ts

+        } catch(e) {
+          this._log.warn({ fileIdent, storeId: targetStoreId }, `transfer failed: ${e.message}`);
+        }
+        finally {


Can you put the finally on same line, as } finally {, like the catch.

paulfitz · 2025-01-15T20:14:57Z

app/server/lib/AttachmentFileManager.ts

+  }
+
+  private async _performPendingTransfers() {
+    while (this._pendingFileTransfers.size > 0) {


Is there some way for this to go badly wrong and loop forever. For example, if all file transfers are failing because the store is misconfigured. If log messages are emitted at too high a rate for too long, it can cause operational problems.

Entries are deleted in a "finally" block inside the for loop which goes over this._pendingFileTransfers.

The only way I can see this looping infinitely is if:

Something is constantly adding to this._pendingFileTransfers

Something is repeatedly and infinitely changing the target store for a transfer, which would prevent the _pendingFileTransfers.delete from being called.

The only call path that leads to _pendingFileTransfers having anything set is via startTransferringAllFilesToOtherStore, which adds all files to the transfer list, then exits.

startTransferringAllFilesToOtherStore can only be called via the API - so should always be a one off.

I can't see a way that _pendingFileTransfers could be constantly added to or modified aside from a malicious client calling the API - so I think we should be okay?

paulfitz · 2025-01-15T20:25:30Z

app/server/lib/AttachmentFileManager.ts

+    const fileMetadata = await this._docStorage.getFileInfo(fileIdent, false);
+    // This check runs before the file is retrieved as an optimisation to avoid loading files into
+    // memory unnecessarily.
+    if (!fileMetadata || fileMetadata.storageId == newStoreId) {


We avoid == in this codebase because anytime it is used reviewers have to think too much.

paulfitz · 2025-01-15T20:30:44Z

app/server/lib/AttachmentFileManager.ts

-      throw new StoreNotAvailableError(storeId);
+
+    // A race condition can occur here, if the file's database record is modified between the
+    // `getFileInfoNoData` call earlier in this function and now.


ActiveDoc has a well worked out mechanism to avoid parallel changes to the document (see the Share class). The underlying SQLite database also applies changes sequentially. It feels like something has gone wrong somewhere, there is some parallelism introduced for attachments, that is making your life harder.

This is theoretical for the most part - I think it might be possible if several add requests for the file come in at once, and we don't apply the share class to the attachment API in ActiveDoc? Hard to be sure though without studying the code very carefully to trace all paths to this function.

I could probably remove this without any harm, but I thought it might be worth pointing out just in case in comes at some point in the future. Could also add "theoretical" to the start of the comment.

Not sure what the best practice here is, what do you think?

paulfitz · 2025-01-15T20:34:34Z

app/server/lib/DocApi.ts

+    this._app.get('/api/docs/:docId/attachments/transferStatus', isOwner, withDoc(async (activeDoc, req, res) => {
+      res.json({
+        status: activeDoc.attachmentTransferStatus(),
+      });


How about sticking in locationSummary for consistency with /transferAll?

berhalak · 2025-01-20T15:06:31Z

app/server/lib/DocApi.ts

+    }));
+
+    // Returns the status of any current / pending attachment transfers
+    this._app.get('/api/docs/:docId/attachments/transferStatus', isOwner, withDoc(async (activeDoc, req, res) => {


Take a look at the endpoint registered above (line 521), it will be matched first and this handler won't be executed.

Ah, nicely spotted, I made the mistake of assuming it was done by specificity. Thanks 🙂

Spoffy changed the base branch from main to spoffy/external-attachments-prototype January 4, 2025 04:23

Spoffy commented Jan 7, 2025

View reviewed changes

berhalak self-assigned this Jan 7, 2025

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 9231ba5 to 70f707c Compare January 8, 2025 02:44

paulfitz added the Attachments label Jan 8, 2025

Base automatically changed from spoffy/external-attachments-prototype to main January 8, 2025 16:43

Spoffy added 17 commits January 8, 2025 16:45

Adds "delete" operator to attachment stores

8342a4c

Enables updating of file info

a0cc24c

Incremental refactoring towards transfers

b62bab8

# Conflicts: # test/server/lib/AttachmentFileManager.ts # Conflicts: # app/server/lib/AttachmentFileManager.ts

Adds functioning transfers of individual files

76db9e5

Adds test for attachment file corruption on transfer

8b23319

Adds additional check to file transfer

71f1314

Adds attachment transfer queue and job

a9d36d3

Adds test for all files being transferred

382847d

Adds test for recent transfers taking priority

011e5d1

Adds test for recent transfer requests taking precedence

c53c016

Updates an existing all files transfer if possible

7e34d8f

Adds location summary function and misc behaviour tweaks

2addc2f

Cleans up transfer logic and adds doc endpoint ID.

6de387c

Fixes test compilation problem

7c0ee63

Removes TODO from _performPendingTransfers

a44944a

Makes transfer code more understandable

d10ba26

Fixes an inconsistent import

cbaf592

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 70f707c to cbaf592 Compare January 8, 2025 16:45

Fixes a linter issue

1ffeb45

fflorent reviewed Jan 9, 2025

View reviewed changes

Improves several checks

ca0ee2d

Co-authored-by: Florent <[email protected]>

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 5372657 to 728c4b4 Compare January 14, 2025 05:13

Splits findOrAttach file into 2 functions

d721c79

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 728c4b4 to d721c79 Compare January 14, 2025 05:14

Spoffy added 2 commits January 14, 2025 05:15

Fixes issue with transfer job not type safety

7153b44

Renames "findOrAttachFile" to "attachFileIfNew"

e1c9a8d

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 5e7e32c to 227350c Compare January 14, 2025 05:50

Switches "UNIQUE" constraint check to a "SELECT"

586d5a4

Spoffy force-pushed the spoffy/external-attachments-transfers branch from 227350c to 586d5a4 Compare January 14, 2025 05:52

Spoffy added 6 commits January 14, 2025 06:07

Splits getFileInfo into two functions

220e6f9

Splits AttachmentFileManager._addFile into two functions

83d984e

Rephrases a comment

fdc3fa4

Fixes a failing AttachmentFileManager test

2dc01d0

Revert "Switches "UNIQUE" constraint check to a "SELECT""

34ac344

This reverts commit 586d5a4.

Adds a comment explaining why UNIQUE constraint is caught.

337c128

fflorent reviewed Jan 15, 2025

View reviewed changes

app/server/lib/DocStorage.ts Outdated Show resolved Hide resolved

paulfitz reviewed Jan 15, 2025

View reviewed changes

Spoffy force-pushed the spoffy/external-attachments-transfers branch from a56cfa0 to 337c128 Compare January 15, 2025 23:38

Spoffy added 5 commits January 18, 2025 08:08

Fixes bad record insert

4bba04e

Serializes attachment prep

5008b27

Moves interfaces to bottom of AttachmentFileManager

c553390

Simplifies AttachmentFileManager transferJob logic

981edbb

Simplifies waiting for transfers to finish

761d351

berhalak reviewed Jan 20, 2025

View reviewed changes

Spoffy added 3 commits January 20, 2025 21:43

Fixes transferStatus being uncallable, adds locationSummary

4b733b3

Minor cleaning in AttachmentFileManager

5fde08f

Merge branch 'main' into spoffy/external-attachments-transfers

30d606e

	// There's a possible race condition if anything changed the record between the initial checks
	// There's a possible race condition in anything changed the record between the initial checks

Adds transfers between stores to external attachments #1358

Are you sure you want to change the base?

Adds transfers between stores to external attachments #1358

Conversation

Spoffy commented Jan 4, 2025 • edited Loading

Context

Related issues

Has this been tested?

Choose a reason for hiding this comment

fflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Spoffy Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Spoffy Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Jan 15, 2025

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spoffy commented Jan 4, 2025 •

edited

Loading

fflorent Jan 9, 2025 •

edited

Loading

Spoffy Jan 14, 2025 •

edited

Loading

Spoffy Jan 14, 2025 •

edited

Loading

Spoffy Jan 14, 2025 •

edited

Loading

Spoffy Jan 14, 2025 •

edited

Loading

Spoffy Jan 20, 2025 •

edited

Loading