diff --git a/docs/superpowers/specs/2026-08-06-upload-review-remediation-design.md b/docs/superpowers/specs/2026-08-06-upload-review-remediation-design.md new file mode 100644 index 000000000..5d3b44fbf --- /dev/null +++ b/docs/superpowers/specs/2026-08-06-upload-review-remediation-design.md @@ -0,0 +1,176 @@ +# Upload Review Remediation Design + +**Issue:** #3750 +**PR:** #4704 +**Status:** Approved in conversation; awaiting written-spec confirmation +**Scope:** Findings from the independent review of the collision-safe upload implementation + +## Problem + +The first implementation prevents two writers from replacing the same destination, but +the filename is still the only identity carried after publication. A delete can therefore +remove a primary while its conversion is running, and a later upload can reuse the same +name before the old conversion publishes. The old conversion then becomes associated with +the new primary. + +The review also found six boundary defects: conversion lifecycle I/O can block the event +loop; AIO sandboxes do not mount the generated-conversion namespace; user filenames can +claim the internal staging pattern; staging-link cleanup can report success after a partial +publication; maximum-length primary names overflow the conversion filename component; and +WeChat does not translate every invalid platform filename into its normal attachment +failure path. + +## Goals + +- Keep one upload generation stable from atomic publication through conversion, chmod, + sandbox synchronization, response construction, or rollback. +- Coordinate publication and deletion across threads and processes without a database. +- Ensure stale work cannot operate on a later upload that reused the same pathname. +- Keep locks scoped by actual filename so unrelated uploads remain concurrent. +- Make the generated-conversion namespace readable but not writable from mounted sandboxes. +- Keep every synchronous filesystem operation off async event loops. +- Reject internal names and handle cleanup and filename-length boundaries explicitly. + +## Non-goals + +- Adding a database table or durable upload manifest. +- Serializing every upload in a thread behind one directory-wide lock. +- Changing the public primary-upload listing schema. +- Making conversion failure fail an otherwise successful primary upload. + +## Name Lease and Publication Identity + +The upload manager introduces two internal values: + +- `UploadIdentity`: the published file's device and inode captured immediately after the + no-replace link succeeds. +- `PublishedUpload`: the actual `Path`, its `UploadIdentity`, and an exclusive name lease. + +Name leases use advisory file locks under the system-owned namespace: + +```text +user-data/.upload-conversions/.locks/.lock +``` + +Lock files are stable and are not deleted during normal operation; retaining the same lock +inode avoids split-brain locking between processes. A bounded set of in-process striped +locks complements the OS file lock so threads and processes use the same exclusion rule. +The lock filename is a digest, so platform filenames cannot escape the lock namespace or +exceed a component limit. + +### Leased publication + +For each candidate name, the publisher performs these operations: + +1. Validate the user basename before creating a staging file. +2. Acquire the candidate's name lease. +3. Attempt the existing atomic hard-link no-replace publication. +4. On `EEXIST`, release that candidate lease and try the next suffix. +5. On success, capture the final inode identity and remove the staging link. +6. Return `PublishedUpload` while retaining the final candidate lease. + +If staging unlink fails after link creation, the publisher removes only the candidate whose +identity it just created and returns a publication failure. It never reports a half-cleaned +publication as successful. Cleanup closes handles with `try/finally` so an exceptional +close cannot suppress staging unlink. + +Compatibility helpers that only need an immediately stable path release the lease before +returning the `Path`. Ingress adapters with post-publication work use the leased form. + +### Lifecycle ownership + +- Gateway holds each lease through conversion, permission adjustment, remote sandbox sync, + response assembly, or identity-safe rollback. +- Embedded client holds the lease through conversion and metadata construction. +- Feishu and DingTalk hold it through remote sandbox synchronization. +- Generic IM ingestion and WeChat download staging have no later pathname-dependent side + effect, so they release immediately after publication. +- Deletion acquires the same actual-name lease before inspecting or unlinking the primary + and its generated conversion. + +Rollback receives `PublishedUpload`, verifies that the current path still has the captured +identity, and removes only that generation. It cannot unlink a later file that reused the +name. + +## Conversion and Delete Coordination + +The conversion wrapper accepts the active `PublishedUpload`. When no publication is +supplied for a standalone internal caller, it acquires the actual-name lease and treats the +currently locked regular file as the generation to convert. + +Conversion runs while the name lease is held. Deletion of that exact primary waits for the +conversion lifecycle to finish, then removes both the primary and its exact generated +asset. Publication of other filenames remains concurrent. This prevents all three unsafe +orders: + +- delete during conversion; +- delete followed by same-name re-upload before old conversion publication; +- rollback, chmod, or sandbox sync operating on a later same-name generation. + +Conversion remains non-fatal to primary upload success. A conversion exception cleans its +own stage, emits no Markdown metadata, and leaves the primary intact. + +## Async Filesystem Boundary + +`convert_uploaded_file_to_markdown()` offloads all synchronous lifecycle work: conversion +directory creation and validation, lease acquisition/release, staging creation/close, +identity checks, publication, and cleanup. The underlying converter also offloads file +stat, parsing, and Markdown writes regardless of input size. A strict Blockbuster test +executes the real wrapper with only the document parser substituted. + +## Sandbox Visibility + +Both Local and AIO providers add an explicit mapping for: + +```text +/.upload-conversions -> /mnt/user-data/.upload-conversions +``` + +The mapping is read-only. AIO creates and validates the source before building the mount. +The nested mapping also overrides Local's writable aggregate `/mnt/user-data` mapping, so +only host conversion code can mutate generated files and lock state. + +## Reserved Names and Filename Lengths + +Basenames matching `.upload-*.part` are reserved and rejected before staging. This keeps +primary listings and startup cleanup unambiguous. + +Normal conversion targets remain `.md`. If that component would +exceed 255 UTF-8 bytes, the layout helper uses: + +```text +..md +``` + +The digest makes the shortened mapping deterministic and collision-resistant; all response, +outline, deletion, and virtual-path helpers use the same function rather than appending +`.md` independently. + +## WeChat Failure Translation + +WeChat validates platform filenames with the shared normalizer before staging. Invalid or +reserved names fall back to its generated safe media name. Any remaining validation error +from the shared publisher is caught and becomes the adapter's existing `None` result, so a +hostile attachment does not abort the whole polling update. + +## Tests + +Tests are added before implementation and must demonstrate: + +1. A paused old conversion cannot publish after delete and same-name re-upload. +2. Delete waits for the leased conversion and removes its exact generated asset. +3. Identity-safe rollback cannot remove a later same-name generation. +4. The real async conversion wrapper passes the strict blocking-I/O gate. +5. Local and AIO mount specifications expose `.upload-conversions` read-only. +6. `.upload-*.part` is rejected before any staging file is created. +7. Injected staging unlink and handle-close failures never report successful publication + and leave no unintended final entry. +8. Convertible primary names at the 252-255 byte boundary produce valid deterministic + conversion paths. +9. WeChat overlong, backslash, and reserved filenames use a safe fallback or normal failure + result without escaping the inbound handler. +10. Existing concurrency, symlink, Gateway, embedded-client, IM, conversion, outline, + deletion, and sandbox-sync suites remain green. + +Final verification is the focused upload/sandbox/channel suite, the full backend test +suite, Ruff lint and format checks, a static ingress audit, and a fresh independent review.