From 22ef38fc3f43071623d1ff4439a24d13fa386b90 Mon Sep 17 00:00:00 2001 From: Lai Jiang Date: Thu, 17 Nov 2022 12:04:34 -0500 Subject: [PATCH] Fix missing autorenew onetime billing events (#1854) This PR fixes the issue where the onetime billing event for an autorenew is not correctly created if the recurrence of the autorenew is closed during the autorenew grace period, such as the case if a manual renew happens during the same grace period. The detailed analysis of the issue is captured in b/258822640. Note that this is a quick and dirty fix to make ongoing billing event expanse work correctly in the future. It does not fix the missing events in the past, nor can it be used to reconstruct the missing ones (by providing a different cursor time), due to timeout when triggering the action from nomulus curl. Per Weimin, the recurrences that fits the new condition along, based on the current cursor, would increase from 382k to 430k, a 12% increase. I checked last nights cron job run, which starts on 22:00 EST and seemed to finish at 22:15 EST (when the last log for this request was recorded), so it should definitely still finish in time for the nightly runs with the new condition. --- .../ExpandRecurringBillingEventsAction.java | 19 ++++++++- ...xpandRecurringBillingEventsActionTest.java | 39 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/google/registry/batch/ExpandRecurringBillingEventsAction.java b/core/src/main/java/google/registry/batch/ExpandRecurringBillingEventsAction.java index 34c1f2845..e1f963e4a 100644 --- a/core/src/main/java/google/registry/batch/ExpandRecurringBillingEventsAction.java +++ b/core/src/main/java/google/registry/batch/ExpandRecurringBillingEventsAction.java @@ -131,12 +131,14 @@ public class ExpandRecurringBillingEventsAction implements Runnable { + "WHERE eventTime <= :executeTime " + "AND eventTime < recurrenceEndTime " + "AND id > :maxProcessedRecurrenceId " - + "AND recurrenceEndTime > :cursorTime " + + "AND recurrenceEndTime > :adjustedCursorTime " + "ORDER BY id ASC", Recurring.class) .setParameter("executeTime", executeTime) .setParameter("maxProcessedRecurrenceId", prevMaxProcessedRecurrenceId) - .setParameter("cursorTime", cursorTime) + .setParameter( + "adjustedCursorTime", + cursorTime.minus(Registry.DEFAULT_AUTO_RENEW_GRACE_PERIOD)) .setMaxResults(batchSize) .getResultList(); for (Recurring recurring : recurrings) { @@ -279,6 +281,19 @@ public class ExpandRecurringBillingEventsAction implements Runnable { .setReason("Domain autorenewal by ExpandRecurringBillingEventsAction") .setRequestedByRegistrar(false) .setType(DOMAIN_AUTORENEW) + // Note: the following statement seems to not be entirely correct as manual renewal + // during the autorenew grace period also closes out the existing recurrence, but in + // that instance the autorenew history entry should still have the transaction records + // for obvious reasons. It can be argued the history entry should always have the + // transaction record, regardless of what happens afterward. If the domain is deleted + // later during the autorenew grace period, another history entry for the delete would + // record that mutation separately, but the previous autorenew should not have its + // history entry retroactively altered, or in this case have the transaction records + // omitted when its created belatedly (when billing time is in scope). However, since + // we will be rewriting this action and only want to do the absolute minimum change to + // fix it for now, we will leave the current logic in place to avoid any unnecessary + // complications. + // // Don't write a domain transaction record if the recurrence was // ended prior to the billing time (i.e. a domain was deleted // during the autorenew grace period). diff --git a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java index 10a12f919..170ae191f 100644 --- a/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java +++ b/core/src/test/java/google/registry/batch/ExpandRecurringBillingEventsActionTest.java @@ -455,6 +455,45 @@ public class ExpandRecurringBillingEventsActionTest { assertCursorAt(currentTestTime); } + @Test + void testSuccess_noEventExpanded_recurrenceEndAfterEvent_cursorTimeTooLate() throws Exception { + // This can occur when a domain is transferred/renewed/deleted during autorenew grace peroid. + recurring = + persistResource( + recurring + .asBuilder() + .setRecurrenceEndTime(recurring.getEventTime().plusDays(5)) + .build()); + action.cursorTimeParam = Optional.of(recurring.getRecurrenceEndTime().plusDays(46)); + runAction(); + // No new history entries should be generated because cursor time is more than 45 days after + // recurrence end time. + assertThat(getHistoryEntriesOfType(domain, DOMAIN_AUTORENEW)).isEmpty(); + assertBillingEventsForResource(domain, recurring); + assertCursorAt(currentTestTime); + } + + @Test + void testSuccess_expandSingleEvent_recurrenceEndAfterEvent() throws Exception { + // This can occur when a domain is transferred/renewed/deleted during autorenew grace peroid. + recurring = + persistResource( + recurring + .asBuilder() + .setRecurrenceEndTime(recurring.getEventTime().plusDays(5)) + .build()); + action.cursorTimeParam = Optional.of(recurring.getRecurrenceEndTime().plusDays(35)); + runAction(); + DomainHistory persistedEntry = + getOnlyHistoryEntryOfType(domain, DOMAIN_AUTORENEW, DomainHistory.class); + assertHistoryEntryMatches( + domain, persistedEntry, "TheRegistrar", DateTime.parse("2000-02-19T00:00:00Z"), false); + BillingEvent.OneTime expected = + defaultOneTimeBuilder().setDomainHistory(persistedEntry).build(); + assertBillingEventsForResource(domain, expected, recurring); + assertCursorAt(currentTestTime); + } + @Test void testSuccess_expandSingleEvent_dryRun() throws Exception { persistResource(recurring);