Skip to content

Commit 84d6425

Browse files
Remove sentinel logcontext where we log in setup, start and exit (#18870)
Remove `sentinel` logcontext where we log in `setup`, `start`, and exit. Instead of having one giant PR that removes all places we use `sentinel` logcontext, I've decided to tackle this more piece-meal. This PR covers the parts if you just startup Synapse and exit it with no requests or activity going on in between. Part of #18905 (Remove `sentinel` logcontext where we log in Synapse) Prerequisite for #18868. Logging with the `sentinel` logcontext means we won't know which server the log came from. ### Why https://github.com/element-hq/synapse/blob/9cc400177822805e2a08d4d934daad6f3bc2a4df/docs/log_contexts.md#L71-L81 (docs updated in #18900) ### Testing strategy 1. Run Synapse normally and with `daemonize: true`: `poetry run synapse_homeserver --config-path homeserver.yaml` 1. Execute some requests 1. Shutdown the server 1. Look for any bad log entries in your homeserver logs: - `Expected logging context sentinel but found main` - `Expected logging context main was lost` - `Expected previous context` - `utime went backwards!`/`stime went backwards!` - `Called stop on logcontext POST-0 without recording a start rusage` 1. Look for any logs coming from the `sentinel` context With these changes, you should only see the following logs (not from Synapse) using the `sentinel` context if you start up Synapse and exit: `homeserver.log` ``` 2025-09-10 14:45:39,924 - asyncio - 64 - DEBUG - sentinel - Using selector: EpollSelector 2025-09-10 14:45:40,562 - twisted - 281 - INFO - sentinel - Received SIGINT, shutting down. 2025-09-10 14:45:40,562 - twisted - 281 - INFO - sentinel - (TCP Port 9322 Closed) 2025-09-10 14:45:40,563 - twisted - 281 - INFO - sentinel - (TCP Port 8008 Closed) 2025-09-10 14:45:40,563 - twisted - 281 - INFO - sentinel - (TCP Port 9093 Closed) 2025-09-10 14:45:40,564 - twisted - 281 - INFO - sentinel - Main loop terminated. ```
1 parent 2bed3fb commit 84d6425

File tree

5 files changed

+53
-33
lines changed

5 files changed

+53
-33
lines changed

changelog.d/18870.misc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Remove `sentinel` logcontext usage where we log in `setup`, `start` and exit.

synapse/app/_base.py

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@
7272
from synapse.events.presence_router import load_legacy_presence_router
7373
from synapse.handlers.auth import load_legacy_password_auth_providers
7474
from synapse.http.site import SynapseSite
75-
from synapse.logging.context import PreserveLoggingContext
75+
from synapse.logging.context import LoggingContext, PreserveLoggingContext
7676
from synapse.logging.opentracing import init_tracer
7777
from synapse.metrics import install_gc_manager, register_threadpool
7878
from synapse.metrics.background_process_metrics import run_as_background_process
@@ -183,25 +183,23 @@ def run() -> None:
183183
if gc_thresholds:
184184
gc.set_threshold(*gc_thresholds)
185185
install_gc_manager()
186-
run_command()
187186

188-
# make sure that we run the reactor with the sentinel log context,
189-
# otherwise other PreserveLoggingContext instances will get confused
190-
# and complain when they see the logcontext arbitrarily swapping
191-
# between the sentinel and `run` logcontexts.
192-
#
193-
# We also need to drop the logcontext before forking if we're daemonizing,
194-
# otherwise the cputime metrics get confused about the per-thread resource usage
195-
# appearing to go backwards.
196-
with PreserveLoggingContext():
197-
if daemonize:
198-
assert pid_file is not None
187+
# Reset the logging context when we start the reactor (whenever we yield control
188+
# to the reactor, the `sentinel` logging context needs to be set so we don't
189+
# leak the current logging context and erroneously apply it to the next task the
190+
# reactor event loop picks up)
191+
with PreserveLoggingContext():
192+
run_command()
193+
194+
if daemonize:
195+
assert pid_file is not None
196+
197+
if print_pidfile:
198+
print(pid_file)
199199

200-
if print_pidfile:
201-
print(pid_file)
200+
daemonize_process(pid_file, logger)
202201

203-
daemonize_process(pid_file, logger)
204-
run()
202+
run()
205203

206204

207205
def quit_with_error(error_string: str) -> NoReturn:
@@ -601,10 +599,12 @@ def run_sighup(*args: Any, **kwargs: Any) -> None:
601599
hs.get_datastores().main.db_pool.start_profiling()
602600
hs.get_pusherpool().start()
603601

602+
def log_shutdown() -> None:
603+
with LoggingContext("log_shutdown"):
604+
logger.info("Shutting down...")
605+
604606
# Log when we start the shut down process.
605-
hs.get_reactor().addSystemEventTrigger(
606-
"before", "shutdown", logger.info, "Shutting down..."
607-
)
607+
hs.get_reactor().addSystemEventTrigger("before", "shutdown", log_shutdown)
608608

609609
setup_sentry(hs)
610610
setup_sdnotify(hs)

synapse/app/generic_worker.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,12 @@ def start(config_options: List[str]) -> None:
355355
except Exception as e:
356356
handle_startup_exception(e)
357357

358-
register_start(_base.start, hs)
358+
async def start() -> None:
359+
# Re-establish log context now that we're back from the reactor
360+
with LoggingContext("start"):
361+
await _base.start(hs)
362+
363+
register_start(start)
359364

360365
# redirect stdio to the logs, if configured.
361366
if not hs.config.logging.no_redirect_stdio:

synapse/app/homeserver.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -377,15 +377,17 @@ def setup(config_options: List[str]) -> SynapseHomeServer:
377377
handle_startup_exception(e)
378378

379379
async def start() -> None:
380-
# Load the OIDC provider metadatas, if OIDC is enabled.
381-
if hs.config.oidc.oidc_enabled:
382-
oidc = hs.get_oidc_handler()
383-
# Loading the provider metadata also ensures the provider config is valid.
384-
await oidc.load_metadata()
380+
# Re-establish log context now that we're back from the reactor
381+
with LoggingContext("start"):
382+
# Load the OIDC provider metadatas, if OIDC is enabled.
383+
if hs.config.oidc.oidc_enabled:
384+
oidc = hs.get_oidc_handler()
385+
# Loading the provider metadata also ensures the provider config is valid.
386+
await oidc.load_metadata()
385387

386-
await _base.start(hs)
388+
await _base.start(hs)
387389

388-
hs.get_datastores().main.db_pool.updates.start_doing_background_updates()
390+
hs.get_datastores().main.db_pool.updates.start_doing_background_updates()
389391

390392
register_start(start)
391393

synapse/util/daemonize.py

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,11 @@
2929
from types import FrameType, TracebackType
3030
from typing import NoReturn, Optional, Type
3131

32+
from synapse.logging.context import (
33+
LoggingContext,
34+
PreserveLoggingContext,
35+
)
36+
3237

3338
def daemonize_process(pid_file: str, logger: logging.Logger, chdir: str = "/") -> None:
3439
"""daemonize the current process
@@ -64,8 +69,14 @@ def daemonize_process(pid_file: str, logger: logging.Logger, chdir: str = "/") -
6469
pid_fh.write(old_pid)
6570
sys.exit(1)
6671

67-
# Fork, creating a new process for the child.
68-
process_id = os.fork()
72+
# Stop the existing context *before* we fork the process. Otherwise the cputime
73+
# metrics get confused about the per-thread resource usage appearing to go backwards
74+
# because we're comparing the resource usage from the original process to the forked
75+
# process. `PreserveLoggingContext` already takes care of restarting the original
76+
# context *after* the block.
77+
with PreserveLoggingContext():
78+
# Fork, creating a new process for the child.
79+
process_id = os.fork()
6980

7081
if process_id != 0:
7182
# parent process: exit.
@@ -140,9 +151,10 @@ def sigterm(signum: int, frame: Optional[FrameType]) -> NoReturn:
140151

141152
# Cleanup pid file at exit.
142153
def exit() -> None:
143-
logger.warning("Stopping daemon.")
144-
os.remove(pid_file)
145-
sys.exit(0)
154+
with LoggingContext("atexit"):
155+
logger.warning("Stopping daemon.")
156+
os.remove(pid_file)
157+
sys.exit(0)
146158

147159
atexit.register(exit)
148160

0 commit comments

Comments
 (0)