Fix infinite cycles in configuration includes/extends#775
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR prevents infinite recursion when configuration files reference each other through Changes:
Technical Notes: Target paths are normalized via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| entry.to_string()), | ||
| "one.json")}; | ||
| const auto new_location{location.concat({"extends"})}; | ||
| if (!visited.emplace(target_path.native()).second) { |
There was a problem hiding this comment.
Using a single visited set for the whole traversal will treat any repeated include/extend of the same file as a “circular reference”, even when the dependency graph is acyclic (e.g., two branches both extend/include a shared file). That can reject valid configurations and the thrown ConfigurationCyclicReferenceError becomes misleading (it’s “already seen” rather than “in the current reference chain”).
Severity: medium
Other Locations
src/configuration/read.cc:100
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
src/configuration/read.cc
Outdated
|
|
||
| dereference(collections_path, configuration_path, data, {}); | ||
| std::unordered_set<std::string> visited; | ||
| visited.emplace(configuration_path.native()); |
There was a problem hiding this comment.
visited is seeded with configuration_path.native(), but targets are inserted as weakly_canonical(...) paths; if the initial configuration_path isn’t already canonicalized (e.g., contains .. or different lexical form), a cycle back to the root may not be detected. In that case the original infinite recursion could still occur for callers that don’t canonicalize before calling Configuration::read.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 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="src/configuration/read.cc">
<violation number="1" location="src/configuration/read.cc:70">
P2: `visited` is treated as global state without being cleared on return, so reusing the same include/extends target in different branches will now throw a cyclic-reference error even when there is no cycle. Track only the current recursion stack or remove the path after the recursive call completes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 2d63a7f | Previous: 5f7a697 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
21 ms |
19 ms |
1.11 |
Add one schema (100 existing) |
26 ms |
24 ms |
1.08 |
Add one schema (1000 existing) |
84 ms |
68 ms |
1.24 |
Add one schema (10000 existing) |
691 ms |
563 ms |
1.23 |
Update one schema (1 existing) |
19 ms |
18 ms |
1.06 |
Update one schema (101 existing) |
28 ms |
25 ms |
1.12 |
Update one schema (1001 existing) |
84 ms |
74 ms |
1.14 |
Update one schema (10001 existing) |
713 ms |
595 ms |
1.20 |
Cached rebuild (1 existing) |
11 ms |
10 ms |
1.10 |
Cached rebuild (101 existing) |
13 ms |
12 ms |
1.08 |
Cached rebuild (1001 existing) |
31 ms |
24 ms |
1.29 |
Cached rebuild (10001 existing) |
234 ms |
178 ms |
1.31 |
Index 100 schemas |
116 ms |
157 ms |
0.74 |
Index 1000 schemas |
1040 ms |
1213 ms |
0.86 |
Index 10000 schemas |
13740 ms |
14280 ms |
0.96 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 2d63a7f | Previous: 5f7a697 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
23 ms |
23 ms |
1 |
Add one schema (100 existing) |
27 ms |
27 ms |
1 |
Add one schema (1000 existing) |
75 ms |
77 ms |
0.97 |
Add one schema (10000 existing) |
693 ms |
619 ms |
1.12 |
Update one schema (1 existing) |
21 ms |
27 ms |
0.78 |
Update one schema (101 existing) |
27 ms |
28 ms |
0.96 |
Update one schema (1001 existing) |
76 ms |
77 ms |
0.99 |
Update one schema (10001 existing) |
610 ms |
635 ms |
0.96 |
Cached rebuild (1 existing) |
13 ms |
12 ms |
1.08 |
Cached rebuild (101 existing) |
14 ms |
14 ms |
1 |
Cached rebuild (1001 existing) |
29 ms |
29 ms |
1 |
Cached rebuild (10001 existing) |
199 ms |
208 ms |
0.96 |
Index 100 schemas |
122 ms |
123 ms |
0.99 |
Index 1000 schemas |
1095 ms |
1104 ms |
0.99 |
Index 10000 schemas |
14413 ms |
15256 ms |
0.94 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com