Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make HTTP server wait for tasks before shutdown #1390

Merged
merged 2 commits into from
Nov 10, 2024
Merged

Conversation

iofq
Copy link
Contributor

@iofq iofq commented Nov 8, 2024

Fixes #1335

Description of the Change

This code change prevents SIGINT/SIGTERM signals from stopping the Aptly API server before it has cleaned up any resource locks in the database. Normally for CLI aptly commands, it would clear these locks if it detects the PID that had the previous lock is no longer running. However, if Aptly is running as an API server in a container, it always runs as PID 1, which means the PID no longer works as a unique identifier to detect what is running at any given time, leaving these locks indefinitely.

Even with this MR, this issue can still present itself if an Aptly container is SIGKILLed (docker rm -f), however that is a more difficult problem to solve that would likely take some significant rearchitecting (since the API was tacked on to aptly after the fact).

Checklist

  • unit-test added (if change is algorithm)

@iofq iofq requested a review from a team November 8, 2024 20:35
@neolynx
Copy link
Member

neolynx commented Nov 9, 2024

excellent work ! thanks a lot :)

I ran a coverage build: #1391 - 60.00% of diff hit (target 74.86%), which is good enough, but I was wondering if we can somehow test the Ctrl-C automatically. There is already a test for parallel operation with threads... do you think it would make sense to try to test this ?

@iofq
Copy link
Contributor Author

iofq commented Nov 9, 2024

excellent work ! thanks a lot :)

I ran a coverage build: #1391 - 60.00% of diff hit (target 74.86%), which is good enough, but I was wondering if we can somehow test the Ctrl-C automatically. There is already a test for parallel operation with threads... do you think it would make sense to try to test this ?

I added a unit test to cover the added lines. As far as a system test, I messed a bit but could not figure out how to cleanly shut down and re-init the test harness mid-test easily

@neolynx neolynx self-requested a review November 10, 2024 14:37
Copy link
Member

@neolynx neolynx left a comment

Choose a reason for hiding this comment

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

thanks !

@neolynx neolynx merged commit 147955c into aptly-dev:master Nov 10, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aptly DB deadlock when running in container as PID 1
2 participants