Skip to content

feat: Sonarr/Radarr scan_source.v1 plugin - #1

Merged
Quick104 merged 18 commits into
Silo-Server:mainfrom
fluxis:pr/scan-source-arr
Jun 3, 2026
Merged

Quick104 merged 18 commits into
Silo-Server:mainfrom
fluxis:pr/scan-source-arr

Conversation

@fluxis

@fluxis fluxis commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

First implementation of the scan_source.v1 plugin for Sonarr / Radarr. When the Silo host polls this plugin, it reads the arr /api/v3/history feed, extracts imported and renamed file paths, and returns them as raw source-namespace paths plus an opaque marker. The host owns the rest of the pipeline (path rewrites, library-folder resolution, scan enqueueing); the plugin stores no state and no credentials.

Capability

  • scan_source.v1 / id arr — "Sonarr / Radarr"

Design

  • Credentials arrive per request. PollChanges reads req.connection.{base_url, api_key} (resolved host-side from the operator's Autoscan connection — own creds or a reused Requests link). Nothing is persisted plugin-side; global_config_schema is empty.
  • Host owns path rewrites. The plugin returns paths verbatim in the arr's namespace; the host applies rewrites + resolves to Silo libraries. (This reverses an earlier design where the plugin rewrote — ownership moved host-side when the SDK merged.)
  • Opaque, monotonic marker. Empty marker ⇒ "from now"; the returned marker never regresses on empty polls and is rejected (error, not silent reset) if unparseable. A future-dated marker is clamped to now.
  • Paginated history (page/pageSize, sortKey=date desc) to avoid the 1 MiB body-cap truncation stall; stops once records predate the marker, emits only records strictly newer than the caller's marker.

Tests

  • internal/arr: history extraction (imports + both rename path fields; grabbed/deleted ignored), boundary-safe rewrites, marker clamping/regression guards.
  • End-to-end PollChanges over the go-plugin gRPC transport against a stub arr.

Build / deps

  • Depends on the merged silo-plugin-sdk scan_source.v1 via pseudo-version (local replace dropped).
  • go build ./... and go test ./... green.

Test Plan

  • go build ./... / go test ./... (green locally)
  • CI builds the cross-platform binaries (linux/amd64, linux/arm64, darwin/arm64)
  • Tag + release v0.1.0 (binaries + checksums.txt) so the silo-plugins catalog entry can be generated via update-catalog

Built host-side and validated live against a 4-arr instance: markers advance, source paths resolve + enqueue rescans correctly. AI-assisted (Claude Code).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added an autoscan plugin for Sonarr/Radarr that detects imported and renamed files and returns raw source paths and next-marker tokens for targeted rescans.
  • Documentation

    • Rewrote README into a detailed plugin specification and usage guide, including capabilities and testing instructions.
  • Chores

    • Added build automation and module configuration; updated ignore rules to exclude build artifacts.
  • Tests

    • Added unit, integration, and manifest-loading tests covering polling, pagination, marker semantics, and end-to-end flow.

fluxis and others added 13 commits June 3, 2026 19:22
Module scaffold for a standalone scan_source.v1 plugin (Sonarr/Radarr):
manifest declaring the capability and a path_rewrites global config schema,
the config parser (path rewrites only; no credentials), and the build
Makefile. Connection (base URL + API key) is delivered per poll in
PollChangesRequest.connection, never stored as plugin config.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add the arr HTTP client (X-Api-Key auth, 1 MiB response cap, request
timeout) and ChangedPaths, which polls /api/v3/history/since and extracts
imported file paths plus both old and new paths of renames; deletes and
grabs are ignored. Credentials are passed per call. Applies a 24h
max-lookback floor and a one-minute overlap buffer, and returns the newest
history timestamp as the next marker.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port ApplyRewrites and NormalizeSeparators: first-match prefix rewriting
that only matches at a path-segment boundary (so /data/media does not
rewrite /data/media2/x), and Windows backslash normalization so paths from
a Windows-hosted arr resolve on the Linux host.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ials from request)

