fix(core): Harden event log parse against malformed input#29004
fix(core): Harden event log parse against malformed input#29004
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/eventbus/message-event-bus-writer/message-event-bus-log-writer.ts">
<violation number="1" location="packages/cli/src/eventbus/message-event-bus-writer/message-event-bus-log-writer.ts:262">
P1: The new max-total-per-file check undercounts processed lines, so the parser can still do unbounded work on files with many confirm lines.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| // Absolute ceiling applied in every mode, counting skipped lines too | ||
| // so 100%-malformed files also trip it. | ||
| if ( | ||
| results.loggedMessages.length - baselineCount + parseSkipped >= |
There was a problem hiding this comment.
P1: The new max-total-per-file check undercounts processed lines, so the parser can still do unbounded work on files with many confirm lines.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/eventbus/message-event-bus-writer/message-event-bus-log-writer.ts, line 262:
<comment>The new max-total-per-file check undercounts processed lines, so the parser can still do unbounded work on files with many confirm lines.</comment>
<file context>
@@ -239,10 +254,29 @@ export class MessageEventBusLogWriter {
+ // Absolute ceiling applied in every mode, counting skipped lines too
+ // so 100%-malformed files also trip it.
+ if (
+ results.loggedMessages.length - baselineCount + parseSkipped >=
+ maxTotalMessagesPerFile
+ ) {
</file context>
There was a problem hiding this comment.
true, but this PR still leaves so code safer, so i'd keep it like that for now
There was a problem hiding this comment.
Thanks for confirming—understood, we’ll keep it as is for now.
Performance ComparisonComparing current → latest master → 14-day baseline Idle baseline with Instance AI module loaded
Memory consumption baseline with starter plan resources
docker-stats
How to read this table
|
Bundle ReportBundle size has no change ✅ |
|
Got released with |
Summary
Hardens
MessageEventBusLogWriterread-side so heap usage stays bounded when parsing event log files that contain malformed lines. Two defensive changes:logger.errorwith the raw line interpolated into the message template. In tight-loop parses this allocates per-line memory faster than the runtime can reclaim.readLoggedMessagesFromFilenow counts skipped lines in stack-local state and emits a single aggregatedlogger.warnat the end of each parse pass, with a 200-char truncated sample for ops triage.maxMessagesPerParseguard (PR fix(core): Guard event log parsing against unbounded memory growth #28594) only applies in non-'all'modes and only counts successful parses, so it doesn't bound pathological files at all. A newN8N_EVENTBUS_LOGWRITER_MAXTOTALMESSAGESPERFILE(default500_000) caps total lines processed in every mode, with skipped lines counted toward the total so 100%-malformed files also trip it.Empirically validated with a micro-benchmark: 100k tight-loop
logger.errorcalls with a 10 kB line interpolated allocate ~1 GB of heap; collapsing to a single aggregated warn drops the delta to 0 MB and the wall time from 2 s to 1 ms.How to test manually
Aggregate-warn path:
errorlines and visible memory growth during boot. After: a singlewarnof the formEvent log parse skipped 10000 malformed line(s) in <file>. Sample (truncated): not-json-..., no per-line errors, flat memory.Total-ceiling path:
N8N_EVENTBUS_LOGWRITER_MAXTOTALMESSAGESPERFILE=100.seq 1 500 > ~/.n8n/n8nEventLog.log).warn:Event log <file> exceeded 100 total lines during parse; aborting to prevent out-of-memory.Healthy-file regression:
With no config changes and a healthy event log, behavior is byte-identical — no new warns, no dropped messages. Covered by the four pre-existing unit tests, which pass unmodified.
Key implementation decisions
processLoggedLinelets exceptions propagate instead of logging-and-swallowing. Aggregation lives in the caller (readLoggedMessagesFromFile) where stack-local counters are natural. The raw line is no longer baked into any log message — the truncated sample in the aggregate warn is sufficient for triage.maxTotalMessagesPerFile. Closes the 100%-malformed-file case the existingmaxMessagesPerParseguard misses. Existing guard left intact to preserve PR fix(core): Guard event log parsing against unbounded memory growth #28594's semantics for the orphaned-messages scenario.send()fanout amplification viaMessageEventBus.initialize(re-emits every parsed message to all log-streaming destinations including Sentry NodeClient) and the unboundedErrorReporter.seenErrorsSet. This PR is partial-defence-in-depth against both (bounded parse → bounded fanout) but doesn't fix them at their source. Separate tickets to follow.Related Links
Review / Merge checklist
Backport to Beta,Backport to Stable, orBackport to v1(if the PR is an urgent fix that needs to be backported)