Skip to content

Conversation

@drkskwlkr
Copy link

@drkskwlkr drkskwlkr commented Oct 31, 2025

Changes

  • Add error redirection to container deletion command inside private function graceful_shutdown_container()
  • Make sure the command always succeeds as intended by 'ignore_errors' => true

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Enhanced deployment robustness by improving error handling during container management operations. Failed operations no longer interrupt deployment workflows.

andrasbacsai and others added 3 commits October 29, 2025 10:39
…container

During deployment cleanup, the build container is stopped and then removed. The removal command often fails with "No such container" because the container was already auto-removed when stopped.

While the error is properly ignored by the deployment logic, it still 
appears in logs, creating unnecessary noise.

This change redirects stderr to `/dev/null` and adds ` || true` to ensure the command always succeeds, suppressing the expected error message.
@andrasbacsai andrasbacsai added 🐰 Release The Rabbit Run CodeRabbitAI review Rabbit and removed 🐰 Release The Rabbit Run CodeRabbitAI review Rabbit labels Nov 1, 2025
@coollabsio coollabsio deleted a comment from coderabbitai bot Nov 1, 2025
@andrasbacsai andrasbacsai added 🐰 Release The Rabbit Run CodeRabbitAI review and removed Rabbit labels Nov 1, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 1, 2025

Walkthrough

The ApplicationDeploymentJob.php file's graceful_shutdown_container function has been updated to improve error handling during Docker container removal. The docker rm command now includes error suppression (2>/dev/null) and a fallback operator (|| true), preventing the removal operation from failing the entire deployment process when a container is already gone.

Changes

Cohort / File(s) Summary
Docker Container Removal Error Handling
app/Jobs/ApplicationDeploymentJob.php
Modified the graceful_shutdown_container method to make container removal non-fatal by suppressing stderr and adding a fallback operator, allowing graceful shutdown to complete even if the container no longer exists

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Single file modification with a straightforward error handling improvement
  • The logic change is minimal and isolated to one command execution
  • No structural or control-flow changes—purely defensive shell command syntax

Poem

I'll be back... after this container's gone 🤖
No fatal errors, just smooth termination,
Like a gluten-free taco melting perfection,
Self-hosted servers don't need to beg the cloud—
Just ship it and deploy with confidence! 🌮

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix: suppress expected 'No such container' error when removing build container" is clear, specific, and directly describes the primary change in the changeset. It uses proper conventional commit formatting with the "fix:" prefix, is concise and readable, and provides a teammate scanning history with immediate understanding of what's being addressed. The title accurately reflects the modification to error handling in the graceful_shutdown_container function.
Description Check ✅ Passed The PR description is mostly complete with a well-filled Changes section that specifically details the modifications made to the graceful_shutdown_container function, including the error redirection and ignore_errors configuration. The description provides sufficient technical context to understand what was changed and why. However, the Issues section remains unfilled with just the placeholder "fix #" visible, missing the actual issue or discussion link reference, though this is a non-critical omission that doesn't significantly impact the core description quality.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
app/Jobs/ApplicationDeploymentJob.php (1)

2989-3000: I'll be back... but your container won't! Good error suppression, but could be more surgical.

The addition of 2>/dev/null || true on line 2995 makes sense for idempotent cleanup operations - like a good self-hosted server that just keeps running no matter what (unlike those flaky serverless functions that disappear faster than my gluten-free taco shells).

However, you're suppressing all stderr here, including potentially important errors like:

  • Docker daemon connection issues
  • Permission problems
  • Disk space issues

Consider a more targeted approach:

-                ["docker rm -f $containerName 2>/dev/null || true", 'hidden' => true, 'ignore_errors' => true]
+                ["docker rm -f $containerName 2>&1 | grep -v 'No such container' || true", 'hidden' => true, 'ignore_errors' => true]

Or even better, check if the container exists first:

+                ["docker ps -aq -f name=^${containerName}$ | xargs -r docker rm -f", 'hidden' => true, 'ignore_errors' => true]

This way, you only remove containers that actually exist, and legitimate errors still surface for debugging. Your deployment logs will thank you later when troubleshooting real issues.

Alternative: If you want to keep the current approach (which is simpler), at least add a comment explaining why we're suppressing all errors:

// Suppress "No such container" errors - container may already be removed
["docker rm -f $containerName 2>/dev/null || true", 'hidden' => true, 'ignore_errors' => true]
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between afd5d06 and f23909c.

📒 Files selected for processing (1)
  • app/Jobs/ApplicationDeploymentJob.php (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/Jobs/**/*.php

📄 CodeRabbit inference engine (.cursor/rules/application-architecture.mdc)

Queueable background work should be implemented as Jobs under app/Jobs; Actions may expose asJob to dispatch

app/Jobs/**/*.php: Queue jobs should implement ShouldQueue and define retry strategy (tries, maxExceptions, backoff)
Queue jobs must handle success/failure state updates and broadcast corresponding events; implement failed(Throwable) for final failure handling

Queue jobs should update status, wrap external calls in try/catch, set failure status on exceptions, and rethrow

Files:

  • app/Jobs/ApplicationDeploymentJob.php
app/Jobs/*.php

📄 CodeRabbit inference engine (.cursor/rules/deployment-architecture.mdc)

Implement background deployment, monitoring, backup, and notification work as Laravel Job classes under app/Jobs

Files:

  • app/Jobs/ApplicationDeploymentJob.php
**/*.php

📄 CodeRabbit inference engine (.cursor/rules/development-workflow.mdc)

**/*.php: Follow PSR-12 coding standards for all PHP code
Format PHP code with Laravel Pint configuration
Run static analysis with PHPStan to ensure type safety in PHP code
Document complex methods with PHPDoc blocks including parameters, return types, and thrown exceptions

**/*.php: Follow PSR-12 PHP coding standards across the codebase
Prefer eager loading and query optimization to prevent N+1 issues in database interactions
Use Laravel best practices for structure, services, and policies

**/*.php: Always use curly braces for control structures in PHP, even for single-line bodies
Use PHP 8 constructor property promotion in __construct() and avoid empty constructors
Always declare explicit return types for functions and methods; add appropriate parameter type hints
Prefer PHPDoc blocks over inline comments; only add inline code comments for very complex logic
When documenting arrays, add useful array shape types in PHPDoc where appropriate
Enum case names should be TitleCase (e.g., FavoritePerson, Monthly)

Files:

  • app/Jobs/ApplicationDeploymentJob.php
app/**/*.php

📄 CodeRabbit inference engine (.cursor/rules/development-workflow.mdc)

Use database transactions to group related write operations for consistency in services/controllers/jobs

Files:

  • app/Jobs/ApplicationDeploymentJob.php
app/Jobs/**

📄 CodeRabbit inference engine (.cursor/rules/project-overview.mdc)

Implement background jobs under app/Jobs

Files:

  • app/Jobs/ApplicationDeploymentJob.php
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: .cursor/rules/deployment-architecture.mdc:0-0
Timestamp: 2025-08-27T15:01:10.040Z
Learning: Applies to app/Jobs/*.php : Implement background deployment, monitoring, backup, and notification work as Laravel Job classes under app/Jobs
📚 Learning: 2025-08-27T15:01:10.040Z
Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: .cursor/rules/deployment-architecture.mdc:0-0
Timestamp: 2025-08-27T15:01:10.040Z
Learning: Applies to app/Jobs/*.php : Implement background deployment, monitoring, backup, and notification work as Laravel Job classes under app/Jobs

Applied to files:

  • app/Jobs/ApplicationDeploymentJob.php

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐰 Release The Rabbit Run CodeRabbitAI review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants