fix(playback): freeze v3 seek reanchor recipes - #548
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughProtocol V3 now persists executable playback recipes and reuses them for seek reanchors. Downloaded subtitles use stable IDs in URLs, artifact generation, and serving. Source metadata and subtitle inventory identities are validated against frozen recipes. ChangesPlayback V3 frozen execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlaybackHandler
participant PlanStore
participant SubtitleRepository
participant Transport
Client->>PlaybackHandler: start or seek playback
PlaybackHandler->>PlanStore: save or load frozen recipe
PlaybackHandler->>SubtitleRepository: validate downloaded subtitle identity
PlaybackHandler->>Transport: prepare route from frozen metadata
Transport-->>PlaybackHandler: return playback transport
PlaybackHandler-->>Client: return playback response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/api/handlers/playback_v3.go (2)
1440-1464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the subtitle segment classification.
freezeExecutableRecipeV3andvalidateFrozenSubtitleIdentityV3(Lines 1480-1507) each re-deriveexternalCount,embeddedCount, and the external/embedded/downloaded boundaries. The comments on both functions state that the layout must mirrorbuildSubtitleURLs. Three copies of that layout can drift independently.Extract one classifier that maps a combined index to a segment and a within-segment offset, then call it from both functions.
As per coding guidelines: "extract shared logic instead of duplicating it".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handlers/playback_v3.go` around lines 1440 - 1464, Extract a shared subtitle-index classifier for the external, embedded, and downloaded segments, returning the segment type and within-segment offset. Replace the boundary calculations in freezeExecutableRecipeV3 and validateFrozenSubtitleIdentityV3 with this classifier, while preserving each function’s existing segment-specific handling and validation.Source: Coding guidelines
882-883: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the identity-bound downloaded subtitle URL builder.
These two lines duplicate
buildSubtitleURLsininternal/api/handlers/playback.goat Lines 2117-2118. Both sites produce the URL thatHandleSubtitleparses fordownloaded_subtitle_id. If one site changes the parameter order, the separator, or the extension handling, the two producers diverge silently.Add one helper and call it from both sites.
♻️ Proposed helper
// downloadedSubtitleStreamURL builds the identity-bound artifact URL for a // downloaded subtitle. Both the inventory listing and frozen-recipe artifact // attachment must emit the identical shape. func downloadedSubtitleStreamURL(sessionID string, trackIndex int, codec string, fileID, downloadedID int) string { return subtitleStreamURL(sessionID, trackIndex, codec, fileID) + "&" + downloadedSubtitleIDParam + "=" + strconv.Itoa(downloadedID) }- url := subtitleStreamURL(sessionID, selectedIndex, string(downloaded.Format), file.ID) - url += "&" + downloadedSubtitleIDParam + "=" + strconv.Itoa(downloaded.ID) + url := downloadedSubtitleStreamURL(sessionID, selectedIndex, string(downloaded.Format), file.ID, downloaded.ID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handlers/playback_v3.go` around lines 882 - 883, The downloaded subtitle URL construction is duplicated and can diverge between playback handlers. Add a shared downloadedSubtitleStreamURL helper using the existing subtitleStreamURL and downloadedSubtitleIDParam symbols, then replace the inline URL assembly in both buildSubtitleURLs and the playback_v3 call site with this helper while preserving their existing arguments.internal/playback/executable_recipe_v3_test.go (1)
5-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd JSON round-trip coverage for the durable recipe.
This test exercises the in-memory freeze and thaw only. The recipe is persisted as JSON, so the production path is marshal, store, load, unmarshal, then
Valid. A wrong or missing struct tag passes this test and fails after a restart.
SubtitleTrackIndexandSubtitleTransportTrackIndexdeliberately omitomitemptybecause-1and0are meaningful. A JSON assertion pins that decision.Add a marshal/unmarshal step, then re-run the existing field comparisons.
💚 Proposed addition
func TestExecutableRecipeV3SurvivesJSONRoundTrip(t *testing.T) { plan := &PlanV3{PlanID: "plan:frozen"} recipe := FreezeExecutableRecipeV3(PlannerResultV3{ Plan: plan, PlayMethod: PlayRemux, SourceVideoCodec: "hevc", SourceDurationSeconds: 7_201, SubtitleTrackIndex: -1, SubtitleTransportTrackIndex: 0, }) recipe.SubtitleSource = SubtitleSourceDownloadedV3 recipe.DownloadedSubtitleID = 71 encoded, err := json.Marshal(recipe) if err != nil { t.Fatalf("marshal: %v", err) } var decoded ExecutableRecipeV3 if err := json.Unmarshal(encoded, &decoded); err != nil { t.Fatalf("unmarshal: %v", err) } if decoded != recipe { t.Fatalf("decoded = %#v, want %#v", decoded, recipe) } if !decoded.ValidFor(*plan) { t.Fatalf("decoded recipe no longer matches its plan: %#v", decoded) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/playback/executable_recipe_v3_test.go` around lines 5 - 37, Add JSON persistence coverage alongside TestExecutableRecipeV3RoundTripPreservesOperationalFields by marshaling a frozen ExecutableRecipeV3, unmarshaling it into a new value, and asserting equality plus ValidFor against the original plan. Include meaningful SubtitleSource, DownloadedSubtitleID, SubtitleTrackIndex=-1, and SubtitleTransportTrackIndex=0 values so JSON tags and non-omitempty behavior are verified; add any required encoding/json import.migrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql (1)
2-5: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider adding the CHECK constraint as
NOT VALID, then validating it separately.The column addition is metadata-only, but the CHECK constraint in the same
ALTER TABLEforces a validating scan underACCESS EXCLUSIVE.playback_v3_attemptsrows expire and are swept, so the table is normally small and the blocking window is short. If the table can grow under load, split the constraint to keep writes available.♻️ Proposed split
-- +goose Up ALTER TABLE playback_v3_attempts - ADD COLUMN frozen_recipe JSONB NOT NULL DEFAULT '{}'::jsonb, - ADD CONSTRAINT playback_v3_attempts_frozen_recipe_object - CHECK (jsonb_typeof(frozen_recipe) = 'object'); + ADD COLUMN frozen_recipe JSONB NOT NULL DEFAULT '{}'::jsonb; + +ALTER TABLE playback_v3_attempts + ADD CONSTRAINT playback_v3_attempts_frozen_recipe_object + CHECK (jsonb_typeof(frozen_recipe) = 'object') NOT VALID; + +ALTER TABLE playback_v3_attempts + VALIDATE CONSTRAINT playback_v3_attempts_frozen_recipe_object;As per coding guidelines: "Prioritize performance, reliability, predictable behavior under load and failures".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql` around lines 2 - 5, Split the migration so the frozen_recipe column is added separately from the playback_v3_attempts_frozen_recipe_object constraint; create the CHECK constraint as NOT VALID to avoid a blocking validation scan, then validate it in a separate migration step or statement when appropriate.Sources: Coding guidelines, Linters/SAST tools
internal/playback/planstore/postgres_test.go (1)
119-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a schema guard for the
frozen_recipecolumn.The fixture now persists
FrozenRecipe, so every subtest needs migration20260806083316.newPlanstoreFixtureprobes forplayback_v3_attemptsand forcurrent_replan_request_id, but not forfrozen_recipe. If a test database lacks the new migration,SaveAttemptfails with a column-not-found error instead of skipping. Extend the existing probe pattern.♻️ Proposed guard in `newPlanstoreFixture`
var hasFrozenRecipe bool if err := pool.QueryRow(ctx, ` SELECT EXISTS ( SELECT 1 FROM information_schema.columns WHERE table_name = 'playback_v3_attempts' AND column_name = 'frozen_recipe' )`).Scan(&hasFrozenRecipe); err != nil { t.Fatalf("check frozen_recipe column: %v", err) } if !hasFrozenRecipe { t.Skip("test database has not applied the playback v3 frozen recipe migration") }As per coding guidelines: "A Go test that cannot pass yet must use
t.Skipwith the reason in its own source".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/playback/planstore/postgres_test.go` around lines 119 - 122, The new FrozenRecipe fixture requires a schema guard before tests execute. Extend newPlanstoreFixture’s existing information_schema probes to check for the frozen_recipe column on playback_v3_attempts, fail the test if the probe errors, and call t.Skip with a clear migration-related reason when the column is absent.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/handlers/playback_v3.go`:
- Around line 533-537: Define a sentinel for subtitle-store failures, wrap the
repository error from ListDownloadedSubtitles in freezeExecutableRecipeV3 and
validateFrozenSubtitleIdentityV3, and classify matching errors as retryable.
Update internal/api/handlers/playback_v3.go:533-537, 1264-1284, and 1326-1331 so
store outages return retryable outcomes while genuine identity changes remain
non-retryable.
---
Nitpick comments:
In `@internal/api/handlers/playback_v3.go`:
- Around line 1440-1464: Extract a shared subtitle-index classifier for the
external, embedded, and downloaded segments, returning the segment type and
within-segment offset. Replace the boundary calculations in
freezeExecutableRecipeV3 and validateFrozenSubtitleIdentityV3 with this
classifier, while preserving each function’s existing segment-specific handling
and validation.
- Around line 882-883: The downloaded subtitle URL construction is duplicated
and can diverge between playback handlers. Add a shared
downloadedSubtitleStreamURL helper using the existing subtitleStreamURL and
downloadedSubtitleIDParam symbols, then replace the inline URL assembly in both
buildSubtitleURLs and the playback_v3 call site with this helper while
preserving their existing arguments.
In `@internal/playback/executable_recipe_v3_test.go`:
- Around line 5-37: Add JSON persistence coverage alongside
TestExecutableRecipeV3RoundTripPreservesOperationalFields by marshaling a frozen
ExecutableRecipeV3, unmarshaling it into a new value, and asserting equality
plus ValidFor against the original plan. Include meaningful SubtitleSource,
DownloadedSubtitleID, SubtitleTrackIndex=-1, and SubtitleTransportTrackIndex=0
values so JSON tags and non-omitempty behavior are verified; add any required
encoding/json import.
In `@internal/playback/planstore/postgres_test.go`:
- Around line 119-122: The new FrozenRecipe fixture requires a schema guard
before tests execute. Extend newPlanstoreFixture’s existing information_schema
probes to check for the frozen_recipe column on playback_v3_attempts, fail the
test if the probe errors, and call t.Skip with a clear migration-related reason
when the column is absent.
In `@migrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql`:
- Around line 2-5: Split the migration so the frozen_recipe column is added
separately from the playback_v3_attempts_frozen_recipe_object constraint; create
the CHECK constraint as NOT VALID to avoid a blocking validation scan, then
validate it in a separate migration step or statement when appropriate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85c89be7-e59f-4f12-a59e-841f9001f62f
📒 Files selected for processing (15)
internal/api/handlers/playback.gointernal/api/handlers/playback_subtitle_urls_test.gointernal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_test.gointernal/api/handlers/stream.gointernal/api/handlers/stream_test.gointernal/api/handlers/subtitle_search_test.gointernal/playback/executable_recipe_v3.gointernal/playback/executable_recipe_v3_test.gointernal/playback/plan_v3.gointernal/playback/planstore/postgres.gointernal/playback/planstore/postgres_test.gointernal/playback/protocol_store_v3.gointernal/playback/protocol_v3_test.gomigrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql
707e43d to
3eb9a9d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@migrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql`:
- Around line 9-10: Remove the VALIDATE CONSTRAINT statement from this
migration, leaving playback_v3_attempts_frozen_recipe_object defined as NOT
VALID. Add a later timestamped Goose SQL migration containing the validation for
that constraint, preserving the existing constraint and table names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88afb547-814c-4b30-8169-f7b4da19c208
📒 Files selected for processing (15)
internal/api/handlers/playback.gointernal/api/handlers/playback_subtitle_urls_test.gointernal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_test.gointernal/api/handlers/stream.gointernal/api/handlers/stream_test.gointernal/api/handlers/subtitle_search_test.gointernal/playback/executable_recipe_v3.gointernal/playback/executable_recipe_v3_test.gointernal/playback/plan_v3.gointernal/playback/planstore/postgres.gointernal/playback/planstore/postgres_test.gointernal/playback/protocol_store_v3.gointernal/playback/protocol_v3_test.gomigrations/sql/20260806083316_add_playback_v3_frozen_recipe.sql
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/playback/protocol_store_v3.go
- internal/playback/protocol_v3_test.go
- internal/playback/plan_v3.go
- internal/api/handlers/subtitle_search_test.go
- internal/api/handlers/stream_test.go
- internal/api/handlers/stream.go
- internal/api/handlers/playback.go
- internal/playback/planstore/postgres.go
- internal/api/handlers/playback_subtitle_urls_test.go
- internal/api/handlers/playback_v3.go
- internal/api/handlers/playback_v3_test.go
2 participants
Summary
Fixes #547.
Protocol-v3 seek reanchors now reuse the exact durable playback recipe that produced the active route instead of replanning against mutable catalog, probe, capability, or subtitle inventory state.
Root cause
The seek-reanchor path treated a runtime correction as a fresh planning request. Inputs that were valid at playback start could drift before the seek, causing the server to select or construct a different route while the Apple client correctly required recipe continuity for the existing attempt.
Read-only production inspection also established that the reported request came from an iPhone (
iPhone18,2) using an HLS remux route, rather than from tvOS. No production state was changed.Changes
seek_reanchor_recipe_unavailablefor legacy active attempts whose migrated recipe is empty, allowing the client to start a fresh attempt safely.Apple client cross-check
Compared against
Silo-Server/silo-applemain at570e781. The client retains the playback attempt ID, selected tracks, quality, and output-route generation during a seek reanchor and intentionally validates that the replacement remains the same recipe. A manual retry creates a new attempt ID. The server fix now matches that contract; no Apple-client change is required.Verification
Focused regression tests:
Covered regressions:
TestBuildSubtitleURLsBindsDownloadedSubtitleIdentityTestPrepareTransportV3UsesFrozenSourceMetadataAfterProbeDriftTestHandleSubtitleUsesBoundDownloadedIdentityAfterInventoryReorderTestAttachSubtitleArtifactV3UsesFrozenDownloadedIdentityWithoutOrdinalLookupTestHandleReplanPlaybackV3SeekReanchorKeepsCurrentRecipeEligibleTestFrozenDownloadedSubtitleV3RejectsInventoryReorderingTestSubtitleArtifactStoreFailuresAreRetryableTestExecutableRecipeV3SurvivesJSONRoundTripAdditional checks:
The repository-wide
make lintstill reports its documented pre-existing whole-tree backlog; the CI-equivalent merge-base-scoped lint above is clean.make test-gopasses the previously failing handler package after rebasing onto currentmain; its only local failures are four unrelated macOS NVENC probe cases:TestResolveHWAccelWithFFmpegAutoPrefersNVENCOverIntel,TestResolveHWAccelWithFFmpegUsesNVIDIADeviceNodesWithoutDRM,TestFFmpegSupportsNVENCCachesByFFmpegPath, andTestFFmpegSupportsNVENCSmokeProbeUsesSafeFrameDimensions.Review follow-up
frozen_recipe.NOT VALID, then validate it in a later timestamped migration so the first Goose transaction releases its stronger lock before the scan.Risk and compatibility
The migration adds a non-null JSONB recipe column defaulting to
{}and aNOT VALIDobject constraint; a later committed migration performs validation under a separate lock phase. Existing in-flight attempts cannot reconstruct trustworthy frozen inputs and therefore receive the explicit retryable outcome instead of silently changing route. The downloaded-subtitle ID query parameter is additive; ordinal behavior remains available for older URLs.AI Disclosure
Summary by CodeRabbit
New Features
Bug Fixes
Reliability