Track last commit (sha + date) per repo server-side; use it to order the explore page by most recent commit #247
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#247
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
Keep track of each repository's last commit — which commit (sha) and when — as queryable server-side state, and use it on the explore page (
/explore) to order repositories by most recent commit instead of today's name-based proxy.Current state (code evidence — this is a documented gap)
<ActivityStamp>(web/src/components/ActivityStamp.jsx, issue #142) renders "active " per repo row by fetchingGET …/commits?n=1per repo (sharedactivity:{o}/{r}cache, 30 s TTL, #117 caps bound it at ~500 GETs cold). The component header explicitly documents the rejected alternative: "the summary (GET …/api, §9.1) carries NO date field at all … true reverse-chronological order wants a server-side shape, not client logic."web/src/lib/owners.js:17-33newestFirst()is reverse-lexicographic on the repo NAME — its own doc comment says: "the listing path exposes NO creation timestamp … if the backend ever carries creation times, this is the single function to replace." The explore page (web/src/pages/Owners.jsx) therefore does NOT order by recent commit today, despite rendering per-row stamps that look like it does.Manifest.Packs(internal/store/proto/types.go:110-122) carries every live pack'sSeqand the WALLogEntry.CreatedAt(:132) gives wall-clock time per entry.RefSnapshot/Refin the WAL state), and its date from the head commit object.Manifest.UpdatedAtis manifest-write time (push time, not commit time — too coarse and semantically wrong per the #142 decision).repoRegistry.Owners/Repos(cmd/walhub/serve.go:498-547) already readsmanifest.pbper repo (parallel, limit 8) just to decide existence — so per-manifest metadata rides the same reads; the missing piece is a durable, sorted index, not more scans.RepoCatalogexists but is name-only.proto.RepoCatalog(meta/repos.pb,internal/store/keys.go:23, types.go:254-258) holdsRepos []string— no timestamps, and it's "optional, not required for correctness".Proposed design
repos/<o>/<r>/meta/activity.jsonfollowing theaccess.jsonsidecar precedent) withlast_commit_sha,last_commit_time(commit date, matching #142's semantic:commit_datefirst,author_datefallback), andlast_push_at(push time — cheap and useful even when a push adds no commits; #142 documents that a branch-delete/tag-only push moves nothing). Update is O(1) per push — no scan.RepoCatalog(or addmeta/activity.pb) to a per-repo{owner, name, last_commit_sha, last_commit_time, last_push_at}row, updated on the same publish path and rebuilt/swept by the maintainer loop (which already walks repos). This is the object-store-analog of an index: one GET serves the full sorted list for the explore page.GET /api/v1/owners/{owner}/repos(and a newGET /api/v1/repos?sort=activityor explore-shaped endpoint) returns rows with the activity fields (never plain string lists — this is a wire-shape change to the §8 endpoints; template the change through all three route twins per the NonRepo pattern ininternal/api/routes.go:46-51). IncludeCache-Control: SWRper the existing conventions.newestFirst()(lib/owners.js:30) with ordering on the server-providedlast_commit_time(its own doc comment names it "the single function to replace"). Explore page rows then sort truly newest-first,ActivityStampreads the same field instead of a per-rowcommits?n=1GET (drops the ~500-GET cold-cache worst case), andRepos.jsx(/:owner) gets the same ordering for free.Acceptance criteria
last_commit_sha/last_commit_time(commit-date semantics per #142:commit_datepreferred,author_datefallback); a tag-only/branch-delete push updateslast_push_atbut not the commit fields./explore) and/:ownerorder repos by most recent commit, newest first.<ActivityStamp>sources the stamp from the listing data (no per-rowcommits?n=1fetch on the explore/owner pages); empty repos show "no commits yet".Review: #247 server-side last-commit tracking (REVIEW ONLY, no code)
Verdict: proceed-with-fixes — the direction is sound and most code evidence checks out, but 5 blocking items must be resolved before implementation (hot-path contention, breaking wire change, unspecified derivation call site, missing frozen-list amendment, inaccurate manifest claim).
What I verified against code (correct)
BLOCKING
B1 — Synchronous bucket-root catalog CAS on the publish hot path is a cross-repo contention funnel and breaks law 6.
§2 has the catalog "updated on the same publish path". The catalog is ONE bucket-root key; every push to every repo would read-modify-write it. Concurrent pushes to unrelated repos then 412 against each other, and the casUpdate ladder (02 §2.7: immediate re-read, counted retries, ErrRetriesExhausted) turns cross-repo concurrency into spurious contention with a failure backstop designed for same-key races, not global ones. It also adds ≥1 sequential store round trip to the push ≤5 budget (law 6) — the acceptance criterion "no second store round trip beyond the index write" already concedes the regression instead of eliminating it.
Required fix: the push path writes ONLY the per-repo sidecar (repo-scoped key, no cross-repo contention), and the aggregate catalog is refreshed OFF the hot path (maintainer sweep / background fold; staleness = documented pass interval). Alternatively per-owner sharded catalogs. Either way §2 needs a rewrite and the sim budget assertion (15_testing.md) must show push +0.
B2 — Changing ownerRepos from ["a","b"] to rows of objects is a retyping breaking change, not an additive field.
internal/api/discovery.go:131-142 returns a plain string list today. 14.12 allows new OPTIONAL fields on existing JSON objects; array-of-string → array-of-object is a retype and "requires a NEW prefix served alongside v1; v1 is never edited in place". The plan must name the new endpoint (new path or v2), add it to discovery endpoints[] (pick a rule — recent feature waves chose no-discovery entries, document which rule this follows), triple-twins, and SDK. Same for the proposed top-level GET /api/v1/repos — 14.3 requires a spec note for new top-level families.
B3 — Derivation call site and failure semantics are unspecified, and the git read is hand-waved.
Commit date is NOT derivable from manifest bytes: manifest.pb carries NO refs (see B5). The exact call site in publish.go runBatch must be named (recommendation: derive once, outside the CAS attempt loop, or best-effort after commitLocal success), with the rule: derivation failure NEVER fails the push — degrade to null and let the maintainer heal it (law 4: the push ACK covers git data; the rollup is explicitly optional/rebuildable, so ACK-before-rollup is fine — but say so). A git subprocess per push also needs its exact argv added to 04_git.md per law 2 (precedent exists: 07_api.md:529,563 %aI/%cI formats — name which one the publish path uses).
B4 — Missing 14 §14.11 rule-2 frozen-list amendment + proto-compat work.
The sidecar family (repos///meta/activity.json or chosen name) and the aggregate (extended RepoCatalog field 3+ as a new RepoActivity message, or new bucket-root meta/activity.pb key — bucket-root keys are also frozen layout) MUST join the frozen overwritable list in the same change, with codec + golden fixtures (TestGoldenWireCompat). Also rule out "manifest-inline" explicitly: Manifest is the linearization point every replica parses; keep rollups out of it — sidecar + catalog only.
B5 — "Last commit sha derivable from the manifest HEAD ref snapshot" is inaccurate.
There is no ref snapshot in manifest.pb. The tip comes from the ref layer (post-sync view / checkpoint refs.pb / log txns) and the date from the commit object. Cold derivation therefore costs a refs sync (log segment GETs) plus a git read — the backfill unit must budget this per repo, and state its enumeration source (note: maintain engine Repos() in bind_wal.go:29 is the LOCAL in-memory list, not bucket enumeration — a fleet-wide backfill needs a bucket LIST, off-hot-path-legal but must be specified, bounded, and resumable).
Should-fix
Nit
Plan revision R1 (review findings — R1 wins on conflict)
Blocking resolutions (normative)
Should-fix adoptions
liveRepos Head-vs-Get accuracy; null-vs-0 semantics; tie-breaks; stale-row detection; SWR class; uint64-as-string; #117 cap interplay. Landing order: #248's rails first, activity second on the same shape.
Implementation ready for review: PR #260 (feat/issue-247, one commit on origin/main). Builds on the #248 rails per R1 — shared sidecar/catalog/sweep/endpoint extended, never duplicated. Production-verified full lifecycle (push hint, tag-push nulls, sweep heal folded=1/1/0, explore orders by commit date). All tiers green; browser render proof blocked by the shared-daemon network guard (documented in 12_web_ui.md, same guard a prior entry cites). Do-not-merge pending review.
Review of PR #260 (feat/issue-247): READY TO MERGE (with one pushed nit fix; see below). Verified in scratch worktree /tmp/pr260 at
5028ab2plus fixupccd7e51; main worktree untouched and clean.R1 COMPLIANCE (all five blocking rulings):
MERGE CONTRACT (walked both directions): publish blind-write re-derives size arithmetically from the held manifest (no clobber of size); sweep foldOne never regresses activity it cannot refresh (TestFoldOneHookFailurePreserves, TestFoldOnePreservesActivityWithoutHook); catalog same-head monotonicity preserves known rows (TestSweepCatalogNeverRegressesSameHeadRow). HeadTarget symref fallback is sound: tip always comes from the WAL view, the file only supplies the target name when the view never recorded one, and every miss preserves + continues (tested incl. serve-sync rescue). One documented residual wart (non-blocking): a tag-only push blind-writes null commit fields until the next sweep heals them (bounded by the pass interval); forced by the no-sidecar-read law, disclosed in E17 + 05_wal_engine.md.
BUDGETS: [5,14]->[5,16] honest — each +1 is a documented blind sidecar PUT (#248 pack publish, #247 ref-txn push clock), flatness asserted separately (TestEvidenceImportFlat), push totals unchanged. Sim fence updated in-test with rationale, not weakened.
SHOULD-FIX: null-vs-0, unknowns-last-either-direction, tie-breaks, stale detection (head_seq + monotonicity), SWR class, #117 slice-after-sort all land tested. uint64-as-JSON-numbers deviates from the 14.12 as-strings convention but matches the LANDED #248 shape — consistency wins, not this PRs to fork. Frontend: explore + /:owner share repos:{owner} detailed(sort=activity&order=desc); ActivityStamp at/empty shortcut sound (undefined=fetch, null+empty=no-commits, null+nonempty=legacy fallback).
NIT FIXED + PUSHED (
ccd7e51): orderByActivity tied on (name,owner) while Go FilterSort ties on (owner,name) and E17 claimed they match. Aligned JS to (owner,name) + added a cross-owner tie test. Zero behavioral change on listing pages (single-owner lists).TESTS: go -race green on proto/git/sizecatalog/maintain/api/store/wal/repoimport/cmd-push-budget; server green except TestUIAssetConcepts (environmental: scratch dist lacks landing GIFs, needs full make web; PR touches only bind_wal.go + its test there). Coverage >=95% on every touched package (95.0-98.3). node --test: all PR-touched files pass (owners/sdk-surface/activity 24/24 incl. the new tie test); 7 unrelated files fail ONLY in scratch (node_modules is gitignored and absent there; no package.json changes). gofmt/vet clean. No new Go modules or npm deps. TestDiscoveryShape passes -count=3, no flake observed. No browser run per task scope (nothing browser-facing beyond data-driven rendering already covered headless).
MERGE RECOMMENDATION: ready to merge.
Implemented in PR #260 incl. review tie-break fix (all R1 rulings verified, merge contract walked both directions; all gates green), merged. Closing.