feat: add a maximum hold duration to expiring locks - #2221
Conversation
Sorry, something went wrong.
Add an optional absolute deadline that activity renewals cannot extend, preventing indefinitely renewed read locks from starving writers. A zero value preserves unlimited existing behavior, while an opt-in file-locker configuration caps holds at one hour.
There was a problem hiding this comment.
Pull request overview
Adds an optional maximum hold duration for expiring locks, preserving uncapped behavior by default.
Changes:
- Adds maximum hold duration support.
- Adds capped and uncapped behavior tests.
- Adds an opt-in one-hour capped file-locker configuration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Review findings |
|---|---|
| test/unit/util/locking/WrappedExpiringReadWriteLocker.test.ts | Nit (3 votes): Add coverage verifying the underlying lock is released when the cap fires. |
| src/util/locking/WrappedExpiringReadWriteLocker.ts | Moderate (3 votes): Prevent renewals and clear inactivity timers after the maximum deadline wins. |
| config/util/resource-locker/file-capped.json | Nit (2 votes): Document the capped configuration in config/util/README.md. |
config/util/resource-locker/file-capped.json:23
- This PR adds a public locking option and ships a new configuration, but RELEASE_NOTES.md is unchanged. The repository guidance requires release notes for relevant feature/configuration changes (and for minor releases); please document the new maximum-hold option and the opt-in file configuration before merging.
"maxHoldDuration": 3600000
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
jeswr
left a comment
There was a problem hiding this comment.
This is a configuration change so should be discussed in the release notes
Sorry, something went wrong.
|
Addressed the review in a6a900b: added the release-note and resource-locker documentation, fixed timer cleanup and post-deadline renewals, and added coverage for underlying lock release. |
Sorry, something went wrong.
|
Follow-up 6e6158d removes unreachable defensive branches identified by the coverage gate. The focused locker suite now has 100% statement, branch, function, and line coverage. |
Sorry, something went wrong.
|
I have reviewed this, it is ready for maintainer review |
Sorry, something went wrong.
📁 Related issues
Related to #2219.
This is the versions/next-major port of jeswr/CommunitySolidServer#87.
✍️ Description
Adds an optional absolute maximum hold duration to WrappedExpiringReadWriteLocker. The existing inactivity timeout can still be renewed, while the new deadline is never extended; reaching either deadline releases the wrapped lock and rejects the operation.
The new constructor argument defaults to 0, preserving the current uncapped behavior. An opt-in file-locker configuration caps total hold time at one hour, and its Components.js contexts have been updated for v8/versions/next-major.
Suggested label: semver.minor.
Review questions:
Validation:
npm run validate currently fails while parsing the generated AuxiliaryLinkMetadataWriter component metadata. The same failure was reproduced on a pristine origin/versions/next-major worktree, before this configuration is evaluated.
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether