feat: Implement throttling and retry logic in secure-fs module #272
Conversation
- Added concurrency limiting using p-limit to prevent ENFILE/EMFILE errors. - Introduced retry logic with exponential backoff for transient file descriptor errors. - Enhanced secure-fs with new functions for configuring throttling and monitoring active/pending operations. - Added unit tests for throttling and retry logic to ensure reliability.
Shironex
added
the
Do Not Merge
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR introduces file descriptor throttling and retry logic to prevent ENFILE/EMFILE errors during concurrent file system operations. It wraps all file-system operations with a configurable concurrency limiter and exponential backoff retry mechanism, then re-exports the throttling configuration APIs from the server layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant Handler as Route Handler
participant SecureFS as Secure FS Wrapper
participant Limiter as p-limit Limiter
participant FSPromises as fs.promises
participant RetryLogic as Retry & Backoff
Client->>Handler: File system operation (e.g., readFile)
Handler->>SecureFS: readFile(path)
SecureFS->>SecureFS: executeWithRetry()
alt Within concurrency limit
SecureFS->>Limiter: Acquire slot
Limiter-->>SecureFS: Slot available
SecureFS->>FSPromises: Execute fs operation
FSPromises-->>SecureFS: Success ✓
SecureFS-->>Handler: Return result
else File descriptor exhausted (ENFILE/EMFILE)
SecureFS->>Limiter: Acquire slot
Limiter-->>SecureFS: Slot available
SecureFS->>FSPromises: Execute fs operation
FSPromises-->>SecureFS: ENFILE/EMFILE error
SecureFS->>RetryLogic: isFileDescriptorError?
RetryLogic-->>SecureFS: true
SecureFS->>RetryLogic: calculateDelay(attempt) with jitter
RetryLogic-->>SecureFS: Exponential backoff delay
SecureFS->>SecureFS: sleep(delay)
rect rgba(255, 193, 7, 0.1)
Note over SecureFS: Retry attempt
end
SecureFS->>FSPromises: Execute fs operation (retry)
FSPromises-->>SecureFS: Success or max retries exceeded
SecureFS-->>Handler: Return result or throw
end
Handler-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @Shironex, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the "secure-fs" module by integrating robust throttling and retry mechanisms. The primary goal is to prevent system instability caused by file descriptor exhaustion (ENFILE/EMFILE errors) and to improve the overall reliability of file system interactions. By limiting concurrent operations and automatically retrying transient failures with an exponential backoff strategy, the module becomes more resilient under heavy load, ensuring smoother and more consistent performance. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable throttling and retry logic to the secure-fs module, which will significantly improve its robustness against file descriptor exhaustion errors. The implementation is clean and the addition of configuration and monitoring functions is a great touch.
My review includes a few suggestions to enhance this new functionality:
- A
highseverity suggestion to handle a potential issue inconfigureThrottlingwhere pending operations could be lost. - A
mediumseverity suggestion to improve logging within the retry logic. - Two
mediumseverity suggestions to strengthen the unit tests for the new concurrency and retry features.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
libs/platform/src/secure-fs.test.ts (2)
64-90: Test does not verify actual concurrency limiting behavior.The test name suggests it verifies concurrency limiting, but the local variables (
activeConcurrency,maxObservedConcurrency,delays) and the loop are dead code that don't interact withsecureFsoperations. The test only asserts the config value was set.Consider either renaming the test to reflect what it actually tests, or implementing a proper concurrency test by mocking
fsoperations and observinggetActiveOperations().
93-106: Tests don't verify actual error handling behavior.These tests are named for ENFILE/EMFILE error identification but only check that
maxRetriesconfig exists. To properly test retry logic, mock anfsmethod to throw file descriptor errors and verify the wrapper retries before succeeding or failing.libs/platform/src/secure-fs.ts (2)
122-124: Consider using a configurable logger instead of console.warn.Using
console.warndirectly in a library pollutes the consuming application's console and is not configurable. Consider accepting a logger viaconfigureThrottlingor using the project's existingcreateLoggerutility (as seen inlibs/git-utils/src/diff.ts).
47-53: Recreating limiter while operations are in-flight can cause issues.When
configureThrottlingis called with a newmaxConcurrencywhile operations are pending on the old limiter, those pending promises may never settle, leading to memory leaks and unexpected behavior.🔎 Proposed fix: Guard against in-flight operations
export function configureThrottling(newConfig: Partial<ThrottleConfig>): void { + const newConcurrency = newConfig.maxConcurrency; + + if (newConcurrency !== undefined && newConcurrency !== config.maxConcurrency) { + if (fsLimit.activeCount > 0 || fsLimit.pendingCount > 0) { + throw new Error( + `[SecureFS] Cannot change maxConcurrency while operations are in flight. Active: ${fsLimit.activeCount}, Pending: ${fsLimit.pendingCount}` + ); + } + fsLimit = pLimit(newConcurrency); + } + config = { ...config, ...newConfig }; - // Recreate the limiter if concurrency changed - if (newConfig.maxConcurrency !== undefined) { - fsLimit = pLimit(config.maxConcurrency); - } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
apps/server/src/lib/secure-fs.tslibs/git-utils/src/diff.tslibs/platform/package.jsonlibs/platform/src/secure-fs.test.tslibs/platform/src/secure-fs.ts
🧰 Additional context used
🧬 Code graph analysis (2)
libs/platform/src/secure-fs.test.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-28)
libs/platform/src/secure-fs.ts (2)
.github/scripts/upload-to-r2.js (2)
attempt(65-65)delay(179-179)apps/server/src/services/auto-mode-service.ts (1)
sleep(2528-2551)
🔇 Additional comments (7)
libs/platform/src/secure-fs.test.ts (2)
9-17: Good test setup with config reset.The
beforeEachhook properly resets the throttling configuration to known defaults before each test, ensuring test isolation.
19-50: LGTM: Configuration tests are well-structured.These tests properly verify:
- Partial config updates work correctly
- Existing values are preserved when updating partial config
- Config structure contains all expected properties
- Default values match expected defaults
apps/server/src/lib/secure-fs.ts (1)
23-27: LGTM: Clean API surface extension.The new throttling configuration and monitoring APIs are properly re-exported, maintaining backward compatibility while exposing the new functionality to server code.
libs/platform/src/secure-fs.ts (3)
110-135: Retry logic implementation is sound.The
executeWithRetryfunction correctly:
- Wraps operations with the p-limit throttle
- Implements exponential backoff with jitter for ENFILE/EMFILE errors
- Respects
maxRetriesconfiguration (loop runsmaxRetries + 1times total)- Re-throws non-retryable errors immediately
94-98: Backoff calculation starts at baseDelay on first retry.Note that on the first retry (
attempt=0), the delay isbaseDelay * 2^0 + jitter = baseDelay + jitter. This is reasonable behavior, but worth documenting if the intent was for the first retry to wait exactlybaseDelay.
140-143: All fs wrappers correctly integrate throttling and retry logic.The wrappers properly:
- Validate paths before execution
- Route all operations through
executeWithRetry- Include meaningful operation names for debugging
Also applies to: 148-159, 164-174, 179-185, 198-209, 214-217, 222-228, 233-236, 241-248, 253-263, 268-275, 281-284
libs/platform/package.json (1)
21-22: p-limit version and ESM compatibility verified.The addition of
p-limitat version^6.2.0is appropriate for implementing concurrency limiting. Verification confirms p-limit v6.2.0 is a valid release (December 19, 2024) published as ESM-only, which aligns correctly with the"type": "module"configuration in package.json.
- Enhanced the configureThrottling function to prevent changes to maxConcurrency while operations are in flight. - Added comprehensive unit tests for secure-fs throttling and retry logic, ensuring correct behavior and configuration. - Removed outdated secure-fs test file and replaced it with a new, updated version to improve test coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libs/platform/src/secure-fs.ts (1)
129-131: Console.warn issue previously flagged.Direct
console.warnusage in a library remains present. As noted in the previous review, this pollutes the application console and isn't configurable. Consider adding a logger option toconfigureThrottlingto allow consumers to handle these warnings.Based on past review comments, this issue was already raised but remains unaddressed.
🧹 Nitpick comments (2)
libs/platform/tests/secure-fs.test.ts (2)
74-82: Consider clarifying test name or improving coverage.The test name "should throw when changing maxConcurrency while operations are in flight" is misleading—it only verifies the success case (no operations in flight). The actual throwing behavior is never tested because simulating in-flight operations would require async operations and coordination.
Consider either:
- Renaming to "should not throw when changing maxConcurrency with no operations in flight"
- Adding a more robust integration test that actually triggers the error condition
💡 Potential integration test approach
it('should throw when changing maxConcurrency while operations are in flight', async () => { // Create a slow operation that keeps the limiter busy const slowOp = secureFs.readFile('/some/large/file'); // Wait briefly to ensure operation is active await new Promise(resolve => setTimeout(resolve, 10)); // Should throw because operation is in flight expect(() => secureFs.configureThrottling({ maxConcurrency: 50 })) .toThrow(/Cannot change maxConcurrency while operations are in flight/); await slowOp; // cleanup });Note: This requires a real file operation, making it more of an integration test.
86-136: LGTM! Retry configuration properly tested.The tests verify retry configuration APIs work correctly. The inline comments (lines 112-114) appropriately acknowledge that actual retry behavior requires integration testing with real file descriptor errors, which is challenging to mock in ESM.
Optional: Consider adding integration tests that trigger actual ENFILE/EMFILE errors in a controlled environment, perhaps by:
- Using resource limits (
ulimit -non Unix) in a separate test suite- Creating a large number of concurrent file operations to exhaust file descriptors
- Or documenting manual testing procedures for file descriptor exhaustion scenarios
These would validate the complete retry flow end-to-end.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/git-utils/src/diff.tslibs/platform/src/secure-fs.tslibs/platform/tests/node-finder.test.tslibs/platform/tests/secure-fs.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/git-utils/src/diff.ts
🧰 Additional context used
🧬 Code graph analysis (2)
libs/platform/src/secure-fs.ts (2)
.github/scripts/upload-to-r2.js (2)
attempt(65-65)delay(179-179)apps/server/src/services/auto-mode-service.ts (1)
sleep(2528-2551)
libs/platform/tests/secure-fs.test.ts (1)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (9)
libs/platform/tests/node-finder.test.ts (1)
133-133: LGTM! Excellent cross-platform improvement.Replacing hard-coded colons with
path.delimitermakes these tests platform-agnostic, correctly handling Windows (;) and Unix (:) path separators.Also applies to: 147-147, 156-156
libs/platform/src/secure-fs.ts (6)
19-41: LGTM! Well-structured throttling configuration.The
ThrottleConfiginterface and defaults are well-designed. The default values (maxConcurrency: 100, maxRetries: 3, baseDelay: 100ms, maxDelay: 5s) strike a good balance between preventing file descriptor exhaustion and maintaining reasonable performance under load.
47-60: Previous concern addressed.The implementation now safely prevents changing
maxConcurrencywhile operations are in flight by checkingactiveCountandpendingCountbefore recreating the limiter. This addresses the concern raised in the previous review about potential memory leaks and lost promises.
65-81: LGTM! Clean API surface.The getter functions properly expose throttling state.
getThrottlingConfig()correctly returns a shallow copy to prevent external mutation of internal configuration.
86-112: LGTM! Solid retry helpers.The helper functions are well-implemented:
isFileDescriptorErrorcorrectly identifies ENFILE/EMFILE errorscalculateDelayuses exponential backoff with jitter to prevent thundering herdsleepis a clean Promise-based delay
147-291: LGTM! Comprehensive fs operation wrapping.All file system operations are correctly wrapped with
executeWithRetry, ensuring consistent throttling and retry behavior. Path validation occurs before throttling (fail-fast for security violations), and operation names include file paths for effective debugging.
297-307: LGTM! Clear path construction utilities.The
joinPathandresolvePathwrappers are well-documented. The explicit note that they don't validate paths makes the security boundary clear: these are for construction, validation happens when the path is used in an fs operation.libs/platform/tests/secure-fs.test.ts (2)
8-50: LGTM! Solid configuration test coverage.The tests thoroughly verify the configuration API behavior, including:
- Updating individual configuration values
- Preserving unspecified fields during partial updates
- Verifying default values
52-62: LGTM! Basic monitoring coverage.The tests verify that monitoring APIs exist and return expected values when idle.
| async function executeWithRetry<T>(operation: () => Promise<T>, operationName: string): Promise<T> { | ||
| return fsLimit(async () => { | ||
| let lastError: unknown; | ||
|
|
||
| for (let attempt = 0; attempt <= config.maxRetries; attempt++) { | ||
| try { | ||
| return await operation(); | ||
| } catch (error) { | ||
| lastError = error; | ||
|
|
||
| if (isFileDescriptorError(error) && attempt < config.maxRetries) { | ||
| const delay = calculateDelay(attempt); | ||
| console.warn( | ||
| `[SecureFS] ${operationName}: File descriptor error (attempt ${attempt + 1}/${config.maxRetries + 1}), retrying in ${delay}ms` | ||
| ); | ||
| await sleep(delay); | ||
| continue; | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| throw lastError; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Minor: Unreachable code at line 140.
The throw lastError statement on line 140 is unreachable. The loop will always either return successfully (line 123) or throw an error (line 136) before completing all iterations. Every error path within the loop leads to either a continue or throw, so the loop cannot exit normally.
While this doesn't affect behavior, it's a code smell that could confuse maintainers.
🔎 Proposed fix
for (let attempt = 0; attempt <= config.maxRetries; attempt++) {
try {
return await operation();
} catch (error) {
- lastError = error;
-
if (isFileDescriptorError(error) && attempt < config.maxRetries) {
const delay = calculateDelay(attempt);
console.warn(
`[SecureFS] ${operationName}: File descriptor error (attempt ${attempt + 1}/${config.maxRetries + 1}), retrying in ${delay}ms`
);
await sleep(delay);
continue;
}
throw error;
}
}
-
- throw lastError;
});
}🤖 Prompt for AI Agents
In libs/platform/src/secure-fs.ts around lines 117 to 142, the final "throw
lastError" is unreachable because every loop iteration either returns,
continues, or throws; remove the unreachable throw statement (or alternatively
replace it with a single explicit throw new Error(`${operationName}: exhausted
retries`) if you want an explicit post-loop failure message) so the function no
longer contains dead code and the control flow is clear.
Resolve
Summary by CodeRabbit
New Features
configureThrottling(),getThrottlingConfig(),getPendingOperations(),getActiveOperations().joinPath()andresolvePath().Chores
✏️ Tip: You can customize this high-level summary in your review settings.