Wire runtimeServer (embedded manifest, Configure parsing path_rewrites from
the host config entries) and scanSourceServer.PollChanges: read the arr
connection from the request (error on a nil/empty connection), parse the
RFC3339 marker (empty => now), poll arr history, normalize and rewrite each
path to Silo-native form, and return the changed paths plus the newest
timestamp as the next marker. Credentials come from the request, never config.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The returned marker was seeded from the rewound query window (caller marker
minus the overlap buffer) and only advanced to record dates. A poll that
returned no records therefore reported caller-marker minus overlap, so an
idle source crept its window back one overlap per poll until the 24h floor,
re-emitting the same imported/renamed paths and triggering redundant host
rescans.

Capture the caller's original marker before rewinding the query window and
floor the returned marker at it, so the marker advances to record dates but
never moves backward. The 24h lookback floor and 1-minute overlap rewind
still apply to the QUERY window only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spawns the real plugin binary via the SDK handshake (as silo's host does) and
drives PollChanges across the gRPC boundary: asserts the resolved connection's
api key reaches arr, the imported path returns Silo-native with a non-empty
marker, and a nil connection is rejected. Gated behind the 'integration' tag.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Scan all rewrites and pick the one with the longest normalized From
prefix instead of returning on the first match. This prevents a broad
rewrite (e.g. /data) listed before a nested one (e.g. /data/media/tv)
from shadowing the more-specific mapping. The caller's slice is never
mutated. Updated existing first-match test expectation and added a
shadowing regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A caller marker ahead of wall-clock (host/arr clock skew or a marker
minted by a faster-clocked arr) was used unchanged as the query since,
pushing the query window into the future and silently skipping real
events until wall-clock caught up. Fix clamps both the effective query
since and the no-regression marker floor to <= now before applying the
existing maxLookback floor and overlap rewind. The returned newest value
therefore also stays <= now. Added a regression test with an httptest
stub arr that asserts the query date param is <= now and the import is
found.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Aligns with the merged scan_source.v1 SDK contract: PollChangesResponse
renamed changed_paths -> source_paths (GetSourcePaths()), and path-rewrite
ownership moved to the host. The plugin now returns raw arr-side paths
directly from ChangedPaths() with no transformation. Consequently the
plugin has no configuration (global_config_schema: []) and Configure
becomes a no-op; internal/arr/rewrite.go and internal/config/ are deleted.
Tests updated to assert GetSourcePaths() and raw /data/... paths.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…l replace)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the unpaginated /history/since endpoint with paged GET
/api/v3/history?page=N&pageSize=200&sortKey=date&sortDirection=descending.
Records are date-descending; paging stops at the first record with
date <= querySince (the overlap-rewound window start), so we never
accumulate more than one window's worth of data per poll.  The per-page
1 MiB LimitReader cap in getJSON is preserved as a per-page safety net.
A 50-page hard cap bounds total work per poll.

Decode uses the paged envelope shape {page,pageSize,totalRecords,records:[...]}.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PollChanges previously ignored a parse failure on a non-empty marker,
leaving since as zero which the history client floors to "now" — silently
losing the polling position. Now an unparseable non-empty marker returns
an error so the host keeps the bad marker visible rather than quietly
skipping history. Empty marker (first run) is unchanged: it still
correctly means "from now".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a scan_source.v1 plugin for Sonarr/Radarr that polls /api/v3/history, extracts imported and renamed file paths, returns raw ARR paths and an opaque composite marker, embeds and stamps a manifest, and provides unit, integration, and build/test tooling.

Changes

Autoscan ARR Plugin

