Make retired state a function of history alone by hardening CRDT logic - #4585
TrueDoctor wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
4cd4b14 to
dee5ad2
Compare
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Confidence score: 3/5
- In
document/graph-storage/src/document.rs, marks that arrive before their delta can leave a stale hot operation behind; a laterSession::mergemay add the delta without retrying cleanup, allowing the operation to be retired again as a duplicate history delta. Retry cleanup when the delta arrives. - In
document/graph-storage/src/history.rs,drop_retired_hot_opsscans the history once per hot-log entry, so cleanup can become O(history × hot_log) and stall sessions as both grow. Build a timestamp set once per retain pass or index timestamps.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="document/graph-storage/src/history.rs">
<violation number="1" location="document/graph-storage/src/history.rs:44">
P2: This scan runs once per hot-log entry in `drop_retired_hot_ops`, making cleanup O(history × hot_log) and potentially stalling sessions as both grow. Build a timestamp set once per retain pass or index timestamps.</violation>
</file>
<file name="document/graph-storage/src/document.rs">
<violation number="1" location="document/graph-storage/src/document.rs:156">
P2: When marks arrive before their delta, this filter retains the hot op; a later canonical `Session::merge` adds the delta without retrying cleanup. The stale op can then be retired again as a second history delta; retry this cleanup whenever history advances and refold after a drop.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| /// Whether history holds a delta authored at `timestamp`. | ||
| pub fn contains_timestamp(&self, timestamp: TimeStamp) -> bool { | ||
| self.deltas.iter().any(|delta| delta.timestamp == timestamp) |
There was a problem hiding this comment.
P2: This scan runs once per hot-log entry in drop_retired_hot_ops, making cleanup O(history × hot_log) and potentially stalling sessions as both grow. Build a timestamp set once per retain pass or index timestamps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/graph-storage/src/history.rs, line 44:
<comment>This scan runs once per hot-log entry in `drop_retired_hot_ops`, making cleanup O(history × hot_log) and potentially stalling sessions as both grow. Build a timestamp set once per retain pass or index timestamps.</comment>
<file context>
@@ -39,6 +39,11 @@ impl History {
+ /// Whether history holds a delta authored at `timestamp`.
+ pub fn contains_timestamp(&self, timestamp: TimeStamp) -> bool {
+ self.deltas.iter().any(|delta| delta.timestamp == timestamp)
+ }
+
</file context>
| /// ahead of the delta carrying the op into history, stranding its effect in the working registry. | ||
| fn drop_retired_hot_ops(&mut self) -> bool { | ||
| let before = self.hot_log.len(); | ||
| self.hot_log.retain(|hot_op| !self.history.contains_timestamp(hot_op.timestamp)); |
There was a problem hiding this comment.
P2: When marks arrive before their delta, this filter retains the hot op; a later canonical Session::merge adds the delta without retrying cleanup. The stale op can then be retired again as a second history delta; retry this cleanup whenever history advances and refold after a drop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At document/graph-storage/src/document.rs, line 156:
<comment>When marks arrive before their delta, this filter retains the hot op; a later canonical `Session::merge` adds the delta without retrying cleanup. The stale op can then be retired again as a second history delta; retry this cleanup whenever history advances and refold after a drop.</comment>
<file context>
@@ -142,18 +141,21 @@ impl Document {
+ /// ahead of the delta carrying the op into history, stranding its effect in the working registry.
+ fn drop_retired_hot_ops(&mut self) -> bool {
+ let before = self.hot_log.len();
+ self.hot_log.retain(|hot_op| !self.history.contains_timestamp(hot_op.timestamp));
+
+ before != self.hot_log.len()
</file context>
dee5ad2 to
4b2a63d
Compare
No description provided.