feat(ytdlp): publish yt-dlp updates as immutable managed releases - #420
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -41 |
🟢 Coverage 84.56% diff coverage · -0.05% coverage variation
Metric Results Coverage variation ✅ -0.05% coverage variation (-1.00%) Diff coverage ✅ 84.56% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (6fc2693) 29159 25051 85.91% Head commit (ace044b) 30103 (+944) 25848 (+797) 85.87% (-0.05%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#420) 1218 1030 84.56% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a8cfe209c
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…covery Addresses three review findings on #420. `runProcess` never escalated to SIGKILL. Node sets `child.killed` as soon as a signal is *sent*, so it was already true after SIGTERM and the escalation returned without signalling again. A pip or probe that ignores SIGTERM would therefore run on while the promise waited for `close` that never came, wedging the process-wide pip queue and the in-flight update dedup for the life of the process. Gate on the real exit state instead, and add a last-resort timer so a grandchild holding the stdio pipes open cannot keep the promise unsettled either. Publish-lock recovery gated on `isPidAlive`, which a container defeats by routinely reassigning the crashed backend's pid to its replacement. The stale lock then looked permanently live and every later update timed out. Record a process-instance id in the owner file and recover purely on expiry: the critical section holds no network or pip work, so a lock past the stale threshold is broken by construction, and a suspended publisher still aborts at its nonce recheck before touching current.json. Leases are deliberately never reclaimed automatically -- an orphaned yt-dlp child may still be reading from the release, and pids are reused -- but the reporting half of that decision was missing. Collection now warns about a lease older than a day belonging to another instance, naming it so an operator can remove it, and still leaves it in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2060efe363
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Windows CI plus two more review findings on #420. The SIGKILL escalation test assumed POSIX signal semantics. Windows has none: kill() is TerminateProcess, so a SIGTERM handler cannot defer it and the first kill is already fatal, which is why the child closed reporting SIGTERM. Assert the platform's actual contract, and make the assertion deterministic rather than timing-based by having the last-resort timer report no signal at all -- it stopped waiting, it did not observe an exit, and claiming SIGKILL there made it indistinguishable from a real escalation. Lock acquisition creates the directory and then writes the owner file, and both halves were unsafe in between: - Recovery deleted an ownerless lock directory on sight. That window is also what a live acquisition looks like microseconds before it writes its owner, so a contender could destroy a valid lock, and the original holder could then overwrite the contender's owner file -- leaving an interleaving where both publishers pass their ownership checks. The owner file is now created exclusively, so a late writer fails instead of clobbering, and an ownerless directory is only reclaimed once it has been that way past the stale window. - A crash partway through the owner write leaves malformed JSON, and recovery gave up unconditionally on a parse failure. The lock stayed wedged forever and every later update timed out. Unreadable and invalidly timestamped owners now fall back to the same age-based reclamation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d4df55675
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c76bf2347
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eage Three follow-up findings on #420, all consequences of adding publication records. The module-origin probe collapsed "optional module absent" and "optional module resolved from an ambient install" into the same `false`, and only gated the exit code on `yt_dlp`. A candidate missing `curl_cffi` from its target but able to import the system copy therefore passed validation and would run against a mutable ambient dependency. The probe now classifies each module as inside/outside/absent and rejects any optional module resolving outside the candidate, while still publishing happily when one is simply unavailable. Garbage collection built its rollback window from install time. A finalized but never-published candidate -- an already-current update, or the loser of two concurrent installs -- carries the newest install time, so it took a slot in current-plus-two and could displace a genuine rollback target into collection. The window now follows publication generation, and unpublished candidates are protected only by retention age and leases. Publication gained a `published.json` write between its ownership check and the pointer swap. That write touches the filesystem and can be suspended long enough for the lock to expire and be reclaimed, after which the original publisher would still overwrite current.json from a stale decision. Re-check ownership immediately before the swap, and withdraw the record if the lock was lost, so recovery cannot promote a transition that never committed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46212bad73
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…path Two more follow-up findings on #420. Finalization renamed staging into releases/ without flushing 9C9A the parent directory. A rename is not durable until its parent is, and current.json's parent *is* flushed when the pointer is replaced, so a host crash could leave a surviving pointer naming a release whose directory entry was lost -- turning a completed update into a silent rollback through recovery. Fsync releases/ after the rename and before anything records the publication. Collection only ran after an install. A release pinned by a long download is skipped for as long as its lease exists, and nothing revisited it afterwards, so it stayed on disk until the next install -- potentially months. Releasing a lease now offers a collection, throttled to at most one store scan an hour so a burst of downloads cannot turn maintenance into per-download work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6880aa1267
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more follow-up findings on #420. `killProcessTree` only ever signalled the immediate child, so pip's build and download helpers survived a timeout. The abandon path would then let the next install start under withPipLock and delete the staging directory while an orphan was still writing into it. It now terminates the whole tree: the probe child gets its own process group on POSIX so a negative-pid signal reaches its descendants, and Windows uses taskkill /T /F, which is the only way to reach descendants there. Signalling the child alone remains the fallback. `ensurePrivateDir` validated the lexical path but followed an existing symlink, so a symlinked `staging` or store root would let `pip --target` write outside `/app/data/ytdlp` and let staging cleanup delete through the link. Store directories are now lstat-checked before use, matching the rejection the release directory and site-packages already got. The spawn-boundary rule caught the new taskkill call in an already-allowed module, which is exactly what it is for; its budget is raised deliberately with the reason recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 903e65bc0a
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sted releases Windows CI plus three more review findings on #420. Publication was still check-then-write: the ownership recheck reads owner.json and returns before a separate rename, so a publisher descheduled past the stale window could resume and overwrite a newer generation. Exclusive file creation is the one primitive the filesystem offers that is atomic across processes, so a publisher now claims `generations/<n>.json` with O_EXCL before touching anything. Two publishers deciding from the same current.json compute the same generation and exactly one can create it, which turns the swap into a real compare-and-swap; the nonce recheck stays as a secondary guard. A persisted store can outlive an image or host upgrade that changes the Python ABI, leaving a release whose interpreter path and site-packages still exist but which no longer runs. Existence checks accepted it and every invocation then failed. Availability now probes the selected release once per process and reinstalls, or falls through to discovery, instead of keeping it. Windows terminates through taskkill rather than a signal, so the escalation test asserted a signal that can no longer appear there; it now checks for a real exit code, which still distinguishes termination from the last-resort timer. Both process tests also raced node's startup at a 100ms timeout, and the grandchild test could pass vacuously if the grandchild never ran -- both now use a startup-proof window and assert the grandchild actually started. Also stop building child scripts by interpolating paths into source; the values travel as argv instead, which is what CodeQL flagged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a88f8fef0e
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f2abf41a5
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…path Three more follow-up findings on #420. A deletion marker left by a killed collector was permanent, and a marker means "somebody is deleting this" to lease acquisition as well as to collection - so the release became neither collectable nor usable. Markers are now reclaimed once older than the stale-lock window, the same signal already used for the publish lock and generation claims; a live collector is still refused. Stale-lock removal deleted whichever owner occupied the path at removal time. Two processes deciding the same lock is stale could therefore have the second delete the fresh lock the first had already replaced it with, and the new publisher would fail its own ownership assertion. Removal is now fenced on the identity that was observed: the owner nonce where there is one, and the directory mtime plus the continued absence of a usable owner where there is not. Runtime validation only happened on the execution path, so status reported a release that cannot run as available, and an operator update then treated a same-version repair as a redundant no-op. Status and the update path now share the same check, memoized per release id so it costs one probe rather than one per call, and the update detects a broken current release by reading the pointer directly - recoverUsableManagedRelease hides exactly the release it needs to see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2901963ec
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ssed collection Four more follow-up findings on #420, three of them the same shape: every mechanism that is reclaimable by age also needs an ownership identity, or a resumed owner destroys its replacement's claim. The publish lock got that in f290196; markers and generation claims now have it too, which completes the pattern across all three. A deletion marker carries a token. A collector paused past the stale interval can have its marker taken over, and previously it could resume and remove the replacement's marker -- after which a reader could take a lease between the replacement's lease recheck and its deletion, and the release would be removed while in use. Removal now requires still owning the marker. A generation claim carries a token for the same reason: a publisher whose claim was reclaimed could delete the replacement's claim, letting a third publisher commit the same generation and defeating the fence. Collection suppressed by the throttle was dropped rather than deferred. Publication runs a collection of its own, which arms the throttle, so the follow-up that matters -- the one after the last lease is released -- was usually the one discarded. A suppressed request is now scheduled for when the interval expires. Finally, a current release whose manifest parses but whose site-packages is gone was reported usable, so a same-version repair was rejected as already current and the broken pointer stayed. An unloadable current release now counts as broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6b24c16e
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… lock Two more follow-up findings on #420. Only the current release was runtime-validated. An image or ABI change breaks every persisted release, so after the current one was marked unusable and the repair failed, acquisition happily returned the rollback release on structural checks alone and the operation ran an incompatible interpreter. Availability now re-evaluates after marking one unusable, so the next candidate is validated in turn; the marked set only grows, so it terminates at external discovery. The publish lock directory is created on demand rather than by ensureManagedStoreLayout, so it was the one store path without a symlink check. Owner reads and stale-lock recovery would follow the link, and recovery could unlink an owner.json outside the managed store before rmdir failed on the symlink itself. It is now lstat-checked before the lock is inspected or taken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f9844de83
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…its own marker Two more follow-up findings on #420. Availability is resolved once per process, so a release published afterwards by another backend sharing the store had never been validated here -- and a publisher on a different image or Python ABI can leave one that is structurally valid but unrunnable. Acquisition now validates whichever release it observes. That is affordable because the probe is memoized per release id, so it costs one child process per new release rather than one per operation, and a release that fails is recorded so the next pass through recovery skips it. Deletion markers no longer compete for a single shared path. Each collector writes its own uniquely named marker file, so a collector only ever deletes the file it wrote and there is no reclamation step that could remove something another process created -- which closes the window where a reader could take a lease while a collector was already deleting the release. A marker left by a killed collector is ignored once older than the stale window rather than reclaimed, so it can neither block a release forever nor be stolen from a collector still using it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a386e553ff
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…irectories Two more follow-up findings on #420. The runtime probe treated "did not answer" as "does not run". A `--version` probe that timed out or failed to spawn under resource pressure permanently condemned an immutable release for the life of the backend, after which every status check and acquisition skipped a perfectly good current release and could roll back or reinstall for no reason. It now distinguishes a definitive verdict from an inconclusive probe and caches only the former -- the same distinction the capability probe already draws. `leases/<releaseId>` and `gc-markers/<releaseId>.deleting` are created on demand per release, so they were the last store paths without a symlink check: the store roots and the publish lock had been hardened but these had not. Following a link there would write a lease outside the managed store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7971b922cf
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Last follow-up finding on #420, and it wanted a protocol change rather than a longer timeout, which is right. Deletion marker visibility was tied to a stale threshold, so a `removeSafe()` that ran longer than two minutes -- a large release on a slow volume -- or a collector suspended after its lease recheck would stop protecting the release while it was still being deleted. A reader could then lease it and spawn yt-dlp against a half-removed directory. Collection now renames the release into a trash directory before deleting it. The rename is atomic and takes microseconds, so the marker only has to stay visible across that, not across the whole deletion; afterwards nothing can resolve the release at all and recovery simply picks another. How long the delete then takes, or whether it finishes, stops mattering. Trash entries are unreachable by construction, so a collector that dies mid-delete needs no reasoning about age or ownership: the next collection sweeps whatever is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec5e5f3be
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…clamation Two follow-ups on #420 that the previous round created. Collection checked leases before renaming a release away but not after. A reader writes its lease before it validates the release directory, so a lease landing during the rename was invisible to that check and the reader would then be holding a release that had just been retired. Collection now re-checks after the rename and puts the release back if a reader got there first -- which lets that reader's own directory check succeed, and means the protocol no longer depends on how long a marker stays live. Reclaiming an abandoned generation claim deleted it and then created a new one, so a slow reclaimer could delete a replacement's fresh claim and let a third publisher commit the same generation. Reclamation now moves the claim aside with a rename: exactly one process can move a given path, so the winner is unambiguous and a loser retries against whatever is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20d11f1b94
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…claims Two follow-ups on #420. A managed store that cannot hand out leases -- unwritable, a file where the per-release lease directory belongs, or collection winning every retry -- failed every operation. Design section 10.5.2 is explicit that a store problem must degrade rather than stop the backend serving, and every other store failure already falls through to discovery; this one did not. It now does. Reclaiming an abandoned generation claim renames it aside, which is atomic about which process moves the path but says nothing about what was sitting there: a reclaimer paused after judging a claim expired could move a replacement that appeared meanwhile. The reclaimer now confirms it took the claim it judged and puts it back otherwise, the same shape as the release retirement check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1215993cd
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a121599 to
38b45a3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38b45a3879
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
38b45a3 to
69d4d6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69d4d6e72b
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
69d4d6e to
9c446cd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c446cd04f
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
9c446cd to
279b8e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 279b8e0779
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`pip install -U` mutates the same Python environment downloads are running from. Serializing pip writers (#418) stops pip racing pip, but it does not make readers safe: a download can start while the console script is being replaced, or lazily import a module while pip is rewriting it. A bounded reader/writer gate was explored in #419 and rejected -- waiting indefinitely lets a long download block an operator-requested fix, timing out updates files underneath that download, and killing the download turns a possible failure into a certain one. Implements reports/ytdlp-update-execution-gate-design-2026-08-23.md. Closes #416. ## Immutable releases, atomic publication Each update installs into a fresh `data/ytdlp/staging/<op>` with `pip --target`, is validated there, then renamed into `releases/<id>` and never written to again. Validation covers the version, `--help`, and impersonation probes plus a module-origin check that asserts `yt_dlp` resolves inside the candidate and that no optional dependency resolves from an ambient install -- an absent optional module is a capability, one imported from system site-packages is a rejection. Publication reserves its generation with an `O_EXCL` create before it moves anything. Two publishers deciding from the same `current.json` compute the same generation and exactly one can create it, which makes the pointer update a compare-and-swap rather than a check followed by a write. `current.json` is then replaced by a single atomic rename, never unlink-then-rename, with the release directory fsynced first so a surviving pointer cannot name a lost directory. ## One snapshot per operation A logical operation acquires one immutable `YtDlpRelease` and keeps it across retries. Command, spawn environment and capabilities all belong to that release, so a job can never combine one release's capabilities with another's executable. Capabilities are cached per `releaseId`, distinguishing a definitive "no" from a probe that never completed; the same distinction applies to the runtime probe that decides whether a persisted release can still run at all. Managed releases run `<python> -m yt_dlp` with `PYTHONPATH` replaced rather than extended and `PYTHONNOUSERSITE=1`, so neither an operator `PYTHONPATH` nor a legacy `--user` install can shadow release modules. External releases keep master's prepend behaviour. ## Coordination Every shared-state mechanism follows the same two rules: exclusion comes from `O_EXCL` or `rename`, and nothing removes a resource it does not own. - Leases pin a release for as long as any child of the operation runs. - Collection retires a release by renaming it out of `releases/` before deleting it, so the deletion has no deadline and cannot outlive its marker, and re-checks for late leases afterwards -- restoring the release if a reader got there first. - Deletion markers, generation claims and the publish lock each carry an owner, and reclaiming an abandoned one moves it aside atomically and confirms identity before discarding it. - No correctness property depends on a timeout. ## Centralized execution All yt-dlp process creation goes through `spawnYtDlp` / `withYtDlpRelease`, enforced by an AST rule that counts process-creation calls per module, so a second spawn added to an already-allowed file fails rather than only a new file. This includes MissAVDownloader, which previously spawned `YT_DLP_PATH` directly and could probe one binary while running another. Timed-out probes terminate the whole process tree, since pip's helpers do not die with the interpreter. ## Compatibility Not a breaking change. With no store present, resolution falls through to `YT_DLP_PATH`, the image-pinned binary, then PATH discovery exactly as before; the store is created lazily on the first install. Any store problem -- corrupt, unwritable, symlinked, or unable to hand out a lease -- degrades to discovery rather than preventing the backend from serving. A legacy `/app/data/.home/.local` install is never modified, and older versions ignore `data/ytdlp/` entirely, so a rollback is safe. Visible but non-breaking: `status.path` reports the Python interpreter for a managed release, MissAV resolves through the release API, and the store uses ~50-100 MB per retained release. Re-running an update that is already current reports no change instead of returning 500, and a release that cannot run is repairable even by the same version. `YT_DLP_PATH` remains operator-managed: never overwritten, `updateSupported: false`, and the update endpoint still 409s. ## Testing 2,962 backend tests. Beyond unit coverage of manifests, publication policy, environment construction and command shape: failure injection at every pre-publication stage; concurrency tests for snapshot isolation, lease/GC races and two publishers; integration tests for first install, operator update, MissAV probe/spawn agreement, same-operation retry, and the three §10.5 data-directory states. Each invariant was verified by breaking the implementation and confirming the test fails. Adds a cross-platform CI job running the release-store suites on ubuntu/macOS/windows: atomic replacement, lease and marker handling, and `PYTHONPATH` delimiters all differ on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
279b8e0 to
ace044b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ace044b639
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Why
masterupdates yt-dlp withpip install -Uin the same Python environment downloads run from. #418 serialized pip writers, which stops pip racing pip — but it does not make readers safe. A download can still start while the console script is being replaced, or lazily import a module while pip is rewriting it.A bounded reader/writer gate was explored in #419 and rejected: waiting indefinitely lets a long download block an operator-requested fix, timing out updates files underneath that download, and killing the download turns a possible failure into a certain one.
Implements
reports/ytdlp-update-execution-gate-design-2026-08-23.md. Closes #416.What changes
Immutable releases, atomic publication. Each update installs into a fresh
data/ytdlp/staging/<op>viapip --target, is validated there, then renamed intoreleases/<id>and never written to again. Publication swaps a smallcurrent.jsonwith a single atomic rename under a filesystem lock, with a monotonicgenerationand a nonce ownership recheck so a suspended publisher cannot overwrite a newer release. It never emulates replacement as unlink-then-rename.Validation before publication. Version,
--help, and impersonation probes, plus a module-origin probe that assertsyt_dlp(andcurl_cffi/yt_dlp_ejs) resolve inside the candidate rather than from an ambient install. A failed candidate cannot become current.One snapshot per operation. A logical operation acquires one immutable
YtDlpReleaseand keeps it across retries. Its command, spawn environment and capabilities all belong to that release, so a job can never combine one release's capabilities with another's executable. Capabilities are cached perreleaseId, distinguishing a definitive "no" (cached for the release's lifetime) from a probe that never completed (evicted so a later acquisition retries).Leases and conservative GC. A lease pins a release directory while any child of the operation is running, using a two-phase deleting-marker protocol. GC keeps current plus two rollbacks, skips anything leased, and never touches a release younger than the retention age — so a candidate between finalize and publish cannot be collected out from under its publisher.
Centralized execution. All yt-dlp process creation goes through
spawnYtDlp/withYtDlpRelease, enforced by an AST rule that counts process-creation calls per module (so a second spawn added to an already-allowed file fails, not just a new file). This includesMissAVDownloader, which previously spawnedYT_DLP_PATHdirectly — its impersonation probe and its spawn could already disagree onmaster.Environment. Managed releases run
<python> -m yt_dlpwithPYTHONPATHreplaced (not extended) andPYTHONNOUSERSITE=1, so an operatorPYTHONPATHor a legacy--userinstall cannot shadow release modules. External releases keepmaster's prepend behaviour unchanged.Compatibility
Not a breaking change:
master— with no store present, resolution falls through toYT_DLP_PATH, the image-pinned binary, then PATH/legacy discovery. The store is created lazily on the first install, never eagerly at startup./app/data/.home/.localinstall is never modified or deleted.data/ytdlp/entirely.Visible but non-breaking:
status.pathreports the Python interpreter for a managed release, MissAV resolves through the release API, and the store uses ~50–100 MB per retained release. Both Docker guides are updated for persistence, rollback, legacy cleanup and disk usage.Behaviour fix: re-running the update when already on the newest release now reports
changed: falseinstead of returning 500.YT_DLP_PATHremains operator-managed: never overwritten,updateSupported: false, update endpoint still 409s withytDlpUpdateCustomPath. The controller contract, thecheckLatestPyPI lookup and the in-flight update dedup are unchanged.Testing
2923 backend tests pass. New coverage, mapped to the design's §16:
ytdlpManagedReleasecurrent.jsonopen), env semantics, command construction, leases/GC retention, store recoveryytdlpReleaseConcurrencyytdlpReleaseFailureInjectioncurrent.jsonrename — each asserting a reader still gets a complete release, never a partial candidate; corrupt-manifest lineage recovery; staging age thresholdytdlpReleaseValidationytdlpReleaseIntegrationcurrent.json, same-version no-op, MissAV probe/spawn agreement, same-operation retry stays on its snapshot while a new call picks up the newer release, plus the three §10.5 data-directory statesytdlpSpawnBoundaryEach new invariant was verified by deliberately breaking the implementation and confirming the test goes red.
CI: adds a
ytdlp-release-storejob on ubuntu / macOS / windows running the six release suites. Atomic replacement (MoveFileEx sharing violations), lease and GC marker handling, andPYTHONPATHdelimiters all differ on Windows; the full backend suite stays on Linux.Review notes
versionProbe.tsandruntime.tskeep directspawncalls by design — the former bootstraps discovery before any release exists, the latter probes Deno, which MyTube does not update at runtime. Both are budgeted explicitly in the boundary rule..gitignorechange ignores the Codacy CLI's auto-generated local config snapshots.