Mirror repos (pull-only) with scheduled upstream syncs: create-from-URL flow, cron-ish schedule presets, next-sync display, and push rejection #240
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#240
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
What's requested
A repo can be created as a mirror of an external git repository: walhub pulls from the upstream on a schedule, the repo is marked read-only (pull-only), pushes to it fail, and the UI shows the mirror badge plus the next scheduled sync. Schedules are presets: every hour, every 8 hours, every day (default), every week, every month. No achievements-style extras — sync, show next fire time, reject pushes.
Current state (what exists / what doesn't)
internal/repoimportis a one-shot clone-and-land (Feature 10):Begin→ task →runImport→ terminal. There is no recurring sync, no upstream credential retention for re-clone, and no mirror flag on the manifest.proto.Manifest(internal/store/proto/types.go:71) carries format/repo/seq/packs/settings only;RepoSettingsis inline TOML. A mirror flag + schedule + upstream pointer needs a home (see notes).internal/server/smart.go:92(gitInfoRefs) and the receive-pack path gate onrequireWrite(principal)— there is no repo-level read-only concept anywhere (grepfor read-only/readonly/RO finds nothing repo-scoped).internal/bundle/cron.gohas a parsed 6-field UTC cron withNext/PreviousFireand the bundle strategies pattern (Plannerininternal/maintain/bundles.go, settings-tab rendering of schedule + next fire inweb/src/pages/Settings.jsxScheduledTab). The maintain package runs loops (maintain.gointerval goroutine +follow.goseparate-cadence pattern) — mirror sync should follow the follow.go shape: its own cadence, never blocking maintenance.internal/wal/tasks.go(repo, kind)single-flight — a sync run should be a task (repo, "mirror-sync") so joins/dedup come free.repoPutininternal/api/summary.go) and CLI. The mirror flow likely extends the import flow rather than replacing it (see notes).Proposed design (for planner/code review to pressure-test)
repos/<o>/<r>/mirror.jsonfollowing theaccess.json/meta/placeholder.jsonsidecar precedent is another — planner's call) with:mirror: true,upstream_url(canonical, from repoimport'sNormalizeSource— same SSRF gate),schedule(one ofhourly|8h|daily|weekly|monthly),last_synced_at,next_sync_at,last_result. Canonical-schedule mapping to the existing 6-field cron sobundle.Cron.Nextcomputes fire times.mirror-synctask kind overinternal/repoimport's existing clone/ingest machinery — a re-import into the same repo (the task system's(repo,kind)single-flight prevents overlap; a sync landing on an in-flight previous sync joins or skips). Driver: a new follow-style loop goroutine (mirror cadence, separate from maintenance interval) that scans mirror repos and fires those due — same shape asmaintain.RunFollow. Scale check: daily-default schedules mean the scanner is cheap.internal/server/smart.gogitInfoRefs :32 and the post-auth receive path, plusinternal/server/bind_ssh.go:91SSHReceivePack): mirror → refuse with a clear plain-text error ("this repository is a read-only mirror; pushes are rejected") — 403 on info/refs service discovery is the right code; pushes must fail for admins too, not just non-admins.mirrorbadge + "pull-only" indicator next to the title (same slot as the #235 description).next_sync_atfrom the summary API (mirror repos only).…/api/mirrorGET/PUT, admin-write). Summary (summaryBody) gainsmirror: {upstream_url, schedule, next_sync_at, last_synced_at, last_result}— ETag caveat: any cached summary change needs ETag coverage per the #235 precedent. Manual sync POST spawns the task (202, same shape as imports).Acceptance criteria
(repo, mirror-sync)task; concurrent syncs of the same repo are joined/skipped, never overlapped.next_sync_atis visible on the repo (header or settings) and updates after each sync.Review: #240 Mirror repos with scheduled upstream syncs (PLAN review, no code)
Reviewer verdict: proceed-with-fixes. The plan's current-state analysis verified accurate against code on every load-bearing claim I checked. The seam mapping (Seam 5 task kind, follow-style loop, bundle cron reuse, repoimport reuse) is sound. But item (a) — upstream credentials — is an unresolved architectural decision that blocks implementation start, and (c)/(d)/(e) each contain one blocking sub-point. Details below with severity tags.
Verified claims (all check out)
Manifest(internal/store/proto/types.go) carries format/repo/seq/packs/settings only;RepoSettingsis inline TOML. No repo-level read-only concept exists (gates arerequireWrite(principal)-only; the sole namespace-level refusal precedent isgit.IsManagedRefforrefs/pull/**).bundle.Cron6-field UTC +Next/PreviousFireconfirmed (internal/bundle/cron.go);Settings.jsxScheduledTabschedule+next rendering pattern confirmed (line ~132);maintain.RunFollowseparate-cadence loop confirmed (internal/maintain/follow.go).wal.TaskTable.Run(repo,kind)single-flight join confirmed (internal/wal/tasks.go:180).repoimportone-shot with never-stored per-request tokens (task memory only, scrubbed params/records, host-pinned credential helper) confirmed (url.go,10_git_import.md§7).smart.gogitInfoRefs gate is ~:92–126, not :92/:32 as cited twice). Re-verify lines at implementation time.(a) UPSTREAM CREDENTIALS — BLOCKING, the biggest decision
The plan's acceptance criteria require "token support" and say credentials "are stored in a way that never leaks" — but never say where the token lives between scheduled syncs. This directly contradicts the #10 deliberate decision (never-stored import tokens;
import.jsoncarries no secret, params carrysecret_set:boolonly). A scheduled sync with no human present cannot use an ephemeral token. Three options:Begin), but does not cover scheduled fires — so it composes with option 1, it is not an alternative.Rule: v1 = option 1 + option 2. Scheduled syncs fetch public upstreams only; "Sync now" MAY accept a memory-only token in the POST body (never persisted, never logged, import S2 scrub rules apply). Any persisted-credential design is explicitly out of scope and needs its own issue + Decisions entry before code. (Weak fourth option, noted only: a single operator-scoped env token à la
WALGIT_UPSTREAM_TOKEN— shares one credential across all mirrors, no per-repo scoping, blast radius unjustifiable. Do not take it.)(b) Mirror flag home — SHOULD-FIX (blocking-adjacent)
repos/<o>/<r>/meta/mirror.json, Create-once-then-CAS'd (same family asimport.json/fork.json), amended into the frozen overwritable list per 14 §14.11 rule 2 in the same change. NOT a manifest proto field (frozen contract #2: new field number, Rust interop, golden fixtures — heavy, and sync state churn does not belong on the git linearization point), NOT settings TOML (any settings writer could flip read-only off; settings is the wrong trust domain for an enforcement flag).last_synced_at+last_result(+ consecutive-failure count, see (f)); DERIVEnext_sync_atat read time (bundle.Cron.Next(last_synced)) — never store it. Stored next-fire invites writer skew and clock bugs; compute-on-read is one pure function the summary handler already has.hourly|8h|daily|weekly|monthly), derive the cron string server-side from a fixed map. Do not accept freeform cron from the API (no cron-injection surface; unknown preset fails closed).(c) Push rejection points — BLOCKING sub-point: placement + sync self-refusal
Write paths that must refuse, enumerated:
gitInfoRefsreceive-pack advertisement (internal/server/smart.go~:126) → 403 plain-text. Plan is right that this is the discovery code.pushPipeline(internal/server/bind_ssh.go:214) — ONE check at its top covers BOTH transports' pack flow, since HTTPreceivePackLocaland SSH both funnel through it (verified). The plan's three-point enumeration (info/refs + post-auth receive path +SSHReceivePack) is redundant if placed here — prefer the funnel. Precedent: theIsManagedRefrefusal at the same site.SSHReceivePack(it writes a v0 advertisement before reading the client — refusal must precede it, else the client hangs).ng/rejected…report lines (managed-ref precedent), since the HTTP body is a git stream by then. The plan's "403" covers discovery only — state both codes.internal/git/managed.goheader comment; follow §8.4 "configuration, not a principal"). Mirror sync MUST publish viaPublish/PublishRefsdirectly, never through pushPipeline, or it refuses itself. State this explicitly in the plan.PUT …/settings(admin — the mirror-flag flip path; admin-only is correct, say so),POST …/ops/{op}(do any ops write refs?repair? — needs a one-line ruling each), and auto-create-on-push (a push to an unborn mirror name must not create a writable repo that then… actually creating the repo is fine, it just must be born mirrored or born refused — rule: mirror targets are created only through the mirror flow, never via auto-create).(d) Schedule storage + next-fire + ticker — SHOULD-FIX (two blocking sub-points)
hourly=@hourly,8h=0 0 */8 * * *,daily=@daily,weekly=@weekly,monthly=@monthly). Reuse isParseSchedule+Nextonly — the bundlePlanner(slots/strategies) is NOT reusable here, and the plan's wording ("Canonical-schedule mapping to the existing 6-field cron so bundle.Cron.Next computes fire times") is correct as long as nobody tries to reusePlan/Build.followgets away with no lease because compare-then-atomic-PublishRefsconverges (second publisher sees in-sync). A clone-based sync does NOT converge cheaply — two instances firing the same due mirror = two full clones + racing publishes. Instance-memory(repo,kind)single-flight does not span instances. Rule: the sync body takes a bucket lease (leases/mirror-<repo>.pb, CAS+TTL, 14.7 avoidance pattern) before cloning, or the loop is placement-gated to exactly one writer. Say which in the plan.followRoundAllshape:m.eng.Repos()+ placement check) and probemeta/mirror.jsonper repo (conditional GET; cheap at daily-default cadence). Never bucket LIST (law 4). Due =Next(last_synced) <= now; overdue-after-restart fires ONCE (not N catch-ups) — state this.(repo,mirror-sync)); deletingmirror.jsonstops the loop for that repo (probe-absent → skip — the plan's criterion is satisfiable exactly this way, no extra machinery).(e) Interactions — one BLOCKING sub-point
completeBodyconverge. Import converge is create-only-by-design (a ref pointing elsewhere aborts loud 409 —task.go~:306-311). Sync's entire job is fast-forwarding refs that moved — the opposite semantics. Rule: write a sync converge modeled onfollowOnce(§8.3 compare + ff-only check + atomicPublishRefstxn), reusing repoimport's clone +for-each-ref+ refmap + scrub layers. "Re-import into the same repo" as literally stated will 409 on the first sync that moves anything.(repo,kind)single-flight —repo-importandmirror-syncare different kinds. Rule:Begin(import) on a target with a livemirror.json→ 409; sync fire on a target with a runningrepo-import(or an in-progressimport.jsonclaim) → skip + narrate. The #79 claim protocol otherwise collides with the sync writer.meta/forks.json(existingremoveSupersededconsults children's manifests — state that sync-replaced packs flow through the same gate; no new GC rule needed). Mirror-as-child-of-external-upstream is out of scope by definition. A mirror must not itself be forkable-into-writeability confusion: forks of mirrors are born writable normal repos (fork gets own namespace/policy — 03 §7) — say so, or someone will file it as a bypass.mirror.jsonlives under therepos/prefix soRegistry.Deletesweeps it likeimport.json; no userspace records reference mirrors. Summary ETag caveat in the plan is correct (per #235 precedent) — just do it.(f) Failures, backoff, rewind — SHOULD-FIX
consecutive_failuresin the sidecar; backoff (skip next fire or capped delay — pick one, state it); every failure narrates via the taskerrorterminal +last_result; a failed sync never clears or moves the computed next fire (plan's criterion stands).merge-base --is-ancestor, refused + narrated, not sticky-silenced); a manual "force resync" op is the escape hatch. One line in the plan closes a real argument.Cross-cutting (law compliance — all SHOULD-FIX, all cheap)
CloneMirrorargv (04 §12); any fetch variant needs a doc'd argv line.RegisterKind+ route viaExtraRouteschain; core packages learn nothing named "mirror".repoimportextension) held to ≥95% +-race+ the plan's listed tests (add: lease-contention test, import-vs-sync 409 test, rewind-refusal test).Acceptance-criteria deltas (concrete edits to the issue)
followOnce-shaped, notcompleteBodyreuse; import-vs-sync mutual exclusion (409/skip); bucket lease for cross-instance exclusion; enumeration via registry + probe (no LIST); rewind = refuse + narrate; backoff counter.next_sync_atis computed at read, not stored.Plan revision R1 (review findings — R1 wins on conflict)
Blocking resolutions (normative)
repos/<o>/<r>/meta/mirror.json, Create-once-then-CAS'd, frozen-list amendment in the same change. NOT manifest proto, NOT settings TOML. Stores preset name +last_synced_at+last_result+consecutive_failures;next_sync_atCOMPUTED at read via the preset→cron map (hourly/8h/daily/weekly/monthly → 6-field cron, no freeform cron).pushPipelinetop (covers HTTP+SSH pack flow) + SSH advertisement refusal before client read; discovery 403, in-pipeline per-refnglines. Sync publishes viaPublish/PublishRefsdirectly (bypasses its own refusal). Audit: settings PUT (admin-only, correct), ops ref-writers ruled, auto-create never creates mirror targets.leases/mirror-<repo>.pb, CAS+TTL) before cloning. Enumeration via in-memory registry +mirror.jsonprobe (no LIST); overdue-after-restart fires ONCE. Manual Sync-now = direct task spawn; deletingmirror.jsonstops the loop.followOnce-shaped (compare + ff-only + atomic PublishRefs), NOTcompleteBodyreuse; import-vs-sync mutual exclusion (Beginon mirrored target → 409; sync fire duringrepo-import/claim → skip + narrate); forks-of-mirrors are born writable; #63 no impact.Standing decisions kept
Task kind via RegisterKind, ExtraRoutes, no new deps, pinned git argv, ≥95% + -race, Decisions entries for (a)(b)(c)(f), EVIDENCE entry, browser-proof UI (badge + next-sync display).
Implementation ready for review: #250 (branch feat/issue-240, one commit). Per plan R1: sidecar + frozen-list amendment, create-from-URL (public-only v1, memory-only token), mirror-sync task + lease + loop, followOnce converge (ff-only + force escape), funnel refusal (HTTP+SSH), import exclusion, backoff, computed next-fire, badge/UI/presets, EVIDENCE E15. Gates: mirror 97.0%, server/api/repoimport/store >=95%, -race green, node 471 green, vet/contract/e2e/full-short green. Live proof: create -> sync -> scheduled fire -> push refused -> summary data. One open item: in-browser render blocked (shared Chrome daemon refuses private/loopback targets, private daemon forbidden) — recorded in 11_mirror.md/E15, not claimed. Not merging.
REVIEW: PR #250 (feat/issue-240) — R1 compliance verified point-by-point, 2 fixes pushed, recommendation at the end.
R1 RULINGS (all hold in code + docs):
(a) Credentials, public-only v1 — PASS. MirrorDoc (internal/mirror/mirror.go:68) has no secret field. Token path is POST body -> SyncAsync goroutine closure -> runSync -> child env only (git.go:139-141, host-pinned helper, never argv/bucket/logs). Task params carry secret_set presence-only (sync.go:143-145). Scheduled loop fires with token="" (sync.go:756). Errors scrubbed (scrubText, git.go:326). Grep confirms no token persistence; TestRunnerTokenClone + TestScrubText pin it.
(b) Sidecar + frozen list — PASS. repos/\u003co\u003e/\u003cr\u003e/meta/mirror.json, Create-once-then-CAS'd (mirror.go:194,214); 14_extensibility.md overwritable list amended + Feature-11 Decisions entry in the same change. Stores preset + last_synced/attempt_at + last_result + consecutive_failures; next_sync_at COMPUTED at read (NextFire/ViewOf, mirror.go:82,303), failures never move it (anchored at last SUCCESS). Preset-only, fail-closed both endpoints (ValidPreset/CronFor).
(c) Funnel refusal — PASS. pushPipeline top = per-ref ng lines (bind_ssh.go:228-235, covers HTTP+SSH pack flow); gitInfoRefs discovery 403 (smart.go:134-137); SSH pre-advertisement refusal (bind_ssh.go:124-126). Fetches/clones untouched. Sync publishes via h.Publish directly (sync.go:401, never enters the funnel — cannot refuse itself). Audit in 11_mirror.md §4: settings PUT stays admin-only, ops ref-writers bypass by construction, auto-create births normal repos only. Core never imports the feature (injected MirrorGuard, server.go:61, nil-safe).
(d) Lease + enumeration — PASS. leases/mirror-\u003cowner\u003e-\u003cname\u003e.pb, CAS+TTL 10m skew 0, taken BEFORE cloning (sync.go:256,625); contention = skip + narrate, never wait. Registry + mirror.json probe enumeration (sync.go:738-752), no LIST; overdue fires once; delete stops loop (probe-absent skip). Loop is its own 1-min goroutine on maintain-role hosts (serve.go:193-201, follow.go shape). Manual sync = async task spawn (202).
(e) Converge + exclusion — PASS. followOnce-shaped: compare + merge-base --is-ancestor ff-only + atomic PublishRefs (sync.go:344-406); no completeBody reuse. Begin on mirrored target -> 409 via key probe, no import-\u003emirror import (service.go:216-220). Sync under live repo-import claim -> skip + narrate (importClaimLive, sync.go:460). Rewind = refuse + narrate, counter untouched (recordRefused); force escape bypasses ff-only AND backoff. Forks-of-mirrors born writable stated (11_mirror.md:77).
(f) Failures/backoff — PASS. consecutive_failures + 15m x 2^(n-1), 24h cap, attempt-anchored (mirror.go:120-150); failed sync never moves next fire.
Plus: push budget +2 exact-key probes documented (E15) and pinned (push-budget test: mirror probes ≤6 per 2 pushes, every other collab family zero); summary +1 probe only when hook set, ETag ~m suffix (summary.go:117-123) + test; UI badge/tab//new mode/next-sync all from shared summary, no extra fetch, no new npm deps; go.mod/go.sum untouched; Decisions entries for (a)(b)(c)(f) + E15 present; law-8 seams respected (RegisterKind/ExtraRoutes/RegisterExposed/Env-hook).
FIXES PUSHED to origin/feat/issue-240 (commit
60fe26d, reviewed + tested):TEST RESULTS (scratch worktree, PR branch + fixup): gofmt clean; go vet clean (mirror/server/api/repoimport/store/cmd); go build ./... ok; -race: internal/mirror ok (96.9% cover), internal/server ok (95.6%), internal/api ok (95.4%), internal/repoimport ok (95.9%), internal/store ok (95.1%), cmd/walhub push-budget + forward-identity tests ok. JS: all 468 unit tests pass (incl. new mirror.test.js + sdk-mirror.test.js); smoke.test.js passes alone but hangs in any multi-file run — reproduced identically on main WITHOUT the PR (pre-existing environmental issue: loopback fetch never settles under this workspace's network guard), not a PR defect. Browser render remains open per 11_mirror.md/E15 (shared daemon blocks loopback; no private daemon per workspace rules) — not attempted, not claimed.
MERGE RECOMMENDATION: ready to merge (CI to confirm the fixup commit; no structural blockers).
Implemented in PR #250 incl. review PUT-404 + scrub fixes (all R1 rulings verified; coverage gates hold), merged. Closing.