fix: improve FTPClient#304
Conversation
… in batches to increase reliability
WalkthroughThis PR serializes FTP operations via a new SafeFTPClient (PQueue), batches multi-step FTP calls, tightens FTP client cache keying and mismatch handling, conditionally clears package-removal state, and skips JSON writes when missing file + undefined result. ChangesFTP Client Queuing and Handler Integration
JSON File Write Optimization
Sequence DiagramssequenceDiagram
participant FTPClient
participant SafeFTPClient
participant PQueue
participant FTPBase["FTP.Client"]
FTPClient->>SafeFTPClient: call batch(callback)
SafeFTPClient->>PQueue: add task to queue
PQueue->>FTPBase: await FTP operation (size/cd/etc)
FTPBase-->>PQueue: result or error
PQueue-->>SafeFTPClient: return result with augmented stack
SafeFTPClient-->>FTPClient: batch callback completes
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🧹 Nitpick comments (2)
shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts (2)
121-154: ⚡ Quick winRedundant
sizecall and TOCTOU window between existence check and batch.
_fileExistsalready issues a queuedsizecall, then this method opens a freshbatchthat issues anothersizecall against the same path. That doubles the round trips and queue entries, and because the two queue entries are not contiguous, another caller's queued operation can run between them — so the file may no longer exist (or may have changed) when the batch executes. The same pattern exists inremoveDirIfExists(Line 287) andupload/uploadContentrely oninithaving connected but not on path state.Consider folding the existence check into the single batch so the entire sequence is atomic from the queue's perspective, and
sizeis fetched once:♻️ Proposed refactor to consolidate into a single batch
async getFileInfo(fullPath: string): Promise<FileInfoReturnType> { await this.init() // Ensure the client is connected this.logger.silly(`Getting file info for: ${fullPath}`) - const exists = await this._fileExists(fullPath) // Ensure the file exists before trying to get its info - - if (!exists.exists) - return { - success: false, - knownReason: exists.knownReason, - reason: exists.reason, - - packageExists: false, - } - return this.client.batch(async (ftpClient) => { - const size = await ftpClient.size(fullPath) + let size: number + try { + size = await ftpClient.size(fullPath) + } catch (e) { + if (e instanceof FTP.FTPError) { + return { + success: false, + knownReason: e.code === 550, + reason: + e.code === 550 + ? { user: `File not found`, tech: `File "${fullPath}" not found on FTP server ([${e.code}]: ${e.message})` } + : { user: `Error response from FTP Server`, tech: `FTP Server: [${e.code}]: ${e.message}` }, + packageExists: false, + } + } + throw e + } let modDate: Date | undefined = undefined try { modDate = await ftpClient.lastMod(fullPath) } catch { // This is not supported by every FTP server, ignore any error } return { success: true, fileInfo: { size, modified: modDate ? modDate.getTime() : 0, }, } }) }🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts` around lines 121 - 154, getFileInfo currently calls _fileExists (which queues a size) and then opens a separate this.client.batch that calls size again, creating duplicate requests and a TOCTOU window; refactor getFileInfo to perform the existence check and the size/lastMod retrieval inside a single this.client.batch callback (use the ftpClient instance to run size and lastMod once, return the same FileInfoReturnType), removing the external _fileExists call; apply the same consolidation pattern to removeDirIfExists and ensure upload/uploadContent do not rely on path-state checks performed outside init/batch so all path-dependent operations run atomically inside the queue batch (refer to getFileInfo, _fileExists, this.client.batch, removeDirIfExists, upload, uploadContent).
306-307: 💤 Low valueMark
ftpClientandqueueasreadonly.Both fields are assigned only in the constructor and never reassigned. Aligning with the SonarCloud hints documents intent and prevents accidental reassignment.
♻️ Proposed diff
- private ftpClient: FTP.Client - private queue: PQueue + private readonly ftpClient: FTP.Client + private readonly queue: PQueue🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts` around lines 306 - 307, The ftpClient and queue fields are assigned only in the constructor and should be declared readonly to prevent reassignment; update the class field declarations for ftpClient (FTP.Client) and queue (PQueue) in FTPClient to use the readonly modifier (e.g., change "private ftpClient: FTP.Client" and "private queue: PQueue" to "private readonly ftpClient: FTP.Client" and "private readonly queue: PQueue") and ensure the constructor continues to initialize them as before.
🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/ftp.ts`:
- Around line 496-502: Log messages leak FTP credentials by serializing
FTPOptions (cacheKey, options, cachedClients.options) and in the creation info
log; instead, before calling this.worker.logger.error or .info, build a
sanitized representation that omits or masks sensitive fields (use the existing
safeUrl getter or create a small sanitizer that copies options and replaces
password/username with masked values) and log that sanitized object/URL; update
both the cache-mismatch log (currently referencing
cacheKey/options/cachedClients.options) and the "Creating new FTP client..."
info log to use the sanitized view, and fix the typo "It the options" → "If the
options".
In
`@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts`:
- Around line 329-331: The error construction in putInQueue currently includes
JSON.stringify(args) which can leak credentials when wrappers like
access(...args) (wrapper for FTPInstance.access) are called; update the
implementation so that sensitive data is not serialized — either remove args
from the orgError message, only include the method name, or redact known
sensitive keys (e.g., user, password) before any stringify; you can implement
this by adding a small sanitizer used by putInQueue (or by adding a flag on
calls from access and other wrappers at lines ~360-377) so errors log safe text
only and avoid embedding raw args.
- Around line 165-166: Remove the redundant explicit removal in the upload()
method: the code currently calls ftpClient.remove(fullPath, true) before calling
ftpClient.uploadFrom, but uploadFrom already overwrites existing files; delete
the remove(...) call so upload() behaves consistently with uploadContent(),
keeping uploadFrom(uploadFromPath, fullPath) as the sole operation for writing
the file and preserving expected overwrite semantics in the upload and
uploadContent functions.
---
Nitpick comments:
In
`@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts`:
- Around line 121-154: getFileInfo currently calls _fileExists (which queues a
size) and then opens a separate this.client.batch that calls size again,
creating duplicate requests and a TOCTOU window; refactor getFileInfo to perform
the existence check and the size/lastMod retrieval inside a single
this.client.batch callback (use the ftpClient instance to run size and lastMod
once, return the same FileInfoReturnType), removing the external _fileExists
call; apply the same consolidation pattern to removeDirIfExists and ensure
upload/uploadContent do not rely on path-state checks performed outside
init/batch so all path-dependent operations run atomically inside the queue
batch (refer to getFileInfo, _fileExists, this.client.batch, removeDirIfExists,
upload, uploadContent).
- Around line 306-307: The ftpClient and queue fields are assigned only in the
constructor and should be declared readonly to prevent reassignment; update the
class field declarations for ftpClient (FTP.Client) and queue (PQueue) in
FTPClient to use the readonly modifier (e.g., change "private ftpClient:
FTP.Client" and "private queue: PQueue" to "private readonly ftpClient:
FTP.Client" and "private readonly queue: PQueue") and ensure the constructor
continues to initialize them as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61eef297-c93f-4c91-82d5-b80d93fb5536
📒 Files selected for processing (3)
shared/packages/worker/src/worker/accessorHandlers/ftp.tsshared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.tsshared/packages/worker/src/worker/accessorHandlers/lib/json-write-file.ts
| // It the options don't match, something is wrong with the cacheKey | ||
|
|
||
| this.worker.logger.error( | ||
| `Something is wrong with the FTP client cacheKey. The options do not match the cacheKey. Deleting cached clients for this key. cacheKey: ${cacheKey}, options: ${JSON.stringify( | ||
| options | ||
| )}, cachedOptions: ${JSON.stringify(cachedClients.options)}` | ||
| ) |
There was a problem hiding this comment.
Avoid logging FTP credentials in cache-mismatch error.
cacheKey, options, and cachedClients.options all serialize the FTPOptions object that includes username and password (constructed at lines 476‑484). Emitting these via logger.error writes the plaintext password to logs/log shipping. The same problem exists at the new info log on line 542 (Creating new FTP client for purpose=...). The file already establishes a redaction convention via the safeUrl getter (line 456), so logs should use a sanitized view of the options.
🔒 Proposed fix — redact password before logging (apply to both call sites)
+ const safeOptions = { ...options, password: options.password ? '***' : options.password }
+ const safeCacheKey = JSON.stringify([this.accessorId, safeOptions, this.accessor.basePath ?? '/'])
+
let cachedClients = accessorCache[cacheKey]
if (cachedClients) {
// Check that options matches:
if (!isEqual(cachedClients.options, options)) {
- // It the options don't match, something is wrong with the cacheKey
-
- this.worker.logger.error(
- `Something is wrong with the FTP client cacheKey. The options do not match the cacheKey. Deleting cached clients for this key. cacheKey: ${cacheKey}, options: ${JSON.stringify(
- options
- )}, cachedOptions: ${JSON.stringify(cachedClients.options)}`
- )
+ // If the options don't match, something is wrong with the cacheKey
+ const safeCachedOptions = {
+ ...cachedClients.options,
+ password: cachedClients.options.password ? '***' : cachedClients.options.password,
+ }
+ this.worker.logger.error(
+ `Something is wrong with the FTP client cacheKey. The options do not match the cacheKey. Deleting cached clients for this key. cacheKey: ${safeCacheKey}, options: ${JSON.stringify(
+ safeOptions
+ )}, cachedOptions: ${JSON.stringify(safeCachedOptions)}`
+ )And at line 542:
- this.worker.logger.info(`Creating new FTP client for purpose="${purpose}", ${JSON.stringify(options)}`)
+ this.worker.logger.info(`Creating new FTP client for purpose="${purpose}", ${JSON.stringify(safeOptions)}`)Also consider fixing the typo "It the options" → "If the options" on line 496.
🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/ftp.ts` around lines 496 -
502, Log messages leak FTP credentials by serializing FTPOptions (cacheKey,
options, cachedClients.options) and in the creation info log; instead, before
calling this.worker.logger.error or .info, build a sanitized representation that
omits or masks sensitive fields (use the existing safeUrl getter or create a
small sanitizer that copies options and replaces password/username with masked
values) and log that sanitized object/URL; update both the cache-mismatch log
(currently referencing cacheKey/options/cachedClients.options) and the "Creating
new FTP client..." info log to use the sanitized view, and fix the typo "It the
options" → "If the options".
| // Remove the file if it already exists: | ||
| await ftpClient.remove(fullPath, true) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
basic-ftp Client uploadFrom overwrite existing file behavior
💡 Result:
The basic-ftp Client's uploadFrom method overwrites existing files on the remote FTP server by default. If a file with the specified remotePath already exists, it will be overwritten with the data from the provided readable stream or local file.[1][2][3]
Citations:
- 1: https://github.com/Torge/basic-ftp
- 2: https://github.com/everhardt/basic-ftp
- 3: https://github.com/JaviRpo/basic-ftp
🏁 Script executed:
cat -n shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts | sed -n '155,200p'Repository: Sofie-Automation/sofie-package-manager
Length of output: 1998
Inconsistency: upload removes target before uploadFrom, uploadContent does not.
The upload() method explicitly calls remove(fullPath, true) before uploadFrom, while uploadContent() omits this step. Since uploadFrom overwrites existing files by default (per basic-ftp documentation), the explicit removal in upload() is redundant. Remove the unnecessary remove() call from upload() to align both methods and avoid defensive programming that masks the intended overwrite semantics.
🤖 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
`@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts`
around lines 165 - 166, Remove the redundant explicit removal in the upload()
method: the code currently calls ftpClient.remove(fullPath, true) before calling
ftpClient.uploadFrom, but uploadFrom already overwrites existing files; delete
the remove(...) call so upload() behaves consistently with uploadContent(),
keeping uploadFrom(uploadFromPath, fullPath) as the sole operation for writing
the file and preserving expected overwrite semantics in the upload and
uploadContent functions.
| async access(...args: Parameters<FTPInstance['access']>): ReturnType<FTPInstance['access']> { | ||
| return this.putInQueue('access', args, async () => this.ftpClient.access(...args)) | ||
| } |
There was a problem hiding this comment.
Avoid serializing arbitrary args into error messages — risk of leaking credentials.
putInQueue builds orgError with JSON.stringify(args) and the wrapper exposes access(...) (Line 329), whose argument is an FTP.AccessOptions object containing user and password. Today access is only invoked inside batch via the raw client, so the password is not stringified — but the public wrapper makes it easy for a future caller to invoke safeFtp.access(options) directly, at which point a thrown error will embed the password in the error stack (which typically lands in logs).
Consider either dropping args from the error string, redacting them, or only including the method name:
🛡️ Proposed hardening
- const orgError = new Error(`Error executing ${methodName}: ${JSON.stringify(args)}`) // Used later
+ const orgError = new Error(`Error executing ${methodName}`) // Used later (args omitted to avoid leaking credentials/secrets)Also applies to: 360-377
🤖 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
`@shared/packages/worker/src/worker/accessorHandlers/lib/FTPClient/FTPClient.ts`
around lines 329 - 331, The error construction in putInQueue currently includes
JSON.stringify(args) which can leak credentials when wrappers like
access(...args) (wrapper for FTPInstance.access) are called; update the
implementation so that sensitive data is not serialized — either remove args
from the orgError message, only include the method name, or redact known
sensitive keys (e.g., user, password) before any stringify; you can implement
this by adding a small sanitizer used by putInQueue (or by adding a flag on
calls from access and other wrappers at lines ~360-377) so errors log safe text
only and avoid embedding raw args.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/ftp.ts`:
- Around line 201-204: The FTP handler conditionally skips calling
fileHandler.clearPackageRemoval when this.workOptions.removeDelay is negative,
which leaves stale entries in packagesToRemove; remove the guard so
clearPackageRemoval(this.filePath) is called unconditionally (matching other
handlers like s3/localFolder/httpProxy/fileShare), or if you intentionally want
conditional behavior apply the same guard across all handlers; update the FTP
methods that currently wrap clearPackageRemoval with the
(this.workOptions.removeDelay ?? -1) >= 0 check to instead always call
fileHandler.clearPackageRemoval(this.filePath) (or make the same guard change in
the other handler implementations) so scheduled removals are correctly cancelled
when a package is rewritten.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3de3f86-0f87-4f31-8090-396462ad8a5e
📒 Files selected for processing (1)
shared/packages/worker/src/worker/accessorHandlers/ftp.ts
| if ((this.workOptions.removeDelay ?? -1) >= 0) { | ||
| // Only handle this if there is a removeDelay set, otherwise we can skip it to save time: | ||
| await this.fileHandler.clearPackageRemoval(this.filePath) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'GenericFileOperations*' shared/packages/worker/src/worker/accessorHandlers | while read -r f; do
echo "== $f =="
rg -n -C4 '\b(clearPackageRemoval|handleRemovePackage|removeDuePackages)\b' "$f"
done
echo "== call sites =="
rg -n -C3 '\bclearPackageRemoval\s*\(' shared/packages/worker/src/worker/accessorHandlers
rg -n -C3 '\bhandleRemovePackage\s*\(' shared/packages/worker/src/worker/accessorHandlersRepository: Sofie-Automation/sofie-package-manager
Length of output: 16656
Remove the guard or apply consistently across all file handlers.
The clearPackageRemoval() guard at lines 201-204 and 272-275 creates a behavioral inconsistency. All other handlers (s3, localFolder, httpProxy, fileShare) call clearPackageRemoval() unconditionally in equivalent methods. When FTP skips this due to removeDelay < 0, any stale delayed-removal entry persists in the packagesToRemove list (see GenericFileOperations.ts:41-45) and can still execute later via removeDuePackages(). If a package is scheduled for removal, then re-written with removeDelay = undefined, the stale removal entry will not be canceled. Either remove the guard to match other handlers or apply it to all handlers consistently.
🤖 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 `@shared/packages/worker/src/worker/accessorHandlers/ftp.ts` around lines 201 -
204, The FTP handler conditionally skips calling fileHandler.clearPackageRemoval
when this.workOptions.removeDelay is negative, which leaves stale entries in
packagesToRemove; remove the guard so clearPackageRemoval(this.filePath) is
called unconditionally (matching other handlers like
s3/localFolder/httpProxy/fileShare), or if you intentionally want conditional
behavior apply the same guard across all handlers; update the FTP methods that
currently wrap clearPackageRemoval with the (this.workOptions.removeDelay ?? -1)
>= 0 check to instead always call fileHandler.clearPackageRemoval(this.filePath)
(or make the same guard change in the other handler implementations) so
scheduled removals are correctly cancelled when a package is rewritten.
2 participants
About Me
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a: Code improvement
New Behavior
This PR refactors the FTPClient to further avoid calling multiple operations at the same time.
Also runs some operations in batches, to improve performance and reliability.
Status