From e75981a873eb2da7c58a017b9d3426fcab63087b Mon Sep 17 00:00:00 2001 From: hetaoBackend Date: Thu, 6 Aug 2026 20:55:36 +0800 Subject: [PATCH] docs: clarify upload sandbox boundaries --- README.md | 4 +- backend/AGENTS.md | 12 ++--- backend/docs/API.md | 4 +- backend/docs/FILE_UPLOAD.md | 6 +-- backend/docs/PATH_EXAMPLES.md | 2 +- .../2026-08-06-upload-review-remediation.md | 46 +++++++++++++------ ...26-08-06-upload-collision-safety-design.md | 7 +-- ...-08-06-upload-review-remediation-design.md | 45 +++++++++++++----- 8 files changed, 84 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index be16b6a2a..ca77553eb 100644 --- a/README.md +++ b/README.md @@ -963,7 +963,7 @@ Each task gets its own execution environment with a full filesystem view — ski Uploads from the Web UI, embedded client, and IM channels share one collision-safe storage rule. A completed payload is published only if its candidate name does not exist; concurrent `report.pdf` uploads become `report.pdf`, `report_1.pdf`, `report_2.pdf`, and so on without replacing one another. The selected name is leased through conversion and sandbox synchronization, so deleting that exact name waits for its active upload lifecycle while unrelated filenames continue concurrently. Internal staging names matching `.upload-*.part` are rejected. -Optional document conversions are system-owned assets under `/mnt/user-data/.upload-conversions/`. Normal targets use `.md`; names that would exceed the filesystem component limit use a deterministic UTF-8-safe prefix plus the full SHA-256 digest. The exact generated path is returned through the upload response and omitted from the primary upload listing. Local and AIO sandboxes expose this namespace read-only. Deleting a primary removes only its exact generated asset and never infers that a user-uploaded sibling such as `uploads/report.md` is disposable. +Optional document conversions are system-owned assets under `/mnt/user-data/.upload-conversions/`. Normal targets use `.md`; names that would exceed the filesystem component limit use a deterministic UTF-8-safe prefix plus the full SHA-256 digest. The exact generated path is returned through the upload response and omitted from the primary upload listing. Mounted AIO sandboxes expose this namespace through a read-only mount, and Local structured file APIs enforce the same rule through path mappings. Local host bash is outside that mapping boundary and must remain disabled for untrusted tasks. Non-mounted remote providers receive a private synchronized copy that may be writable but cannot mutate the authoritative host conversion or lock state. Deleting a primary removes only its exact generated asset and never infers that a user-uploaded sibling such as `uploads/report.md` is disposable. The built-in `grep` tool searches either one text file or all matching text files below a directory, so an agent can search an uploaded document directly without first broadening the request to the entire uploads directory. @@ -995,7 +995,7 @@ This is the difference between a chatbot with tool access and an agent with an a # Paths inside the sandbox container /mnt/user-data/ ├── uploads/ ← your primary files -├── .upload-conversions/ ← generated Markdown (read-only; hidden from upload listings) +├── .upload-conversions/ ← system-owned Markdown (hidden from upload listings) ├── workspace/ ← agents' working directory └── outputs/ ← final deliverables ``` diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 9e2e2f3db..ae3158d24 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -699,8 +699,8 @@ that cannot tell sibling branches apart. **Provider Pattern**: `SandboxProvider` with `acquire`, `acquire_async`, `get`, `release` lifecycle. Async agent/tool paths call async sandbox lifecycle hooks so Docker sandbox creation, discovery, cross-process locking, readiness polling, and release stay off the event loop. **Environment policy** (`sandbox/env_policy.py`): `execute_command` no longer inherits the full `os.environ`. `build_sandbox_env()` scrubs secret-looking names (`*KEY*`/`*SECRET*`/`*TOKEN*`/`*PASS*`/`*CREDENTIAL*`) from the inherited environment before layering injected request secrets on top, so platform credentials (e.g. `OPENAI_API_KEY`) never leak into skill subprocesses. Benign vars (`PATH`, `HOME`, `LANG`, `VIRTUAL_ENV`, ...) are preserved. **Implementations**: -- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` API honours the `/mnt/user-data` contract uniformly with AIO. The more-specific `/mnt/user-data/.upload-conversions` mapping is read-only even though the aggregate `/mnt/user-data` mapping is writable. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Public, custom, legacy, and managed integration skill mappings point at stable enabled-only projection roots rather than raw skill directories. -- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths — it never touches the ownership store (that would be blocking IO on the event loop); ownership is published on acquire/reclaim and refreshed off the event loop by the dedicated renewal thread (`_renew_owned_leases`). `uses_thread_data_mounts` defaults to backend detection (`LocalContainerBackend=True`, remote/provisioner backends=False), while the optional `sandbox.thread_data_mounts` boolean takes precedence for deployments that guarantee the Gateway and sandbox share the same thread user-data directories. Setting it `true` skips upload-time sandbox acquire/sync; a false positive leaves uploads unavailable to the sandbox. Thread mounts create and expose `.upload-conversions` explicitly as read-only, separate from the writable uploads mount. Local-container and hostPath-provisioner mounts use the same stable skill projection roots; PVC-backed skills remain governed by the operator-supplied PVC layout until PVC materialization is implemented. Readiness probes and `agent_sandbox` clients classify loopback/private IPs, single-label cluster hosts, and Docker/Podman internal hostnames as direct control-plane destinations and set `trust_env=False`; external FQDNs and public IPs retain environment proxy support. +- `LocalSandboxProvider` - Local filesystem execution. `acquire(thread_id)` returns a per-thread `LocalSandbox` (id `local:{thread_id}`) whose `path_mappings` resolve `/mnt/user-data/{workspace,uploads,outputs}` and `/mnt/acp-workspace` to that thread's host directories, so the public `Sandbox` file APIs honour the `/mnt/user-data` contract uniformly with AIO. The more-specific `/mnt/user-data/.upload-conversions` mapping rejects writes through those structured file APIs even though the aggregate `/mnt/user-data` mapping is writable. This is not an OS isolation boundary: explicitly enabled Local host bash operates on host paths outside `PathMapping` write enforcement and must remain disabled for untrusted tasks. `acquire()` / `acquire(None)` keeps the legacy generic singleton (id `local`) for callers without a thread context. Per-thread sandboxes are held in an LRU cache (default 256 entries) guarded by a `threading.Lock`. Public, custom, legacy, and managed integration skill mappings point at stable enabled-only projection roots rather than raw skill directories. +- `AioSandboxProvider` (`packages/harness/deerflow/community/`) - Docker-based isolation. Active-cache and warm-pool entries are checked with the backend during acquire/reuse; definitively dead containers are dropped from all in-process maps so the thread can discover or create a fresh sandbox instead of reusing a stale client. Backend health-check failures are treated as unknown, not dead; local discovery likewise treats an unverifiable container as not adoptable and falls through to create rather than failing acquire. `get()` remains an in-memory lookup for event-loop-safe tool paths — it never touches the ownership store (that would be blocking IO on the event loop); ownership is published on acquire/reclaim and refreshed off the event loop by the dedicated renewal thread (`_renew_owned_leases`). `uses_thread_data_mounts` defaults to backend detection (`LocalContainerBackend=True`, remote/provisioner backends=False), while the optional `sandbox.thread_data_mounts` boolean takes precedence for deployments that guarantee the Gateway and sandbox share the same thread user-data directories. Setting it `true` skips upload-time sandbox acquire/sync; a false positive leaves uploads unavailable to the sandbox. Thread mounts create and expose `.upload-conversions` explicitly as read-only, separate from the writable uploads mount. Non-mounted providers instead receive only the requested generated file as a private synchronized copy; that copy may be writable but does not expose or mutate authoritative host files or lock state. Local-container and hostPath-provisioner mounts use the same stable skill projection roots; PVC-backed skills remain governed by the operator-supplied PVC layout until PVC materialization is implemented. Readiness probes and `agent_sandbox` clients classify loopback/private IPs, single-label cluster hosts, and Docker/Podman internal hostnames as direct control-plane destinations and set `trust_env=False`; external FQDNs and public IPs retain environment proxy support. - `E2BSandboxProvider` (`packages/harness/deerflow/community/e2b_sandbox/`) provides E2B remote isolation. New sandboxes receive a one-shot upload from the enabled-only public, custom, legacy, and managed integration projections. Existing E2B VMs keep their @@ -765,9 +765,9 @@ that cannot tell sibling branches apart. **Shared warm-pool lifecycle:** community sandbox providers that keep released sandboxes alive for fast reuse share `deerflow.community.warm_pool_lifecycle.WarmPoolLifecycleMixin`. The mixin owns the common `DEFAULT_IDLE_TIMEOUT=600`, `IDLE_CHECK_INTERVAL=60`, `DEFAULT_REPLICAS=3`, idle-checker loop, warm-pool expiry, oldest-warm eviction, replica counting, and soft-cap logging. Providers remain responsible for their own active registries, creation/discovery, health checks, and destroy hook (`_destroy_warm_entry`): AIO destroys `SandboxInfo` through its backend; Boxlite closes loop-affine `BoxliteBox` handles; Tenki closes the microVM session (`TenkiSandbox.close`, which terminates the remote sandbox). AIO keeps active-idle cleanup outside the mixin and delegates only warm-pool expiry to the shared helper. **Virtual Path System**: -- Agent sees: `/mnt/user-data/{workspace,uploads,outputs}`, read-only `/mnt/user-data/.upload-conversions`, and `/mnt/skills` +- Agent sees: `/mnt/user-data/{workspace,uploads,outputs}`, system-owned `/mnt/user-data/.upload-conversions`, and `/mnt/skills` - Physical: `backend/.deer-flow/users/{user_id}/threads/{thread_id}/user-data/...`; raw skills stay under `deer-flow/skills/` and managed integration storage, while sandboxes read `backend/.deer-flow/skills_view/public/` and `backend/.deer-flow/users/{user_id}/skills_view/{custom,legacy,integrations}/` -- Translation: `LocalSandboxProvider` builds per-thread `PathMapping`s for the user-data prefixes at acquire time; `tools.py` keeps `replace_virtual_path()` / `replace_virtual_paths_in_command()` as a defense-in-depth layer (and for path validation). AIO has the directories volume-mounted at the same virtual paths inside its container, so both implementations accept `/mnt/user-data/...` natively. Both providers mount `.upload-conversions` read-only; only host upload-conversion code may mutate generated files or the stable `.locks/.lock` coordination files. +- Translation: `LocalSandboxProvider` builds per-thread `PathMapping`s for the user-data prefixes at acquire time; its structured file APIs reject writes through the more-specific conversion mapping, while Local host bash is deliberately outside that enforcement boundary. Mounted AIO exposes the conversion directory as a read-only nested mount. Non-mounted providers receive individual synchronized copies and never receive the host `.locks` directory. `tools.py` keeps `replace_virtual_path()` / `replace_virtual_paths_in_command()` as a defense-in-depth layer (and for path validation). - Detection: `is_local_sandbox()` accepts both `sandbox_id == "local"` (legacy / no-thread) and `sandbox_id.startswith("local:")` (per-thread) **Sandbox Tools** (in `packages/harness/deerflow/sandbox/tools.py`): @@ -1461,9 +1461,9 @@ Multi-file upload with automatic document conversion: - Reuses one conversion worker per request when called from an active event loop - Files stored in thread-isolated directories under the resolving user's bucket (`users/{user_id}/threads/{thread_id}/user-data/uploads`). For IM channels the owner is threaded explicitly via the `user_id=` kwarg (see IM Channels → Owner-scoped file storage); HTTP/embedded callers resolve it from `get_effective_user_id()` - Every ingress stages a complete payload and atomically publishes it without replacing an existing entry. Collisions across requests, processes, HTTP, embedded, and IM adapters use `name.ext`, `name_1.ext`, `name_2.ext`; storage that cannot provide atomic no-replace publication fails explicitly. -- Gateway HTTP uploads use same-directory `.upload-*.part` staging files. Staging files are hidden from upload listings, agent upload context, and sandbox listing/search tools, and swept on Gateway startup if a hard crash leaves one behind. +- Gateway HTTP uploads use same-directory `.upload-*.part` staging files. Each active stage holds a cross-process liveness lock under `.upload-conversions/.locks/stages/`; startup cleanup skips held stages and sweeps only crash-orphaned files. Staging files are hidden from upload listings, agent upload context, and sandbox listing/search tools. - Generated Markdown is owned by `user-data/.upload-conversions/.md` and is omitted from primary upload listings. Deletion removes only the selected primary and that exact generated asset; it never guesses or deletes a legacy/user-owned `uploads/.md` sibling. -- Gateway HTTP upload/list/delete handlers offload filesystem work through `deerflow.utils.file_io.run_file_io`, a dedicated ContextVar-preserving file IO executor. Non-mounted sandbox uploads acquire sandboxes with `SandboxProvider.acquire_async()` and offload `read_bytes()` plus `sandbox.update_file()` together. +- Gateway HTTP upload/list/delete handlers offload filesystem work through `deerflow.utils.file_io.run_file_io`, a dedicated ContextVar-preserving file IO executor; potentially blocking name-lease acquisition uses a separate pool so waiters cannot starve release. Non-mounted sandbox uploads acquire sandboxes with `SandboxProvider.acquire_async()` and offload `read_bytes()` plus `sandbox.update_file()` together. The route records each completed remote path and, on later failure or cancellation, calls the provider-neutral `Sandbox.remove_file()` for those exact paths before host rollback and lease release. - Mounted upload paths skip both sandbox acquisition and per-file synchronization. For AIO remote/provisioner deployments this requires an explicit, accurate `sandbox.thread_data_mounts: true`; omission preserves backend auto-detection. - Agent receives uploaded file list via `UploadsMiddleware` diff --git a/backend/docs/API.md b/backend/docs/API.md index 000486173..34013b6b6 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -636,9 +636,9 @@ Content-Type: multipart/form-data - Excel (`.xls`, `.xlsx`) - Word (`.doc`, `.docx`) -All upload entry points publish complete payloads without replacing an existing name. Concurrent collisions are returned as `document.pdf`, `document_1.pdf`, `document_2.pdf`, and so on. A published filename remains leased through conversion, permission adjustment, sandbox synchronization, and response construction; deletion of that exact filename waits for the active lifecycle, while other filenames remain independent. Basenames matching the internal `.upload-*.part` staging pattern are rejected. +All upload entry points publish complete payloads without replacing an existing name. Concurrent collisions are returned as `document.pdf`, `document_1.pdf`, `document_2.pdf`, and so on. A published filename remains leased through conversion, permission adjustment, sandbox synchronization, and response construction; deletion of that exact filename waits for the active lifecycle, while other filenames remain independent. If a non-mounted sandbox update later fails or the request is cancelled, DeerFlow removes only the remote paths already completed by that request before rolling back its host generations. Basenames matching the internal `.upload-*.part` staging pattern are rejected. -Generated Markdown is stored outside the primary namespace and is not returned by the list endpoint. Normal conversion names are `.md`; if that component would exceed 255 UTF-8 bytes, the response contains a deterministic UTF-8-safe prefix plus the full SHA-256 digest and `.md`. Clients must consume the returned `markdown_*` fields rather than derive the path. Local and AIO sandboxes mount `.upload-conversions` read-only. Deleting `document.pdf` also deletes only its exact generated conversion; an independent `uploads/document.md` is preserved. +Generated Markdown is stored outside the primary namespace and is not returned by the list endpoint. Normal conversion names are `.md`; if that component would exceed 255 UTF-8 bytes, the response contains a deterministic UTF-8-safe prefix plus the full SHA-256 digest and `.md`. Clients must consume the returned `markdown_*` fields rather than derive the path. Mounted AIO sandboxes use a read-only conversion mount, while Local structured file APIs reject writes through a read-only path mapping; Local host bash is outside that boundary. Non-mounted providers receive a private synchronized copy rather than the authoritative host namespace. Deleting `document.pdf` also deletes only its exact generated conversion; an independent `uploads/document.md` is preserved. #### List Uploaded Files diff --git a/backend/docs/FILE_UPLOAD.md b/backend/docs/FILE_UPLOAD.md index 777986cb7..23880d54c 100644 --- a/backend/docs/FILE_UPLOAD.md +++ b/backend/docs/FILE_UPLOAD.md @@ -130,7 +130,7 @@ Deletion: 删除 report.pdf 时只删除 .upload-conversions/report.pdf.md； /mnt/user-data/uploads/report.md 永远不会被推断为生成文件或自动删除。 ``` -通常生成名为 `<实际主文件名>.md`。如果这一文件名组件会超过 255 个 UTF-8 字节，系统会使用 UTF-8 安全截断的主文件名前缀、完整 SHA-256 摘要和 `.md`，并在响应中返回精确的 `markdown_*` 路径。客户端和 Agent 不应自行拼接生成路径。Local 与 AIO 沙箱都将 `.upload-conversions` 显式挂载为只读；只有 DeerFlow 宿主进程中的转换代码可以写入生成文件和内部锁。 +通常生成名为 `<实际主文件名>.md`。如果这一文件名组件会超过 255 个 UTF-8 字节，系统会使用 UTF-8 安全截断的主文件名前缀、完整 SHA-256 摘要和 `.md`，并在响应中返回精确的 `markdown_*` 路径。客户端和 Agent 不应自行拼接生成路径。AIO 挂载模式把 `.upload-conversions` 显式挂载为只读；Local 的结构化文件 API 通过只读路径映射拒绝写入，但可选的 Local 宿主机 bash 不受该映射约束，不应对不受信任任务启用。非挂载远端沙箱得到的是独立同步副本，该副本可能可写，但不会修改宿主机上的权威生成文件或内部锁。 默认情况下，自动转换是关闭的，以避免在网关主机上对不受信任的 Office/PDF 上传执行解析。只有在受信任部署中明确接受此风险时，才应将 `uploads.auto_convert_documents` 设置为 `true`。 @@ -181,8 +181,8 @@ read_file(path="/mnt/user-data/.upload-conversions/document.pdf.md") 上传流程采用“线程目录优先”策略： - 先写入 `backend/.deer-flow/threads/{thread_id}/user-data/uploads/` 作为权威存储 - 本地沙箱（`sandbox_id=local`）直接使用线程目录内容 -- Local 与 AIO 的挂载模式会把 `/mnt/user-data/.upload-conversions` 单独映射为只读，即使 `/mnt/user-data` 或主上传目录可写 -- 默认情况下，非本地沙箱通过 `acquire_async` 获取后，再额外同步到 `/mnt/user-data/uploads/*`，确保运行时可见 +- AIO 挂载模式把 `/mnt/user-data/.upload-conversions` 单独挂载为只读；Local 的结构化文件 API 通过更具体的只读路径映射执行同一规则，但 Local 宿主机 bash 不属于该边界 +- 默认情况下，非本地沙箱通过 `acquire_async` 获取后，再额外同步到 `/mnt/user-data/uploads/*`，确保运行时可见；同步副本是沙箱私有副本，失败时只回滚本次已完成同步的精确路径 - 如果 Gateway 与远端沙箱保证挂载同一份线程 user-data（例如正确对齐的共享 PVC、NFS 或 hostPath），可设置 `sandbox.thread_data_mounts: true`；上传路由会跳过 sandbox acquire 和逐文件同步 - 不确定挂载关系时应省略该配置并保留自动检测。错误地设为 `true` 会导致文件只存在于 Gateway 存储、沙箱内不可见 diff --git a/backend/docs/PATH_EXAMPLES.md b/backend/docs/PATH_EXAMPLES.md index 012faffb1..9386d5848 100644 --- a/backend/docs/PATH_EXAMPLES.md +++ b/backend/docs/PATH_EXAMPLES.md @@ -282,7 +282,7 @@ function FileUploadList({ threadId }: { threadId: string }) { 4. **Markdown 转换** - 转换成功时，会返回额外的 `markdown_*` 字段 - 常规生成文件位于 `.upload-conversions/<完整主文件名>.md`；超长名称使用 UTF-8 安全前缀和完整 SHA-256 摘要，因此始终以上传响应中的 `markdown_*` 字段为准 - - `.upload-conversions` 在 Local 与 AIO 沙箱内只读，并且不会出现在主文件列表中 + - AIO 挂载模式以只读挂载暴露 `.upload-conversions`；Local 结构化文件 API 通过只读映射拒绝写入，但 Local 宿主机 bash 不受该映射约束；该目录不会出现在主文件列表中 - 同名主文件按 `file.pdf`、`file_1.pdf`、`file_2.pdf` 原子发布，不会覆盖 - 删除主文件会等待该实际文件名的活跃生命周期，然后只删除其精确生成资产，不会删除用户上传的 `uploads/file.md` - `.upload-*.part` 是内部暂存名称，不能作为用户上传 basename diff --git a/docs/superpowers/plans/2026-08-06-upload-review-remediation.md b/docs/superpowers/plans/2026-08-06-upload-review-remediation.md index 4582bc333..eba6cb965 100644 --- a/docs/superpowers/plans/2026-08-06-upload-review-remediation.md +++ b/docs/superpowers/plans/2026-08-06-upload-review-remediation.md @@ -4,7 +4,7 @@ **Goal:** Resolve every actionable finding from the independent review of PR #4704 and reach a fresh zero-finding review before marking the PR ready. -**Architecture:** Add a cross-process per-filename lease that is acquired before atomic publication and retained through every pathname-dependent side effect. Conversion and deletion share the lease; `PublishedUpload` carries an inode identity for safe rollback. Generated conversions gain deterministic long-name mapping and explicit read-only Local/AIO mounts, while all async conversion filesystem work is offloaded. +**Architecture:** Add a cross-process per-filename lease that is acquired before atomic publication and retained through every pathname-dependent side effect. Conversion and deletion share the lease; `PublishedUpload` carries an inode identity for safe rollback. Generated conversions gain deterministic long-name mapping and explicit mounted-AIO/Local-file-API read-only boundaries, while all async conversion filesystem work is offloaded and cancellation drains active workers. **Tech Stack:** Python 3.12, asyncio, POSIX `fcntl` / Windows `msvcrt`, FastAPI, pytest, pytest-asyncio, Blockbuster, Ruff. @@ -13,10 +13,10 @@ - Keep PR #4704 in Draft until a fresh independent review reports zero actionable findings. - Use test-driven development: every production behavior change must have a test observed failing for the intended reason before implementation. - Locks are per actual filename, work across threads and processes, and never require a database or manifest. -- Stable lock files live under `user-data/.upload-conversions/.locks/` and are not deleted during normal operation. +- Stable name-lock files live under `user-data/.upload-conversions/.locks/` and are not deleted during normal operation; transient stage-liveness locks live below `.locks/stages/`. - Unrelated filenames remain concurrent; only deletion of the exact name waits for its active lifecycle. - Conversion failure remains non-fatal to a successfully published primary upload. -- Local and AIO sandboxes see `.upload-conversions` through a read-only mapping. +- Mounted AIO uses a read-only conversion mount; Local structured file APIs use a read-only mapping, while Local host bash remains outside that enforcement boundary. - All filesystem calls made from async conversion and Gateway code run off the event loop. - Normal conversion names remain `.md`; overlong components use deterministic UTF-8-safe truncation plus a full SHA-256 digest. - Preserve public response/list/delete compatibility except where an internal staging-pattern filename is now rejected as unsafe. @@ -124,7 +124,7 @@ def test_abort_unlinks_stage_when_close_raises(tmp_path): Add a multiprocessing regression with a top-level child-process helper: the parent holds the lease for `report.pdf`, the child attempts `delete_file_safe()`, and a queue/event proves the child cannot finish before release. This is the cross-process acceptance gate; -the thread test separately protects the in-process striped-lock behavior. +the thread test separately protects the in-process exact-name keyed-lock behavior. - [ ] **Step 2: Run the tests and verify the intended failures** @@ -162,7 +162,10 @@ def ensure_upload_lock_dir(uploads_dir: Path) -> Path: return lock_dir ``` -Create `lease.py` using the repository's existing `fcntl`/`msvcrt` pattern. Use 64 pre-created `threading.Lock` stripes chosen from the first digest byte; do not retain an unbounded dictionary of filenames: +Create `lease.py` using the repository's existing `fcntl`/`msvcrt` pattern. Use exact-name +in-process locks keyed by upload-directory identity and filename. Reference-count holders +and waiters, and remove the dictionary entry when its count reaches zero so unrelated names +never alias and historical names are not retained: ```python @dataclass(frozen=True, slots=True) @@ -190,10 +193,10 @@ class UploadNameLease: def acquire(cls, uploads_dir: Path, filename: str) -> "UploadNameLease": digest = hashlib.sha256(filename.encode("utf-8")).hexdigest() lock_path = ensure_upload_lock_dir(uploads_dir) / f"{digest}.lock" - # acquire stripe, open stable lock file, then acquire fcntl/msvcrt lock + # acquire the exact-name thread lock, then the stable fcntl/msvcrt lock def release(self) -> None: - # unlock and close the file before releasing the stripe; idempotent + # unlock and close the file before releasing the keyed thread lock; idempotent ``` Keep the lock file in place after release. @@ -549,7 +552,7 @@ git commit -m "fix: retain upload leases through adapter sync" --- -### Task 5: Read-only conversion mounts in Local and AIO sandboxes +### Task 5: Read-only conversion boundaries for mounted AIO and Local file APIs **Files:** - Modify: `backend/packages/harness/deerflow/sandbox/local/local_sandbox_provider.py` @@ -559,7 +562,8 @@ git commit -m "fix: retain upload leases through adapter sync" **Interfaces:** - Consumes: `UPLOAD_CONVERSIONS_DIRNAME`, `ensure_conversion_dir`, and existing `join_host_path`. -- Produces: explicit read-only `/mnt/user-data/.upload-conversions` mappings. +- Produces: an explicit read-only AIO mount and a Local structured-file mapping for + `/mnt/user-data/.upload-conversions`; Local host bash is documented as outside the mapping. - [ ] **Step 1: Write failing mount-contract tests** @@ -590,8 +594,8 @@ Expected: neither provider exposes the explicit mapping. - [ ] **Step 3: Add the Local and AIO mappings** Both builders call `ensure_conversion_dir(paths.sandbox_uploads_dir(...))` before returning. -Local adds a longer, read-only `PathMapping` beneath the aggregate writable user-data map. -AIO adds: +Local adds a longer, read-only `PathMapping` beneath the aggregate writable user-data map +for structured file operations. AIO adds: ```python ( @@ -635,8 +639,24 @@ git commit -m "fix: mount upload conversions read-only" Set the remediation spec status to `Implemented; awaiting independent review`. Explain that delete waits only for an active lifecycle of the exact filename. Document that generated -conversions are read-only inside mounted sandboxes and that overlong conversion components -use `..md`. State that `.upload-*.part` is reserved. +conversions are read-only through mounted AIO and Local structured file interfaces, while +Local host bash and non-mounted private copies have narrower guarantees. Overlong conversion +components use `..md`. State that `.upload-*.part` is reserved. + +### Independent review round 2 amendments + +- Replace hash-striped thread locks with reference-counted exact-name locks; unrelated + filenames must never deadlock merely because their digests share a stripe. +- Run blocking lease acquisition on a dedicated executor so same-name waiters cannot starve + lease release in the general file-I/O pool. Windows acquisition retries until success. +- Give each active stage a cross-process liveness lock and make startup cleanup skip any + stage whose lock is held. +- Drain conversion and publication workers before cancellation cleans a stage or releases a + generation lease. +- Track successful non-mounted sandbox updates and remove those exact remote copies before + host rollback on later failure or cancellation. +- Describe read-only behavior at the actual enforcement boundary: mounted AIO and Local + structured file APIs, not Local host bash or a non-mounted private copy. - [ ] **Step 2: Run the complete focused suite** diff --git a/docs/superpowers/specs/2026-08-06-upload-collision-safety-design.md b/docs/superpowers/specs/2026-08-06-upload-collision-safety-design.md index e24e69398..7f3968a5a 100644 --- a/docs/superpowers/specs/2026-08-06-upload-collision-safety-design.md +++ b/docs/superpowers/specs/2026-08-06-upload-collision-safety-design.md @@ -176,9 +176,10 @@ parallel messages cannot overwrite one another before the thread upload copy occ ### Sandbox synchronization The existing primary upload sync remains unchanged. Generated conversion sync uses its -exact virtual path under `/mnt/user-data/.upload-conversions/`. Local and AIO providers -mount that namespace explicitly read-only; non-mounted providers sync the exact file -explicitly. +exact virtual path under `/mnt/user-data/.upload-conversions/`. Mounted AIO providers use +a read-only nested mount, while Local structured file APIs enforce a read-only path +mapping; Local host bash is outside that boundary. Non-mounted providers sync an isolated +copy of the exact file and never receive authoritative host lock state. ## Error Handling 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 index c0bcffba6..71031c5a9 100644 --- a/docs/superpowers/specs/2026-08-06-upload-review-remediation-design.md +++ b/docs/superpowers/specs/2026-08-06-upload-review-remediation-design.md @@ -27,7 +27,8 @@ failure path. - 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. +- Reject generated-conversion writes through mounted sandbox file interfaces and document + the separate Local host-bash and non-mounted-copy boundaries. - Keep every synchronous filesystem operation off async event loops. - Reject internal names and handle cleanup and filename-length boundaries explicitly. @@ -52,11 +53,12 @@ Name leases use advisory file locks under the system-owned namespace: 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. +Name-lock files are stable and are not deleted during normal operation; retaining the same +lock inode avoids split-brain locking between processes. In-process locks are keyed by the +upload directory identity and exact filename, reference-counted, and removed as soon as no +holder or waiter remains. This preserves unrelated-name concurrency without retaining an +unbounded historical filename registry. The lock filename is a digest, so platform +filenames cannot escape the lock namespace or exceed a component limit. ### Leased publication @@ -77,6 +79,11 @@ 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. +Every hidden `.upload-*.part` file also owns a short-lived cross-process liveness lock. +Startup cleanup takes that lock non-blocking and skips a stage held by any live worker, so +staggered Gateway startup cannot remove another process's active upload or conversion. +Orphaned stages remain recoverable after a hard crash because the OS releases the lock. + ### Lifecycle ownership - Gateway holds each lease through conversion, permission adjustment, remote sandbox sync, @@ -92,6 +99,10 @@ Rollback receives `PublishedUpload`, verifies that the current path still has th identity, and removes only that generation. It cannot unlink a later file that reused the name. +For non-mounted sandboxes, Gateway records each virtual path only after its update +completes. If a later file fails or the request is cancelled, it removes exactly those +completed remote copies before rolling back host generations and releasing their leases. + ## Conversion and Delete Coordination The conversion wrapper accepts the active `PublishedUpload`. When no publication is @@ -116,19 +127,25 @@ own stage, emits no Markdown metadata, and leaves the primary intact. 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. +executes the real wrapper with only the document parser substituted. Cancellation drains +any already-running worker before stage cleanup or lease release, so a background thread +cannot write through a deleted stage or outlive its generation lease. ## Sandbox Visibility -Both Local and AIO providers add an explicit mapping for: +Mounted AIO and per-thread Local 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. +For mounted AIO, this is a read-only nested OS mount. For Local, the more-specific read-only +`PathMapping` rejects writes through structured sandbox file APIs even though the aggregate +`/mnt/user-data` mapping is writable. Local host bash executes directly on host paths and +is outside `PathMapping` enforcement; operators must keep it disabled for untrusted tasks. +Non-mounted providers receive only private synchronized file copies. Those copies may be +writable inside the sandbox, but they cannot mutate authoritative host generations or the +host-only `.locks` namespace. ## Reserved Names and Filename Lengths @@ -161,7 +178,8 @@ Tests are added before implementation and must demonstrate: 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. +5. Mounted AIO and Local structured file mappings reject conversion writes, with the Local + host-bash and non-mounted-copy limitations documented explicitly. 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. @@ -171,6 +189,9 @@ Tests are added before implementation and must demonstrate: result without escaping the inbound handler. 10. Existing concurrency, symlink, Gateway, embedded-client, IM, conversion, outline, deletion, and sandbox-sync suites remain green. +11. Active stages survive cleanup from a second process, cancellation waits for conversion + workers, unrelated legacy-stripe names do not block, and partial remote synchronization + removes only completed copies. 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.