Repository navigation
deck: deck.media review follow-ups (#515) - #517
Merged
Merged
Conversation
#515's review found one minor issue and four nits. - A link at a clip's own name, pointing outside storage, blocked that clip for good: the path went through crate::inside(), which follows a final link and refused before the stale-entry repair ran, and eviction, clear, forget and prune remove only regular files. The path is now the checked plain .cache/media folder joined with the service-built name (a hex SHA-256 and an extension from EXTS), so the repair removes whatever is there, a link itself and never what it points to, and writes the clip anew. - Eviction takes clips no open document uses first, and an open document's clips only when that is not enough, each the one used longest ago first; deck.media reads the set before the call takes the documents table. - Only what can fit is staged: after the render cache and then the media cache are cleared, a clip still bigger than the room left is storage_full: before any staging, and writing.rs::place (save, export) clears the render cache and refuses in the commit's own words before it stages. Both stage through crate::stage, counted in tests. - The 32 MiB cap is checked before the clip is hashed. New tests: a link at a clip's name (to a file outside storage, and to one holding the clip's very bytes) goes, the clip is written as a plain file and the file outside is untouched; eviction takes a newer clip no document uses before an open document's oldest. The storage_full tests check that nothing is staged. The deck paragraph in apps/AGENTS.md follows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"Writes are staged and never replace a file the call didn't create" now says that a stale entry at a clip's own name (a file that doesn't hold the clip's bytes, or a link, never followed) is removed and the clip written there anew, as #515's review asked. The as-built "oldest first" bullet adds that clips no open document uses go before any that one does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Local CI passed on b81b102 ( GitHub would run for this PR: desktop.yml, phone.yml, apps.yml. Merged on this local pass (macOS runner queue); GitHub CI runs on the merge commit on main.
Skipped (these did NOT pass; (!) marks a skip that blocks tools/ci-local-merge.sh):
Notes:
|
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.
Why
#515 (
deck.media, merged as083f89f0) came back from review clean on safety, with one minor issue and four nits. This PR fixes all five.What
src/media.rs).crate::inside(), which follows a final link. A link at.cache/media/<hash>.<ext>pointing to a file outside storage was refused asinvalid:before the stale-entry repair could run. Eviction, clear, forget and prune only remove regular files, so nothing ever removed the link.EXTS. The folder is checked to be plain (.cacheand.cache/media, links refused, as before). Nothing at that name is resolved.media()reads the set (docs::clips_in_use, nowpub(crate)) before the call takes the documents table, since the table is borrowed during the call.deck.media: after the render cache and then the media cache are cleared, a clip still bigger than the room left isstorage_full:(the contract's words) before any staging folder is made.writing.rs::place(save and export): the render-cache clear and the room check now come before staging. What still doesn't fit is refused with the commit's own words (storage_full: the results are N bytes, more than the R bytes left in this storage), and nothing is staged.crate::stage, which counts staging folders in tests (a#[cfg(test)]counter, likeENGINE_READS).Docs follow the eviction change. SERVICE.md's "Oldest first" as-built bullet adds "Clips no open document uses go before any that one does". The deck paragraph in
apps/AGENTS.mddescribes the eviction order and the refusal before staging. The module doc insrc/media.rscovers all five changes.Tests
On the Linux build host (x86_64), at head
ef2c8f79on main083f89f0:cargo test --locked -p octosense-deck-service -p octosense-engine-area:first_callandskill: 1 passed each;cargo clippy --locked -p octosense-deck-service -p octosense-engine-area --all-targets --no-deps: no findings in either crate.crate::inside()) put back, the new link test fails withinvalid: deck.media: `path` stays inside this app's storage, which is the reported bug.New and changed, in
src/doc_tests.rs:a_link_at_a_clips_name_goes_and_is_never_followed(Unix), the test for the minor issue. A link at the clip's own name points to a file outside the storage. In a second case, the outside file holds the clip's exact bytes. In both cases:deck.mediaanswers that name;Before this fix,
deck.mediaansweredinvalid:here.eviction_takes_clips_no_open_document_uses_first: seven clips handed to an open document, then a stray clip no document uses, written last, so the newest.the_render_cache_then_the_media_cache_are_cleared_before_storage_fullnow also checks the staging counter:the_cache_is_cleared_before_storage_full(export, viaplace) now also checks that the refused export isstorage_full:in the commit's own words, with nothing staged.Not run: no shell, desktop or Quick Deck UI.
🤖 Generated with Claude Code