fix: Add manifest corruption prevention with configurable delays #1551
Conversation
This commit adds several improvements to prevent manifest corruption: 1. File Size Stability Check: Monitors file size stability before reading, ensures manifest isn't being written to during read 2. Configurable Delays: Added preRebuildDelay setting (default: 0ms), Added manifestRebuildDebounce setting (default: 1000ms), Users can increase delays if experiencing issues 3. Telemetry Events: Track manifest read errors and recovery, Monitor rebuild timings and success rates, Collect data on delay setting effectiveness Closes #[issue-number]
WalkthroughThis pull request introduces enhanced configuration options and improved manifest parsing for a dbt-related extension. The changes focus on adding several new configuration properties in Changes
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to b8abb82 in 1 minute and 18 seconds
More details
- Looked at
245lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/manifest/parsers/index.ts:312
- Draft comment:
Consider making the retry delay configurable instead of hardcoding it to 2000ms. This would allow users to adjust the delay based on their environment and needs. - Reason this comment was not posted:
Confidence changes required:50%
The code inreadAndParseManifestmethod insrc/manifest/parsers/index.tshas a potential issue with the retry logic. The delay between retries is hardcoded to 2000ms, which might not be suitable for all environments. It would be better to make this configurable, similar to other delays in the code.
2. src/manifest/parsers/index.ts:190
- Draft comment:
Please specify a return type for thereadAndParseManifestfunction to improve code readability and maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/manifest/dbtProject.ts:412
- Draft comment:
Ensure that the functiongetDebounceForRebuildManifesthas a specified return type in its signature. This comment applies to other instances where this function is used. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_8OwU9Aa7lZIVyJ0W
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/manifest/parsers/index.ts (2)
Line range hint
190-313: Consider using asynchronous file I/O operations to prevent blocking the event loopUsing synchronous file system methods like
existsSync,statSync, andreadFileSyncinside an asynchronous function can block the event loop and degrade performance. It's recommended to use their asynchronous counterparts (fs.promises.exists,fs.promises.stat, andfs.promises.readFile) to avoid blocking.Apply this diff to make the changes:
+import { promises as fsPromises } from "fs"; +const { access, stat, readFile } = fsPromises; ... - if (!existsSync(manifestLocation)) { + try { + await access(manifestLocation); + } catch { throw new Error("Manifest file does not exist"); + } ... - const stats = statSync(manifestLocation); + const stats = await stat(manifestLocation); ... - const manifestFile = readFileSync(manifestLocation, "utf8"); + const manifestFile = await readFile(manifestLocation, "utf8");
213-217: Consider making retry parameters configurableThe retry parameters
maxRetries,stabilityDelay, andmaxStabilityChecksare currently hardcoded. To provide flexibility for different environments and use cases, consider making these values configurable via workspace settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)src/manifest/dbtProject.ts(1 hunks)src/manifest/parsers/index.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
src/manifest/parsers/index.ts (1)
Line range hint
190-313: Robust implementation of manifest reading with error handling and retriesThe added logic for manifest corruption prevention, including file size stability checks, retry logic, and manifest validation, enhances the robustness of the manifest parsing process. This approach helps mitigate issues arising from incomplete or corrupted manifest files.
src/manifest/dbtProject.ts (2)
363-412: Implementation of configurable delays and telemetry in 'onSourceFileChanged' looks goodThe addition of configurable
preRebuildDelayandmanifestRebuildDebouncesettings, along with telemetry events to monitor rebuild timings and success rates, improves the flexibility and observability of the manifest rebuild process.
365-371: Validate configuration values to ensure they are within expected rangesWhile fetching
preRebuildDelayandmanifestRebuildDebouncefrom workspace settings, consider adding validation to ensure the values are within the defined minimum and maximum limits. This prevents potential issues caused by misconfiguration.package.json (1)
267-279: New configuration properties 'preRebuildDelay' and 'manifestRebuildDebounce' are well-definedThe addition of
dbt.preRebuildDelayanddbt.manifestRebuildDebouncesettings include appropriate types, descriptions, default values, and constraints. This allows users to adjust manifest rebuild timings to suit their environments.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on eac7b40 in 30 seconds
More details
- Looked at
121lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/manifest/parsers/index.ts:327
- Draft comment:
Consider making the retry delay configurable, similar tostabilityDelay, to allow users to adjust based on their needs. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/manifest/parsers/index.ts:222
- Draft comment:
Consider using early returns to reduce nesting and improve readability in thereadAndParseManifestfunction. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_RRJBiqgOR8mFEWpp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/manifest/parsers/index.ts (3)
212-215: Consider making retry parameters configurable.Since the PR objectives mention configurable delays, consider making these parameters configurable through settings:
maxRetriesstabilityDelaymaxStabilityChecksThis would allow users experiencing issues to fine-tune these values based on their specific environment.
327-327: Consider exponential backoff for retries.The current implementation uses a fixed 2-second delay between retries. Consider implementing exponential backoff to better handle varying system loads.
- await new Promise((resolve) => setTimeout(resolve, 2000)); + const backoffDelay = Math.min(2000 * Math.pow(2, attempt), 10000); // Max 10 seconds + await new Promise((resolve) => setTimeout(resolve, backoffDelay));
239-244: Consider consolidating telemetry events.The code sends similar telemetry events in multiple places. Consider creating a helper method to reduce code duplication and ensure consistent event properties.
+ private sendManifestReadTelemetry( + eventName: string, + attempt: number, + fileSize: number, + error?: Error, + stabilityChecks?: number, + ) { + const baseProperties = { + attempt: attempt.toString(), + fileSize: fileSize.toString(), + }; + + if (error) { + Object.assign(baseProperties, { + errorType: error.name || 'unknown', + errorMessage: error.message || 'unknown', + }); + } + + if (stabilityChecks !== undefined) { + Object.assign(baseProperties, { + stabilityChecks: stabilityChecks.toString(), + }); + } + + this.telemetry.sendTelemetryEvent(eventName, baseProperties); + }Also applies to: 302-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manifest/parsers/index.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/manifest/parsers/index.ts (3)
52-52: LGTM! Good error handling for manifest parsing failures.The change to async/await and the graceful handling of undefined manifest with empty cache creation is a robust approach.
201-211: Excellent documentation of the manifest corruption prevention strategy.The comments clearly explain the problem and the implemented solution strategy. This documentation will be valuable for future maintenance.
231-233: Verify manifest schema completeness.The current validation only checks for the presence of key sections. Consider enhancing the validation to:
- Check for empty objects/arrays
- Validate the structure of critical fields
- Add version checking if the manifest has a schema version
| while (stableChecks < maxStabilityChecks) { | ||
| if (!existsSync(manifestLocation)) { | ||
| throw new Error("Manifest file does not exist"); | ||
| } | ||
| const stats = statSync(manifestLocation); | ||
| const currentSize = stats.size; | ||
| lastFileSize = currentSize; | ||
|
|
||
| if (currentSize === lastSize) { | ||
| stableChecks++; | ||
| } else { | ||
| stableChecks = 0; | ||
| lastSize = currentSize; | ||
| } | ||
|
|
||
| stabilityCheckAttempts++; | ||
|
|
||
| if (stableChecks === maxStabilityChecks) { | ||
| break; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, stabilityDelay)); | ||
| } |
There was a problem hiding this comment.
Potential infinite loop in stability check.
The stability check loop could potentially run indefinitely if the file size keeps changing. Consider adding a maximum duration or total number of checks to prevent this.
+ const maxTotalChecks = 10; // Maximum number of total checks
+ let totalChecks = 0;
while (stableChecks < maxStabilityChecks) {
+ if (totalChecks >= maxTotalChecks) {
+ throw new Error("File size failed to stabilize within maximum attempts");
+ }
if (!existsSync(manifestLocation)) {
throw new Error("Manifest file does not exist");
}
const stats = statSync(manifestLocation);
const currentSize = stats.size;
lastFileSize = currentSize;
if (currentSize === lastSize) {
stableChecks++;
} else {
stableChecks = 0;
lastSize = currentSize;
}
stabilityCheckAttempts++;
+ totalChecks++;
if (stableChecks === maxStabilityChecks) {
break;
}
await new Promise((resolve) => setTimeout(resolve, stabilityDelay));
}📝 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.
| while (stableChecks < maxStabilityChecks) { | |
| if (!existsSync(manifestLocation)) { | |
| throw new Error("Manifest file does not exist"); | |
| } | |
| const stats = statSync(manifestLocation); | |
| const currentSize = stats.size; | |
| lastFileSize = currentSize; | |
| if (currentSize === lastSize) { | |
| stableChecks++; | |
| } else { | |
| stableChecks = 0; | |
| lastSize = currentSize; | |
| } | |
| stabilityCheckAttempts++; | |
| if (stableChecks === maxStabilityChecks) { | |
| break; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, stabilityDelay)); | |
| } | |
| const maxTotalChecks = 10; // Maximum number of total checks | |
| let totalChecks = 0; | |
| while (stableChecks < maxStabilityChecks) { | |
| if (totalChecks >= maxTotalChecks) { | |
| throw new Error("File size failed to stabilize within maximum attempts"); | |
| } | |
| if (!existsSync(manifestLocation)) { | |
| throw new Error("Manifest file does not exist"); | |
| } | |
| const stats = statSync(manifestLocation); | |
| const currentSize = stats.size; | |
| lastFileSize = currentSize; | |
| if (currentSize === lastSize) { | |
| stableChecks++; | |
| } else { | |
| stableChecks = 0; | |
| lastSize = currentSize; | |
| } | |
| stabilityCheckAttempts++; | |
| totalChecks++; | |
| if (stableChecks === maxStabilityChecks) { | |
| break; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, stabilityDelay)); | |
| } |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 048e285 in 55 seconds
More details
- Looked at
52lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/manifest/parsers/index.ts:211
- Draft comment:
Consider making the retry delay configurable instead of hardcoding it to 2000ms. This would provide more flexibility for users to adjust based on their system performance. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/manifest/parsers/index.ts:17
- Draft comment:
Please provide a return type for thecreateFullPathForNodefunction to improve code readability and maintainability. This comment applies to other utility functions without return types as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
3. src/manifest/parsers/index.ts:211
- Draft comment:
Use semantic versioning (semver) for version comparisons instead of direct array index comparisons. This comment applies to other similar instances as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_2k5IHYC0TPTwl3is
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/manifest/parsers/index.ts (1)
252-282:⚠️ Potential issueAdd maximum duration check to prevent infinite loops.
The stability check loop could potentially run indefinitely if the file size keeps changing.
As mentioned in a previous review, consider adding a maximum duration check:
+ const MAX_DURATION_MS = 30000; // 30 seconds maximum + const startTime = Date.now(); while (stableChecks < maxStabilityChecks) { + if (Date.now() - startTime > MAX_DURATION_MS) { + throw new Error("File size failed to stabilize within maximum duration"); + } // ... existing code ... }
🧹 Nitpick comments (1)
src/manifest/parsers/index.ts (1)
225-250: Consider extracting manifest validation into a constant.The implementation is solid, but the required sections (
nodes,sources,macros) could be defined as a constant for better maintainability.+const REQUIRED_MANIFEST_SECTIONS = ['nodes', 'sources', 'macros'] as const; + try { // ... existing code ... - if (!parsed.nodes || !parsed.sources || !parsed.macros) { + if (!REQUIRED_MANIFEST_SECTIONS.every(section => parsed[section])) { throw new Error("Manifest file appears to be incomplete"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/manifest/parsers/index.ts(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/manifest/parsers/index.ts (2)
1-1: LGTM! Good async/await implementation.The change to make
readAndParseManifestasynchronous is appropriate for implementing delays and stability checks.Also applies to: 17-17, 191-191
202-219: Excellent documentation and configuration setup!The code is well-documented with:
- Clear explanation of the manifest corruption issue
- Detailed strategy for prevention
- Sensible default values for configuration
package.json (1)
267-293: Well-structured configuration properties!The new configuration properties are well-defined with:
- Appropriate type constraints
- Reasonable min/max values
- Clear descriptions explaining their purpose and usage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/manifest/dbtProject.ts (2)
384-392: Consider enhancing telemetry with additional context.While the current implementation is good, consider:
- Using a more specific event name (e.g.,
manifestRebuildConfigSettings).- Including additional context such as the adapter type for better analytics.
- this.telemetry.sendTelemetryEvent("manifestRebuildSettings", { + this.telemetry.sendTelemetryEvent("manifestRebuildConfigSettings", { preRebuildDelay: preRebuildDelay.toString(), debounceTime: ( this.dbtProjectIntegration.getDebounceForRebuildManifest() || defaultDebounceTime ).toString(), projectName: DBTProject.hashProjectRoot(this.projectRoot.fsPath), + adapterType: this.getAdapterType(), });
394-422: Consider adding retry logic for transient failures.The current implementation handles errors well but could be enhanced with retry logic for transient failures. This would align with the PR's goal of preventing manifest corruption.
try { await this.rebuildManifest(); // Track successful rebuilds and their timing this.telemetry.sendTelemetryEvent("manifestRebuildSuccess", { duration: (Date.now() - startTime).toString(), preRebuildDelay: preRebuildDelay.toString(), projectName: DBTProject.hashProjectRoot(this.projectRoot.fsPath), }); } catch (error) { + // Retry logic for transient failures + const maxRetries = 3; + let retryCount = 0; + while (retryCount < maxRetries) { + try { + await new Promise(resolve => setTimeout(resolve, 1000 * Math.pow(2, retryCount))); // Exponential backoff + await this.rebuildManifest(); + this.telemetry.sendTelemetryEvent("manifestRebuildRetrySuccess", { + duration: (Date.now() - startTime).toString(), + retryCount: retryCount.toString(), + projectName: DBTProject.hashProjectRoot(this.projectRoot.fsPath), + }); + return; + } catch (retryError) { + retryCount++; + if (retryCount === maxRetries) { + error = retryError; // Use the last error for final reporting + } + } + } + // Track failed rebuilds this.telemetry.sendTelemetryEvent("manifestRebuildError", { duration: (Date.now() - startTime).toString(), preRebuildDelay: preRebuildDelay.toString(), projectName: DBTProject.hashProjectRoot(this.projectRoot.fsPath), errorType: error instanceof Error ? error.name : "unknown", errorMessage: error instanceof Error ? error.message : "unknown", + retryCount: retryCount.toString(), }); throw error; // Re-throw to maintain original error handling }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/manifest/dbtProject.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 GitHub Check: test (ubuntu-latest)
src/manifest/dbtProject.ts
[failure] 423-423:
Cannot find name 'defaultDebounceTime'.
🪛 GitHub Check: test (macos-latest)
src/manifest/dbtProject.ts
[failure] 423-423:
Cannot find name 'defaultDebounceTime'.
🪛 GitHub Actions: Tests
src/manifest/dbtProject.ts
[error] 423-423: Cannot find name 'defaultDebounceTime'. The variable or constant is not defined in the current scope.
🔇 Additional comments (1)
src/manifest/dbtProject.ts (1)
374-382: LGTM! Well-structured configuration retrieval.The implementation correctly retrieves the configurable delays with appropriate default values:
preRebuildDelay: 0ms (fast systems)manifestRebuildDebounce: 1000ms (1s)
| }); | ||
| throw error; // Re-throw to maintain original error handling | ||
| } | ||
| }, this.dbtProjectIntegration.getDebounceForRebuildManifest() || defaultDebounceTime), |
There was a problem hiding this comment.
Fix undefined variable error.
The variable defaultDebounceTime is used but not defined, causing pipeline failures. Use the value retrieved from workspace configuration.
- }, this.dbtProjectIntegration.getDebounceForRebuildManifest() || defaultDebounceTime),
+ }, this.dbtProjectIntegration.getDebounceForRebuildManifest() || workspace.getConfiguration("dbt").get<number>("manifestRebuildDebounce", 1000)),📝 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.
| }, this.dbtProjectIntegration.getDebounceForRebuildManifest() || defaultDebounceTime), | |
| }, this.dbtProjectIntegration.getDebounceForRebuildManifest() || workspace.getConfiguration("dbt").get<number>("manifestRebuildDebounce", 1000)), |
🧰 Tools
🪛 GitHub Check: test (ubuntu-latest)
[failure] 423-423:
Cannot find name 'defaultDebounceTime'.
🪛 GitHub Check: test (macos-latest)
[failure] 423-423:
Cannot find name 'defaultDebounceTime'.
🪛 GitHub Actions: Tests
[error] 423-423: Cannot find name 'defaultDebounceTime'. The variable or constant is not defined in the current scope.
Manifest Corruption Prevention with Configurable Delays
Problem
Users reported that the extension would stop working after about 20 minutes of usage due to manifest corruption. This occurred because:
Solution
Implemented a smart progressive retry mechanism with configurable timing controls:
Fast First Attempt (Default Path)
Smart Fallback (Only on Failure)
dbt.manifestStabilityChecks: Number of consecutive stable size checks (default: 3)dbt.manifestStabilityDelay: Delay between checks in ms (default: 1000)Rebuild Controls
dbt.preRebuildDelay: Optional delay before starting rebuild (default: 0ms)dbt.manifestRebuildDebounce: Debounce time between rebuilds (default: 1000ms)Telemetry Events
Benefits
Configuration
Users can adjust four settings based on their needs:
Manifest Reading
dbt.manifestStabilityChecks: Number of checks (1-10, default: 3)dbt.manifestStabilityDelay: Delay between checks (100-5000ms, default: 1000)Rebuild Timing
dbt.preRebuildDelay: Initial delay (0-10000ms, default: 0)dbt.manifestRebuildDebounce: Debounce period (500-10000ms, default: 1000)Recommended Values
preRebuildDelayto 2000msmanifestStabilityChecksto 5Testing
Important
Adds retry mechanism with configurable delays to prevent manifest corruption and updates telemetry for manifest operations.
readAndParseManifest()inindex.tsto prevent manifest corruption by checking file size stability.index.ts.dbt.preRebuildDelay,dbt.manifestRebuildDebounce,dbt.manifestStabilityChecks, anddbt.manifestStabilityDelaytopackage.jsonfor configurable delays.dbtProject.ts.rebuildManifest()logic indbtProject.tsto include delay settings and telemetry.This description was created by
for 048e285. It will automatically update as commits are pushed.
Summary by CodeRabbit
Release Notes
New Features
preRebuildDelay: Allows specifying a delay before manifest rebuilds.manifestRebuildDebounce: Controls the time between manifest rebuild attempts.manifestStabilityChecks: Specifies the number of checks for file size stability before reading the manifest.manifestStabilityDelay: Sets a delay between stability checks.Improvements
Performance