From 42c1e2a3c0b0d562b2ebab2f2bed1ce95896992d Mon Sep 17 00:00:00 2001 From: meejah Date: Sat, 4 Jun 2022 02:48:35 -0600 Subject: [PATCH 1/4] Add ability to re-try a recovery (if it has previously failed) --- src/_zkapauthorizer/recover.py | 2 +- src/_zkapauthorizer/resource.py | 5 + .../tests/test_client_resource.py | 108 ++++++++++++++++++ 3 files changed, 114 insertions(+), 1 deletion(-) diff --git a/src/_zkapauthorizer/recover.py b/src/_zkapauthorizer/recover.py index 842eb269..971c4f60 100644 --- a/src/_zkapauthorizer/recover.py +++ b/src/_zkapauthorizer/recover.py @@ -119,7 +119,7 @@ async def recover( :param cursor: A database cursor which can be used to populate the database with recovered state. """ - if self._state.stage != RecoveryStages.inactive: + if self._state.stage not in {RecoveryStages.inactive, RecoveryStages.download_failed, RecoveryStages.import_failed}: return self._set_state(RecoveryState(stage=RecoveryStages.started)) diff --git a/src/_zkapauthorizer/resource.py b/src/_zkapauthorizer/resource.py index c956cf8b..5b7ad364 100644 --- a/src/_zkapauthorizer/resource.py +++ b/src/_zkapauthorizer/resource.py @@ -336,9 +336,14 @@ def err(f): ) ) disconnect_clients() + self.recovering_d = None def happy(_): + # note that the StatefulRecoverer eats download / + # import errors so we'll exit here sometimes even + # though an error message has been sent... disconnect_clients() + self.recovering_d = None self.recovering_d.addCallbacks(happy, err) diff --git a/src/_zkapauthorizer/tests/test_client_resource.py b/src/_zkapauthorizer/tests/test_client_resource.py index d5730fde..b2aed1a3 100644 --- a/src/_zkapauthorizer/tests/test_client_resource.py +++ b/src/_zkapauthorizer/tests/test_client_resource.py @@ -961,6 +961,114 @@ def create_proto(): ), ) + @given( + tahoe_configs(), + api_auth_tokens(), + ) + def test_recover_retry(self, get_config, api_auth_token): + """ + If at first our download fails, we can still retry using the API. + """ + downloads = [] + fails = [RuntimeError("downloader fails")] + + def get_sometimes_fail_downloader(cap): + async def do_download(set_state): + nonlocal downloads, fails + if fails: + raise fails.pop(0) + downloads.append(set_state) + return ( + # this data is CBOR for {"version": 1, "statements": []} + lambda: BytesIO(b"\xa2gversion\x01jstatements\x80"), + [], # no event-streams + ) + + return do_download + + clock = MemoryReactorClockResolver() + store = self.useFixture(TemporaryVoucherStore(aware_now, get_config)).store + recoverer = StatefulRecoverer() + factory = RecoverFactory(store, get_sometimes_fail_downloader, recoverer) + pumper = create_pumper() + self.addCleanup(pumper.stop) + + def create_proto(): + addr = IPv4Address("TCP", "127.0.0.1", "0") + proto = factory.buildProtocol(addr) + return proto + + agent = create_memory_agent(clock, pumper, create_proto) + pumper.start() + + async def recover(): + proto = await agent.open( + "ws://127.0.0.1:1/storage-plugins/privatestorageio-zkapauthz-v2/recover", + {"headers": {"Authorization": f"tahoe-lafs {api_auth_token}"}}, + ) + updates = [] + proto.on("message", lambda *args, **kw: updates.append((args, kw))) + await proto.is_open + proto.sendMessage( + json.dumps({"recovery-capability": self.GOOD_CAPABILITY}).encode("utf8") + ) + await proto.is_closed + return updates + + # first recovery will fails + d0 = Deferred.fromCoroutine(recover()) + pumper._flush() + + # try again + d1 = Deferred.fromCoroutine(recover()) + pumper._flush() + + self.assertThat( + d0, + succeeded( + AfterPreprocessing( + lambda messages: list( + loads(args[0]) for (args, kwargs) in messages + ), + Equals([ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "download_failed", + "failure-reason": "downloader fails", + }, + ]), + ) + ), + ) + # second attempt should have succeeded + self.assertThat( + d1, + succeeded( + AfterPreprocessing( + lambda messages: list( + loads(args[0]) for (args, kwargs) in messages + ), + Equals([ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "succeeded", + "failure-reason": None, + }, + ]), + ) + ), + ) + def maybe_extra_tokens(): """ From 2a732be92fadce69e32ea6280f5d0cef407072e3 Mon Sep 17 00:00:00 2001 From: meejah Date: Sat, 4 Jun 2022 02:51:16 -0600 Subject: [PATCH 2/4] darken --- src/_zkapauthorizer/recover.py | 6 ++- .../tests/test_client_resource.py | 52 ++++++++++--------- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/_zkapauthorizer/recover.py b/src/_zkapauthorizer/recover.py index 971c4f60..b045548f 100644 --- a/src/_zkapauthorizer/recover.py +++ b/src/_zkapauthorizer/recover.py @@ -119,7 +119,11 @@ async def recover( :param cursor: A database cursor which can be used to populate the database with recovered state. """ - if self._state.stage not in {RecoveryStages.inactive, RecoveryStages.download_failed, RecoveryStages.import_failed}: + if self._state.stage not in { + RecoveryStages.inactive, + RecoveryStages.download_failed, + RecoveryStages.import_failed, + }: return self._set_state(RecoveryState(stage=RecoveryStages.started)) diff --git a/src/_zkapauthorizer/tests/test_client_resource.py b/src/_zkapauthorizer/tests/test_client_resource.py index b2aed1a3..6e93465f 100644 --- a/src/_zkapauthorizer/tests/test_client_resource.py +++ b/src/_zkapauthorizer/tests/test_client_resource.py @@ -1030,18 +1030,20 @@ async def recover(): lambda messages: list( loads(args[0]) for (args, kwargs) in messages ), - Equals([ - { - "stage": "started", - "failure-reason": None, - }, - # "our" downloader (above) doesn't set any downloading etc - # state-updates - { - "stage": "download_failed", - "failure-reason": "downloader fails", - }, - ]), + Equals( + [ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "download_failed", + "failure-reason": "downloader fails", + }, + ] + ), ) ), ) @@ -1053,18 +1055,20 @@ async def recover(): lambda messages: list( loads(args[0]) for (args, kwargs) in messages ), - Equals([ - { - "stage": "started", - "failure-reason": None, - }, - # "our" downloader (above) doesn't set any downloading etc - # state-updates - { - "stage": "succeeded", - "failure-reason": None, - }, - ]), + Equals( + [ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "succeeded", + "failure-reason": None, + }, + ] + ), ) ), ) From c43cfdb7db9e268df8af17c6439d0326a31329b1 Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 8 Jun 2022 16:47:23 -0600 Subject: [PATCH 3/4] retry recovery test --- .../tests/test_client_resource.py | 107 ++++++++---------- 1 file changed, 46 insertions(+), 61 deletions(-) diff --git a/src/_zkapauthorizer/tests/test_client_resource.py b/src/_zkapauthorizer/tests/test_client_resource.py index 6e93465f..7c669c13 100644 --- a/src/_zkapauthorizer/tests/test_client_resource.py +++ b/src/_zkapauthorizer/tests/test_client_resource.py @@ -979,8 +979,7 @@ async def do_download(set_state): raise fails.pop(0) downloads.append(set_state) return ( - # this data is CBOR for {"version": 1, "statements": []} - lambda: BytesIO(b"\xa2gversion\x01jstatements\x80"), + lambda: BytesIO(statements_to_snapshot([])), [], # no event-streams ) @@ -988,8 +987,7 @@ async def do_download(set_state): clock = MemoryReactorClockResolver() store = self.useFixture(TemporaryVoucherStore(aware_now, get_config)).store - recoverer = StatefulRecoverer() - factory = RecoverFactory(store, get_sometimes_fail_downloader, recoverer) + factory = RecoverFactory(store, get_sometimes_fail_downloader) pumper = create_pumper() self.addCleanup(pumper.stop) @@ -1001,76 +999,63 @@ def create_proto(): agent = create_memory_agent(clock, pumper, create_proto) pumper.start() - async def recover(): - proto = await agent.open( - "ws://127.0.0.1:1/storage-plugins/privatestorageio-zkapauthz-v2/recover", - {"headers": {"Authorization": f"tahoe-lafs {api_auth_token}"}}, - ) - updates = [] - proto.on("message", lambda *args, **kw: updates.append((args, kw))) - await proto.is_open - proto.sendMessage( - json.dumps({"recovery-capability": self.GOOD_CAPABILITY}).encode("utf8") - ) - await proto.is_closed - return updates - - # first recovery will fails - d0 = Deferred.fromCoroutine(recover()) + # first recovery will fail + d0 = Deferred.fromCoroutine(recover( + agent, + DecodedURL.from_text("ws://127.0.0.1:1/"), + api_auth_token, + self.GOOD_CAPABILITY, + )) pumper._flush() - # try again - d1 = Deferred.fromCoroutine(recover()) + # try to recover again (this one should work, as we only fail + # once in the test-provided downloader) + d1 = Deferred.fromCoroutine(recover( + agent, + DecodedURL.from_text("ws://127.0.0.1:1/"), + api_auth_token, + self.GOOD_CAPABILITY, + )) pumper._flush() self.assertThat( d0, succeeded( - AfterPreprocessing( - lambda messages: list( - loads(args[0]) for (args, kwargs) in messages - ), - Equals( - [ - { - "stage": "started", - "failure-reason": None, - }, - # "our" downloader (above) doesn't set any downloading etc - # state-updates - { - "stage": "download_failed", - "failure-reason": "downloader fails", - }, - ] - ), + Equals( + [ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "download_failed", + "failure-reason": "downloader fails", + }, + ] ) - ), + ) ) - # second attempt should have succeeded + # second attempt should succeed self.assertThat( d1, succeeded( - AfterPreprocessing( - lambda messages: list( - loads(args[0]) for (args, kwargs) in messages - ), - Equals( - [ - { - "stage": "started", - "failure-reason": None, - }, - # "our" downloader (above) doesn't set any downloading etc - # state-updates - { - "stage": "succeeded", - "failure-reason": None, - }, - ] - ), + Equals( + [ + { + "stage": "started", + "failure-reason": None, + }, + # "our" downloader (above) doesn't set any downloading etc + # state-updates + { + "stage": "succeeded", + "failure-reason": None, + }, + ] ) - ), + ) ) From 3edb65eee4ebe3c653df632f787347fc13f4fe6f Mon Sep 17 00:00:00 2001 From: meejah Date: Wed, 8 Jun 2022 16:47:52 -0600 Subject: [PATCH 4/4] darken --- .../tests/test_client_resource.py | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/_zkapauthorizer/tests/test_client_resource.py b/src/_zkapauthorizer/tests/test_client_resource.py index 7c669c13..2918db65 100644 --- a/src/_zkapauthorizer/tests/test_client_resource.py +++ b/src/_zkapauthorizer/tests/test_client_resource.py @@ -1000,22 +1000,26 @@ def create_proto(): pumper.start() # first recovery will fail - d0 = Deferred.fromCoroutine(recover( - agent, - DecodedURL.from_text("ws://127.0.0.1:1/"), - api_auth_token, - self.GOOD_CAPABILITY, - )) + d0 = Deferred.fromCoroutine( + recover( + agent, + DecodedURL.from_text("ws://127.0.0.1:1/"), + api_auth_token, + self.GOOD_CAPABILITY, + ) + ) pumper._flush() # try to recover again (this one should work, as we only fail # once in the test-provided downloader) - d1 = Deferred.fromCoroutine(recover( - agent, - DecodedURL.from_text("ws://127.0.0.1:1/"), - api_auth_token, - self.GOOD_CAPABILITY, - )) + d1 = Deferred.fromCoroutine( + recover( + agent, + DecodedURL.from_text("ws://127.0.0.1:1/"), + api_auth_token, + self.GOOD_CAPABILITY, + ) + ) pumper._flush() self.assertThat( @@ -1035,7 +1039,7 @@ def create_proto(): }, ] ) - ) + ), ) # second attempt should succeed self.assertThat( @@ -1055,7 +1059,7 @@ def create_proto(): }, ] ) - ) + ), )