feat(macadam): only install macadam when vm features are used #2179
Conversation
cdrage
requested review from
SoniaSandler,
deboer-tim,
feloy and
jeffmaury
and removed request for
a team
📝 WalkthroughWalkthroughLazy-initialize Macadam: activation no longer forces macadam install or monitoring. The extension defers initialization until the macadam binary is present or a VM operation runs; new API and tests added, and README updated with macOS/Linux install notes and a Windows support note. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / VM Operation
participant Ext as Extension
participant FS as Filesystem
participant Mac as Macadam (binary/API)
participant Monitor as Monitor/EventEmitter
Note over Ext,FS: Extension activation — do not force install
Ext->>FS: check macadam binary present?
alt binary present
Ext->>Mac: init()
Mac-->>Ext: init success
Ext->>Monitor: start monitoring
else binary missing
Ext-->>Ext: defer init (no monitoring)
end
Note over User,Ext: User triggers VM operation
User->>Ext: call VM operation (list/create/start/stop/remove)
Ext->>FS: check macadam binary present?
alt binary present
Ext->>Mac: ensureMacadamInitialized() -> init()
Mac-->>Ext: init success
Ext->>Monitor: start monitoring
Ext->>Mac: perform VM operation
Mac-->>Ext: operation result
else binary missing
Ext-->>User: proceed without monitoring or surface install prompt when appropriate
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
🧹 Nitpick comments (2)
packages/backend/src/macadam.spec.ts (1)
79-105: Consider adding a test to verifyensureMacadamInitializedis called after VM creation.The
createVmmethod inmacadam.tsnow callsensureMacadamInitialized()after successful VM creation (line 73 in macadam.ts). However, there's no test verifying this integration point. Adding such a test would ensure the monitoring loop trigger behavior is preserved.🔎 Example test addition
import { ensureMacadamInitialized } from './extension'; test('Test createVm calls ensureMacadamInitialized after success', async () => { const macadamVm = new MacadamHandler(TELEMETRY_LOGGER_MOCK); const macadamInstance = vi.mocked(macadam.Macadam).mock.results[0].value; macadamInstance.createVm.mockResolvedValue({ stdout: '', stderr: '' }); vi.spyOn(extensionApi.window, 'withProgress').mockImplementation((_options, task) => { return task(progress, {} as unknown as extensionApi.CancellationToken); }); vi.mocked(extensionApi.env).isMac = true; await macadamVm.createVm({ name: 'test', imagePath: '/test.qcow2', sshIdentityPath: '/key', }); expect(ensureMacadamInitialized).toHaveBeenCalled(); });packages/backend/src/extension.spec.ts (1)
136-193: Test isolation concern: module-levelmacadamInitializedstate may leak between tests.The
extension.tsmodule uses a module-levelmacadamInitializedflag. Sincevi.clearAllMocks()inbeforeEachdoesn't reset module state, tests that set this flag totrue(like "should not re-initialize macadam if already initialized") could affect subsequent tests.Consider using
vi.resetModules()before each test in this describe block, or re-importing the module dynamically to ensure clean state.🔎 Proposed fix for test isolation
describe('test ensureMacadamInitialized doesnt double init', () => { beforeEach(() => { + vi.resetModules(); vi.spyOn(fs.promises, 'readFile').mockImplementation(() => { return Promise.resolve('<html></html>'); }); (podmanDesktopApi.version as string) = '1.8.0'; });Note: This may require adjusting how mocks are set up, as
resetModulesclears the module cache.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/img/escalated_privileges.pngis excluded by!**/*.png
📒 Files selected for processing (5)
README.md(1 hunks)packages/backend/src/extension.spec.ts(7 hunks)packages/backend/src/extension.ts(8 hunks)packages/backend/src/macadam.spec.ts(1 hunks)packages/backend/src/macadam.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/backend/src/macadam.ts (1)
packages/backend/src/extension.ts (1)
ensureMacadamInitialized(262-281)
packages/backend/src/extension.spec.ts (1)
packages/backend/src/extension.ts (2)
activate(102-214)getJSONMachineListByProvider(329-342)
🪛 markdownlint-cli2 (0.18.1)
README.md
192-192: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: linter, formatter, and tests / macos-14
- GitHub Check: linter, formatter, and tests / windows-2022
- GitHub Check: e2e tests
🔇 Additional comments (11)
README.md (1)
192-196: Documentation content aligns well with PR objectives.The new documentation clearly communicates the lazy initialization behavior: the password prompt now occurs during first-time VM operations (not on extension activation), it's a one-time installation, and platform-specific paths are documented. The Windows support note on line 196 is also valuable context.
packages/backend/src/macadam.spec.ts (1)
24-27: Mock setup looks correct for test isolation.The mock for
ensureMacadamInitializedproperly isolates theMacadamHandlertests from the actual initialization logic inextension.ts.packages/backend/src/macadam.ts (2)
69-73: CallingensureMacadamInitializedafter local init is correct for triggering monitoring.The code correctly calls
ensureMacadamInitialized()after VM creation to start the monitoring loop inextension.ts. SinceMacadamHandleruses its ownMacadaminstance (this.macadam) whileextension.tsmaintains a separate module-level instance, callingensureMacadamInitialized()here properly initializes the extension's instance and triggersmonitorMachines().
95-105:listVmsdoes not callensureMacadamInitializedbut other VM operations do.The
listVmsmethod callsthis.macadam.init()locally but does not triggerensureMacadamInitialized()fromextension.ts. This differs from the pattern established increateVm. WhilegetJSONMachineListByProviderinextension.tshandles initialization for the extension's instance, iflistVmsinMacadamHandleris called directly, the monitoring loop won't start.Is this intentional? If
listVmscan be invoked independently ofgetJSONMachineListByProvider, consider addingensureMacadamInitialized()here as well for consistency.packages/backend/src/extension.ts (4)
51-57: Binary path constants and state management are well-structured.The constants for binary paths clearly document the source, and the module-level state variables (
macadamInitialized,macadamProvider,macadamExtensionContext) properly track initialization state for the lazy init pattern.
197-212: Lazy initialization logic correctly defers sudo prompt.The activation now checks for binary existence before calling
init(), achieving the PR objective of avoiding the sudo prompt on extension activation when macadam isn't installed. The error handling logs failures without throwing, allowing the extension to continue loading.
258-281: Consider whether repeated init retries on failure are desired.If
macadam.init()fails (e.g., user cancels sudo prompt),macadamInitializedremainsfalse, causing subsequent VM operations to retry initialization. This could lead to repeated sudo prompts.If this retry behavior is intentional (giving users another chance), this is fine. If you want to avoid repeated prompts after a failure, consider adding a
macadamInitFailedflag to short-circuit future attempts.
329-342:getJSONMachineListByProviderproperly guards with lazy init.The function correctly calls
ensureMacadamInitialized()before listing VMs and captures any initialization errors in the return object. This ensures the sudo prompt only appears when users actually invoke VM operations.packages/backend/src/extension.spec.ts (3)
45-56: Clean fs mock setup for controlling binary existence.The mock correctly exports
existsSyncboth as a default export property and as a named export, ensuring compatibility with both import styles.
109-116: Class-based Macadam mock aligns with actual library structure.The mock properly creates a class with
initandlistVmsinstance methods, matching hownew Macadam(name)is used in the production code.
283-355: Comprehensive test coverage for lazy initialization across platforms.The tests thoroughly cover:
- macOS and Linux: binary existence checks controlling init behavior
- Windows: correctly skipping macadam (not yet supported)
- Error handling during activation
This test suite effectively validates the PR objectives.
|  | ||
|
|
||
| > **Note (macOS and Linux):** When performing virtual machine operations (create, start, stop, delete) for the first time, you will be prompted for your password to install the [macadam](https://github.com/crc-org/macadam) binary. This is a one-time installation that enables VM management. On macOS, the binary is installed to `/opt/macadam/bin/macadam`. On Linux, the binary will be installed automatically, but if issues occur it can be [installed manually](#linux-only-unable-to-create-virtual-machine). |
There was a problem hiding this comment.
Invalid link fragment: reference target is not a markdown heading.
Line 192 contains a link to #linux-only-unable-to-create-virtual-machine, but the referenced section on line 247 (**(Linux only) Unable to create virtual machine:**) is formatted as bold text, not a markdown heading. Markdown processors won't generate an anchor for non-heading text, making this link invalid. This also triggered the markdownlint MD051 warning.
🔎 Proposed fix: Convert line 247 to a proper markdown heading
Convert the bold text to a level 3 markdown heading so it generates a valid anchor:
-**(Linux only) Unable to create virtual machine:**
+### (Linux only) Unable to create virtual machineThis will create the anchor #linux-only-unable-to-create-virtual-machine that the link on line 192 references.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
192-192: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
README.md around lines 192 and 247: the link at line 192 targets
#linux-only-unable-to-create-virtual-machine but the target at line 247 is plain
bold text, not a markdown heading, so no anchor is generated; fix by converting
the bold text at line 247 into a proper markdown heading (e.g., "### (Linux
only) Unable to create virtual machine" or similar) so the generated anchor
matches the existing link, or alternatively update the link to exactly match the
final heading text; after change, run a markdown linter to ensure MD051 is
resolved.
packages/backend/src/extension.ts
Outdated
| console.error('Error initializing macadam', error); | ||
| } | ||
|
|
||
| monitorMachines(provider, extensionContext).catch((error: unknown) => { |
There was a problem hiding this comment.
I think it would better separate concerns to fire an event initialized here, and have an initialized.on(() => monitorMachines) at the place of the original monitorMachines()
cdrage
force-pushed
the
install-on-using-vms
branch
from
6b38132 to
674df49
Compare
|
Updated! Great suggestion @feloy I switched it to use events emitter instead to retrigger the monitoring. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/backend/src/macadam.ts (1)
96-105: Consider triggering monitoring in listVms as well.The
listVmsmethod initializes macadam but doesn't callensureMacadamInitialized(), unlikecreateVm(Line 73). This creates an inconsistency:
- If a user lists VMs before creating one, monitoring won't be triggered
- Per PR objectives, both createVM and listVMs should "lazily activate monitoring"
Consider adding
await ensureMacadamInitialized();after Line 98 to ensure consistent behavior across VM operations, or document why monitoring should only start after VM creation.🔎 Proposed fix to trigger monitoring in listVms
async listVms(): Promise<VmDetails[]> { try { await this.macadam.init(); + await ensureMacadamInitialized(); const vms = await this.macadam.listVms({}); return vms; } catch (err) { console.error('Failed to list VMs:', err); throw new Error(`Error listing VMs: ${err instanceof Error ? err.message : String(err)}`); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/img/escalated_privileges.pngis excluded by!**/*.png
📒 Files selected for processing (5)
README.mdpackages/backend/src/extension.spec.tspackages/backend/src/extension.tspackages/backend/src/macadam.spec.tspackages/backend/src/macadam.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/backend/src/extension.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/backend/src/extension.spec.ts (1)
packages/backend/src/extension.ts (2)
activate(105-218)getJSONMachineListByProvider(329-342)
packages/backend/src/macadam.ts (1)
packages/backend/src/extension.ts (1)
ensureMacadamInitialized(266-281)
🪛 markdownlint-cli2 (0.18.1)
README.md
192-192: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linter, formatter, and tests / windows-2022
🔇 Additional comments (8)
README.md (1)
192-196: Documentation accurately reflects lazy-initialization behavior.The new notes clearly explain the deferred macadam installation and when users encounter the password prompt (during first VM operations rather than on extension activation). This correctly documents the PR's objectives.
packages/backend/src/macadam.spec.ts (1)
24-27: LGTM!The mock correctly prevents actual macadam initialization during tests while allowing the test to verify the MacadamHandler behavior.
packages/backend/src/extension.spec.ts (5)
21-21: LGTM!The test infrastructure correctly supports lazy initialization testing:
getJSONMachineListByProviderimport enables direct testing of the lazy init pathway- Mock additions (
existsSyncMock,macadamInitMock,macadamListVmsMock) allow simulation of various initialization scenariosfs.existsSyncmock enables testing binary presence/absence conditionsAlso applies to: 33-36, 46-56
67-69: LGTM!Mock enhancements properly support platform-specific and error handling test scenarios:
- Platform flags (
isMac,isWindows,isLinux) enable OS-specific activation tests- Class-based Macadam mock with observable methods allows verification of initialization patterns
consoleErrorMockenables error logging assertionsAlso applies to: 109-116, 122-122
136-193: LGTM!The test suite thoroughly validates the lazy initialization guard and error propagation:
- Error propagation test correctly verifies that init failures surface through
getJSONMachineListByProviderand prevent VM listing- Lazy initialization test confirms macadam is not initialized at activation when the binary is missing, then initializes on first use
- Double initialization test verifies the guard flag prevents redundant init calls
283-344: LGTM!Platform-specific activation tests comprehensively cover lazy initialization behavior:
- macOS and Linux tests verify binary presence determines whether initialization occurs at activation
- Windows test correctly expects no macadam initialization (feature not yet implemented)
- Test scenarios align with the PR's lazy initialization objectives
345-354: LGTM!The error handling test correctly verifies that macadam initialization failures during activation are caught and logged without crashing the extension.
packages/backend/src/macadam.ts (1)
64-73: The concern about double initialization is incorrect—these are two separate Macadam instances with different purposes.
MacadamHandlercreates its own instance (this.macadam) on line 35 for VM operations, whileextension.tsmaintains a separate global instance for monitoring and lifecycle management. Line 64 initializes the MacadamHandler's instance for the current VM operation, and line 73 callsensureMacadamInitialized()to initialize the global instance and trigger the monitoring loop. The guard flag inensureMacadamInitialized()prevents redundant initialization of the global instance, not the MacadamHandler instance. This separation is intentional and correct—no changes are needed.Likely an incorrect or invalid review comment.
| > **Note (macOS and Linux):** When performing virtual machine operations (create, start, stop, delete) for the first time, you will be prompted for your password to install the [macadam](https://github.com/crc-org/macadam) binary. This is a one-time installation that enables VM management. On macOS, the binary is installed to `/opt/macadam/bin/macadam`. On Linux, the binary will be installed automatically, but if issues occur it can be [installed manually](#linux-only-unable-to-create-virtual-machine). | ||
|  | ||
|
|
||
| > **Note (Windows):** Windows support is currently _not supported_ due to Hyper-V limitations (must run as a privileged user), WSL2 support is _not supported_ due to Windows using a custom kernel for Virtual Machine usage which is incompatible with bootc-based images. |
There was a problem hiding this comment.
Fix broken link fragment by converting target to a markdown heading.
Line 192 contains a link to #linux-only-unable-to-create-virtual-machine, but line 247 is formatted as bold text, not a heading, so no anchor is generated. This breaks the link and triggers the MD051 linter error.
🔎 Proposed fix: Convert line 247 to a proper markdown heading
-**(Linux only) Unable to create virtual machine:**
+### (Linux only) Unable to create virtual machineThis will generate the anchor #linux-only-unable-to-create-virtual-machine that the link on line 192 expects.
Also applies to: 247-247
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
192-192: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
README.md lines 192-196 (link) and line 247 (target) — the link to
#linux-only-unable-to-create-virtual-machine is broken because the target is
bold text, not a markdown heading; replace the bold text at line 247 with a
proper markdown heading whose text exactly matches the expected slug, e.g. "##
Linux only: Unable to create virtual machine" (or another heading level) so the
anchor linux-only-unable-to-create-virtual-machine is generated and the link on
line 192 resolves.
### What does this PR do? Macadam is used to run VM's, this was previously activated on boot and would install macadam immediately by showing a sudo prompt. This PR adds "lazy" installing functionality where we only prompt if VM features are clicked / used. This PR: * Moves the init functionality to only happen when creating a VM * The VM monitoring loop is triggered either at the beginning (if macadam is already installed) or after create VM is used which would be the first point a VM is accessed / used * Adds documentation as well as screenshot ### Screenshot / video of UI <!-- If this PR is changing UI, please include screenshots or screencasts showing the difference --> ### What issues does this PR fix or reference? <!-- Include any related issues from Podman Desktop repository (or from another issue tracker). --> Closes podman-desktop#2171 ### How to test this PR? <!-- Please explain steps to reproduce --> 1. REMOVE MACADAM (macOS): ```sh ~ $ whereis macadam macadam: /opt/macadam/bin/macadam ~ $ sudo rm -rf /opt/macadam Password: ``` If you are on Linux, it must be removed at: `/usr/local/bin/macadam` 2. Start bootc extension, you should NO LONGER receive the sudo prompt. 3. Go to **Disk Images > Create VM** 4. Expect the sudo prompt to appear to install 5. Be able to create virtual machine / list VM functionality like usual. Signed-off-by: Charlie Drage <[email protected]>
cdrage
force-pushed
the
install-on-using-vms
branch
from
674df49 to
d4f0c9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
200-200: Broken link fragment already flagged in previous reviews.The link to
#linux-only-unable-to-create-virtual-machinepoints to a target (line 247) that is formatted as bold text rather than a markdown heading, preventing anchor generation. This issue has been identified in prior reviews.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/img/escalated_privileges.pngis excluded by!**/*.png
📒 Files selected for processing (5)
README.mdpackages/backend/src/extension.spec.tspackages/backend/src/extension.tspackages/backend/src/macadam.spec.tspackages/backend/src/macadam.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/backend/src/macadam.ts (1)
packages/backend/src/extension.ts (1)
ensureMacadamInitialized(266-281)
packages/backend/src/extension.spec.ts (1)
packages/backend/src/extension.ts (2)
activate(105-218)getJSONMachineListByProvider(329-342)
🪛 markdownlint-cli2 (0.18.1)
README.md
200-200: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: linter, formatter, and tests / windows-2022
- GitHub Check: e2e tests
- GitHub Check: linter, formatter, and tests / macos-14
🔇 Additional comments (11)
README.md (1)
200-204: Clear documentation of the lazy installation behavior.The added notes effectively communicate the one-time macadam installation requirement and privilege escalation to users, aligning well with the PR's objective to defer sudo prompts until VM features are used.
packages/backend/src/macadam.spec.ts (1)
24-27: LGTM: Mock setup enables clean testing of MacadamHandler.The mock for
ensureMacadamInitializedappropriately simulates a successful initialization without triggering actual side effects, allowing tests to focus on MacadamHandler behavior.packages/backend/src/macadam.ts (2)
23-23: LGTM: Import supports lazy initialization pattern.The import of
ensureMacadamInitializedenables the MacadamHandler to trigger monitoring after VM creation, aligning with the PR's lazy initialization strategy.
70-73: Well-documented trigger point for monitoring initialization.The call to
ensureMacadamInitialized()after VM creation correctly ensures the monitoring loop starts only after a user-initiated VM operation, with a clear comment explaining the behavior.packages/backend/src/extension.ts (3)
203-216: Clean separation of initialization from monitoring startup.The conditional initialization based on binary existence successfully defers the sudo prompt to the first VM operation when the binary is not pre-installed, meeting the PR's core objective.
262-281: LGTM: Clear API for lazy initialization.The exported
ensureMacadamInitialized()function provides a straightforward gate for VM operations, with appropriate error handling and event emission to trigger monitoring.
333-333: Consistent initialization guards across all VM operations.All VM operation entry points (
getJSONMachineListByProvider,startMachine,stopMachine, and thedeletelifecycle method) correctly ensure macadam is initialized before proceeding.Also applies to: 355-355, 385-385, 417-417
packages/backend/src/extension.spec.ts (4)
45-56: Well-structured fs module mock for binary existence simulation.The mock setup for
fs.existsSyncenables precise control over binary existence checks in tests, supporting validation of both initialization paths (binary present vs. absent).
109-116: LGTM: Macadam mock structure aligns with production usage.The class-based mock with
initandlistVmsmethods accurately reflects the Macadam API surface used in the extension, enabling realistic test scenarios.
136-193: Comprehensive test coverage for initialization lifecycle.The test suite thoroughly validates:
- Error propagation through the lazy initialization path
- Deferred initialization when binary is absent at activation
- Prevention of re-initialization
These tests provide strong confidence in the lazy initialization behavior.
283-355: Excellent OS-specific test coverage.The test suite validates platform-specific behavior (macOS, Linux, Windows) for both binary-present and binary-absent scenarios, including error handling during activation. This comprehensive coverage aligns well with the cross-platform nature of the extension.
| // Macadam binary paths - used to check if already installed | ||
| // MACADAM_MACOS_PATH is exported from macadam.js, Linux path is hardcoded in the library | ||
| const MACADAM_BINARY_PATH_MACOS = `${MACADAM_MACOS_PATH}/macadam`; | ||
| const MACADAM_BINARY_PATH_LINUX = '/usr/local/bin/macadam'; | ||
| let macadamInitialized = false; | ||
|
|
||
| // Event emitter for macadam lifecycle events - separates initialization from monitoring | ||
| const macadamEvents = new EventEmitter(); | ||
| const MACADAM_INITIALIZED_EVENT = 'initialized'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/backend/src/extension.ts | head -100Repository: podman-desktop/extension-bootc
Length of output: 4300
🏁 Script executed:
sed -n '200,300p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 3178
🏁 Script executed:
rg -n "ensureMacadamInitialized" packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 313
🏁 Script executed:
sed -n '330,340p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 391
🏁 Script executed:
sed -n '350,360p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 460
🏁 Script executed:
sed -n '380,390p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 477
🏁 Script executed:
sed -n '410,425p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 604
🏁 Script executed:
sed -n '266,282p' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 453
🏁 Script executed:
rg -n "macadamInitialized\|macadamInitPromise" packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 56
🏁 Script executed:
rg -n "macadamInitialized" packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 214
Race condition in concurrent initialization must be fixed.
The macadamInitialized flag lacks synchronization. When multiple VM operations call ensureMacadamInitialized() concurrently—for example, if a user starts two VMs simultaneously—they can both read macadamInitialized === false before the first initialization completes, causing multiple concurrent macadam.init() calls. This can trigger multiple sudo prompts or installation attempts.
Fix by introducing a Promise to serialize initialization:
let macadamInitialized = false;
+let macadamInitPromise: Promise<void> | undefined;
// Event emitter for macadam lifecycle events - separates initialization from monitoring
const macadamEvents = new EventEmitter();
const MACADAM_INITIALIZED_EVENT = 'initialized';Then update ensureMacadamInitialized():
export async function ensureMacadamInitialized(): Promise<void> {
if (macadamInitialized) {
return;
}
+
+ // If initialization is already in progress, wait for it
+ if (macadamInitPromise) {
+ return macadamInitPromise;
+ }
- try {
- await macadam.init();
- macadamInitialized = true;
-
- // Emit initialized event to trigger monitoring
- macadamEvents.emit(MACADAM_INITIALIZED_EVENT);
- } catch (error) {
- console.error('Error initializing macadam', error);
- throw error;
- }
+ macadamInitPromise = (async () => {
+ try {
+ await macadam.init();
+ macadamInitialized = true;
+
+ // Emit initialized event to trigger monitoring
+ macadamEvents.emit(MACADAM_INITIALIZED_EVENT);
+ } catch (error) {
+ console.error('Error initializing macadam', error);
+ throw error;
+ } finally {
+ macadamInitPromise = undefined;
+ }
+ })();
+
+ return macadamInitPromise;
}Committable suggestion skipped: line range outside the PR's diff.
| // Register listener to start monitoring when macadam is initialized | ||
| macadamEvents.on(MACADAM_INITIALIZED_EVENT, () => { | ||
| monitorMachines(provider, extensionContext).catch((error: unknown) => { | ||
| console.error('Error while monitoring machines', error); | ||
| }); | ||
| }); | ||
|
|
||
| // Only initialize and start monitoring if macadam binary is already installed. | ||
| // This avoids prompting for sudo on extension activation (macOS). | ||
| // If not installed, init() will be called lazily when user performs a VM operation. | ||
| const macadamBinaryPath = extensionApi.env.isMac ? MACADAM_BINARY_PATH_MACOS : MACADAM_BINARY_PATH_LINUX; | ||
| if (fs.existsSync(macadamBinaryPath)) { | ||
| try { | ||
| await macadam.init(); | ||
| macadamInitialized = true; | ||
| } catch (error) { | ||
| console.error('Error initializing macadam', error); | ||
| } | ||
|
|
||
| macadamEvents.emit(MACADAM_INITIALIZED_EVENT); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the monitorMachines function implementation
rg -nP -A20 'async function monitorMachines' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 892
🏁 Script executed:
# Search for stopLoop variable declaration and usage
rg -nP 'stopLoop' packages/backend/src/extension.ts | head -20Repository: podman-desktop/extension-bootc
Length of output: 144
🏁 Script executed:
# Search for ensureMacadamInitialized function
rg -nP -B3 -A15 'function ensureMacadamInitialized' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 741
🏁 Script executed:
# Check the event listener registration to see if it uses .on() or .once()
rg -nP -B5 -A5 'macadamEvents.on.*MACADAM_INITIALIZED_EVENT' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 528
🏁 Script executed:
# Check where monitorMachines is called and how many times it could be invoked
rg -nP 'monitorMachines' packages/backend/src/extension.tsRepository: podman-desktop/extension-bootc
Length of output: 351
Improve event listener registration to prevent duplicate monitoring loops.
The MACADAM_INITIALIZED_EVENT listener at line 197 uses .on(), which means it will trigger every time the event is emitted. While the macadamInitialized guard in ensureMacadamInitialized() (line 267) currently prevents a second emission in normal flow, this design is fragile. If the event is emitted multiple times—whether intentionally or through future code changes—multiple monitorMachines() invocations would run concurrently, each spawning independent recursive loops checking the same stopLoop flag.
Use .once() instead of .on() to ensure the listener fires only once, or add a guard flag to track whether monitoring is already active.
🤖 Prompt for AI Agents
packages/backend/src/extension.ts lines 196-216: the MACADAM_INITIALIZED_EVENT
listener is registered with .on(), which can allow multiple monitorMachines()
loops to start if the event fires more than once; change the listener to .once()
so it only fires a single time, or alternatively add a simple guard (e.g., let
monitoringActive = false; check it before calling monitorMachines and set it
true once started) to ensure monitorMachines() is invoked at most once; keep the
existing error handling wrapper when calling monitorMachines().
2 participants
feat(macadam): only install macadam when vm features are used
What does this PR do?
Macadam is used to run VM's, this was previously activated on boot and would
install macadam immediately by showing a sudo prompt. This PR adds
"lazy" installing functionality where we only prompt if VM features are
clicked / used.
This PR:
macadam is already installed) or after create VM is used which would
be the first point a VM is accessed / used
Screenshot / video of UI
Video shows functionality of it installed and not installed:
Screen.Recording.2025-12-19.at.3.40.09.PM.mov
What issues does this PR fix or reference?
Closes #2171
How to test this PR?
If you are on Linux, it must be removed at:
/usr/local/bin/macadamStart bootc extension, you should NO LONGER receive the sudo prompt.
Go to Disk Images > Create VM
Expect the sudo prompt to appear to install
Be able to create virtual machine / list VM functionality like usual.
Signed-off-by: Charlie Drage [email protected]