Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

poetry install over a running instance causes processes to segfault #15461

Closed
DMRobertson opened this issue Apr 20, 2023 · 4 comments · Fixed by #15570
Closed

poetry install over a running instance causes processes to segfault #15461

DMRobertson opened this issue Apr 20, 2023 · 4 comments · Fixed by #15570
Labels
A-Packaging Our Debian packages, docker images; or issues relevant to downstream packagers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases

Comments

@DMRobertson
Copy link
Contributor

DMRobertson commented Apr 20, 2023

When we looking into this, we saw that poetry install is "editing" the shared object file rather than replacing it.

3510767 stat("/home/erikj/synapse/target/debug/libsynapse.so", {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 stat("/home/erikj/synapse/synapse/synapse_rust.abi3.so", {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 stat("/home/erikj/synapse/target/debug/libsynapse.so", {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 stat("/home/erikj/synapse/synapse/synapse_rust.abi3.so", {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 openat(AT_FDCWD, "/home/erikj/synapse/target/debug/libsynapse.so", O_RDONLY|O_CLOEXEC) = 3
3510767 fstat(3, {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 ioctl(3, TCGETS, 0x7ffd23d7be30) = -1 ENOTTY (Inappropriate ioctl for device)
3510767 lseek(3, 0, SEEK_CUR)           = 0
3510767 openat(AT_FDCWD, "/home/erikj/synapse/synapse/synapse_rust.abi3.so", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 4
3510767 fstat(4, {st_mode=S_IFREG|0755, st_size=0, ...}) = 0
3510767 ioctl(4, TCGETS, 0x7ffd23d7be30) = -1 ENOTTY (Inappropriate ioctl for device)
3510767 lseek(4, 0, SEEK_CUR)           = 0
3510767 fstat(3, {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 sendfile(4, 3, [0] => [35088552], 35088552) = 35088552
3510767 sendfile(4, 3, [35088552], 35088552) = 0
3510767 close(4)                        = 0
3510767 close(3)                        = 0
3510767 stat("/home/erikj/synapse/synapse/synapse_rust.abi3.so", {st_mode=S_IFREG|0755, st_size=35088552, ...}) = 0
3510767 chmod("/home/erikj/synapse/synapse/synapse_rust.abi3.so", 0100755) = 0

Erik investigated further and ultimately submitted a patch to setuptools-rust: PyO3/setuptools-rust#295. This was accepted, but has yet to be released.


Once a newer setuptools-rust release is available, we should take advantage of this to ensure that we don't accidentally segfault running processes when we actually want to do a rolling restart.

At the moment, setuptools_rust is currently upper-bounded at the current release version:

synapse/pyproject.toml

Lines 366 to 371 in ae69d69

# The upper bounds here are defensive, intended to prevent situations like
# #13849 and #14079 where we see buildtime or runtime errors caused by build
# system changes.
# We are happy to raise these upper bounds upon request,
# provided we check that it's safe to do so (i.e. that CI passes).
requires = ["poetry-core>=1.0.0,<=1.5.0", "setuptools_rust>=1.3,<=1.5.2"]

which will presumably need bumping; confusingly we also have a lower bound here:

synapse/pyproject.toml

Lines 212 to 220 in ae69d69

# This is for building the rust components during "poetry install", which
# currently ignores the `build-system.requires` directive (c.f.
# https://github.com/python-poetry/poetry/issues/6154). Both `pip install` and
# `poetry build` do the right thing without this explicit dependency.
#
# This isn't really a dev-dependency, as `poetry install --no-dev` will fail,
# but the alternative is to add it to the main list of deps where it isn't
# needed.
setuptools_rust = ">=1.3"

@DMRobertson DMRobertson added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases labels Apr 20, 2023
@DMRobertson
Copy link
Contributor Author

Once a newer setuptools-rust release is available

Until then, the current workaround is to

  • mv synapse_rust_abi.so synapse_rust_abi.so.backup
  • and then poetry install

which should keep currently active processes running.

@DMRobertson DMRobertson added the A-Packaging Our Debian packages, docker images; or issues relevant to downstream packagers label Apr 20, 2023
@DMRobertson
Copy link
Contributor Author

#15512 means that anything using the poetry environment should now include the fix.

@landryb
Copy link

landryb commented May 11, 2023

#15512 means that anything using the poetry environment should now include the fix.

shouldnt the dependency in https://github.com/matrix-org/synapse/blame/develop/pyproject.toml#L371 be relaxed to accept 1.6.0 now ?

@DMRobertson
Copy link
Contributor Author

Urgh, yes it should. Thank you for spotting.

@DMRobertson DMRobertson reopened this May 11, 2023
DMRobertson pushed a commit that referenced this issue May 11, 2023
This was bumped by dependabot in #15512, but we didn't bump also raise
the version guard here. I don't know how we can avoid this happening in
the future.

Closes #15461.

Spotted in [1] by @landryb.

[1]: #15461 (comment)
DMRobertson pushed a commit that referenced this issue May 11, 2023
* Allow `pip install` to use setuptools_rust 1.6.0

This was bumped by dependabot in #15512, but we didn't bump also raise
the version guard here. I don't know how we can avoid this happening in
the future.

Closes #15461.

Spotted in [1] by @landryb.

[1]: #15461 (comment)

* Changelog
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Packaging Our Debian packages, docker images; or issues relevant to downstream packagers T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants