feat(configuration): strip redundant config.yaml blocks once persisted#1907
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughA new ChangesWorker config block strip on engine startup
Sequence DiagramsequenceDiagram
participant Client as Engine Boot
participant Serve as EngineBuilder::serve
participant ConfigRewrite as config_rewrite::apply_strip
participant ConfigStore as Configuration Store
participant FileSystem as config.yaml
participant Watcher as File Watcher
Client->>Serve: start engine
Serve->>ConfigStore: configuration::get(worker_id)
ConfigStore-->>Serve: persisted value (if exists)
alt Value already persisted
Serve->>ConfigRewrite: apply_strip(path, id, location)
ConfigRewrite->>FileSystem: read config.yaml
ConfigRewrite->>ConfigRewrite: strip_worker_config_block(content)
ConfigRewrite->>FileSystem: atomic_write (temp + rename)
ConfigRewrite-->>Serve: true (rewritten)
Serve->>Serve: clear entry.config
else No persisted value
Serve-->>Serve: skip strip
end
Serve->>Watcher: create filesystem watcher
Watcher-->>Client: ready (no self-triggered reload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@engine/tests/config_reload_e2e.rs`:
- Line 321: Replace the fixed sleep calls at lines 321, 415, and 498 in the
config_reload_e2e test with a polling loop that checks for the expected
condition (the rewritten content) within a bounded timeout. Instead of using
tokio::time::sleep(Duration::from_millis(1000)).await, implement a loop that
periodically checks the actual state/content and breaks when the expected
condition is met or the timeout expires, eliminating the nondeterministic timing
dependency.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b5c61c3-40aa-44d9-bdc2-c54497ca453b
📒 Files selected for processing (4)
engine/src/lib.rsengine/src/workers/config.rsengine/src/workers/config_rewrite.rsengine/tests/config_reload_e2e.rs
Documents the unreleased configuration work landing after 0.19.6: redundant worker config: blocks are stripped from config.yaml once the value is persisted (#1907), plus the value-only persisted store files (#1901). Claude-Session: https://claude.ai/code/session_012BHDssDNdbRrcgvCwH4cYU
After a worker's value is persisted in the configuration store, its `config:` block in config.yaml is dead weight: the store is the runtime source of truth and the block can silently drift. On boot, before the file watcher starts, serve() now strips the block (keeping the `- name:` entry) for every worker entry whose value is already in the store, and replaces it with a breadcrumb comment pointing at the value's location (e.g. ./data/configuration/<id>.yaml for the fs adapter). The strip is store-driven (queries configuration::get), so it covers built-in workers and external workers like `shell` uniformly with no per-worker hooks. Built-in workers seed during the boot loop, so their blocks strip on first boot; an external worker registers over the bus after boot, so its block strips on the first boot after it persisted. Entries with no persisted value (e.g. a worker that hasn't run yet) are left untouched. The strip runs before the watcher exists, so self-writes never trigger a reload; the matching in-memory entry is nulled only on a confirmed write so a later reload diff sees no phantom change.
…ge e2e cargo fmt --check failed on config_rewrite.rs (the `let Some(..) =` binding needed wrapping). Separately, the Engine Coverage job timed out in observability_configuration_e2e::alert_rules_hot_swap_and_prune: the shared wait_for() poll capped at 5s, which is borderline under llvm-cov instrumentation. Widen the failure-case deadline to 20s (happy path is unaffected — it returns as soon as the predicate holds). Claude-Session: https://claude.ai/code/session_012BHDssDNdbRrcgvCwH4cYU
CodeRabbit flagged the three fixed 1s sleeps in config_reload_e2e before reading the rewritten config.yaml: they race on slow CI runners (the same flake class that just timed out the coverage job). Replace them with wait_for_config_rewrite(), which polls the file for the seed-strip breadcrumb under a bounded 10s deadline. The breadcrumb assertion is now the poll's break condition, so the redundant follow-up asserts are dropped. The happy path returns as soon as the rewrite lands. Claude-Session: https://claude.ai/code/session_012BHDssDNdbRrcgvCwH4cYU
Documents the unreleased configuration work landing after 0.19.6: redundant worker config: blocks are stripped from config.yaml once the value is persisted (#1907), plus the value-only persisted store files (#1901). Claude-Session: https://claude.ai/code/session_012BHDssDNdbRrcgvCwH4cYU
0964fb3 to
df781a9
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
What
After a worker's value is persisted in the configuration store, its
config:block inconfig.yamlis dead weight — the store is the runtime source of truth (the block is seed-only) and a stale block silently drifts from reality.On boot, before the file watcher starts,
serve()now strips that block (keeping the- name:entry) for every worker entry whose value is already in the store, replacing it with a breadcrumb comment pointing at where the value lives:The location is resolved from the
configurationworker's own fs-adapterdirectory(default./data/configuration); a non-fs adapter (e.g.bridge) falls back to "the configuration worker store".Why store-driven
The strip queries
configuration::getrather than relying on each worker to report that it seeded. That makes it uniform across:shell,coder) — they register over the SDK bus under their config.yaml name and persist a value; their block is stripped on the first boot after they persisted.Entries with no persisted value yet (e.g. a worker that hasn't run) are left untouched. No per-worker hooks needed — the entire feature is one new module plus one block in
serve().Safety
serde_yamlround-trip): comments, formatting, and${VAR}placeholders in other blocks are byte-preserved. Idempotent — a second pass finds no block and no-ops.Tradeoff (by design)
External workers connect after boot, so their block is stripped one boot late (never on the boot they first register). Same-boot stripping would need file-watcher self-write suppression (reload-storm risk); this keeps the simpler model that matches the existing seed-only semantics.
Test plan
engine/src/workers/config_rewrite.rsunit tests (8): strip/keep-entry, env-placeholder preservation, nestedconfig:, idempotency, absent entry/block, after-image:field, first-of-duplicates.engine/tests/config_reload_e2e.rs(3 new, boot throughserve()):cargo checkclean.Summary by CodeRabbit
configseed blocks after the worker’s value has been persisted, replacing removed sections with a breadcrumb pointing to the persisted config source to reduce unnecessary reload triggers.config.yamlbehavior details.