diff --git a/src/shared/local/localShadowRepo.ts b/src/shared/local/localShadowRepo.ts index 625ab2fc..8f1bd9bd 100644 --- a/src/shared/local/localShadowRepo.ts +++ b/src/shared/local/localShadowRepo.ts @@ -17,9 +17,11 @@ import path from 'node:path'; import * as os from 'node:os'; import * as fs from 'graceful-fs'; -import { NamedPackageDir, Lifecycle, Logger, SfError } from '@salesforce/core'; +import { NamedPackageDir, Lifecycle, Logger, SfError, lockInit } from '@salesforce/core'; import { env } from '@salesforce/kit'; import git from 'isomorphic-git'; +import { GitIndexManager } from 'isomorphic-git/managers'; +import { FileSystem as IsoGitFS } from 'isomorphic-git/models'; import { RegistryAccess } from '@salesforce/source-deploy-retrieve'; import { chunkArray, excludeLwcLocalOnlyTest, folderContainsPath } from '../functions'; import { filenameMatchesToMap, getLogMessage, getMatches } from './moveDetection'; @@ -79,12 +81,15 @@ export class ShadowRepo { private status!: StatusRow[]; private logger!: Logger; private readonly registry: RegistryAccess; + private lockHeld = false; + private readonly indexLockPath: string; private constructor(options: ShadowRepoOptions) { this.gitDir = getGitDir(options.orgId, options.projectPath); this.projectPath = options.projectPath; this.packageDirs = options.packageDirs.map(packageDirToRelativePosixPath(options.projectPath)); this.registry = options.registry; + this.indexLockPath = path.join(this.gitDir, 'index'); } // think of singleton behavior but unique to the projectPath @@ -113,12 +118,14 @@ export class ShadowRepo { */ public async gitInit(): Promise { this.logger.trace(`initializing git repo at ${this.gitDir}`); - await fs.promises.mkdir(this.gitDir, { recursive: true }); - try { - await git.init({ fs, dir: this.projectPath, gitdir: this.gitDir, defaultBranch: 'main' }); - } catch (e) { - redirectToCliRepoError(e); - } + await this.withGitLock(async () => { + await fs.promises.mkdir(this.gitDir, { recursive: true }); + try { + await git.init({ fs, dir: this.projectPath, gitdir: this.gitDir, defaultBranch: 'main' }); + } catch (e) { + redirectToCliRepoError(e); + } + }); } /** @@ -127,8 +134,10 @@ export class ShadowRepo { * @returns the deleted directory */ public async delete(): Promise { - await fs.promises.rm(this.gitDir, { recursive: true, force: true }); - return this.gitDir; + return this.withGitLock(async () => { + await fs.promises.rm(this.gitDir, { recursive: true, force: true }); + return this.gitDir; + }); } /** * If the status already exists, return it. Otherwise, set the status before returning. @@ -142,32 +151,35 @@ export class ShadowRepo { this.logger.trace(`start: getStatus (noCache = ${noCache})`); if (!this.status || noCache) { - try { - // status hasn't been initialized yet - this.status = await git.statusMatrix({ - fs, - dir: this.projectPath, - gitdir: this.gitDir, - filepaths: this.packageDirs, - ignored: true, - filter: fileFilter(this.packageDirs), - }); + // lock reads too: a concurrent write could leave a partially-written index that statusMatrix would misparse + await this.withGitLock(async () => { + try { + // status hasn't been initialized yet + this.status = await git.statusMatrix({ + fs, + dir: this.projectPath, + gitdir: this.gitDir, + filepaths: this.packageDirs, + ignored: true, + filter: fileFilter(this.packageDirs), + }); - // isomorphic-git stores things in unix-style tree. Convert to windows-style if necessary - if (IS_WINDOWS) { - this.status = this.status.map((row) => [path.normalize(row[FILE]), row[HEAD], row[WORKDIR], row[3]]); - } + // isomorphic-git stores things in unix-style tree. Convert to windows-style if necessary + if (IS_WINDOWS) { + this.status = this.status.map((row) => [path.normalize(row[FILE]), row[HEAD], row[WORKDIR], row[3]]); + } - if (env.getBoolean('SF_DISABLE_SOURCE_MOBILITY') === true) { - await Lifecycle.getInstance().emitTelemetry({ eventName: 'moveFileDetectionDisabled' }); - } else { - // Check for moved files and update local git status accordingly - await Lifecycle.getInstance().emitTelemetry({ eventName: 'moveFileDetectionEnabled' }); - await this.detectMovedFiles(); + if (env.getBoolean('SF_DISABLE_SOURCE_MOBILITY') === true) { + await Lifecycle.getInstance().emitTelemetry({ eventName: 'moveFileDetectionDisabled' }); + } else { + // Check for moved files and update local git status accordingly + await Lifecycle.getInstance().emitTelemetry({ eventName: 'moveFileDetectionEnabled' }); + await this.detectMovedFiles(); + } + } catch (e) { + redirectToCliRepoError(e); } - } catch (e) { - redirectToCliRepoError(e); - } + }); } this.logger.trace(`done: getStatus (noCache = ${noCache})`); return this.status; @@ -248,77 +260,131 @@ export class ShadowRepo { return 'no files to commit'; } - if (deployedFiles.length) { - const chunks = chunkArray( + return this.withGitLock(async () => { + // Phase 1: Write blob objects and collect file metadata. + // Blob writes go to .git/objects/ and are independent of the index. + // Chunked to avoid EMFILE (too many open files). + const insertions: Array<{ filepath: string; stats: fs.Stats; oid: string }> = []; + + if (deployedFiles.length) { // these are stored in posix/style/path format. We have to convert inbound stuff from windows - [...new Set(IS_WINDOWS ? deployedFiles.map(normalize).map(ensurePosix) : deployedFiles)], - MAX_FILE_ADD - ); - for (const chunk of chunks) { - try { - this.logger.debug(`adding ${chunk.length} files of ${deployedFiles.length} deployedFiles to git`); - // these need to be done sequentially (it's already batched) because isogit manages file locking + const uniqueFiles = [...new Set(IS_WINDOWS ? deployedFiles.map(normalize).map(ensurePosix) : deployedFiles)]; + const chunks = chunkArray(uniqueFiles, MAX_FILE_ADD); + + for (const chunk of chunks) { + this.logger.debug(`writing ${chunk.length} blobs of ${uniqueFiles.length} deployedFiles`); // eslint-disable-next-line no-await-in-loop - await git.add({ - fs, - dir: this.projectPath, - gitdir: this.gitDir, - filepath: chunk, - force: true, - }); - } catch (e) { - if (e instanceof git.Errors.MultipleGitError) { - this.logger.error(`${e.errors.length} errors on git.add, showing the first 5:`, e.errors.slice(0, 5)); + const settled = await Promise.allSettled( + chunk.map(async (filepath) => { + const fullPath = path.join(this.projectPath, filepath); + const stats = await fs.promises.lstat(fullPath); + const fileBuffer = await fs.promises.readFile(fullPath); + const oid = await git.writeBlob({ + fs, + dir: this.projectPath, + gitdir: this.gitDir, + blob: fileBuffer, + }); + return { filepath, stats, oid }; + }) + ); + + // Mirror isomorphic-git's addToIndex: allSettled -> aggregate errors + const rejected = settled + .filter((r): r is PromiseRejectedResult => r.status === 'rejected') + .map((r) => r.reason as Error); + if (rejected.length > 1) { + this.logger.error(`${rejected.length} errors writing blobs, showing the first 5:`, rejected.slice(0, 5)); throw SfError.create({ - message: e.message, - name: e.name, - data: e.errors.map((err) => err.message), - cause: e, + message: `Multiple errors occurred writing blob objects (${rejected.length} failures)`, + name: 'MultipleGitError', + data: rejected.map((err) => err.message), + cause: rejected[0], actions: [ `One potential reason you're getting this error is that the number of files that source tracking is batching exceeds your user-specific file limits. Increase your hard file limit in the same session by executing 'ulimit -Hn ${MAX_FILE_ADD}'. Or set the 'SFDX_SOURCE_TRACKING_BATCH_SIZE' environment variable to a value lower than the output of 'ulimit -Hn'.\nNote: Don't set this environment variable too close to the upper limit or your system will still hit it. If you continue to get the error, lower the value of the environment variable even more.`, ], }); } - redirectToCliRepoError(e); + if (rejected.length === 1) { + redirectToCliRepoError(rejected[0]); + } + + insertions.push( + ...settled + .filter( + (r): r is PromiseFulfilledResult<{ filepath: string; stats: fs.Stats; oid: string }> => + r.status === 'fulfilled' + ) + .map((r) => r.value) + ); } } - } - if (deletedFiles.length) { - // Using a cache here speeds up the performance by ~24.4% - let cache = {}; + const deletions = deletedFiles.length + ? [...new Set(IS_WINDOWS ? deletedFiles.map(normalize).map(ensurePosix) : deletedFiles)] + : []; + + // Phase 2: Single index update — one GitIndexManager.acquire() call reads the index, + // applies all inserts and deletes in memory, and writes it back once when the callback resolves. + if (insertions.length || deletions.length) { + const isoGitFS = new IsoGitFS(fs); + await GitIndexManager.acquire({ fs: isoGitFS, gitdir: this.gitDir, cache: {} }, (index) => { + for (const { filepath, stats, oid } of insertions) { + index.insert({ filepath, stats, oid }); + } + for (const filepath of deletions) { + index.delete({ filepath }); + } + }); + } - for (const filepath of [...new Set(IS_WINDOWS ? deletedFiles.map(normalize).map(ensurePosix) : deletedFiles)]) { - try { - // these need to be done sequentially because isogit manages file locking. Isogit remove does not support multiple files at once - // eslint-disable-next-line no-await-in-loop - await git.remove({ fs, dir: this.projectPath, gitdir: this.gitDir, filepath, cache }); - } catch (e) { - redirectToCliRepoError(e); + try { + this.logger.trace('start: commitChanges git.commit'); + + const sha = await git.commit({ + fs, + dir: this.projectPath, + gitdir: this.gitDir, + message, + author: { name: 'sfdx source tracking' }, + }); + // status changed as a result of the commit. This prevents users from having to run getStatus(true) to avoid cache + if (needsUpdatedStatus) { + await this.getStatus(true); } + this.logger.trace('done: commitChanges git.commit'); + return sha; + } catch (e) { + redirectToCliRepoError(e); } - // clear cache - cache = {}; + }); + } + + /** + * Cross-process file lock on the git index using proper-lockfile (via @salesforce/core lockInit). + * The lockHeld flag provides reentrancy within the same process, needed because + * getStatus() -> detectMovedFiles() -> commitChanges() -> getStatus(true). + * Cross-process coordination is handled by proper-lockfile's mkdir-based lock. + */ + private async withGitLock(operation: () => Promise): Promise { + if (this.lockHeld) { + return operation(); } + this.logger.trace('acquiring git index lock'); + const { unlock } = await lockInit(this.indexLockPath); + this.lockHeld = true; try { - this.logger.trace('start: commitChanges git.commit'); - - const sha = await git.commit({ - fs, - dir: this.projectPath, - gitdir: this.gitDir, - message, - author: { name: 'sfdx source tracking' }, - }); - // status changed as a result of the commit. This prevents users from having to run getStatus(true) to avoid cache - if (needsUpdatedStatus) { - await this.getStatus(true); + return await operation(); + } finally { + this.lockHeld = false; + try { + await unlock(); + this.logger.trace('released git index lock'); + } catch (e) { + // unlock can fail if gitDir was deleted (e.g., by delete()), which is expected + this.logger.trace('could not release git index lock (lock dir may have been removed)', e); } - this.logger.trace('done: commitChanges git.commit'); - return sha; - } catch (e) { - redirectToCliRepoError(e); } } diff --git a/test/unit/localDetectMovedFiles.test.ts b/test/unit/localDetectMovedFiles.test.ts index 2f515558..9f758b88 100644 --- a/test/unit/localDetectMovedFiles.test.ts +++ b/test/unit/localDetectMovedFiles.test.ts @@ -58,14 +58,14 @@ describe('local detect moved files', () => { registry, }); - const gitAdd = sinon.spy(git, 'add'); + const gitCommit = sinon.spy(git, 'commit'); // Manually commit this first file const labelsFile = path.join('force-app', 'labels', 'CustomLabels.labels-meta.xml'); const labelsFileTwo = path.join('force-app', 'labels', 'CustomLabelsTwo.labels-meta.xml'); const sha = await shadowRepo.commitChanges({ deployedFiles: [labelsFile, labelsFileTwo] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + expect(gitCommit.calledOnce).to.be.true; // Move the file and refresh the status fs.renameSync( @@ -80,7 +80,7 @@ describe('local detect moved files', () => { await shadowRepo.getStatus(true); // Moved file should have been detected and committed - expect(gitAdd.calledTwice).to.be.true; + expect(gitCommit.calledTwice).to.be.true; expect(await shadowRepo.getChangedRows()).to.be.empty; } finally { if (projectDir) await fs.promises.rm(projectDir, { recursive: true }); @@ -111,13 +111,13 @@ describe('local detect moved files', () => { registry, }); - const gitAdd = sinon.spy(git, 'add'); + const gitCommit = sinon.spy(git, 'commit'); // Manually commit this first file const labelsFile = path.join('force-app', 'labels', 'CustomLabels.labels-meta.xml'); const sha = await shadowRepo.commitChanges({ deployedFiles: [labelsFile] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + expect(gitCommit.calledOnce).to.be.true; // Move the file and refresh the status fs.renameSync( @@ -128,7 +128,7 @@ describe('local detect moved files', () => { await shadowRepo.getStatus(true); // Moved file should NOT have been detected and committed - expect(gitAdd.calledTwice).to.be.false; + expect(gitCommit.calledTwice).to.be.false; expect(await shadowRepo.getChangedRows()).to.have.lengthOf(2); } finally { if (projectDir) await fs.promises.rm(projectDir, { recursive: true }); @@ -176,14 +176,14 @@ describe('local detect moved files', () => { registry, }); - const gitAdd = sinon.spy(git, 'add'); + const gitCommit = sinon.spy(git, 'commit'); // Manually commit the two files first const singleMatchFile = path.join('force-app', 'labels', 'CustomLabelsSingleMatch.labels-meta.xml'); const multiMatchFile = path.join('force-app', 'labels', 'CustomLabelsMultiMatch.labels-meta.xml'); const sha = await shadowRepo.commitChanges({ deployedFiles: [singleMatchFile, multiMatchFile] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + expect(gitCommit.calledOnce).to.be.true; // For the multi-match file, copy it to two different directories and then rename it // The reason we create 3 new copies is to ensure the added/deletedIgnoredSet is working correctly @@ -210,7 +210,7 @@ describe('local detect moved files', () => { await shadowRepo.getStatus(true); // The single moved file should have been detected and committed - expect(gitAdd.calledTwice).to.be.true; + expect(gitCommit.calledTwice).to.be.true; // However, the ones with multiple matches should have been ignored // - Expect getChangedRows to return 4 rows. 1 deleted and 3 added expect(await shadowRepo.getChangedRows()).to.have.lengthOf(4); @@ -245,13 +245,13 @@ describe('local detect moved files', () => { registry, }); - const gitAdd = sinon.spy(git, 'add'); + const gitCommit = sinon.spy(git, 'commit'); // Manually commit this first file const labelsFile = path.join('force-app', 'labels', 'CustomLabels.labels-meta.xml'); const sha = await shadowRepo.commitChanges({ deployedFiles: [labelsFile] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + expect(gitCommit.calledOnce).to.be.true; // Move the file fs.renameSync( @@ -263,8 +263,8 @@ describe('local detect moved files', () => { // Refresh the status await shadowRepo.getStatus(true); - // delete is detected and committed, but the add is still considered a change - expect(gitAdd.calledOnce).to.be.true; + // delete is detected and committed (second commit), but the add is still considered a change + expect(gitCommit.calledTwice).to.be.true; expect(await shadowRepo.getDeletes()).to.have.lengthOf(0); expect(await shadowRepo.getAdds()).to.have.lengthOf(1); } finally { @@ -301,12 +301,12 @@ describe('local detect moved files', () => { registry, }); - const gitAdd = sinon.spy(git, 'add'); + const gitCommit = sinon.spy(git, 'commit'); // Manually commit the files first const sha = await shadowRepo.commitChanges({ deployedFiles: [moveFile, modifyFile, deleteFile] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + expect(gitCommit.calledOnce).to.be.true; // Move the file this file fs.renameSync( @@ -324,7 +324,7 @@ describe('local detect moved files', () => { await shadowRepo.getStatus(true); // Moved file should have been detected and committed, leaving the remaining changes - expect(gitAdd.calledTwice).to.be.true; + expect(gitCommit.calledTwice).to.be.true; expect(await shadowRepo.getAddFilenames()).to.have.lengthOf(1); expect(await shadowRepo.getAddFilenames()).to.have.members([addFile]); expect(await shadowRepo.getDeleteFilenames()).to.have.lengthOf(1); diff --git a/test/unit/localShadowRepo.test.ts b/test/unit/localShadowRepo.test.ts index 7d59a0e8..fe3c30a6 100644 --- a/test/unit/localShadowRepo.test.ts +++ b/test/unit/localShadowRepo.test.ts @@ -29,6 +29,28 @@ afterEach(() => { sinon.restore(); }); +/** Helper: create a temp project dir with a ShadowRepo instance */ +const setupShadowRepo = async (orgId = '00D456789012345'): Promise<{ projectDir: string; shadowRepo: ShadowRepo }> => { + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'localShadowRepoTest')); + fs.mkdirSync(path.join(projectDir, 'force-app')); + fs.writeFileSync(path.join(projectDir, 'force-app', 'CustomLabels.labels-meta.xml'), ''); + + const shadowRepo = await ShadowRepo.getInstance({ + orgId, + registry: new RegistryAccess(), + projectPath: projectDir, + packageDirs: [ + { + name: 'dummy', + fullPath: path.join(projectDir, 'force-app'), + path: path.join(projectDir, 'force-app'), + }, + ], + }); + + return { projectDir, shadowRepo }; +}; + describe('localShadowRepo', () => { const registry = new RegistryAccess(); it('does not add same file multiple times', async () => { @@ -51,15 +73,165 @@ describe('localShadowRepo', () => { ], }); - const gitAdd = sinon.spy(git, 'add'); + const writeBlob = sinon.spy(git, 'writeBlob'); const labelsFile = path.join('force-app', 'CustomLabels.labels-meta.xml'); const sha = await shadowRepo.commitChanges({ deployedFiles: [labelsFile, labelsFile] }); expect(sha).to.not.be.empty; - expect(gitAdd.calledOnce).to.be.true; + // Deduplication via Set means only one blob write despite duplicate input + expect(writeBlob.calledOnce).to.be.true; } finally { if (projectDir) await fs.promises.rm(projectDir, { recursive: true }); } }); }); + +describe('git index locking', () => { + it('holds lock during commitChanges', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_commit'); + + try { + let lockExistedDuringAdd = false; + const lockDir = path.join(shadowRepo.gitDir, 'index.lock'); + + const origWriteBlob = git.writeBlob.bind(git); + sinon.stub(git, 'writeBlob').callsFake(async (args) => { + lockExistedDuringAdd = fs.existsSync(lockDir); + return origWriteBlob(args); + }); + + const labelsFile = path.join('force-app', 'CustomLabels.labels-meta.xml'); + await shadowRepo.commitChanges({ deployedFiles: [labelsFile] }); + + expect(lockExistedDuringAdd).to.be.true; + // lock should be released after the operation + expect(fs.existsSync(lockDir)).to.be.false; + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('holds lock during getStatus', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_status'); + + try { + let lockExistedDuringStatus = false; + const lockDir = path.join(shadowRepo.gitDir, 'index.lock'); + + const origStatusMatrix = git.statusMatrix.bind(git); + sinon.stub(git, 'statusMatrix').callsFake(async (args) => { + lockExistedDuringStatus = fs.existsSync(lockDir); + return origStatusMatrix(args); + }); + + await shadowRepo.getStatus(true); + + expect(lockExistedDuringStatus).to.be.true; + expect(fs.existsSync(lockDir)).to.be.false; + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('skips locking for empty commitChanges (no files)', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_empty'); + + try { + const result = await shadowRepo.commitChanges({ deployedFiles: [], deletedFiles: [] }); + + expect(result).to.equal('no files to commit'); + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('skips locking for cached getStatus', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_cache'); + + try { + // First call populates cache + await shadowRepo.getStatus(); + + let lockAcquiredOnSecondCall = false; + const lockDir = path.join(shadowRepo.gitDir, 'index.lock'); + + const origStatusMatrix = git.statusMatrix.bind(git); + sinon.stub(git, 'statusMatrix').callsFake(async (args) => { + lockAcquiredOnSecondCall = fs.existsSync(lockDir); + return origStatusMatrix(args); + }); + + // Second call (cached) should not re-acquire lock or call statusMatrix + await shadowRepo.getStatus(); + expect(lockAcquiredOnSecondCall).to.be.false; + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('does not deadlock on reentrant getStatus -> commitChanges chain', async () => { + // This tests the reentrancy path: getStatus() -> detectMovedFiles() -> commitChanges() + // If locking were not reentrant, this would deadlock. + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_reentrant'); + + try { + fs.mkdirSync(path.join(projectDir, 'force-app', 'new', 'labels'), { recursive: true }); + fs.mkdirSync(path.join(projectDir, 'force-app', 'labels'), { recursive: true }); + fs.writeFileSync(path.join(projectDir, 'force-app', 'labels', 'CustomLabels.labels-meta.xml'), ''); + + // Commit the file so it's tracked + const labelsFile = path.join('force-app', 'labels', 'CustomLabels.labels-meta.xml'); + await shadowRepo.commitChanges({ deployedFiles: [labelsFile] }); + + // Move the file, which triggers detectMovedFiles -> commitChanges inside getStatus + fs.renameSync( + path.join(projectDir, labelsFile), + path.join(projectDir, 'force-app', 'new', 'labels', 'CustomLabels.labels-meta.xml') + ); + + // This must complete without deadlocking + const status = await shadowRepo.getStatus(true); + expect(status).to.be.an('array'); + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('releases lock when operation throws', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_throw'); + + try { + const lockDir = path.join(shadowRepo.gitDir, 'index.lock'); + + // Commit a non-existent file to trigger an error inside the lock + try { + await shadowRepo.commitChanges({ deployedFiles: ['force-app/does-not-exist.cls'] }); + } catch { + // expected + } + + // Lock should have been released + expect(fs.existsSync(lockDir)).to.be.false; + + // A second operation should succeed (not blocked by a stale lock) + const status = await shadowRepo.getStatus(true); + expect(status).to.be.an('array'); + } finally { + await fs.promises.rm(projectDir, { recursive: true }); + } + }); + + it('delete removes gitDir and survives lock cleanup', async () => { + const { projectDir, shadowRepo } = await setupShadowRepo('00Dlock_delete'); + + try { + const deletedDir = await shadowRepo.delete(); + + expect(deletedDir).to.equal(shadowRepo.gitDir); + expect(fs.existsSync(shadowRepo.gitDir)).to.be.false; + } finally { + await fs.promises.rm(projectDir, { recursive: true, force: true }); + } + }); +});