Skip to content

Conversation

@theuargb
Copy link

@theuargb theuargb commented Feb 28, 2025

Description (*)

This tries to address the issue when the generated/* directory is not consistently empty on deployment (due to the running process with outdated runtime or other custom code/configs), introducing simple retry.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Deployment is a critical application process. There’s a behavior in this process that sometimes leads to failed deployments, which is far worse than retry logic in a critical path. The app should not fail if it can recover — it should do everything possible to complete the process gracefully.

Some deployments may involve processes that temporarily occupy the generated folder. It's acceptable for those processes to continue with outdated runtime until they finish.

Good apps handle recoverable errors gracefully — they try to recover instead of failing outright.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Feb 28, 2025

Hi @theuargb. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@m2-github-services m2-github-services added Partner: Perspective partners-contribution Pull Request is created by Magento Partner labels Feb 28, 2025
@theuargb theuargb marked this pull request as draft February 28, 2025 15:34
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

At first look, your fix looks as a workaround.

For better context, could you please provide some steps to reproduce, some exception stacktrace, or any other additional information?

@andrewbess
Copy link
Contributor

Hello @ihor-sviziev
This happens sometimes and not often. Probably, if you do not enable maintenance mode in developer mode, and simultaneously cron or something else is running. That is, when the system has already cleared the generated directory during setup:upgrade. Still, between this action (clearing directory) and the subsequent setup:di:compile, some class has already been generated there and the directory is not empty.

@andrewbess
Copy link
Contributor

Hello @theuargb
Thank you for your contribution.
It looks like a workaround, and this fix should not be in the system (IMHO).
Just, use setup commands more correctly.

@engcom-Charlie engcom-Charlie moved this to Pending Review in Community Dashboard Apr 10, 2025
@engcom-Charlie engcom-Charlie added the Project: Community Picked PRs upvoted by the community label Apr 10, 2025
@engcom-Hotel
Copy link
Contributor

Hello @theuargb,

Have you got a chance to look into this #39684 (comment)? I agree with the @ihor-sviziev and @andrewbess here.

I think we should close this PR as of now, please suggest. Meanwhile moving this PR On Hold.

Thanks

@theuargb
Copy link
Author

theuargb commented Apr 21, 2025

i honestly do not understand why community members criticize issue/PR IN DRAFT STATE WITHOUT ANY INFORMATION in description. if this one bothers you in this state - remove it.

there is also normally described less disscussive PR (magento/inventory#3411) which lays there for years without any attention

@theuargb
Copy link
Author

hi @engcom-Hotel @andrewbess

my generic point:

  1. there is a bug in the deployment system which leads to deployments failures in certain circumstances.
  2. this error, for which retry is implemented here, almost certainly trigged once (due to the concurrent nature of the FS) and very unlikely to repeat if retry immidiately.
  3. there could be deployments where it is impossible to stop all the processes occupying generated folder while IT IS FINE TO THEESE PROCESSES TO CONTINUE WITH OUTDATED RUNTIME until they completes

my original plan:
why does application should fail (even if there is environment who is guilty) if it could not?
good apps does not fail if there is recoverable error - they try to recover as much as possible

my proposal:
deployment is a critical application process. there is an certain behavior in this process which sometimes leads to failed deployment - this is much more bad IMHO than normal retry logic in the critical place.
so application should do everything it could to complete process gracefully

@engcom-Hotel
Copy link
Contributor

Hello @theuargb,

i honestly do not understand why community members criticize issue/PR IN DRAFT STATE WITHOUT ANY INFORMATION in description. if this one bothers you in this state - remove it.

Thank you for your comment. For draft PRs, we typically assume that there are pending tasks or incomplete code, which is why they are in draft status. As such, we do not proceed with further processing, such as review or testing, until the draft is finalized. This is why we have moved it to the On Hold bucket. Additionally, since the description was empty, we have requested more information regarding the purpose of this PR and its manual testing scenarios etc.

for this #39684 (comment):

After reviewing your comment above, it appears that you are encountering this problem with a specific file system. Could you please provide us with the following information:

  1. Magento version
  2. OS information
  3. Steps to reproduce
  4. Expected result
  5. Actual result

Thanks

@theuargb theuargb closed this Apr 23, 2025
@theuargb theuargb deleted the fix-di-compile-sometimes-crashing branch April 23, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Partner: Perspective partners-contribution Pull Request is created by Magento Partner Progress: on hold Project: Community Picked PRs upvoted by the community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants