From 2eea5ff885d29b60f5e8a729f38a895e88a72bb8 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Sat, 13 Jun 2026 19:50:29 -0500 Subject: [PATCH] small fix for null columns and background pds inserts on push --- pkg/appview/db/queries.go | 14 +++--- pkg/appview/db/queries_test.go | 78 ++++++++++++++++++++++++++++++++++ pkg/hold/oci/xrpc.go | 12 +++++- 3 files changed, 96 insertions(+), 8 deletions(-) diff --git a/pkg/appview/db/queries.go b/pkg/appview/db/queries.go index a317b0c..bb598db 100644 --- a/pkg/appview/db/queries.go +++ b/pkg/appview/db/queries.go @@ -145,7 +145,7 @@ func SearchRepositories(db DBTX, query string, limit, offset int, currentUserDID COALESCE(m.artifact_type, 'container-image'), COALESCE((SELECT tag FROM tags WHERE did = m.did AND repository = m.repository ORDER BY created_at DESC LIMIT 1), ''), COALESCE(m.digest, ''), - MAX(rs.last_push, m.created_at), + MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM matching_repos mr JOIN manifests m ON mr.latest_id = m.id @@ -153,7 +153,7 @@ func SearchRepositories(db DBTX, query string, limit, offset int, currentUserDID JOIN repo_stats ON m.did = repo_stats.did AND m.repository = repo_stats.repository LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository LEFT JOIN repo_pages rp ON m.did = rp.did AND m.repository = rp.repository - ORDER BY MAX(rs.last_push, m.created_at) DESC + ORDER BY MAX(COALESCE(rs.last_push, m.created_at), m.created_at) DESC LIMIT ? OFFSET ? ` @@ -2253,7 +2253,7 @@ func GetRepoCards(db DBTX, limit int, currentUserDID string, sortOrder RepoCardS var orderBy string switch sortOrder { case SortByLastUpdate: - orderBy = "MAX(rs.last_push, m.created_at) DESC" + orderBy = "MAX(COALESCE(rs.last_push, m.created_at), m.created_at) DESC" default: // SortByScore orderBy = "(COALESCE(rs.pull_count, 0) + COALESCE((SELECT COUNT(*) FROM stars WHERE owner_did = m.did AND repository = m.repository), 0) * 10) DESC, m.created_at DESC" } @@ -2279,7 +2279,7 @@ func GetRepoCards(db DBTX, limit int, currentUserDID string, sortOrder RepoCardS COALESCE(m.artifact_type, 'container-image'), COALESCE((SELECT tag FROM tags WHERE did = m.did AND repository = m.repository ORDER BY created_at DESC LIMIT 1), ''), COALESCE(m.digest, ''), - MAX(rs.last_push, m.created_at), + MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM latest_manifests lm JOIN manifests m ON lm.latest_id = m.id @@ -2358,7 +2358,7 @@ func GetUserRepoCards(db DBTX, userDID string, currentUserDID string) ([]RepoCar COALESCE(m.artifact_type, 'container-image'), COALESCE((SELECT tag FROM tags WHERE did = m.did AND repository = m.repository ORDER BY created_at DESC LIMIT 1), ''), COALESCE(m.digest, ''), - MAX(rs.last_push, m.created_at), + MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, '') FROM latest_manifests lm JOIN manifests m ON lm.latest_id = m.id @@ -2366,7 +2366,7 @@ func GetUserRepoCards(db DBTX, userDID string, currentUserDID string) ([]RepoCar LEFT JOIN repository_stats rs ON m.did = rs.did AND m.repository = rs.repository LEFT JOIN repo_pages rp ON m.did = rp.did AND m.repository = rp.repository WHERE ` + activeTakedownClause("m") + ` - ORDER BY MAX(rs.last_push, m.created_at) DESC + ORDER BY MAX(COALESCE(rs.last_push, m.created_at), m.created_at) DESC ` rows, err := db.Query(query, userDID, currentUserDID, currentUserDID, currentUserDID) @@ -2444,7 +2444,7 @@ func GetStarredRepoCards(db DBTX, starrerDID string, currentUserDID string) ([]R COALESCE(m.artifact_type, 'container-image'), COALESCE((SELECT tag FROM tags WHERE did = m.did AND repository = m.repository ORDER BY created_at DESC LIMIT 1), ''), COALESCE(m.digest, ''), - MAX(rs.last_push, m.created_at), + MAX(COALESCE(rs.last_push, m.created_at), m.created_at), COALESCE(rp.avatar_cid, ''), st.starred_at FROM latest_manifests lm diff --git a/pkg/appview/db/queries_test.go b/pkg/appview/db/queries_test.go index ba19fe7..7cc043c 100644 --- a/pkg/appview/db/queries_test.go +++ b/pkg/appview/db/queries_test.go @@ -1909,3 +1909,81 @@ func TestGetUserRepositories_Empty(t *testing.T) { t.Errorf("expected nil slice for user with no repos, got %#v", repos) } } + +// TestGetRepoCards_NullLastPushStillSortsByCreatedAt is a regression test for +// the "What's New" NULL-poison bug: a repo whose repository_stats row exists +// but has last_push = NULL (e.g. created by a pull when the push-notify never +// recorded a push) must still appear and sort by its manifest created_at. +// +// SQLite's scalar 2-arg max() returns NULL if any argument is NULL, so the old +// `MAX(rs.last_push, m.created_at)` produced a NULL sort key for such repos, +// dropping them to the bottom (and out of the limited "What's New" list) and +// rendering a blank "last updated" timestamp. The fix wraps last_push in +// COALESCE so it falls back to created_at. +func TestGetRepoCards_NullLastPushStillSortsByCreatedAt(t *testing.T) { + d := setupBatchTestDB(t) + createBatchTestUser(t, d, "did:plc:alice") + + // A public hold so the manifests are accessible to anonymous viewers. + if _, err := d.Exec(` + INSERT INTO hold_captain_records (hold_did, owner_did, public, allow_all_crew) + VALUES ('did:web:hold', 'did:plc:alice', 1, 0) + `); err != nil { + t.Fatalf("seed captain: %v", err) + } + + now := time.Now() + old := now.Add(-240 * time.Hour) // 10 days ago + + // "fresh": recent push, but its stats row has last_push = NULL (pulled, + // never push-recorded). "stale": older, with a real last_push. + if _, err := BatchInsertManifests(d, []Manifest{ + {DID: "did:plc:alice", Repository: "fresh", Digest: "sha256:fresh", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: now}, + {DID: "did:plc:alice", Repository: "stale", Digest: "sha256:stale", HoldEndpoint: "did:web:hold", SchemaVersion: 2, MediaType: "application/vnd.oci.image.manifest.v1+json", ArtifactType: "container-image", CreatedAt: old}, + }); err != nil { + t.Fatalf("insert manifests: %v", err) + } + + // fresh: pull-only stats row, last_push explicitly NULL (the bug trigger). + if _, err := d.Exec(` + INSERT INTO repository_stats (did, repository, pull_count, last_pull, push_count, last_push) + VALUES ('did:plc:alice', 'fresh', 2, ?, 0, NULL) + `, now.Format(time.RFC3339)); err != nil { + t.Fatalf("seed fresh stats: %v", err) + } + // stale: has a genuine last_push, older than fresh's created_at. + if _, err := d.Exec(` + INSERT INTO repository_stats (did, repository, pull_count, last_pull, push_count, last_push) + VALUES ('did:plc:alice', 'stale', 0, NULL, 1, ?) + `, old.Format(time.RFC3339)); err != nil { + t.Fatalf("seed stale stats: %v", err) + } + + cards, err := GetRepoCards(d, 18, "", SortByLastUpdate) + if err != nil { + t.Fatalf("GetRepoCards: %v", err) + } + if len(cards) != 2 { + t.Fatalf("expected 2 cards, got %d: %#v", len(cards), cards) + } + + // fresh (recent created_at) must sort above stale (old last_push), despite + // fresh's last_push being NULL. + if cards[0].Repository != "fresh" { + t.Errorf("expected 'fresh' first (sorted by created_at), got order %q, %q", cards[0].Repository, cards[1].Repository) + } + + // And its "last updated" must be populated from created_at, not blank. + var fresh *RepoCardData + for i := range cards { + if cards[i].Repository == "fresh" { + fresh = &cards[i] + } + } + if fresh == nil { + t.Fatal("'fresh' repo missing from What's New") + } + if fresh.LastUpdated.IsZero() { + t.Error("expected 'fresh' LastUpdated to fall back to created_at, got zero time") + } +} diff --git a/pkg/hold/oci/xrpc.go b/pkg/hold/oci/xrpc.go index 962cda5..f4056fb 100644 --- a/pkg/hold/oci/xrpc.go +++ b/pkg/hold/oci/xrpc.go @@ -2,11 +2,13 @@ package oci import ( + "context" "encoding/json" "fmt" "log/slog" "net/http" "strings" + "time" "atcr.io/pkg/atproto" "atcr.io/pkg/hold/pds" @@ -189,7 +191,15 @@ func (h *XRPCHandler) HandleAbortUpload(w http.ResponseWriter, r *http.Request) // For pulls: Just increments stats (no layer records or posts) // Always increments stats (pull or push counts) func (h *XRPCHandler) HandleNotifyManifest(w http.ResponseWriter, r *http.Request) { - ctx := r.Context() + // Detach from the request context for the PDS writes below. AppView sends + // this notify as a best-effort fire-and-forget call (with its own 30s + // timeout), and an upstream proxy may 504 around the 10s mark. If we wrote + // layer/config/stats records on r.Context(), a client disconnect or proxy + // timeout cancels them mid-flight — leaving the manifest with missing + // records ("getb tx, context canceled"). Once we start writing to the + // embedded PDS we must finish; cap with a timeout so it can't run unbounded. + ctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), 2*time.Minute) + defer cancel() // Validate service token (same auth as blob:write endpoints) validatedUser, err := pds.ValidateBlobWriteAccess(r, h.pds, h.httpClient)