Skip to content

fix: recover from upstream parser panic to keep LS alive - #140

Open
SAY-5 wants to merge 2 commits into
gruntwork-io:mainfrom
SAY-5:fix/ls-nil-deref-134
Open

fix: recover from upstream parser panic to keep LS alive#140
SAY-5 wants to merge 2 commits into
gruntwork-io:mainfrom
SAY-5:fix/ls-nil-deref-134

Conversation

@SAY-5

@SAY-5 SAY-5 commented May 3, 2026

Copy link
Copy Markdown

Fixes #134.

When opening a root.hcl whose remote_state block depends on values produced by read_terragrunt_config / find_in_parent_folders that cannot be resolved during LS parsing, the upstream Terragrunt parser passes a nil *remotestate.Config to remotestate.New, which dereferences config.BackendName and panics. Because the parser runs in-process, the LS crashes for every open editor window.

This change wraps config.ParseConfigString and config.ReadStackConfigString in recover() shims at the LS boundary so a panic becomes a logged error and a nil config, matching the parser's existing error path. A regression test based on the reporter's reproducer is included.

Summary by CodeRabbit

  • Bug Fixes

    • Improved configuration parsing reliability by converting internal crashes into returned errors and preserving stack details to aid debugging.
  • Tests

    • Added a regression test to ensure malformed or unresolvable configs no longer cause the application to panic.

Review Change Stack

Wraps config.ParseConfigString and config.ReadStackConfigString in
recover() shims so an unresolvable remote_state block (read_terragrunt_config
inside a try) no longer crashes the language server. Includes a regression
test based on the reproducer.

Fixes gruntwork-io#134

Signed-off-by: SAY-5 <say.apm35@gmail.com>
@SAY-5
SAY-5 requested review from diofeher and yhakbar as code owners May 3, 2026 18:02
@coderabbitai

coderabbitai Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b23a4d58-3981-471c-bb5a-47595c0a488e

📥 Commits

Reviewing files that changed from the base of the PR and between 0844cab and b9d20e7.

📒 Files selected for processing (1)
  • internal/tg/parse_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/tg/parse_test.go

Walkthrough

This PR adds panic recovery handlers to two parsing functions. When Terragrunt's parser panics on malformed configurations, the new wrappers catch those panics, convert them to errors (with filename and stack trace), and return nil config instead of crashing the language server.

Changes

Panic-Safe Parser Wrappers

Layer / File(s) Summary
Error Handling Infrastructure
internal/tg/parse.go
Added fmt and runtime/debug imports to support error construction and stack trace capture.
Safe Wrapper Functions
internal/tg/parse.go
Introduced safeParseConfigString and safeReadStackConfigString helpers that defer recover() to intercept panics, wrap them into errors containing filename and debug.Stack(), and return nil config on panic.
Core Integration
internal/tg/parse.go
Updated ParseTerragruntBuffer and ParseStackBuffer to call their respective safe wrappers instead of invoking the underlying parser directly.
Regression Test
internal/tg/parse_test.go
Added TestParseTerragruntBuffer_DoesNotPanic with a representative root.hcl reproducer (using read_terragrunt_config and find_in_parent_folders in locals). Test asserts execution does not panic via require.NotPanics.

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

A parser once panicked in fright,
When configs gave it a bad bite,
With recover in place,
And errors to face,
The language server sleeps well at night. 😴✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding panic recovery wrappers around upstream parser calls to keep the language server alive.
Linked Issues check ✅ Passed The code changes directly address issue #134 by wrapping parser calls in panic recovery, logging errors, and preventing crashes.
Out of Scope Changes check ✅ Passed All changes are scoped to the parser boundary fix; no unrelated modifications to other areas of the codebase.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

terragrunt-ls panics with a nil-pointer dereference

1 participant