Skip to content

fix: prevent stale activeFlowConfig reference in type-registered handler#5458

Open
Dennis-SEG wants to merge 1 commit intonode-red:masterfrom
Dennis-SEG:fix/flows-index-race-condition
Open

fix: prevent stale activeFlowConfig reference in type-registered handler#5458
Dennis-SEG wants to merge 1 commit intonode-red:masterfrom
Dennis-SEG:fix/flows-index-race-condition

Conversation

@Dennis-SEG
Copy link
Copy Markdown
Contributor

Summary

Fixes #5454 - The type-registered event handler could operate on a stale activeFlowConfig reference.

Changes

Cache the activeFlowConfig reference at handler start and verify it hasn't changed before calling start().

Test Plan

  • All flow tests pass
  • Manual testing with rapid type registration

Cache activeFlowConfig reference at handler start to prevent issues if
activeFlowConfig is reassigned between indexOf and splice operations.

Also check that config is still active before calling start() to avoid
starting flows for an outdated configuration.

Fixes: #6
Copy link
Copy Markdown
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

Not sure this is actually fixing the issue you've described. You have added line 58 which sets config to the current value of activeFlowConfig. But that's inside the callback handler, so I don't think it has any difference in behaviour to the existing code that references activeFlowConfig directly.

Have you seen an instance of this issue happen? Or is this fixed based on code-review only?

@Dennis-SEG
Copy link
Copy Markdown
Contributor Author

You're right. I traced through the code paths and activeFlowConfig can only be modified in setFlows() inside a promise .then() callback. Since promise callbacks don't interrupt synchronous code, and there's no synchronous path from this handler that could trigger setFlows(), the check is unnecessary.

The events.emit("runtime-event",...) only triggers handleRuntimeEvent in comms.js which just logs and publishes to clients - it doesn't modify any flow state.

This was based on code review only, not an observed issue. Happy to close this PR if you prefer, or keep it as a minor defensive measure - your call.

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.

Race condition in flows/index.js - stale activeFlowConfig reference

2 participants