Skip to content

fix(ci): bound system dependency downloads - #4514

Merged
CyMule merged 3 commits into
mainfrom
fix/ci-apt-download-timeouts
Oct 2, 2026
Merged

CyMule merged 3 commits into
mainfrom
fix/ci-apt-download-timeouts

Conversation

@CyMule

@CyMule CyMule commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

An Ubuntu package download in #4357's CI took 54 minutes for 262 MB, compared with 7–39 seconds on the other runners. The existing retry loop waits indefinitely for a command to return, so it cannot recover from a download that continues extremely slowly.

This change shares dependency setup across the unit, extras, and ingest jobs. It applies 30-second HTTP/HTTPS inactivity timeouts, APT retries, and three bounded five-minute attempts for index updates and downloads. Packages are downloaded first and installed with --no-download, keeping dpkg installation outside the outer timeout. Setup appears as a separate step, and the PPA's implicit index refresh is replaced with an explicitly bounded refresh.

The package set, Tesseract version check, and test commands are preserved. Persistently poor networking now produces a clear setup failure instead of an hour-long download; the change does not guarantee a faster mirror.

Validation: Bash syntax, ShellCheck, shfmt, workflow YAML parsing, and whitespace checks passed. A mocked harness verified success, retry after timeout, exhaustion, and installation sequencing. A disposable Ubuntu 24.04 container validated the APT flags with strict index update, download-only fetching, and offline installation of libmagic-dev. The full helper also completed successfully in a disposable Ubuntu 24.04 ARM64 container, including LibreOffice, the Tesseract PPA, Korean OCR data, and diffstat; Tesseract reported 5.5.1. The repository release-version check passed on Ubuntu. GitHub CI passed on aa97013, including the unit, dependency-extras, ingest and Dockerfile jobs that use the new helper; no end-to-end CI speedup has been established.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-01T19:08:02.940838Z 979d9bf PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread scripts/ci/install-system-deps.sh

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic

@paulkarayan

Copy link
Copy Markdown
Contributor

Automated review, forwarded by @paulkarayan. Findings are code-verified where they name a file and line; the strong-review gpt-pro leg and cubic did not run.

GO -- 0 blocker(s), 0 concern(s), 3 note(s), 0 nit(s).

Moves CI apt setup into one bounded-retry script; CI green at head with the script running on the real runners, no defects found.

Notes (3)

  1. codeflash.yml:35, ingest-test-fixtures-update-pr.yml:83 and partition-benchmark.yaml:65 still run the old unbounded add-apt-repository + apt-get sequence, so those workflows can still hang on a slow mirror.
  2. The timeout/retry/exhaustion paths were exercised only by the author's mocked harness; CI at aa97013 succeeded on attempt 1/3 everywhere, so no live run has hit a timeout.
  3. A mirror slower than about 0.9 MB/s sustained now fails setup after three 5-minute attempts instead of finishing slowly; completed .debs persist across attempts, and the PR body states this trade.
Full review
# Unstructured-IO/unstructured#4514 -- fix(ci): bound system dependency downloads

Requested by pk via a forward to Thorn: https://unstructuredai.slack.com/archives/D0BFPFGDNP7/p1790951521108809
Author: CyMule (Nick Franck). Reviewed at head `aa970136` vs `origin/main`.

**Verdict:** GO
**Blast radius:** 3/5

The PR replaces three copies of an inline, unbounded apt retry loop in `ci.yml` (test_unit, test_unit_dependency_extras, test_ingest_src) with one script, `scripts/ci/install-system-deps.sh`. That script caps each network step (index update, `--download-only` fetch, PPA add) at 5 minutes over 3 attempts with 30s HTTP inactivity timeouts, and runs the dpkg install outside the timeout with `--no-download`. Package sets and the Tesseract 5.5.1 pin are unchanged. CI is all green at this exact head (run 37012105700, headSha aa970136), and the logs show the script itself running on the `opensource-linux-8core` runners (observed). The one part nobody has run live is the timeout/exhaustion path. It is simple enough to trust from reading.

## Notes