Layer / File(s) Summary
ARR history polling and path extraction
internal/arr/client.go, internal/arr/history.go, internal/arr/history_test.go
HTTP client with 1 MiB read cap and 30s timeout; composite Marker with ParseMarker/String; ChangedPaths computes query window (clamp/overlap/lookback), paginates newest-first /api/v3/history, emits import/rename paths (including both sides of renames), filters re-emission at caller boundary, tracks newest (Date,ID) marker, and includes unit tests for paging, boundary dedup, caps, and edge cases.
Plugin runtime, manifest, PollChanges
main.go, manifest.json, manifest_load_test.go
Embedded manifest.json with optional build-time version override and checksum stamping; runtime Configure no-op; PollChanges validates Connection, parses composite or RFC3339 markers, calls arr.ChangedPaths, and returns SourcePaths and NextMarker.
Unit and integration tests
main_test.go, internal/arr/history_test.go, e2e_test.go
Unit tests verify PollChanges request construction, header propagation, marker semantics, empty polls, invalid markers, and ChangedPaths behaviors; integration //go:build integration e2e builds the plugin binary, runs it via go-plugin, and exercises PollChanges against an httptest ARR history stub.
Build automation, deps, docs
go.mod, Makefile, .gitignore, README.md
Makefile adds build, test, lint, clean, and build-all cross-compile targets with version embedding; go.mod declares dependencies including silo-plugin-sdk and go-plugin; .gitignore excludes /plugin and /dist/; README.md documents polling flow, event extraction, config (none), and build/test instructions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop through ARR's recorded trails,
sniffing imports, tracking renamed tails,
markers stitched from second and id so neat,
tests keep paging safe and boundaries feet.
Build tools hum — the plugin's set to meet.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: Sonarr/Radarr scan_source.v1 plugin' accurately summarizes the main change: implementing a new scan_source.v1 plugin capability for Sonarr/Radarr (ARR) that polls the /api/v3/history endpoint and returns changed file paths.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (2)
Makefile (2)

17-18: ⚡ Quick win

Consider cleaning the dist/ directory.

The clean target only removes the single $(BINARY) artifact but leaves dist/ from build-all intact, potentially causing confusion with stale cross-platform binaries.

♻️ Proposed improvement
 clean:
-	rm -f $(BINARY)
+	rm -f $(BINARY)
+	rm -rf dist/
🤖 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 `@Makefile` around lines 17 - 18, The clean target currently only removes
$(BINARY) and leaves the dist/ directory behind; update the Makefile's clean
recipe (target "clean") to also remove the dist/ directory (or use the existing
variable for it if defined) using a safe recursive removal (e.g., rm -rf dist or
rm -rf $(DIST_DIR)) so artifacts produced by build-all are removed; ensure you
still remove $(BINARY) and any other temporary build outputs in the same target.

5-5: ⚡ Quick win

Consider a fallback for shallow clones.

The git describe command will fail in shallow clones (common in CI environments with depth=1). While the current 2>/dev/null suppresses errors, VERSION will be empty on failure.

🛡️ Proposed improvement
-VERSION ?= $(shell git describe --tags --always 2>/dev/null | sed 's/^v//')
+VERSION ?= $(shell git describe --tags --always 2>/dev/null | sed 's/^v//' || echo "dev")
🤖 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 `@Makefile` at line 5, The VERSION assignment can be empty in shallow clones
because git describe fails; update the Makefile's VERSION ?= line to try git
describe and, on failure, fall back to a short commit hash (git rev-parse
--short HEAD) and finally a safe default (e.g., 0.0.0). Modify the assignment
that defines VERSION to execute git describe --tags --always and if that exits
non-zero, run git rev-parse --short HEAD, and if that also fails use a literal
default string so the VERSION variable is never empty.
🤖 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 `@go.mod`:
- Line 22: Update the grpc dependency to a patched version to address
GHSA-p77j-4mvh-x3m3: add or bump the requirement for google.golang.org/grpc to
at least v1.79.3 (prefer v1.81.1) in go.mod, and if grpc is pulled indirectly
via hashicorp/go-plugin, update that module instead (or add a direct require for
google.golang.org/grpc v1.81.1) so the newer grpc is selected; then run go mod
tidy to refresh go.sum and vendor state and verify the final chosen version is
>= v1.79.3.

In `@internal/arr/history.go`:
- Around line 109-138: The current marker is a timestamp only, which causes
records with the same Date (same-second) to be permanently skipped; update the
marker to be a deterministic tie-breaker pair (timestamp + unique per-record
identifier) and use that for comparisons: when iterating pg.Records in the loop
in history.go, replace the single-field comparison using marker and rec.Date
(the `!rec.Date.After(marker)` check and the `if !rec.Date.After(marker) {
continue }` logic) with a composite comparison that treats (rec.Date, rec.ID) <=
(marker.Date, marker.ID) as already-seen; ensure the code that sets the next
marker (where `newest` / marker is updated after scanning pages) stores both the
UTC timestamp and the last-emitted record's unique ID so subsequent polls can
correctly include same-second records; refer to pg.Records, rec.Date, rec.ID (or
equivalent unique field), the marker variable, and the for-page fetch loop using
c.getJSON to locate and update the logic.
- Around line 101-107: The code seeds newest from querySince which causes the
wrong initial next_marker; change the initialization to seed newest from since
(replace "newest = querySince" with "newest = since") and keep the existing
clamp that updates newest when marker.After(newest) so the rest of the logic
(marker comparison) stays intact.

