Skip to content

Commit 818c4cb

Browse files
committed
fix(digest): honor per-user cron schedules on the digest tick
Product claimed “your own schedule” while SlackDailyDigestTaskConfig only ran the global slack.daily-digest.cron. Switch to a minute tick that evaluates each enabled UserDigestPreference.cronExpression in that user’s timezone (Spring CronExpression), skips already-logged fire windows, and keeps legacy singleton broadcast when no enabled prefs exist.
1 parent e8e6765 commit 818c4cb

9 files changed

Lines changed: 665 additions & 45 deletions

File tree

backend/src/main/java/com/dbaagent/model/UserDigestPreference.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ public class UserDigestPreference {
103103

104104
/**
105105
* Optional timezone for schedule interpretation.
106-
* When null, uses system default (typically UTC).
106+
* When null or invalid, the digest tick evaluates the cron in UTC.
107107
* Format: IANA timezone ID (e.g., "America/New_York", "Europe/London").
108108
*/
109109
@Column(name = "timezone", length = 64)

backend/src/main/java/com/dbaagent/repository/SlackDigestLogRepository.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,4 +97,23 @@ SELECT DISTINCT ON (recipient_username) *
9797
* Used for determining the window start for new digests.
9898
*/
9999
Optional<SlackDigestLog> findTopByConnectionIdOrderBySentAtDesc(String connectionId);
100+
101+
/**
102+
* Idempotency for per-preference scheduling: true if this preference already
103+
* produced a digest log for the connection at or after the cron fire time.
104+
*/
105+
boolean existsByPreferenceIdAndConnectionIdAndSentAtGreaterThanEqual(
106+
Long preferenceId,
107+
String connectionId,
108+
LocalDateTime sentAt
109+
);
110+
111+
/**
112+
* Fallback idempotency when preferenceId is missing on older rows.
113+
*/
114+
boolean existsByConnectionIdAndRecipientUsernameAndSentAtGreaterThanEqual(
115+
String connectionId,
116+
String recipientUsername,
117+
LocalDateTime sentAt
118+
);
100119
}

backend/src/main/java/com/dbaagent/service/SlackDailyDigestService.java

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import com.dbaagent.repository.SlackChannelBindingRepository;
4747
import com.dbaagent.repository.SlackDigestLogRepository;
4848
import com.dbaagent.repository.TableStatsHistoryRepository;
49+
import com.dbaagent.service.digest.DigestCronMatcher;
4950
import com.dbaagent.service.digest.DigestInsightAssemblerService;
5051
import com.slack.api.Slack;
5152
import com.slack.api.methods.MethodsClient;
@@ -60,6 +61,9 @@
6061

