mirror of
https://github.com/element-hq/synapse.git
synced 2026-07-11 22:49:31 +00:00
ff034c1f62
This was all done through claude. ## Fix flaky `test_edu_large_messages_not_splitting_one_user` (`TooLong` under `trial -jN`) ### Problem The "Build .deb packages" CI step intermittently failed with: ``` twisted.protocols.amp.TooLong ...in tests.rest.client.test_sendtodevice.SendToDeviceTestCase.test_edu_large_messages_not_splitting_one_user ``` The deb build runs the suite with `twisted.trial -j2`. In that mode, worker log events are shipped to the manager process over Twisted's AMP protocol, which encodes each value with a 2-byte length prefix — so any single log line of **64 KiB or more** raises `TooLong`. ### Root cause Not a bug in the to-device/EDU logic, and **not** debug logging enabled by the build (it runs at the default `ERROR` level). It's a **log-level leak between tests sharing a `-j2` worker**: - `tests/logging/test_loggers.py::ExplicitlyConfiguredLoggerTestCase` calls `root_logger.setLevel(logging.DEBUG)` directly and never restores it (no `setUp`/`tearDown`/`addCleanup`). - When that test runs before `test_edu_large_messages_not_splitting_one_user` **in the same worker process**, the root logger is left at `DEBUG`. - That test deliberately builds an EDU of exactly `SOFT_MAX_EDU_SIZE - 1` (65 535) bytes. Storing it triggers `synapse/storage/database.py`'s `[SQL values]` DEBUG log, which dumps the full query params — producing a **65 708-byte** line that overflows AMP's cap. It looks "flaky" purely because of `-j2` scheduling: whether the two tests land on the same worker, and in what order. ### Fix Three commits: 1. **Restore the root logger level in `ExplicitlyConfiguredLoggerTestCase`** — `addCleanup` to put the level back. Fixes the root cause. 2. **Truncate oversized log lines in the test log handler** (`ToTwistedHandler.emit`) — caps lines at 1000 chars so no debug line can break `trial -jN`, regardless of which query is logged (defense in depth). 3. **Truncate values in `[SQL values]` debug logging** — caps the logged param repr at 1000 chars (guarded by `isEnabledFor(DEBUG)` to keep the hot path lazy). Keeps production debug logs sane too. ### Testing - The reproduction (`trial -j2 tests.logging.test_loggers <the EDU test>`) went from **~3/8 failing** to **10/10 passing**. - Confirmed the root level is restored after the logging tests, and that the `[SQL values]` line is now capped (~50 KB with a `[truncated]` marker, was 65 708). - `ruff` + `mypy` clean; `tests.logging.test_loggers` and `tests.rest.client.test_sendtodevice` pass (14/14). --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Eric Eastwood <erice@element.io>
141 lines
5.4 KiB
Python
141 lines
5.4 KiB
Python
#
|
|
# This file is licensed under the Affero General Public License (AGPL) version 3.
|
|
#
|
|
# Copyright (C) 2025 New Vector, Ltd
|
|
#
|
|
# This program is free software: you can redistribute it and/or modify
|
|
# it under the terms of the GNU Affero General Public License as
|
|
# published by the Free Software Foundation, either version 3 of the
|
|
# License, or (at your option) any later version.
|
|
#
|
|
# See the GNU Affero General Public License for more details:
|
|
# <https://www.gnu.org/licenses/agpl-3.0.html>.
|
|
#
|
|
#
|
|
#
|
|
import logging
|
|
|
|
from synapse.logging.loggers import ExplicitlyConfiguredLogger
|
|
|
|
from tests.unittest import TestCase
|
|
|
|
|
|
class ExplicitlyConfiguredLoggerTestCase(TestCase):
|
|
def setUp(self) -> None:
|
|
super().setUp()
|
|
|
|
# These tests mutate the root logger's level (see the various
|
|
# `root_logger.setLevel(...)` calls below). Make sure we restore the
|
|
# original level afterwards, otherwise we leak a raised log level into
|
|
# the other tests running in the same worker process. A raised level can
|
|
# cause oversized debug log lines which break `trial -jN` (worker log
|
|
# events are shipped to the manager over trials's AMP, which caps each
|
|
# value at 64KiB, c.f. https://github.com/twisted/twisted/issues/12482).
|
|
root_logger = logging.getLogger()
|
|
self.addCleanup(root_logger.setLevel, root_logger.level)
|
|
|
|
def _create_explicitly_configured_logger(self) -> logging.Logger:
|
|
original_logger_class = logging.getLoggerClass()
|
|
logging.setLoggerClass(ExplicitlyConfiguredLogger)
|
|
logger = logging.getLogger("test")
|
|
# Restore the original logger class
|
|
logging.setLoggerClass(original_logger_class)
|
|
|
|
return logger
|
|
|
|
def test_no_logs_when_not_set(self) -> None:
|
|
"""
|
|
Test to make sure that nothing is logged when the logger is *not* explicitly
|
|
configured.
|
|
"""
|
|
root_logger = logging.getLogger()
|
|
root_logger.setLevel(logging.DEBUG)
|
|
|
|
logger = self._create_explicitly_configured_logger()
|
|
|
|
with self.assertLogs(logger=logger, level=logging.NOTSET) as cm:
|
|
# XXX: We have to set this again because of a Python bug:
|
|
# https://github.com/python/cpython/issues/136958 (feel free to remove once
|
|
# that is resolved and we update to a newer Python version that includes the
|
|
# fix)
|
|
logger.setLevel(logging.NOTSET)
|
|
|
|
logger.debug("debug message")
|
|
logger.info("info message")
|
|
logger.warning("warning message")
|
|
logger.error("error message")
|
|
|
|
# Nothing should be logged since the logger is *not* explicitly configured
|
|
#
|
|
# FIXME: Remove this whole block once we update to Python 3.10 or later and
|
|
# have access to `assertNoLogs` (replace `assertLogs` with `assertNoLogs`)
|
|
self.assertIncludes(
|
|
set(cm.output),
|
|
set(),
|
|
exact=True,
|
|
)
|
|
# Stub log message to avoid `assertLogs` failing since it expects at least
|
|
# one log message to be logged.
|
|
logger.setLevel(logging.INFO)
|
|
logger.info("stub message so `assertLogs` doesn't fail")
|
|
|
|
def test_logs_when_explicitly_configured(self) -> None:
|
|
"""
|
|
Test to make sure that logs are emitted when the logger is explicitly configured.
|
|
"""
|
|
root_logger = logging.getLogger()
|
|
root_logger.setLevel(logging.INFO)
|
|
|
|
logger = self._create_explicitly_configured_logger()
|
|
|
|
with self.assertLogs(logger=logger, level=logging.DEBUG) as cm:
|
|
logger.debug("debug message")
|
|
logger.info("info message")
|
|
logger.warning("warning message")
|
|
logger.error("error message")
|
|
|
|
self.assertIncludes(
|
|
set(cm.output),
|
|
{
|
|
"DEBUG:test:debug message",
|
|
"INFO:test:info message",
|
|
"WARNING:test:warning message",
|
|
"ERROR:test:error message",
|
|
},
|
|
exact=True,
|
|
)
|
|
|
|
def test_is_enabled_for_not_set(self) -> None:
|
|
"""
|
|
Test to make sure `logger.isEnabledFor(...)` returns False when the logger is
|
|
not explicitly configured.
|
|
"""
|
|
|
|
logger = self._create_explicitly_configured_logger()
|
|
|
|
# Unset the logger (not configured)
|
|
logger.setLevel(logging.NOTSET)
|
|
|
|
# The logger shouldn't be enabled for any level
|
|
self.assertFalse(logger.isEnabledFor(logging.DEBUG))
|
|
self.assertFalse(logger.isEnabledFor(logging.INFO))
|
|
self.assertFalse(logger.isEnabledFor(logging.WARNING))
|
|
self.assertFalse(logger.isEnabledFor(logging.ERROR))
|
|
|
|
def test_is_enabled_for_info(self) -> None:
|
|
"""
|
|
Test to make sure `logger.isEnabledFor(...)` returns True any levels above the
|
|
explicitly configured level.
|
|
"""
|
|
|
|
logger = self._create_explicitly_configured_logger()
|
|
|
|
# Explicitly configure the logger to `INFO` level
|
|
logger.setLevel(logging.INFO)
|
|
|
|
# The logger should be enabled for INFO and above once explicitly configured
|
|
self.assertFalse(logger.isEnabledFor(logging.DEBUG))
|
|
self.assertTrue(logger.isEnabledFor(logging.INFO))
|
|
self.assertTrue(logger.isEnabledFor(logging.WARNING))
|
|
self.assertTrue(logger.isEnabledFor(logging.ERROR))
|