In `@Makefile`:
- Around line 20-24: The build-all target writes binaries into dist/ but never
ensures that directory exists; update the build-all recipe (target "build-all")
to create the output directory before the loop (e.g., run mkdir -p dist or
equivalent) so that the go build -o dist/$(BINARY)-... calls cannot fail when
dist is missing; keep the rest of the loop using PLATFORMS and the GOOS/GOARCH
expansion unchanged.

In `@README.md`:
- Line 17: Update the README to document the correct API endpoint used by the
plugin: replace the incorrect `/api/v3/history/since` reference with
`/api/v3/history` and mention that the implementation (see
internal/arr/history.go, function handling the history request around line ~97)
uses pagination query parameters (`page`, `pageSize`, `sortKey`,
`sortDirection`) rather than a `/since` endpoint; also keep or update the 24h
max-lookback note to reflect how the code enforces lookback when assembling
paginated results.
- Around line 47-48: The README's note about a local replace in go.mod is out of
sync with the current go.mod (which contains a pseudo-version for the SDK
dependency instead of a local replace); update the README.md text to accurately
reflect the current state (e.g., remove or reword the line "Until that is
tagged, `go.mod` carries a local `replace`" to mention the pseudo-version or
instructions for adding a local replace when needed), or alternatively add a
local replace entry into go.mod if the intent was to point at a local module
while waiting for a tag; reference the README.md sentence and the go.mod SDK
dependency pseudo-version when making the change so the documentation and
dependency declaration match.
- Line 38: Update the README line that currently says "make build          #
cross-platform binaries" to accurately reflect the Makefile targets: indicate
that "make build" builds only for the current platform and that "make build-all"
produces cross-platform binaries; reference the Makefile targets "build" and
"build-all" so readers use the correct command.

---

Nitpick comments:
In `@Makefile`:
- Around line 17-18: The clean target currently only removes $(BINARY) and
leaves the dist/ directory behind; update the Makefile's clean recipe (target
"clean") to also remove the dist/ directory (or use the existing variable for it
if defined) using a safe recursive removal (e.g., rm -rf dist or rm -rf
$(DIST_DIR)) so artifacts produced by build-all are removed; ensure you still
remove $(BINARY) and any other temporary build outputs in the same target.
- Line 5: The VERSION assignment can be empty in shallow clones because git
describe fails; update the Makefile's VERSION ?= line to try git describe and,
on failure, fall back to a short commit hash (git rev-parse --short HEAD) and
finally a safe default (e.g., 0.0.0). Modify the assignment that defines VERSION
to execute git describe --tags --always and if that exits non-zero, run git
rev-parse --short HEAD, and if that also fails use a literal default string so
the VERSION variable is never empty.
🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7f6aeae1-d044-49af-92c9-db6667ab41f7

📥 Commits

Reviewing files that changed from the base of the PR and between cf74a61 and 974a786.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (12)
  • .gitignore
  • Makefile
  • README.md
  • e2e_test.go
  • go.mod
  • internal/arr/client.go
  • internal/arr/history.go
  • internal/arr/history_test.go
  • main.go
  • main_test.go
  • manifest.json
  • manifest_load_test.go

Comment thread go.mod Outdated
Comment thread internal/arr/history.go Outdated
Comment thread internal/arr/history.go
Comment thread Makefile
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
fluxis and others added 4 commits June 3, 2026 19:33
Patches authorization-bypass CVE-2026-33186 (fixed in >= v1.79.3).
grpc is pulled in transitively via the plugin SDK so it stays an
indirect require; the version pin is what closes the vulnerability.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…story

The poll marker was a bare second-granularity timestamp and the emit
filter was `!rec.Date.After(marker)`. When a poll stopped on a page
boundary or the maxHistoryPages cap mid-second, the remaining records
sharing that second were filtered out forever on the next poll, silently
missing rescans during bulk imports.

- Add `ID` to historyRecord (arr records carry a numeric id).
- Make the marker a composite (date, id), encoded "<RFC3339>|<id>".
  ParseMarker accepts a bare RFC3339 string as (date, id=0) for backward
  compat with already-stored v1 markers.
- Emit a record iff it is strictly after the caller's (date, id) marker
  (markerLess), so a same-second straggler from a prior page-bounded poll
  is emitted on the next poll instead of being dropped.
- Warn (not silently truncate) when the maxHistoryPages cap is hit before
  reaching the marker boundary; the composite marker lets the next poll
  resume exactly where this one stopped.
- Seed the returned marker from the effective `since` (not the rewound
  querySince) so an empty/first poll does not replay the overlap window.
- main.go parses via the backward-compatible ParseMarker and formats the
  outgoing NextMarker in composite form; the host treats it opaquely.

Adds TestChangedPathsSameSecondStragglerNotDropped covering the split
poll-A/poll-B same-second case, and tightens the empty-marker test to
assert the seed is ~now not since-overlap.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- README: the plugin queries /api/v3/history (paginated page/pageSize/
  sortKey/sortDirection), not /api/v3/history/since; also note the
  composite next_marker format.
- README: `make build` builds only the current platform; cross-platform
  is `make build-all`.
- README: go.mod uses an SDK pseudo-version, not a local replace.
- Makefile: build-all now `mkdir -p dist` before writing dist/ binaries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the previously-undocumented exported and notable unexported
declarations the docstring-coverage check flagged: GetManifest, main,
loadManifest, newClient, the arr history event-type constants, and the
new Marker / ParseMarker / String / markerLess marker helpers. Comments
follow Go convention (full sentences, leading identifier name) and note
non-obvious contracts (marker round-trip / overlap rationale).

Coverage over top-level declarations is now 29/29 (100%), comfortably
above the 80% threshold.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
main_test.go (1)

138-140: ⚡ Quick win

Assert full composite marker monotonicity (date,id), not date only.

These checks only guard timestamp regression. With composite markers, same-second regressions (id decreasing) can slip through undetected. Please compare (Date, ID) lexicographically in both tests.

Suggested test assertion update
-	if secondMarker.Date.Before(firstMarker.Date) {
-		t.Fatalf("returned marker regressed: %v < caller marker %v", secondMarker.Date, firstMarker.Date)
-	}
+	if secondMarker.Date.Before(firstMarker.Date) ||
+		(secondMarker.Date.Equal(firstMarker.Date) && secondMarker.ID < firstMarker.ID) {
+		t.Fatalf("returned marker regressed: got (%v,%d), want >= (%v,%d)",
+			secondMarker.Date, secondMarker.ID, firstMarker.Date, firstMarker.ID)
+	}
-	if got.Date.Before(prev) {
-		t.Fatalf("empty-poll marker regressed: got %v, want >= caller marker %v (not prev-overlap)", got.Date, prev)
-	}
+	prevMarker, _ := arr.ParseMarker(prev.Format(time.RFC3339))
+	if got.Date.Before(prevMarker.Date) ||
+		(got.Date.Equal(prevMarker.Date) && got.ID < prevMarker.ID) {
+		t.Fatalf("empty-poll marker regressed: got (%v,%d), want >= (%v,%d)",
+			got.Date, got.ID, prevMarker.Date, prevMarker.ID)
+	}

Also applies to: 170-172

🤖 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 `@main_test.go` around lines 138 - 140, The current assertions only compare
marker.Date and miss regressions when Date is equal but ID decreases; update the
check that compares firstMarker and secondMarker to perform a lexicographic
comparison on (Date, ID): fail if secondMarker.Date is before firstMarker.Date
OR if dates are equal and secondMarker.ID is less than firstMarker.ID. Apply the
same change to the other similar assertion (the later check that currently
compares only Date).
🤖 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/arr/history.go`:
- Around line 222-225: The current loop updates newest using markerLess for
every record scanned (newest, Marker{Date: recDate, ID: rec.ID}) which lets an
early exit due to maxHistoryPages advance the returned marker past unvisited
older pages; instead, stop advancing the global newest when you will exit early:
track a separate newestCandidate for records on the current page (or the head
marker of the last fully scanned page) and only assign newest = newestCandidate
when you have not hit maxHistoryPages (or when you finish scanning all pages up
to callerBoundary). Concretely, keep the existing newest, add a page-local
candidate (e.g., pageNewest) updated with markerLess inside the per-page loop,
and on normal continuation promote pageNewest into newest; if you break out
because maxHistoryPages was reached, do not promote pageNewest so the returned
Marker does not skip older pages; use the existing symbols newest, Marker,
markerLess, maxHistoryPages and callerBoundary to locate and implement this
change.
- Around line 194-197: The code sets newest = Marker{Date: sinceDate} which
drops the caller's since.ID and causes cursor regression on idle polls; fix by
preserving the composite (Date, ID) ordering: initialize newest with both fields
(e.g., newest = Marker{Date: sinceDate, ID: sinceID}) and/or change the
comparison to consider ID when dates are equal (if
marker.Date.After(newest.Date) || (marker.Date.Equal(newest.Date) && marker.ID >
newest.ID) { newest = marker }) so the caller's full (Date,ID) marker is kept
when no newer records exist.

---

Nitpick comments:
In `@main_test.go`:
- Around line 138-140: The current assertions only compare marker.Date and miss
regressions when Date is equal but ID decreases; update the check that compares
firstMarker and secondMarker to perform a lexicographic comparison on (Date,
ID): fail if secondMarker.Date is before firstMarker.Date OR if dates are equal
and secondMarker.ID is less than firstMarker.ID. Apply the same change to the
other similar assertion (the later check that currently compares only Date).
🪄 Autofix (Beta)

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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f1bd3377-c370-4d8f-aab3-991fe0f2150c

📥 Commits

Reviewing files that changed from the base of the PR and between 974a786 and 463ba3f.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (8)
  • Makefile
  • README.md
  • go.mod
  • internal/arr/client.go
  • internal/arr/history.go
  • internal/arr/history_test.go
  • main.go
  • main_test.go
✅ Files skipped from review due to trivial changes (1)
  • README.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • Makefile
  • internal/arr/client.go
  • main.go

Comment thread internal/arr/history.go
Comment thread internal/arr/history.go
Comment on lines +222 to +225
// Track the newest (Date, ID) processed across all pages.
if markerLess(newest, Marker{Date: recDate, ID: rec.ID}) {
newest = Marker{Date: recDate, ID: rec.ID}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Page-cap polls still drop part of the backlog.

This tracks the newest (Date, ID) seen while scanning descending pages. If maxHistoryPages is hit before the caller boundary, the returned marker jumps to page 1’s head, so the next poll permanently skips the older unvisited pages from this run. That breaks bulk-import recovery despite the warning saying those records will be processed later.

Also applies to: 262-267

🤖 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/arr/history.go` around lines 222 - 225, The current loop updates
newest using markerLess for every record scanned (newest, Marker{Date: recDate,
ID: rec.ID}) which lets an early exit due to maxHistoryPages advance the
returned marker past unvisited older pages; instead, stop advancing the global
newest when you will exit early: track a separate newestCandidate for records on
the current page (or the head marker of the last fully scanned page) and only
assign newest = newestCandidate when you have not hit maxHistoryPages (or when
you finish scanning all pages up to callerBoundary). Concretely, keep the
existing newest, add a page-local candidate (e.g., pageNewest) updated with
markerLess inside the per-page loop, and on normal continuation promote
pageNewest into newest; if you break out because maxHistoryPages was reached, do
not promote pageNewest so the returned Marker does not skip older pages; use the
existing symbols newest, Marker, markerLess, maxHistoryPages and callerBoundary
to locate and implement this change.

…rning

- Idle/empty polls now keep the caller's full (Date, ID) marker via markerLess
  instead of comparing Date only, so same-second records are not re-emitted next
  poll (CodeRabbit: history.go:197).
- The page-cap warning no longer claims skipped records are recovered later:
  arr's offset-paginated, newest-first /history has no date filter, so deeper
  pages cannot be resumed. Reworded to the truth and raised the per-poll page
  budget (50 -> 100) for more burst headroom (CodeRabbit: history.go:225).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/arr/history.go (1)

163-183: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize the emit boundary when the caller marker is empty or future-dated.

sinceDate is rewritten to now, but marker still comes from the raw input. That makes an empty marker emit the entire overlap window on the first poll, and a future marker can keep a bogus same-second ID after its date is clamped. Build marker from the normalized boundary in those cases before it is used for emission and idle-marker preservation.

