Repository navigation
docs(scan-source): document the scan source contract and add an example plugin - #37
Conversation
A minimal scan_source.v1 plugin that reads changed paths from a change-log file. It shows the metadata.scan_source setup descriptor, reading a per-source setting from source_config, marker handling (empty marker starts from now, truncated logs resynchronize), and structured FILE/SUBTREE changes. Unit tests cover the marker behavior and the manifest descriptor.
Add a developer guide for scan_source.v1 plugins: the setup descriptor in metadata.scan_source, how source_config and the resolved connection arrive on each poll, marker rules, change scopes, and what the host does after a poll (rewrites, matching, debounce, queueing, activity), and link it and the example from the README.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change documents the scan-source capability and adds ChangesScan-source capability
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant Silo
participant hello-scan-source
participant Changelog
Silo->>hello-scan-source: PollChanges request with marker and configuration
hello-scan-source->>Changelog: Read complete lines after marker
Changelog-->>hello-scan-source: Changelog lines
hello-scan-source-->>Silo: Changes and continuation marker
Silo->>Silo: Rewrite paths, match libraries, and queue scans
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established that would prevent merging after normal checks and the stated server prerequisites. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 2 files. (6 skipped: 6 unsupported.)
✨ 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 |
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}
|
|
||
| // readLines returns the non-blank lines of the change log. A missing file is | ||
| // an empty log, so a source can be created before the other tool first writes. | ||
| func readLines(path string) ([]string, error) { |
There was a problem hiding this comment.
Unbounded I/O and memory growth occur because readLines scans the entire change-log and retains every non-blank line on every poll, causing O(total log size) work and allocation even though the marker only needs the unread suffix. Track a persisted byte offset or bounded cursor-based log position, return only entries after that cursor, and detect rotation or truncation explicitly.
// Track a byte offset in the marker and read only newly appended records;
// reset the offset when the file is truncated or replaced.
func readLines(path string, offset int64) ([]string, int64, error) {
f, err := os.Open(path)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return nil, 0, nil
}
return nil, offset, fmt.Errorf("open change log: %w", err)
}
defer func() { _ = f.Close() }()
if _, err := f.Seek(offset, io.SeekStart); err != nil {
return nil, offset, fmt.Errorf("seek change log: %w", err)
}
// Scan only the appended suffix and return the new byte offset.
// Handle file-size < offset as rotation/truncation and resynchronize.
}Prompt for LLM
File examples/hello-scan-source/main.go:
Line 101:
Unbounded I/O and memory growth occur because `readLines` scans the entire change-log and retains every non-blank line on every poll, causing O(total log size) work and allocation even though the marker only needs the unread suffix. Track a persisted byte offset or bounded cursor-based log position, return only entries after that cursor, and detect rotation or truncation explicitly.
Suggested Code:
// Track a byte offset in the marker and read only newly appended records;
// reset the offset when the file is truncated or replaced.
func readLines(path string, offset int64) ([]string, int64, error) {
f, err := os.Open(path)
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return nil, 0, nil
}
return nil, offset, fmt.Errorf("open change log: %w", err)
}
defer func() { _ = f.Close() }()
if _, err := f.Seek(offset, io.SeekStart); err != nil {
return nil, offset, fmt.Errorf("seek change log: %w", err)
}
// Scan only the appended suffix and return the new byte offset.
// Handle file-size < offset as rotation/truncation and resynchronize.
}
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.
Fixed in c91ac85: the marker is now the byte offset just past the last complete line reported, so each poll seeks there and reads only what was appended. A line still being written waits for the next poll, and a marker past the end (truncated or rotated log) still resynchronizes. Tests cover each case.
There was a problem hiding this comment.
Thanks—this correctly handles incremental reads, incomplete trailing lines, and log truncation or rotation. The test coverage for each case is a good addition.
| consumed, err := strconv.Atoi(req.GetMarker()) | ||
| if err != nil || consumed < 0 || consumed > len(lines) { | ||
| // The file was truncated or rotated, or the marker is not ours. | ||
| // Resynchronize to the end instead of replaying the whole file. | ||
| return resp, nil |
There was a problem hiding this comment.
The marker validation accepts a replacement or rotated log with at least the old number of nonblank lines because it checks only consumed > len(lines), causing the plugin to read from an unrelated offset, report new entries as old, or skip entries permanently. Store a verifiable file identity or continuity token, such as inode plus byte offset and file size/content checks, and resynchronize when the current file cannot be proven to match the marker.
// Use a marker containing file identity and a byte offset; reject/resync
// when the current file identity or prefix continuity does not match.
marker, err := parseMarker(req.GetMarker())
if err != nil || !marker.MatchesCurrentFile(path, lines) {
return resp, nil
}
// Read only entries after the verified marker position.Prompt for LLM
File examples/hello-scan-source/main.go:
Line 69 to 73:
The marker validation accepts a replacement or rotated log with at least the old number of nonblank lines because it checks only `consumed > len(lines)`, causing the plugin to read from an unrelated offset, report new entries as old, or skip entries permanently. Store a verifiable file identity or continuity token, such as inode plus byte offset and file size/content checks, and resynchronize when the current file cannot be proven to match the marker.
Suggested Code:
// Use a marker containing file identity and a byte offset; reject/resync
// when the current file identity or prefix continuity does not match.
marker, err := parseMarker(req.GetMarker())
if err != nil || !marker.MatchesCurrentFile(path, lines) {
return resp, nil
}
// Read only entries after the verified marker position.
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.
Documented rather than changed: this is the minimal example, and a portable continuity check (inode via build-tagged syscalls, or a checksum of the bytes before the offset) would roughly double it. The README now says the example cannot tell a replaced longer file from one that grew, that the log should be rotated by truncating it or starting the new file empty, and that a production plugin should add a file identity or continuity check to its marker. Truncation is still detected, and since c91ac85 the marker is a byte offset.
There was a problem hiding this comment.
Understood. No changes or saved records have been made.
The example re-read the whole change log on every poll. Use a byte offset as the marker so each poll reads only what was appended, leave a line that is still being written for the next poll, and keep resynchronizing when the log is truncated.
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:
|
| offset, err := strconv.ParseInt(req.GetMarker(), 10, 64) | ||
| size, statErr := fileSize(path) | ||
| if statErr != nil { | ||
| return nil, statErr | ||
| } | ||
| if err != nil || offset < 0 || offset > size { | ||
| // The file was truncated or rotated, or the marker is not ours. | ||
| // Resynchronize to the end instead of replaying the whole file. | ||
| end, err := completeLinesEnd(path, 0) |
There was a problem hiding this comment.
Marker validation accepts a replacement or rotated log when the old byte offset is no greater than the new file size, causing readAppended to seek into unrelated content and report it as new changes or advance past entries permanently. Persist a verifiable file identity and continuity token with the offset, such as inode plus offset and size/content metadata, and resynchronize to the current complete-line end when the current file cannot be proven to be the same log.
info, statErr := os.Stat(path)
if statErr != nil {
return nil, statErr
}
marker, err := decodeMarker(req.GetMarker())
if err != nil || marker.Offset < 0 || marker.Offset > info.Size() || marker.FileID != fileID(info) {
end, err := completeLinesEnd(path, 0)
if err != nil {
return nil, err
}
return &pluginv1.PollChangesResponse{NextMarker: encodeMarker(fileID(info), end)}, nil
}Prompt for LLM
File examples/hello-scan-source/main.go:
Line 68 to 76:
Marker validation accepts a replacement or rotated log when the old byte offset is no greater than the new file size, causing `readAppended` to seek into unrelated content and report it as new changes or advance past entries permanently. Persist a verifiable file identity and continuity token with the offset, such as inode plus offset and size/content metadata, and resynchronize to the current complete-line end when the current file cannot be proven to be the same log.
Suggested Code:
info, statErr := os.Stat(path)
if statErr != nil {
return nil, statErr
}
marker, err := decodeMarker(req.GetMarker())
if err != nil || marker.Offset < 0 || marker.Offset > info.Size() || marker.FileID != fileID(info) {
end, err := completeLinesEnd(path, 0)
if err != nil {
return nil, err
}
return &pluginv1.PollChangesResponse{NextMarker: encodeMarker(fileID(info), end)}, nil
}
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.
Not changing this in the example, for the reason in the earlier thread: a portable continuity check would roughly double a minimal example. The README states the limit and tells production plugins to add a file identity or prefix check to the marker.
There was a problem hiding this comment.
Understood. Keeping the example minimal is a reasonable trade-off given the README’s explicit limitation and guidance for production plugins.
The rotation/replacement risk is real, but with that boundary documented, I wouldn’t treat it as a high-severity issue or a merge blocker for this example. No continuity-check implementation is needed here.
There was a problem hiding this comment.
Partly addressed in cbb3ff3. When the marker is in range, the example now checks that the byte before it is a newline, and reads the log from the start when it isn't. That catches a rotated or replaced log whenever the marker lands mid-line in the new content. A replacement that happens to have a newline at the old offset is still missed. The README says so and keeps the advice to add a file identity or prefix check in a production plugin.
There was a problem hiding this comment.
Thanks—that partially addresses the finding. The newline check detects offsets that no longer fall on a line boundary, but it cannot establish log continuity: a replacement with a newline at the old offset can still skip entries or report unrelated content.
Given this is a minimal example and the README explicitly documents that limitation and recommends a file identity or prefix check for production, I wouldn’t insist on the full continuity mechanism here. The remaining risk is documented, not fully fixed.
Quick104
left a comment
Quick104
left a comment
There was a problem hiding this comment.
Code review of the scan-source guide and example at ca476d3, with the guide checked against Silo server main with #1931 and #1934 applied.
There are 10 inline findings. Two are high: in the example, truncating the log loses the new lines, and a briefly missing log causes the whole log to be replayed later. The rest are places where the guide or the unchanged proto comments disagree with the server. Four are marked "needs your call" because the fix could go in the server or a follow-up PR instead. go vet, go test, and gofmt pass on this head.
AI disclosure: reviewed by claude-opus-5-5 in Claude Code via T3 Code (/silo code-review).
| that changed, was deleted, or reappeared is always scanned. Directory | ||
| reports (including `SUBTREE` changes and directories reported as `AUTO`) | ||
| are never debounced; the scan queue merges them with a matching scan that |
There was a problem hiding this comment.
Medium, needs your call: a deleted directory is debounced, even though this says directory reports never are.
Trigger: a plugin reports a deleted directory as SUBTREE and reports it again within the 60-second window. observePathState in internal/autoscan/suppress.go returns stateAbsent with debounce enabled on ErrNotExist, claimTarget calls ShouldScan, and the repeat is suppressed and counted in scans suppressed. Only directories that still exist skip the debounce. (Checked against server main with #1931 applied.)
The deleted-directory advice at lines 205-210 relies on this exemption. Either narrow the sentence to "Existing directories are never debounced; a missing directory is debounced like a deleted file", or change the server so missing subtrees are exempt too.
There was a problem hiding this comment.
Narrowed the guide in 4160c47: reports of an existing directory are never debounced, and a missing path, directory or file, is debounced like a deleted file. Whether the server should exempt missing subtrees is left for a separate change.
| normal interval, not immediately. | ||
| - Silo gives `PollChanges` up to five minutes and runs at most one poll per |
There was a problem hiding this comment.
Low, needs your call: both guarantees in this sentence are weaker than stated.
- Five minutes: the v1 manual trigger (
handlers/autoscan.go,HandleTrigger) runs the whole poll cycle under one 5-minute context, and the plugin host keeps an existing earlier deadline (ensureDeadlineinpluginhost/client.go). A source polled late in a manual cycle gets only the time that is left. Scheduled polls and the v2 trigger do get five minutes per call. - One poll per source: the only exclusion is the
runningevent row. A node restart marks every running event as errored, with no node filter (MarkInterruptedEvents), so another node can start a second poll with the same marker.AdvanceMarkeris last-writer-wins. I have not run this on two nodes, so treat this part as plausible rather than confirmed.
Impact: a plugin that uses a consuming cursor (one that acks queue messages) and trusts "at most one poll" can lose changes. A safer wording: "Silo normally gives PollChanges up to five minutes and avoids overlapping polls of one source, but does not guarantee either. Keep polls side-effect free so a repeated or overlapping poll is harmless." Whether to fix the server instead is your decision.
There was a problem hiding this comment.
Used your safer wording in 4160c47: Silo normally gives five minutes and avoids overlapping polls but guarantees neither, so polls should be side-effect free. Server-side changes are left for a separate decision.
…s missing A marker past the end of the log now means the log was truncated, so the poll reads it from the start instead of skipping what was appended since. A log that goes missing after the source has read it now fails the poll, so the host keeps the marker and the log is not replayed when it returns.
…polling behavior State that one bad config_form field drops the whole form, that fill_from values are newline-separated, which library kinds keep FILE changes exact or skip them, that a missing directory is debounced, that the poll timeout and overlap protection are not guarantees, and that polls can run on any node.
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:
|
source_paths now says it is legacy and handled like AUTO (parent directory), changes says when a missing directory is not reconciled, and each ScanSourceChangeScope value says what the host does with it.
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:
|
Quick104
left a comment
Quick104
left a comment
There was a problem hiding this comment.
Second-round review of the scan-source guide and example at 912db4c. The guide and proto were checked against Silo server main (#1931 and #1934 are merged there).
The 10 findings from the first round are resolved, and the new tests cover the truncation and missing-log cases. This round has 10 new findings, each on its line below. One is in the example: the rotation the README recommends can drop a line and send a broken path. The rest are places where the guide or the new proto comments say something the server or the example doesn't do. I'm fixing all ten in commits on this branch. One part of one finding needs your call: whether the server should record two failure types on the source instead of the guide documenting that it doesn't. A summary with the verdict follows after the push.
AI disclosure: reviewed by claude-opus-5-5 in Claude Code via T3 Code (/silo review), with read-only review subagents.
| - `PollChanges` returned an error or timed out; | ||
| - matching any path failed with an internal error, even if other paths | ||
| matched. Scans already queued for the matched paths stay queued; | ||
| - the scans could not be queued, or the new marker could not be saved. |
There was a problem hiding this comment.
Medium, needs your call: these two failures don't record the error on the source.
On an enqueue failure and on an AdvanceMarker error, internal/autoscan/service.go only finishes the event with an error status. It never calls RecordError, which is the call that sets last_error and stamps last_run_at. The source's status column, built from last_error and last_run_at, keeps its previous value, so the error shows only in the activity log. Because last_run_at isn't stamped, the source polls again on the next global cycle and ignores a longer per-source interval, which also contradicts line 249 ("A failed poll also counts as a run").
Repair: I'm making the guide describe current behavior, and lines 171-177 and 249-250 say these two failures appear only in the activity log and retry on the next cycle. I'm fixing this in a commit on this branch, so there's no need to change it yourself. Whether the server should call RecordError on these paths is a separate decision.
There was a problem hiding this comment.
The guide part is fixed in f9e7828. The keep-marker list now names only the three paths that call RecordError, and a separate paragraph says enqueue and marker-save failures show only in the activity log and poll again on the next cycle. The Cadence bullet says the same. I'm leaving this thread open for the server question: should those two paths call RecordError?
…e marker A truncated log that grew past the old marker before the next poll was read from the old offset, which dropped the lines before it and sent the rest of a line as a FILE path. Every marker the example writes ends just past a newline, so a marker that no longer follows one now reads the log from the start. Resynchronizing to the end now reads the log in blocks instead of keeping every line, and the README states when rotation is detected and the build command for ARM servers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…or text as the host does Read a truncated log from the start, matching the example. Say when Silo clears the marker, that enqueue and marker-save failures appear only in the activity log, that a required connection is not guaranteed, that the Unavailable code shows as "Plugin unavailable.", that mixed libraries count for both fill_from values, and how to build for an ARM server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The FILE comments said every deleted file is reconciled and every file is scanned. Non-media files and podcast files are skipped, and a missing file is marked missing only while the library root is mounted, as the guide says. Comments only; pluginv1 regenerated. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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:
|
Review summaryReviewed head: AddressedFirst round (raised at Second round (raised at
Checked: Remaining
Validation, manual, parity
VerdictFit: 9/10 · Readiness: 9/10 — ready
AI disclosure: reviewed and fixed by claude-opus-5-5 in Claude Code via T3 Code (/silo review), with read-only review subagents (claude-opus-5-5 and claude-sonnet-5-5). |
v0.24.0 adds per-connection settings for watch-sync plugins (Silo-Server/silo-plugin-sdk#39) and documents the scan source contract with an example plugin (Silo-Server/silo-plugin-sdk#37). Nothing on main uses the new fields yet; this pins the release so changes that do can build. Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 participants
Problem
Related issue: Silo-Server/silo-server#1230
A community developer cannot write an Autoscan source-adapter plugin from this SDK alone. The README has a short "Scan sources" paragraph and the proto is commented, but nothing documents the setup descriptor Silo reads from
metadata.scan_source, how per-source settings and the resolved connection arrive on each poll, the marker rules, what each change scope makes the host do, or what the host does after a poll. There is no example. Writing a test plugin for the Autoscan 1.0 validation required reading Silo's server code.Approach
docs/scan-source.md: a developer guide covering the descriptor keys (onlypolldelivery is honored for plugins;connectionnone/optional/required;config_formfields),source_configandconnectiononPollChangesRequest, markers (empty means start from now; always return a non-emptynext_marker; polls must be repeatable),changesvs legacysource_pathsand the FILE/SUBTREE/AUTO scopes, and the host's rewrites, matching, debounce, queueing and activity log.examples/hello-scan-source: a minimal plugin that reads paths another tool appends to a change-log file, with tests for its marker behavior and descriptor. The README links both, and CONTRIBUTING lists the example build.proto/silo/plugin/v1/scan_source.proto: the comments onsource_paths,changesand theScanSourceChangeScopevalues now say what the host does with each, matching the guide, and the generatedpluginv1code is regenerated (comments only).Merge order: the guide describes the debounce and required-connection behavior from Silo-Server/silo-server#1931 and Silo-Server/silo-server#1934. Merge this after those.
Validation
go build ./...,go vet ./...andgo test ./...pass, including the example's tests.Risks
Documentation, proto comments and an example only; no SDK API or wire change.
Checklist
AI Disclosure
Note
Document the
scan_source.v1plugin contract and add ahello-scan-sourceexamplescan_source.v1capability: setup descriptor, per-source config,PollChangesrequest/response fields, marker lifecycle, path scopes, host-side matching and debouncing, polling cadence, errors, and multi-node deployment requirements.changes_fileconfig.source_pathsas legacy and expands scope-enum comments in scan_source.proto; no generated code or wire format changed.Macroscope summarized 1a5e626.