retry find files if nothing is returned #1615
Conversation
--- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/OneCyrus/vscode-dbt-power-user?shareId=XXXX-XXXX-XXXX-XXXX).
WalkthroughThe pull request introduces a new private asynchronous method, Changes
Sequence Diagram(s)sequenceDiagram
participant DP as discoverProjects
participant RB as retryWithBackoff
participant WF as workspace.findFiles
DP ->> RB: Invoke retryWithBackoff(fn, retries, backoff)
loop Retry Attempts
RB ->> WF: Call findFiles
WF -->> RB: Return file list (empty or valid)
alt Returned empty list
RB ->> RB: Log debug message, throw error to retry
else Valid project list
RB -->> DP: Return projects
end
end
alt Maximum retries reached
RB -->> DP: Throw error "no projects found after maximum retries"
end
Assessment against linked issues
Suggested reviewers
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 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 (
|
OneCyrus
changed the title
![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 0dfb9fd in 2 minutes and 17 seconds
More details
- Looked at
72lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/manifest/dbtWorkspaceFolder.ts:88
- Draft comment:
Using retryWithBackoff on workspace.findFiles is appropriate for the known bug. However, please ensure that an empty result indeed always indicates a failure case. In some configurations, an empty file list might be valid, so consider clarifying this assumption. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. src/utils.ts:468
- Draft comment:
Consider adding debug logging of each failed attempt in retryWithBackoff to better trace intermittent failures and backoff delays. This can simplify troubleshooting if the bug recurs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. src/manifest/dbtWorkspaceFolder.ts:88
- Draft comment:
The retryWithBackoff is a neat solution but consider handling the scenario where an empty array is a valid result (i.e. no dbt projects in the workspace) to avoid unnecessary retries. Adding a log or condition to differentiate a genuine error from a valid empty result might improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is speculative - it assumes empty arrays should be handled differently but provides no clear evidence this is needed. An empty array is a valid result meaning no projects were found. The retryWithBackoff is likely there to handle actual errors like filesystem issues, not empty results. The code already handles empty results gracefully through the subsequent filtering and processing.
I could be wrong about the purpose of retryWithBackoff - maybe there are legitimate cases where empty results should bypass retries. The implementation of retryWithBackoff isn't visible here.
Without seeing the retryWithBackoff implementation or evidence of actual issues, this seems like unnecessary speculation. The code appears to handle empty results appropriately already.
This comment should be deleted as it makes speculative suggestions without clear evidence of an actual problem to solve.
4. src/utils.ts:468
- Draft comment:
Consider adding logging within retryWithBackoff for each retry attempt (e.g. logging the attempt count and error details) to assist with debugging if the underlying function keeps returning an empty array. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While logging could be helpful for debugging, it's not strictly necessary for the function to work. The comment is more of a nice-to-have suggestion rather than pointing out a clear issue. Debug logging is often an implementation detail left to the developer's discretion. The function works correctly without logging.
The function is handling error cases silently which could make debugging harder in production. Having some form of logging could help track retry patterns and identify recurring issues.
However, logging strategy is often an application-wide concern that should be consistent across the codebase. Adding logging here without knowing the broader logging approach could be inconsistent.
The comment should be removed as it's more of an optional suggestion than a required change. The function works correctly without logging.
5. src/manifest/dbtWorkspaceFolder.ts:62
- Draft comment:
Typo: The variable 'nonFilteredAlolowListFolders' appears to contain a misspelling. Consider renaming it to 'nonFilteredAllowListFolders' for clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/manifest/dbtWorkspaceFolder.ts:307
- Draft comment:
Typo: The function name 'notInDBtPackages' mixes casing. For consistency with other identifiers (e.g., 'DBTTerminal', 'DBTProject'), consider renaming it to 'notInDBTPackages'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_IIdQQWGTsxHqNrVj
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 (3)
src/utils.ts (2)
477-479: Consider making the empty array check configurable.The current implementation throws an error when an empty array is returned, which works for the
findFilesuse case. However, for greater reusability in other contexts, you might want to make this check optional via a parameter.-export async function retryWithBackoff<T>( - fn: () => Thenable<T>, - retries: number = 5, - backoff: number = 1000, +export async function retryWithBackoff<T>( + fn: () => Thenable<T>, + retries: number = 5, + backoff: number = 1000, + treatEmptyArrayAsFailure: boolean = true, ): Promise<T> { let attempt = 0; while (attempt < retries) { try { const result = await fn(); - if (Array.isArray(result) && result.length === 0) { + if (treatEmptyArrayAsFailure && Array.isArray(result) && result.length === 0) { throw new Error("Empty array returned"); } return result;
486-486: Consider adding jitter to the backoff delay.In distributed systems, adding randomness (jitter) to backoff delays can help prevent thundering herd problems when multiple clients retry at the same time.
- await new Promise((resolve) => setTimeout(resolve, backoff * attempt)); + const jitter = Math.random() * 0.3 + 0.85; // Random value between 0.85 and 1.15 + await new Promise((resolve) => setTimeout(resolve, backoff * attempt * jitter));src/manifest/dbtWorkspaceFolder.ts (1)
88-99: Consider adding logging during retries.To help with debugging and monitoring the retry behavior, consider adding logging in the
retryWithBackofffunction or logging retry attempts here.const dbtProjectFiles = await retryWithBackoff( () => workspace.findFiles( new RelativePattern( this.workspaceFolder, `**/${DBTProject.DBT_PROJECT_FILE}`, ), new RelativePattern(this.workspaceFolder, excludePattern), ), 5, 1000, + ).catch(error => { + this.dbtTerminal.error( + "discoverProjects", + "Failed to find project files after multiple retries", + error + ); + return []; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/manifest/dbtWorkspaceFolder.ts(2 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/utils.ts (1)
src/dbt_client/dbtTerminal.ts (1) (1)
error(105-125)
🔇 Additional comments (3)
src/utils.ts (1)
468-490: Well-implemented retry mechanism with exponential backoff.The
retryWithBackofffunction provides a robust solution for handling transient failures. It correctly implements:
- Exponential backoff with a configurable delay
- Proper error propagation after maximum retries
- Special handling for empty arrays
This is a great pattern that can be reused for other async operations that might fail intermittently.
src/manifest/dbtWorkspaceFolder.ts (2)
29-29: LGTM! Import for the new retry utility function.The import statement correctly brings in the
retryWithBackofffunction from the utils module.
88-99: Effectively addresses VSCode's findFiles unreliability issue.The implementation properly wraps the
workspace.findFilescall with the retry mechanism, addressing the project discovery failures described in the PR objectives. The 5 retries with a 1-second increasing backoff is a reasonable configuration.
src/utils.ts
Outdated
| export async function retryWithBackoff<T>( | ||
| fn: () => Thenable<T>, | ||
| retries: number = 5, | ||
| backoff: number = 1000, | ||
| ): Promise<T> { | ||
| let attempt = 0; | ||
| while (attempt < retries) { | ||
| try { | ||
| const result = await fn(); | ||
| if (Array.isArray(result) && result.length === 0) { | ||
| throw new Error("Empty array returned"); | ||
| } | ||
| return result; | ||
| } catch (error) { | ||
| attempt++; | ||
| if (attempt >= retries) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, backoff * attempt)); | ||
| } | ||
| } | ||
| throw new Error("Failed after maximum retries"); | ||
| } |
There was a problem hiding this comment.
If it is meant to be reusable, it should not assume array as output type. I would just add it as private function in above code.
There was a problem hiding this comment.
Also error messages are a little generic make them more specific
There was a problem hiding this comment.
just updated the error messages and used the terminal logger.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/manifest/dbtWorkspaceFolder.ts (3)
84-88: Consider adding JSDoc comments for clarity
Currently, the method signature lacks any in-code explanation or JSDoc. Adding a concise description of what the retry logic does, along with details about the parameters and return type, would improve maintainability.
102-109: Consider exponential backoff for improved resilience
The current linear backoff (backoff * attempt) might be sufficient. However, if you’re dealing with transient or long-lasting failures, exponential backoff could help avoid retrying too often in quick succession.
121-131: Consider configurable defaults for retry count and backoff
Using magic numbers (5 retries and 1000 ms backoff) might be acceptable in the short term, but they could be extracted into constants or reading from user settings for flexibility and clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manifest/dbtWorkspaceFolder.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/manifest/dbtWorkspaceFolder.ts (1)
src/manifest/dbtProject.ts (1) (1)
DBTProject(80-1743)
🔇 Additional comments (2)
src/manifest/dbtWorkspaceFolder.ts (2)
90-101: Verify handling of empty array as an error condition
Throwing an error when the result is an empty array enforces retries, but make sure this behavior aligns with all call sites—some consumers might expect an empty array to be a valid result.
110-116: Clear final debug message
This helpful debug message indicates that the maximum retry limit has been reached. This is a good addition for troubleshooting and logging.
Co-authored-by: Michiel De Smet <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/manifest/dbtWorkspaceFolder.ts (5)
84-116: Add generic typing for array results in retryWithBackoff methodThe implementation of
retryWithBackofflooks good and addresses the issue mentioned in the PR objectives. However, the function checks if the result is an array with length 0, but there's no generic type constraint to indicate that T could be an array.Consider adding a specialized implementation for array results:
- private async retryWithBackoff<T>( + private async retryWithBackoff<T extends any[]>( fn: () => Thenable<T>, retries: number = 5, backoff: number = 1000, ): Promise<T> { let attempt = 0; while (attempt < retries) { try { const result = await fn(); - if (Array.isArray(result) && result.length === 0) { + if (result.length === 0) { this.dbtTerminal.debug( "discoverProjects", "no projects found. retrying...", false, ); throw new Error("no projects found. retrying"); } return result;Or create an overload to handle both array and non-array cases:
private async retryWithBackoff<T extends any[]>(fn: () => Thenable<T>, retries?: number, backoff?: number): Promise<T>; private async retryWithBackoff<T>(fn: () => Thenable<T>, retries?: number, backoff?: number): Promise<T>; private async retryWithBackoff<T>(fn: () => Thenable<T>, retries: number = 5, backoff: number = 1000): Promise<T> { // Implementation remains the same }
94-99: Make debug logging and error messages more specificThe current debug message and error message for retrying are somewhat generic.
Consider making these messages more specific to provide better context for debugging:
this.dbtTerminal.debug( "discoverProjects", - "no projects found. retrying...", + `No projects found. Retrying (attempt ${attempt + 1}/${retries})...`, false, ); - throw new Error("no projects found. retrying"); + throw new Error("No projects found in workspace.findFiles result. Triggering retry.");
110-115: Improve final error message to include attempt countThe final error message doesn't include the number of attempts made.
Enhance the error message to provide more context:
this.dbtTerminal.debug( "discoverProjects", - "no projects found after maximum retries", + `No projects found after ${retries} retries`, false, ); - throw new Error("no projects found after maximum retries"); + throw new Error(`No projects found after ${retries} retries with exponential backoff`);
101-108: Consider adding explicit error loggingThe retry mechanism catches errors but doesn't log them, which might make debugging harder.
Consider adding error logging to provide more context when an attempt fails:
return result; } catch (error) { + this.dbtTerminal.debug( + "discoverProjects", + `Attempt ${attempt + 1} failed: ${error instanceof Error ? error.message : String(error)}`, + false, + ); attempt++; if (attempt >= retries) { throw error; }
102-107: Consider implementing exponential backoff rather than linearThe current implementation uses a linear backoff strategy (backoff * attempt). While this is better than no backoff, an exponential strategy might be more effective for handling transient failures.
Consider implementing a true exponential backoff:
attempt++; if (attempt >= retries) { throw error; } - await new Promise((resolve) => setTimeout(resolve, backoff * attempt)); + // Exponential backoff: 2^attempt * backoff (e.g., 1s, 2s, 4s, 8s, 16s) + const delay = Math.pow(2, attempt - 1) * backoff; + await new Promise((resolve) => setTimeout(resolve, delay));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manifest/dbtWorkspaceFolder.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/manifest/dbtWorkspaceFolder.ts (1)
src/manifest/dbtProject.ts (1) (1)
DBTProject(80-1743)
🔇 Additional comments (2)
src/manifest/dbtWorkspaceFolder.ts (2)
84-87: Follow function parameter convention in retryWithBackoff methodThere's an existing suggestion from a previous review to change the method signature.
private async retryWithBackoff<T>(
121-132: Good implementation of the retry mechanismThe changes to the
discoverProjectsmethod effectively utilize the newretryWithBackofffunction to handle potential issues withworkspace.findFiles. This should correctly address the known VSCode bug mentioned in the PR objectives.The explicit parameters (5 retries, 1000ms backoff) match the default values, which makes the code more self-documenting. This matches the PR objectives of implementing a retry mechanism for handling cases where findFiles returns empty results.
…IfNothingIsReturned' into feat_retryFindFilesIfNothingIsReturned
looks like this repo has strange permissions on the code files. all the code files are marked as executable. Just marked the files as executable again. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/manifest/dbtWorkspaceFolder.ts (1)
121-132: Effective application of retry logic for workspace.findFilesThis is a good application of the retry mechanism to handle the known VSCode issue. The implementation correctly wraps the original call to ensure files are properly discovered.
Consider extracting the retry parameters (5, 1000) to class constants or configuration settings for better maintainability:
+ private static readonly MAX_FIND_FILES_RETRIES = 5; + private static readonly FIND_FILES_BACKOFF_MS = 1000; // Then in the method: const dbtProjectFiles = await this.retryWithBackoff( () => workspace.findFiles( new RelativePattern( this.workspaceFolder, `**/${DBTProject.DBT_PROJECT_FILE}`, ), new RelativePattern(this.workspaceFolder, excludePattern), ), - 5, - 1000, + DBTWorkspaceFolder.MAX_FIND_FILES_RETRIES, + DBTWorkspaceFolder.FIND_FILES_BACKOFF_MS, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/manifest/dbtWorkspaceFolder.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/manifest/dbtWorkspaceFolder.ts (1)
src/manifest/dbtProject.ts (1) (1)
DBTProject(80-1743)
🔇 Additional comments (1)
src/manifest/dbtWorkspaceFolder.ts (1)
84-116: Good implementation of retry mechanism with backoffThe
retryWithBackoffmethod is a well-implemented solution to address the issue withworkspace.findFilessometimes returning empty results. The approach of retrying with increasing delays is appropriate for handling transient failures.A couple of minor suggestions:
- Consider making the retry count and backoff duration configurable through extension settings rather than hardcoded values
- Line 110-115 contain unreachable code since the error is already thrown at line 105 when maximum retries are reached
- this.dbtTerminal.debug( - "discoverProjects", - "no projects found after maximum retries", - false, - ); - throw new Error("no projects found after maximum retries");
AFAIK are permissions not reflected in git, so not sure about this. Maybe you can squash your commits and rebase it on master and force push it? |
it does. I reverted the permissions in this PR and it has only the needed changes. |
Overview
this allows to skip the project discovery as it often fails to discover projects
Problem
the extension often didn't discover our projects and returned an empty list. this is basically due to a bug in vscode workspace.findFiles (microsoft/vscode#179203)
fixes #1437
Solution
this forces to retry the workspace.findFiles request for up to 5 times with a backoff time increase of 1 second if no files are found.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Add retry mechanism with backoff for project discovery in
dbtWorkspaceFolder.tsto handle VSCodeworkspace.findFilesbug.discoverProjects()indbtWorkspaceFolder.tsusingretryWithBackoff().workspace.findFilesup to 5 times with increasing backoff if no files are found.retryWithBackoff()inutils.tsto handle retries with backoff logic.dbtWorkspaceFolder.tsandutils.ts.This description was created by
for 0dfb9fd. It will automatically update as commits are pushed.
Summary by CodeRabbit