Refuse a pending dataset write while one is in place - #275
Merged
Conversation
set_pending_dataset_tlvs() refuses a write while a pending dataset is in flight, but create_pending_dataset() -- the other way to write that same endpoint, and the one set_channel() uses -- did not. Superseding an in-flight dataset races the delay timer on every device that already holds it, so a late replacement can split the mesh, and it silently undoes whatever the in-flight dataset was doing, such as a channel change. That is a property of the endpoint, not of the encoding the caller happens to use. Move the guard into create_pending_dataset(), where both a local check and the If-None-Match header the border router evaluates atomically with the write (openthread/ot-br-posix#3552, which covers the endpoint rather than the content type, so the JSON path gets it too) now apply. It raises PendingDatasetConflictError, as the TLV write does. set_channel() drops its own check, which would otherwise read the pending dataset twice per channel change. It keeps refusing, with the same request count as before; what changes is that the active dataset is read first, so a router with no active dataset reports that rather than the conflict, and the error is now the PendingDatasetConflictError subclass instead of a plain OTBRError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
@LorbusChris can you have a look at this PR? It is essentially a follow up to your #269 and #272 PRs. I intend to release a new version after that. |
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed changes are well-covered with no unresolved issues.
Pull request overview
Prevents unsafe replacement of in-flight pending datasets.
Changes:
- Adds local and atomic conflict checks.
- Raises
PendingDatasetConflictErroron conflicts. - Centralizes channel-change protection.
File summaries
| File | Description |
|---|---|
tests/test_init_legacy.py |
Updates request expectations and conflict coverage. |
python_otbr_api/__init__.py |
Implements conflict-safe pending-dataset creation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #269, which added the in-flight refusal to
set_pending_dataset_tlvs().create_pending_dataset()writes the same endpoint — and is whatset_channel()goes through — but had no such guard.Superseding an in-flight pending dataset races the delay timer on every device that already holds it, so a late replacement can split the mesh, and it silently undoes whatever the in-flight dataset was doing, such as a channel change. That is a property of the endpoint, not of the encoding the caller happens to use, so the guard belongs on both writes.
create_pending_dataset()now does the local check and sendsIf-None-Match: *, raisingPendingDatasetConflictErrorfor both the local refusal and a 412. openthread/ot-br-posix#3552 put the precondition inSetDataset(), which is shared by both endpoints and both content types, so the JSON path gets the atomic version for free.set_channel()drops its own check from #272, which would otherwise read the pending dataset twice per channel change.Behavior changes
create_pending_dataset()refuses while a pending dataset is in place; it previously replaced it. This is the point of the PR, but it is a change for existing callers.set_channel()makes the same three requests as before, but reads the active dataset first. A router with no active dataset now reports that rather than the pending conflict.PendingDatasetConflictErrorinstead of a plainOTBRError. It subclassesOTBRError, so existingexcept OTBRErrorhandlers are unaffected.No version bump — that follows the usual separate bump PR.
🤖 Generated with Claude Code