fix: Detect potential loops, don't eat errors #230
Conversation
| @@ -54,6 +54,7 @@ def purge_old_records(config_file, grace_period=-1, max_per_loop=10, | |||
| try: | |||
| backend = config.registry.getUtility(INodeAssignment) | |||
| patterns = config.registry['endpoints_patterns'] | |||
| previous_list = [] | |||
There was a problem hiding this comment.
This should be per service I believe (2 lines down)?
I'd argue this loop detection isn't really necessary after the last fix.
There was a problem hiding this comment.
I was debating moving it down, but then realized that we're detecting if things are the same. So any change would be fine so long as it didn't match the prior state. (And if we're doing things right, we're removing stuff along the way.)
If anything, loop detection should be nerf'd for "dry_run".
There was a problem hiding this comment.
hrm, there's no way to provide an offset to the backend.get_old_user_records, so dryrun will always return the same list. nerf'ing isn't possible.
There was a problem hiding this comment.
I agree it's not likely to trigger a loop detected when iterating between services which will have different sets of users, but it also seems confusing to me to reuse the same state across them like that.
|
Huh, but you did get me thinking about two services that might have empty results, which would raise a false exception. Thanks! |
| if not rows: | ||
| logger.info("No more data for %s", service) | ||
| break | ||
| if [service].extend(rows) == previous_list: |
There was a problem hiding this comment.
What's [service] doing? extend is going to add on inplace, always returning None.
service here is a value on the user record but it's more of a foreign key to the services table. I don't think we actually use it, it's legacy from when tokenserver was built to support multiple services.
At one point we had a 'sync-1.1' and 'sync-1.5' protocols to support but I don't think tokenserver even used this feature to support that. Long story short, consider service to be unrelated, separated buckets of users on potentially different services.
There was a problem hiding this comment.
If we have more than one service, there's the chance that we'd fail out early if a and b had no entries because [] == []. I just prefix the list with the service name so that we detect looping for a given service.
I don't expect it to trigger with what we're doing, but it's not a bad idea to handle it in any case, because I don't know who else might use this.
simple loop detection for purge_old_records.py