Skip to content

fix(sync): refresh stage 3 candidates only when tags are missing - #1163

Merged
ashwkun merged 1 commit into
masterfrom
fix/stage3-candidate-refresh
Oct 22, 2026
Merged

ashwkun merged 1 commit into
masterfrom
fix/stage3-candidate-refresh

Conversation

@ashwkun

@ashwkun ashwkun commented Oct 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Stage 3 no longer re-reads its candidate list from Turso on every run. The one-time check that applies relaxed (48 h) tags now records which core countries the tags were computed with, instead of counting shows without a tag.

Motivation

  • The check counted shows with no x flag as untagged, but core-country shows never carry x by design (applyCountryCheckFlag removes it). About 10,400 core shows were always counted, far over the 5% threshold, so every run refreshed candidates: a charts pre-aggregate plus a paged podcasts read, a few seconds and tens of thousands of reads each time.
  • Because the cadence book seed also runs whenever candidates refresh, it ran every run instead of about daily.

What changed

  • lib/staleness.js: candidateRefreshReason(st, { now, cadence, tagsKey }) decides whether stage 3 refreshes and why: no cached list, charts refreshed since, list over 20 hours old, the core countries differ from the state's relaxedTagsCore, or (cadence on) more than 5% of candidates, at least 50, without an iTunes ID. relaxedTagsKey() is the sorted core-country list. The 20-hour limit moved here as CANDIDATE_CACHE_MAX_AGE_MS.
  • 03-sync-episodes.js: each refresh records relaxedTagsCore; main() uses candidateRefreshReason, and the refresh log line now names the reason.
  • lib/state.js documents relaxedTagsCore and cadenceSeededAt; scripts/README.md describes when stage 3 refreshes.
  • Tests in lib/staleness.test.js: core shows without x keep the cached list (the regression), a cache never tagged or tagged for other core countries refreshes once and then uses the cache, the iTunes ID refresh still fires, and the existing reasons still refresh.

Behavior & compatibility

  • The first run after deploy refreshes once (state written before this change has no relaxedTagsCore); later runs use the cached list until charts refresh or it is 20 hours old. Changing the core countries in config.js re-tags on the next run.
  • Checked against a copy of the live sync state: as is, it refreshes once to apply tags; after that refresh it uses the cache. No candidate lacks an iTunes ID, so the cadence refresh stays off.
  • Tiers, due checks and the check cadence shadow are unchanged. The cadence book seed returns to about once a day.

Impact (required)

User impact — pick exactly one

  • user-impact-critical
  • user-impact-high
  • user-impact-medium
  • user-impact-low
  • no-user-impact

Backend — optional, pairable with any user-impact level

  • backend-change

Release copy (verbatim — highest priority)

CHANGELOG.md (required developer copy)

Fixed

  • Catalog sync: stage 3 re-reads its candidate list only when charts refresh, the list is 20 hours old or the core countries change, instead of every run; core-country shows without a relaxed tag no longer count as untagged.

README What's New / Upcoming (required for user-impact changes)

Test plan

  • npm run test:sync --prefix scripts: 195 tests pass
  • Decision checked against a copy of the live sync state
  • Required checks are green before merge

@ashwkun ashwkun added no-user-impact No listener-facing change (CI, docs, internal) backend-change Touches backend/proxy/infra — pairable with any user-impact level labels Oct 22, 2026
@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 4b76736 and 02b978a.

📒 Files selected for processing: 5
  • scripts/README.md
  • scripts/sync/03-sync-episodes.js
  • scripts/sync/lib/staleness.js
  • scripts/sync/lib/staleness.test.js
  • scripts/sync/lib/state.js

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Updates
    • Candidate lists are refreshed when they are missing, older than 20 hours, or out of date with chart updates or core-country settings.
    • When check cadence is enabled, candidate lists are also refreshed if more than 5% of candidates—and at least 50—lack an iTunes ID.
    • Missing tags alone no longer trigger a refresh for core shows; otherwise, runs without a refresh use the cached candidate list.

Stage 3 now delegates candidate refresh decisions to shared staleness logic. The policy covers cache age, chart updates, relaxed-tag country changes, and missing iTunes IDs when cadence is enabled. Refresh logs include the reason, and refreshed state records the relaxed-tag key.

Changes: Candidate Refresh

