Close cache and chaos polish followups#2
Open
HardcoreMonk wants to merge 21 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces the P9 production MVP track for kvfs as an internal MinIO/S3-compatible replacement, finalizes governance alignment, and delivers two polish items. The changes span governance docs (ADR-064, project identity), cache bounding for CoordClient (LRU eviction), chaos script identity checking, and comprehensive planning artifacts for future P9 slices. ChangesGovernance Charter & Project Identity
CoordClient Cache & Chaos Script Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmd/kvfs-edge/main.go`:
- Around line 364-379: The code parses EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES (via
strconv.Atoi into lookupCacheMax) even when EDGE_COORD_LOOKUP_CACHE_TTL
(flagLookupCache) is empty; move the strconv.Atoi call and the lookupCacheMax <
0 check inside the TTL-enabled branch so that lookupCacheMax is only parsed and
validated when *flagLookupCache != ""; then keep the existing time.ParseDuration
for flagLookupCache and call coordClient.SetLookupCacheWithLimit(ttl,
lookupCacheMax) as before.
In `@docs/superpowers/plans/2026-05-15-project-design-governance.md`:
- Line 13: The heading "### Task 1: Lifecycle Artifacts" jumps from an H1,
violating markdownlint; change that heading to "## Task 1: Lifecycle Artifacts"
and update the other task headings that follow (e.g., any "### Task 2..." / "###
Task 3...") to the same level so the document uses H1 -> H2 -> H3 progression
consistently, ensuring subsequent subtask headings remain one level deeper if
needed.
🪄 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: b3d79e4b-79a3-4d88-a503-a812fb67b8e5
📒 Files selected for processing (27)
.gitignoreAGENTS.mdREADME.mdcmd/kvfs-edge/main.godocs/ARCHITECTURE.mddocs/FOLLOWUP.mddocs/GUIDE.mddocs/adr/ADR-015-coordinator-daemon-split.mddocs/adr/ADR-032-nfs-gateway-deferred.mddocs/adr/ADR-064-production-mvp-profile.mddocs/adr/README.mddocs/guide.htmldocs/operations/2026-05-15-project-design-governance-handoff.mddocs/operations/2026-05-16-production-mvp-charter-supersede-handoff.mddocs/superpowers/grill-me/2026-05-15-project-design-governance.mddocs/superpowers/grill-me/2026-05-16-production-mvp-replacement.mddocs/superpowers/grill-me/2026-05-16-s3-compatibility-foundation.mddocs/superpowers/plans/2026-05-15-project-design-governance.mddocs/superpowers/plans/2026-05-16-production-mvp-charter-supersede.mddocs/superpowers/plans/2026-05-16-s3-compatibility-foundation.mddocs/superpowers/plans/2026-05-16-small-polish-batch.mddocs/superpowers/specs/2026-05-15-project-design-governance-design.mddocs/superpowers/specs/2026-05-16-production-mvp-replacement-design.mddocs/superpowers/specs/2026-05-16-small-polish-batch-design.mdinternal/edge/coord_client.gointernal/edge/coord_client_test.goscripts/chaos-coord-quorum-loss.sh
Comment on lines
+364
to
379
| lookupCacheMax, perr := strconv.Atoi(*flagLookupCacheMax) | ||
| if perr != nil { | ||
| fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES parse: " + perr.Error()) | ||
| } | ||
| if lookupCacheMax < 0 { | ||
| fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES must be >= 0") | ||
| } | ||
| // P6-10/P6-12 opt-in cache. | ||
| if *flagLookupCache != "" { | ||
| ttl, perr := time.ParseDuration(*flagLookupCache) | ||
| if perr != nil { | ||
| fatal("EDGE_COORD_LOOKUP_CACHE_TTL parse: " + perr.Error()) | ||
| } | ||
| coordClient.SetLookupCache(ttl) | ||
| log.Info("coord lookup cache enabled (P6-10)", "ttl", ttl) | ||
| coordClient.SetLookupCacheWithLimit(ttl, lookupCacheMax) | ||
| log.Info("coord lookup cache enabled (P6-10/P6-12)", "ttl", ttl, "max_entries", lookupCacheMax) | ||
| } |
There was a problem hiding this comment.
Gate max-entry parsing behind TTL-enabled cache config.
EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES is currently parsed/fatal even when lookup cache TTL is unset, despite being documented as TTL-gated. This can fail startup for an unused setting.
💡 Proposed fix
- lookupCacheMax, perr := strconv.Atoi(*flagLookupCacheMax)
- if perr != nil {
- fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES parse: " + perr.Error())
- }
- if lookupCacheMax < 0 {
- fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES must be >= 0")
- }
- // P6-10/P6-12 opt-in cache.
- if *flagLookupCache != "" {
+ // P6-10/P6-12 opt-in cache.
+ if *flagLookupCache != "" {
+ lookupCacheMax, perr := strconv.Atoi(*flagLookupCacheMax)
+ if perr != nil {
+ fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES parse: " + perr.Error())
+ }
+ if lookupCacheMax < 0 {
+ fatal("EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES must be >= 0")
+ }
ttl, perr := time.ParseDuration(*flagLookupCache)
if perr != nil {
fatal("EDGE_COORD_LOOKUP_CACHE_TTL parse: " + perr.Error())
}
coordClient.SetLookupCacheWithLimit(ttl, lookupCacheMax)
log.Info("coord lookup cache enabled (P6-10/P6-12)", "ttl", ttl, "max_entries", lookupCacheMax)
}🤖 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 `@cmd/kvfs-edge/main.go` around lines 364 - 379, The code parses
EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIES (via strconv.Atoi into lookupCacheMax) even
when EDGE_COORD_LOOKUP_CACHE_TTL (flagLookupCache) is empty; move the
strconv.Atoi call and the lookupCacheMax < 0 check inside the TTL-enabled branch
so that lookupCacheMax is only parsed and validated when *flagLookupCache != "";
then keep the existing time.ParseDuration for flagLookupCache and call
coordClient.SetLookupCacheWithLimit(ttl, lookupCacheMax) as before.
|
|
||
| --- | ||
|
|
||
| ### Task 1: Lifecycle Artifacts |
There was a problem hiding this comment.
Fix heading level jump to satisfy markdownlint.
Line 13 starts at ### directly under an # heading, which violates heading increment rules (h1 -> h3). Use ## here (and keep subsequent task headings consistent).
Suggested patch
-### Task 1: Lifecycle Artifacts
+## Task 1: Lifecycle Artifacts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Task 1: Lifecycle Artifacts | |
| ## Task 1: Lifecycle Artifacts |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@docs/superpowers/plans/2026-05-15-project-design-governance.md` at line 13,
The heading "### Task 1: Lifecycle Artifacts" jumps from an H1, violating
markdownlint; change that heading to "## Task 1: Lifecycle Artifacts" and update
the other task headings that follow (e.g., any "### Task 2..." / "### Task
3...") to the same level so the document uses H1 -> H2 -> H3 progression
consistently, ensuring subsequent subtask headings remain one level deeper if
needed.
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
1 participant
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.
Summary
Test Plan
origin/mainis behind localmain, so this PR includes the shared local documentation history leading up to this branch as well as the four small-polish implementation commits.Summary by CodeRabbit
New Features
EDGE_COORD_LOOKUP_CACHE_MAX_ENTRIESenvironment variable to configure lookup cache capacity limits.Documentation