Skip to content

Commit 5d02541

Browse files
committed
fix(slack): address hosted polling review findings
1 parent a06e58b commit 5d02541

6 files changed

Lines changed: 849 additions & 80 deletions

File tree

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
{
2+
"ok": true,
3+
"channels": [
4+
{
5+
"id": "C08CONNECT01",
6+
"context_team_id": "T08LOCALITY1",
7+
"team_id": "T08LOCALITY1",
8+
"name": "shared-partner",
9+
"topic": {
10+
"value": "Cross-company coordination"
11+
},
12+
"purpose": {
13+
"value": "Slack Connect"
14+
},
15+
"created": 1780000000,
16+
"updated": 1780000010123,
17+
"is_archived": false,
18+
"is_private": true,
19+
"is_shared": true,
20+
"is_ext_shared": true,
21+
"is_org_shared": false,
22+
"is_member": true,
23+
"shared_team_ids": [
24+
"T08EXTERNAL1",
25+
"T08LOCALITY1"
26+
]
27+
}
28+
],
29+
"response_metadata": {
30+
"next_cursor": ""
31+
}
32+
}
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
{
2+
"ok": true,
3+
"has_more": false,
4+
"messages": [
5+
{
6+
"type": "message",
7+
"subtype": "channel_join",
8+
"ts": "1780000000.000100",
9+
"user": "U08ADA00001",
10+
"text": "<@U08ADA00001> joined the channel"
11+
},
12+
{
13+
"type": "message",
14+
"subtype": "channel_topic",
15+
"ts": "1780000001.000100",
16+
"user": "U08GRACE001",
17+
"text": "set the channel topic",
18+
"files": [
19+
{
20+
"id": "F08SYSTEM01",
21+
"url_private": "https://provider.invalid/system-file-secret"
22+
}
23+
]
24+
},
25+
{
26+
"type": "message",
27+
"subtype": "huddle_thread",
28+
"ts": "1780000002.000100",
29+
"text": "",
30+
"room": {
31+
"secret": "system-room-secret"
32+
}
33+
}
34+
],
35+
"response_metadata": {
36+
"next_cursor": ""
37+
}
38+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
[
2+
{
3+
"message": {
4+
"channel_id": "C08ENGINEER1",
5+
"ts": "1780000000.000100",
6+
"thread_ts": null,
7+
"user_id": "U08ADA00001",
8+
"text": "[Slack system event: channel_join] <@U08ADA00001> joined the channel",
9+
"edited_ts": null,
10+
"deleted": false,
11+
"file_ids": []
12+
},
13+
"reply_count": 0
14+
},
15+
{
16+
"message": {
17+
"channel_id": "C08ENGINEER1",
18+
"ts": "1780000001.000100",
19+
"thread_ts": null,
20+
"user_id": "U08GRACE001",
21+
"text": "[Slack system event: channel_topic] set the channel topic",
22+
"edited_ts": null,
23+
"deleted": false,
24+
"file_ids": []
25+
},
26+
"reply_count": 0
27+
},
28+
{
29+
"message": {
30+
"channel_id": "C08ENGINEER1",
31+
"ts": "1780000002.000100",
32+
"thread_ts": null,
33+
"user_id": null,
34+
"text": "[Slack system event: huddle_thread]",
35+
"edited_ts": null,
36+
"deleted": false,
37+
"file_ids": []
38+
},
39+
"reply_count": 0
40+
}
41+
]

crates/locality-slack/src/portable/hosted/poll.rs

Lines changed: 77 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,8 @@ pub struct HostedSlackRepliesPageV1 {
7575
pub poll_overlap_watermark: String,
7676
pub root_message_id: String,
7777
pub root_reply_count: u32,
78+
#[serde(default, skip_serializing_if = "Option::is_none")]
79+
pub reconciliation: Option<HostedSlackRepliesReconciliationV1>,
7880
pub request_cursor: Option<String>,
7981
pub next_cursor: Option<String>,
8082
pub observed_at: String,
@@ -83,6 +85,12 @@ pub struct HostedSlackRepliesPageV1 {
8385
pub files: Vec<RawHostedSlackFileMetadata>,
8486
}
8587

88+
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
89+
#[serde(rename_all = "snake_case")]
90+
pub enum HostedSlackRepliesReconciliationV1 {
91+
ThreadNotFound,
92+
}
93+
8694
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
8795
pub enum HostedSlackPageApplyOutcomeV1 {
8896
Applied,
@@ -280,6 +288,20 @@ impl HostedSlackRepliesPageV1 {
280288
));
281289
}
282290
}
291+
if self.reconciliation == Some(HostedSlackRepliesReconciliationV1::ThreadNotFound)
292+
&& (self.request_cursor.is_some()
293+
|| self.next_cursor.is_some()
294+
|| self.root_reply_count != 0
295+
|| self.messages.len() != 1
296+
|| self.messages[0].ts != self.root_message_id
297+
|| !self.messages[0].deleted
298+
|| !self.messages[0].text.is_empty()
299+
|| !self.messages[0].file_ids.is_empty())
300+
{
301+
return Err(HostedSlackPollError::IncompleteCandidate(
302+
"thread-not-found reconciliation",
303+
));
304+
}
283305
validate_serialized_page_size(self, "replies page")?;
284306
Ok(())
285307
}
@@ -451,11 +473,21 @@ impl HostedSlackPollCheckpointV1 {
451473
return Err(HostedSlackPollError::PageWindowMismatch);
452474
}
453475
let first_page = page.request_cursor.is_none();
476+
let deleted_root_reconciliation =
477+
page.reconciliation == Some(HostedSlackRepliesReconciliationV1::ThreadNotFound);
454478
let expectation_is_from_this_catch_up = page.phase
455479
!= HostedSlackPollPhaseV1::CatchUpReplies
456480
|| catch_up_history_contains_root(&next, &page.root_message_id)?
457481
|| !first_page;
458-
if expectation_is_from_this_catch_up {
482+
if deleted_root_reconciliation {
483+
upsert_root_expectation(
484+
&mut next.candidate.root_expectations,
485+
HostedSlackRootExpectationV1 {
486+
root_message_id: page.root_message_id.clone(),
487+
expected_reply_count: 0,
488+
},
489+
);
490+
} else if expectation_is_from_this_catch_up {
459491
let expected = expected_reply_count(&next, &page.root_message_id)?;
460492
if expected != page.root_reply_count {
461493
return Err(HostedSlackPollError::ReplyCountMismatch {
@@ -483,6 +515,21 @@ impl HostedSlackPollCheckpointV1 {
483515
for message in &accepted_page.messages {
484516
let message_time = parse_slack_timestamp("page.message.ts", &message.ts)?;
485517
if message.ts == page.root_message_id {
518+
if deleted_root_reconciliation {
519+
let root = next
520+
.candidate
521+
.messages
522+
.iter_mut()
523+
.find(|candidate| candidate.ts == message.ts)
524+
.ok_or_else(|| HostedSlackPollError::MissingRoot(message.ts.clone()))?;
525+
if normalized_root_id(root).is_some() {
526+
return Err(HostedSlackPollError::InvalidMessageRelationship(
527+
message.ts.clone(),
528+
));
529+
}
530+
*root = message.clone();
531+
continue;
532+
}
486533
apply_message_current_state(
487534
&mut next.candidate.messages,
488535
message.clone(),
@@ -1014,19 +1061,45 @@ fn prepare_reply_phase(
10141061
.filter(|message| normalized_root_id(message).is_none())
10151062
.map(|message| message.ts.clone())
10161063
.collect::<Vec<_>>();
1064+
let yielded = checkpoint
1065+
.candidate
1066+
.stage_yielded_reply_root_ids
1067+
.iter()
1068+
.cloned()
1069+
.collect::<BTreeSet<_>>();
1070+
checkpoint.completed_roots.clear();
1071+
let mut pending = Vec::new();
1072+
for root in roots {
1073+
let exact_zero_from_history = touched.contains(&root)
1074+
&& !yielded.contains(&root)
1075+
&& expected_reply_count(checkpoint, &root)? == 0;
1076+
if exact_zero_from_history {
1077+
checkpoint
1078+
.candidate
1079+
.messages
1080+
.retain(|message| normalized_root_id(message) != Some(root.as_str()));
1081+
checkpoint.completed_roots.push(HostedSlackCompletedRootV1 {
1082+
root_message_id: root,
1083+
expected_reply_count: 0,
1084+
observed_reply_count: 0,
1085+
completed_phase: reply_phase,
1086+
});
1087+
} else {
1088+
pending.push(root);
1089+
}
1090+
}
10171091
let applied_pages = checkpoint
10181092
.evidence
10191093
.iter()
10201094
.filter(|evidence| matches!(evidence, HostedSlackPollEvidenceV1::AppliedPage { .. }))
10211095
.count();
1022-
let minimum_required_pages = applied_pages.saturating_add(roots.len());
1096+
let minimum_required_pages = applied_pages.saturating_add(pending.len());
10231097
if minimum_required_pages > MAX_HOSTED_SLACK_APPLIED_PAGES_V1 {
10241098
return Err(HostedSlackPollError::CollectionTooLarge(
10251099
"catch-up root sweep",
10261100
));
10271101
}
1028-
checkpoint.completed_roots.clear();
1029-
roots
1102+
pending
10301103
};
10311104
checkpoint
10321105
.completed_roots

0 commit comments

Comments
 (0)