chore: Add capability to crash a worker if triggered from main process - #274
Open
jason-famedly wants to merge 8 commits into
Open
chore: Add capability to crash a worker if triggered from main process#274jason-famedly wants to merge 8 commits into
jason-famedly wants to merge 8 commits into
Conversation
jason-famedly
force-pushed
the
jason/desict-redis
branch
from
July 13, 2026 11:29
7e8b974 to
3e07b87
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #274 +/- ##
==========================================
- Coverage 80.58% 80.57% -0.01%
==========================================
Files 501 501
Lines 72107 72145 +38
Branches 10848 10852 +4
==========================================
+ Hits 58105 58133 +28
- Misses 10755 10764 +9
- Partials 3247 3248 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
jason-famedly
marked this pull request as ready for review
July 13, 2026 12:03
Member
Author
|
Only test that failed on initial run was |
| ) | ||
| self.synapse_handler.new_connection(self) | ||
| await self._async_send_command(ReplicateCommand()) | ||
| self.send_command(ReplicateCommand()) |
Member
There was a problem hiding this comment.
why did you remove the await here?
Member
Author
There was a problem hiding this comment.
Look inside send_command(), it does the await as a background task, which feels kinda dumb. Just send the command. No real strong opinions otherwise, just seemed silly
Member
There was a problem hiding this comment.
Hmm, it's just that we immediately log logger.info("REPLICATE successfully sent") although it might actually fail
Member
Author
There was a problem hiding this comment.
It took me a little while to think about that, but yes I agree. Good catch. Changed it back and did the SERVER command the same way as they do not need to be background tasks in 744f1c9
| self.max_event_delay_ms = None | ||
|
|
||
| force_crash_workers_after_main_restart = config.get( | ||
| "force_crash_workers_after_main_restart", False |
Member
There was a problem hiding this comment.
we should probably document this config option somwhere
Member
Author
There was a problem hiding this comment.
yes, good call. Forgot that
Member
Author
| ServerCommand( | ||
| self.server_name, | ||
| self.hs.get_instance_name(), | ||
| self.hs.get_clock().time_msec(), |
Member
There was a problem hiding this comment.
won't this value be the time of the redis connection (re)start? This would mean that a redis restart would crash the worker.
Member
Author
There was a problem hiding this comment.
Hmm. Yes probably, good catch again
… then adding the in the sending and receiving processing
…ad of backgrounding the task
jason-famedly
force-pushed
the
jason/desict-redis
branch
2 times, most recently
from
August 4, 2026 18:53
200b373 to
2099a1c
Compare
jason-famedly
force-pushed
the
jason/desict-redis
branch
from
August 4, 2026 18:54
2099a1c to
51464f5
Compare
Member
Author
|
I strongly recommend adding a small grace period. During testing this, each process is spawned at the same time but because of the main process having a slightly different startup routine it lagged behind. This caused each worker to restart almost immediately after the main process got it's feet under it. Probably don't need more than one second |
Member
So you are saying it is okay, if the workers start when the main process isn't ready yet? Or do you want to delay the startup of the workers? |
Member
Author
|
Fascinating. The test failures for sytest on the updated round of commits are very different
From the synapse logs(server-0)
from the synapse logs(server-1)I'm not certain what to make of these, other than that they are using room version 12. Isn't that a bit ahead of schedule? EDIT: Ah, I think I see. This required the associated PR on Synapse that was sent upstream to pair with sytest changes. Will wait until v1.158 is merged, that should sort it out. EDIT 2: The complement failure appears interesting, but I am not sure how actionable it is. From the complement logs for hs2 |
Member
Author
Yes, I am saying it is ok. The main process does not always bear the responsibility of every stream. The (In theory, that actually implies that this work is not needed at all) EDIT: But I am also saying that there is no need to delay startup of the workers. One second as a grace period should be acceptable |
Member
|
Hm, I need to look into this more, before this is merged. This inverts the control of how the restart happens, which means instead of pull it is now push with a command that could get missed. I need to see how that affects things. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SYN-53
It has been found that sometimes a worker is missed when restarting containers. This adds a new top-level setting
force_crash_workers_after_main_restart(bool) to enable sending a SIGTERM signal to restart worker when triggered by redis.An existing(although currently unused) redis "command" has been reworked for this purpose to send not only the server's instance name but also the name of the worker sending the command and when that process started. It should be received by any workers currently connected. Any worker receiving this command shall check their own start up time to compare against this received value and, if this should not be the main process, raise a SIGTERM. In theory, may not even really need to include the timestamp itself, as this command alone should only be broadcast on the start of the process.
This command needs to be sent before the redis replication process starts or it may interfere with the SUBSCRIBE and REPLICATE commands that follow since we are borrowing that connection
Note to reviewer: I included the changes into the old deprecated TCP replication system as well. It is completely unused and disconnected, but is still tested by old tests for some unknown reason. It can be safely stripped out when this is removed