Skip to content

refactor(traefik): improve config removal logic and error handling#4115

Merged
Siumauricio merged 1 commit intocanaryfrom
4086-stale-traefik-dynamic-config-files-not-cleaned-up-on-application-deletion
Mar 30, 2026
Merged

refactor(traefik): improve config removal logic and error handling#4115
Siumauricio merged 1 commit intocanaryfrom
4086-stale-traefik-dynamic-config-files-not-cleaned-up-on-application-deletion

Conversation

@Siumauricio
Copy link
Copy Markdown
Contributor

@Siumauricio Siumauricio commented Mar 30, 2026

  • Consolidated command execution for removing Traefik config files by using a single command string.
  • Enhanced error handling to log issues encountered during the removal process for both local and remote configurations.

What is this PR about?

Please describe in a short paragraph what this PR is about.

Checklist

Before submitting this PR, please make sure that:

  • You created a dedicated branch based on the canary branch.
  • You have read the suggestions in the CONTRIBUTING.md file https://github.com/Dokploy/dokploy/blob/canary/CONTRIBUTING.md#pull-request
  • You have tested this PR in your local instance. If you have not tested it yet, please do so before submitting. This helps avoid wasting maintainers' time reviewing code that has not been verified by you.

Issues related (if applicable)

closes #4086

Screenshots (if applicable)

Greptile Summary

This PR refactors the Traefik config removal helpers in application.ts with three improvements: it fixes a genuine bug where local fs.promises.unlink was called unconditionally even after a successful remote rm (the final block sat outside the if/else); it adds the -f flag to rm so missing-file errors are silently ignored on both local and remote paths; and it adds structured console.error logging to the previously empty catch blocks.

Key observations

  • The bug fix is correct — the old code would attempt a local file deletion even when serverId was set (i.e., a remote server target).
  • The -f flag addition is a good defensive improvement, eliminating spurious errors when a config file is already absent.
  • The local deletion path now uses execAsync("rm -f ...") instead of the previous fs.promises.unlink call. This is consistent with the remote approach but introduces an avoidable shell spawn and leaves the unquoted configPath variable susceptible to shell metacharacter interpretation if appName ever contains special characters. Using fs.promises.unlink with an ENOENT guard would be slightly safer and cheaper for the local case.

Confidence Score: 5/5

Safe to merge — fixes a real but low-impact bug and the only remaining finding is a P2 style suggestion.

The PR resolves a confirmed bug (double local deletion on remote path) and improves observability. The single open comment is a P2 style preference (native fs API vs shell) with no current production impact since application names in Dokploy are alphanumeric/hyphen-only and the local path constant is fixed.

No files require special attention.

Important Files Changed

Filename Overview
packages/server/src/utils/traefik/application.ts Fixes a real bug (unconditional local unlink after remote rm), adds -f to suppress missing-file errors, and improves error logging. Local deletion now uses execAsync shell command instead of the native fs API, introducing minor shell-overhead and a theoretical path-injection surface.

Reviews (1): Last reviewed commit: "refactor(traefik): improve config remova..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

- Consolidated command execution for removing Traefik config files by using a single command string.
- Enhanced error handling to log issues encountered during the removal process for both local and remote configurations.
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 30, 2026
Comment on lines 64 to 66
} else {
if (fs.existsSync(configPath)) {
await fs.promises.unlink(configPath);
}
}
if (fs.existsSync(configPath)) {
await fs.promises.unlink(configPath);
await execAsync(command);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Prefer native fs API over shell for local file removal

The previous code used fs.promises.unlink (with an existence check) for the local path, which avoids spawning a shell process entirely. Replacing it with execAsync("rm -f ...") introduces two minor concerns:

  1. Unnecessary shell overhead: spinning up a child process to call rm is heavier than a direct syscall via fs.promises.unlink.
  2. Path injection surface: if appName ever contained shell metacharacters (spaces, semicolons, backticks, etc.) the unquoted configPath in the command string would be interpreted by the shell. Wrapping the path in quotes, or reverting to the native API, removes this class of risk.
Suggested change
} else {
if (fs.existsSync(configPath)) {
await fs.promises.unlink(configPath);
}
}
if (fs.existsSync(configPath)) {
await fs.promises.unlink(configPath);
await execAsync(command);
}
} else {
try {
await fs.promises.unlink(configPath);
} catch (err: any) {
if (err?.code !== "ENOENT") throw err;
}
}

This mirrors the -f / "ignore missing file" semantics while keeping the local path on the safer, shell-free code path.

@Siumauricio Siumauricio merged commit c7b5e73 into canary Mar 30, 2026
5 checks passed
@Siumauricio Siumauricio deleted the 4086-stale-traefik-dynamic-config-files-not-cleaned-up-on-application-deletion branch March 30, 2026 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stale Traefik dynamic config files not cleaned up on application deletion

1 participant