From 25526b0cef5296359dab45cb12072f9e22b42c20 Mon Sep 17 00:00:00 2001
From: Mathias <mthbqv@gmail.com>
Date: Thu, 11 Jun 2026 18:40:21 +0200
Subject: [PATCH] feat(discovery): persist video duration_s instead of
 discarding it (ADR-028)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

ADR-023's filterLowValue already fetches each candidate's duration via the
cheap videos.list quota call to drop Shorts/live, then threw it away — the
videos.duration_s column (migration 001) was never written. Carry it onto the
kept domain.Video and have UpsertVideo persist it, COALESCE-preserving a known
value so an unknown (0) re-upsert never clobbers it (the channel_title backfill
stance, migration 014). This is the enabling change for length-aware burst
selection. No new migration — the column already exists.
---
 internal/adapters/store/videos.go         | 20 ++++++++++++---
 internal/adapters/store/videos_test.go    | 30 +++++++++++++++++++++++
 internal/adapters/youtube/youtube.go      |  4 +++
 internal/adapters/youtube/youtube_test.go |  5 ++++
 internal/domain/domain.go                 |  5 ++++
 5 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/internal/adapters/store/videos.go b/internal/adapters/store/videos.go
index 7b28cbb..29b6b2d 100644
--- a/internal/adapters/store/videos.go
+++ b/internal/adapters/store/videos.go
@@ -46,15 +46,16 @@ func (s *Store) UpsertVideo(ctx context.Context, v domain.Video) (string, error)
 		}
 
 		if err := tx.QueryRow(ctx,
-			`INSERT INTO videos (user_id, provider, provider_video_id, title, url, published_at, channel_title)
-			 VALUES ($1, $2, $3, $4, $5, $6, $7)
+			`INSERT INTO videos (user_id, provider, provider_video_id, title, url, published_at, channel_title, duration_s)
+			 VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
 			 ON CONFLICT (user_id, provider, provider_video_id) DO UPDATE SET
 			     title         = EXCLUDED.title,
 			     url           = EXCLUDED.url,
 			     published_at  = EXCLUDED.published_at,
-			     channel_title = COALESCE(NULLIF(EXCLUDED.channel_title, ''), videos.channel_title)
+			     channel_title = COALESCE(NULLIF(EXCLUDED.channel_title, ''), videos.channel_title),
+			     duration_s    = COALESCE(EXCLUDED.duration_s, videos.duration_s)
 			 RETURNING id`,
-			v.UserID, provider, v.ProviderVideoID, v.Title, v.URL, nullTime(v.PublishedAt), v.ChannelTitle,
+			v.UserID, provider, v.ProviderVideoID, v.Title, v.URL, nullTime(v.PublishedAt), v.ChannelTitle, nullDuration(v.DurationSeconds),
 		).Scan(&id); err != nil {
 			return fmt.Errorf("store: upsert video: %w", err)
 		}
@@ -74,6 +75,17 @@ func nullTime(t time.Time) *time.Time {
 	return &t
 }
 
+// nullDuration maps an unknown duration (0) to SQL NULL so the upsert's
+// COALESCE(EXCLUDED.duration_s, videos.duration_s) preserves a previously-known
+// value instead of clobbering it with 0 (ADR-028; the channel_title backfill
+// stance, migration 014).
+func nullDuration(seconds int) *int {
+	if seconds <= 0 {
+		return nil
+	}
+	return &seconds
+}
+
 // NewestUnsummarizedVideoIDs returns up to limit of the user's videos that have
 // no summary yet, newest first (published_at DESC, NULLS LAST). It caps the
 // connect-time onboarding burst (Feature 1) at a fixed count: the caller marks
diff --git a/internal/adapters/store/videos_test.go b/internal/adapters/store/videos_test.go
index 3451a8e..954f79a 100644
--- a/internal/adapters/store/videos_test.go
+++ b/internal/adapters/store/videos_test.go
@@ -52,6 +52,36 @@ func TestUpsertVideo_ReturnsStableID(t *testing.T) {
 	require.Equal(t, 1, count, "must not duplicate the row")
 }
 
+func TestUpsertVideo_PersistsAndPreservesDuration(t *testing.T) {
+	ctx := context.Background()
+	s := newStore(t)
+	resetDB(t, rawPool(t))
+
+	// First upsert carries a known duration (ADR-028: discovery enriches it).
+	v := ytVideo(userA, "dur0000001x", "with duration")
+	v.DurationSeconds = 750
+	id, err := s.UpsertVideo(ctx, v)
+	require.NoError(t, err)
+
+	p := rawPool(t)
+	readDuration := func() *int {
+		var d *int
+		require.NoError(t, p.QueryRow(ctx, `SELECT duration_s FROM videos WHERE id = $1`, id).Scan(&d))
+		return d
+	}
+	require.NotNil(t, readDuration())
+	require.Equal(t, 750, *readDuration(), "duration must persist")
+
+	// A later upsert that does NOT know the duration (0) must not clobber it —
+	// the channel_title backfill stance (migration 014): COALESCE-preserve.
+	v2 := ytVideo(userA, "dur0000001x", "title updated, duration unknown")
+	v2.DurationSeconds = 0
+	_, err = s.UpsertVideo(ctx, v2)
+	require.NoError(t, err)
+	require.NotNil(t, readDuration(), "a 0/unknown re-upsert must not erase a known duration")
+	require.Equal(t, 750, *readDuration())
+}
+
 func TestUpsertVideo_IDMatchesSummaryDedup(t *testing.T) {
 	ctx := context.Background()
 	s := newStore(t)
diff --git a/internal/adapters/youtube/youtube.go b/internal/adapters/youtube/youtube.go
index 3e9fb22..bbb0997 100644
--- a/internal/adapters/youtube/youtube.go
+++ b/internal/adapters/youtube/youtube.go
@@ -302,6 +302,10 @@ func (a *Adapter) filterLowValue(ctx context.Context, client *http.Client, video
 		if m.seconds > 0 && m.seconds < a.cfg.MinVideoSeconds {
 			continue // Short / sub-threshold clip
 		}
+		// Carry the duration we already fetched onto the kept video so the store
+		// can persist it (ADR-028) — the burst's length-aware selection depends on
+		// it. Discarding it here was the gap the onboarding investigation found.
+		v.DurationSeconds = m.seconds
 		kept = append(kept, v)
 	}
 	return kept
diff --git a/internal/adapters/youtube/youtube_test.go b/internal/adapters/youtube/youtube_test.go
index 1797796..2f2cfdd 100644
--- a/internal/adapters/youtube/youtube_test.go
+++ b/internal/adapters/youtube/youtube_test.go
@@ -224,6 +224,11 @@ func TestNewVideosFiltersShortsAndLive(t *testing.T) {
 	if len(vids) != 1 || vids[0].ProviderVideoID != "long1" {
 		t.Fatalf("expected only long1 to survive the filter, got %+v", vids)
 	}
+	// The duration fetched for the filter is carried onto the kept video so the
+	// store can persist it (ADR-028) instead of discarding it.
+	if vids[0].DurationSeconds != 750 {
+		t.Fatalf("kept video DurationSeconds = %d, want 750 (PT12M30S)", vids[0].DurationSeconds)
+	}
 }
 
 // TestNewVideosNoFilterWhenDisabled: MinVideoSeconds=0 keeps the pre-ADR-023
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 367924a..5cf988b 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -77,6 +77,11 @@ type Video struct {
 	URL             string
 	PublishedAt     time.Time
 	SeenAt          time.Time
+	// DurationSeconds is the video length in seconds, when known (fetched by the
+	// ADR-023 videos.list enrichment at discovery). 0 means unknown — the store
+	// preserves a previously-known value rather than overwriting it with 0, and
+	// the onboarding burst (ADR-028) treats unknown as degrade-open (kept).
+	DurationSeconds int
 }
 
 // Transcript is the text of a video (or a record that none was available).