-
Notifications
You must be signed in to change notification settings - Fork 401
Description
Follow-up items from PR #3150: Native File Locking for Multi-Threaded CLI
This issue tracks follow-up work identified during the review of #3150.
We can probably live without addressing many of these items, but it would at least be good to explicitly document our compromises.
The following is AI-summarized. I have not fully reviewed it but will do so as part of this issue.
Code Cleanup
- Remove unused load balancer code (
packages/playground/cli/src/load-balancer.ts)- The load balancer is no longer needed with the new object pool proxy approach
- File already has TODO: "Is there any reason we should not delete this unused module and its tests?"
- Consider also evaluating process manager and instance manager for removal/consolidation (@adamziel suggested this in review)
Windows File Locking Improvements
-
Implement fcntl()-style lock upgrading/downgrading (
file-lock-manager-for-windows.ts:248-250)- Windows does not natively support these operations like POSIX does
- TODOs at lines 248-250: lock upgrading, relocking of preexisting locks, merging locked ranges
-
Implement partial range unlocking (
file-lock-manager-for-windows.ts:329)- fcntl() allows unlocking a subset of a locked range
- Windows requires a different approach - potentially breaking and re-acquiring locks
- Risk of race conditions needs careful handling
-
Consider bigint for Win API offsets (
file-lock-manager-for-windows.ts:76)- TODO: "Consider converting the exposed Win API to just use bigint for offset and length"
POSIX File Locking Improvements
- 64-bit int (bigint) support for lock offsets (
file-lock-manager-for-posix.ts:96)- Current API uses
Number()which loses precision for large file offsets - JS only supports 64-bit ints as bigint
- Needed for complete address space support, but may not be required in practice
- Current API uses
Architecture Improvements
-
Fix boot race condition properly (
packages/playground/wordpress/src/boot.ts:454)- Current workaround: boot one worker first, then the rest concurrently
- TODO at line 454: "There is a race here when multiple workers are calling bootRequestHandler(). Fix it."
- Should implement proper synchronization for
/internal/.boot-files-writtencheck
-
Worker pool management enhancements (
run-cli.ts:97)- TODO: "Consider creating more workers on demand if other workers blocked to avoid deadlock"
- Detect worker crashes and replace with new workers
- Consider on-demand worker creation/scaling
-
Clarify
/internal/preloadsharing behavior (boot.ts:185)- @adamziel asked: "Is
/internal/preloadactually shared between instances? Both in CLI and on the web?" - Document that "installing once" only applies to shared FS; private FS needs per-instance creation
- @adamziel asked: "Is
-
Worker respawn on crash (
run-cli.ts:1289)- TODO: "Should we respawn the worker if it exited with an error and the CLI is not shutting down?"
Syscall/WASM Improvements
-
wasm-user-space context validation (
wasm-user-space.ts:25)- TODO: "When receiving this context, validate that all these fields exist"
-
PROXYFS support evaluation (
wasm-user-space.ts:196)- TODO: "Do we still need to support PROXYFS now that Playground CLI uses NODEFS everywhere?"
-
Process exit cleanup (
wasm-user-space.ts:932, 967-968)- TODOs for implementing process-based lock release
- "Explore how to ensure cleanup" for locks held when fd_close fails to find native FD
-
Add test for gethostbyname (
wasm-user-space.ts:990)- TODO: "Add a test for this"
Features
-
Blueprints v2 JS runner (
run-cli.ts:574-578)- Create a JavaScript-based Blueprints v2 runner
- Test against the same compliance test suite as the PHP-based v2 runner
- Currently disabled with: "Blueprints v2 are temporarily disabled while we rework their runtime implementation."
-
FileLockManagerComposite tracing (
file-lock-manager-composite.ts:9)- TODO: "Add optional granular tracing"
Testing
-
Xdebug during boot (
run-cli.ts:1351)- TODO: "Consider how to avoid Xdebug being enabled during boot"
-
HTTP connection adoption for workers (
run-cli.ts:1513)- TODO: "Explore switching to a worker thread method to adopt an entire HTTP connection"
Review Requests
- xdebug-bridge type change (
packages/php-wasm/xdebug-bridge/src/lib/start-bridge.ts:14)- @mho22 to review the adjustment made for type compatibility with run-cli.ts
Related PR: #3150