Repository navigation
plugin: fix stdio framing that crashed lightningd on concatenated reads - #81
Merged
Merged
Conversation
✅ Deploy Preview for cln4go canceled.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 439acc9bec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1f448e192
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
vincenzopalazzo
added a commit
that referenced
this pull request
May 7, 2026
Two follow-ups from the Codex review on PR #81: 1. The bufio.Scanner reader had a 16 MB frame-size cap; any single JSON-RPC message larger than that returned bufio.ErrTooLong and terminated the read loop, dropping every subsequent request. The previous (broken) implementation had no cap. Replace the scanner with a bufio.Reader + ReadBytes('\n') loop that accumulates lines until the "\n\n" delimiter is seen, with no fixed cap — memory is bounded only by the largest single message. 2. writeMessage was void: when encoding/json could not marshal a response (e.g. result contained a channel, function, or NaN/Inf float), the failure was traced via the optional tracer and the request id went unanswered, making lightningd hang. Make writeMessage return its first error, and have dispatchRequest fall back to a minimal JSON-RPC error response (code -32603, "Internal error" per the JSON-RPC 2.0 spec) so the request id is always answered. Tests: - TestRunHandlesLargeFrame parses a 1 MiB params payload — far above the previous 16 MB cap is now uncapped, but this size also exceeds bufio.Scanner's 64 KB default buffer, so it would have failed under any earlier scanner-based variant. - TestDispatchSendsErrorOnUnencodableResponse registers an RPC that returns a function value, drives one request through run(), and asserts that the framework answers with a -32603 error response carrying the original id rather than dropping the reply. All five pre-existing tests still pass under -race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The framework's Start() loop was vulnerable to a JSON-RPC framing bug
that could panic the plugin and, since plugins are typically marked
important, take down lightningd with it.
Three issues compounded:
1. The stdin loop detected message boundaries with the heuristic
`bytesResp1 < buffSize`, which is unreliable on streams. When two
JSON-RPC messages happened to be delivered in a single Read() the
loop concatenated them into one buffer, json.Unmarshal failed
with "invalid character '{' after top-level value", and the
framework panicked.
2. Responses written to stdout were not terminated with the "\n\n"
delimiter the CLN plugin protocol uses. Combined with #1 on the
reading side, peers had no reliable way to split messages either.
3. Log() created a fresh bufio.NewWriter(os.Stdout) on every call,
completely separate from the writer used by the response loop.
Concurrent calls (and even sequential ones across the two
writers) could interleave bytes on stdout.
Replace the read loop with a bufio.Reader.ReadBytes loop that
accumulates lines until the "\n\n" delimiter, with no fixed size cap
- memory is bounded only by the largest single message in the
stream. Funnel every outbound message - responses and log
notifications - through a single shared bufio.Writer guarded by
sync.Mutex, with the "\n\n" delimiter appended after each one.
writeMessage returns the first encode/write/flush error, and
dispatchRequest falls back to a minimal -32603 ("Internal error" per
JSON-RPC 2.0) response when the original cannot be serialized, so
the request id is always answered. Parse failures now log and
continue rather than panic, so a single corrupt frame can no longer
terminate the plugin.
Also fix the invalid CLN log level "warn" -> "unusual" used when an
RPC method returns a non-JSONRPCError.
Tests cover concatenated reads, fragmented reads, malformed-frame
recovery, the delimiter contract, concurrent writes from many
goroutines (race-detector clean), 1 MiB frames (no cap), and the
encode-failure fallback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mock-server handlers in client/unix_unit_test.go intentionally ignore I/O errors — the test cases assert the *client* behaviour and the server side just needs to drain the request, optionally write a fixed response, and close. errcheck flagged 13 such call sites, breaking the lint job in CI. Assign the return values to `_` to make the intent explicit and keep the linter quiet, without changing test behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Integration testing workflow had zero historical runs because multiple deprecated dependencies silently prevented it from registering: - actions/checkout@v2 and actions/upload-artifact@v2 were retired by GitHub Actions and silently no longer trigger workflows that reference them. - 'docker-compose' (with hyphen) was removed from the ubuntu-latest runner; only 'docker compose' (the v2 plugin) is available. - The integration container pinned Go 1.18.4, which predates the 'toolchain' directive added to go.work. The current go.work specifies 'toolchain go1.24.2' so the test entrypoint failed with: "reading go.work: ... unknown directive: toolchain". Bump the action versions to v4, switch to 'docker compose', match the Go install to the toolchain directive, and add workflow_dispatch so the integration suite can be triggered manually from the Actions tab or via 'gh workflow run'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
fix/plugin-stdio-framing
branch
from
May 7, 2026 11:06
3ceae84 to
c318e29
Compare
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
1 participant
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.
Summary
Fixes a JSON-RPC framing bug in the plugin runtime that could panic the
plugin and, since plugins are typically marked important, take down
lightningd with it. Discovered while debugging
vincenzopalazzo/cln-offers#35,
which fixes the cln-offers-side trigger but acknowledges the framework
fragility as separate work.
The crash signature in production logs was:
Root cause
Three compounding issues in
plugin/plugin.go:bytesResp1 < buffSize. This is unreliable on streams: if two JSON-RPCmessages get delivered in a single
Read()(which happens when the OSpipe buffer fills, or when log notifications race a request), they end
up concatenated in the same buffer,
json.Unmarshalfails withinvalid character '{' after top-level value, and the frameworkpanics.
\n\ndelimiter that the CLN plugin protocol uses to separate messages.
Combined with cln: implement unix client to talk with core lightning #1 on the reading side, peers had no reliable way to
split messages either.
Log()writer. It created a freshbufio.NewWriter(os.Stdout)onevery call, separate from the writer the response loop used.
Concurrent calls and even sequential calls across the two writers
could interleave bytes on stdout.
Changes
bufio.Scannerwhose split function emitseach chunk between two consecutive
\n\ndelimiters as a discretetoken. Each
Scan()returns exactly one JSON-RPC message regardless ofhow the bytes were delivered.
*bufio.Writerfield onPlugin[T]guarded bysync.Mutex. Every outbound message (responses and log notifications)is funneled through one
writeMessagehelper that appends the\n\ndelimiter and flushes under the lock.
Log("broken", ...)and continue, insteadof panicking. A single corrupt frame can no longer terminate the
plugin.
"warn"->"unusual"in thenon-
JSONRPCErrorfallback path (valid levels areio|debug|info|unusual|broken).run(io.Reader, io.Writer)method sothe framing behaviour is unit-testable without touching real stdio.
Test plan
plugin/plugin_io_test.goadds five tests, all passing under-race:TestRunHandlesConcatenatedRequests— two requests in onestrings.NewReaderproduce two correctly-id'd responses(reproduces the original crash without the fix).
TestRunHandlesSplitRequest— achunkedReaderthat returns themessage in two
Read()calls is reassembled correctly.TestRunRecoversFromMalformedRequest— a garbage frame followedby a valid one results in one response and a
failed to parse requestlog notification (no panic).TestWriteMessageIncludesDelimiter— every emitted message endswith
\n\nand contains no internal\n\n.TestLogAndResponseDoNotInterleave— 16 goroutines × 32 messagesvia
writeMessageproduce 512 cleanly-framed JSON objects with nointerleaving.
golangci-lint runandgofmt -lare both clean for the plugin module.The pre-existing
TestCallFistMethod/TestOptionValueExistintegrationtests still require
CLN_UNIX_SOCKETto be set — unchanged by this PR.🤖 Generated with Claude Code