Re: [PR] fix(runtime-host): claim Daily Review archives before reading and widen the owned settle bound [maka]

["hqhq1025 (via GitHub)" <gi...@apache.org>]

hqhq1025 commented on code in PR #4672:
URL: https://github.com/apache/maka/pull/4672#discussion_r3926876779


##########
packages/runtime-host/src/server/daily-review-coordinator.ts:
##########
@@ -285,21 +282,23 @@ export class HostDailyReviewCoordinator {
     readonly trigger: 'cron' | 'manual';
     readonly replaceExisting: boolean;
   }): Promise<DailyReviewArchive> {
-    const summary = await this.#buildSummary(input.offsetDays, input.range);
-    const archiveId = dailyReviewArchiveId(summary.day, input.range);
-    const existing = await this.#store.getArchive(archiveId);
-    if (existing && !input.replaceExisting) return existing;
-    const config = await this.#store.readConfig();
-    const modelKey = input.modelKeyOverride.trim() || config.config.modelKey;
+    const now = this.#now();
+    const day = dayRange(now, input.offsetDays, input.range);
+    const archiveId = dailyReviewArchiveId(day, input.range);
+    const modelKeyOverride = input.modelKeyOverride.trim();
+    // Claim the archive before the first await. Two Clients asking for the
+    // same archive at once share one generation; a claim taken only after the
+    // reads let the second request slip past a first that had already
+    // published, and each Client then saw its own archive.
     const inFlight = this.#inFlight.get(archiveId);
     if (inFlight) {
-      if (inFlight.modelKey === modelKey && inFlight.trigger === input.trigger) {
+      if (inFlight.modelKeyOverride === modelKeyOverride && inFlight.trigger === input.trigger) {

Review Comment:
   [P2] Keep `replaceExisting` in the claim identity
   
   The early claim closes the duplicate-generation race, but it currently treats `replaceExisting: false` and `replaceExisting: true` as the same operation. If an archive already exists and a non-replacing request reaches `#run` first, it claims the archive while `#buildSummary` is pending. A concurrent replacing request then joins that promise here, and the leader returns the existing archive at line 323 without regenerating it. I reproduced this against this head: the replacing caller received the old `generatedAt`, and the model was called zero times. This silently drops the explicit replace request. Please include `replaceExisting` in the in-flight compatibility check (or otherwise give replace precedence), and cover the existing-archive ordering in both directions.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org