Skip to content

better dcache spine integration#52

Open
ninaiiad wants to merge 9 commits intomainfrom
ng/dcache+
Open

better dcache spine integration#52
ninaiiad wants to merge 9 commits intomainfrom
ng/dcache+

Conversation

@ninaiiad
Copy link
Contributor

@ninaiiad ninaiiad commented Mar 19, 2026

@ninaiiad ninaiiad requested a review from a team March 19, 2026 17:31
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

@ninaiiad ninaiiad changed the title fix consume_with_dcache closure better dcache spine integration Mar 20, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 12 additional findings in Devin Review.

Open in Devin Review

Comment on lines +234 to +239
match c.consume_collaborative(&mut self.producers, &mut read) {
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 consume_with_dcache_collaborative unconditionally sets did_work = true and calls handler on Empty

The catch-all match pattern result => { self.did_work = true; handle(result, ...) } matches every variant including DCacheRead::Empty and DCacheRead::SpedPast. The previous version of this code (commit 48671fa) correctly filtered these out with DCacheRead::Empty => break and DCacheRead::SpedPast => {}. The fix commit (1aa4c05) correctly removed the loop (collaborative should consume one item at a time) but accidentally also removed the Empty/SpedPast filtering. This causes did_work to always be true, which affects the tile event loop metrics at crates/flux/src/tile/mod.rs:112, and passes Empty/SpedPast results to the handler callback that the old API never exposed.

Suggested change
match c.consume_collaborative(&mut self.producers, &mut read) {
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
match c.consume_collaborative(&mut self.producers, &mut read) {
DCacheRead::Empty | DCacheRead::SpedPast => {}
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +276 to +281
match c.consume_collaborative_internal_message(&mut self.producers, &mut read) {
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 consume_with_dcache_collaborative_internal_message unconditionally sets did_work = true and calls handler on Empty

Same issue as the sibling method: the catch-all match pattern always matches, including DCacheRead::Empty and DCacheRead::SpedPast. The previous version (commit 48671fa) correctly filtered these. This always marks did_work = true regardless of whether any message was consumed, and passes Empty/SpedPast to the handler callback.

Suggested change
match c.consume_collaborative_internal_message(&mut self.producers, &mut read) {
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
match c.consume_collaborative_internal_message(&mut self.producers, &mut read) {
DCacheRead::Empty | DCacheRead::SpedPast => {}
result => {
self.did_work = true;
handle(result, &mut self.producers);
}
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants