fix(servers): make PATCH a field-scoped, revision-guarded write - #1722
Open
atirna wants to merge 1 commit into
Open
fix(servers): make PATCH a field-scoped, revision-guarded write#1722atirna wants to merge 1 commit into
atirna wants to merge 1 commit into
Conversation
PATCH /api/servers/{path} read the full card, merged the client patch,
and persisted the whole merged card with an unconditional full-card
$set. Two problems (issue agentic-community#1716):
- a card stored under the slash variant of its path (readable that way
because get() falls back to the alternate _id) never matched the
update filter, so the route returned 500 "Failed to save server"
for a description-only patch
- any field written concurrently between the read and the write (an
egress OAuth config, a rotated credential, health status) was
silently overwritten even though the patch never mentioned it
update() now scopes its $set to the fields the patch actually
supplies (plus updated_at), tries the slash-variant _id the same way
get() does, and accepts an expected_updated_at that joins the _id in
the same update_one filter, making the If-Match check an atomic
compare-and-set instead of a client-side pre-check. The PATCH route
passes the field scope and the revision it read; PUT keeps its
full-card replacement semantics but its If-Match now also moves into
the repository write.
Verified against a real MongoDB CE (mongo:8.2): the failing
description-only patch on a slash-variant card, the lost credential
update, and the stale If-Match race each reproduce on main and pass
with this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
I changed
PATCH /api/servers/{path}from a read-merge-full-card-write into a field-scoped, revision-guarded write, and moved the If-Match check into the repositoryupdate_oneitself.What was happening (issue #1716), on the MongoDB CE backend:
update_server_endpoint/patch_server_endpointread the full card, merged the narrow patch, thenDocumentDBServerRepository.update()persisted the whole merged card with an unconditional full-card$setget()falls back to the alternate_id) never matched the update filter, so a description-only PATCH came back500 Failed to save serverWhat changed:
DocumentDBServerRepository.update()scopes its$settoupdated_fields(plusupdated_at) when given, tries the slash-variant_idon a miss the same wayget()does, and takes an optionalexpected_updated_atthat joins the_idin the sameupdate_onefilter — so the If-Match compare-and-set is one atomic database operation, not a client-side pre-checkPATCHpasses the patch's field scope plus the revision it read; a guarded write that matches nothing is a412, not a500PUTkeeps its full-card replacement semantics, but its If-Match now also rides the repository writeTesting
main, against a real mongo:8.2: a description-only PATCH on a card stored under/legacy/read fine butupdate_onematched 0 documents and the route returned500 Failed to save server$set(storedauth_credential_encryptedreverted to the pre-rotation value)pytest tests/integration/test_server_patch_concurrency.py tests/unit/repositories/test_documentdb_server_repository.py tests/unit/api/test_server_routes_patch.py tests/unit/api/test_server_routes_put.py— the slash-variant card patches with 2xx, the concurrent credential survives, a stale If-Match fails atomically with nothing written, an identical replay is harmless, and clearingproxy_pass_urlstill$unsets the identity-url sidecarThe new integration test follows the existing
test_agent_batch_repository.pypattern (real collection, skip when MongoDB is unreachable), so it runs in CI alongside the mongo:8.2 service the workflow already starts.Fixes #1716