fix(agent): remove filetransfer map entries when HandleTransfer returns (#2388) by ToddHebebrand · Pull Request #2391 · LanternOps/breeze

ToddHebebrand · 2026-07-12T22:04:58Z

Summary

filetransfer.Manager.transfers added an entry per transfer and never removed one — no delete( call existed anywhere in the package. The Manager is owned by the process-lifetime Heartbeat singleton, so the map grew unbounded for the life of the agent (#2388).

Fix

Simpler than the reaper proposed in the issue: nothing ever reads a transfer back after it reaches terminal state — the only map reader is CancelTransfer, which needs the entry only while the transfer is in flight (the upload/download loops poll transfer.Status through the shared pointer). So HandleTransfer now defer-deletes its map entry on return, covering success, failure, and cancelled paths. A late cancel arriving after removal hits the existing not-found no-op branch in CancelTransfer (covered by TestCancelTransferNonexistentIsNoop). No timestamp field or seenCommands-style reaper needed.

Also fixed a latent data race in the same file: HandleTransfer/upload/download wrote transfer.Status/transfer.Progress/transfer.Error without holding m.mu, while CancelTransfer and the in-flight cancellation checks used the mutex. Writes are now guarded, and reportProgress snapshots the mutable fields under RLock.

Tests

New assertNoTransfers helper asserts len(m.transfers) == 0 after HandleTransfer returns in upload-success, download-success, upload-failure, download-failure, and TestConcurrentHandleTransfer.
TestCancelTransferSetsStatus (mid-flight cancel semantics — entry exists while in flight) still passes unchanged.
cd agent && go test -race ./internal/filetransfer/... — ok; gofmt -l clean.

Closes #2388

🤖 Generated with Claude Code

…ns (#2388) filetransfer.Manager.transfers never had a removal path — entries accumulated for the life of the process-lifetime Heartbeat singleton. Nothing reads a transfer back after terminal state (the only map reader is CancelTransfer, which needs the entry only while in flight), so HandleTransfer now defer-deletes its entry on return, covering success, failure, and cancelled paths. A late cancel after removal hits the existing not-found no-op branch. Also fixes a latent data race: HandleTransfer/upload/download wrote transfer.Status/Progress/Error without holding m.mu while CancelTransfer and the in-flight cancellation checks used the mutex; writes are now guarded and reportProgress snapshots the fields under RLock. Closes #2388 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

cloudflare-workers-and-pages · 2026-07-12T22:06:17Z

Deploying breeze with Cloudflare Pages

Latest commit:	`ea3d689`
Status:	✅ Deploy successful!
Preview URL:	https://63b76406.breeze-9te.pages.dev
Branch Preview URL:	https://fix-2388-filetransfer-map-cl.breeze-9te.pages.dev

View logs

…r race guards Review follow-up: exercise CancelTransfer concurrently with HandleTransfer's terminal-state writes on the same Transfer so the new mutex guards are regression-detectable under -race, and pin mid-flight cancel behavior (failed + "transfer cancelled" + map entry removed) through the real path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

ToddHebebrand · 2026-07-12T22:12:45Z

Review run: /pr-review-toolkit:review-pr (code-reviewer, pr-test-analyzer, silent-failure-hunter)
Findings: 1 raised (pr-test-analyzer: the new mutex guards weren't exercised by any test — a revert of them would still pass -race) → addressed in ea3d689 with a deterministic mid-flight cancel test that races CancelTransfer against HandleTransfer's terminal-state writes; 0 outstanding. code-reviewer verified no lock-ordering/deadlock path (reportProgress/serverURL never called under m.mu); silent-failure-hunter confirmed nothing reads a transfer after terminal state and a late cancel was already an observable no-op pre-PR.
Declined (optional hardening): pointer-identity check in the deferred delete — only matters if a transferId is ever reused concurrently, which is unreachable (fresh DB UUID per transfer, command dedupe by commandId).
Tests: cd agent && go test -race -count=2 ./internal/filetransfer/... green; go vet and gofmt -l clean.
Status: review-clean, awaiting maintainer merge.

…2396) (#2404) ## Summary `Refs #2396` — implements **option 1** (client-side abort, the immediate honest fix) from the investigation comment on the issue. Options 2 (deprecate the dead `/remote/transfers` chunked path) and 3 (revive it as the FileManager backend) remain open for a maintainer decision, so this PR intentionally does **not** close the issue. Previously `cancelTransfer` in `FileManager.tsx` only did `setTransfers(prev => prev.filter(...))` — it removed the row from React state while the in-flight browser fetch kept running to completion. The UI claimed a cancel that never happened. ## Changes - **AbortController per in-flight transfer** (upload + download), stored in a ref map keyed by transfer id. Cancel calls `.abort()` on the transfer's controller; the existing 2-minute upload watchdog now shares the same controller. - **Distinct `cancelled` status** on `TransferItem` — a muted "Cancelled" label plus the dismiss affordance, instead of silently dropping the row. The catch paths in the upload/download flows recognize a user cancel (via a cancelled-ids ref) and skip the generic failure state, so an intentional abort never renders as "Failed to download/upload". The watchdog-timeout abort still surfaces as a failure. - **Honest UX copy**: aborting the fetch stops the client side only — there is no device-side cancellation on the single-shot `file_read`/`file_write` path. An upload cancelled after the write command was dispatched shows a note that the file may still be saved on the device. Strings added in both `en` and `pt-BR` locales. - **Dead code untouched**: no changes to `/remote/transfers*`, `fileTransfers`, or `agent/internal/filetransfer/` (which open PR #2391 is editing). ## Verification - `pnpm exec vitest run src/components/remote/FileManager.test.tsx` — 3 passed (new: cancel aborts the request signal + renders cancelled state for download; cancelled upload shows the device-side caveat and no failure toast) - `pnpm exec vitest run src/lib/i18n/localeParity.test.ts src/lib/i18n/keyUsage.test.ts` — 7 passed - `npx tsc --noEmit` — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Todd Hebebrand <todd@lanternops.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

…2413) Closes #2396 Executes **option 2** from the investigation on #2396: remove the dead chunked file-transfer subsystem end-to-end. The path has **never fired in production** — no server code has ever dispatched `file_transfer` / `cancel_transfer` commands to the agent, and nothing has called `POST /remote/transfers` since the web FileManager moved to the system-tools path (9379f17, Feb 2026); even before that, the create route only inserted DB rows. The live transfer mechanism (single-shot `file_read`/`file_write` via `systemTools/fileBrowser.ts`, plus PR #2404's client-side abort) is untouched. ## Removed **API** - `routes/remote/transfers.ts` (all `/remote/transfers*` routes) + `transfers.test.ts` - `routes/remote/internal.ts` (its only route was the agent-facing transfer-progress PATCH) and both mounts in `routes/remote/index.ts` - `workers/transferCleanup.ts` + its init/stop wiring in `index.ts` - `services/fileStorage.ts` (chunk save/assemble/stream helpers — no remaining consumers) - `createTransferSchema` / `listTransfersSchema` in `routes/remote/schemas.ts` - `getTransferWithOrgCheck` + `MAX_ACTIVE_TRANSFERS_PER_*` in `routes/remote/helpers.ts`; `hasSessionOrTransferOwnership` renamed to `hasSessionOwnership` (session paths keep using it) - openapi: `FileTransfer` component schema + `/remote/transfers*` paths - `EVENT_TYPES.REMOTE_FILE_TRANSFERRED` (`remote.file.transferred` — never emitted) and the transfer chunk-upload carve-out in `middleware/bodyLimit.ts` - Stale "use file transfer for larger files" copy in `systemTools/schemas.ts`, `aiToolsFilesystem.ts`, and `bodyLimit.ts` — that alternative no longer exists (and never worked) **Agent** - `agent/internal/filetransfer/` package (incl. its tests; PR #2391's fix is deleted with it — expected) - `handleFileTransfer` / `handleCancelTransfer` + registry entries in `heartbeat/handlers_desktop.go` - `CmdFileTransfer` / `CmdCancelTransfer` in `remote/tools/types.go` - `fileTransferMgr` field, `ftConfig` plumbing, and failover `SetServerURL` propagation in `heartbeat/heartbeat.go` (token init kept for the helper manager, renamed `ftToken` → `secToken`) **DB** - Migration `2026-07-12-drop-file-transfers.sql`: idempotent `DROP TABLE IF EXISTS file_transfers` + drops the two orphaned enum types. Per the forensic-trail rule it logs `RAISE WARNING 'dropping file_transfers with % rows'` before the drop. **The drop is destructive, but the data is dead**: rows (if any) are inert metadata from the never-functional feature — nothing could ever read or act on them. - `fileTransfers` table + `file_transfer_direction`/`file_transfer_status` enums removed from `db/schema/remote.ts` **Registration lists** (`file_transfers` removed from all three) - `DEVICE_ID_JOIN_POLICY_TABLES` in `rls-coverage.integration.test.ts` - device cascade-delete list in `routes/devices/core.ts` + `INTENTIONALLY_NO_ORG_ID` in `moveOrg.coverage.test.ts` - site-scope exempt lists in `site-scope-coverage.integration.test.ts` **Config/docs** - `TRANSFER_STORAGE_PATH` / `MAX_TRANSFER_SIZE_MB` / `MAX_ACTIVE_TRANSFERS_PER_*` removed from `.env.example`, `docker-compose.yml`, `deploy/docker-compose.prod.yml`, and the env docs (droplets may keep a stray empty `data/transfers` dir; harmless) - Docs: transfer API sections removed from `features/remote-access.mdx`, `reference/api.mdx`, `agents/commands.mdx`, `features/webhooks.mdx`, `docs/architecture.md` - `docsIndex.json` regenerated via `scripts/build-docs-index.ts` — note the checked-in index was stale, so the refresh also picks up docs pages added since it was last built (generated file) ## Intentionally untouched The `remote_session_type` enum value `'file_transfer'` and everything typed on it (`remote_sessions`, `SessionHistory.tsx`, `aiToolsRemote.ts`, `REMOTE_SESSION_TYPES`) — remote *sessions* typed `file_transfer` are a separate, live concept, and PG enum values can't be dropped safely anyway. `systemTools/fileBrowser.ts` and the FileManager UI are also untouched. ## Verification - Migration applied to a fresh scratch DB: table + enum types gone, ledger row recorded, `db:check-drift` green (396 files match); the forensic `WARNING: dropping file_transfers with 0 rows` fired - RLS coverage contract test run against a real Postgres **with the table actually dropped**: 53 passed - Site-scope coverage contract: 7 passed; moveOrg coverage + remote/sessions/bodyLimit/systemTools suites: green - `cd agent && go test -race ./...` green; `GOOS=windows` and `GOOS=linux` builds green - `tsc --noEmit` clean for api, web, shared; `@breeze/shared` tests 1078 passed 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Todd Hebebrand <todd@lanternops.io> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

This was referenced Jul 12, 2026

[Agent] filetransfer.Manager.transfers is never deleted from — unbounded map on a process-lifetime singleton #2388

Closed

[API] Transfer cancel never reaches the agent — cancel_transfer has no server-side sender #2396

Closed

ToddHebebrand merged commit 58735e2 into main Jul 13, 2026
41 checks passed

ToddHebebrand deleted the fix/2388-filetransfer-map-cleanup branch July 13, 2026 01:06

ToddHebebrand mentioned this pull request Jul 13, 2026

fix(web): FileManager cancel actually aborts the in-flight transfer (#2396) #2404

Merged

ToddHebebrand mentioned this pull request Jul 13, 2026

chore(remote): remove dead chunked file-transfer subsystem (#2396) #2413

Merged

Navigation Menu

Uh oh!

fix(agent): remove filetransfer map entries when HandleTransfer returns (#2388) - #2391

fix(agent): remove filetransfer map entries when HandleTransfer returns (#2388)#2391
ToddHebebrand merged 2 commits into
mainfrom
fix/2388-filetransfer-map-cleanup

ToddHebebrand commented Jul 12, 2026

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 12, 2026 •

edited

Loading

Uh oh!

ToddHebebrand commented Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ToddHebebrand commented Jul 12, 2026

Summary

Fix

Tests

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying breeze with Cloudflare Pages

Uh oh!

ToddHebebrand commented Jul 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cloudflare-workers-and-pages Bot commented Jul 12, 2026 •

edited

Loading