Revert "feat(assembler): on-demand assembler serve with live reload and improved frontend DX"#3437
Merged
Merged
Conversation
…nd impro…" This reverts commit 5cab154.
Mpdreamz
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
Contributor
📝 WalkthroughWalkthroughThis PR eliminates the dynamic live-reload assembler serving pipeline by removing AssemblerServeWebHost and AssemblerReloadService, switching the Serve command from on-demand rendering to static file hosting. Corresponding changes simplify resource ownership patterns (removing IDisposable from Aws3LinkIndexReader, the ownsReader parameter from CrossLinkFetcher, and mid-flight invalidation from DocumentationSet). The build system is updated to replace Watch_Full with Watch, and CI/Aspire configuration is updated to use the new Serve command. AssemblerBuilder's public API is consolidated by removing CreateGenerator and BuildOneAsync methods. Sequence Diagram(s)Not applicable. This PR is primarily a refactoring that removes live-reload functionality and simplifies disposal patterns; it does not introduce new multi-component interactions or significantly alter control flow beyond removing complexity. Possibly related PRs
🚥 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. ✨ Finishing Touches✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs`:
- Around line 61-62: CrossLinkFetcher is disposing the injected ILoggerFactory
(logFactory) in its Dispose implementation which it does not own; remove the
call that disposes logFactory inside CrossLinkFetcher.Dispose (or remove the
IDisposable implementation if the class has no other owned disposable state).
Ensure Dispose still disposes only owned resources (e.g.,
ScopedFileSystem/fileSystem) and suppress finalization if appropriate, but do
not call logFactory.Dispose() or otherwise assume ownership of ILoggerFactory.
In
`@src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs`:
- Line 19: The constructor for DocSetConfigurationCrossLinkFetcher currently
calls Aws3LinkIndexReader.CreateAnonymous() inline in multiple places, producing
two distinct readers where only one should be used; fix by creating a single
local variable (e.g., var reader = linkIndexProvider ??
Aws3LinkIndexReader.CreateAnonymous()) and pass that same reader instance
wherever the code currently calls Aws3LinkIndexReader.CreateAnonymous() (notably
the base CrossLinkFetcher(...) call and the later usage around lines 34–35) so
the same reader and its cache are reused by DocSetConfigurationCrossLinkFetcher.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96c712c1-92a8-4193-b2e7-f3b130198018
📒 Files selected for processing (15)
.github/workflows/ci.ymlaspire/AppHost.csbuild/CommandLine.fsbuild/Targets.fsdocs/cli-schema.jsonsrc/Elastic.Documentation.LinkIndex/LinkIndexReader.cssrc/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cssrc/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cssrc/Elastic.Markdown/IO/DocumentationSet.cssrc/services/Elastic.Documentation.Assembler/AssembleSources.cssrc/services/Elastic.Documentation.Assembler/Building/AssemblerBuilder.cssrc/tooling/docs-builder/Commands/Assembler/AssemblerCommands.cssrc/tooling/docs-builder/Http/AssemblerReloadService.cssrc/tooling/docs-builder/Http/AssemblerServeWebHost.cssrc/tooling/docs-builder/Http/ReloadableGeneratorState.cs
💤 Files with no reviewable changes (5)
- src/tooling/docs-builder/Http/AssemblerServeWebHost.cs
- src/tooling/docs-builder/Http/AssemblerReloadService.cs
- src/tooling/docs-builder/Http/ReloadableGeneratorState.cs
- build/Targets.fs
- build/CommandLine.fs
Comment on lines
+61
to
62
| public abstract class CrossLinkFetcher(ILoggerFactory logFactory, ILinkIndexReader linkIndexProvider, ScopedFileSystem? fileSystem = null) : IDisposable | ||
| { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant parts of CrossLinkFetcher.cs (class header + Dispose region)
FILE="src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "=== Class declaration (around 55-75) ==="
nl -ba "$FILE" | sed -n '55,80p'
echo
echo "=== Dispose() implementation (around 180-240) ==="
nl -ba "$FILE" | sed -n '180,240p'
echo
echo "=== Search for Dispose calls / IDisposable implementation in this file ==="
rg -n "IDisposable|Dispose\(" "$FILE"Repository: elastic/docs-builder
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "=== Class declaration (lines 55-80) ==="
awk 'NR>=55 && NR<=80 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Dispose() implementation (lines 180-240) ==="
awk 'NR>=180 && NR<=240 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Search for IDisposable/Dispose in this file ==="
rg -n "IDisposable|Dispose\(" "$FILE"Repository: elastic/docs-builder
Length of output: 2558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find derived types of CrossLinkFetcher
rg -n ":\s*CrossLinkFetcher" -S src
# Find any calls to base.Dispose() in derived classes
rg -n "base\.Dispose\(\)" -S src
# Find Dispose overrides/implementations that might interact with CrossLinkFetcher
rg -n "override\s+void\s+Dispose|void\s+Dispose\(" -S src | rg -n "CrossLinkFetcher|CrossLink" || trueRepository: elastic/docs-builder
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for Dispose patterns in all classes that derive from CrossLinkFetcher
rg -n "class\s+.+:\s*CrossLinkFetcher|:\s*CrossLinkFetcher" src
echo "---- Derived Dispose overrides (override/Dispose methods) ----"
rg -n "override\s+void\s+Dispose\s*\(|\bvoid\s+Dispose\s*\(" src/services/Elastic.Documentation.Assembler/Links/AssemblerCrossLinkFetcher.cs src/Elastic.Documentation.Links/InboundLinks/LinkIndexCrossLinkFetcher.cs src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs
echo "---- base.Dispose() calls in those derived files ----"
rg -n "base\.Dispose\s*\(" src/services/Elastic.Documentation.Assembler/Links/AssemblerCrossLinkFetcher.cs src/Elastic.Documentation.Links/InboundLinks/LinkIndexCrossLinkFetcher.cs src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs
echo "---- Check if derived types explicitly implement IDisposable beyond the base ----"
rg -n "IDisposable" src/services/Elastic.Documentation.Assembler/Links/AssemblerCrossLinkFetcher.cs src/Elastic.Documentation.Links/InboundLinks/LinkIndexCrossLinkFetcher.cs src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.csRepository: elastic/docs-builder
Length of output: 668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
"src/services/Elastic.Documentation.Assembler/Links/AssemblerCrossLinkFetcher.cs"
"src/Elastic.Documentation.Links/InboundLinks/LinkIndexCrossLinkFetcher.cs"
"src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs"
)
echo "---- Other derived declarations across src (': CrossLinkFetcher') ----"
rg -n ":\s*CrossLinkFetcher" src || true
echo "---- Dispose( occurrences in known derived files ----"
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "== $f =="
rg -n "Dispose\s*\(" "$f" || true
else
echo "Missing $f"
fi
doneRepository: elastic/docs-builder
Length of output: 984
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all registrations/usages mentioning CrossLinkFetcher or its derived types
rg -n "CrossLinkFetcher|AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher" src -S -g'*.cs'
echo
echo "---- check ScopedFileSystem type ----"
# Find the declaration file for ScopedFileSystem
fd -a "ScopedFileSystem" src || true
rg -n "class\s+ScopedFileSystem|record\s+ScopedFileSystem|struct\s+ScopedFileSystem|interface\s+ScopedFileSystem" src -S -g'*.cs' || true
echo
echo "---- check whether ScopedFileSystem implements IDisposable ----"
rg -n "ScopedFileSystem.*IDisposable|IDisposable.*ScopedFileSystem" src -S -g'*.cs' || true
echo
echo "---- inspect derived fetcher files for constructors/fields ----"
for f in \
"src/services/Elastic.Documentation.Assembler/Links/AssemblerCrossLinkFetcher.cs" \
"src/Elastic.Documentation.Links/InboundLinks/LinkIndexCrossLinkFetcher.cs" \
"src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs"
do
if [ -f "$f" ]; then
echo "== $f (top 140 lines) =="
head -n 140 "$f" | nl -ba | sed -n '1,140p'
fi
doneRepository: elastic/docs-builder
Length of output: 4181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find definition of ScopedFileSystem ==="
rg -n "(\bclass\b|\brecord\b|\bstruct\b|\binterface\b)\s+ScopedFileSystem\b" src -S -g'*.cs' || true
echo
echo "=== Check whether ScopedFileSystem is IDisposable ==="
rg -n "ScopedFileSystem.*IDisposable|IDisposable.*ScopedFileSystem" src -S -g'*.cs' || true
echo
echo "=== Show usages of CrossLinkFetcher-derived types to see how they are disposed ==="
rg -n -C 3 "new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" src -S -g'*.cs'
echo
echo "=== Search for explicit Dispose() calls on those fetchers ==="
rg -n "(\.Dispose\(\)|using\s+var)\s*(fetcher|crossLinkFetcher|_crossLinkFetcher)\b" src -S -g'*.cs' || trueRepository: elastic/docs-builder
Length of output: 9552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Files likely involved in lifecycle/disposal
targets=(
"src/services/Elastic.Documentation.Assembler/AssembleSources.cs"
"src/services/Elastic.Documentation.Assembler/ContentSources/RepositoryPublishValidationService.cs"
"src/services/Elastic.Documentation.Assembler/Navigation/NavigationPrefixChecker.cs"
"src/Elastic.Documentation.Links/InboundLinks/LinkIndexLinkChecker.cs"
"src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs"
"src/tooling/docs-builder/Http/ReloadableGeneratorState.cs"
)
for f in "${targets[@]}"; do
echo "==== $f (around instantiation) ===="
rg -n "new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" "$f" -S || true
# show a window around each match
rg -n -S "new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" "$f" | cut -d: -f1 | head -n 5 | while read -r line; do
start=$((line-15)); end=$((line+15))
sed -n "${start},${end}p" "$f" | sed "s/^/[$f:$start] /" >/dev/null
done
# simpler: just print a safe chunk near the first match line if exists
first=$(rg -n -S "new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" "$f" | head -n1 | cut -d: -f1 || true)
if [ -n "${first:-}" ]; then
start=$((first-30)); end=$((first+60))
[ $start -lt 1 ] && start=1
echo "---- context $start-$end ----"
sed -n "${start},${end}p" "$f"
fi
echo
done
echo "==== ReloadableGeneratorState dispose implementation ===="
rg -n "Dispose\s*\(" src/tooling/docs-builder/Http/ReloadableGeneratorState.cs -S || true
echo
if [ -f "src/tooling/docs-builder/Http/ReloadableGeneratorState.cs" ]; then
sed -n '1,220p' src/tooling/docs-builder/Http/ReloadableGeneratorState.cs
fiRepository: elastic/docs-builder
Length of output: 367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
show_chunk () {
local file="$1" start="$2" end="$3"
echo "---- $file [$start-$end] ----"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d\t%s\n", NR, $0}' "$file"
echo
}
targets=(
"src/services/Elastic.Documentation.Assembler/AssembleSources.cs"
"src/services/Elastic.Documentation.Assembler/ContentSources/RepositoryPublishValidationService.cs"
"src/services/Elastic.Documentation.Assembler/Navigation/NavigationPrefixChecker.cs"
"src/Elastic.Documentation.Links/InboundLinks/LinkIndexLinkChecker.cs"
"src/services/Elastic.Documentation.Isolated/IsolatedBuildService.cs"
"src/tooling/docs-builder/Http/ReloadableGeneratorState.cs"
"src/Elastic.Codex/Building/CodexBuildService.cs"
)
patterns=(
"new[[:space:]]+AssemblerCrossLinkFetcher"
"new[[:space:]]+LinksIndexCrossLinkFetcher"
"new[[:space:]]+DocSetConfigurationCrossLinkFetcher"
)
# Print first instantiation context for each known file
for f in "${targets[@]}"; do
[ -f "$f" ] || continue
for p in "${patterns[@]}"; do
line=$(rg -n -S "$p" "$f" | head -n1 | cut -d: -f1 || true)
if [ -n "${line:-}" ]; then
start=$((line-25)); end=$((line+60))
[ $start -lt 1 ] && start=1
show_chunk "$f" "$start" "$end"
fi
done
done
echo "==== ReloadableGeneratorState.cs Dispose implementation ===="
[ -f "src/tooling/docs-builder/Http/ReloadableGeneratorState.cs" ] && rg -n "Dispose\s*\(" src/tooling/docs-builder/Http/ReloadableGeneratorState.cs -S || true
[ -f "src/tooling/docs-builder/Http/ReloadableGeneratorState.cs" ] && show_chunk "src/tooling/docs-builder/Http/ReloadableGeneratorState.cs" 1 220
echo "==== Any Dispose calls on CrossLinkFetcher instances ===="
rg -n -S "\.Dispose\s*\(\s*\)\s*;|using\s+var\s+\w+\s*=\s*new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)" src -g'*.cs' || true
echo "==== Any disposal of ILoggerFactory instances (logFactory.Dispose) ===="
rg -n -S "logFactory\.Dispose\(|_logFactory\.Dispose\(|ILoggerFactory.*Dispose\(" src -g'*.cs' || trueRepository: elastic/docs-builder
Length of output: 39105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs"
echo "=== CrossLinkFetcher.cs (around class header/fields/constructor params) ==="
awk 'NR>=55 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== CrossLinkFetcher.cs (around end / Dispose / owned resources) ==="
awk 'NR>=120 && NR<=230 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "=== Search for disposal of specific fetcher variables ==="
rg -n -S "(crossLinkFetcher|_crossLinkFetcher)\.Dispose\(" src -g'*.cs' || true
rg -n -S "using\s+var\s+\w+\s*=\s*new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" src -g'*.cs' || true
rg -n -S "\bDispose\(\)\s*;\s*.*CrossLinkFetcher|CrossLinkFetcher.*Dispose\(" src -g'*.cs' || true
echo
echo "=== Search for any cast/dispose of CrossLinkFetcher instances ==="
rg -n -S "(\(IDisposable\).*)?CrossLinkFetcher.*\.Dispose\(" src -g'*.cs' || true
rg -n -S "\b(IDisposable)\s*\)\s*.*CrossLinkFetcher" src -g'*.cs' || true
echo
echo "=== CrossLinkFetcher constructor ownership: look for stored members ==="
rg -n -S "_fileSystem|linkIndexProvider|_linkIndexProvider|_cachedLinkReferences|_linkIndex" "$FILE"Repository: elastic/docs-builder
Length of output: 8200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Any using-var or explicit Dispose for CrossLinkFetcher-derived types ==="
rg -n -S "using\s+var\s+\w+\s*=\s*new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" src -g'*.cs' || true
rg -n -S "\b(new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b).*\bDispose\s*\(" src -g'*.cs' || true
rg -n -S "(CrossLinkFetcher|AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b" src -g'*.cs' | head -n 200
echo
echo "=== Any Dispose calls where the variable type is IDisposable and created from CrossLinkFetcher ==="
rg -n -S "(IDisposable)\s+\w+\s*=\s*new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b|new\s+(AssemblerCrossLinkFetcher|LinksIndexCrossLinkFetcher|DocSetConfigurationCrossLinkFetcher)\b\s*\)|\.Dispose\s*\(\s*\)" src -g'*.cs' || true
echo
echo "=== Any casts to IDisposable around CrossLinkFetcher instances ==="
rg -n -S "\(IDisposable\)\s*\w+.*CrossLinkFetcher|CrossLinkFetcher.*\(IDisposable\)" src -g'*.cs' || trueRepository: elastic/docs-builder
Length of output: 7006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Look for explicit LoggerFactory creation ==="
rg -n -S "new\s+LoggerFactory|LoggerFactory\.Create" src -g'*.cs' || true
echo
echo "=== Look for explicit ILoggerFactory.Dispose calls outside CrossLinkFetcher ==="
rg -n -S "(\.Dispose\(\)\s*;|\bDispose\(\)\b)\s*$" src -g'*.cs' | rg -n "ILoggerFactory|logFactory" || true
echo
echo "=== Look for any using/Dispose of CrossLinkFetcher instances (not just fetcher type) ==="
rg -n -S "using\s+var\s+\w+\s*=\s*new\s+.*CrossLinkFetcher|\.Dispose\(\)\s*;.*CrossLinkFetcher|_crossLinkFetcher\s*\.Dispose" src -g'*.cs' || trueRepository: elastic/docs-builder
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Call sites for AssembleSources.AssembleAsync( ... , ILoggerFactory logFactory, ...) ==="
rg -n "AssembleSources\.AssembleAsync\s*\(" src -g'*.cs' || true
echo
echo "=== Call sites for ReloadableGeneratorState constructor ==="
rg -n "new\s+ReloadableGeneratorState\s*\(" src -g'*.cs' || true
echo
echo "=== DI registrations for ReloadableGeneratorState (if any) ==="
rg -n "AddSingleton<\s*ReloadableGeneratorState|AddScoped<\s*ReloadableGeneratorState|AddTransient<\s*ReloadableGeneratorState" src -g'*.cs' || true
echo
echo "=== Show ReloadableGeneratorState.Dispose implementation (dispose of _crossLinkFetcher/logFactory?) ==="
rg -n "void\s+Dispose\s*\(" src/tooling/docs-builder/Http/ReloadableGeneratorState.cs -g'*.cs' || true
sed -n '240,310p' src/tooling/docs-builder/Http/ReloadableGeneratorState.cs || trueRepository: elastic/docs-builder
Length of output: 1863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Any explicit .Dispose() on CrossLinkFetcher types/vars ==="
rg -n -S "\.Dispose\(\)\s*;|using\s+var\s+\w+\s*=\s*new\s+\w*CrossLinkFetcher" src -g'*.cs' || true
echo
echo "=== Any Dispose() call where the receiver is a variable named like *fetcher* that holds a CrossLinkFetcher ==="
rg -n -S "Dispose\(\)\s*;|using\s+var\s+\w+\s*=\s*new\s+.*CrossLinkFetcher" src/services src/tooling -g'*.cs' || true
echo
echo "=== Inspect DocumentationWebHost.cs for ILoggerFactory source ==="
FILE="src/tooling/docs-builder/Http/DocumentationWebHost.cs"
if [ -f "$FILE" ]; then
sed -n '1,220p' "$FILE"
else
echo "Missing $FILE"
fiRepository: elastic/docs-builder
Length of output: 12591
Do not dispose the injected ILoggerFactory in CrossLinkFetcher
CrossLinkFetcher implements IDisposable and calls logFactory.Dispose() in Dispose() (around lines 207-210), but ILoggerFactory is injected and not owned by this class—disposing it can break logging for other components sharing the same factory. Remove the base Dispose()/IDisposable (or at least stop disposing logFactory).
🔧 Minimal fix
-public abstract class CrossLinkFetcher(ILoggerFactory logFactory, ILinkIndexReader linkIndexProvider, ScopedFileSystem? fileSystem = null) : IDisposable
+public abstract class CrossLinkFetcher(ILoggerFactory logFactory, ILinkIndexReader linkIndexProvider, ScopedFileSystem? fileSystem = null)
{
@@
- public void Dispose()
- {
- logFactory.Dispose();
- GC.SuppressFinalize(this);
- }
}🤖 Prompt for 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.
In `@src/Elastic.Documentation.Links/CrossLinks/CrossLinkFetcher.cs` around lines
61 - 62, CrossLinkFetcher is disposing the injected ILoggerFactory (logFactory)
in its Dispose implementation which it does not own; remove the call that
disposes logFactory inside CrossLinkFetcher.Dispose (or remove the IDisposable
implementation if the class has no other owned disposable state). Ensure Dispose
still disposes only owned resources (e.g., ScopedFileSystem/fileSystem) and
suppress finalization if appropriate, but do not call logFactory.Dispose() or
otherwise assume ownership of ILoggerFactory.
| ILinkIndexReader? linkIndexProvider = null, | ||
| ILinkIndexReader? codexLinkIndexReader = null) | ||
| : CrossLinkFetcher(logFactory, linkIndexProvider ?? Aws3LinkIndexReader.CreateAnonymous(), ownsReader: linkIndexProvider is null) | ||
| : CrossLinkFetcher(logFactory, linkIndexProvider ?? Aws3LinkIndexReader.CreateAnonymous()) |
Contributor
There was a problem hiding this comment.
Reuse a single public link-index reader.
Line 19 and Line 34 each call Aws3LinkIndexReader.CreateAnonymous() when no reader is supplied. That creates two separate readers; only the second one is used here, so the base instance is dead weight and its cache never helps.
🔧 One way to keep a single reader
public class DocSetConfigurationCrossLinkFetcher(
ILoggerFactory logFactory,
ConfigurationFile configuration,
ILinkIndexReader? linkIndexProvider = null,
ILinkIndexReader? codexLinkIndexReader = null)
- : CrossLinkFetcher(logFactory, linkIndexProvider ?? Aws3LinkIndexReader.CreateAnonymous())
+ : CrossLinkFetcher(logFactory, linkIndexProvider ??= Aws3LinkIndexReader.CreateAnonymous())
{
+ private readonly ILinkIndexReader _publicReader = linkIndexProvider;
private readonly ILogger _logger = logFactory.CreateLogger(nameof(DocSetConfigurationCrossLinkFetcher));
private readonly ILinkIndexReader? _codexReader = codexLinkIndexReader;
@@
- var publicReader = linkIndexProvider ?? Aws3LinkIndexReader.CreateAnonymous();
+ var publicReader = _publicReader;Also applies to: 34-35
🤖 Prompt for 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.
In
`@src/Elastic.Documentation.Links/CrossLinks/DocSetConfigurationCrossLinkFetcher.cs`
at line 19, The constructor for DocSetConfigurationCrossLinkFetcher currently
calls Aws3LinkIndexReader.CreateAnonymous() inline in multiple places, producing
two distinct readers where only one should be used; fix by creating a single
local variable (e.g., var reader = linkIndexProvider ??
Aws3LinkIndexReader.CreateAnonymous()) and pass that same reader instance
wherever the code currently calls Aws3LinkIndexReader.CreateAnonymous() (notably
the base CrossLinkFetcher(...) call and the later usage around lines 34–35) so
the same reader and its cache are reused by DocSetConfigurationCrossLinkFetcher.
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
1 participant
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.
Reverts #3179
Was too out of date when merging caused failures on main.