-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add file watching capabilities with inotify support #324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fb3531a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…ing or encoding Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Documents the new sandbox.watch() method that enables real-time filesystem monitoring using Linux inotify. Includes: - API reference for watch() method in files.mdx - Comprehensive how-to guide with practical examples - Cross-references between related documentation This documentation covers: - Basic file watching with callbacks - Filtering by file type with glob patterns - Event type filtering (create, modify, delete, rename) - Cancellation with AbortController - Hot reload and log monitoring examples - Best practices and troubleshooting Synced from cloudflare/sandbox-sdk#324
commit: |
🐳 Docker Images PublishedDefault: FROM cloudflare/sandbox:0.0.0-pr-324-8ce0c94With Python: FROM cloudflare/sandbox:0.0.0-pr-324-8ce0c94-pythonWith OpenCode: FROM cloudflare/sandbox:0.0.0-pr-324-8ce0c94-opencodeVersion: Use the 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-324-8ce0c94 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 21010367084 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-324-8ce0c94 cat /container-server/sandbox > sandbox && chmod +x sandbox |
…ses. Update tests to validate new event parsing logic and ensure proper handling of inotifywait output.
…efault and custom excludes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude Code Review
This PR adds real-time file watching using Linux's inotify. The implementation follows the three-layer architecture correctly with good separation of concerns. However, there are critical error handling issues that need to be addressed before merging.
Critical Issues
1. Silent process cleanup failures
watch-service.ts:383-387 - The finally block swallows all process.kill() errors:
finally {
try {
proc.kill();
} catch {
// Process may already be dead
}
self.activeWatches.delete(watchId);
}This hides permission errors, system failures, and other unexpected issues. Users won't know if cleanup failed, potentially leading to zombie processes. Log unexpected errors (not just ESRCH).
2. FileWatch.stop() fails silently
sandbox.ts:311-330 - Multiple silent failures:
- Lines 313, 330:
loopPromise.catch(() => {})swallows all stream errors - Lines 321-327: Server-side stop failures only logged, never surfaced to user
Users call await watcher.stop() and get success even when server-side cleanup failed. This leaves watches active on the server.
3. Establishment promise can hang forever
sandbox.ts:168-182 - If the watch is cancelled (via AbortSignal) during establishment before receiving the "watching" event, the establishment promise never resolves or rejects. The AbortSignal handler (lines 1420-1426) doesn't call establishedReject(), causing FileWatch.create() to hang indefinitely.
4. Weak type guard allows undefined access
sandbox.ts:228-233 - isFileWatchSSEEvent only checks that type is a string, but doesn't validate required properties exist:
if (typeof obj.type !== 'string') return false;
return ['watching', 'event', 'error', 'stopped'].includes(obj.type);This allows accessing event.watchId (line 255) or event.path (line 265) when they might be undefined, causing runtime errors.
Important Issues
5. Comments reference historical context
watch-service.ts:374-377 - Per CLAUDE.md, avoid phrases like "to avoid", "to prevent", "expected case":
// Expected case: process already exited (no longer running)
// Unexpected: permission errors, system issuesDescribe current behavior, not why the code exists.
6. No validation of watchId parameter
watch-client.ts:50-61 - stopWatch() sends user input directly to API without checking it's a non-empty string.
7. Malformed events silently dropped
sandbox.ts:240-244, 246-248 - Malformed JSON and invalid event structures are silently ignored with no logging. Users won't know events are being lost.
Test Coverage Gaps (High Priority)
8. Missing: Concurrent watch operations
No tests for:
- Multiple watches on same directory
stopAllWatches()called while events streaming- Rapid start/stop cycles
The stopAllWatches() implementation could have race conditions with concurrent modifications to the activeWatches map.
9. Missing: inotifywait crash handling
What happens when inotifywait exits unexpectedly (permission denied, kernel limit reached)? The code monitors stderr but no test verifies error events are sent before stream closes.
10. Missing: Stream cancellation during active events
No test verifies calling reader.cancel() while events are streaming properly kills the process and cleans up the watch from activeWatches.
Positive Observations
- Follows three-layer architecture correctly
- Strong security focus: comprehensive ReDoS prevention tests (watch-service.test.ts:84-160)
- Proper logger injection via constructor
- Good use of ErrorCode enum
- FileWatch state machine is well-designed
- Comprehensive E2E tests for happy path scenarios
Verdict
The architecture and happy path implementation are solid, but the error handling needs significant work. The silent failures will make debugging very difficult for users and could lead to resource leaks.
Recommendations before merge:
- Fix the 4 critical issues (especially #2 and #3 which affect user experience directly)
- Add logging to error paths (issues #1, #7)
- Add tests for concurrent operations and error scenarios (issues #8, #9, #10)
Let me know if you'd like me to help with any of these fixes!
Resolve conflicts in container.ts and sandbox.ts: - Keep WatchService import in container.ts - Keep watch methods and merge updated exposePort with token parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is an exceptionally well-architected file watching feature that demonstrates excellent understanding of the codebase patterns and conventions. The implementation seamlessly integrates with existing infrastructure and follows all established architectural patterns.
Architectural Strengths:
- Perfect adherence to the modular client pattern (
WatchClientextendsBaseHttpClient) - Consistent with service layer patterns (
WatchServiceusesServiceResult<T>) - Proper integration with existing SSE infrastructure
- State machine pattern in
FileWatchis clean and well-designed - Resource management follows established patterns (similar to ProcessManager)
Implementation Quality:
- Error handling is comprehensive and follows SDK conventions
- Stream management uses proper cleanup patterns with AbortSignal
- Security considerations for glob matching with proper regex escaping
- Comprehensive test coverage including edge cases and security scenarios
Documentation Update Needed:
Since this adds a new major API surface (sandbox.watch()), the Cloudflare docs at /sandbox/api/ will need a new "File Watching" section to document:
- The
watch()method andWatchHandleinterface - Event types and filtering options
- Usage examples and best practices
- Performance considerations and limitations
Minor Suggestions:
- Container Cleanup: Consider adding watch cleanup to container shutdown sequence (similar to ProcessManager cleanup)
- Resource Limits: Consider adding limits on concurrent watches per sandbox to prevent resource exhaustion
This implementation is production-ready and demonstrates sophisticated understanding of the platform architecture. Looks good to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is a well-architected file watching implementation that adds real-time filesystem monitoring using Linux's native inotify. The feature follows established codebase patterns closely and includes comprehensive testing.
Strengths:
- Proper SSE streaming integration following existing patterns
- Clean state machine design in
FileWatchclass - Comprehensive unit tests with security-focused glob matching tests
- Full E2E workflow tests covering creation, modification, deletion, filtering
- Proper resource cleanup and AbortSignal handling
- Transport-agnostic design (works with HTTP and WebSocket)
- Security-conscious glob pattern implementation preventing ReDoS attacks
Architecture fits well:
- WatchService follows ProcessService patterns for lifecycle management
- WatchClient extends BaseHttpClient like other clients
- Error handling uses ServiceResult pattern with proper error codes
- Container integration via dependency injection follows established patterns
- WebSocket adapter SSE parsing improvement is a thoughtful enhancement
Suggestions:
- Changeset description: Should be more user-focused per project guidelines. Consider describing the user benefit and how to use the feature rather than technical implementation details.
- E2E test cleanup: Some debug logging (like line 256 in file-watch-workflow.test.ts) should be removed before merge.
- Documentation: The API examples in the PR description are helpful - consider if any additional docs are needed.
Overall: The implementation demonstrates strong understanding of the codebase architecture and maintains consistency with existing patterns while adding valuable functionality. The security considerations in glob matching and proper resource management are especially well done.
Looks good to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is a comprehensive implementation of file watching capabilities using Linux's native inotify. The feature adds real-time filesystem monitoring with proper event streaming, filtering, and lifecycle management.
Architecture Assessment
Excellent architectural decisions:
- Clean separation between SDK (public API), container service (inotifywait integration), and file watch state machine
- Proper use of existing client pattern with specialized
WatchClient - Well-designed SSE streaming with structured event types
- State machine approach in
FileWatchprovides clear lifecycle management
Code Quality
Strong implementation:
- Comprehensive error handling and graceful degradation
- Proper resource cleanup (process killing, stream cancellation)
- Good use of TypeScript for type safety
- Follows existing patterns for logging, error codes, and client architecture
Testing Coverage
Thorough testing:
- Unit tests cover critical components (glob matching, event parsing, inotify args)
- E2E tests validate real-world scenarios including edge cases
- Good use of helper functions for test readability
- Tests cover both success and error paths
Security Considerations
Security handled appropriately:
- Glob pattern escaping prevents ReDoS and injection attacks
- Input validation follows existing patterns (path length, null byte checks)
- inotify exclude patterns use proper regex escaping
- Follows container isolation security model
Notable Strengths
- Robust SSE parsing - The WebSocket adapter improvements handle partial events across chunk boundaries correctly
- Proper concurrency handling - Aligns with project's concurrency model (sessions, mutex usage)
- Comprehensive filtering - Both include/exclude patterns with security-conscious glob implementation
- Clean API design - High-level
watch()method with lower-levelwatchStream()for flexibility
Minor Observations
- The extensive testing and documentation show strong engineering practices
- Good balance between features and maintainability
- Changeset properly formatted for end users
Verdict: Looks good to merge
This is a well-implemented feature that integrates cleanly with the existing architecture. The code quality is high, security considerations are addressed, and test coverage is comprehensive.
Streaming responses (like file watch events) were blocking the WebSocket message handler because handleStreamingResponse was awaited. This prevented other messages from being processed while a stream was active. Run streaming response handlers in the background with error logging, allowing the message handler to return immediately and process subsequent messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
File watching with inotify - solid implementation with some concerns
This PR adds real-time file watching capabilities using Linux's native inotify. The implementation follows the SDK's established patterns well and integrates cleanly with the existing SSE streaming architecture.
Key strengths:
- Uses native inotify for efficient filesystem monitoring (vs polling)
- Follows established SSE streaming patterns consistently
- Proper state machine design in FileWatch class
- Good integration with WebSocket adapter's SSE parsing
- Comprehensive test coverage including edge cases
- Security-conscious glob pattern handling with ReDoS prevention
Areas that need attention:
1. Changeset description (.changeset/empty-poets-serve.md)
Per AGENTS.md, changeset descriptions should be user-focused, not technical. Current description is too implementation-focused. Should explain the benefit to users and how to use the feature.
2. Hard timeouts in E2E tests (tests/e2e/file-watch-workflow.test.ts)
The E2E tests use fixed delays (1.5s, 3s) that could cause flakiness in CI environments. Consider using event-based synchronization instead of arbitrary delays.
3. WebSocket adapter changes (packages/sandbox-container/src/handlers/ws-adapter.ts)
The changes to parseSSEEvents() add partial event state tracking across chunks. While good for file watching, this affects ALL SSE streams. Verify this doesn't break existing streaming operations.
4. Container cleanup (packages/sandbox-container/src/services/watch-service.ts)
The WatchService spawns inotifywait processes but there's no explicit cleanup hook in the container lifecycle. Consider adding cleanup when the container shuts down to prevent zombie processes.
Architecture assessment: The implementation correctly uses the three-layer architecture, follows established patterns for SSE streaming, and integrates well with the WebSocket transport layer. The use of inotify is appropriate for the container environment.
Verdict: Good implementation with comprehensive testing. Address the changeset description and E2E test timing before merging.
Add detailed logging to trace streaming response handling and track active streams to prevent potential garbage collection of Response objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This PR adds comprehensive file watching capabilities using Linux's native inotify system. The implementation follows the project's architectural patterns and includes thorough testing. Here are my findings:
Strengths
Solid Architecture: The implementation properly follows the three-layer architecture with WatchService in the container, WatchClient in the SDK, and proper error propagation. The state machine pattern in FileWatch is clean and handles edge cases well.
Comprehensive Testing: Excellent test coverage including unit tests for security-critical glob matching (ReDoS prevention), integration tests for the full workflow, and proper WebSocket adapter handling.
Security Considerations: The glob pattern implementation correctly escapes regex metacharacters and prevents ReDoS attacks. Default excludes for sensitive directories (.git, node_modules) are appropriate.
Issues to Address
-
Error Handling Consistency: The WebSocket adapter changes introduce inconsistency - streaming errors are now caught and logged but don't propagate to clients properly.
-
Resource Management: The FileWatch class has potential resource leaks if stop() isn't called properly. Consider adding cleanup mechanisms.
-
Log Levels: Some error logging in WatchService should be debug level rather than warn, since process termination is expected when stopping watches.
-
Changeset: The changeset description could be more user-focused ("Enable real-time file system monitoring" vs technical description).
Minor Issues
- Type assertions in tests could use proper type guards
- Some debug logging verbosity could be reduced
- inotifywait format string needs better documentation
Verdict
Strong implementation with good architecture and testing. Address the error handling and resource management issues before merge. The file watching feature will be a valuable addition to the SDK.
The WebSocket message handler needs to capture the Response body reader before any await points. When getReader() was called inside the async handleStreamingResponse method, Bun's WebSocket handler would return before the reader was acquired, potentially invalidating the Response body stream. By getting the reader synchronously in handleRequest before the promise starts executing, we ensure the stream remains valid throughout the async streaming loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is a well-architected implementation of file watching using Linux inotify. The feature follows the established patterns in the codebase and includes comprehensive test coverage. The WebSocket streaming improvements are a valuable bonus.
Strengths:
- Proper layered architecture following the 3-layer SDK pattern
- Sound concurrency handling with mutex serialization in SessionManager
- Comprehensive security with glob pattern escaping and ReDoS prevention
- Critical WebSocket streaming fixes that prevent garbage collection issues
- Extensive E2E test coverage with realistic scenarios
- Good error handling and resource cleanup
Architecture compliance: The implementation properly follows the SDK's modular client pattern, integrates cleanly with the dependency injection system, and uses the established SSE streaming approach.
Looks good to merge.
The watching SSE event was sent immediately when the stream started, before inotifywait finished setting up watches. This caused flaky tests because file operations could occur before the watch was truly ready. Now we read stderr and wait for the 'Watches established' message from inotifywait before sending the watching event to clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is a well-designed implementation of file watching capabilities that integrates cleanly with the existing architecture. The use of native inotify via inotifywait is the right choice for performance and reliability. The state machine design in FileWatch and enhanced SSE parsing in the WebSocket adapter show good engineering practices.
Main issue to address: The WatchService cleanup is not integrated into the server shutdown process in packages/sandbox-container/src/server.ts. This could leave inotifywait processes running after container restart. Add this to the cleanup function around line 139:
const watchService = app.container.get('watchService');
const stoppedCount = await watchService.stopAllWatches();
logger.info('Stopped file watches', { count: stoppedCount });Minor observations:
- Comprehensive test coverage including security edge cases
- Good error handling and resource management patterns
- WebSocket adapter improvements fix a critical timing issue with stream readers
The changeset is appropriately user-focused. Overall, this adds valuable functionality following project conventions.
If inotifywait fails to output 'Watches established' within 10 seconds, the function will return and allow the stream to proceed. This prevents indefinite hangs if inotifywait behaves unexpectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This is a well-implemented file watching feature that adds real-time filesystem monitoring using Linux's native inotify. The implementation follows the repository's three-layer architecture correctly and includes comprehensive testing.
Strengths:
- Clean API design with high-level
watch()and low-levelwatchStream()methods - Proper state machine pattern in
FileWatchclass for lifecycle management - Comprehensive security considerations (ReDoS prevention, glob escaping)
- Good separation of concerns across the three layers
- Thorough E2E test coverage including edge cases and error conditions
- Appropriate WebSocket streaming fixes that were needed for this feature
Architecture adherence:
- Correctly follows the SDK → Durable Object → Container HTTP API → inotifywait pattern
- WatchService properly uses dependency injection in the container layer
- Error handling flows correctly through all layers with proper status mapping
- SSE streaming implementation is consistent with existing patterns
Testing:
- E2E tests properly isolate sessions and use realistic timing delays
- Unit tests cover private method behavior through type assertions (appropriate for this testing scope)
- WebSocket adapter test fixes ensure streaming works correctly
This feature significantly enhances the SDK's capabilities for real-time development workflows. The implementation quality is high and consistent with the existing codebase standards.
Looks good to merge
For WebSocket streaming, errors were deferred until stream consumption. This caused issues where watchStream() would return successfully even when the server returned an error response. Now requestStream() waits for the first message before returning: - If it's a stream chunk, return the stream (success case) - If it's an error response, throw immediately (error case) This makes WebSocket streaming behavior match HTTP streaming, where errors are thrown immediately rather than deferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenCode Review
This PR implements comprehensive file watching capabilities using native inotify support - well-designed feature that follows the SDK's architecture patterns correctly.
Architecture alignment: The three-layer design is properly maintained with WatchClient/WatchHandler/WatchService, clean separation between SDK and container runtime, and proper error handling flow. The client follows the established modular pattern and integrates cleanly into SandboxClient.
Implementation highlights:
- Smart use of Server-Sent Events for real-time file notifications
- Proper lifecycle management with FileWatch state machine (establishing → active → stopped)
- Good concurrency handling - watch streams run independently without blocking other operations
- Comprehensive filtering with include/exclude patterns and secure glob matching that prevents ReDoS
- WebSocket transport enhancements handle streaming edge cases well
Container integration: Adding inotify-tools to the Docker image is appropriate - small addition that enables efficient native filesystem monitoring. The WatchService properly manages inotifywait processes and handles stderr/stdout parsing robustly.
Testing: Excellent E2E test coverage including edge cases like non-existent paths, concurrent operations, and cleanup scenarios. Tests properly handle timing considerations for file system events.
Documentation impact: This adds a significant new capability that needs documentation. The API examples in code comments are helpful but users will need:
- File watching guide covering use cases, patterns, and limitations
- Performance considerations (inotify is efficient but has kernel limits)
- Lifecycle behavior (watches stop when sandbox sleeps, as expected from the architecture docs)
Minor observations:
- Error codes WATCH_NOT_FOUND/WATCH_START_ERROR properly mapped to HTTP statuses
- Good security in glob matching - treats regex metacharacters as literals
- Changeset correctly categorizes as patch-level change
This adds real value for developers building file-monitoring workflows. The implementation is solid and follows project conventions well.
Verdict: Looks good to merge after documentation is added.
Adds real-time file watching using Linux's native inotify via inotifywait.
API
Implementation