fix(jobs): resolve portability issues with background job cleanup - #61830
Conversation
Altahrim
left a comment
There was a problem hiding this comment.
Nice improvement, thank you!
(and yes… remaining test thank you for catching it!)
|
Glad the cause for the error was found but atm the pull request is still open, is someone able to fix the tests so we can get this merged please? I'm receiving 21 emails from cron a day and disabling those emails is not optimal because that would suppress other issues, and manually applying the fix causes checksum failures. Thanks. |
|
Rebased the branch. Previously failing CI seems to be unrelated (some database test and failing permissions on a workflow), looks like the rebase might fix at least the latter one. |
The previous implementation executes "ps" with flags -p and -o to check whether a job process was still running. This approach has portability issues regarding e.g. BusyBox ps (Alpine Linux) where `ps -p` fails or BSD/macOS where `ps -o` fails. Replace with `posix_kill($pid, 0)`, the standard POSIX way to probe process existence without delivering a signal. EPERM is treated as "process exists but not owned by current user". This should be available on all supported platforms. Fixes: 60ce92a Suggested-by: Michele Marcionelli Signed-off-by: Stefan Kalscheuer <stefan@stklcode.de>
Snowflake ID in `JobRuns:deleteBefore()` is computed from given timestamp, but immediately overwritten by a literal value making the expiration setting have no effect. This looks like a leftover artifact from development that should be cleaned up. Remove the hardcoded ID to restore the desired behavior. Fixes: dc5499a Suggested-by: Michele Marcionelli Signed-off-by: Stefan Kalscheuer <stefan@stklcode.de>
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
|
/backport to stable34 |
|
/backport to stable33 |
|
The backport to # Switch to the target branch and update it
git checkout stable33
git pull origin stable33
# Create the new backport branch
git checkout -b backport/61830/stable33
# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick f3874e0f 5a4737e6
# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/61830/stable33Error: Failed to check for changes with origin/stable33: No changes found in backport branch Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports. |
|
Thanks @stklcode for merging this. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
background_jobs_expiration_daysis ignored #61635Summary
This PR resolves two issues with background job cleanup introduced recently:
Portability of cleanup on Alpine, BSD and macOS
The previous implementation executes "ps" with flags -p and -o to check whether a job process was still running. This approach has portability issues regarding e.g.
ps -pfails(this affects the official "fpm-alpine" Docker image)
ps -ofailsReplace with
posix_kill($pid, 0), the standard POSIX way to probe process existence without delivering a signal.EPERMis treated as "process exists but not owned by current user". This should be available on all supported platforms.Hardcoded snowflake ID prevents
background_jobs_expiration_daysSnowflake ID in
JobRuns:deleteBefore()is computed from given timestamp, but immediately overwritten by a literal value making the expiration setting have no effect.This looks like a leftover artifact from development that should be cleaned up. Remove the hardcoded ID to restore the desired behavior.
Both fixes suggested by @mmdevl in the linked issue.
Checklist
3. to review, feature component)stable32)AI (if applicable)