fix: hot-load file changes on every sync in long-running services - #47
Open
matthewscobell wants to merge 11 commits into
Open
fix: hot-load file changes on every sync in long-running services#47matthewscobell wants to merge 11 commits into
matthewscobell wants to merge 11 commits into
Conversation
The parsed-file cache was built exactly once at process startup inside ResourceSyncers(), so long-lived services served stale data until restart. Validate() — which the SDK runs at the start of every sync — now rebuilds the cache from the current file contents and publishes it atomically to a holder shared by all resource builders, restoring the hot-load behavior that regressed in #40. - Generation-stamped page tokens: a listing resumed across a cache swap restarts instead of silently skipping/duplicating rows; legacy bare numeric tokens are still honored so in-flight syncs survive upgrades - Nil-cache guards in List/Entitlements/Grants replace potential panics - An invalid file edit now fails the sync loudly while the last successfully loaded data keeps serving - nolint:staticcheck on the deprecated trait options: they populate both trait- and resource-level fields, so migrating to the WithResource* replacements would drop trait-level fields from sync output - Regression tests enforce the hot-load contract; README documents it Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if err != nil { | ||
| return nil, fmt.Errorf("baton-file: failed to read input file: %w", err) | ||
| } | ||
| sum := sha256.Sum256(raw) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: loadValidatedCache unconditionally re-reads (twice — os.ReadFile then client.LoadFileData), re-validates, and rebuilds the full index set on every Validate(), including the health-check probes the cacheHolder comment calls out. Since sum is already computed before parsing, comparing it against the currently published cache's gen and returning that cache unchanged would skip the parse, index rebuild, and the ~2x-memory swap whenever the file has not actually changed.
| var syncers []connectorbuilder.ResourceSyncerV2 | ||
| for _, rt := range cache.resourceTypes { | ||
| syncers = append(syncers, &resourceBuilder{cache: cache, resourceType: rt}) | ||
| syncers = append(syncers, &resourceBuilder{cache: &fc.cache, resourceType: rt}) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: resourceType is captured from the first cache and never refreshed, while the data cache is. If an edit changes an existing type's trait (e.g. team from group to role), the reloaded cache builds those resources with the new trait (buildResource switches on the new cache's resource type) while ResourceType() keeps reporting the old trait to the SDK — a trait mismatch that persists until restart. The README caveat only covers a brand-new resource type; worth widening it to any resource-type/trait change, or resolving resourceType from the live cache by id here.
Contributor
Connector PR Review: fix: hot-load file changes on every sync in long-running servicesBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff (including Security IssuesNone found. Correctness IssuesNone found. The generation-stamped token grammar round-trips ( Suggestions
Prompt for AI agents |
Hot-load covers the file's data sections only; the set of resource types and their traits is schema, fixed for the process lifetime because the SDK registers syncers by type exactly once. Document that intent at the builder-registration site and widen the README/contract wording from "a new resource type" to any resource-type or trait change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add rationale comments at every site where this connector deviates from standard patterns, for reviewers and future maintainers: the nil holder on capabilities-only builders, the deprecated trait options kept to preserve trait-level sync output, the raw-bytes fingerprint and its benign double-read, the Debug/Warn log-level choices, and why hot-load tests use real files instead of in-memory data. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cuit - Legacy bare-numeric page tokens now restart the listing when the cache is generation-stamped: their generation is unknowable and they only appear after a restart, exactly when the file may have changed. Generation-less caches (built directly in tests) still honor their own numeric mints — restarting those would loop forever. - loadValidatedCache short-circuits when the content fingerprint matches the published cache generation: skips the redundant parse and rebuild on health-check probes and keeps no-op Validate calls from swapping the pointer under an in-flight sync. - Document the cross-listing consistency limit in the cacheHolder contract: generation stamps make each listing consistent, not a whole sync; the short-circuit confines swaps to actual content changes. - Add the hot-load note to the customer-facing docs/connector.mdx. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Serialize the read -> build -> publish sequence behind a mutex (extracted as FileConnector.refresh): the atomic pointer prevented data races but not lost updates — two concurrent Validate calls racing across two file edits could publish out of order. A CAS loop cannot fix this because generations are unordered content hashes; serializing makes publish order follow read order and prevents concurrent full cache builds. - Correct the ResourceSyncers comment, which had the lifecycle backwards: the SDK calls it at construction BEFORE any Validate, so the construction-time load is the first load, not a fallback. Document the honest consequence: a file that is invalid at startup registers zero resource types, and recovery requires fixing the file AND restarting — hot-load only helps a connector that started successfully. Also documented in README and connector.mdx. - Rebuild the hot-load tests on the real SDK entry point (connectorbuilder.NewConnector) and server RPCs so the SDK dictates the lifecycle order instead of the tests assuming it; the previous helper called Validate before ResourceSyncers — the reverse of production — leaving the actual startup path untested. Add TestHotReload_InvalidFileAtStartupRequiresRestart pinning the startup-failure behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The in-code contract was corrected to reflect that ResourceSyncers() performs the first file load at construction; the README paragraph mirroring it still said "do not move the file load to construction time", contradicting the code it points to. Reword to the actual invariant: the construction-time load is the first load, never the only one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A file with schema but no data rows previously registered zero resource types, so every sync failed with FailedPrecondition for the process lifetime — the same dead-end as an invalid file. Per product intent an empty file is a valid state: ResourceSyncers now registers the standard trait types (the same set StaticCapabilitiesConnector declares as the connector's capabilities), so syncs succeed, emit nothing, and data rows added later hot-load without a restart. Rows using custom type IDs remain a schema change requiring restart; Validate() now warns when a refresh publishes types that were not registered at startup — previously that drift was completely silent. The invalid-file-at-startup trap is unchanged and still pinned by its test; new tests pin empty-start sync + hot-load and the custom-type restart contract. README and connector.mdx updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The schema-drift check now runs on EVERY Validate(), not just the one where the file's content hash changed: the condition persists until restart, so an edge-triggered warning meant one missable log line for a permanent problem. Logarithmic sampling (occurrences 1, 2, 4, 8, ... with total_occurrences) keeps probed deployments from flooding; the counter resets when drift resolves so a new episode warns immediately. TestHotReload_SchemaDriftCheckIsLevelTriggered pins the level-triggered contract. - paginate() now logs a Warn when a generation mismatch (or a legacy pre-upgrade token) actually restarts a listing, making mid-sync cache swaps and the documented cross-phase consistency window observable in logs instead of inferable-only. The probe/sync-start distinction the alternative guard would need does not exist connector-side (same RPC, no sync-lifecycle hook) — recorded on the cacheHolder contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Page tokens now carry a restart count ("<gen>:<offset>[:<restarts>]"):
a file rewritten faster than a listing can finish previously
restarted that listing forever with no forward progress; after
maxListingRestarts (3) the listing fails with a clear error naming
the churning file, turning unbounded work into a retryable sync
failure. The count is stateless — it rides the token — so it is
scoped to one listing chain and needs no shared state.
- Assert the actual log emissions, not just internal state: a zap
observer (go.uber.org/zap/zaptest/observer, vendored; same module
version, go.mod untouched) now pins the drift Warn's logarithmic
sampling (fires at occurrences 1, 2 and 4, not 3, with
total_occurrences) and the two paginate restart Warns with their
generation fields.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The restart budget now resets when a listing completes a page under the current generation: the churn episode is over, so the count no longer accumulates for the lifetime of a listing chain, where a checkpointed token could have turned one later file change into a hard failure. The bound is therefore consecutive-restarts-without- progress; docs updated to say exactly that, and README/connector.mdx now document the bounded-restart failure mode operators can hit. - Close a read-then-parse race in loadValidatedCache: the fingerprint was hashed from one read while the parser did its own second read, so an edit landing between them mislabeled the build — and a later revert to the hashed content would short-circuit onto the mislabeled cache indefinitely. The file is now re-hashed after parsing and the load retried (bounded) when the fingerprints disagree, so gen always describes the bytes the parser consumed. Adversarially reviewed (token state machine enumerated, concurrency stress-tested under -race through the real SDK server) and e2e-verified: all 12 example inputs produce output identical to main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- A rewrite landing during the parse can tear the read and make a valid file transiently unparseable — at startup that needlessly forced a process restart. On parse failure the file is now re-hashed: changed (or momentarily unreadable) bytes mean a racing write and the load retries; unchanged bytes mean the file is genuinely invalid and the error is returned as before. - Record the restart-bound liveness adjudication on maxListingRestarts: the bound is deliberately consecutive (progress resets it); decaying instead of resetting fails the same alternating-churn pattern it purports to fix, and a cumulative count reintroduces the checkpoint hard-failure. Documented so it is not "fixed" without solving that tension. - Docs precision: the restart bound applies to consecutive restarts with no page under an unchanged file in between, and the new "kept changing while being loaded" failure is now documented in README and connector.mdx. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Re the restart-budget decay suggestion: adjudicated as won't-fix. The alternating-churn pattern is real, but the proposed decay does not terminate under it either — mismatch +1 / progress −1 oscillates below any bound forever; any decay rate ≥ the churn rate loops. Guaranteed termination requires a cumulative budget, which reintroduces the checkpointed-token hard failure fixed earlier on this PR. We deliberately chose liveness: the exploiting rhythm is implausible against microsecond in-memory pages, and task-level timeouts bound a hung sync externally. The trade-off is recorded on the maxListingRestarts comment. The torn-read retry and docs-precision suggestions from the same review are addressed in the latest commit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What kind of change is this?
Problem
A customer running baton-file as a long-lived Windows service reported that file edits were not reflected in their tenant until the service was restarted. The connector parsed the input file exactly once — inside
ResourceSyncers(), which the SDK invokes a single time at connector construction — and every subsequent sync served resources/entitlements/grants from that frozen in-memory cache. Ironically,Validate()re-read and re-validated the file on every sync, then discarded the result into a field nothing consumed.This regressed in #40 (the connector redesign). Before #40,
List/Entitlements/Grantsre-read the file on every call, so hot-load worked by construction. One-shot CLI runs masked the regression because the process exits after each sync.Fix
Validate()— the only per-sync hook the SDK gives this connector — now rebuilds the sync cache from the file's current contents and publishes it atomically (cacheHolderwrappingatomic.Pointer[syncCache]) to the resource builders. Data-section edits are picked up on the next sync; schema changes (a brand-new resource type) still require a restart by design, since the SDK registers resource types once at startup.<file-content-fingerprint>:<offset>): if a listing's token was minted against a different cache generation (mid-sync health-probe revalidation, or a sync resumed after a restart with a changed file), the listing restarts instead of replaying offsets into changed slices — which would silently skip or duplicate rows. Restarting is safe because c1z writes are idempotent upserts. Legacy bare-numeric tokens are still honored so in-flight syncs survive a binary upgrade.List/Entitlements/Grantsreturn an error where a nil cache would previously have panicked.no resource builders found).Validate()wrote the plainvalidatedDatafield from concurrently served RPCs (e.g. health checks) with no synchronization.Not a breaking change
review-breaking-changes.md, behavior-preserving changes and performance work are explicitly non-breaking. Full e2e verification: all 12 example inputs (everytest/integration/testdata/fixture and every template — YAML/JSONC/CSV/XLSX, full + quickstart) synced with a main-built and branch-built binary produce semantically identical resources/entitlements/grants dumps and identical capabilities JSON. The external-grants (shared identity source) annotations are byte-identical.baton_capabilities.jsonandconfig_schema.jsonare unchanged; no flag/config surface changes.Lint (pre-existing failure on main)
verify/lintis currently red on main: 7staticcheck SA1019hits from the recent SDK bump deprecating trait-level options. This PR silences them with explained//nolint:staticcheckcomments instead of migrating, deliberately: the deprecated options populate both the trait-level and resource-level fields, while theWithResource*replacements populate only the resource level — a mechanical migration would drop trait-level fields from sync output. That migration needs its own coordinated change.Intentional design notes for reviewers
cacheHolderis deliberate shared instance state — a documented deviation from the stateless-connector guideline, which assumes a remote API. Here the file is the source of truth and is re-read every sync, so a cold start is identical to a warm one. See the contract comment oncacheHolderinpkg/connector/connector.go.ResourceSyncers()logging load failures at Warn and returning nil syncers is intentional: the method signature cannot return an error, andValidate()surfaces the real error to the SDK on every sync.TestHotReload_*inpkg/connector/hot_reload_test.goenforce the hot-load contract; if a refactor makes them fail, the refactor reintroduces this bug.Validation
go test ./...andgo test -race ./pkg/connector/...green🤖 Generated with Claude Code