Skip to content

⚡ [optimize saveConfig to use asynchronous I/O] #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cmuench wants to merge 2 commits into
base: main
Choose a base branch
from
Open

⚡ [optimize saveConfig to use asynchronous I/O] #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d6a8fb3
⚡ optimize saveConfig to use asynchronous I/O
google-labs-jules[bot] May 8, 2026
c9affed
⚡ optimize saveConfig to use asynchronous I/O (resolved conflicts)
google-labs-jules[bot] May 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions lib/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ async function migrateOldConfig() {
if (fs.existsSync(OLD_CONFIG_FILE)) {
try {
console.log(`Migrating config from ${OLD_CONFIG_FILE} to ${CONFIG_FILE}`);
await mkdirp(CONFIG_DIR);
fs.copyFileSync(OLD_CONFIG_FILE, CONFIG_FILE);
await mkdirp(CONFIG_DIR, { mode: 0o700 });
await fs.promises.copyFile(OLD_CONFIG_FILE, CONFIG_FILE);
await fs.promises.chmod(CONFIG_FILE, 0o600);
} catch (e) {
console.error("Error migrating config:", e.message);
}
Expand All @@ -44,7 +45,7 @@ export async function loadConfig() {
if (!fs.existsSync(CONFIG_FILE)) {
return { $schema: "https://mage-remote-run.muench.dev/config.schema.json", profiles: {}, activeProfile: null, plugins: [] };
}
const data = fs.readFileSync(CONFIG_FILE, 'utf-8');
const data = await fs.promises.readFile(CONFIG_FILE, 'utf-8');
const config = JSON.parse(data);
if (!config.plugins) {
config.plugins = [];
Expand All @@ -58,9 +59,9 @@ export async function loadConfig() {

export async function saveConfig(config) {
try {
await mkdirp(CONFIG_DIR);
fs.writeFileSync(CONFIG_FILE, JSON.stringify(config, null, 2), { mode: 0o600 });
fs.chmodSync(CONFIG_FILE, 0o600);
await mkdirp(CONFIG_DIR, { mode: 0o700 });
await fs.promises.writeFile(CONFIG_FILE, JSON.stringify(config, null, 2), { mode: 0o600 });
await fs.promises.chmod(CONFIG_FILE, 0o600);
if (process.env.DEBUG) {
console.log(`Configuration saved to ${CONFIG_FILE}`);
}
Expand Down Expand Up @@ -98,7 +99,7 @@ export async function loadTokenCache() {
if (!fs.existsSync(TOKEN_CACHE_FILE)) {
return {};
}
const data = fs.readFileSync(TOKEN_CACHE_FILE, 'utf-8');
const data = await fs.promises.readFile(TOKEN_CACHE_FILE, 'utf-8');
return JSON.parse(data);
} catch (e) {
console.error("Error loading token cache:", e.message);
Expand All @@ -108,9 +109,9 @@ export async function loadTokenCache() {

export async function saveTokenCache(cache) {
try {
await mkdirp(CACHE_DIR);
fs.writeFileSync(TOKEN_CACHE_FILE, JSON.stringify(cache, null, 2), { mode: 0o600 });
fs.chmodSync(TOKEN_CACHE_FILE, 0o600);
await mkdirp(CACHE_DIR, { mode: 0o700 });
await fs.promises.writeFile(TOKEN_CACHE_FILE, JSON.stringify(cache, null, 2), { mode: 0o600 });
await fs.promises.chmod(TOKEN_CACHE_FILE, 0o600);
} catch (e) {
console.error("Error saving token cache:", e.message);
throw e;
Expand All @@ -120,7 +121,7 @@ export async function saveTokenCache(cache) {
export async function clearTokenCache() {
try {
if (fs.existsSync(TOKEN_CACHE_FILE)) {
fs.unlinkSync(TOKEN_CACHE_FILE);
await fs.promises.unlink(TOKEN_CACHE_FILE);
}
} catch (e) {
console.error("Error clearing token cache:", e.message);
Expand Down
55 changes: 31 additions & 24 deletions tests/config.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,14 @@ jest.unstable_mockModule('fs', () => ({
writeFileSync: jest.fn(),
copyFileSync: jest.fn(),
chmodSync: jest.fn(),
unlinkSync: jest.fn()
unlinkSync: jest.fn(),
promises: {
readFile: jest.fn(),
writeFile: jest.fn(),
chmod: jest.fn(),
unlink: jest.fn(),
copyFile: jest.fn()
}
}
}));

Expand Down Expand Up @@ -77,7 +84,7 @@ describe('Config Management', () => {

it('should load and parse config file', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: { test: {} },
activeProfile: 'test'
}));
Expand All @@ -89,12 +96,12 @@ describe('Config Management', () => {
activeProfile: 'test',
plugins: []
});
expect(fs.readFileSync).toHaveBeenCalledWith(EXPECTED_CONFIG_FILE, 'utf-8');
expect(fs.promises.readFile).toHaveBeenCalledWith(EXPECTED_CONFIG_FILE, 'utf-8');
});

it('should handle JSON parse errors', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue('invalid json');
fs.promises.readFile.mockResolvedValue('invalid json');

const config = await configMod.loadConfig();

Expand All @@ -104,7 +111,7 @@ describe('Config Management', () => {

it('should ensure plugins array exists', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: {}
}));

Expand All @@ -120,13 +127,13 @@ describe('Config Management', () => {

await configMod.saveConfig(config);

expect(mkdirp).toHaveBeenCalledWith(EXPECTED_CONFIG_DIR);
expect(fs.writeFileSync).toHaveBeenCalledWith(
expect(mkdirp).toHaveBeenCalledWith(EXPECTED_CONFIG_DIR, { mode: 0o700 });
expect(fs.promises.writeFile).toHaveBeenCalledWith(
EXPECTED_CONFIG_FILE,
JSON.stringify(config, null, 2),
{ mode: 0o600 }
);
expect(fs.chmodSync).toHaveBeenCalledWith(EXPECTED_CONFIG_FILE, 0o600);
expect(fs.promises.chmod).toHaveBeenCalledWith(EXPECTED_CONFIG_FILE, 0o600);
});

it('should log debug message if DEBUG env var is set', async () => {
Expand Down Expand Up @@ -163,14 +170,14 @@ describe('Config Management', () => {
await configMod.addProfile('NewProfile', { url: 'http://test.com' });

// Verify saveConfig was called with correct data
expect(fs.writeFileSync).toHaveBeenCalledWith(
expect(fs.promises.writeFile).toHaveBeenCalledWith(
EXPECTED_CONFIG_FILE,
expect.stringContaining('"NewProfile":'),
expect.anything()
);

// Verify activeProfile was set
const saveCall = fs.writeFileSync.mock.calls[0];
const saveCall = fs.promises.writeFile.mock.calls[0];
const savedConfig = JSON.parse(saveCall[1]);
expect(savedConfig.activeProfile).toBe('NewProfile');
expect(savedConfig.profiles['NewProfile']).toEqual({ url: 'http://test.com' });
Expand All @@ -179,15 +186,15 @@ describe('Config Management', () => {
it('should add profile but NOT change active if one exists', async () => {
// Mock loadConfig to return existing profile
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: { 'Existing': {} },
activeProfile: 'Existing',
plugins: []
}));

await configMod.addProfile('NewProfile', { url: 'http://test.com' });

const saveCall = fs.writeFileSync.mock.calls[0];
const saveCall = fs.promises.writeFile.mock.calls[0];
const savedConfig = JSON.parse(saveCall[1]);
expect(savedConfig.activeProfile).toBe('Existing');
expect(savedConfig.profiles['NewProfile']).toEqual({ url: 'http://test.com' });
Expand All @@ -197,7 +204,7 @@ describe('Config Management', () => {
describe('getActiveProfile', () => {
it('should return active profile with name', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: { 'MyProfile': { type: 'saas' } },
activeProfile: 'MyProfile'
}));
Expand All @@ -209,7 +216,7 @@ describe('Config Management', () => {

it('should return null if no active profile set', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: { 'MyProfile': {} },
activeProfile: null
}));
Expand All @@ -221,7 +228,7 @@ describe('Config Management', () => {

it('should return null if active profile does not exist in profiles', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({
fs.promises.readFile.mockResolvedValue(JSON.stringify({
profiles: { 'Other': {} },
activeProfile: 'Missing'
}));
Expand All @@ -241,12 +248,12 @@ describe('Config Management', () => {
describe('Token Cache', () => {
it('should load token cache', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue(JSON.stringify({ token: 'abc' }));
fs.promises.readFile.mockResolvedValue(JSON.stringify({ token: 'abc' }));

const cache = await configMod.loadTokenCache();

expect(cache).toEqual({ token: 'abc' });
expect(fs.readFileSync).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE, 'utf-8');
expect(fs.promises.readFile).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE, 'utf-8');
});

it('should return empty object if token cache missing', async () => {
Expand All @@ -259,7 +266,7 @@ describe('Config Management', () => {

it('should handle errors loading token cache', async () => {
fs.existsSync.mockReturnValue(true);
fs.readFileSync.mockReturnValue('invalid json');
fs.promises.readFile.mockResolvedValue('invalid json');

const cache = await configMod.loadTokenCache();

Expand All @@ -271,13 +278,13 @@ describe('Config Management', () => {
const cache = { token: 'abc' };
await configMod.saveTokenCache(cache);

expect(mkdirp).toHaveBeenCalledWith(EXPECTED_CACHE_DIR);
expect(fs.writeFileSync).toHaveBeenCalledWith(
expect(mkdirp).toHaveBeenCalledWith(EXPECTED_CACHE_DIR, { mode: 0o700 });
expect(fs.promises.writeFile).toHaveBeenCalledWith(
EXPECTED_TOKEN_CACHE_FILE,
JSON.stringify(cache, null, 2),
{ mode: 0o600 }
);
expect(fs.chmodSync).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE, 0o600);
expect(fs.promises.chmod).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE, 0o600);
});

it('should handle errors saving token cache', async () => {
Expand All @@ -293,20 +300,20 @@ describe('Config Management', () => {

await configMod.clearTokenCache();

expect(fs.unlinkSync).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE);
expect(fs.promises.unlink).toHaveBeenCalledWith(EXPECTED_TOKEN_CACHE_FILE);
});

it('should do nothing if token cache to clear does not exist', async () => {
fs.existsSync.mockReturnValue(false);

await configMod.clearTokenCache();

expect(fs.unlinkSync).not.toHaveBeenCalled();
expect(fs.promises.unlink).not.toHaveBeenCalled();
});

it('should handle errors clearing token cache', async () => {
fs.existsSync.mockReturnValue(true);
fs.unlinkSync.mockImplementation(() => { throw new Error('Unlink failed'); });
fs.promises.unlink.mockImplementation(() => { throw new Error('Unlink failed'); });

await expect(configMod.clearTokenCache()).rejects.toThrow('Unlink failed');
expect(consoleErrorSpy).toHaveBeenCalledWith(expect.stringContaining('Error clearing token cache'), 'Unlink failed');
Expand Down
Loading