- Three other workflows still use the old unbounded sequence (observed): `.github/workflows/codeflash.yml:35`, `.github/workflows/ingest-test-fixtures-update-pr.yml:83`, `.github/workflows/partition-benchmark.yaml:65`. A slow mirror can still hang those jobs. This is outside this PR's scope and not a defect in it.
- The retry and exhaustion paths have only been tested with the author's mocked harness. In CI at aa970136, every `retry_download` call succeeded on attempt 1/3 (observed in the test_unit (3.11) log: the whole setup took about 85s, 13:21:41 => 13:23:06 UTC).
- A mirror that stays below about 0.9 MB/s now fails setup after three 5-minute attempts instead of finishing slowly. Downloaded `.deb`s and apt partials persist across attempts, so progress carries over (inferred from apt's archive cache behaviour, not executed). The PR body states this trade ("does not guarantee a faster mirror").

## Findings

None. No blocker or concern survived verification.

## Dismissed

- `--no-download` install could miss archives fetched by the `--download-only` step: dismissed. It is the same package list against the same index, nothing clears `/var/cache/apt/archives` between the two calls, and CI installed successfully on every job.
- `timeout` could kill dpkg mid-install: dismissed. `timeout` wraps only `update`, `--download-only install` and `add-apt-repository --no-update`. The dpkg-running install at `scripts/ci/install-system-deps.sh:30` is outside it.
- The first `apt-get update` now uses `APT::Update::Error-Mode=any`, which is stricter than before: dismissed. On base, the post-PPA update already ran with the same flag against the same sources, so the job already failed on any broken source.
- `set -u` with no script args (`"$@"` at line 38): dismissed. An empty `"$@"` is safe under `set -u` in bash, and the no-arg callers (test_unit, extras) passed.
- `add-apt-repository --no-update` may not be supported on the runner: dismissed. CI logs show it accepted on ubuntu-24.04.
- Moving apt out of the ingest job's `Test (end-to-end)` step loses that step's env: dismissed. None of those secrets were used by apt, and the ingest job passed.
- Version 0.27.13 could collide: dismissed. `origin/main` is at 0.27.12 and the collision leg found no pairs.
- Blast-radius rationale "privileged script from the repo in jobs with secrets": dismissed as a finding. The same sudo apt commands ran inline before, and no new secret exposure.

## What each leg said

- **blast-radius:** 3/5. One shared script now gates three CI jobs, and a bug there blocks every PR's CI but no shipped code. The rationale named the 5-minute caps and the download/install split as the risks; both are covered under Notes and Dismissed.
- **cheshire:** 0 flags, complexity COMPLEX, no slop. It ran without AGENTS.md, so the briefing is ungrounded. Briefing: /Users/pk/.cheshire/briefings/Unstructured-IO-unstructured-4514.md
- **collisions (review-graph):** ran, `complete: true`, 9 PRs checked, 0 pairs, 4/25 gh calls. Skipped drafts unstructured #3605 and #4521 (https://github.com/Unstructured-IO/unstructured/pull/3605, https://github.com/Unstructured-IO/unstructured/pull/4521).
- **fable (strong-review, report-only, no PR marker):** SAFE TO MERGE, no actionable findings. It confirmed CI at aa970136, the 262 MB fetch in 11s, the diffstat passthrough and tesseract 5.5.1. It argued the candidates in Dismissed and dropped them. It flagged the three unmigrated workflows as an adjacent issue (relayed as a Note).
- **codex (`codex exec review --base origin/main`):** no actionable regression. The shared script preserves each job's package set.
- **ruff:** pass. The only changed .py file is `unstructured/__version__.py`. Local `shellcheck` on the new script also passes, and so do CI `shellcheck`/`shfmt`.
- **CI (`gh pr checks`, read 14:47 UTC 2026-10-02):** every check passes. `claude` is skipped. Nothing is queued or failing.
- **Not run:** the strong-review gpt-pro/oracle leg (it needs a browser on a shared CDP port and is unavailable unattended), cubic (disabled for this run), test-critic and marmalade (outside this run's scope; the PR touches no test files).

## Not covered

- No live run of the timeout, retry or exhaustion path. Fable's suggested check is the script in `ubuntu:24.04` behind a blackholed apt proxy, expecting a non-zero exit after three attempts within about 17 minutes.
- The gpt-pro second-model leg of strong-review, and cubic.
- Whether the change actually reduces CI tail latency over time. The PR does not claim it does.

@CyMule
CyMule added this pull request to the merge queue Oct 2, 2026
Merged via the queue into main with commit 727db66 Oct 2, 2026
57 checks passed
@CyMule
CyMule deleted the fix/ci-apt-download-timeouts branch October 2, 2026 15:29
rohan-patnaik pushed a commit to rohan-patnaik/unstructured that referenced this pull request Oct 6, 2026
…d-IO#4521)

## What
Test, license, changelog, and lint jobs start together instead of
waiting on each other.

```
Before: setup (44–68s) → lint (45–68s) → no-extras tests → extras tests
After:  setup ┐
        lint  ├─ all start immediately
        tests ┘
```

- `setup` now only checks that `uv.lock` is current, and lint installs
just Ruff instead of every ML/connector extra.
- System package setup retains bounded download retries and offline
installation from Unstructured-IO#4514. Only the jobs that need them install packages:
OCR/Poppler goes to the `pdf-image` extra, and LibreOffice to the
`docx`/`pptx` extras (legacy `.doc`/`.ppt` conversion).
- A new push cancels the PR's older CI run.

Every job name, Python/extra matrix entry, test command, and coverage
gate is unchanged.

## Why
In the [baseline
run](https://github.com/Unstructured-IO/unstructured/actions/runs/36956464188),
tests could not start until setup and then lint had finished. Each of
the 24 extras jobs also spent 52–86s installing OCR and office tools
that most of them never use.

## Tradeoffs
- **Failed lint no longer stops tests.** A PR that fails lint still runs
the full test matrix on 8-core runners.
- **No shared cache warm-up.** `setup` used to fill the dependency cache
before other jobs started. When `uv.lock` changes, each job now installs
dependencies itself and they race to save the cache. Runs with a cache
hit are unaffected.
- **Version bump.** The version moves to `0.27.19-dev0` with a changelog
entry, as the repository's version check requires. Other open PRs that
bump the version will need the usual rebase.

## Validation
actionlint passes, and the lint-only sync, Ruff, and `uv lock --check`
pass locally. CI passes on this branch, including every extras suite
with the narrower system packages.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants