pyln-testing: don't let wait_for_log match its own forwarded announcement#9344
pyln-testing: don't let wait_for_log match its own forwarded announcement#9344ksedgwic wants to merge 1 commit into
Conversation
AF_UNIX path too long :/ |
…ment An inline plugin's Plugin() object lives in the test process, and Plugin() installs a PluginLogHandler on the root logger so that a plugin author's `logging` calls reach lightningd. In the test process that handler also forwards pyln's own machinery logs into the node's log whenever the test-side logger emits at DEBUG. In particular wait_for_logs() announces 'Waiting for [pattern]' with the pattern embedded verbatim, so the announcement lands in the very log being scanned and matches itself, silently reducing the wait to a no-op. That let test_sendpay_notifications_nowaiter race its channel close against the first payment (both payments then fail and the success assertion sees an empty list); any literal-pattern wait_for_log on an inline-plugin node is similarly voided under DEBUG logging. Attach a filter to the inline plugin's log handler that only forwards records originating outside the pyln packages, so author logging still reaches the node log but pyln internals never do. The new test covers both directions: an author log line is found by wait_for_log, and a never-logged sentinel genuinely times out (it matched instantly via the forwarded announcement before this fix). The node directory embeds the test name, and this test's long name pushed the inline-plugin.sock path past Linux's 108-byte AF_UNIX limit on the liquid-regtest CI run. Teach _inline_plugin() to fall back to binding through a directory fd alias under /proc/self/fd, the bind-side analogue of UnixSocket.connect's existing workaround; the socket file still lands where the shim's cwd-relative connect expects it. Fixes: ElementsProject#9343 Changelog-None
2a59c49 to
c23ecc1
Compare
|
Force-pushed: rebased on current master and fixed the liquid CI failure above. The test's long name pushed the inline-plugin.sock path past Linux's 108-byte AF_UNIX limit (the node directory embeds the test name, and liquid-regtest is 7 chars longer than regtest, which fit at 106). _inline_plugin() now falls back to binding through a directory fd alias under /proc/self/fd when the direct bind fails, the bind-side analogue of UnixSocket.connect's existing long-path workaround. Verified locally with the default short path and with TEST_DIR padded past the limit to force the fallback. |
| # the bind-side analogue of UnixSocket.connect's workaround; the | ||
| # socket file still lands at sock_path, where the shim's | ||
| # cwd-relative connect expects it. | ||
| if e.args[0] != "AF_UNIX path too long" or os.uname()[0] != "Linux": |
There was a problem hiding this comment.
Ideally we want tests to work on macos as well. There the limit is 104 characters.
I asked the clanker and it thinks you can switch into the directory fist and then bind via file name. Directory switching is process wide and in get_nodes we start nodes in parallel. So need to guard with a lock.
# Module-level lock serializing the (chdir, bind) critical section across
# threads. get_nodes() runs get_node() concurrently via a thread pool, and
# os.chdir() is process-wide state, so two inline-plugin binds racing here
# could each end up binding in the wrong directory.
_inline_plugin_bind_lock = threading.Lock()
def _inline_plugin(node, setup_fn):
"""Set up an inline plugin serve thread for a not-yet-started node.
Normally called via get_node(inline_plugin=setup_fn). The plugin's cwd
(set by lightningd) is node.daemon.lightning_dir/TEST_NETWORK/, which is
where the shim looks for inline-plugin.sock.
"""
sockdir = os.path.join(node.daemon.lightning_dir, TEST_NETWORK)
sockname = 'inline-plugin.sock'
sock_path = os.path.join(sockdir, sockname)
# AF_UNIX enforces a hard cap on the bind path (108 bytes on Linux,
# 104 on macOS/BSD), measured on the string passed to bind(), not the
# resolved absolute path. CI checkout dirs can exceed that, so chdir
# into the socket's directory and bind with the short relative name
# instead. The lock protects the chdir from concurrent get_node() calls
# (get_nodes() runs these in a thread pool).
srv = socket.socket(socket.AF_UNIX)
with _inline_plugin_bind_lock:
old_cwd = os.getcwd()
os.chdir(sockdir)
try:
srv.bind(sockname)
finally:
os.chdir(old_cwd)
# ... rest of listen/accept/serve-thread setup, using sock_path
# (the absolute path) wherever the socket needs to be referenced
# afterward, e.g. for cleanup/unlink.
This is the clanker suggestion but i would consider the NodeFactory own lock around the _inline_plugin call at line 1898.
Fixes #9343.
An inline plugin's
Plugin()lives in the test process, andPlugin()installs aPluginLogHandleron the root logger so a plugin author'sloggingcalls reach lightningd. In the test process that handler also forwards pyln's own machinery logs into the node's log whenever the test-side logger emits at DEBUG.wait_for_logs()announcesWaiting for [pattern]with the pattern embedded verbatim, so the announcement lands in the very log being scanned and matches itself - the wait silently becomes a no-op. That is what lettest_sendpay_notifications_nowaiterrace its channel close against the first payment (assert 0 == 1); any literal-patternwait_for_logon an inline-plugin node is similarly voided under DEBUG logging, which can mask races in tests that currently pass.The fix attaches a filter to the inline plugin's log handler that only forwards records originating outside the pyln packages: author logging still reaches the node log, pyln internals never do.
The new test proves both directions and reproduces the bug deterministically: before the fix it fails with
DID NOT RAISE TimeoutError(the sentinel wait matched its own announcement); after it, an author-logged marker is still found while the never-logged sentinel genuinely times out.