-
Notifications
You must be signed in to change notification settings - Fork 22
feat: atomic bean updates with body modifications #72
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
Conversation
hmans
left a comment
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.
Wonderful PR, thank you very much! Also, apologies for taking a bit to respond -- just working on too much stuff in parallel. Aaaaaah!
|
@matleh May I ask you to resolve the merge conflicts with |
Agents can now atomically update status, metadata, and body content together with a single etag validation, eliminating race conditions. - Add BodyModification input type supporting multiple replacements and append - Add bodyMod field to UpdateBeanInput for structured body modifications - Remove separate replaceInBody/appendToBody mutations (consolidated) - Update CLI to allow combining --body-replace-old and --body-append - Implement transactional all-or-nothing semantics - Add comprehensive test coverage
Move etag validation inside Core.Update() under write lock to prevent lost updates in concurrent scenarios. - Add ETagMismatchError and ETagRequiredError to beancore package - Update Core.Update() signature to accept ifMatch parameter - Validate etag against on-disk content to handle Go pointer aliasing - Update all Core.Update() call sites to pass ifMatch - Remove validateETag from graph resolver (now in Core) - Add comprehensive tests for etag validation scenarios - Add setupTestCoreWithRequireIfMatch helper for testing The race condition occurred when two threads validated etags before either acquired the write lock, allowing both to pass validation and the second update to overwrite the first. Now validation happens atomically with the update under the write lock, preventing lost updates.
d1e9b77 to
a186d7e
Compare
Rebase Completed - Conflicts ResolvedI've resolved the merge conflicts and rebased this PR on Upstream Changes IncorporatedFrom PR #67: Added From PR #60: Added Conflict Resolution Decisions1. Removed These are superseded by the The
2. Applied ETag race condition fix to new upstream mutations The upstream
Testing✅ All tests passing Let me know if you'd like me to adjust anything about the conflict resolution approach! |
|
also see follow-up PR #81 |
|
There seems to be a driveby addition of a session Markdown file, may I ask you to remove it? |
Add comprehensive etag validation tests for: - updateBean - setParent - addBlocking - removeBlocking Each mutation now has tests verifying: - Success with correct etag - Failure with wrong etag returning ETagMismatchError
a186d7e to
2b39d4b
Compare
I am sorry - removed. |
|
Merged, thanks for the PR! 🙏 |
Motivation
A previous PR introduced
replaceInBodyandappendToBodymutations for modifying bean body content. While these worked well for single operations, we quickly realized limitations:This PR consolidates body modification capabilities into a single atomic operation and fixes the concurrency issue.
Changes
GraphQL API Improvements
Consolidated body modifications into
updateBean:BodyModificationinput type supporting multiplereplaceoperations andappendReplaceOperationinput type witholdandnewfieldsbodyModfield toUpdateBeanInputfor structured body modificationsreplaceInBodyandappendToBodymutations (redundant, breaking change)New capabilities:
Race Condition Fix
Moved etag validation under write lock:
ETagMismatchErrorandETagRequiredErrorto beancore packageCore.Update()signature to acceptifMatchparameterCore.Update()under write lockThe race condition occurred when two threads validated etags before either acquired the write lock, allowing both to pass validation. The second update would silently overwrite the first. Now validation happens atomically with the update operation.
CLI Updates
--body-replace-old/newand--body-appendtogetherapplyBodyMod()to properly check forbodyModfield--bodyand--body-file(full replacement)beans querycommandDocumentation
cmd/prompt.tmplwith concise body modification examplesbodyModusage for multiple replacementsExamples
CLI - Combined operations:
GraphQL - Multiple replacements:
Execution Order
replaceoperations applied sequentially (each operates on result of previous)appendoperation applied to final resultBreaking Changes
replaceInBodymutation removed (useupdateBeanwithbodyMod.replace)appendToBodymutation removed (useupdateBeanwithbodyMod.append)Migration:
Testing
Manual testing performed:
Unit tests added:
UpdateBeanwithbodyModtest coverage (10 cases)All tests passing ✅