Repository navigation
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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 |
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
| key := "" | ||
| parts := strings.Split(strings.Trim(path, "/"), "/") | ||
| if len(parts) >= 2 && (parts[0] == drainTranscodeSegment || parts[0] == "remux") && parts[1] != drainStartSegment { | ||
| key = parts[1] | ||
| } |
There was a problem hiding this comment.
Admission failure in internal/transcodenode/drain.go gives existing prepared-download deliveries an empty admission key because only transcode and remux paths populate it, causing Gate.Begin to reject every GET/HEAD to /downloads/artifacts/{artifact_id} after fence publication, including already admitted downloads continuing through HTTPPreparer.Open. Identify and retain artifact delivery permits before fencing so existing transfers can continue without admitting new artifacts.
Prompt for LLM
File internal/transcodenode/drain.go:
Line 44 to 48:
Admission failure in internal/transcodenode/drain.go gives existing prepared-download deliveries an empty admission key because only transcode and remux paths populate it, causing Gate.Begin to reject every GET/HEAD to /downloads/artifacts/{artifact_id} after fence publication, including already admitted downloads continuing through HTTPPreparer.Open. Identify and retain artifact delivery permits before fencing so existing transfers can continue without admitting new artifacts.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| g.mu.Lock() | ||
| defer g.mu.Unlock() | ||
| authorityConfirmed := true | ||
| if err := g.syncLocked(ctx); err != nil { |
There was a problem hiding this comment.
Request serialization in internal/workerdrain/gate.go occurs because Gate.Begin performs a durable PostgreSQL lookup while holding the process-wide gate mutex on every media request, making concurrent manifest, segment, subtitle, and download requests wait on database round-trip latency and allowing slow lookups to block request releases and drain observations. Move authority I/O outside the shared state lock while preserving an ordering mechanism that prevents stale admissions after a sealed observation.
Prompt for LLM
File internal/workerdrain/gate.go:
Line 123 to 126:
Request serialization in internal/workerdrain/gate.go occurs because Gate.Begin performs a durable PostgreSQL lookup while holding the process-wide gate mutex on every media request, making concurrent manifest, segment, subtitle, and download requests wait on database round-trip latency and allowing slow lookups to block request releases and drain observations. Move authority I/O outside the shared state lock while preserving an ordering mechanism that prevents stale admissions after a sealed observation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
| if len(parts) == 3 && parts[0] == "downloads" && parts[1] == "artifacts" && downloadprepare.ValidArtifactID(parts[2]) && (r.Method == http.MethodGet || r.Method == http.MethodHead) { | ||
| key = artifactDrainKey(parts[2]) |
There was a problem hiding this comment.
Unbounded permit renewal in internal/transcodenode/drain.go occurs because artifact GET/HEAD admission sends the new artifact key through Gate.Begin's renewal branch, resetting the retained permit's expiry to now + MaxTokenTTL on every request, even after fencing; repeated HEAD polling therefore prevents bounded permit expiry and drain acknowledgment indefinitely. Preserve an existing artifact permit's expiry during delivery and extend it only through successful preparation or reuse before fencing.
Prompt for LLM
File internal/transcodenode/drain.go:
Line 61 to 62:
Unbounded permit renewal in `internal/transcodenode/drain.go` occurs because artifact GET/HEAD admission sends the new artifact key through `Gate.Begin`'s renewal branch, resetting the retained permit's expiry to `now + MaxTokenTTL` on every request, even after fencing; repeated HEAD polling therefore prevents bounded permit expiry and drain acknowledgment indefinitely. Preserve an existing artifact permit's expiry during delivery and extend it only through successful preparation or reuse before fencing.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Silo Kody — review completeReview finished. Check the inline comments for findings and verify each suggestion against the code and tests. Reviewing changes in Silo
Review settingsReview OptionsThe following review options are enabled or disabled:
|
1 participant
Problem
External deployment controllers cannot safely commission or retire workers through the current node API. Creation enables placement immediately, and ordinary disable does not prove that execution or client traffic has stopped. This change adds disabled registration and a guarded drain acknowledgment.
Related issue: #1176
Validation tasks: changes #1177 C1/C3; changes #1206 C2
Approach
Native API v2 advertises
disabled_creationandworker_drain. Controllers can register a disabled worker, verify it, and activate it under the original configuration validator. Identical create retries match the requested enabled state and cannot undo later activation.Drain disables placement and publishes an admission fence in one guarded transaction. Existing admitted work and authenticated GET/HEAD requests for prepared downloads can finish. The fence blocks new preparation and cached preparation reuse. Delivery keeps the original artifact expiry; successful pre-fence preparation or reuse can extend it. No known permit renews after fencing. Successful deletion revokes the exact artifact permit. Workers count requests, executing jobs, detached FFmpeg work and bounded media permits through completion and cleanup, then seal admission when all counters reach zero. Each acknowledgment binds the node, fence, process and persisted server identity. Missing fields, stale configuration or unavailable authority cannot produce a retirement receipt.
Controllers must confirm zero, delete the exact registration under its current validator, and acknowledge deletion before stopping the process they own. Explicit cancellation invalidates earlier receipts and leaves placement disabled. Missing or recreated registrations cannot reopen an old worker. Database checks run outside the request-accounting mutex; identity epochs reject results from checks that raced cancellation, fencing or registration loss.
Ordinary disable and main-listener v1 business contracts retain their behavior. Apple and Android need no new product surface for these administrator operations; Jellyfin compatibility needs no separate management endpoint. The existing web create request explicitly keeps
enabled=truewithout changing its presentation.Validation
Evidence
The server-only commissioning contract changes; no current client renders the new administrator fields. These excerpts came from the actual registered v2 handler, production NodeHandler and AdminConfigurationStore against migrated private PostgreSQL. Only the changed field is shown.
Before: a controlled private-copy mutation of
dfef30dfomitted forwarding the requestedEnabledvalue (Enabled: nil). This is a fault reproduction, not a screenshot of a released build.After: the frozen
dfef30dfsource was restored byte-for-byte.Both phases acknowledged guarded native disable/delete and proved the exact fixture registration absent.
The assembled transcode listener also verifies artifact continuity and expiry over HTTP, using synthetic artifacts and a controllable accounting clock:
These are controlled private-copy regressions, not captures of released builds. Restored cases preserve artifact bytes and modification time, then delete the exact artifact and partial file. The private listeners and disposable fixtures establish protocol behavior, not human acceptance or fleet playback.
Risks
Controllers may retire only workers supporting the new private protocol. Missing or incompatible acknowledgments leave workers running. Media permits can remain until positive revocation or token expiry, up to 24 hours. Unknown admission and retirement observations require current PostgreSQL authority; already admitted, unsealed playback can continue on its original bounded permit during a transient outage.
Rolling back the migration requires canceling all drains and preserves the latest configuration validator. An uncertain registration deletion requires retaining the process and explicit recovery. Deployment-controller custody and real fleet playback remain separate integration checks. Manual follow-up: siloserver.org #125, covering API-driven commissioning and retirement after this change merges.
Checklist
AI Disclosure