Stop the nightly translation re-translating everything, most days - #685
Stop the nightly translation re-translating everything, most days#685chhhee10 wants to merge 5 commits into
Conversation
`rust-quality` cached `~/.cargo` AND `target/` under a combined `actions/cache@v6`, which writes a ref-scoped copy from every branch that misses the exact key. Because the entry carries build output, each copy is 1.5-2.3 GiB, and five were live at once — PR refs 677, 679, 680, 681 and main — putting the repository at 11.56 GiB against GitHub's 10 GiB cap and therefore permanently in LRU eviction. The thing being evicted was not another cargo build. It was the 13 KB doc translation cache, read once every 24 hours by the nightly `translate-docs` run and so always the least-recently-used entry in the store. Losing it re-translated 48 pages into 14 languages the next morning: ~125 runner-minutes and a full LLM pass per language, against a 4-minute baseline when it survives. Six consecutive days of that, Aug 6-11, cost ~750 runner-minutes and six full-corpus passes through the gateway. Restore on every run, save only on a push to main — the split `build-daemon.yml` already uses, whose comment gives the other reason to want it (a PR branch can otherwise write a poisoned `target/` that a later release run restores straight into a published binary). `cache-hit != 'true'` keeps a run that changed nothing from re-uploading 2 GiB. What a PR gives up: one whose `Cargo.lock` moved rebuilds from a stale-but-close main cache. That is already what `restore-keys` hands it today. Co-Authored-By: Claude Opus 5 <[email protected]>
The only save sat in `consolidate`, downstream of both the matrix gate
(`if: needs.translate.result == 'success'`) and `mintlify validate`. So the
day's cache was contingent on fourteen languages and a nav check all succeeding:
Aug 6 discarded ~110 minutes of completed translation because one `ko` page
failed validation, and Aug 12 discarded a full run because consolidate's
validation failed. In both cases every language had finished its work and
uploaded its fragment; the cache was thrown away anyway.
Each fragment is already authoritative for its own language, so nothing has to
be merged before it can be stored. Each language now saves its own, in the job
that produced it, immediately after the step that proved it good. Consolidate's
merged save stays as the cross-language fallback.
The restore key changes for a related reason. It read
`translation-cache-${{ hashFiles('scripts/translate-docs/.translation-cache.json') }}`,
which ALWAYS evaluated to the bare literal `translation-cache-`: the file is
gitignored, so it is absent at checkout and `hashFiles` returns "" for a path
that matches nothing. Every restore that ever worked was a `restore-keys` prefix
match. That is not a bug on its own — but it means a total miss and a hit are
indistinguishable, so the expensive case was silent. It is now a per-language
key with the merged entry as fallback, and a miss emits a `::warning` naming
what it is about to cost.
Artifact retention 1 → 7 days, so a run that dies mid-pipeline leaves a human a
recovery path rather than expiring overnight.
Co-Authored-By: Claude Opus 5 <[email protected]>
58a0a80 to
06833d8
Compare
`isCached` is a pure function of the ENGLISH source hash. It records that a page
was translated once — never that the translation is on disk now — and those two
facts came apart in production.
Translations land on an auto-translate PR branch. While that branch sits
unmerged, `main` lacks the files and the cache still reports them done, so they
are never regenerated. Meanwhile `--update-nav` reads the ENGLISH tree and emits
nav entries for them, and `mintlify validate` fails on entries pointing at files
that are not there. Verified on the live repo: `docs/cli/{update,migrate}.mdx`
exist on main, `docs/zh/cli/` has neither, and PR #682 carrying them is still
open — 28 missing files across 14 locales.
That is non-convergent, which is what makes it worth a code change rather than a
merge. A cache HIT writes nothing, so validation fails and the cache is never
saved; a full cache MISS spends 120 runner-minutes and goes green. The pipeline
had no path to a cheap success while #682 stayed open, and Aug 12 is exactly
that: all 14 languages finished in ~20 seconds each, and consolidate failed.
Statting the output makes the cache self-healing against any "translated once,
never landed" gap, whatever opened it — an unmerged PR, a hand-reverted file, a
locale added to the matrix after the fact.
Guarded at all four sites rather than one. `cli.ts` is load-bearing: the batch
path sorts pages into cached/uncached itself and never calls `translateMdxPage`
for a cached one, so guarding only the translator would have fixed nothing. The
two single-page paths are guarded too, or they and the batch path disagree about
what "cached" means.
The tests pin both directions — a missing output re-translates, a present one is
still skipped. The second matters as much as the first: without it a later
refactor could satisfy this commit by making the guard a cache bypass, and every
run would be a full re-translation with the suite still green. Confirmed the
first test fails on the pre-fix code rather than assuming it would.
NOTE: this changes translation OUTPUT, not just caching. The first run after it
lands re-sends those 28 pages to the model, so the text will not be byte-identical
to what sits on #682 — expect a noisy diff there once. ~10 minutes, once.
Co-Authored-By: Claude Opus 5 <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTranslation cache hits now require existing output files. CI and translation workflows use explicit cache restore and save steps. Release notes document these changes, and tests cover missing and present cached outputs. ChangesTranslation cache reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
06833d8 to
b20ebad
Compare
Hermes
The cache fragments can record a new English source hash before its translated output is durably available on main. After a later job or consolidation failure, the next run can treat an older on-disk translation as current and publish it without retranslating. What this changesflowchart LR
n0Translationworkflow["~ Translation workflow"]
n1Translationcache["~ Translation cache"]
n2Localizeddocumentation["~ Localized documentation"]
n3Documentationnavigation["~ Documentation navigation"]
n4RustCIcache["~ Rust CI cache"]
n5Translationtests["~ Translation tests"]
n0Translationworkflow -- "restores and saves fragments" --> n1Translationcache
n1Translationcache -- "controls translation skips" --> n2Localizeddocumentation
n0Translationworkflow -- "uploads and consolidates output" --> n2Localizeddocumentation
n0Translationworkflow -- "regenerates localized entries" --> n3Documentationnavigation
n3Documentationnavigation -- "references MDX pages" --> n2Localizeddocumentation
n4RustCIcache -- "shares repository cache capacity" --> n1Translationcache
n5Translationtests -- "verifies output-presence guard" --> n1Translationcache
Rounds
FindingsOpen
|
The save key embeds `github.run_id`, and GitHub REUSES that id when someone re-runs a failed job. On the second attempt the primary key already exists, the restore scores an exact hit, and the save collides with itself. Found by running it rather than reasoning about it. A throwaway workflow exercising the same key shapes showed all four cases: - first run: cache-matched-key='', save proceeds - next run: restored from probe-zh-<prev_id>, cache-hit='false' - re-run: cache-hit='true' <- the collision - languages stayed isolated: ja restored ja's payload, zh restored zh's The same `cache-hit != 'true'` guard `build-daemon.yml:137` carries. With it the re-run skips the save and stays green. That probe also confirmed the two things the rest of this branch assumes and could not otherwise check: `cache-matched-key` really is empty on a total miss — so the new warning fires exactly when a language is about to re-translate everything, and stays silent on the prefix hits that are the normal case — and the `restore-keys` prefix genuinely carries the previous run's file across, which is the whole mechanism by which tomorrow's run inherits today's cache. Co-Authored-By: Claude Opus 5 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/translate-docs.yml:
- Around line 105-108: Update the workflow’s prepare logic to validate every
language from inputs.languages against the supported language allowlist and
reject any invalid value before shell execution. Pass only the validated
language value to translation steps through an environment variable, and quote
that variable wherever the translation command uses it so shell metacharacters
cannot execute commands.
In `@scripts/translate-docs/readme-translator.ts`:
- Around line 223-225: Add direct tests for translateReadme covering both
cache-condition outcomes: regenerate the README translation when
docs/i18n/README.<lang>.md is missing, and skip generation when that file
exists. Add these cases to
__tests__/scripts/translate-docs/readme-translator.test.ts, using the existing
test setup and mocks to verify translation is invoked only for the missing-file
case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b204146e-eeac-40f8-8d0c-3ec775e03b57
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/translate-docs.ymlCHANGELOG.md__tests__/scripts/translate-docs/mdx-translator.test.tsscripts/translate-docs/cli.tsscripts/translate-docs/mdx-translator.tsscripts/translate-docs/readme-translator.ts
| - name: Warn on translation cache miss | ||
| if: steps.restore-cache.outputs.cache-matched-key == '' | ||
| run: | | ||
| echo "::warning title=Translation cache MISS::${{ matrix.lang }} will re-translate every page (~9 runner-minutes and one full LLM pass)" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,125p' .github/workflows/translate-docs.yml
rg -n -C 4 'translate-docs\.yml|uses:.*translate-docs|inputs:\s*$|languages:' .github/workflowsRepository: FailproofAI/failproofai
Length of output: 9116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow remainder ---'
sed -n '120,260p' .github/workflows/translate-docs.yml
printf '%s\n' '--- language-related implementation ---'
rg -n -C 5 'languages|language|lang' scripts/translate-docs package.json .github/workflows/translate-docs.yml \
-g '*.ts' -g '*.js' -g '*.json' -g '*.yml' -g '*.yaml' -g 'package.json' \
| head -n 500
printf '%s\n' '--- workflow references and permissions ---'
rg -n -C 5 'translate-docs|workflow_call|workflow_dispatch|permissions:|ANTHROPIC_API_KEY|ANTHROPIC_AUTH_TOKEN' .github README.md scripts \
-g '*.yml' -g '*.yaml' -g '*.md' -g '*.ts' -g '*.js' -g 'package.json' \
| head -n 500
printf '%s\n' '--- candidate files ---'
git ls-files scripts/translate-docs .github/workflows/translate-docs.ymlRepository: FailproofAI/failproofai
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
workflow = Path(".github/workflows/translate-docs.yml").read_text()
cases = [
'en',
'en; echo INJECTED',
"en' ; echo INJECTED; #",
'en", "ja"]',
]
needles = [
"if [ -n \"${{ inputs.languages }}\" ]; then",
"echo '${{ inputs.languages }}' | jq -Rc",
"echo \"::warning title=Translation cache MISS::${{ matrix.lang }}",
"bun run translate --languages ${{ matrix.lang }}",
]
for value in cases:
print(f"=== input: {value!r} ===")
expanded = workflow.replace("${{ inputs.languages }}", value).replace("${{ matrix.lang }}", value)
for needle in needles:
pos = expanded.find(needle.split("${{")[0])
if pos >= 0:
print(expanded[pos:expanded.find("\n", pos)])
PYRepository: FailproofAI/failproofai
Length of output: 1652
Validate and quote inputs.languages before shell use
A workflow_dispatch user can set inputs.languages. The value reaches prepare shell source and the unquoted translation command. For example, en;id runs id as a separate command while ANTHROPIC_API_KEY is set. Validate each language against the supported allowlist in prepare, then pass the validated value through a quoted environment variable.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 108-108: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/translate-docs.yml around lines 105 - 108, Update the
workflow’s prepare logic to validate every language from inputs.languages
against the supported language allowlist and reject any invalid value before
shell execution. Pass only the validated language value to translation steps
through an environment variable, and quote that variable wherever the
translation command uses it so shell metacharacters cannot execute commands.
Source: Linters/SAST tools
| // `&& existsSync(outputPath)` — see the MDX path. Cached records that a | ||
| // translation was produced, not that the file is there now. | ||
| if (isCached(cache, "README.md", lang, sourceContent) && existsSync(outputPath)) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add direct tests for the README cache condition.
The supplied tests invoke translateMdxPage only. They do not exercise translateReadme.
Add tests in __tests__/scripts/translate-docs/readme-translator.test.ts for both cases: regenerate when docs/i18n/README.<lang>.md is absent, and skip generation when it exists. As per coding guidelines: “When you add or change logic, add a corresponding test in __tests__/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/translate-docs/readme-translator.ts` around lines 223 - 225, Add
direct tests for translateReadme covering both cache-condition outcomes:
regenerate the README translation when docs/i18n/README.<lang>.md is missing,
and skip generation when that file exists. Add these cases to
__tests__/scripts/translate-docs/readme-translator.test.ts, using the existing
test setup and mocks to verify translation is invoked only for the missing-file
case.
Source: Coding guidelines
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Hermes found no blocking issues in this revision.
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Hermes found blocking issues that should be addressed.
High: Do not persist a source cache entry before its output is durable
- Rule:
COR-001 - Location:
.github/workflows/translate-docs.yml:138 - Evidence: The matrix job saves the new per-language source-hash cache at
.github/workflows/translate-docs.yml:138before artifact upload and before consolidation. If artifact upload or any consolidation step fails, no newly translated bytes reach main, but the cache remains. On the next scheduled run,cli.tsconsiders an existing old locale file cached when the saved source hash matches (isCachedat line 271 plusexistsSyncat line 282). It then uploads that stale file and a later successful consolidation can publish it as the translation for the newer English source. - Required change: Only mark an entry cached once the exact translated bytes are durable on the branch future runs check out, or store a target-content hash in cache entries and require the on-disk output to match it. If preserving per-language work across consolidation failures is required, persist and restore the corresponding output artifact atomically with its cache entry rather than relying on file existence alone.
| # `github.run_id`, which is REUSED when someone re-runs a failed job. On | ||
| # that second attempt the primary key already exists, so the restore above | ||
| # scores an exact hit and this save would collide with itself. | ||
| - name: Save translation cache fragment |
There was a problem hiding this comment.
Hermes — High/High (COR-001): Do not persist a source cache entry before its output is durable
The matrix job saves the new per-language source-hash cache at .github/workflows/translate-docs.yml:138 before artifact upload and before consolidation. If artifact upload or any consolidation step fails, no newly translated bytes reach main, but the cache remains. On the next scheduled run, cli.ts considers an existing old locale file cached when the saved source hash matches (isCached at line 271 plus existsSync at line 282). It then uploads that stale file and a later successful consolidation can publish it as the translation for the newer English source.
Required change: Only mark an entry cached once the exact translated bytes are durable on the branch future runs check out, or store a target-content hash in cache entries and require the on-disk output to match it. If preserving per-language work across consolidation failures is required, persist and restore the corresponding output artifact atomically with its cache entry rather than relying on file existence alone.
|
Superseded by #694. All four fix commits are in #694 unchanged: the cargo/translation cache split, the per-language save, the Worth recording why the guard matters more there than here. On Actions the eviction this PR fixes was accidentally load-bearing — a cache hit whose output existed only on an unmerged PR branch failed validation, and only a full 120-minute miss went green. On the box nothing evicts the cache, so that escape hatch is gone and the This branch also carries a later commit of 679 auto-generated translation files ( |
2 participants
The
Translate Docscron cost 4 minutes on Aug 3-5 and 118-136 minutesevery day from Aug 6-11 — ~750 wasted runner-minutes and six full-corpus passes
through the LLM gateway in six days.
Three causes compound. None of them is the translation cache's own logic, which
is sound —
isCachedvalidates per entry against the English source hash, andthe top-level
sourceHashfield is vestigial (written in three places, read innone).
1. The cache was evicted between runs — dominant, ~105 min/day
ci.ymlcachedtarget/alongside the cargo registry under a combinedactions/cache@v6, which writes a ref-scoped copy from every branch thatmisses the exact key. With build output included each copy is 1.5-2.3 GiB, and
five were live at once — PR refs 677, 679, 680, 681 and main — putting the repo
at 11.56 GiB against GitHub's 10 GiB cap, so the store sits permanently in
LRU eviction.
What that evicted was the 13 KB translation cache, read once every 24 hours
and therefore always the least-recently-used entry in the store. Aug 9's jobs
say so directly:
Cache not found for input keys: translation-cache-, translation-cache-, the morning after Aug 8 loggedCache saved with key: translation-cache-1adebf3f….Fixed with the restore/save split
build-daemon.yml:117-144already uses, whosecomment gives the other reason to want it (a PR branch can otherwise write a
poisoned
target/that a release run restores into a published binary).2. The cache was saved once, at the end of a serial pipeline
The only save sat in
consolidate, downstream of both the matrix gate andmintlify validate. One page failing in one language discarded all fourteen —Aug 6 lost ~110 minutes of finished translation to a single
kopage. Eachlanguage now saves its own fragment in the job that produced it, right after the
step that proved it good. Each fragment is already authoritative for its own
language, so nothing needs merging first; consolidate's merged save stays as the
cross-language fallback.
A miss is also visible now. The old restore key always evaluated to the bare
literal
translation-cache-— the file is gitignored, so it is absent atcheckout and
hashFilesreturns""for a path matching nothing. Every restorethat ever worked was a
restore-keysprefix match, and a total miss lookedexactly like a hit: nothing failed, nothing warned, the job just spent nine
minutes and a full LLM pass.
3. A cache HIT never checked the file exists — this is Aug 12, and it deadlocks
isCachedrecords that a page was translated once, not that it is on disknow. Translations land on an auto-translate PR branch; while that sits
unmerged,
mainlacks the files and the cache still says done, so they are neverregenerated — while
--update-navreads the English tree and emits naventries for them.
mintlify validatethen fails on 28 missing files.Verified live rather than inferred:
docs/cli/update.mdxanddocs/cli/migrate.mdxare onmain,docs/zh/cli/has neither, and #682carrying them is still open.
That is non-convergent: a cache hit writes nothing → validation fails → the
cache is never saved; a full cache miss spends 120 minutes → goes green. There
was no path to a cheap success while #682 stayed open.
re-sends those 28 pages to the model, so the text will not be byte-identical to
what is on #682 — expect one noisy diff there. ~10 minutes, once.
Guarded at all four call sites.
cli.tsis the load-bearing one: the batchpath sorts pages into cached/uncached itself and never calls
translateMdxPagefor a cached page, so guarding only the translator would have fixed nothing.
Testing
Beyond the suite, the deadlock was reproduced against the real repo with a
realistically seeded cache (every English page marked translated for
zhat itscurrent hash, as a successful Aug 11 run would have left it):
The new unit tests pin both directions — a missing output re-translates, a
present one is still skipped. The second matters as much as the first: without
it a later refactor could satisfy this PR by turning the guard into a cache
bypass, and every run would be a full re-translation with the suite still green.
I also confirmed the first test fails on the pre-fix code rather than
assuming it would.
bun run test:run— 3458 pass, 10 skippedtsc --noEmit— cleaneslinton every changed file — cleanDeliberately not included
already makes the cache survive that failure, so this is reorder risk for
little return.
Expected effect
Steady state returns to the ~4-minute cache-hit day. The first run after merge
re-translates the 28 pages
mainis missing (~10 minutes, once), and the repocache should fall back under the 10 GiB cap as the four PR-scoped cargo copies
age out.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HEwcerc9jE7ZBkfBiYRbep
Hermes review
bd3c110cdeebc3430053b9f70afa7a213a7ee9521d8f31d926828f3bae215c58f5b35baa44acbff0gpt-5.6-terraSummary
The cache fragments can record a new English source hash before its translated output is durably available on main. After a later job or consolidation failure, the next run can treat an older on-disk translation as current and publish it without retranslating.
Changes
Validation
Passeddocker run --rm --network=none -v /review/input/workspace:/workspace:ro -w /workspace oven/bun:latest bun -e '<navigation reference check>'— Parsed docs/docs.json and verified all 720 navigation page references resolve to an MDX file. (0s)Findings
.github/workflows/translate-docs.yml:138before artifact upload and before consolidation. If artifact upload or any consolidation step fails, no newly translated bytes reach main, but the cache remains. On the next scheduled run,cli.tsconsiders an existing old locale file cached when the saved source hash matches (isCachedat line 271 plusexistsSyncat line 282). It then uploads that stale file and a later successful consolidation can publish it as the translation for the newer English source. (.github/workflows/translate-docs.yml:138)Open questions
None.
Policy overrides
None.
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
Tests