Refactor virtual drive sync engine#405
Open
egalvis27 wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds lazy virtual-drive hydration with SQLite-backed directory state, switches temporal uploads to an async queue, updates remote-sync fetch behavior and limits, and adds SQLite bootstrap initialization and transactional batch writes. ChangesVirtual drive hydration, upload queue, and sync flow
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant FuseOperation
participant LazyVirtualDriveHydrator
participant FolderRepository
participant DirectoryStateSqliteRepository
participant ServerAPI
FuseOperation->>LazyVirtualDriveHydrator: readDirectory(path)
LazyVirtualDriveHydrator->>FolderRepository: ensureFolderMaterialized(path)
LazyVirtualDriveHydrator->>DirectoryStateSqliteRepository: isFresh(folderId, scope)
alt stale
LazyVirtualDriveHydrator->>ServerAPI: fetchFolder(folderId)
ServerAPI-->>LazyVirtualDriveHydrator: children DTOs
LazyVirtualDriveHydrator->>FolderRepository: upsert children
LazyVirtualDriveHydrator->>DirectoryStateSqliteRepository: markLoaded/markError
end
LazyVirtualDriveHydrator-->>FuseOperation: local directory entries
sequenceDiagram
participant ReleaseService
participant TemporalFileUploadQueue
participant TemporalFileRepository
participant Uploader
ReleaseService->>TemporalFileUploadQueue: enqueue(temporalFile, path, processName)
TemporalFileUploadQueue->>TemporalFileRepository: stage(path, targetFolder)
TemporalFileUploadQueue->>Uploader: run(task)
Uploader-->>TemporalFileUploadQueue: success or error
TemporalFileUploadQueue-->>ReleaseService: queued (async)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/features/virtual-drive/ipc/handlers.ts (1)
14-24: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the fire-and-forget failure path.
void Promise.all([...])drops rejections from either async call, so a failure inupdateVirtualDriveContainerorDirectoryStateSqliteRepository.clear()becomes an unhandled promise rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/features/virtual-drive/ipc/handlers.ts` around lines 14 - 24, The fire-and-forget logic in remoteChangesSyncedHandler currently ignores failures from both updateVirtualDriveContainer and DirectoryStateSqliteRepository.clear(), which can surface as unhandled rejections. Update this handler to explicitly catch and log errors from the Promise.all work, using the existing remoteChangesSyncedHandler, updateVirtualDriveContainer, and DirectoryStateSqliteRepository.clear symbols so the failure path is handled safely without dropping rejected promises.
🧹 Nitpick comments (5)
src/backend/features/virtual-drive/services/operations/opendir.service.test.ts (1)
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers rejection with an already-well-formed
FuseError.This masks the cast issue in
opendir.service.ts(error: err as FuseError) — add a case wherehydrator.readDirectoryrejects with a plainErrorto verify the service still returns a validFuseErrorwith a correct.code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/features/virtual-drive/services/operations/opendir.service.test.ts` around lines 57 - 59, The existing opendir service test only exercises a rejection that is already a FuseError, so it does not catch the unsafe cast in opendir.service.ts. Extend the test in opendir.service.test.ts around hydrator.readDirectory to also reject with a plain Error and assert the service still resolves to a FuseError with the expected code. Use the opendir service path and the FuseError/FuseCodes assertions to verify the fallback behavior when err is not already a FuseError.src/backend/features/virtual-drive/services/operations/get-attributes.service.ts (1)
86-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate file/folder attribute-mapping logic.
The hydrated-entry file/folder attribute blocks (Lines 89-117) closely mirror the earlier local-entry blocks (Lines 54-82). Consider extracting
toFileAttributes(file)/toFolderAttributes(folder)helpers to avoid drift between the two near-identical mappings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/features/virtual-drive/services/operations/get-attributes.service.ts` around lines 86 - 118, The file and folder attribute mapping in GetAttributesService is duplicated between the local-entry and hydrated-entry branches, which risks drift over time. Refactor the shared logic into reusable helpers such as toFileAttributes and toFolderAttributes (or equivalent methods near findLocalEntry / ensurePathLoaded) and have both branches call them so the mode, size, timestamps, uid/gid, and nlink mapping stays consistent in one place.src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts (1)
75-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for hydrated-entry early-return branches.
The new logic in
getAttributes(implementation Lines 86-117) re-resolvesfindLocalEntryafter hydration and returns early if a file/folder is now found — this is the core new behavior for this layer. This test only exercises the path where hydration finds nothing and falls back todocument. Add cases wherefileSearcher.run/folderSearcher.runresolve to a value only afterhydrator.ensurePathLoadedis called, to verify the hydrated early-return paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts` around lines 75 - 84, The current getAttributes test only covers the fallback path after hydration, but it misses the new early-return behavior after re-running findLocalEntry. Add test cases in get-attributes.service.test.ts where hydrator.ensurePathLoaded is invoked and then fileSearcher.run or folderSearcher.run resolves to a hydrated file/folder, and assert getAttributes returns those attributes immediately instead of continuing to document fallback. Use the existing getAttributes, hydrator.ensurePathLoaded, fileSearcher.run, and folderSearcher.run symbols to target the new hydrated-entry branches.src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts (1)
12-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
PropsandUploadTaskare structurally identical.Consider reusing one type to avoid duplication as the shape evolves.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts` around lines 12 - 22, Props and UploadTask are duplicated with the same shape, so reuse a single type definition in TemporalFileUploadQueue to avoid drift as fields change. Update the queue’s type declarations so one of the symbols, Props or UploadTask, becomes the shared source of truth and the other references it directly.src/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts (1)
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
// TODO: can be private?comment on line 28 is now stale — this binding is intentionally made non-private so the lazy hydrator can resolveFolderRepository. Consider removing the TODO to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts` around lines 28 - 30, The TODO comment in registerFolderServices is stale because the FolderRepository binding is intentionally public for lazy hydrator resolution. Remove the “can be private?” comment from the builder.register(FolderRepository).use(InMemoryFolderRepository).asSingleton() registration so the intent is clear and there’s no confusion for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/apps/main/database/data-source.ts`:
- Around line 41-49: `initializeVirtualDriveSqlite()` is currently
all-or-nothing, so a single failing bootstrap statement can stop startup and
prevent the `APP_DATA_SOURCE_INITIALIZED` emit from happening. Update this
helper to make `SQLITE_BOOTSTRAP_STATEMENTS` best-effort by handling errors per
statement inside the loop, logging failures with context via
`AppDataSource.query`, and continuing instead of throwing so the virtual-drive
startup path can proceed.
In
`@src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts`:
- Around line 51-52: The `readLocalDirectory` call in `LazyVirtualDriveHydrator`
is passing an unused `statusScope` argument, which is misleading and flagged by
lint. Remove the `statusScope` parameter from the `readLocalDirectory` signature
and update the `LazyVirtualDriveHydrator` call site (and any other referenced
overloads around the same code path) so `folder` is the only needed argument,
keeping the behavior unchanged.
- Around line 79-96: The loop in LazyVirtualDriveHydrator is intentionally
sequential, so the no-await-in-loop lint warning should be handled explicitly
rather than left unresolved. Add a targeted lint disable comment on the await
inside the path-segment loop in LazyVirtualDriveHydrator, and include a brief
justification that each segment depends on the previous folder being hydrated
before continuing.
- Around line 127-196: The delete order in fetchAndStoreChildren is unsafe
because folderRepository.deleteMatchingPartial and
fileRepository.deleteMatchingPartial run before the new local children are fully
persisted. Update fetchAndStoreChildren to write the replacement folders/files
first using createOrUpdateFolderByBatch, createOrUpdateFileByBatch, and the
Promise.all add/upsert step, then remove stale rows afterward. Keep the existing
mapping logic and error handling, but make sure the durable insert/update
completes before any delete calls.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts`:
- Around line 40-56: The deduplication in TemporalFileUploadQueue.enqueue is
vulnerable to a race because queuedPaths is only updated after the await on
repository.stage, allowing duplicate uploads for the same path. Reserve the path
in queuedPaths before calling repository.stage in enqueue, then proceed to stage
and push the task using the existing enqueue, drain, and queuedPaths symbols so
concurrent calls can no longer pass the has check before the path is marked.
In `@src/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts`:
- Around line 107-118: deleteMatchingPartial in InMemoryFileRepository should
use the same contentsId normalization behavior as matchingPartial, since strict
equality can miss entries that should match. Update the comparison logic inside
deleteMatchingPartial to special-case contentsId the same way matchingPartial
does, while keeping the rest of the key checks unchanged, so the two methods
stay consistent.
---
Outside diff comments:
In `@src/backend/features/virtual-drive/ipc/handlers.ts`:
- Around line 14-24: The fire-and-forget logic in remoteChangesSyncedHandler
currently ignores failures from both updateVirtualDriveContainer and
DirectoryStateSqliteRepository.clear(), which can surface as unhandled
rejections. Update this handler to explicitly catch and log errors from the
Promise.all work, using the existing remoteChangesSyncedHandler,
updateVirtualDriveContainer, and DirectoryStateSqliteRepository.clear symbols so
the failure path is handled safely without dropping rejected promises.
---
Nitpick comments:
In `@src/apps/drive/dependency-injection/virtual-drive/registerFolderServices.ts`:
- Around line 28-30: The TODO comment in registerFolderServices is stale because
the FolderRepository binding is intentionally public for lazy hydrator
resolution. Remove the “can be private?” comment from the
builder.register(FolderRepository).use(InMemoryFolderRepository).asSingleton()
registration so the intent is clear and there’s no confusion for future readers.
In
`@src/backend/features/virtual-drive/services/operations/get-attributes.service.test.ts`:
- Around line 75-84: The current getAttributes test only covers the fallback
path after hydration, but it misses the new early-return behavior after
re-running findLocalEntry. Add test cases in get-attributes.service.test.ts
where hydrator.ensurePathLoaded is invoked and then fileSearcher.run or
folderSearcher.run resolves to a hydrated file/folder, and assert getAttributes
returns those attributes immediately instead of continuing to document fallback.
Use the existing getAttributes, hydrator.ensurePathLoaded, fileSearcher.run, and
folderSearcher.run symbols to target the new hydrated-entry branches.
In
`@src/backend/features/virtual-drive/services/operations/get-attributes.service.ts`:
- Around line 86-118: The file and folder attribute mapping in
GetAttributesService is duplicated between the local-entry and hydrated-entry
branches, which risks drift over time. Refactor the shared logic into reusable
helpers such as toFileAttributes and toFolderAttributes (or equivalent methods
near findLocalEntry / ensurePathLoaded) and have both branches call them so the
mode, size, timestamps, uid/gid, and nlink mapping stays consistent in one
place.
In
`@src/backend/features/virtual-drive/services/operations/opendir.service.test.ts`:
- Around line 57-59: The existing opendir service test only exercises a
rejection that is already a FuseError, so it does not catch the unsafe cast in
opendir.service.ts. Extend the test in opendir.service.test.ts around
hydrator.readDirectory to also reject with a plain Error and assert the service
still resolves to a FuseError with the expected code. Use the opendir service
path and the FuseError/FuseCodes assertions to verify the fallback behavior when
err is not already a FuseError.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts`:
- Around line 12-22: Props and UploadTask are duplicated with the same shape, so
reuse a single type definition in TemporalFileUploadQueue to avoid drift as
fields change. Update the queue’s type declarations so one of the symbols, Props
or UploadTask, becomes the shared source of truth and the other references it
directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 522a22bd-87c6-469c-a716-8b578d062dac
📒 Files selected for processing (37)
src/apps/drive/dependency-injection/offline-drive/registerTemporalFilesServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerFilesServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerFolderServices.tssrc/apps/drive/dependency-injection/virtual-drive/registerVirtualDriveSharedServices.tssrc/apps/main/database/data-source.tssrc/apps/main/remote-sync/RemoteSyncManager.test.tssrc/apps/main/remote-sync/RemoteSyncManager.tssrc/apps/main/remote-sync/service.tssrc/backend/features/virtual-drive/ipc/handlers.tssrc/backend/features/virtual-drive/services/drive-folder/virtual-drive.service.test.tssrc/backend/features/virtual-drive/services/drive-folder/virtual-drive.service.tssrc/backend/features/virtual-drive/services/lazy/DirectoryStateSqliteRepository.tssrc/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.test.tssrc/backend/features/virtual-drive/services/operations/get-attributes.service.tssrc/backend/features/virtual-drive/services/operations/open.service.test.tssrc/backend/features/virtual-drive/services/operations/open.service.tssrc/backend/features/virtual-drive/services/operations/opendir.service.test.tssrc/backend/features/virtual-drive/services/operations/opendir.service.tssrc/backend/features/virtual-drive/services/operations/release.service.test.tssrc/backend/features/virtual-drive/services/operations/release.service.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.tssrc/context/storage/TemporalFiles/domain/TemporalFileRepository.tssrc/context/storage/TemporalFiles/infrastructure/NodeTemporalFileRepository.tssrc/context/virtual-drive/files/domain/FileRepository.tssrc/context/virtual-drive/files/infrastructure/InMemoryFileRepository.tssrc/context/virtual-drive/folders/__mocks__/FolderRepositoryMock.tssrc/context/virtual-drive/folders/application/create/SimpleFolderCreator.tssrc/context/virtual-drive/folders/domain/FolderRepository.tssrc/context/virtual-drive/folders/infrastructure/InMemoryFolderRepository.tssrc/context/virtual-drive/remoteTree/infrastructure/SQLiteRemoteItemsGenerator.tssrc/core/bootstrap/register-session-event-handlers.tssrc/core/electron/paths.tssrc/infra/local-file-system/safe-readdir.test.tssrc/infra/sqlite/services/file/create-or-update-file-by-batch.tssrc/infra/sqlite/services/folder/create-or-update-folder-by-batch.tsvitest.setup.main.ts
Comment on lines
+41
to
+49
| export async function initializeVirtualDriveSqlite() { | ||
| if (!AppDataSource.isInitialized) { | ||
| return; | ||
| } | ||
|
|
||
| for (const statement of SQLITE_BOOTSTRAP_STATEMENTS) { | ||
| await AppDataSource.query(statement); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bootstrap failure will block virtual-drive startup. initializeVirtualDriveSqlite() has no error handling, and in the login flow it is awaited before eventBus.emit('APP_DATA_SOURCE_INITIALIZED') (see register-session-event-handlers.ts line 33-34). A single failing PRAGMA/CREATE INDEX (e.g. an index referencing a not-yet-synced column) throws, the emit is skipped, and the virtual drive mount trigger — which this stack switches to APP_DATA_SOURCE_INITIALIZED — never fires.
Index/pragma tuning is best-effort; wrap each statement so a failure is logged but non-fatal.
🛡️ Make bootstrap best-effort
for (const statement of SQLITE_BOOTSTRAP_STATEMENTS) {
- await AppDataSource.query(statement);
+ try {
+ await AppDataSource.query(statement);
+ } catch (error) {
+ logger.error({ msg: 'Error running SQLite bootstrap statement', statement, error });
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function initializeVirtualDriveSqlite() { | |
| if (!AppDataSource.isInitialized) { | |
| return; | |
| } | |
| for (const statement of SQLITE_BOOTSTRAP_STATEMENTS) { | |
| await AppDataSource.query(statement); | |
| } | |
| } | |
| export async function initializeVirtualDriveSqlite() { | |
| if (!AppDataSource.isInitialized) { | |
| return; | |
| } | |
| for (const statement of SQLITE_BOOTSTRAP_STATEMENTS) { | |
| try { | |
| await AppDataSource.query(statement); | |
| } catch (error) { | |
| logger.error({ msg: 'Error running SQLite bootstrap statement', statement, error }); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint / 🔍 Lint
[warning] 47-47: ESLint no-await-in-loop: Unexpected await inside a loop.
🪛 GitHub Actions: Lint / 0_🔍 Lint.txt
[warning] 47-47: ESLint warning: Unexpected await inside a loop. (no-await-in-loop)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/apps/main/database/data-source.ts` around lines 41 - 49,
`initializeVirtualDriveSqlite()` is currently all-or-nothing, so a single
failing bootstrap statement can stop startup and prevent the
`APP_DATA_SOURCE_INITIALIZED` emit from happening. Update this helper to make
`SQLITE_BOOTSTRAP_STATEMENTS` best-effort by handling errors per statement
inside the loop, logging failures with context via `AppDataSource.query`, and
continuing instead of throwing so the virtual-drive startup path can proceed.
Comment on lines
+51
to
+52
|
|
||
| return readLocalDirectory({ folder, statusScope }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused statusScope param from readLocalDirectory.
Flagged by pipeline lint as unused. Currently harmless since DirectoryStatusScope has a single member, but it misleadingly implies scope-based filtering that isn't implemented.
🧹 Proposed fix
- return readLocalDirectory({ folder, statusScope });
+ return readLocalDirectory({ folder });
...
function readLocalDirectory({
folder,
- statusScope,
}: {
folder: Folder;
- statusScope: DirectoryStatusScope;
}): DirectoryEntries {Also applies to: 208-223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts`
around lines 51 - 52, The `readLocalDirectory` call in
`LazyVirtualDriveHydrator` is passing an unused `statusScope` argument, which is
misleading and flagged by lint. Remove the `statusScope` parameter from the
`readLocalDirectory` signature and update the `LazyVirtualDriveHydrator` call
site (and any other referenced overloads around the same code path) so `folder`
is the only needed argument, keeping the behavior unchanged.
Source: Pipeline failures
Comment on lines
+79
to
+96
| for (const segment of requestedPath.split('/').filter(Boolean)) { | ||
| currentPath = `${currentPath}/${segment}`; | ||
|
|
||
| const existing = folderRepository.matchingPartial({ path: currentPath })[0]; | ||
| if (existing) { | ||
| currentFolder = existing; | ||
| continue; | ||
| } | ||
|
|
||
| await refreshChildrenIfNeeded({ folder: currentFolder, statusScope }); | ||
|
|
||
| const hydrated = folderRepository.matchingPartial({ path: currentPath })[0]; | ||
| if (!hydrated) { | ||
| throw new FuseError(FuseCodes.ENOENT, `[FUSE - Lazy] Folder not found: ${currentPath}`); | ||
| } | ||
|
|
||
| currentFolder = hydrated; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Silence the no-await-in-loop warning with justification instead of leaving it unresolved.
Flagged by pipeline lint. The sequential await here is intentional — each path segment's materialization depends on the previous one being resolved — so this isn't a real parallelization opportunity; add an explicit disable comment to document that.
🧹 Proposed fix
const existing = folderRepository.matchingPartial({ path: currentPath })[0];
if (existing) {
currentFolder = existing;
continue;
}
+ // eslint-disable-next-line no-await-in-loop -- each segment must be resolved sequentially before descending
await refreshChildrenIfNeeded({ folder: currentFolder, statusScope });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const segment of requestedPath.split('/').filter(Boolean)) { | |
| currentPath = `${currentPath}/${segment}`; | |
| const existing = folderRepository.matchingPartial({ path: currentPath })[0]; | |
| if (existing) { | |
| currentFolder = existing; | |
| continue; | |
| } | |
| await refreshChildrenIfNeeded({ folder: currentFolder, statusScope }); | |
| const hydrated = folderRepository.matchingPartial({ path: currentPath })[0]; | |
| if (!hydrated) { | |
| throw new FuseError(FuseCodes.ENOENT, `[FUSE - Lazy] Folder not found: ${currentPath}`); | |
| } | |
| currentFolder = hydrated; | |
| } | |
| for (const segment of requestedPath.split('/').filter(Boolean)) { | |
| currentPath = `${currentPath}/${segment}`; | |
| const existing = folderRepository.matchingPartial({ path: currentPath })[0]; | |
| if (existing) { | |
| currentFolder = existing; | |
| continue; | |
| } | |
| // eslint-disable-next-line no-await-in-loop -- each segment must be resolved sequentially before descending | |
| await refreshChildrenIfNeeded({ folder: currentFolder, statusScope }); | |
| const hydrated = folderRepository.matchingPartial({ path: currentPath })[0]; | |
| if (!hydrated) { | |
| throw new FuseError(FuseCodes.ENOENT, `[FUSE - Lazy] Folder not found: ${currentPath}`); | |
| } | |
| currentFolder = hydrated; | |
| } |
🧰 Tools
🪛 GitHub Actions: Lint / 0_🔍 Lint.txt
[warning] 88-88: ESLint warning: Unexpected await inside a loop. (no-await-in-loop)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts`
around lines 79 - 96, The loop in LazyVirtualDriveHydrator is intentionally
sequential, so the no-await-in-loop lint warning should be handled explicitly
rather than left unresolved. Add a targeted lint disable comment on the await
inside the path-segment loop in LazyVirtualDriveHydrator, and include a brief
justification that each segment depends on the previous folder being hydrated
before continuing.
Source: Pipeline failures
Comment on lines
+127
to
+196
| async function fetchAndStoreChildren({ folder, statusScope }: { folder: Folder; statusScope: DirectoryStatusScope }) { | ||
| try { | ||
| const { data, error } = await fetchFolder(folder.uuid); | ||
|
|
||
| if (error || !data) { | ||
| throw error ?? new FolderNotFoundError(folder.path); | ||
| } | ||
|
|
||
| const remoteFolders = data.children.filter(isExistingFolder).map((child) => toRemoteFolder(child)); | ||
| const remoteFiles = data.files.filter(isExistingFile).map((child) => toRemoteFile(child)); | ||
|
|
||
| // SQLite (better-sqlite3) uses a single writer connection. | ||
| // Running both batch transactions in parallel causes nested transaction errors. | ||
| await createOrUpdateFolderByBatch({ folders: remoteFolders }); | ||
| await createOrUpdateFileByBatch({ files: remoteFiles }); | ||
| await folderRepository.deleteMatchingPartial({ parentId: folder.id }); | ||
| await fileRepository.deleteMatchingPartial({ folderId: folder.id }); | ||
|
|
||
| const localFolders = data.children.filter(isExistingFolder).map((child) => { | ||
| return createFolderFromServerFolder( | ||
| { | ||
| bucket: child.bucket ?? null, | ||
| createdAt: child.createdAt, | ||
| id: child.id, | ||
| name: child.name, | ||
| parentId: child.parentId, | ||
| updatedAt: child.updatedAt, | ||
| plain_name: child.plainName, | ||
| status: ServerFolderStatus.EXISTS, | ||
| uuid: child.uuid, | ||
| }, | ||
| joinVirtualPath(folder.path, child.plainName), | ||
| ); | ||
| }); | ||
|
|
||
| const localFiles = data.files.filter(isExistingFile).map((child) => { | ||
| return createFileFromServerFile( | ||
| { | ||
| bucket: child.bucket, | ||
| createdAt: child.createdAt, | ||
| encrypt_version: child.encryptVersion, | ||
| fileId: child.fileId ?? '', | ||
| folderId: child.folderId, | ||
| id: child.id, | ||
| modificationTime: child.modificationTime ?? child.updatedAt, | ||
| name: child.name, | ||
| size: Number.parseInt(String(child.size), 10), | ||
| type: child.type, | ||
| updatedAt: child.updatedAt, | ||
| userId: child.userId, | ||
| status: ServerFileStatus.EXISTS, | ||
| plainName: child.plainName, | ||
| uuid: child.uuid, | ||
| }, | ||
| joinVirtualPath(folder.path, buildFileName(child)), | ||
| ); | ||
| }); | ||
|
|
||
| await Promise.all([ | ||
| ...localFolders.map((child) => folderRepository.add(child)), | ||
| ...localFiles.map((child) => fileRepository.upsert(child)), | ||
| ]); | ||
|
|
||
| await directoryStateRepository.markLoaded({ folderId: folder.id, statusScope }); | ||
| } catch (error) { | ||
| await directoryStateRepository.markError({ folderId: folder.id, statusScope }); | ||
| logger.error({ msg: '[FUSE - Lazy] Failed to fetch folder children', error, path: folder.path }); | ||
| throw new FuseError(FuseCodes.EIO, `[FUSE - Lazy] Unable to hydrate path: ${folder.path}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: check FolderRepository's add/upsert contract to see if a non-destructive reconciliation is possible
ast-grep run --pattern 'add($_) { $$$ }' --lang typescript src/context/virtual-drive/folders
rg -n "add\(|upsert\(" src/context/virtual-drive/folders/domain/FolderRepository.ts src/context/virtual-drive/folders/infrastructure/InMemoryFolderRepository.tsRepository: internxt/drive-desktop-linux
Length of output: 2008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files first.
git ls-files | rg '^src/(backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator\.ts|context/virtual-drive/folders/.*)$' || true
# Show the relevant sections if the files exist.
for f in \
src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts \
src/context/virtual-drive/folders/domain/FolderRepository.ts \
src/context/virtual-drive/folders/infrastructure/InMemoryFolderRepository.ts
do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
sed -n '1,260p' "$f"
fi
doneRepository: internxt/drive-desktop-linux
Length of output: 1979
Avoid deleting children before the replacement set is durable
If mapping or the final Promise.all fails after these deletes, the folder is left empty in the local store until a later hydration repopulates it. Write the new entries first, then remove stale ones.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/features/virtual-drive/services/lazy/LazyVirtualDriveHydrator.ts`
around lines 127 - 196, The delete order in fetchAndStoreChildren is unsafe
because folderRepository.deleteMatchingPartial and
fileRepository.deleteMatchingPartial run before the new local children are fully
persisted. Update fetchAndStoreChildren to write the replacement folders/files
first using createOrUpdateFolderByBatch, createOrUpdateFileByBatch, and the
Promise.all add/upsert step, then remove stale rows afterward. Keep the existing
mapping logic and error handling, but make sure the durable insert/update
completes before any delete calls.
Comment on lines
+40
to
+56
| async function enqueue({ temporalFile, path, processName }: Props) { | ||
| if (queuedPaths.has(path)) { | ||
| logger.debug({ msg: '[UploadQueue] Path already queued', path, processName }); | ||
| return; | ||
| } | ||
|
|
||
| const stagedTemporalFile = await repository.stage(temporalFile.path, PATHS.UPLOAD_QUEUE); | ||
|
|
||
| tasks.push({ | ||
| temporalFile: stagedTemporalFile, | ||
| path, | ||
| processName, | ||
| }); | ||
| queuedPaths.add(path); | ||
|
|
||
| void drain(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race condition: dedup check and queuedPaths.add are split by an await, allowing duplicate staged uploads for the same path.
queuedPaths.has(path) is checked synchronously, but queuedPaths.add(path) only happens after await repository.stage(...) resolves (Line 53). If enqueue is called twice for the same path in quick succession (e.g., rapid FUSE release events), both calls can pass the has() check before either adds to the set, resulting in two staged copies and two uploads for the same file.
🔒 Proposed fix: reserve the path before the await
async function enqueue({ temporalFile, path, processName }: Props) {
if (queuedPaths.has(path)) {
logger.debug({ msg: '[UploadQueue] Path already queued', path, processName });
return;
}
- const stagedTemporalFile = await repository.stage(temporalFile.path, PATHS.UPLOAD_QUEUE);
-
- tasks.push({
- temporalFile: stagedTemporalFile,
- path,
- processName,
- });
- queuedPaths.add(path);
+ queuedPaths.add(path);
+
+ try {
+ const stagedTemporalFile = await repository.stage(temporalFile.path, PATHS.UPLOAD_QUEUE);
+ tasks.push({ temporalFile: stagedTemporalFile, path, processName });
+ } catch (error) {
+ queuedPaths.delete(path);
+ throw error;
+ }
void drain();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function enqueue({ temporalFile, path, processName }: Props) { | |
| if (queuedPaths.has(path)) { | |
| logger.debug({ msg: '[UploadQueue] Path already queued', path, processName }); | |
| return; | |
| } | |
| const stagedTemporalFile = await repository.stage(temporalFile.path, PATHS.UPLOAD_QUEUE); | |
| tasks.push({ | |
| temporalFile: stagedTemporalFile, | |
| path, | |
| processName, | |
| }); | |
| queuedPaths.add(path); | |
| void drain(); | |
| } | |
| async function enqueue({ temporalFile, path, processName }: Props) { | |
| if (queuedPaths.has(path)) { | |
| logger.debug({ msg: '[UploadQueue] Path already queued', path, processName }); | |
| return; | |
| } | |
| queuedPaths.add(path); | |
| try { | |
| const stagedTemporalFile = await repository.stage(temporalFile.path, PATHS.UPLOAD_QUEUE); | |
| tasks.push({ temporalFile: stagedTemporalFile, path, processName }); | |
| } catch (error) { | |
| queuedPaths.delete(path); | |
| throw error; | |
| } | |
| void drain(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploadQueue.ts`
around lines 40 - 56, The deduplication in TemporalFileUploadQueue.enqueue is
vulnerable to a race because queuedPaths is only updated after the await on
repository.stage, allowing duplicate uploads for the same path. Reserve the path
in queuedPaths before calling repository.stage in enqueue, then proceed to stage
and push the task using the existing enqueue, drain, and queuedPaths symbols so
concurrent calls can no longer pass the has check before the path is marked.
Comment on lines
+107
to
+118
| async deleteMatchingPartial(partial: Partial<FileAttributes>): Promise<void> { | ||
| const keys = Object.keys(partial) as Array<keyof Partial<FileAttributes>>; | ||
|
|
||
| for (const [uuid, attributes] of this.filesByUuid.entries()) { | ||
| const matches = keys.every((key: keyof FileAttributes) => attributes[key] === partial[key]); | ||
|
|
||
| if (matches) { | ||
| this.filesByUuid.delete(uuid); | ||
| this.filesByContentsId.delete(attributes.contentsId); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent contentsId comparison vs matchingPartial.
matchingPartial normalizes contentsId before comparing (Line 67), but deleteMatchingPartial uses strict === for all keys, including contentsId. This can cause deletions to silently miss entries that matchingPartial would otherwise match.
🐛 Proposed fix to mirror the normalize special-case
async deleteMatchingPartial(partial: Partial<FileAttributes>): Promise<void> {
const keys = Object.keys(partial) as Array<keyof Partial<FileAttributes>>;
for (const [uuid, attributes] of this.filesByUuid.entries()) {
- const matches = keys.every((key: keyof FileAttributes) => attributes[key] === partial[key]);
+ const matches = keys.every((key: keyof FileAttributes) => {
+ if (key === 'contentsId') {
+ return attributes[key].normalize() === (partial[key] as string).normalize();
+ }
+ return attributes[key] === partial[key];
+ });
if (matches) {
this.filesByUuid.delete(uuid);
this.filesByContentsId.delete(attributes.contentsId);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async deleteMatchingPartial(partial: Partial<FileAttributes>): Promise<void> { | |
| const keys = Object.keys(partial) as Array<keyof Partial<FileAttributes>>; | |
| for (const [uuid, attributes] of this.filesByUuid.entries()) { | |
| const matches = keys.every((key: keyof FileAttributes) => attributes[key] === partial[key]); | |
| if (matches) { | |
| this.filesByUuid.delete(uuid); | |
| this.filesByContentsId.delete(attributes.contentsId); | |
| } | |
| } | |
| } | |
| async deleteMatchingPartial(partial: Partial<FileAttributes>): Promise<void> { | |
| const keys = Object.keys(partial) as Array<keyof Partial<FileAttributes>>; | |
| for (const [uuid, attributes] of this.filesByUuid.entries()) { | |
| const matches = keys.every((key: keyof FileAttributes) => { | |
| if (key === 'contentsId') { | |
| return attributes[key].normalize() === (partial[key] as string).normalize(); | |
| } | |
| return attributes[key] === partial[key]; | |
| }); | |
| if (matches) { | |
| this.filesByUuid.delete(uuid); | |
| this.filesByContentsId.delete(attributes.contentsId); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/virtual-drive/files/infrastructure/InMemoryFileRepository.ts`
around lines 107 - 118, deleteMatchingPartial in InMemoryFileRepository should
use the same contentsId normalization behavior as matchingPartial, since strict
equality can miss entries that should match. Update the comparison logic inside
deleteMatchingPartial to special-case contentsId the same way matchingPartial
does, while keeping the rest of the key checks unchanged, so the two methods
stay consistent.
… safe-readdir tests
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is Changed / Added
opendir,getattr,open,release) to work with lazy hydration and the new sync behavior.Why
Summary by CodeRabbit
New Features
Bug Fixes
open,opendir, andgetAttributesso paths resolve more consistently.