fix: importing stacks if files are in the data/stacks directory#161
fix: importing stacks if files are in the data/stacks directory#161
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change removes the Changes
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
kmendell
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/lib/types/docker/stack.type.ts (1)
41-41:⚠️ Potential issueRemove unused autoUpdate property from StackUpdate interface.
The
autoUpdateproperty is still present in theStackUpdateinterface despite being removed from all the API methods and UI components. This appears to be an oversight since the PR removes the auto-update switch per stack.export interface StackUpdate { name?: string; composeContent?: string; envContent?: string; - autoUpdate?: boolean; }
🧹 Nitpick comments (2)
docs/docs/guides/auto-update.md (1)
14-17: Align label syntax across the guide for clarityThe prose section uses the YAML-map style (
arcane.stack.auto-update: "true"), whereas the example snippet later switches to the array style ('arcane.stack.auto-update=true'). New users may copy-paste the latter into a map block or vice-versa and hit a compose-parsing error.
Consider adding a short sentence explaining both styles and ensuring the sample matches the earlier wording, e.g.:- - 'arcane.stack.auto-update=true' # Enables auto-update … + # Either array or map style works – choose one: + # - 'arcane.stack.auto-update=true' # array style + # arcane.stack.auto-update: "true" # map styleAlso applies to: 41-42
src/lib/services/docker/auto-update-service.ts (1)
115-133: Potential performance bottleneck –getStackserial lookupInside the eligibility loop we await
getStacksequentially, which increases execution time proportionally to stack count.
Consider batching withPromise.allor at least parallelising eligibility checks:const eligibleStacksForProcessing = ( await Promise.all( allListedStacks .filter((s) => ['running', 'partially running'].includes(s.status)) .map(async (s) => { try { const full = await getStack(s.id); return hasAutoUpdateLabel(full) ? s : null; } catch { return null; } }) ) ).filter(Boolean) as Stack[];This remains bounded because per-stack work is mostly I/O-bound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/docs/guides/auto-update.md(1 hunks)src/lib/services/api/stack-api-service.ts(1 hunks)src/lib/services/docker/auto-update-service.ts(2 hunks)src/lib/services/docker/stack-service.ts(8 hunks)src/lib/types/docker/stack.type.ts(1 hunks)src/lib/utils/api.util.ts(1 hunks)src/routes/api/stacks/[stackId]/+server.ts(1 hunks)src/routes/stacks/[stackId]/+page.server.ts(0 hunks)src/routes/stacks/[stackId]/+page.svelte(4 hunks)
💤 Files with no reviewable changes (1)
- src/routes/stacks/[stackId]/+page.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/routes/api/stacks/[stackId]/+server.ts (2)
src/lib/utils/try-catch.ts (1)
tryCatch(14-21)src/lib/services/docker/stack-service.ts (1)
updateStack(533-595)
src/lib/services/docker/auto-update-service.ts (2)
src/lib/services/docker/stack-service.ts (3)
listStacks(1168-1192)getStack(482-528)fullyRedeployStack(794-827)src/lib/types/docker/stack.type.ts (1)
Stack(22-35)
🪛 LanguageTool
docs/docs/guides/auto-update.md
[uncategorized] ~56-~56: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ggered, Arcane will typically perform a full stack redeployment (similar to `docker-compos...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 Biome (1.9.4)
src/lib/services/docker/auto-update-service.ts
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
src/routes/api/stacks/[stackId]/+server.ts (2)
9-9: Removed autoUpdate from request destructuring.The destructuring of the request JSON now only extracts
name,composeContent, andenvContent, removing the previously extractedautoUpdateproperty. This change properly aligns with the PR objective of removing the auto-update switch per stack.
11-11: Updated stack update call to remove autoUpdate parameter.The call to
updateStackhas been modified to only passname,composeContent, andenvContentas parameters, removing theautoUpdateflag. This properly implements the PR objective of removing auto-update per stack in favor of using labels in the compose file.src/lib/services/api/stack-api-service.ts (1)
42-42: Removed autoUpdate parameter from save method signature.The
savemethod no longer accepts theautoUpdateparameter, which is consistent with the changes in the API endpoint and the overall goal of removing the per-stack auto-update flag. The method now only accepts the stack ID, name, content, and optional environment content.src/lib/types/docker/stack.type.ts (1)
28-28: Added 'unknown' as a possible stack status value.The
statusfield in theStackinterface now includes'unknown'as a possible value, which improves error handling when a stack's status cannot be determined. This change supports the goal of better stack import and management mentioned in the PR.src/routes/stacks/[stackId]/+page.svelte (6)
14-14: Added navigation imports for stack ID changes.Updated imports to include
invalidateAllandgotofrom$app/navigation, which are needed for proper navigation handling after saving a stack with a modified ID. This is an important enhancement that supports the improved stack renaming mentioned in the PR objective.
46-46: Removed autoUpdate from change detection.The
hasChangesderived state calculation no longer includes checking for changes in theautoUpdateproperty, which is consistent with removing the auto-update switch per stack as described in the PR objectives.
61-63: Added tracking of original stack ID for redirection after save.Now storing the original stack ID before saving, which enables the redirection logic to work correctly if the stack ID changes during the save operation (such as when renaming a stack). This enhancement improves the UX for stack renaming.
65-65: Removed autoUpdate parameter from save API call.The call to
stackApi.savenow only passes the stack ID, name, compose content, and environment content, removing theautoUpdateparameter. This is consistent with the changes in the API service and the PR objective.
68-84: Enhanced navigation after stack save.Added smarter navigation logic to handle the case when a stack ID changes after saving (such as during a rename operation). Now it will use
gototo navigate to the new stack URL if the ID changed, maintaining a consistent UX. This improvement aligns with the PR goal of enhancing stack renaming.
253-256: Improved stack name editing UX.Added appropriate UI restrictions for stack name editing by disabling the input field when the stack is running or partially running. Also added a helpful explanatory message to guide users. This improves the user experience by preventing potentially problematic rename operations while a stack is active.
src/lib/services/docker/auto-update-service.ts (3)
125-156: Label-detection logic can be simplified & hardened
- Optional chaining removes several null checks (
service.labels?.).- Support both
=and:separators in array style, because users might write'key:"true"'.- Using
some()over services is clearer.-let stackIsEligibleByLabel = false; - -if (composeData && composeData.services) { - for (const serviceName in composeData.services) { - const service = composeData.services[serviceName]; - if (service.labels) { - let labelValue: string | undefined = undefined; - if (Array.isArray(service.labels)) { - const foundLabel = service.labels.find((l: string) => l.startsWith('arcane.stack.auto-update=')); - if (foundLabel) { - labelValue = foundLabel.split('=')[1]; - } - } else if (typeof service.labels === 'object' && service.labels !== null) { - labelValue = service.labels['arcane.stack.auto-update']; - } - if (labelValue === 'true') { - stackIsEligibleByLabel = true; - break; // Found one service with the label, stack is eligible - } - } - } -} +const stackIsEligibleByLabel = + composeData?.services && + Object.values(composeData.services).some((svc: any) => { + const labels = svc.labels; + if (!labels) return false; + if (Array.isArray(labels)) { + return labels.some((l: string) => + /^arcane\.stack\.auto-update[:=]["']?true["']?$/i.test(l.trim()) + ); + } + return labels['arcane.stack.auto-update'] === 'true'; + });Benefits: fewer branches, case-insensitive match, and prevention of the Biome optional-chain warning.
[ suggest_essential_refactor ]🧰 Tools
🪛 Biome (1.9.4)
[error] 135-135: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
183-187:checkStackImagesUpdatere-loads stack content inside tight loop
checkStackImagesUpdateinternally callsgetStackagain, resulting in two disk reads per stack (eligibility + update check). PassingcomposeContentyou already hold would halve the I/O:-const updateAvailable = await checkStackImagesUpdate(stackToUpdate); +const updateAvailable = await checkStackImagesUpdate({ ...stackToUpdate, composeContent: fullStack.composeContent });Alternatively overload the util to accept raw compose as second arg.
[ suggest_optional_refactor ]
179-181: Lock handling looks correct – good jobThe Set-based guard plus deletion in both success and error paths prevents stuck locks and concurrent redeploys.
Also applies to: 195-196, 200-207
src/lib/services/docker/stack-service.ts (6)
120-140:getStackMetaPathappears to be dead code – safe to delete.Since
.stack.json/meta.jsonfiles are no longer part of the design, this helper is no longer referenced anywhere in the module. Keeping it around:
- keeps an unused
StackMetatype import alive (line 11)- increases cognitive load for future readers
Consider deleting the helper and the
StackMetaimport to keep the codebase lean.-import type { Stack, StackMeta, StackService, StackUpdate } from '$lib/types/docker/stack.type'; +import type { Stack, StackService, StackUpdate } from '$lib/types/docker/stack.type'; @@ -async function getStackMetaPath(stackId: string): Promise<string | null> { - … -}[ suggest_essential_refactor ]
235-253: Expensive full–container scan each call – use Docker API filters first.
getStackServicescallsdocker.listContainers({ all:true })and then filters the entire container list in memory.
On hosts running many containers this becomes O(N) per stack inspection and can seriously degrade UI latency.Because every Compose-created container carries
com.docker.compose.project=<stackId>you can narrow the list at the daemon side:-const containers = await docker.listContainers({ - all: true -}); +const containers = await docker.listContainers({ + all: true, + filters: JSON.stringify({ label: [`com.docker.compose.project=${stackId}`] }) +});Keep the secondary “name starts with” heuristic as a fallback only when the primary query returns zero results.
This change slashes both network payload and local CPU usage.
[ suggest_essential_refactor , performance ]
874-949:renameStacksuffers from a race & missing atomicity safeguards.
- Between the
directoryExistscheck (lines 910-918) and the subsequentfs.rename, another process could create the target directory, leading to an overwrite or unexpectedEEXIST.- The loop only retries 100 times; under a race it may still throw even though a name is actually free.
- No advisory lock is taken, so two concurrent rename requests can clash.
A safer pattern:
-while (counter <= MAX_ATTEMPTS) { - const pathToCheck = join(stacksDir, newUniqueDirName); - const exists = await directoryExists(pathToCheck); - if (!exists && newUniqueDirName !== currentStackId) break; - … -} +while (counter <= MAX_ATTEMPTS) { + try { + await fs.mkdir(join(stacksDir, newUniqueDirName), { recursive: false }); + // If mkdir succeeds we immediately rename into the *empty* dir, + // guaranteeing uniqueness without TOCTOU. + await fs.rmdir(join(stacksDir, newUniqueDirName)); // remove the placeholder + break; + } catch (e: any) { + if (e.code !== 'EEXIST') throw e; // real error + newUniqueDirName = `${newDirBaseName}-${counter++}`; + } +}Alternatively wrap the whole operation in a filesystem lock (e.g.,
fs.openwithO_EXCL).
[ suggest_optional_refactor , concurrency ]
1056-1119: Path-traversal / information-disclosure via untrusted label paths.
importExternalStackblindly trustscom.docker.compose.project.config_files, allowing a container author to point Arcane at any host-readable path (e.g./etc/shadow).
Because you read the file and return its content to the UI/API, this is a serious security gap.Mitigation guideline:
- Limit reads to a whitelist (e.g., sub-paths of
/data/stacks,/var/lib/docker/volumes/**, etc.).- Reject absolute paths that are outside the allowed roots.
- Use
path.resolve()+ prefix check beforefs.readFile.Example hardening patch:
-actualComposeFilePathUsed = pathToTry; +const resolved = path.resolve(pathToTry); +const allowedRoots = ['/data', '/var/lib/docker/volumes']; +if (!allowedRoots.some((root) => resolved.startsWith(root))) { + throw new Error( + `Refusing to read compose file outside allowed roots: ${resolved}` + ); +} +actualComposeFilePathUsed = resolved;Fail fast rather than silently generating a new compose file; that makes exploitation harder and highlights mis-configured labels.
[ raise_critical_issue , security ]
1180-1189: Minor: keephasArcaneMetaconsistent or drop it.
discoverExternalStacksnever setshasArcaneMeta, so you patch it here.
If the flag is now redundant (metadata files are gone) consider removing it altogether to avoid future confusion.
Otherwise guarantee the field exists on all Stack objects by normalising in a single helper.
[ suggest_nitpick ]
1199-1223:isStackRunningduplicates container queries – combine them.Two separate
docker.listContainerscalls double the network chatter.
You can reuse the first full list when the label filter returns nothing, or better – call the broad query once and partition locally.const all = await docker.listContainers({ all: true }); const byLabel = all.filter(...label test...) if (byLabel.length) return byLabel.some(c => c.State === 'running'); const byName = all.filter(...name test...); return byName.some(c => c.State === 'running');Saves an extra daemon round-trip and simplifies error handling.
[ suggest_optional_refactor , performance ]
Deploying arcane with
|
| Latest commit: |
9154fa4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5e07bfff.arcane-7v3.pages.dev |
| Branch Preview URL: | https://fix-stack-import.arcane-7v3.pages.dev |
1 participant
Fixes: #159
This PR also removes the auto update switch per satck, and requires a certain label to be used in the compose file
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Refactor
Style
Chores