Skip to content

Conversation

@erights
Copy link
Contributor

@erights erights commented Jan 2, 2026

Most PRs should close a specific Issue. All PRs should at least reference one or more Issues. Edit and/or delete the following lines as appropriate (note: you don't need both refs and closes for the same one):

Closes: #XXXX
Refs: #XXXX

Description

Add a description of the changes that this PR introduces and the files that are the most critical to review.

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

@erights erights changed the base branch from master to kriskowal-cancel January 2, 2026 07:35
@erights erights force-pushed the markm-alt-kriskowal-cancel branch 3 times, most recently from 650bf07 to c888749 Compare January 2, 2026 07:56
Comment on lines +93 to +98
// Propagate cancellation from parent if provided
if (parentCancelled) {
if (isKnownCancelled(parentCancelled)) {
knownCancelledSet.add(cancelled);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an orthogonal observable change from #3032 . I claim it is an improvement, in that if the parent is already known to be cancelled, then the child is born cancelled. You could adopt this observable change by itself, without the rest of the PR.

Btw, this observable change did not affect any tests.

@erights erights force-pushed the markm-alt-kriskowal-cancel branch from c888749 to 39d36fa Compare January 2, 2026 07:59
/**
* @type {WeakSet<Promise>}
*/
const knownCancelledSet = new WeakSet();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that isKnownCancelled's use of this piece of static mutable state does make it a communication channel. This SECURITY BUG seems inherent in this approach, which is why this is a failed experiment. See below.

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