fix(lcm): bound error collections embedded in exception messages - #2089
Merged
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUser filter and variable execution errors now cap sampled error details in logs and exceptions while retaining total failure counts. Tests cover MUF creation failures containing 500 errors. ChangesBounded error reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/gooddata/models/user_filters/user_filter_builder.rb`:
- Around line 589-592: Move the delete_errors failure check out of the
each_slice(...).flat_map block in the user-filter deletion flow, so it runs only
after delete_results has been assigned. Then inspect the accumulated
delete_results (or each batch’s results) and preserve the existing logging and
failure message when errors are present.
- Around line 496-498: Bound serialized error samples by adding one shared
formatter that truncates rendered entries or enforces a total byte budget, then
reuse its output for both logs and exception messages at
lib/gooddata/models/user_filters/user_filter_builder.rb#L496-L498 (MUF
maqlization), `#L553-L555` (MUF creation), `#L590-L592` (MUF deletion), and
`#L662-L664` (variable maqlization). Preserve the existing error counts while
ensuring each reported sample remains within the configured bound.
In `@spec/unit/models/user_filters/user_filter_builder_spec.rb`:
- Around line 213-216: Strengthen the error-message assertions in the `raises an
error with a message bounded to the first few errors` example by verifying it
includes `user0@example.com` and `user9@example.com`, while excluding
`user10@example.com`; retain the existing count, “first 10”, and size
assertions.
🪄 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: cf657ebd-afc8-4c7d-b2de-e4c7d5a8296b
📒 Files selected for processing (2)
lib/gooddata/models/user_filters/user_filter_builder.rbspec/unit/models/user_filters/user_filter_builder_spec.rb
milandufek
added a commit
that referenced
this pull request
Jul 29, 2026
Capping only the entry count leaves the message unbounded when a single entry embeds a large value list (CI measured 21KB for 10 entries carrying 1KB details duplicated by the user-hash merge). Render samples through a shared bounded_error_sample helper that truncates each entry to 1000 chars, and assert sampled entries explicitly in the regression test. Addresses CodeRabbit review findings 1 and 3 on PR #2089. JIRA: GRIF-951 Claude-Session: https://claude.ai/code/session_015xTbPyZxxECyx93VgEfW4p
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/gooddata/models/user_filters/user_filter_builder.rb`:
- Around line 34-35: Update bounded_error_sample to avoid calling e.inspect
before truncation; replace it with a formatter that enforces a hard recursive
and total serialization budget while producing each error entry. Preserve
MAX_ERRORS_IN_MESSAGE and MAX_ERROR_ENTRY_CHARS as the limits for the number of
entries and final per-entry message size.
🪄 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: 20ef3dcd-8fff-47c4-b43f-796cd1ffa99d
📒 Files selected for processing (2)
lib/gooddata/models/user_filters/user_filter_builder.rbspec/unit/models/user_filters/user_filter_builder_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/unit/models/user_filters/user_filter_builder_spec.rb
SynchronizeUserFilters (and the variables path) raised exceptions with the
raw error Array/Hash as the message. Stringifying it runs Array#inspect /
Hash#inspect over up to one entry per user/value of a whole domain sync,
and every downstream #message / "#{e}" / JSON.pretty_generate call re-derives
the same giant string (measured 53.9 MB for 200k entries, 4 call sites) --
exhausting the JRuby heap and firing LcmJavaHeapSpaceError (~50x/month on NA3).
Cap the message (and the error log dump) to the first 10 entries plus a total
count, following the existing synchronize_users.rb pattern. Full collections
are no longer embedded in exceptions; bounded message is ~3 KB.
JIRA: GRIF-951
Claude-Session: https://claude.ai/code/session_015xTbPyZxxECyx93VgEfW4p
Capping only the entry count leaves the message unbounded when a single entry embeds a large value list (CI measured 21KB for 10 entries carrying 1KB details duplicated by the user-hash merge). Render samples through a shared bounded_error_sample helper that truncates each entry to 1000 chars, and assert sampled entries explicitly in the regression test. Addresses CodeRabbit review findings 1 and 3 on PR #2089. JIRA: GRIF-951 Claude-Session: https://claude.ai/code/session_015xTbPyZxxECyx93VgEfW4p
milandufek
force-pushed
the
md-lcm-heap-space-fix
branch
from
July 29, 2026 13:02
fe82ba6 to
68a1723
Compare
hung-nguyen-hoang
approved these changes
Jul 30, 2026
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
2 participants
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.
Why
LcmJavaHeapSpaceErrorfires ~50×/month on NA3 (GRIF-951):java.lang.OutOfMemoryError: Java heap spacein the user_filters brick duringGoodData::LCM2::SynchronizeUserFilters, withRubyHash.inspect/RubyArray.inspectframes in the stack trace.Root cause:
UserFilterBuilder.execute_mufs(and the variables path inexecute) raise exceptions with the raw error Array/Hash embedded as the message:Stringifying that collection runs
inspectover up to one entry per user/value across an entire domain sync — and Ruby re-derives the string on every downstream.message/"#{e}"/JSON.pretty_generatecall (4 such call sites vialcm2.rb,run_brick.rb,execution_result_middleware.rb). Measured: 200k error entries → 53.9 MB message per derivation. Container memory bumps can't fix this (NA3 already went 4Gi→5Gi→6Gi with no effect).What
MAX_ERRORS_IN_MESSAGE = 10entries plus a total count, at all four raise sites — same pattern already used bysynchronize_users.rb. Bounded message: ~3 KB.Creating/Deleting MUFs resulted in errors) are preserved.count: 500, first 10.Notes for reviewers
FilterMaqlizationErroris a bareRuntimeError; the only consumers read.message(both inspec/integration/user_filters_spec.rb), and for ≤10 errors the message still contains the full detail, so those tests are unaffected.failis currently dead code (since b7adb56delete_resultsis nil inside its own defining block). The bounding here is defense-in-depth for when that gets fixed separately.JIRA: GRIF-951
🤖 Generated with Claude Code
https://claude.ai/code/session_015xTbPyZxxECyx93VgEfW4p
Summary by CodeRabbit