Layer / File(s) Summary
Refresh policy and state contract
scripts/sync/lib/staleness.js, scripts/sync/lib/state.js, scripts/sync/lib/staleness.test.js
The staleness module adds a normalized relaxed-tag key and a helper that returns a refresh reason for missing candidates, newer charts, expired caches, changed tag keys, or the cadence-specific missing-iTunes-ID threshold. State documentation adds the relaxed-tag key and cadence seed fields. Tests cover the policy and key normalization.
Stage 3 refresh integration
scripts/sync/03-sync-episodes.js, scripts/README.md
Stage 3 uses the shared policy, logs the refresh reason, stores the relaxed-tag key after refreshing, and uses the shared cache-age limit for cadence seeding. The README documents the refresh conditions.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 02b97

The candidate refresh change is mergeable after normal checks; no actionable issue was identified.

Pre-merge checks: Passed 8 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 8 passed
Check name Status Explanation
Title check Passed The title follows Conventional Commits with the allowed type 'fix' and scope '(sync)'. It uses imperative mood and is 64 characters, under the ~72 limit. It matches the main change: stage 3 candidates…
Description check Passed The description states the problem (stage 3 re-reading candidates every run because core-country shows were counted as untagged), lists the changes to lib/staleness.js, 03-sync-episodes.js, lib/state.…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Unresolved Review Threads Passed No unresolved review threads are reported. The supplied review context states that no CodeRabbit review threads were returned and that the current review produced zero actionable findings.
Architecture Compliance Passed PASS: The PR changes only catalog-sync scripts and documentation under scripts/; it does not touch Android feature/core modules, Gradle dependencies, or composition-root code. The changed content co…
Module Readme Updated Passed The PR changes only five files under scripts/, including JavaScript, a test, and scripts/README.md. It changes no Kotlin files under app/src/main/, core//src/main/, or feature//src/mai…
Jvm Tests For Changed Logic Passed The authoritative diff changes only JavaScript files under scripts/sync and adds regression coverage in scripts/sync/lib/staleness.test.js. The changed helper candidateRefreshReason and refresh …
Full details: Title check

Explanation

The title follows Conventional Commits with the allowed type 'fix' and scope '(sync)'. It uses imperative mood and is 64 characters, under the ~72 limit. It matches the main change: stage 3 candidates are refreshed only when tags are missing, which the changeset implements through candidateRefreshReason and the relaxedTagsKey check.

Full details: Description check

Explanation

The description states the problem (stage 3 re-reading candidates every run because core-country shows were counted as untagged), lists the changes to lib/staleness.js, 03-sync-episodes.js, lib/state.js, scripts/README.md, and the tests, and reports test results and compatibility impact. It is clearly related to the changeset.

Full details: Architecture Compliance

Explanation

PASS: The PR changes only catalog-sync scripts and documentation under scripts/; it does not touch Android feature/core modules, Gradle dependencies, or composition-root code. The changed content contains no PostHog, Hilt, Koin, Dagger, MockK, PlaybackRepository, AppContainer, designsystem, or feature-module references. The new relaxedTagsCore field belongs to pipeline state, which state.js documents as app-private; missing legacy values trigger a one-time refresh, so no identity or storage contract is broken.

Full details: Module Readme Updated

Explanation

The PR changes only five files under scripts/, including JavaScript, a test, and scripts/README.md. It changes no Kotlin files under app/src/main/, core/<module>/src/main/, or feature/<module>/src/main/. The module README requirement is therefore not triggered.

Full details: Jvm Tests For Changed Logic

Explanation

The authoritative diff changes only JavaScript files under scripts/sync and adds regression coverage in scripts/sync/lib/staleness.test.js. The changed helper candidateRefreshReason and refresh behavior are tested for cached core shows, missing/mismatched tags, cadence IDs, missing lists, newer charts, and expiry. No JVM/Android production logic changed, so src/test JVM tests are not applicable.

Full details: Docstring Coverage

Explanation

Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • 🛠️ update changelog
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@ashwkun
ashwkun merged commit 8d8b9a8 into master Oct 22, 2026
11 of 12 checks passed
@ashwkun
ashwkun deleted the fix/stage3-candidate-refresh branch October 22, 2026 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend-change Touches backend/proxy/infra — pairable with any user-impact level no-user-impact No listener-facing change (CI, docs, internal)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant