Repository navigation
Conversation
…lled URL Clearing a source's marker on an edit made its next poll start from now, but last_run_at stayed, so the source still waited out its interval before that poll. Whatever the upstream imported in between was never reported. Clear last_run_at with the marker in UpdateSource and UpdateConnection, so the next cycle polls the source and the gap ends at the next run of the poll task. UpdateConnection also reset markers when the base URL was only spelled differently (a trailing slash, a different host case), which skipped history for no reason. Compare base URLs with the scheme and host lower-cased and trailing slashes dropped. Run the two reset tests as database contracts so CI covers them.
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:
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
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 |
Quick104
added
priority: P2
Cursor
|
The PR explains why a running capture was unavailable, but the database test output does not show the user-facing changes. Please add this evidence if available:
Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
![silo-kody[bot]](https://avatars.githubusercontent.com/u/287058192?s=60&v=4){kind=small}
| u.Path = strings.TrimRight(u.Path, "/") | ||
| u.RawPath = "" |
There was a problem hiding this comment.
URL normalization collisions occur when clearing RawPath makes distinct URLs such as http://host/a%2Fb and http://host/a/b compare equal, even though the resolver forwards their original escaped paths to the plugin. Preserve escaped path distinctions during normalization so edits between separately routed upstreams do not retain the old continuation token and let AdvanceMarker accept an old-upstream poll result.
Prompt for LLM
File internal/autoscan/repository.go:
Line 252 to 253:
URL normalization collisions occur when clearing RawPath makes distinct URLs such as http://host/a%2Fb and http://host/a/b compare equal, even though the resolver forwards their original escaped paths to the plugin. Preserve escaped path distinctions during normalization so edits between separately routed upstreams do not retain the old continuation token and let AdvanceMarker accept an old-upstream poll result.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Good catch, fixed in 7f7bfeb: the comparison now trims the trailing slash from both Path and RawPath instead of dropping RawPath, so /a%2Fb and /a/b stay different. There's a test for both.
There was a problem hiding this comment.
Thanks for fixing it. Preserving RawPath keeps /a%2Fb distinct from /a/b, and the regression test covers the reported collision. Based on your description of 7f7bfeb59, the original finding is addressed.
| if _, err := tx.Exec(ctx, ` | ||
| UPDATE autoscan_sources | ||
| SET marker = NULL, updated_at = now() | ||
| SET marker = NULL, last_run_at = NULL, updated_at = now() |
There was a problem hiding this comment.
Incomplete last_run_at resets exclude bound sources whose marker is already NULL, including sources whose initial poll failed and recorded a recent run time, leaving them waiting out their interval after an upstream change. Remove the marker-presence predicate so every bound source receives the scheduling reset.
SET marker = NULL, last_run_at = NULL, updated_at = now()
WHERE connection_id = $1Prompt for LLM
File internal/autoscan/repository.go:
Line 206:
Incomplete last_run_at resets exclude bound sources whose marker is already NULL, including sources whose initial poll failed and recorded a recent run time, leaving them waiting out their interval after an upstream change. Remove the marker-presence predicate so every bound source receives the scheduling reset.
Suggested Code:
SET marker = NULL, last_run_at = NULL, updated_at = now()
WHERE connection_id = $1
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Agreed, fixed in 7f7bfeb. The reset now also clears last_run_at for bound sources that have no marker but have run, such as one whose first poll failed, so it retries the new URL at the next cycle. The connection reset test now includes such a source.
There was a problem hiding this comment.
Thanks—that addresses the reported gap. Clearing last_run_at regardless of whether a bound source has a marker lets a source whose initial poll failed retry the changed upstream at the next cycle. Including that case in the connection reset test covers the regression.
… apart A connection URL change skipped bound sources that had no marker, such as one whose first poll failed, so after fixing the URL that source still waited out its interval. Clear last_run_at for those too. Clearing RawPath made an escaped slash (%2F) compare equal to a path separator. Trim the trailing slash from both forms instead.
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:
|
|
The latest commits add two user-visible cases: a connection change now clears Suggested fix: add before-and-after admin source-row captures and matching API excerpts for the failed-first-poll reset, plus a recording or same-query before-and-after showing pickup after an escaped-path upstream switch. Identify the build or commit, and include desktop and phone-width captures if the status change is visible at both widths. Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness. Posted via Macroscope — Visible change evidence |
2 participants
Problem
Related issue: #1230
Validation tasks: touches #1231 C3 and #1232 C1 (neither has a result yet)
When an admin edit resets an autoscan source's poll marker (#1935: a new connection or
source_config, or a connection pointed at another server), the source's next poll starts from now, which is the documented behaviour. Butlast_run_atstays, so the scheduler still waits out the source's interval from its last run before that first poll. Anything the upstream imports between the edit and that poll is never reported. With the default 10-minute cycle and a source set to poll hourly, that's up to an hour of imports that never get a targeted scan. This change clearslast_run_atalong with the marker, so the source polls at the next cycle.A second, smaller case:
UpdateConnectiontreats a base URL that is only spelled differently (http://Sonarr:8989saved ashttp://sonarr:8989/) as a new server and resets every bound source's marker, which skips history for no reason.Approach
UpdateSourceclearslast_run_atin the same statement and under the same condition as the marker (connection orsource_configchanged).UpdateConnectionclears it in the same statement as the bound sources' markers, including for a bound source that has run but has no marker yet (its first poll failed), so it retries the new URL at the next cycle. Other edits keep both.Service.pollalready treats a source with nolast_run_atas due, so the gap now ends at the next run of the poll task. If that first poll fails,RecordErrorstampslast_run_atagain and the normal interval applies.connectionUpstream.differsFromcompares base URLs with the scheme and host lower-cased, surrounding space trimmed and trailing slashes dropped. A different port, scheme or path still counts as a different server, and an escaped slash (%2F) still differs from a path separator.AdvanceMarkeruses the same comparison, so a poll that overlaps such an edit still stores its marker.TestUpdateSourceMarkerResetandTestUpdateConnectionMarkerResetare now database contracts, soGo DB pinsruns them. Today no CI job sets a database for them.What an admin sees: right after an edit that resets the marker, the source list shows "Not run yet" (or "Last poll failed" without a time, if the last poll failed) until the next cycle polls it, and the API returns
last_run_at: nullfor that source in the meantime. Webhook sources aren't affected.Validation
last_run_at: cleared with the marker on a connection switch, an unbind, asource_configchange and a connection URL change; kept on every edit that keeps the marker. Two new connection cases check that a trailing slash or a different host case keeps the markers, and every connection case includes a source whose first poll failed. Unit tests cover the URL comparison, including escaped paths. Against a local migrated PostgreSQL 18 database, all of these fail on ca186fe (last_run_atkept, markers reset for the respelled URLs) and pass on this branch.go test ./internal/autoscan/passes with and without a database.make lint-changed: clean.Go DB pins, which now runs the two reset tests (run).Benchmarks
Not applicable. One more column in two existing updates.
Evidence
Evidence: https://evidence.siloserver.org/r/silo-server/pr-2080/
The change a user can see is the status text and
last_run_atabove, between an edit and the next cycle. I don't have a capture from a running server; the evidence is the database test output.Risks
http://hostandhttp://host:80) still count as different servers, as before.Checklist
AI Disclosure
AI-assisted with Claude Opus. I directed the task and designed the work.