fix(cli): avoid cli-table3 wordWrap deadlock with 4+ subgraphs#2620
fix(cli): avoid cli-table3 wordWrap deadlock with 4+ subgraphs#2620kamil-gwozdz wants to merge 2 commits intowundergraph:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a paragraph-aware text-wrapping utility (TABLE_CONTENT_WIDTH, wrapText) and replaces cli-table3 wordWrap usage by pre-wrapping error/warning messages before inserting into CLI tables. Adds re-export and unit/regression tests verifying correctness and large-message performance. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/test/wrap-text.test.ts (1)
25-33: Consider tightening the assertion tolerance.The test asserts
line.length <= 35for amaxWidthof 30, which is a 16% overage. WhilewrapTextcan legitimately exceedmaxWidthfor single words longer than the limit, this test's assertion is loose enough that it wouldn't catch certain wrapping bugs.For word-boundary wrapping where words are short, lines should be at or below
maxWidth. Consider splitting the assertion:for (const line of lines) { // Lines should not exceed maxWidth unless they contain a single word longer than maxWidth const words = line.split(' '); if (words.length > 1) { expect(line.length).toBeLessThanOrEqual(30); } }This is a minor suggestion—the current test still validates the core behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/wrap-text.test.ts` around lines 25 - 33, The test's length tolerance is too loose; update the assertion in the wrap-text.test to enforce maxWidth for normal wrapped lines: iterate over lines (the lines array from result) and for each line split into words, and if words.length > 1 assert line.length <= maxWidth (use the same numeric maxWidth variable used when calling wrapText, e.g., 30); allow longer lengths only when a single word exceeds maxWidth. This change targets the test block using input, result, lines and wrapText.cli/src/handle-composition-result.ts (1)
12-12: Apply the samewrapTextfix tohandle-proposal-result.tsfor consistency.
cli/src/handle-proposal-result.tscurrently useswordWrap: truein multiple Table configurations (lines 26, 32, 38, 49, 61). Update these tables to usewrapTextfrom./utils.jsinstead, consistent with the fix applied tohandle-composition-result.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/src/handle-composition-result.ts` at line 12, handle-proposal-result.ts still uses deprecated/incorrect Table options with wordWrap: true in several Table configurations; replace those usages by importing and using the wrapText helper from ./utils.js (same as in handle-composition-result.ts). Specifically, add import { wrapText } from './utils.js' at the top of handle-proposal-result.ts if missing, then for each Table definition that sets wordWrap: true (the tables around the blocks with wordWrap at the spots corresponding to the earlier lines 26, 32, 38, 49, 61) remove wordWrap and pass wrapText for the cell wrapping (e.g., use wrapText(cellValue, width) or whatever wrapText signature is used elsewhere) so the table rendering matches handle-composition-result.ts behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cli/src/handle-composition-result.ts`:
- Line 12: handle-proposal-result.ts still uses deprecated/incorrect Table
options with wordWrap: true in several Table configurations; replace those
usages by importing and using the wrapText helper from ./utils.js (same as in
handle-composition-result.ts). Specifically, add import { wrapText } from
'./utils.js' at the top of handle-proposal-result.ts if missing, then for each
Table definition that sets wordWrap: true (the tables around the blocks with
wordWrap at the spots corresponding to the earlier lines 26, 32, 38, 49, 61)
remove wordWrap and pass wrapText for the cell wrapping (e.g., use
wrapText(cellValue, width) or whatever wrapText signature is used elsewhere) so
the table rendering matches handle-composition-result.ts behavior.
In `@cli/test/wrap-text.test.ts`:
- Around line 25-33: The test's length tolerance is too loose; update the
assertion in the wrap-text.test to enforce maxWidth for normal wrapped lines:
iterate over lines (the lines array from result) and for each line split into
words, and if words.length > 1 assert line.length <= maxWidth (use the same
numeric maxWidth variable used when calling wrapText, e.g., 30); allow longer
lengths only when a single word exceeds maxWidth. This change targets the test
block using input, result, lines and wrapText.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2743d5-d797-4e83-8bf3-7412675425e2
📒 Files selected for processing (5)
cli/src/commands/router/commands/compose.tscli/src/handle-composition-result.tscli/src/utils.tscli/src/wrap-text.tscli/test/wrap-text.test.ts
kamil-gwozdz
force-pushed
the
fix/compose-table-deadlock-2619
branch
from
1c30315 to
ea579d1
Compare
…rgraph#2619) Replace cli-table3's built-in wordWrap with a lightweight wrapText() utility that wraps text on word boundaries using simple character counting. This avoids the expensive string-width/emoji-regex code path in cli-table3 that causes the Node.js main thread to block indefinitely when rendering large error tables. Fixes: wundergraph#2619 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kamil-gwozdz
force-pushed
the
fix/compose-table-deadlock-2619
branch
from
ea579d1 to
5d92b16
Compare
kamil-gwozdz
requested review from
JivusAyrus,
StarpTech,
endigma,
jensneuse and
wilsonrivera
as code owners
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
kamil-gwozdz
force-pushed
the
fix/compose-table-deadlock-2619
branch
from
23ac6c9 to
b6a5edd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/test/compose-error-table.test.ts (2)
17-25: Avoid repeated full joins in the fixture generator.Recomputing
lines.join('\n').lengtheach loop is unnecessarily expensive for large payloads. Tracking accumulated length keeps this helper linear.Suggested patch
function generateLargeErrorMessage(sizeBytes: number): string { const lines: string[] = ['The field "id" is unresolvable at the following path:']; + let currentLength = lines[0].length; let depth = 1; - while (lines.join('\n').length < sizeBytes) { + while (currentLength < sizeBytes) { const indent = ' '.repeat(depth); - lines.push(`${indent}type${depth} {`); - lines.push(`${indent} edges {`); - lines.push(`${indent} node {`); + const nextLines = [`${indent}type${depth} {`, `${indent} edges {`, `${indent} node {`]; + lines.push(...nextLines); + currentLength += nextLines.reduce((sum, line) => sum + line.length, 0) + nextLines.length; // '\n' separators depth += 3; if (depth > 300) depth = 1; } return lines.join('\n').substring(0, sizeBytes); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/compose-error-table.test.ts` around lines 17 - 25, The loop repeatedly calls lines.join('\n').length which is O(n) each iteration; replace this with an accumulated length counter (e.g., let accumulated = lines.join('\n').length or start from 0 if lines empty) and update it whenever you push new strings: compute each newLine (using indent/type/edges/node strings), push to lines, and increment accumulated by newLine.length + 1 (for the newline separator) so the while condition uses accumulated < sizeBytes; maintain the existing depth wrap logic and substring at the end as before, referencing the variables lines, sizeBytes, depth, and indent in the diff.
53-56: The hard 1-second wall-clock gate is likely CI-flaky.Line [53] can fail intermittently on slower or contended runners even when the deadlock fix is valid. Consider a less brittle upper bound while preserving the non-hang guarantee.
Suggested patch
- expect(elapsed).toBeLessThan(1000); + expect(elapsed).toBeLessThan(4_000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/compose-error-table.test.ts` around lines 53 - 56, The test currently asserts expect(elapsed).toBeLessThan(1000) which is a brittle 1s wall-clock gate and can flake on CI; update the assertion on elapsed (the variable used in compose-error-table.test.ts) to a looser bound (for example toBeLessThan(3000)) or assert relative to the test timeout (e.g., expect(elapsed).toBeLessThan(5_000 - 500)) so the check still prevents hangs but is CI-tolerant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/test/compose-error-table.test.ts`:
- Line 45: The test currently allows one extra character by asserting
expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH + 1); which can hide
off-by-one wrap bugs; update the assertion to
expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH) so the test matches
production wrapping behavior (reference the TABLE_CONTENT_WIDTH constant and the
expect(...).toBeLessThanOrEqual call in compose-error-table.test.ts).
---
Nitpick comments:
In `@cli/test/compose-error-table.test.ts`:
- Around line 17-25: The loop repeatedly calls lines.join('\n').length which is
O(n) each iteration; replace this with an accumulated length counter (e.g., let
accumulated = lines.join('\n').length or start from 0 if lines empty) and update
it whenever you push new strings: compute each newLine (using
indent/type/edges/node strings), push to lines, and increment accumulated by
newLine.length + 1 (for the newline separator) so the while condition uses
accumulated < sizeBytes; maintain the existing depth wrap logic and substring at
the end as before, referencing the variables lines, sizeBytes, depth, and indent
in the diff.
- Around line 53-56: The test currently asserts
expect(elapsed).toBeLessThan(1000) which is a brittle 1s wall-clock gate and can
flake on CI; update the assertion on elapsed (the variable used in
compose-error-table.test.ts) to a looser bound (for example toBeLessThan(3000))
or assert relative to the test timeout (e.g., expect(elapsed).toBeLessThan(5_000
- 500)) so the check still prevents hangs but is CI-tolerant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e9d6ea6-1096-446c-96b1-68d42f349771
📒 Files selected for processing (1)
cli/test/compose-error-table.test.ts
cli/test/compose-error-table.test.ts
Outdated
| // Every line should respect the max width | ||
| for (const line of wrapped.split('\n')) { | ||
| if (line.trim().length > 0) { | ||
| expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH + 1); |
There was a problem hiding this comment.
Tighten the width assertion to avoid masking wrap regressions.
Line [45] allows TABLE_CONTENT_WIDTH + 1, but production call sites wrap with exactly TABLE_CONTENT_WIDTH. This can hide an off-by-one bug in wrapping output.
Suggested patch
- expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH + 1);
+ expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH + 1); | |
| expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/test/compose-error-table.test.ts` at line 45, The test currently allows
one extra character by asserting
expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH + 1); which can hide
off-by-one wrap bugs; update the assertion to
expect(line.length).toBeLessThanOrEqual(TABLE_CONTENT_WIDTH) so the test matches
production wrapping behavior (reference the TABLE_CONTENT_WIDTH constant and the
expect(...).toBeLessThanOrEqual call in compose-error-table.test.ts).
The previous test used minimal schemas (4 subgraphs with circular @OverRide) that only produced ~1 KB of error text. This was too small to trigger the cli-table3 wordWrap deadlock, so the test passed both with and without the fix. The new test: - Feeds 500 KB of synthetic error text (simulating deeply nested composition path resolution failures) through the fixed table rendering path - Verifies it completes in <2s (with wordWrap: true, 500 KB takes 3+ seconds and 9 MB takes hours) - Directly compares wrapText vs wordWrap performance - No longer depends on @wundergraph/composition (removes a build dependency) Reproduction repo: https://github.com/kamil-gwozdz/cosmo-compose-deadlock-repro Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kamil-gwozdz
force-pushed
the
fix/compose-table-deadlock-2619
branch
from
b6a5edd to
79fa0da
Compare
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Hi @kamil-gwozdz, thanks for your contribution! Closing this in favour of #2715 |
2 participants
Summary
Fixes #2619
wgc router composedeadlocks when federation composition produces errors and the config references 4+ subgraphs. The process prints the error header but hangs indefinitely before rendering the error table.Root Cause
cli-table3's built-in
wordWrap: trueoption triggers an expensive code path throughstring-width→emoji-regexfor every word in every table cell. With enough error content (4+ subgraphs), this blocks the Node.js main thread indefinitely, preventing signal handling and event loop progress.Fix
wrapText()utility (cli/src/wrap-text.ts) that wraps text on word boundaries using simple character counting — no heavy regex dependencies.TABLE_CONTENT_WIDTHconstant (116 = 120 column width − 2 padding − 2 borders) to avoid magic numbers.wordWrap: truefrom all affected table configurations.Files Changed
cli/src/wrap-text.tswrapText()andTABLE_CONTENT_WIDTH— zero dependenciescli/src/utils.tswrapTextfor backward compatibilitycli/src/commands/router/commands/compose.tscli/src/handle-composition-result.tscli/test/wrap-text.test.tswrapText, including a regression test verifying 500-word inputs complete in <1scli/test/compose-error-table.test.ts@override, verifying the error table renders without hanging (5s timeout)Testing
wrap-text.test.ts— 10 tests covering word wrapping edge cases, empty inputs, long words, paragraph preservation, and a performance regression test.compose-error-table.test.ts— Integration test using the exact reproduction schemas from the issue (4 subgraphs with circular@overridebetween subgraph-b and subgraph-c). RunsfederateSubgraphs, asserts composition fails, renders the error table through the fixed code path, and verifies output correctness within a 5-second timeout.Summary by CodeRabbit
Bug Fixes
Tests