-
Notifications
You must be signed in to change notification settings - Fork 403
Fix preview crash on re-render (BadResource error) #13967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
…t each render in preview mode the project context is owned by `preview()` and cleaned up when preview exits. Calling `cleanup()` here would close shared resources (like SassCache KV handles) causing BadResource errors on subsequent re-renders (#13955).
… ensure changes are picked up in preview mode
… cross-platform consistency ensureFileInformationCache now normalizes file paths before using them as cache keys. This ensures cache invalidation works correctly on Windows where paths may have backslash separators. Co-Authored-By: Claude Opus 4.5 <[email protected]>
de69c0b to
b796ec4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This normalization is related to
Windows / Linux and Project vs Single file leads to different paths. I tried to make that works because we need to delete by key, so it is important to get the right path, but obviously this modify current behavior, and leads to failed test.
|
😞 I tried to preview quarto-web with this PR, and I still encounter some issue with sass Kv, we may have an other issue. |
|
Ok so I don't manage to reproduce, maybe it was a bad state. However, as expected, change .scss file for example does trigger re-render, but the change are not shown. Though I am really not even sure this working with current 1.8 as discussed. However, testing And I am quite convinced this is because of this normalize path key. So not good to merge yet
|
and add a debug log message when we do so.
|
🤦 It is late for me and I spent much time of this, now this should really be non-normalized path for lookup. I'll test tomorrow manually the behavior |
When re-rendering a file during
quarto previewin a project, the preview crashes withBadResource: Bad resource IDfrom the Sass cache Deno.Kv operations.Root Cause
This is a regression from #13804 which changed how
ProjectContextis managed during preview. That PR made the project context persist across re-renders (instead of being recreated each time), butrenderForPreview()still calledrenderResult.context.cleanup()after each render.The cleanup closes shared resources like the SassCache Deno.Kv handle. On subsequent re-renders, the code returns the cached
SassCacheinstance whose KV handle was already closed, causing theBadResourceerror when callingkv.get().Fix
Remove the
cleanup()call fromrenderForPreview()- the project context is owned bypreview()and should only be cleaned up when preview exits viaonCleanup()Invalidate the
fileInformationCacheentry for the rendered file before each render - sincecleanup()previously cleared all caches (includingfileInformationCache), removing the call means stale file content would be reused. The cache entry for the specific file being rendered is now deleted so changes are picked up.Fixes #13955