feat(studio): optimistic concurrency for role/connector catalog drafts#434
feat(studio): optimistic concurrency for role/connector catalog drafts#434
Conversation
Closes #432. Catalog state and draft persistence is now version-aware. Proto: - RoleCatalogState / ConnectorCatalogState: add `last_applied_event_version` - *CatalogSavedEvent / *DraftSavedEvent / *DraftDeletedEvent: add `optional int64 expected_version` (proto3 optional; unset = skip check) Actor (RoleCatalogGAgent, ConnectorCatalogGAgent): - Each Apply step bumps `LastAppliedEventVersion`. - Each handler validates `expected_version` if set; mismatch raises EventStoreOptimisticConcurrencyException (foundation-defined). Store / contracts: - IRoleCatalogStore / IConnectorCatalogStore: write methods now accept optional `expectedVersion` (default null = backward compatible). - Stored DTOs (StoredRole/ConnectorCatalog/Draft) carry `Version`. - Service requests (Save*Request) carry `ExpectedVersion?`; responses carry `Version`. HTTP (RolesController, ConnectorsController): - GET writes `ETag: "<version>"` response header. - PUT/DELETE read `If-Match` header (also accept `expectedVersion` in request body); 409 Conflict on EventStoreOptimisticConcurrencyException with `code: VERSION_CONFLICT`. - New ETagSupport helper for header parsing/emission. Backward compatibility: - Callers omitting `expected_version` / `If-Match` continue to work as last-writer-wins (existing CLI tests / fixtures unchanged). Tests: - Apply increment: RoleCatalog_Apply_IncrementsLastAppliedEventVersion + ConnectorCatalog equivalent. - Store-level event plumbing: SaveDraft/DeleteDraft with/without expectedVersion → event ExpectedVersion field correctly set/unset. - Store-level read: GetDraft surfaces Version from projection state. Out of scope (deferred): - End-to-end actor harness for the conflict-throw path. Foundation's EventStoreOptimisticConcurrencyException semantics are exercised separately; integration coverage to follow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
I found two correctness issues in the optimistic concurrency plumbing. The focused test slice passes locally, but these should be addressed before relying on the new ETag flow.
| FileExists: true, | ||
| Roles: catalog.Roles); | ||
| Roles: catalog.Roles, | ||
| Version: refreshed?.LastAppliedEventVersion ?? ((expectedVersion ?? catalog.Version) + 1)); |
There was a problem hiding this comment.
This can return a stale version/ETag after a successful write. ReadProjectedStateAsync reads the CQRS projection immediately after dispatch, but that projection is eventually consistent; if it still contains the pre-write document, the response uses the old LastAppliedEventVersion. A client can then GET v5 -> PUT If-Match v5 successfully, receive ETag: "5" again, and hit a false VERSION_CONFLICT on the next guarded write. The same pattern exists in the connector catalog and draft save paths. The write response should derive the next version from the committed actor/write-side result, or avoid emitting a stronger post-write ETag than the system can actually guarantee.
There was a problem hiding this comment.
Fixed in 8b1d922. Dropped the post-write projection re-read entirely — SaveRoleCatalogAsync / SaveRoleDraftAsync / connector equivalents now return Version = expectedVersion + 1 deterministically (the actor enforces the match and Apply increments by exactly one). When the caller doesn't supply expected_version, the response carries Version = 0 and the controller skips the ETag header — clients re-GET for an authoritative value instead of being lied to. Helper NextDeterministicVersion documents the invariant. Controller-level test SaveDraft_WithValidIfMatch_PassesExpectedVersionToStore_AndEmitsDeterministicETag locks in If-Match: "5" → ETag: "6" for the happy path.
| return null; | ||
|
|
||
| var unquoted = raw.Trim('"'); | ||
| return long.TryParse(unquoted, out var version) ? version : null; |
There was a problem hiding this comment.
Malformed or unsupported If-Match values currently disable concurrency protection because this returns null for both “header absent” and “header present but invalid”. That means a caller intending a guarded write with If-Match: W/"5", If-Match: "5", "6", or a typo silently falls back to last-writer-wins. Please distinguish absence from parse failure and reject invalid preconditions instead of treating them as unguarded writes.
There was a problem hiding this comment.
Fixed in 8b1d922. TryParseIfMatch is replaced by tristate ParseIfMatch(HttpRequest, out long) → IfMatchStatus { Absent | Valid | Invalid }. Controllers reject Invalid with 400 MALFORMED_IF_MATCH; only Valid values bind ExpectedVersion; Absent passes through unchanged. The new theory ParseIfMatch_WhenMalformed_ReturnsInvalid covers W/"5", "5","6", *, non-numeric, empty quoted, and negative; controller-level SaveDraft_WithMalformedIfMatch_Returns400_AndDoesNotInvokeStore and the DELETE counterpart prove the store is never called when a malformed precondition arrives.
…d preconditions Addresses the two correctness issues raised in #434 review: 1) Stale post-write ETag (ActorBackedRoleCatalogStore.cs / ConnectorCatalogStore.cs): The previous code re-read the projection immediately after dispatch to compute the response Version. The projection is eventually consistent and may still contain the pre-write document, so the response ETag could be the same one the caller just used as If-Match — guaranteeing a false VERSION_CONFLICT on the next guarded write. Fix: derive the post-write Version deterministically from `expected_version + 1` (the actor enforces the match and increments by exactly one in Apply). When the caller did not supply expected_version, return Version = 0 to signal "unknown — re-GET for authoritative version", and the controller no longer emits an ETag in that case. 2) Malformed If-Match silently bypassing concurrency control (ETagSupport.cs): The previous TryParseIfMatch returned null for both "absent" and "invalid", letting `If-Match: W/"5"`, `If-Match: "5","6"`, `If-Match: *`, typos, etc. silently fall back to last-writer-wins. Fix: ParseIfMatch is now tristate (Absent / Valid / Invalid). Controllers reject Invalid with 400 MALFORMED_IF_MATCH; only Valid values bind to ExpectedVersion; Absent passes through unchanged. Tests: - New ETagSupportTests: theory matrix covering valid forms (bare / quoted), absent/empty header, weak validators, multi-value, wildcard, non-numeric, negative. - New RolesControllerETagTests: malformed If-Match → 400 on PUT/DELETE with no store invocation; valid If-Match plumbs ExpectedVersion and emits deterministic ETag; absent If-Match emits no ETag; store-thrown EventStoreOptimisticConcurrencyException → 409. - Updated existing store tests to assert deterministic Version semantics (expected_version=5 → response Version=6; no expected_version → Version=0). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Re-reviewed the fix. The stale post-write ETag issue and malformed If-Match fallback are addressed, and the focused ETag/controller/store tests pass locally. I left one remaining inline concern around mixed header/body preconditions.
| return true; | ||
| } | ||
|
|
||
| effective = status == IfMatchStatus.Valid && request.ExpectedVersion is null |
There was a problem hiding this comment.
This still lets the request body override an explicit If-Match precondition. The XML comment says the header takes precedence, but when both are present this keeps request.ExpectedVersion; more importantly, an HTTP If-Match: "5" can be bypassed by sending expectedVersion: 6 in the body, allowing the write to succeed when the header precondition should fail. If If-Match is present and valid, use that value as the expected version, or reject the request when the body value disagrees. The connector controller has the same pattern.
There was a problem hiding this comment.
Fixed in 819b6ab. When If-Match parses cleanly the header is authoritative; if the body explicitly disagrees the request is rejected with 400 IF_MATCH_BODY_MISMATCH. Body-only (header absent) and header-only (body null) paths are unchanged. Symmetric fix in both RolesController and ConnectorsController. New tests Save_WhenIfMatchHeaderDisagreesWithBodyExpectedVersion_Returns400, SaveDraft_WhenIfMatchHeaderDisagreesWithBodyExpectedVersion_Returns400, and SaveDraft_WhenIfMatchHeaderAgreesWithBodyExpectedVersion_HeaderWinsAndStoreReceivesIt cover both controllers.
Add ConnectorsControllerETagTests mirroring RolesControllerETagTests
(ConnectorsController.cs was at 0% — never exercised). Expand the
role tests to cover Get / GetDraft (ETag emission), catalog Save with
malformed/valid/conflict If-Match, and DeleteDraft happy + conflict
paths.
Per controller, locked-in HTTP-boundary contract is:
- GET / GET draft → ETag header from current state version
- PUT / PUT draft
- malformed If-Match → 400 MALFORMED_IF_MATCH, store not invoked
- valid If-Match → store called with expected_version,
ETag header is expected+1
- absent If-Match → no ETag header (Version 0 = unknown)
- store throws OCC → 409 VERSION_CONFLICT
- DELETE draft
- malformed/valid/conflict mirror the PUT semantics
Total: 24 new controller-boundary tests across the two files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…agree Addresses PR #434 review: previously when both an HTTP If-Match header and a body `expectedVersion` were present, the controller silently kept the body value. That meant `If-Match: "5"` paired with body `expectedVersion: 6` would let the write succeed against actor v=6 even though the HTTP precondition would have failed at v=5. New rule: when If-Match parses cleanly, the header is authoritative. If the body explicitly disagrees, the request is internally inconsistent and is rejected with 400 IF_MATCH_BODY_MISMATCH. Body-only (header absent) and header-only (body null) paths are unchanged. Tests: - {Save, SaveDraft}_WhenIfMatchHeaderDisagreesWithBodyExpectedVersion_Returns400 - SaveDraft_WhenIfMatchHeaderAgreesWithBodyExpectedVersion_HeaderWinsAndStoreReceivesIt Same fix in both RolesController and ConnectorsController. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Re-reviewed the latest fix at 819b6ab. The remaining If-Match/body mismatch issue is addressed: valid If-Match is now authoritative, mismatched body expectedVersion returns 400, and both roles/connectors have coverage. No remaining findings from my side on the optimistic concurrency/ETag flow.
Local verification in a detached PR worktree:
dotnet test test/Aevatar.Tools.Cli.Tests/Aevatar.Tools.Cli.Tests.csproj --no-restore --filter "FullyQualifiedName~ETagSupportTests|FullyQualifiedName~RolesControllerETagTests|FullyQualifiedName~ConnectorsControllerETagTests|FullyQualifiedName~ActorBackedStoreAdapterTests" --nologo— 104 passedbash tools/ci/test_stability_guards.sh— passed
1 participant
Summary
Closes #432. Adds optimistic concurrency control to role and connector catalog drafts so that two concurrent writers in the same scope no longer silently overwrite each other.
ETag: \"<version>\"; PUT/DELETE acceptIf-Matchheader (orexpectedVersionrequest body field) and return 409 withcode: VERSION_CONFLICTon mismatch.long? expectedVersion; stored DTOs and responses surfaceVersion.LastAppliedEventVersion; each handler validatesexpected_versionwhen set and raisesEventStoreOptimisticConcurrencyExceptionon mismatch.expected_version/If-Matchcontinue to work as last-writer-wins.Proto changes
RoleCatalogState/ConnectorCatalogState: addint64 last_applied_event_version.*CatalogSavedEvent/*DraftSavedEvent/*DraftDeletedEvent: addoptional int64 expected_version(proto3 optional; unset = skip check).Files
agents/Aevatar.GAgents.RoleCatalog/RoleCatalogGAgent.cs,agents/Aevatar.GAgents.ConnectorCatalog/ConnectorCatalogGAgent.csIRoleCatalogStore/ActorBackedRoleCatalogStore,IConnectorCatalogStore/ActorBackedConnectorCatalogStoreIStudioWorkspaceStore.cs(StoredRoleCatalog/Draft,StoredConnectorCatalog/DraftgetVersion)RoleContracts.cs,ConnectorContracts.cs(Versionon responses,ExpectedVersion?on requests)RoleCatalogService.cs,ConnectorService.csRolesController.cs,ConnectorsController.cs(ETag/If-Match plumbing + 409)ETagSupport.cs(shared header helpers)Test plan
New test coverage
In `ActorBackedGAgentStateTransitionTests.cs`:
In `ActorBackedStoreAdapterTests.cs`:
Out of scope (deferred)
🤖 Generated with Claude Code