Container image packaging with periodic fetch worker #46
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/yunjin!46
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-40-container-packaging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements #40 per the findings/plan comment on the issue.
What
uv sync --frozen --no-dev --no-install-projectbuilds the locked dependency set into/app/.venvat its final path; the source tree,alembic.ini, andalembic/ship at/appbecauseinit_dbresolvesalembic.iniagainst the source root (parents[3]of the package file) — a plain wheel install would break that lookup. Non-root system user, OCI labels,/datavolume holding both the SQLite db (/data/yunjin.db) and the media directory (/data/media, derived by the app from the db path).container-entrypoint.sh):web(default): waitress servesyunjin.web.wsgi:appon$PORT(default 8080).worker: runsyunjin init(with bounded retry) once, then loopsyunjin fetch→yunjin aggregate→yunjin extract-tags→yunjin extract-mediaevery$YUNJIN_WORKER_INTERVALseconds (default 3600). Task failures are logged, not fatal; SIGTERM exits promptly (current child is killed).waitress~=3.0.2(pure-Python production WSGI server — the Flask counterpart to cang's single-process uvicorn).uv.lockandpylock.tomlregenerated in the same commit. waitress is ignored for deptry DEP002 (invoked by the entrypoint, never imported), mirroring cang's per-rule-ignores pattern.containerjob build-only buildah build on PRs/pushes/dispatch;publish-containerjob on (non-prerelease) release publication pushesregistry.marvin8.zone/marvin8/yunjin:{latest,<version>}after gitleaks + checks, mirroring cang. Adds therelease: types: [published]trigger (existing triggers unchanged).container-entrypoint.shand.containerignore(buildah/podman read.containerignorenatively); no docker-specific references anywhere.Verification
webserved the index with HTTP 200 on port 8080;workerran full init → fetch → aggregate → extract-tags → extract-media cycles and repeated on the interval; invalidYUNJIN_WORKER_INTERVALvalues (0,00,000,1h,abc,-5) fail fast; SIGTERM during a running task or the sleep phase exits immediately.uv run nox: all 9 sessions green (ty, ruff, complexipy, pysentry, pytest on 3.12/3.13/3.14, deptry, zaojun).containerjob (green on zhuiri via buildah).Notes for review
podman exec yunjin python -m yunjin extract-media --article-id 42(orpodman run --rm -v yunjin-data:/data <image> python -m yunjin ...— the entrypoint's default case execs arbitrary commands).Closes #40
- container job: build-only buildah build on PRs, pushes, and manual dispatch (skipped on release events so publish is the only build) - publish-container job: on release publication, build and push registry.marvin8.zone/marvin8/yunjin:{latest,<version>} after gitleaks and checks pass, mirroring cang's release pipeline - add release: types: [published] trigger so the publish job can runWuMing
Found 12 issue(s). See inline comments below.
@ -58,2 +60,4 @@uv run noxuv buildcontainer:config [LOW]
The new
containerbuild job has noneedsentry, so it runs in parallel with (and independently of)gitleaksandchecks. Unlikepublish-container, which gates onneeds: [gitleaks, checks], this job will happily build an image from a revision that fails linting/tests. If the image build is meant to be part of CI validation, addneeds: [gitleaks, checks](or otherwise constrain it) for consistency with the publish job.@ -60,0 +65,4 @@timeout-minutes: 30if: github.event_name != 'release'steps:- uses: actions/checkout@v4security [LOW]
A08: Third-party CI action is referenced by the mutable tag
v4instead of a full commit SHA, which can be retargeted. Pinactions/checkoutto a verified commit SHA.security [LOW]
A08: Third-party CI action is referenced by the mutable tag
v4instead of a full commit SHA, which can be retargeted. Pinactions/checkoutto a verified commit SHA.@ -60,0 +97,4 @@- name: Install buildahrun: apt-get update -qq && apt-get install -y -qq podman buildah- name: Login to registryrun: buildah login --username "${{ secrets.registry_user }}" --password "${{ secrets.registry_password }}" registry.marvin8.zoneconfig [MEDIUM]
The registry password is passed directly as a command-line argument (
--password "${{ secrets.registry_password }}"). Command lines are visible to other processes on the runner (and can end up in shell traces/logs). Use stdin instead, e.g.printf '%s' "${{ secrets.registry_password }}" | buildah login --username "${{ secrets.registry_user }}" --password-stdin registry.marvin8.zone, or pass the credentials via environment variables.security [MEDIUM]
A02: The registry password is passed on the command line, which can expose it via process listings or CI logs. Use
buildah login --password-stdinor a credentials file instead.@ -60,0 +118,4 @@--annotation "org.opencontainers.image.url=https://forge.marvin8.zone/marvin8/yunjin" \--annotation "org.opencontainers.image.revision=${REVISION}" \--annotation "org.opencontainers.image.version=${VERSION}" \-t registry.marvin8.zone/marvin8/yunjin:latest \config [MEDIUM]
release: types: [published](line 7) fires for pre-releases as well, and this job unconditionally pushes the movinglatesttag. Publishing a pre-release would therefore overwrite thelatestimage with a pre-release build. Guard the job/tag with the pre-release flag, e.g.if: github.event_name == 'release' && !github.event.release.prerelease, and only apply thelatesttag on non-prerelease releases.@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS buildersecurity [LOW]
A08: The builder base image is referenced by a mutable tag rather than an immutable digest. A retagged or compromised image could be pulled; pin
python:3.12-slim-bookwormwith a@sha256:...digest.@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSIONshell [MEDIUM]
ARG UV_VERSIONis declared with no default value and is interpolated directly into an image reference on line 4 (COPY --from=ghcr.io/astral-sh/uv:${UV_VERSION}). Any build that does not explicitly pass--build-arg UV_VERSION=...(localdocker build, a new CI job, a contributor reproducing the build) collapses the reference toghcr.io/astral-sh/uv:and fails with an invalid-reference error, or silently resolves to whatever floating tag is supplied. Supply a pinned default so the Containerfile is self-contained and the tool version is recorded in the repo, e.g.ARG UV_VERSION=0.x.y@sha256:<digest>, and keep passing it explicitly from CI.@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSIONCOPY --from=ghcr.io/astral-sh/uv:${UV_VERSION} /uv /usr/local/bin/uvsecurity [LOW]
A08: The uv tool image is downloaded by a mutable version tag without digest pinning, allowing tag substitution in the supply chain. Pin
ghcr.io/astral-sh/uvto a digest.@ -0,0 +15,4 @@COPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectFROM python:3.12-slim-bookwormsecurity [LOW]
A08: The runtime base image is referenced by a mutable tag rather than an immutable digest. A retagged or compromised image could be pulled; pin
python:3.12-slim-bookwormwith a@sha256:...digest.@ -0,0 +20,4 @@case "${1:-web}" inweb): "${PORT:=8080}"exec waitress-serve --host=0.0.0.0 --port="${PORT}" yunjin.web.wsgi:appshell [MEDIUM]
The
webentry point execs waitress without ever initializing the database, whileworkerrunsyunjin init(line 28). The application resolves its SQLite path against cwd (/data) and ships alembic for schema creation, so on a fresh/datavolume — or whenever the web container is started before the worker — it will serve against a missing/empty database and fail on the first query.alembic upgrade headis idempotent, so runpython -m yunjin initbefore the exec here as well, or explicitly document and enforce that the worker must be started first.@ -0,0 +23,4 @@exec waitress-serve --host=0.0.0.0 --port="${PORT}" yunjin.web.wsgi:app;;worker): "${YUNJIN_WORKER_INTERVAL:=3600}"shell [MEDIUM]
YUNJIN_WORKER_INTERVALis taken from the environment without validation. If it is non-numeric (e.g.1h,60s) or0,sleepfails immediately; because the sleep is backgrounded and its exit status is discarded bywait "$!" || :(lines 37-38),set -enever fires and the loop spins with no delay, running fetch/aggregate/extract-tags and the LLM calls back-to-back. Validate the value before entering the loop, e.g.case "$YUNJIN_WORKER_INTERVAL" in ''|*[!0-9]*) echo "invalid YUNJIN_WORKER_INTERVAL" >&2; exit 1;; esacplus a check that it is greater than 0.WuMing
Found 2 issue(s). See inline comments below.
@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS buildersecurity [LOW]
A08: The base image is referenced by the mutable tag python:3.12-slim-bookworm without a digest pin. A compromised or updated upstream tag could inject malicious content into the final image. Pin the base image by digest (e.g. python:3.12-slim-bookworm@sha256:...) and verify integrity.
@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSION=0.12.18COPY --from=ghcr.io/astral-sh/uv:${UV_VERSION} /uv /usr/local/bin/uvsecurity [LOW]
A08: The uv build dependency is copied from a mutable image tag (ghcr.io/astral-sh/uv:${UV_VERSION}) with no digest or integrity verification. The UV_VERSION value is derived from pyproject.toml, so changes to that file control which image is executed during the build. Pin this image by digest and restrict UV_VERSION to an approved allow-list.
Containerfile✅ Fixed in commit
228c706—ARG UV_VERSIONnow defaults to0.12.18(matching theuv~=0.12.18dev-group pin in pyproject.toml); CI still passes it explicitly via the pyproject grep, so the recorded default and the CI-resolved value stay aligned.docker-entrypoint.sh✅ Fixed in commit
228c706— a POSIXcaseguard now rejects empty, non-numeric, or zero values with a clear stderr message andexit 1before the loop starts. Smoke-tested:YUNJIN_WORKER_INTERVAL=1hand=0both fail fast;=3loops normally..forgejo/workflows/ci.yml✅ Fixed in commit
228c706— thecontainerjob now hasneeds: [gitleaks, checks], so images are only built from revisions that pass linting/tests, consistent withpublish-container..forgejo/workflows/ci.yml✅ Fixed in commit
228c706—publish-containernow runs only ongithub.event_name == 'release' && !github.event.release.prerelease, so a pre-release can never overwritelatest..forgejo/workflows/ci.yml✅ Fixed in commit
228c706— registry login now pipes the password via stdin:printf '%s' ... | buildah login --username ... --password-stdin registry.marvin8.zone. This also covers the duplicate of this finding posted by the security agent (identical wuming marker, comment 3530)..forgejo/workflows/ci.yml✅ Same finding as comment 3525 (identical wuming marker) — fixed in commit
228c706viabuildah login --password-stdin; see the reply there.docker-entrypoint.sh🔴 Not actioned — the premise doesn't hold for this codebase.
webexecswaitress-serve yunjin.web.wsgi:app, and importingyunjin.web.wsgibuilds the app at module level (app = create_app()), which runsinit_db()— an idempotent alembicupgrade headplus default-user seed — before the server starts. Verified in the smoke test recorded in the PR description: web mode alone on a fresh empty directory created the database and schema on boot and served the index with HTTP 200 before any worker existed. There is no web/worker ordering constraint.Containerfile❓ Over to Marvin8 — repo-convention call, not actioned yet (covers comments 3531, 3532, 3533: builder base, runtime base, and uv tool image digest pinning). The Containerfile deliberately mirrors cang's — the stated template for this issue — which uses the same mutable references (
python:3.12-slim-bookworm,ghcr.io/astral-sh/uv:${UV_VERSION}) with no digest pinning. Digest-pinning yunjin alone would diverge from the sibling template and add manual bump discipline. If you want digest pinning, say the word and I'll add it here (and propose it for cang for parity)..forgejo/workflows/ci.yml❓ Over to Marvin8 — repo-convention call, not actioned yet (covers comments 3534 and 3535).
actions/checkout@v4(andmarvin8/wuming@main) pre-exist across this workflow and sibling repos (cang uses the same); this PR only added two more instances of the established pattern. Pinning to commit SHAs is a repo-wide (arguably fleet-wide) change beyond this PR's scope — happy to do a follow-up PR that pins everything if you want it.Duplicates of wave-1 findings (already escalated, awaiting Marvin8)
Containerfilebase image digest ·Containerfileuv image digestSame substance as the digest-pinning group answered in the wave-1 reply (new markers only because the diff changed):
Containerfileintentionally mirrors cang's mutable-tag layout; digest pinning is Marvin8's convention call. No further action.WuMing
Found 4 issue(s). See inline comments below.
@ -0,0 +15,4 @@COPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectFROM python:3.12-slim-bookwormshell [MEDIUM]
Both stages (this runtime stage and the builder on line 2) use the floating tag
python:3.12-slim-bookworm, so the resulting image is not reproducible and a silent upstream retag changes (or breaks) what you ship. Pin the base images by digest, e.g.FROM python:3.12-slim-bookworm@sha256:<digest>, and update the digest explicitly (Renovate/Dependabot) when you intend to move.@ -0,0 +25,4 @@worker): "${YUNJIN_WORKER_INTERVAL:=3600}"case "$YUNJIN_WORKER_INTERVAL" in''|*[!0-9]*|0)shell [LOW]
The validation rejects the literal
0and any non-digit, but not zero written with leading zeros:YUNJIN_WORKER_INTERVAL=00(or000) falls through, andsleep 00returns immediately, turning the loop into a tight loop that runsfetch/aggregate/extract-tagsback to back with no delay. Validate numerically instead of by pattern, e.g. run the case to reject non-digits, then[ "$YUNJIN_WORKER_INTERVAL" -ge 1 ] || { ... exit 1; }(or strip leading zeros first).shell [LOW]
Under
set -euthis unguardedinitis fatal: any transient failure (DB locked, volume not yet writable, brief network error fetching migrations) exits the container and sends it into an orchestrator restart/crash-loop, even though the periodic tasks in the loop below are deliberately tolerated. Consider a small retry loop or logging the failure explicitly before exiting so the cause is obvious in logs; keep it distinct from the logged-but-continuing task failures.@ -0,0 +35,4 @@while :; dofor task in fetch aggregate extract-tags; doecho "yunjin worker: running ${task}"if ! python -m yunjin "${task}"; thenshell [LOW]
SIGTERM/SIGINT is only honoured between tasks: POSIX defers trap execution while a foreground command is running, so a long
yunjin fetch/aggregatekeeps the shell alive until it finishes. In an orchestrator with a 10s default grace period the container is then SIGKILLed at PID 1, which can interrupt SQLite mid-write. Run the task in the background (python -m yunjin "${task}" & pid=$!), record the PID, and in the trapkill "$pid"before exiting, thenwaitfor it.docker-entrypoint.sh✅ Fixed in commit
8907c19— the digit-pattern case is now followed by a numeric check (-lt 1rejects it), so00,000, and0all fail fast with the same message. Smoke-tested:0,00,000,1h,abc,-5all exit 1.docker-entrypoint.sh✅ Fixed in commit
8907c19— init now retries 3 times with a 5s pause, logging each failed attempt to stderr, and only exits non-zero after the final attempt, keeping persistent misconfiguration fail-fast while tolerating transient locks/hiccups.docker-entrypoint.sh✅ Fixed in commit
8907c19— loop tasks now run in the background withwait, and the TERM/INT trap kills the current child (taskorsleep) before exiting, so shutdown is prompt instead of deferred to task completion. Smoke-tested with SIGTERM during both a runningfetchand the sleep phase — exits immediately, exit code 0.Duplicate of wave-1 finding (already escalated, awaiting Marvin8)
Containerfilebase image digestSame substance as the digest-pinning group answered in the wave-1 reply (new marker only because the diff changed): both stages intentionally mirror cang's mutable-tag layout; digest pinning is Marvin8's convention call. No further action.
8907c198b6410373153eWuMing
Found 9 issue(s). See inline comments below.
@ -60,0 +66,4 @@needs: [gitleaks, checks]if: github.event_name != 'release'steps:- uses: actions/checkout@v4security [MEDIUM]
A08: actions/checkout@v4 is pinned only to a mutable major tag, not a full commit SHA. This job runs with registry credentials, so a compromised or malicious update to the v4 tag could exfiltrate those secrets. Pin the action to a reviewed commit SHA and use Action pinning.
@ -60,0 +73,4 @@env:REVISION: ${{ github.sha }}run: |UV_VERSION=$(grep -oP '"uv~=\K[0-9]+\.[0-9]+\.[0-9]+' pyproject.toml)config [LOW]
UV_VERSIONis scraped withgrep -oP '"uv~=\K...'. If the pin in pyproject.toml is ever written differently (uv==0.12.19, single quotes, spacing), grep exits non-zero and, because run steps execute underbash -e, the step aborts with a confusing error; if the failure is tolerated the build silently receives an empty--build-arg UV_VERSION. Add a fallback/validation (UV_VERSION=$(grep -oP ... || true)plus an explicit non-empty check) and reuse one helper instead of duplicating the same grep in the publish job at line 108.@ -60,0 +98,4 @@- name: Install buildahrun: apt-get update -qq && apt-get install -y -qq podman buildah- name: Login to registryrun: printf '%s' "${{ secrets.registry_password }}" | buildah login --username "${{ secrets.registry_user }}" --password-stdin registry.marvin8.zoneconfig [MEDIUM]
Registry credentials are interpolated directly into the shell script with
${{ secrets.* }}. Any quote, backtick or$(...)in the password will break the command or allow shell injection on the self-hosted runner. Pass the values throughenv:and reference"$REGISTRY_USER"/"$REGISTRY_PASSWORD"instead. Also,buildah loginwrites the credentials into the runner's containers auth file, which persists after the job on a self-hosted runner such aszhuiri; finish the job withbuildah logout registry.marvin8.zoneor use a throwaway--authfileunder$RUNNER_TEMP.@ -60,0 +111,4 @@--build-arg REVISION="${REVISION}" \--build-arg VERSION="${VERSION}" \--build-arg UV_VERSION="${UV_VERSION}" \--annotation "org.opencontainers.image.source=https://forge.marvin8.zone/marvin8/yunjin" \config [LOW]
This is the second verbatim copy of the seven
--annotationflags already present in thecontainerjob (lines 81-87); onlyorg.opencontainers.image.versiondiffers. The two lists will drift the next time metadata changes. Factor the shared annotations into one variable/script used by both build steps, or set them asLABELs in the Containerfile and pass only the per-build values (REVISION,VERSION) as build args.@ -0,0 +16,4 @@# Stop promptly on SIGTERM/SIGINT: kill the current background child (task# or sleep), since POSIX defers traps until the foreground wait returns.trap 'test -n "${child_pid:-}" && kill "$child_pid" 2>/dev/null; exit 0' TERM INTshell [MEDIUM]
The SIGTERM/SIGINT handler kills the current child and then calls
exit 0immediately without waiting for the child to exit, so in-flight work (e.g.fetchpartway through writing SQLite,aggregatemid-transaction) is interrupted and the container reports a clean shutdown, hiding the truncation from the supervisor and return-code based alerting. It also only signals the direct child, not grandchildren. Reap the child after signalling, e.g.trap 'if [ -n "${child_pid:-}" ]; then kill "$child_pid" 2>/dev/null; wait "$child_pid" 2>/dev/null; fi; exit 0' TERM INT, and consider forwarding the signal to the whole process group (kill -TERM 0) so spawned subprocesses also stop.@ -0,0 +21,4 @@case "${1:-web}" inweb): "${PORT:=8080}"exec waitress-serve --host=0.0.0.0 --port="${PORT}" yunjin.web.wsgi:appshell [LOW]
$PORTis interpolated into the waitress command line without any validation, unlikeYUNJIN_WORKER_INTERVALa few lines below. A non-numeric or out-of-range value makes the container fail at startup with an opaque waitress usage error. Validate it beforeexec(reject anything not matching[1-9][0-9]*and not within 1-65535) so misconfiguration fails fast with a clear message.@ -0,0 +62,4 @@child_pid=python -m yunjin "${task}" &child_pid=$!if ! wait "$child_pid"; thenshell [LOW]
Every task failure is swallowed with only a log line and the loop continues forever, so a permanently failing worker (expired credentials, unreachable feeds, schema drift) still looks healthy to Docker/orchestrators and never triggers a restart. Consider tracking consecutive failures and exiting non-zero after a threshold, or writing a heartbeat/health file that a HEALTHCHECK can inspect.
@ -447,3 +447,3 @@[[packages]]name = "filelock"version = "3.32.6"version = "4.0.3"config [LOW]
filelockjumps from 3.32.6 to 4.0.3, a major-version change of a transitive dependency (virtualenv/nox toolchain) pulled in by the same wholesale lock regeneration. Verify that no dependent package constrainsfilelock<4(otherwise the lock is unbuildable for other environments) and consider splitting the unrelated upgrade out of this container PR.@ -1403,3 +1347,3 @@[[packages]]name = "sqlalchemy"version = "2.0.52"version = "2.1.0"config [MEDIUM]
SQLAlchemy is bumped 2.0.52 -> 2.1.0, a major runtime-dependency upgrade bundled into a container-packaging change (the same regeneration also drops the conditional
greenletpin). Major runtime bumps deserve their own PR/verification: confirm the application's ORM usage and migrations are compatible with 2.1 and that removinggreenletis intentional, rather than shipping it silently inside a lock refresh.WuMing
Found 7 issue(s). See inline comments below.
@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS buildershell [MEDIUM]
Both stages reference base images by mutable tag (
python:3.12-slim-bookwormhere and on line 18,ghcr.io/astral-sh/uv:${UV_VERSION}on line 4). Tags can be re-pointed, so a rebuild may silently pick up different (or compromised) content and the image is not reproducible. Pin each to an immutable digest (FROM python:3.12-slim-bookworm@sha256:<digest> AS builder) and bump the digest deliberately, e.g. via a Renovate/Dependabot rule.@ -0,0 +1,58 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSION=0.12.18COPY --from=ghcr.io/astral-sh/uv:${UV_VERSION} /uv /usr/local/bin/uvsecurity [MEDIUM]
A08: The uv binary is copied from a mutable image tag without an immutable digest or checksum verification. If the tag is republished, an attacker can control the build tool and poison the resulting image. Pin this reference by digest, e.g. ghcr.io/astral-sh/uv@sha256:...
@ -0,0 +15,4 @@COPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectFROM python:3.12-slim-bookwormsecurity [MEDIUM]
A08: The runtime base image is referenced by the floating tag python:3.12-slim-bookworm rather than an immutable digest. This permits the tag to be republished with malicious content without detection. Pin the base image by digest (also applies to the builder stage at line 2).
@ -0,0 +17,4 @@FROM python:3.12-slim-bookwormRUN useradd --system --no-create-home --shell /usr/sbin/nologin yunjinshell [LOW]
useradd --systemwithout--uid/--gidallocates a UID/GID from the system range that can change between base-image rebuilds, so the UID baked into /data is not predictable. Operators who bind-mount a host directory over /data then cannot chown it correctly and the worker/web will fail to write the SQLite db. Use an explicit, high, non-overlapping pair, e.g.RUN useradd --system --uid 10001 --gid 10001 --no-create-home --shell /usr/sbin/nologin yunjin, and keep it documented.@ -0,0 +17,4 @@# Stop promptly on SIGTERM/SIGINT: kill the current background child (task# or sleep), since POSIX defers traps until the foreground wait returns.trap 'test -n "${child_pid:-}" && kill "$child_pid" 2>/dev/null; exit 0' TERM INTshell [LOW]
The handler kills the current child but never
waits for it, so the script exits (status 0) while ayunjin fetch/aggregatetask may still be running/aborting. In addition, underset -euthetest ... && kill ...AND-list is the last command of that list; ifkillfails (child already gone) the list returns non-zero andset -eaborts the trap beforeexit 0is reached. Use an explicit block, e.g.trap 'if [ -n "${child_pid:-}" ]; then kill "$child_pid" 2>/dev/null || true; wait "$child_pid" 2>/dev/null || true; fi; exit 0' TERM INT.@ -0,0 +22,4 @@case "${1:-web}" inweb): "${PORT:=8080}"exec waitress-serve --host=0.0.0.0 --port="${PORT}" yunjin.web.wsgi:appshell [LOW]
Only the
workerbranch runsyunjin init; the defaultwebbranch goes straight to waitress. With a fresh/datavolume (e.g.docker run <image>, which uses the defaultwebCMD, or a web-only deployment) the app will start against an unmigrated/empty SQLite database and fail on the first query, with no indication that a worker container must run first. Consider running the same retryinginitbefore serving in thewebbranch, or document/enforce the start order (and add a compose/healthcheck dependency).@ -0,0 +58,4 @@exit 1fiwhile :; dofor task in fetch aggregate extract-tags extract-media; doshell [MEDIUM]
The loop runs
extract-mediaevery interval, which contradicts the PR description ("extract-media is deliberately not in the loop (heavy network use each interval)") and the stated verification run (init -> fetch -> aggregate -> extract-tags). As written, every container will hit the origin sites for media downloads on each tick, which is exactly the behaviour the PR says was excluded. Dropextract-mediafrom this list (and fix the header comment on lines 7-8) or make it an opt-in via an env var such as${YUNJIN_WORKER_TASKS:-fetch aggregate extract-tags}..forgejo/workflows/ci.yml✅ Fixed in commit
74d4a27— login step now passesREGISTRY_USER/REGISTRY_PASSWORDthroughenv:and references them as quoted shell variables, so special characters in the password can't break or inject into the command..forgejo/workflows/ci.yml✅ Fixed in commit
74d4a27— both build jobs now tolerate the grep failure and abort with an explicitunable to determine uv version from pyproject.tomlmessage instead of passing a silent empty--build-arg. (Note the defaultARG UV_VERSION=0.12.18in the Containerfile still covers plain local builds.).forgejo/workflows/ci.yml✅ Fixed in commit
74d4a27by taking your second option: the redundant--annotationlists are dropped from both jobs — the ContainerfileLABELs already carry all OCI metadata includingrevision/versionvia build args, so the jobs now pass only their per-build--build-args.docker-entrypoint.sh✅ Fixed in commit
74d4a27— the trap now (a) guards thekillwith|| :so a vanished child can't abort the trap throughset -ebeforeexit 0, and (b)waits for the killed child before exiting. (Path renamed tocontainer-entrypoint.shin the meantime.)container-entrypoint.sh✅ Same finding as comment 3586 (both the missing reap and the
set -eAND-list hazard) — fixed in commit74d4a27; see the reply there.container-entrypoint.sh✅ Fixed in commit
74d4a27—PORTis validated beforeexec(digits only, then range 1–65535) with the same fail-fast message style asYUNJIN_WORKER_INTERVAL. Smoke-tested:abc,0,70000,-1all exit 1; a valid port boots and serves.Containerfile✅ Fixed in commit
74d4a27—yunjinnow gets an explicit uid/gid 1000 (groupadd --system --gid 1000+useradd --system --uid 1000), so/dataownership is stable across base-image rebuilds and predictable for bind mounts.pylock.toml🔴 Not actioned — the lock regeneration is this repo's prescribed workflow: the AGENTS tooling table mandates
uv sync -U --all-groupsafter any dependency change, and-Ure-resolves the whole lock (same for thefilelockjump in comment 3585). Compatibility is verified, not assumed: the full suite — including the alembic migration path and every nox session that exercises virtualenv/filelock — passes on 3.12, 3.13, and 3.14 in CI. If you'd rather scope lock churn out of feature PRs, that's a workflow convention change to make repo-wide, not here.pylock.toml🔴 Same answer as comment 3584: wholesale
-Ure-resolution is the repo's prescribed post-dependency-change workflow, andfilelock4.0.3 is exercised by every nox/virtualenv session in the green CI run. No action.container-entrypoint.sh🔴 Intentional and owner-directed: after the original review rounds, Marvin8 asked for extract-media to be added to the loop (it completes the issue goal of keeping articles, images, and metadata up to date). Re-runs are incremental — the extractor skips media whose URL is already stored per article (
media_url_exists) — and outbound requests remain gated byYUNJIN_HTTP_MIN_HOST_INTERVAL. The PR body was updated to match.container-entrypoint.sh❓ Over to Marvin8 — the logged-not-fatal behavior is what the issue plan specifies, and exiting on failure would reintroduce exactly the crash-looping an earlier finding asked to avoid. A consecutive-failure threshold (e.g. exit after N full cycles where every task failed) or a heartbeat file for a HEALTHCHECK are both easy to add if you want them — say which, or leave as-is.
WuMing
Found 7 issue(s). See inline comments below.
@ -60,0 +66,4 @@needs: [gitleaks, checks]if: github.event_name != 'release'steps:- uses: actions/checkout@v4security [LOW]
GitHub Action is referenced by mutable version tag 'v4' rather than a commit SHA. A compromised action tag could execute malicious code in the CI pipeline. Pin actions/checkout to the full-length commit SHA (A08).
@ -60,0 +91,4 @@needs: [gitleaks, checks]if: github.event_name == 'release' && !github.event.release.prereleasesteps:- uses: actions/checkout@v4security [LOW]
GitHub Action is referenced by mutable version tag 'v4' rather than a commit SHA. A compromised action tag could execute malicious code in the release publishing pipeline. Pin actions/checkout to the full-length commit SHA (A08).
@ -0,0 +1,59 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS buildershell [LOW]
Base images are pinned only by mutable tag, not digest.
python:3.12-slim-bookworm(here and again at line 18) andghcr.io/astral-sh/uv:${UV_VERSION}(line 4) can be re-tagged upstream, so builds are not reproducible and a compromised upstream tag would be pulled silently. Pin to a digest (e.g.python:3.12-slim-bookworm@sha256:...) or, at minimum, pin the uv image by digest and document the update process.security [MEDIUM]
Base image is referenced by a mutable tag without a content digest (also repeated for the runtime stage at line 18). A compromised registry or upstream tag update could silently inject malicious code into the image. Pin the image to a digest, e.g. python:3.12-slim-bookworm@sha256:... (A08).
@ -0,0 +1,59 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSION=0.12.18COPY --from=ghcr.io/astral-sh/uv:${UV_VERSION} /uv /usr/local/bin/uvsecurity [MEDIUM]
The uv binary is copied from a mutable container tag without a digest. This provides no integrity guarantee that the fetched binary is the expected, untampered artifact. Pin to a digest, e.g. ghcr.io/astral-sh/uv@sha256: (A08).
@ -0,0 +18,4 @@# Stop promptly on SIGTERM/SIGINT: kill the current background child (task# or sleep), since POSIX defers traps until the foreground wait returns, then# reap it before exiting so shutdown status reflects the child's termination.trap 'if [ -n "${child_pid:-}" ]; then kill "$child_pid" 2>/dev/null || :; wait "$child_pid" 2>/dev/null || :; fi; exit 0' TERM INTshell [LOW]
The TERM/INT trap always
exit 0, so a container stopped while a fetch/aggregate task was killed mid-run still reports a successful exit status; the comment above it ('shutdown status reflects the child's termination') is not what the code does. Capture the child's status fromwaitand propagate it (e.g.exit $?or[ $st -eq 0 ] || exit 143) if you want the runtime/podman stopto surface a killed task. Also note only the direct child is killed, not any grandchildren it may spawn.@ -0,0 +55,4 @@# worker; retry briefly, then fail loudly on persistent errors.init_ok=0for attempt in 1 2 3; doif python -m yunjin init; thenshell [MEDIUM]
This init runs as a foreground command, and POSIX shells defer trap handlers until the currently executing foreground command returns. A slow/hung
yunjin init(or the foregroundsleep 5on line 64) therefore blocks SIGTERM/SIGINT handling entirely, so the container will be SIGKILLed at the stop grace period instead of shutting down promptly, contradicting the 'SIGTERM exits promptly' design claim and risking a partially migrated SQLite db. Run init in the background andwaiton it (like the task loop does) or wrap it with a bounded timeout so the trap can fire.Duplicates of already-answered findings (waves 4/5)
Pinning:
ci.ymlcheckout · pinning:Containerfilebuilder · pinning: uv image · pinning: runtime base · web-init (container-entrypoint.sh)create_app()runs the idempotentinit_db()at import time, before waitress accepts a request; verified on a fresh volume. No worker-first ordering requirement exists.No further action.
container-entrypoint.sh✅ Fixed in commit
8778a04— the init retry loop moved into aninit_databasefunction run as a background child under the samechild_pidmechanism as worker tasks, so a slow/hung init (or its retrysleep) is killed promptly on SIGTERM/SIGINT instead of deferring the trap. Verified against a read-only/data: SIGTERM during the retry sleep exits immediately; without a signal, three attempts still end in a logged give-up with exit 1.container-entrypoint.sh✅ Fixed the comment in commit
8778a04— you're right that the code doesn't propagate the child's status. That's deliberate: exiting non-zero on a signal-driven stop is conventionally treated by orchestrators as a crash, and the worker's semantics are at-least-once (an interrupted task simply re-runs on the next start). The comment now states this explicitly instead of implying propagation. If you want status propagation instead, it's a one-line change — say so.container-entrypoint.sh🔴 Resolved by the repo owner: a permanently failing worker should just log and keep running — no consecutive-failure threshold and no stop. The logged-not-fatal behavior is the intended design; closing this.
Duplicates of the deferred pinning findings (waves 4–6)
Containerfilebase digests ·Containerfileruntime base ·Containerfileuv image ·ci.ymlcheckout (build job) ·ci.ymlcheckout (publish job)Same substance as the digest/SHA-pinning findings escalated in wave 1 and answered in the wave-1 reply: the repo owner has decided to defer all image-digest and action-SHA pinning to a later effort. No action in this PR; these markers are tracked so repeats are recognized as duplicates.
WuMing
Found 5 issue(s). See inline comments below.
@ -0,0 +15,4 @@COPY pyproject.toml uv.lock ./RUN uv sync --frozen --no-dev --no-install-projectFROM python:3.12-slim-bookwormshell [LOW]
Neither the runtime base image (line 18) nor the builder base (line 2) nor the uv image (line 4) is pinned to an immutable digest, only to mutable tags (
python:3.12-slim-bookworm,ghcr.io/astral-sh/uv:${UV_VERSION}). Tags can be re-pointed upstream, so the same commit can build different images and a compromised/retagged upstream would be pulled silently. Pin by digest, e.g.FROM python:3.12-slim-bookworm@sha256:<digest>andCOPY --from=ghcr.io/astral-sh/uv@sha256:<digest> /uv ..., and update them deliberately (Dependabot/Renovate).@ -0,0 +43,4 @@LABEL org.opencontainers.image.title="yunjin"LABEL org.opencontainers.image.url="https://forge.marvin8.zone/marvin8/yunjin"ARG REVISIONshell [LOW]
ARG REVISION/ARG VERSIONhave no defaults, so labels built into the image via lines 48-49 (org.opencontainers.image.revision=$REVISION,...version=$VERSION) are empty strings whenever the build is invoked without--build-arg— which is the normal path for local builds and for the CIcontainerjob if it does not pass them. Give them sentinel defaults (e.g.ARG REVISION=unknown,ARG VERSION=0.0.0) or fail the build when they are unset, so the OCI metadata is never blank.@ -0,0 +20,4 @@# then reap it. Signal-driven shutdown deliberately reports success (the# conventional container stop status); interrupted work simply re-runs on# the next start.trap 'if [ -n "${child_pid:-}" ]; then kill "$child_pid" 2>/dev/null || :; wait "$child_pid" 2>/dev/null || :; fi; exit 0' TERM INTshell [MEDIUM]
The TERM/INT trap only sends SIGTERM to the direct child and then blocks in
waitwith no timeout or escalation to SIGKILL. If a long-running task (e.g.yunjin fetchmid-network-call) ignores or is slow to handle SIGTERM, PID 1 never exits and the container hangs until the orchestrator's grace period expires and it is SIGKILLed — losing the graceful-shutdown behaviour the comment promises. Add a bounded wait pluskill -KILLfallback (e.g. kill the child's process group, sleep N seconds, thenkill -KILL), or use an init such astini.@ -0,0 +52,4 @@echo "yunjin: invalid PORT: '${PORT}' (integer 1-65535 required)" >&2exit 1fiexec waitress-serve --host=0.0.0.0 --port="${PORT}" yunjin.web.wsgi:appshell [MEDIUM]
Only the
workerbranch runsyunjin init(the alembic upgrade that creates/migrates the schema); thewebbranch execs waitress immediately. Starting the image with the defaultwebentry point against a fresh/datavolume (or on a first rollout where the worker has not run yet) serves the app against a missing/out-of-date database, producing 500s until a worker happens to run init. Run the same bounded init beforeexec waitress-serve(it is idempotent), or gate the web start on the schema and document that a worker must run first.@ -0,0 +90,4 @@echo "yunjin worker: sleeping ${YUNJIN_WORKER_INTERVAL}s"sleep "${YUNJIN_WORKER_INTERVAL}" &child_pid=$!wait "$child_pid" || :shell [MEDIUM]
The result of the sleep wait is discarded (
|| :), so any failure ofsleep(e.g. a valuetest -gt/-ltaccepted but coreutils rejects as out of range, or atestthat mis-parses a leading-zero value like08as invalid octal and therefore skips the-lt 1guard) is turned into a zero-length interval. The outerwhile :then re-runs fetch → aggregate → extract-tags → extract-media back-to-back in a tight loop, hammering the feeds and the SQLite database with no delay and no log. Check the sleep status and exit/log on failure (or validate the value with an explicit range/leading-zero check) instead of swallowing it.container-entrypoint.sh✅ Fixed in commit
516500bon both fronts: intervals longer than 9 digits are now rejected at validation (values that would overflow the shell's integer comparison could otherwise bypass the range guard), and the sleep wait logs asleep failed or was interrupted; resumingline instead of discarding the status. Note the octal half of the finding doesn't apply: POSIXtestparses decimal, so[ "08" -lt 1 ]compares 8 — smoke-tested,08is accepted and sleeps 8 seconds.Containerfile✅ Fixed in commit
516500b— both ARGs now default todev, so local builds carry honest labels instead of empty strings; CI overrides both with the real revision/version.container-entrypoint.sh🔴 Not actioned — the tasks are CPython processes, and Python installs no SIGTERM handler by default: the signal takes the OS default disposition, which terminates the process immediately (even mid-syscall); there is no Python-level path that could ignore or defer it, so a kill escalation would be dead code. (SIGINT differs — Python does default-handle it — but we never send SIGINT for container stop.)
container-entrypoint.sh🔴 Standing declined finding, third re-raise — see the wave-1 reply with evidence:
waitress-serve yunjin.web.wsgi:appimportsyunjin.web.wsgi, which builds the app at module level viacreate_app(); that call runs the idempotentinit_db()(alembic upgrade to head + default-user seed) before the server accepts a connection. Verified on a fresh empty volume: web alone migrates the schema and serves HTTP 200. No worker-first ordering exists.Duplicate of the deferred pinning finding (wave 7)
Containerfileimage digestsSame substance as the digest/SHA-pinning group escalated in wave 1 (wave-1 reply, deferral consolidated here): the repo owner has deferred image-digest and action-SHA pinning to a later effort. No action in this PR.
WuMing
Found 2 issue(s). See inline comments below.
@ -0,0 +1,59 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS buildersecurity [LOW]
A08: The base Python image is referenced by a mutable tag rather than pinning to a digest. An attacker who can influence the tag or registry could cause the build to use a tampered base image. Pin to a specific digest, e.g. FROM python:3.12-slim-bookworm@sha256:...
@ -0,0 +1,59 @@# SPDX-License-Identifier: AGPL-3.0-or-laterFROM python:3.12-slim-bookworm AS builderARG UV_VERSION=0.12.18COPY --from=ghcr.io/astral-sh/uv:${UV_VERSION} /uv /usr/local/bin/uvsecurity [LOW]
A08: The uv binary is copied from a remote container image referenced only by version tag, with no digest or checksum verification. This is a downloaded build artefact without an integrity check. Pin to an immutable digest or verify the binary checksum after copying.
Duplicates of the deferred pinning finding (wave 8)
Containerfilebase image ·Containerfileuv imageSame substance as the digest/SHA-pinning group answered in the wave-1 reply and the deferral consolidation: the repo owner has deferred image-digest and action-SHA pinning to a later effort. No action in this PR.
agent-pi referenced this pull request2026-10-04 19:35:16 +00:00
agent-pi referenced this pull request2026-10-04 19:35:17 +00:00
agent-pi referenced this pull request2026-10-04 19:35:18 +00:00
agent-pi referenced this pull request2026-10-04 19:35:19 +00:00
agent-pi referenced this pull request2026-10-04 19:35:20 +00:00
agent-pi referenced this pull request2026-10-04 19:35:20 +00:00
agent-pi referenced this pull request2026-10-04 19:35:23 +00:00
agent-pi referenced this pull request2026-10-04 19:35:23 +00:00
agent-pi referenced this pull request2026-10-04 19:35:24 +00:00
agent-pi referenced this pull request2026-10-04 19:35:25 +00:00
agent-pi referenced this pull request2026-10-04 19:35:26 +00:00
agent-pi referenced this pull request2026-10-04 19:35:27 +00:00
agent-pi referenced this pull request2026-10-04 19:35:27 +00:00
agent-pi referenced this pull request2026-10-04 19:35:28 +00:00
agent-pi referenced this pull request2026-10-04 19:35:29 +00:00
agent-pi referenced this pull request2026-10-04 19:35:30 +00:00
agent-pi referenced this pull request2026-10-04 19:35:30 +00:00
agent-pi referenced this pull request2026-10-04 19:35:31 +00:00
agent-pi referenced this pull request2026-10-04 19:35:32 +00:00
agent-pi referenced this pull request2026-10-04 19:35:33 +00:00
agent-pi referenced this pull request2026-10-04 19:35:33 +00:00
agent-pi referenced this pull request2026-10-04 19:35:34 +00:00
agent-pi referenced this pull request2026-10-04 19:35:34 +00:00
agent-pi referenced this pull request2026-10-04 19:35:35 +00:00
agent-pi referenced this pull request2026-10-04 19:35:35 +00:00
agent-pi referenced this pull request2026-10-04 19:35:36 +00:00
agent-pi referenced this pull request2026-10-04 19:35:36 +00:00
agent-pi referenced this pull request2026-10-04 19:35:37 +00:00
agent-pi referenced this pull request2026-10-04 19:35:38 +00:00
agent-pi referenced this pull request2026-10-04 19:35:40 +00:00
agent-pi referenced this pull request2026-10-04 19:35:40 +00:00
agent-pi referenced this pull request2026-10-04 19:35:41 +00:00
agent-pi referenced this pull request2026-10-04 19:35:42 +00:00
agent-pi referenced this pull request2026-10-04 19:35:42 +00:00
agent-pi referenced this pull request2026-10-04 19:35:43 +00:00