feat(home-feed): on-visit rollup refresh + scheduler cadence drop [JARVIS-512]#25599
Conversation
…RVIS-512]
Final step of JARVIS-512. Flips the rollup producer's primary
trigger from the scheduler (every N minutes) to the HTTP route
(debounced, fire-and-forget on each GET /v1/home/feed). The
scheduler becomes a safety net with a much longer cadence for idle
stretches where nobody opens the Home page.
home-feed-routes.ts
- New module-level debounce gate (10-minute window,
ON_VISIT_REFRESH_DEBOUNCE_MS) tracks the last refresh fire time.
- handleGetHomeFeed computes its response FIRST, then fires
maybeTriggerOnVisitRollupRefresh at the end so nothing in the
trigger path can delay the GET. Returns immediately with the
cached feed; the rollup's writes publish home_feed_updated via
the writer's SSE path, and the client auto-refreshes.
- Any error inside the producer is swallowed into a warn log so
an LLM hiccup can never turn a GET into a 500.
- __resetOnVisitRefreshStateForTests exported for test isolation.
feed-scheduler.ts
- ROLLUP_INTERVAL_MS drops from 30 minutes to 2 hours. The
docstring makes it explicit that the scheduler is now the
safety net, not the primary trigger, so the long cadence is
intentional and doesn't compete with the route trigger.
- Adds in_flight to the cooldown-exclusion list alongside
no_provider and no_actions: if a concurrent caller (usually
the on-visit refresh) is already running the producer, that
caller's result effectively counts as this scheduler tick's
run — advancing the gate here would force the next tick to
wait out the full 2h window for no reason.
rollup-producer.ts
- New in_flight skip reason + module-level producerInFlight
guard. Second concurrent caller short-circuits before entering
the body so two LLM requests never race each other. Lock is
released in a finally so a thrown exception can't strand it in
the true state.
Tests
- rollup-producer.test.ts: new test that gates the provider
behind a manual deferred and asserts a second concurrent call
returns in_flight without invoking the provider.
- feed-scheduler.test.ts: updated cadence test for the new 2h
window, new in_flight cooldown-exclusion test.
- home-feed-routes.test.ts: mock.module stub for the rollup
producer + tests for (a) first GET fires the rollup,
(b) repeat GETs within the debounce window do NOT re-fire,
(c) producer failure does not turn the GET into a 500.
Both test files pass individually (29 + 149 = 178 tests, 0 fails).
The cross-file mock.module leak between src/home/ and src/runtime/
routes/__tests__/home-feed-routes.test.ts is pre-existing from
Phase 5 and noted in context.md as a separate fix-up.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| void emitFeedEvent({ | ||
| source: "assistant", | ||
| title: sequence.name, | ||
| summary: `Sent step ${step.index + 1} of ${sequence.steps.length} to ${enrollment.contactEmail}.`, | ||
| dedupKey: `sequence-step:${enrollment.id}:${step.index}`, |
There was a problem hiding this comment.
🚩 Contact email included in sequence feed event summary
At engine.ts:212, the feed event summary includes enrollment.contactEmail in plaintext: Sent step N of M to user@example.com. This data is persisted to the local home-feed.json file. While this is the user's own workspace (not shared), it's worth noting that PII (contact emails from outreach sequences) now appears in the activity feed file. This is consistent with how the sequence engine already stores enrollment data locally, so it's not a new exposure vector, but reviewers may want to consider whether the summary should be redacted.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ed2b7d54c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function maybeTriggerOnVisitRollupRefresh(now: Date): void { | ||
| const nowMs = now.getTime(); | ||
| if (nowMs - lastOnVisitRefreshAt < ON_VISIT_REFRESH_DEBOUNCE_MS) return; | ||
| lastOnVisitRefreshAt = nowMs; |
There was a problem hiding this comment.
The debounce gate is advanced before runRollupProducer returns, so even an immediate skippedReason: "no_provider" consumes the full 10-minute window. In practice, if the first feed visit happens while no inference provider is available (for example right before credentials are configured), subsequent GET /v1/home/feed calls cannot retry the refresh until the window expires, leaving Home stale despite user visits being the primary trigger. The timestamp should only be committed for outcomes that represent a real run attempt, or no_provider should be exempted from burning the debounce.
Useful? React with 👍 / 👎.
alex-nork
force-pushed
the
alex-nork/jarvis-512-onvisit-refresh
branch
from
3ed2b7d to
09e5564
Compare
…512] Address review feedback on #25599: 1. Codex P2 (real bug) — on-visit debounce was committed before runRollupProducer returned, so a `no_provider` result on daemon boot (scheduler + route both run before provider registry is ready) burned the full 10-minute window. User-visible impact: Home stayed stale for 10 minutes after startup even though the user was actively refreshing. Fix: eager-advance the gate for concurrency safety (blocks parallel GETs during the LLM call), then roll it back in the producer's .then() callback when the skip reason is one of no_provider / no_actions / in_flight (the three reasons that short-circuit before the LLM call). Real LLM attempts (success, empty_items, malformed_output, provider_error) keep the advanced gate to preserve backoff against a broken producer. Defensive: rollback only fires if lastOnVisitRefreshAt still equals our eager-advance value — if a concurrent GET somehow raced past the guard and advanced past nowMs we don't clobber its newer timestamp. New test asserts the rollback path: first GET gets no_provider, second GET within the window must still fire the producer. 2. Devin BUG_0001 / BUG_0002 — two stale "30 minutes" references in scheduler test comments that were missed when the cadence was changed to 2 hours in the same PR. The actual assertions already use 2h; only the prose was stale. Fixed both. Also updated the default rollupProducerSpy mock in home-feed-routes.test.ts to return empty_items instead of no_actions so the default case matches production debounce semantics (gate holds on a real LLM attempt), with individual tests explicitly overriding to exercise the rollback path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
Final step of JARVIS-512, stacked on #25594 (rollup producer rewrite, open). Flips the rollup producer's primary trigger from the scheduler (every N minutes) to the HTTP route (debounced, fire-and-forget on each GET /v1/home/feed). The scheduler becomes a safety net with a much longer cadence for long idle stretches where nobody opens the Home page.
Changes
home-feed-routes.ts— on-visit refreshON_VISIT_REFRESH_DEBOUNCE_MS) tracks the last refresh fire time.handleGetHomeFeedcomputes its response FIRST, then firesmaybeTriggerOnVisitRollupRefreshat the end so nothing in the trigger path can delay the GET. Returns immediately with the cached feed; the rollup's writes publishhome_feed_updatedvia the writer's SSE path, and the client auto-refreshes.__resetOnVisitRefreshStateForTestsexported for test isolation.feed-scheduler.ts— cadence drop +in_flighthandlingROLLUP_INTERVAL_MSdrops from 30 minutes to 2 hours. Docstring makes it explicit that the scheduler is now the safety net, not the primary trigger, so the long cadence is intentional and doesn't compete with the route trigger.in_flightto the cooldown-exclusion list alongsideno_providerandno_actions: if a concurrent caller (usually the on-visit refresh) is already running the producer, that caller's result effectively counts as this scheduler tick's run — advancing the gate here would force the next tick to wait out the full 2h window for no reason.rollup-producer.ts— concurrency guardin_flightskip reason + module-levelproducerInFlightguard. Second concurrent caller short-circuits before entering the body so two LLM requests never race each other. The lock is released in afinallyso a thrown exception can't strand it in thetruestate.runRollupProducerInnerso the guard wraps the whole pipeline cleanly.Loop / runaway-cost audit (for the concern you raised earlier)
digest/threaditems only. Neither type feeds back into the producer's input (defaultLoadRecentActionsfilters fortype === "action"and nothing else).in_flightguard ensures only one LLM request runs. The second caller returns immediately withskippedReason: "in_flight"and neither the scheduler gate nor the route gate advances incorrectly.no_actionsshort-circuiting before the LLM call when the log is empty, an idle daemon costs ~0 LLM calls.Testing
bun run typecheck— cleanbun run lint— cleanbun test src/home/— 149/149 pass (added: in-flight guard test, updated 2h cadence test, in_flight cooldown test)bun test src/runtime/routes/__tests__/home-feed-routes.test.ts— 29/29 pass (added: fires rollup on GET, debounce prevents refire, producer failure doesn't turn GET into error)mock.moduleleak betweensrc/home/andsrc/runtime/routes/__tests__/home-feed-routes.test.tsthat context.md flagged back in Phase 5 still exists but is unrelated to this change.JARVIS-512 wrap-up
With this PR, the original ticket scope is complete:
Follow-up work still open, not blocking closing the ticket:
🤖 Generated with Claude Code