6162
import java.io.IOException;
6263
import java.time.LocalDateTime;
64+
import java.time.ZoneId;
65+
import java.time.Instant;
66+
import java.time.Duration;
6367
import java.time.format.DateTimeFormatter;
6468
import java.util.ArrayList;
6569
import java.util.Comparator;
@@ -89,6 +93,14 @@ public class SlackDailyDigestService {
8993
@Value("${slack.digest.admins-only:true}")
9094
private boolean digestAdminsOnly;
9195

96+
/**
97+
* Global / legacy digest schedule. Also the default when a preference leaves
98+
* {@code cronExpression} blank. Interpreted in UTC for legacy; per-user prefs
99+
* use their own timezone.
100+
*/
101+
@Value("${slack.daily-digest.cron:0 0 9 * * *}")
102+
private String globalDigestCron;
103+
92104
private final SlackRuntimeSettingsService slackRuntimeSettingsService;
93105
private final SlackChannelBindingRepository channelBindingRepository;
94106
private final CredentialService credentialService;
@@ -828,9 +840,149 @@ private LocalDateTime getLastDigestTime(String connectionId) {
828840
.orElse(LocalDateTime.now().minusHours(24));
829841
}
830842

843+
private static final Duration DIGEST_TICK_LOOKBACK = Duration.ofMinutes(1);
844+
845+
/**
846+
* Minute-tick entry point used by {@code SlackDailyDigestTaskConfig}.
847+
*
848+
* <p>When no enabled preferences exist, runs the legacy channel broadcast
849+
* only if the global {@code slack.daily-digest.cron} is due (UTC).
850+
* When preferences exist, delivers only to preferences whose cron matches
851+
* in that user's timezone and that have not already been logged for this
852+
* fire window.
853+
*/
854+
public void processDigestTick() {
855+
processDigestTick(Instant.now());
856+
}
857+
858+
/**
859+
* Testable overload of {@link #processDigestTick()}.
860+
*/
861+
public void processDigestTick(Instant now) {
862+
if (now == null) {
863+
now = Instant.now();
864+
}
865+
866+
long enabledCount = userDigestPreferenceRepository.countByEnabledTrue();
867+
if (enabledCount == 0) {
868+
if (DigestCronMatcher.isDue(globalDigestCron, ZoneId.of("UTC"), now, DIGEST_TICK_LOOKBACK)) {
869+
log.info("Digest tick: no enabled preferences; running legacy broadcast (global cron due)");
870+
runLegacyBroadcastForAllConnections();
871+
} else {
872+
log.debug("Digest tick: no enabled preferences; global cron not due");
873+
}
874+
return;
875+
}
876+
877+
String globalCron = (globalDigestCron == null || globalDigestCron.isBlank())
878+
? "0 0 9 * * *"
879+
: globalDigestCron;
880+
881+
List<UserDigestPreference> enabledPrefs = userDigestPreferenceRepository
882+
.findByEnabledTrueAndDeliveryMethod(DigestDeliveryMethod.SLACK_DM);
883+
884+
int dueCount = 0;
885+
int delivered = 0;
886+
for (UserDigestPreference pref : enabledPrefs) {
887+
String cron = pref.getEffectiveCronExpression(globalCron);
888+
ZoneId zone = DigestCronMatcher.resolveZone(pref.getTimezone());
889+
var window = DigestCronMatcher.dueWindowStart(cron, zone, now, DIGEST_TICK_LOOKBACK);
890+
if (window.isEmpty()) {
891+
continue;
892+
}
893+
dueCount++;
894+
Instant fireInstant = window.get();
895+
List<String> connectionIds = resolvePreferenceConnections(pref);
896+
for (String connectionId : connectionIds) {
897+
if (alreadyDeliveredForWindow(pref, connectionId, fireInstant)) {
898+
log.debug("Skipping digest for {} / {} — already delivered this window",
899+
pref.getUsername(), connectionId);
900+
continue;
901+
}
902+
try {
903+
boolean slackEnabled = isSlackDeliveryEnabled();
904+
String connName = connectionName(connectionId);
905+
LocalDateTime since = getLastDigestTime(connectionId);
906+
PersonalizedDigestResult result = sendPersonalizedDigestToUser(
907+
connectionId, connName, pref, since, slackEnabled);
908+
if (result.generated() || result.sent()) {
909+
delivered++;
910+
}
911+
} catch (Exception e) {
912+
log.error("Failed due-preference digest for user {} connection {}: {}",
913+
pref.getUsername(), connectionId, e.getMessage(), e);
914+
}
915+
}
916+
}
917+
918+
log.info("Digest tick (per-user): {} due preference(s), {} delivery attempt(s)",
919+
dueCount, delivered);
920+
921+
// Connections with no recipients still get legacy broadcast when the global cron fires.
922+
if (DigestCronMatcher.isDue(globalCron, ZoneId.of("UTC"), now, DIGEST_TICK_LOOKBACK)) {
923+
for (String connectionId : digestConnectionIds()) {
924+
if (userDigestPreferenceRepository.findEnabledForConnection(connectionId).isEmpty()) {
925+
try {
926+
sendLegacyDigest(connectionId);
927+
} catch (Exception e) {
928+
log.error("Legacy fallback digest failed for {}: {}", connectionId, e.getMessage(), e);
929+
}
930+
}
931+
}
932+
}
933+
}
934+
935+
private void runLegacyBroadcastForAllConnections() {
936+
List<String> connectionIds = digestConnectionIds();
937+
if (connectionIds.isEmpty()) {
938+
log.info("No connections available — skipping legacy digest");
939+
return;
940+
}
941+
for (String connectionId : connectionIds) {
942+
try {
943+
sendLegacyDigest(connectionId);
944+
} catch (Exception e) {
945+
log.error("Failed legacy digest for connection {}: {}", connectionId, e.getMessage(), e);
946+
}
947+
}
948+
}
949+
950+
private List<String> resolvePreferenceConnections(UserDigestPreference pref) {
951+
if (pref.getConnectionId() != null && !pref.getConnectionId().isBlank()) {
952+
return List.of(pref.getConnectionId());
953+
}
954+
return digestConnectionIds();
955+
}
956+
957+
/**
958+
* True when SlackDigestLog already has a row for this preference/connection
959+
* at or after the cron fire time (idempotent minute-tick).
960+
*/
961+
private boolean alreadyDeliveredForWindow(
962+
UserDigestPreference pref,
963+
String connectionId,
964+
Instant fireInstant) {
965+
// sentAt is LocalDateTime.now() (JVM default zone) — compare in that zone.
966+
LocalDateTime since = LocalDateTime.ofInstant(fireInstant, ZoneId.systemDefault());
967+
if (pref.getId() != null) {
968+
if (digestLogRepository.existsByPreferenceIdAndConnectionIdAndSentAtGreaterThanEqual(
969+
pref.getId(), connectionId, since)) {
970+
return true;
971+
}
972+
}
973+
if (pref.getUsername() != null && !pref.getUsername().isBlank()) {
974+
return digestLogRepository.existsByConnectionIdAndRecipientUsernameAndSentAtGreaterThanEqual(
975+
connectionId, pref.getUsername(), since);
976+
}
977+
return false;
978+
}
979+
831980
/**
832981
* Entry point for the hybrid digest run: per-user when preferences exist,
833982
* legacy broadcast otherwise.
983+
*
984+
* <p>Used by manual/admin triggers. The scheduled tick uses
985+
* {@link #processDigestTick()} so per-user crons are honored.
834986
*/
835987
public void sendDailyDigestHybrid() {
836988
List<String> connectionIds = digestConnectionIds();
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
package com.dbaagent.service.digest;
2+
3+
import org.springframework.scheduling.support.CronExpression;
4+
5+
import java.time.Duration;
6+
import java.time.Instant;
7+
import java.time.ZoneId;
8+
import java.time.ZonedDateTime;
9+
import java.util.Optional;
10+
11+
/**
12+
* Helpers for deciding whether a Spring 6-field cron expression is due
13+
* within a recent lookback window, interpreted in a given timezone.
14+
*
15+
* <p>Used by the digest minute-tick scheduler so each
16+
* {@code UserDigestPreference.cronExpression} fires in that user's timezone
17+
* without registering a separate db-scheduler task per preference.
18+
*/
19+
public final class DigestCronMatcher {
20+
21+
private DigestCronMatcher() {}
22+
23+
/**
24+
* Resolve an IANA timezone id; blank/invalid values fall back to UTC.
25+
*/
26+
public static ZoneId resolveZone(String timezone) {
27+
if (timezone == null || timezone.isBlank()) {
28+
return ZoneId.of("UTC");
29+
}
30+
try {
31+
return ZoneId.of(timezone.trim());
32+
} catch (Exception e) {
33+
return ZoneId.of("UTC");
34+
}
35+
}
36+
37+
/**
38+
* Returns the fire instant for the cron if it falls in {@code (now - lookback, now]},
39+
* interpreted in {@code zone}. Empty when not due or the cron is invalid.
40+
*
41+
* @param cronExpression Spring 6-field cron (sec min hour dom month dow)
42+
* @param zone timezone used to evaluate the cron
43+
* @param now current instant (typically clock.instant())
44+
* @param lookback how far back to search for a matching fire (e.g. 1 minute for a minute tick)
45+
*/
46+
public static Optional<Instant> dueWindowStart(
47+
String cronExpression,
48+
ZoneId zone,
49+
Instant now,
50+
Duration lookback) {
51+
if (cronExpression == null || cronExpression.isBlank() || zone == null || now == null) {
52+
return Optional.empty();
53+
}
54+
if (lookback == null || lookback.isNegative() || lookback.isZero()) {
55+
lookback = Duration.ofMinutes(1);
56+
}
57+
58+
final CronExpression cron;
59+
try {
60+
cron = CronExpression.parse(cronExpression.trim());
61+
} catch (IllegalArgumentException ex) {
62+
return Optional.empty();
63+
}
64+
65+
ZonedDateTime zonedNow = now.atZone(zone);
66+
ZonedDateTime from = zonedNow.minus(lookback);
67+
ZonedDateTime next = cron.next(from);
68+
if (next != null && !next.isAfter(zonedNow)) {
69+
return Optional.of(next.toInstant());
70+
}
71+
return Optional.empty();
72+
}
73+
74+
/**
75+
* Convenience: true when {@link #dueWindowStart} is present.
76+
*/
77+
public static boolean isDue(
78+
String cronExpression,
79+
ZoneId zone,
80+
Instant now,
81+
Duration lookback) {
82+
return dueWindowStart(cronExpression, zone, now, lookback).isPresent();
83+
}
84+
}

backend/src/main/java/com/dbaagent/service/scheduler/SlackDailyDigestTaskConfig.java

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,20 @@
1111
import org.springframework.context.annotation.Profile;
1212

1313
/**
14-
* Scheduled task configuration for daily digest delivery.
14+
* Scheduled task configuration for digest delivery.
1515
*
16-
* <p>Uses the hybrid mode: per-user personalized delivery when UserDigestPreference
17-
* rows exist; legacy channel-broadcast when none exist.
18-
*
19-
* <h3>PR3 Changes</h3>
16+
* <p>Ticks frequently (default: every minute) and delegates to
17+
* {@link SlackDailyDigestService#processDigestTick()} which:
2018
* <ul>
21-
* <li>Now calls {@code sendDailyDigestHybrid()} which routes to per-user or legacy
22-
* based on whether any preferences are configured</li>
23-
* <li>Two users with different personas on the same connection get different digests</li>
24-
* <li>Fallback to legacy broadcast if no per-user preferences exist for a connection</li>
19+
* <li>Honors each enabled {@code UserDigestPreference.cronExpression} in that
20+
* user's timezone when preferences exist</li>
21+
* <li>Falls back to the legacy singleton broadcast gated by
22+
* {@code slack.daily-digest.cron} when no enabled preferences exist</li>
2523
* </ul>
24+
*
25+
* <p>The global property {@code slack.daily-digest.cron} remains the default
26+
* schedule for preferences that leave {@code cronExpression} blank, and the
27+
* legacy broadcast schedule when the system is still in singleton mode.
2628
*/
2729
@Configuration
2830
@Profile("!test")
@@ -32,12 +34,11 @@ public class SlackDailyDigestTaskConfig {
3234
@Bean
3335
Task<Void> slackDailyDigestTask(
3436
SlackDailyDigestService service,
35-
@Value("${slack.daily-digest.cron:0 0 9 * * *}") String cron) {
36-
return Tasks.recurring("slack-daily-digest", Schedules.cron(cron))
37+
@Value("${slack.daily-digest.tick-cron:0 * * * * *}") String tickCron) {
38+
return Tasks.recurring("slack-daily-digest", Schedules.cron(tickCron))
3739
.execute((inst, ctx) -> {
38-
log.info("Starting scheduled daily digest delivery");
39-
service.sendDailyDigestHybrid();
40-
log.info("Completed scheduled daily digest delivery");
40+
log.debug("Digest scheduler tick");
41+
service.processDigestTick();
4142
});
4243
}
4344
}

backend/src/main/resources/application.properties

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,6 +321,9 @@ slack.bot-token=${SLACK_BOT_TOKEN:}
321321
slack.signing-secret=${SLACK_SIGNING_SECRET:}
322322
slack.deepsql-bot-username=${SLACK_DEEPSQL_BOT_USERNAME:}
323323
slack.daily-digest.cron=${SLACK_DAILY_DIGEST_CRON:0 0 9 * * *}
324+
# Minute tick that evaluates per-user UserDigestPreference.cronExpression (+ timezone).
325+
# Legacy/global cron above gates singleton broadcast when no enabled prefs exist.
326+
slack.daily-digest.tick-cron=${SLACK_DAILY_DIGEST_TICK_CRON:0 * * * * *}
324327

325328
# Brain understanding defaults
326329
brain.profile.max-columns-per-table=40

0 commit comments

Comments
 (0)