Skip to content

Fix Content window recovery after filesystem watcher overflow - #4807

Open
mhijaziB wants to merge 2 commits into
mainfrom
fix/4642
Open

Fix Content window recovery after filesystem watcher overflow#4807
mhijaziB wants to merge 2 commits into
mainfrom
fix/4642

Conversation

@mhijaziB

Copy link
Copy Markdown
Contributor

Fixes #4642

Brief Description

Large local content operations can overflow the filesystem watcher's internal buffer. When this happened, some file notifications were lost and the Unity Content window displayed an incomplete deleted count until the user manually refreshed it.

This change:

  • Converts watcher overflow errors into a coalesced full-rescan request for the affected PID and manifest.
  • Rebuilds the local content state from disk instead of applying an incomplete incremental update.
  • Emits a full-rebuild event so Unity replaces its cached content state, including when the rebuilt manifest is empty.
  • Handles files disappearing while a rescan is running.
  • Prevents duplicate content IDs when local files are deleted during reconciliation.
  • Adds focused automated coverage for the overflow and full-rescan recovery path.

Verification

  • ContentWatcherRecoveryTests: 8/8 passed.
  • CLI build completed with 0 errors.
  • Manually tested deleting 2,000+ local content files in Unity.
  • The Content window reached the correct deleted count after an overflow without requiring a manual refresh.

Follow-up

Overflow recovery currently takes approximately 7–10 seconds in the large-content reproduction. That performance and observability work is intentionally outside the scope of this fix and is tracked separately in #4806 — Bulk local content changes can take 7–10 seconds to appear in the Unity Content window.

Checklist

  • Have you added appropriate text to the CHANGELOG.md files?

Add those to list or remove the list below altogether:

Notes

When you are merging a feature branch into main, please squash merge and make sure the final commit contains any relevent JIRA ticket number. If you are merging from main to staging, or staging to production, please use a regular merge commit.

Does this introduce tech-debt? If so, have you added an entry to the Tech-debt document?

@github-actions

Copy link
Copy Markdown
Contributor

Lightbeam link

@allister-beamable allister-beamable left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, generally I think the changes are in a good direction, with one sort of architectural concern: it feels like we are adding moving parts to an already-complex system in hopes of making it more robust.

In the design tension of simplicity vs. featureful functionality, I think we are already on the "make it shiny and reactive" side of the bridge, so we may have to reserve true simplification for the moderate to far future.

All this to say: ship it! (but look for opportunities to simplify if possible)

Comment on lines +319 to +320
var entries = ContentService.ContentFileToLocalContentManifestEntries(contentFiles)
.ToArray();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this the part that is slow when there are lots and lots of content entries?

Comment on lines +196 to +215
public static void HandleWatcherError(Exception exception, string pid, string manifestId, ILogger logger,
ChannelWriter<ChangedContentFile> changedContentFiles)
{
if (TryCreateWatcherRecoveryChange(exception, pid, manifestId, out var recoveryChange))
{
logger.LogWarning(
"Content filesystem notifications were lost. Scheduling a full local content rescan. " +
"PID={Pid}, ManifestId={ManifestId}",
pid,
manifestId);
changedContentFiles.TryWrite(recoveryChange);
return;
}

logger.LogError(
exception,
"Content filesystem watcher failed. PID={Pid}, ManifestId={ManifestId}",
pid,
manifestId);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something about this watcher error handler makes my brain itch, like it is a bridge too far in terms of moving parts. I see that we already have a filesystem watcher as part of the mechanism, but something bothers me about that.

Or, alternatively, could we make it clearer to the user that something is busted in a way that refreshing will fix, and give them a big prominent refresh button to fix it?

Third-alternatively, would it make sense to just have a heuristic where if you delete a bunch of junk the system triggers the full refresh when the volume of change exceeds a certain threshold (say 500 entries or 1000 entries)?

If these worries of mine spark thoughts for improvements, purpose of review achieved. If, on the other hand, you think this way is still the best way, go with it.

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.

Beam Content Window UI not updating properly

2 participants