From e379dec97c9ca75385ebbd9730d43c23e6f96994 Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Thu, 3 Sep 2026 16:16:41 +0200 Subject: [PATCH] Refuse a pending dataset write while one is in place 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 --- python_otbr_api/__init__.py | 32 +++++++++++++---- tests/test_init_legacy.py | 69 ++++++++++++++++++++++++++++++------- 2 files changed, 81 insertions(+), 20 deletions(-) diff --git a/python_otbr_api/__init__.py b/python_otbr_api/__init__.py index 74f09d5..adde6ee 100644 --- a/python_otbr_api/__init__.py +++ b/python_otbr_api/__init__.py @@ -336,17 +336,36 @@ async def create_pending_dataset(self, dataset: PendingDataSet) -> None: The passed in PendingDataSet does not need to be fully populated, any fields not set will be automatically set by the open thread border router. - Raises if the http status is 400 or higher or if the response is invalid. + + A write while a pending dataset is in flight is refused outright, for the + same reasons set_pending_dataset_tlvs refuses it: superseding one races + the delay timer on every device that already holds the old dataset, so a + late replacement can split the mesh, and it would also silently undo + whatever the in-flight dataset was doing, such as a channel change. + + The check runs locally first, and again on the border router for one + that honors If-None-Match on this endpoint + (https://github.com/openthread/ot-br-posix/pull/3552), where it is + atomic with the write; older border routers ignore the header. + + Raises PendingDatasetConflictError when either check refuses the write, + and OTBRError if the http status is 400 or higher for any other reason + or the response is invalid. """ + if await self.get_pending_dataset_tlvs() is not None: + raise PendingDatasetConflictError("a pending dataset is already in place") await self._maybe_detect_key_format() response = await self._session.put( f"{self._url}/node/dataset/pending", json=self._encode(dataset.as_json()), + headers={"If-None-Match": "*"}, timeout=aiohttp.ClientTimeout(total=self._timeout), ) if response.status == HTTPStatus.CONFLICT: raise ThreadNetworkActiveError + if response.status == HTTPStatus.PRECONDITION_FAILED: + raise PendingDatasetConflictError("a pending dataset is already in place") if response.status not in (HTTPStatus.CREATED, HTTPStatus.OK): raise OTBRError(f"unexpected http status {response.status}") @@ -424,15 +443,14 @@ async def set_channel( """Change the channel The channel is changed by creating a new pending dataset based on the active - dataset. If a pending dataset is already in place, the change is refused: - stamping from the active dataset alone would make the mesh silently ignore - it while the router accepts the write, and superseding the pending dataset - would race its delay timer on devices that miss the update. + dataset. If a pending dataset is already in place, create_pending_dataset + refuses the write and raises PendingDatasetConflictError: stamping from the + active dataset alone would make the mesh silently ignore it while the router + accepts the write, and superseding the pending dataset would race its delay + timer on devices that miss the update. """ if not 11 <= channel <= 26: raise OTBRError(f"invalid channel {channel}") - if await self.get_pending_dataset_tlvs() is not None: - raise OTBRError("a pending dataset is already in place") if not (dataset := await self.get_active_dataset()): raise OTBRError("router has no active dataset") diff --git a/tests/test_init_legacy.py b/tests/test_init_legacy.py index 75429b5..d2bee2b 100644 --- a/tests/test_init_legacy.py +++ b/tests/test_init_legacy.py @@ -310,13 +310,15 @@ async def test_create_pending_dataset(aioclient_mock: AiohttpClientMocker): BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE ) + aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT) aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.CREATED) await otbr.create_pending_dataset(python_otbr_api.PendingDataSet()) - assert aioclient_mock.call_count == 1 + assert aioclient_mock.call_count == 2 assert aioclient_mock.mock_calls[-1][0] == "PUT" assert aioclient_mock.mock_calls[-1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[-1][2] == {} + assert aioclient_mock.mock_calls[-1][3]["If-None-Match"] == "*" await otbr.create_pending_dataset( python_otbr_api.PendingDataSet( @@ -325,7 +327,7 @@ async def test_create_pending_dataset(aioclient_mock: AiohttpClientMocker): python_otbr_api.Timestamp(), ) ) - assert aioclient_mock.call_count == 2 + assert aioclient_mock.call_count == 4 assert aioclient_mock.mock_calls[-1][0] == "PUT" assert aioclient_mock.mock_calls[-1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[-1][2] == { @@ -342,7 +344,7 @@ async def test_create_pending_dataset(aioclient_mock: AiohttpClientMocker): 23456, ) ) - assert aioclient_mock.call_count == 3 + assert aioclient_mock.call_count == 6 assert aioclient_mock.mock_calls[-1][0] == "PUT" assert aioclient_mock.mock_calls[-1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[-1][2] == { @@ -393,9 +395,9 @@ async def test_set_channel(aioclient_mock: AiohttpClientMocker) -> None: await otbr.set_channel(new_channel, 1234) assert aioclient_mock.call_count == 3 assert aioclient_mock.mock_calls[0][0] == "GET" - assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/pending" + assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/active" assert aioclient_mock.mock_calls[1][0] == "GET" - assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/active" + assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][0] == "PUT" assert aioclient_mock.mock_calls[2][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][2] == expected_pending_dataset @@ -425,9 +427,9 @@ async def test_set_channel_default_delay(aioclient_mock: AiohttpClientMocker) -> await otbr.set_channel(new_channel) assert aioclient_mock.call_count == 3 assert aioclient_mock.mock_calls[0][0] == "GET" - assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/pending" + assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/active" assert aioclient_mock.mock_calls[1][0] == "GET" - assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/active" + assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][0] == "PUT" assert aioclient_mock.mock_calls[2][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][2] == expected_pending_dataset @@ -460,9 +462,9 @@ async def test_set_channel_no_timestamp(aioclient_mock: AiohttpClientMocker) -> await otbr.set_channel(new_channel) assert aioclient_mock.call_count == 3 assert aioclient_mock.mock_calls[0][0] == "GET" - assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/pending" + assert aioclient_mock.mock_calls[0][1].path == "/node/dataset/active" assert aioclient_mock.mock_calls[1][0] == "GET" - assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/active" + assert aioclient_mock.mock_calls[1][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][0] == "PUT" assert aioclient_mock.mock_calls[2][1].path == "/node/dataset/pending" assert aioclient_mock.mock_calls[2][2] == expected_pending_dataset @@ -484,7 +486,6 @@ async def test_set_channel_no_dataset(aioclient_mock: AiohttpClientMocker) -> No BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE ) - aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT) aioclient_mock.get(f"{BASE_URL}/node/dataset/active", status=HTTPStatus.NO_CONTENT) with pytest.raises(python_otbr_api.OTBRError): @@ -498,7 +499,8 @@ async def test_set_channel_rejected_while_pending( Stamping from the active dataset alone would make the mesh silently ignore the write, and superseding the pending dataset would race its delay timer - on devices that miss the update; refusing is the only honest answer. + on devices that miss the update; refusing is the only honest answer. The + refusal comes from create_pending_dataset, so nothing is written. """ otbr = python_otbr_api.OTBR( BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE @@ -509,11 +511,13 @@ async def test_set_channel_rejected_while_pending( "0708FD17C9D59809B27A05107546326F20BCCFD946609FBAF7F39AD5030F4F70656E5468726561" "642D32366363010226CC0410FA7EC34EBE58DD1FD74F13F65D021C5B0C0402A0F7F8" ) + aioclient_mock.get(f"{BASE_URL}/node/dataset/active", json=DATASET_JSON) aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", text=mock_response) - with pytest.raises(python_otbr_api.OTBRError): + with pytest.raises(python_otbr_api.PendingDatasetConflictError): await otbr.set_channel(16) - assert aioclient_mock.call_count == 1 + assert not [call for call in aioclient_mock.mock_calls if call[0] == "PUT"] + assert aioclient_mock.call_count == 2 async def test_get_extended_address(aioclient_mock: AiohttpClientMocker) -> None: @@ -681,6 +685,7 @@ async def test_create_pending_dataset_thread_active( BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE ) + aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT) aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.CONFLICT) with pytest.raises(python_otbr_api.ThreadNetworkActiveError): @@ -693,6 +698,7 @@ async def test_create_pending_dataset_202(aioclient_mock: AiohttpClientMocker): BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE ) + aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT) aioclient_mock.put(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.ACCEPTED) with pytest.raises(python_otbr_api.OTBRError): @@ -877,3 +883,40 @@ async def test_set_pending_dataset_tlvs_202( with pytest.raises(python_otbr_api.OTBRError): await otbr.set_pending_dataset_tlvs(b"") + + +async def test_create_pending_dataset_refused_while_pending( + aioclient_mock: AiohttpClientMocker, +) -> None: + """Test a write is refused while a pending dataset is in place.""" + otbr = python_otbr_api.OTBR( + BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE + ) + + in_flight = "0E080000000000010000340400006699000300000C" + aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", text=in_flight) + + with pytest.raises(python_otbr_api.PendingDatasetConflictError): + await otbr.create_pending_dataset(python_otbr_api.PendingDataSet()) + assert aioclient_mock.call_count == 1 + + +async def test_create_pending_dataset_refused_by_router( + aioclient_mock: AiohttpClientMocker, +) -> None: + """Test the border router refusing the precondition is surfaced. + + A pending dataset created between the local check and the write is only + caught by the router, which evaluates If-None-Match atomically with it. + """ + otbr = python_otbr_api.OTBR( + BASE_URL, aioclient_mock.create_session(), key_format=KeyFormat.PASCAL_CASE + ) + + aioclient_mock.get(f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.NO_CONTENT) + aioclient_mock.put( + f"{BASE_URL}/node/dataset/pending", status=HTTPStatus.PRECONDITION_FAILED + ) + + with pytest.raises(python_otbr_api.PendingDatasetConflictError): + await otbr.create_pending_dataset(python_otbr_api.PendingDataSet())