Suggested fix
 	now := time.Now().UTC()
-	// marker is the caller's original (Date, ID); the returned marker must never
-	// regress below it.
-	marker := Marker{Date: since.Date.UTC(), ID: since.ID}
 	sinceDate := since.Date
 	if sinceDate.IsZero() {
 		sinceDate = now
 	}
 	sinceDate = sinceDate.UTC()
@@
 	if sinceDate.After(now) {
 		sinceDate = now
 	}
-	if marker.Date.After(now) {
-		marker.Date = now
-	}
+	// Use the caller marker for historical polls, but treat empty/future markers
+	// as "from now" for emission as well.
+	marker := Marker{Date: since.Date.UTC(), ID: since.ID}
+	if since.Date.IsZero() || marker.Date.After(now) {
+		marker = Marker{Date: sinceDate}
+	}
🤖 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/arr/history.go` around lines 163 - 183, The marker is built from the
raw input before sinceDate is normalized, which lets an empty or future-dated
caller marker emit the full overlap or retain a bogus same-second ID after
clamping; fix this by performing the sinceDate normalization/clamping first
(operate on sinceDate and clamp to now), then construct marker := Marker{Date:
sinceDate.UTC(), ID: since.ID} from that normalized boundary, and if you changed
the date because it was zero or clamped to now, reset marker.ID to the
zero-value (e.g., empty string or 0) so a preserved ID from the original
future/empty input cannot persist; update uses of marker afterward to rely on
this normalized marker.
♻️ Duplicate comments (1)
internal/arr/history.go (1)

268-282: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't advance the cursor past unvisited pages when the page cap is hit.

This still drops backlog permanently: by the time this warning runs, newest has already advanced to records from the scanned head pages, so the next poll filters out every older record that lived on the unvisited pages. Rewording the log helps operators, but it doesn't make the poll result safe to consume. To preserve correctness, fail the poll or keep the caller marker unchanged when maxHistoryPages is exhausted.

🤖 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/arr/history.go` around lines 268 - 282, The current logic advances
the cursor/newest even when !reachedBoundary after hitting maxHistoryPages,
which permanently drops older pages; instead, when reachedBoundary is false and
you've exhausted maxHistoryPages (using historyPageSize), do NOT update the
caller marker/newest and make the poll return a failure (or explicit "page cap
hit" error) so the caller can retry with no marker advancement; update the code
around the page-scan completion path that currently logs and sets newest to
preserve the caller marker (or return an error) and include maxHistoryPages and
historyPageSize in the error/context.
🤖 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.

Outside diff comments:
In `@internal/arr/history.go`:
- Around line 163-183: The marker is built from the raw input before sinceDate
is normalized, which lets an empty or future-dated caller marker emit the full
overlap or retain a bogus same-second ID after clamping; fix this by performing
the sinceDate normalization/clamping first (operate on sinceDate and clamp to
now), then construct marker := Marker{Date: sinceDate.UTC(), ID: since.ID} from
that normalized boundary, and if you changed the date because it was zero or
clamped to now, reset marker.ID to the zero-value (e.g., empty string or 0) so a
preserved ID from the original future/empty input cannot persist; update uses of
marker afterward to rely on this normalized marker.

---

Duplicate comments:
In `@internal/arr/history.go`:
- Around line 268-282: The current logic advances the cursor/newest even when
!reachedBoundary after hitting maxHistoryPages, which permanently drops older
pages; instead, when reachedBoundary is false and you've exhausted
maxHistoryPages (using historyPageSize), do NOT update the caller marker/newest
and make the poll return a failure (or explicit "page cap hit" error) so the
caller can retry with no marker advancement; update the code around the
page-scan completion path that currently logs and sets newest to preserve the
caller marker (or return an error) and include maxHistoryPages and
historyPageSize in the error/context.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 40ca7cc2-dc7e-4044-8204-975b0933f570

📥 Commits

Reviewing files that changed from the base of the PR and between 463ba3f and 9c8835c.

📒 Files selected for processing (2)
  • internal/arr/history.go
  • internal/arr/history_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/arr/history_test.go

@Quick104
Quick104 merged commit 884c562 into Silo-Server:main Jun 3, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants