CIP-3933: Deepen Context protocol lifecycle ownership - #463
Conversation
5398073 to
2117cf8
Compare
Concentrate PostgreSQL operation, portal, and statement-metrics lifecycle transitions behind Context while preserving fail-closed correlation and tolerant wire handling. Refs: CIP-3933 Signed-off-by: Toby Hede <toby@cipherstash.com>
2117cf8 to
d3800a1
Compare
fd22c05 to
b4e33f2
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Requesting changes for three blocking issues:
-
Distinct portals contaminate each other’s statement metrics. bind_handler gets the prepared Statement template scope and mutates it with Bind-specific param_bytes, encryption duration, encrypted count, and encrypted state before storing that same scope ID in each Portal (frontend.rs:1101-1145, 1210-1220). set_execute_for_portal only clones the template when Execute arrives (context/mod.rs:563-577). For Parse S; Bind P1(A); Bind P2(B); Execute P1, P1 inherits B metadata and accumulated P1+P2 encryption time; P2 later gets the same contaminated snapshot. This violates ADR-0002: distinct Portals and repeated executions receive distinct scopes. Please isolate Bind measurements per Portal/execution and add a regression test with two portals from one Statement and different parameters.
-
Correlated stale state fails open despite ADR-0002 requiring inaccessible, stale, or inconsistent state to fail the connection closed. Context getters return None for an unknown OperationId (context/mod.rs:973-1030), so Backend treats a DataRow for Some(unknown_operation) as passthrough (backend.rs:775-792). Backend also catches UnknownOperation/OperationWithoutExecute for terminal and Describe transitions and forwards them (backend.rs:89-123); the tests at backend.rs:946-1025 codify this. A response carrying a correlated but unknown operation is stale/inconsistent, unlike a response with no operation. Please distinguish those cases and fail closed for the former.
-
Commits fddd09d and b4e33f2 have no Signed-off-by trailer, contrary to CONTRIBUTING.md requiring every commit to be DCO signed off.
Non-blocking documentation note: the new proxy architecture walkthrough is not linked from the documentation index/site navigation, so it is difficult to discover. This does not affect my request-changes decision.
Validation: cargo test -q -p cipherstash-proxy --lib -- --test-threads=1 passes locally (242 tests), and git diff --check passes.
Signed-off-by: Toby Hede <toby@cipherstash.com>
Signed-off-by: Toby Hede <toby@cipherstash.com>
Signed-off-by: Toby Hede <toby@cipherstash.com>
b4e33f2 to
244b44d
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Re-review of 244b44d: the earlier cross-portal contamination, fail-open terminal/Describe/DataRow paths, and DCO issues are resolved. Two lifecycle blockers remain:
-
A failed Bind leaks its newly allocated portal metrics scope. start_portal_metrics_scope inserts the scope at context/mod.rs:886-905, before encryption/rewrite at frontend.rs:1113-1131, and the scope only becomes referenced when add_portal succeeds at frontend.rs:1144-1146. Every Bind error instead follows frontend.rs:226-267, which records the operation error but never removes that scope. ReadyForQuery cleanup has no candidate ID for it, so repeated rejected Binds grow connection-local state even though no execution occurred. Please reclaim the scope on every failure path and add a regression test asserting the active scope count after a failed Bind.
-
Correlated ParseComplete/BindComplete for a stale operation still fails open. Backend calls complete_non_execution at backend.rs:149-155, but context/mod.rs:397-403 removes the ID without checking whether it existed and always returns success. Some(unknown_operation_id) is stale/inconsistent correlated state under ADR-0002 and should fail the connection closed, just like the other corrected paths. Please return UnknownOperation when the removal finds nothing and cover it with a regression test.
Repository-standard follow-up: packages/cipherstash-proxy/CONTEXT.md explicitly says to avoid session for the statement-metrics-scope concept, but this change introduces new names such as records_session, record_finished_session, finished_sessions, and a session local in frontend.rs:122. Please use metrics-scope terminology for new identifiers; the acknowledged legacy SessionId and metric names do not need a sweep in this PR.
Non-blocking documentation note: the architecture walkthrough remains unlinked from the documentation index/site navigation. This does not affect the request-changes decision.
Validation on this head: 243 library tests pass; cargo check --all-features, cargo fmt --check, and git diff --check pass. CI is still in progress.
Signed-off-by: Toby Hede <toby@cipherstash.com>
Signed-off-by: Toby Hede <toby@cipherstash.com>
freshtonic
left a comment
There was a problem hiding this comment.
Approved on re-review of f6be991. The prior lifecycle, DCO, and statement-metrics terminology blockers are resolved. Portal metrics are isolated per Bind/execution, failed Bind scopes are reclaimed, and stale correlated completions now fail closed with regression coverage.
Non-blocking documentation note: proxy-architecture-walkthrough.html is still not linked from the documentation index/site navigation, so it may be difficult to discover.
Optional non-blocking cleanup: ResultOptionTestExt hides UnknownOperation as is_none in tests, and the failed-Bind fixture has some duplicated ColumnConfig/Column construction.
Validation on this head: 245 library tests pass; cargo check --all-features, cargo fmt --check, and git diff --check pass. CI is still in progress.
Signed-off-by: Toby Hede <toby@cipherstash.com>
Signed-off-by: Toby Hede <toby@cipherstash.com>
Summary
Validation
cargo test -q -p cipherstash-proxy --lib -- --test-threads=1(223 passed)cargo check -p cipherstash-proxy --all-featurescargo fmt --all -- --checkgit diff --checkLinear: CIP-3933