Skip to content

fix(msfs): correct virt child-dir-entry-map SIGHUP immutability check#110

Open
joshuarobinson wants to merge 1 commit into
mainfrom
fix/mr907-sighup-virt-evict-low-limit
Open

fix(msfs): correct virt child-dir-entry-map SIGHUP immutability check#110
joshuarobinson wants to merge 1 commit into
mainfrom
fix/mr907-sighup-virt-evict-low-limit

Conversation

@joshuarobinson

@joshuarobinson joshuarobinson commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

The SIGHUP config-immutability guard for
virt_child_dir_entry_map_page_evict_low_limit compared the phys field (globals.config.physChildDirEntryMapPageEvictLowLimit) instead of the virt field, so changing the virt low-limit via SIGHUP was silently accepted and the phys low-limit was checked twice.

Also fix the adjacent phys_child_dir_entry_map_page_evict_low_limit guard, whose error message was mistakenly copied from the inode_eviction_queue block.

Found in merged MR !907.

Description

Change description.

{Relates to/Closes} {Task ID}.

Checklist

  • Development PR
    • .release_notes/.unreleased.md
      • Notable changes to the client (i.e. not related to tooling, CI/CD, etc.) from this PR have been added.
  • Release PR
    • CI/CD
      • The default branch pipelines are passing in both GitHub + GitLab (latter for SwiftStack E2E tests).
    • multi-storage-client/pyproject.toml
      • The package version has been bumped.
    • .release_notes/.unreleased.md
      • This file's contents have been moved into a .release_notes/{bumped package version}.md file.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected configuration reload validation so changes to child directory entry eviction limits are accurately detected.
    • Updated validation messages to identify the correct configuration setting when a mismatch occurs.

The SIGHUP config-immutability guard for
virt_child_dir_entry_map_page_evict_low_limit compared the *phys*
field (globals.config.physChildDirEntryMapPageEvictLowLimit) instead of
the virt field, so changing the virt low-limit via SIGHUP was silently
accepted and the phys low-limit was checked twice.

Also fix the adjacent phys_child_dir_entry_map_page_evict_low_limit
guard, whose error message was mistakenly copied from the
inode_eviction_queue block.

Found in merged MR !907.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@joshuarobinson
joshuarobinson requested a review from a team July 14, 2026 14:16
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Corrects two SIGHUP configuration validation issues: one reported setting name and one comparison that used physical instead of virtual configuration fields.

Changes

SIGHUP validation

Layer / File(s) Summary
Correct reload mismatch checks
multi-storage-file-system/config.go
Corrects the physical setting name in the mismatch error and compares the virtual eviction limit against its corresponding configuration value.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Suggested reviewers: edmc-ss

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the fix, but it leaves the template placeholders in place and omits a real task/close reference. Replace the placeholders with a filled Description section and a Relates to/Closes task ID; optionally note any release-note impact.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix to the virt child-dir-entry-map SIGHUP immutability check and matches the code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mr907-sighup-virt-evict-low-limit

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
multi-storage-file-system/config.go (1)

2135-2159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for the independent physical and virtual checks.

Test changing only each setting, asserting rejection and the correct setting name in the error. This protects against future copy/paste regressions between adjacent physical and virtual fields.

As per coding guidelines, run go test ./... and just analyze before submitting changes.

🤖 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 `@multi-storage-file-system/config.go` around lines 2135 - 2159, Add regression
tests for the SIGHUP validation covering each physical and virtual setting
independently, including physChildDirEntryMapPageEvictLowLimit,
physChildDirEntryMapPageEvictHighLimit,
physChildDirEntryMapPageDirtyFlushTrigger, physChildDirEntryMapFlushedPerGC, and
virtChildDirEntryMapKeysPerPageMax. For each case, change only that field,
assert rejection, and verify the error names the corresponding setting
correctly; run go test ./... and just analyze.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@multi-storage-file-system/config.go`:
- Around line 2135-2159: Add regression tests for the SIGHUP validation covering
each physical and virtual setting independently, including
physChildDirEntryMapPageEvictLowLimit, physChildDirEntryMapPageEvictHighLimit,
physChildDirEntryMapPageDirtyFlushTrigger, physChildDirEntryMapFlushedPerGC, and
virtChildDirEntryMapKeysPerPageMax. For each case, change only that field,
assert rejection, and verify the error names the corresponding setting
correctly; run go test ./... and just analyze.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9bee474a-bdc4-47c7-8e11-7d0a807c5994

📥 Commits

Reviewing files that changed from the base of the PR and between d72a5de and 992ef1f.

📒 Files selected for processing (1)
  • multi-storage-file-system/config.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant