Skip to content

Add circuit breaker for hydration status#1913

Merged
tyrielv merged 2 commits intomicrosoft:masterfrom
tyrielv:tyrielv/hydration-status-2-circuit-breaker
Mar 18, 2026
Merged

Add circuit breaker for hydration status#1913
tyrielv merged 2 commits intomicrosoft:masterfrom
tyrielv:tyrielv/hydration-status-2-circuit-breaker

Conversation

@tyrielv
Copy link
Contributor

@tyrielv tyrielv commented Mar 17, 2026

Auto-disable hydration status computation after 3 failures per UTC day. Resets on new day or GVFS version change. Checked in hooks (pre-command) and GitStatusCache (background) but NOT in manual gvfs health.

  • New HydrationStatusCircuitBreaker class using System.IO directly for file-link compatibility with GVFS.Hooks
  • Marker file at .gvfs/gitStatusCache/HydrationStatusDisabled.dat
  • 12 unit tests covering trip, reset, parse, and edge cases

Auto-disable hydration status computation after 3 failures per UTC day.
Resets on new day or GVFS version change. Checked in hooks (pre-command)
and GitStatusCache (background) but NOT in manual gvfs health.

- New HydrationStatusCircuitBreaker class using System.IO directly
  for file-link compatibility with GVFS.Hooks
- Marker file at .gvfs/gitStatusCache/HydrationStatusDisabled.dat
- 12 unit tests covering trip, reset, parse, and edge cases

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tyrielv tyrielv force-pushed the tyrielv/hydration-status-2-circuit-breaker branch from b16b123 to ebac74c Compare March 17, 2026 15:45
Copy link

@KeithIsSleeping KeithIsSleeping left a comment

Choose a reason for hiding this comment

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

see below

@KeithIsSleeping
Copy link

(Corrected re-post — the original review had rendering issues with backtick escaping)

Well-designed safety valve. The circuit breaker pattern is a good fit here, and the implementation is solid. A couple of notes:

  1. Minor TOCTOU near midnightIsDisabled() reads the date with DateTime.UtcNow and RecordFailure() reads it again independently. If called right at midnight, IsDisabled could see "2026-03-17" and RecordFailure could see "2026-03-18", effectively writing a reset. Extremely unlikely to matter in practice, but passing the timestamp as a parameter would make it both consistent and more testable.

  2. Hooks path uses NullTracer.Instance — Circuit breaker warnings in the pre-command hook path are silently discarded. Is that intentional? The hooks are lightweight so I understand the tradeoff, but it means you can't tell from logs that the circuit breaker fired during a git status hook invocation.

  3. Design choice feedback — Good call avoiding PhysicalFileSystem dependency for hooks file-link compatibility. The exclusive file locking with graceful skip-if-locked fallback is clean. Version-based reset is clever — a GVFS update that fixes the underlying bug automatically re-enables the feature.

Test coverage is thorough (12 tests covering trip, reset, parse edge cases, version change, day rollover). Looks good.

@tyrielv tyrielv enabled auto-merge March 18, 2026 20:00
@tyrielv tyrielv merged commit 39d3282 into microsoft:master Mar 18, 2026
49 checks passed
@tyrielv tyrielv deleted the tyrielv/hydration-status-2-circuit-breaker branch March 18, 2026 21:10
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.

3 participants