Skip to content

fix(upload): a stuck attachment can always be removed, and closing the peek no longer strands one - #124

Open
Dygreens wants to merge 3 commits into
mainfrom
fix/stuck-upload-attachments
Open

fix(upload): a stuck attachment can always be removed, and closing the peek no longer strands one#124
Dygreens wants to merge 3 commits into
mainfrom
fix/stuck-upload-attachments

Conversation

@Dygreens

Copy link
Copy Markdown
Collaborator

The report

I want to upload pictures but something [was] preventing me from uploading pictures last time so I re-uploaded them except I can't hit send because there are still pictures that seem to be trying to upload and no delete button. No remove button no way for me to send this prompt.

Root cause — two defects that compound

1. Cancellation was inferred from array membership. uploadAndAttach decided "did the user remove this chip?" by asking peekFiles.indexOf(placeholder) >= 0. But _peekFilesStash swaps peekFiles wholesale when the peek closes or switches worker — so closing the peek mid-upload read as a removal. The transfer was abandoned and the placeholder was left sitting in the stashed array at 0%, with nothing left to drive it.

The artifact confirms it: four empty .chunked-* dirs in ~/.amux/uploads. /api/upload/start had succeeded; chunk 0 never wrote. That is the signature of the loop bailing at i = 0.

2. The escape hatch existed only in the success state. The chip rendered its × inside the done branch, so the one state where you need it most — an upload that never finished — had no remove control at all. sendPeekCmd() refuses while any chip lacks a .path, so a single stranded chip made the composer permanently unsendable.

The fix

  • Cancellation is an explicit flag + AbortController, not array membership. The upload no longer cares which array it currently lives in: close the peek, switch worker, reopen — it still lands on its path.
  • The × renders on every chip in every state.
  • A failed upload keeps its chip with a retry, instead of being spliced out behind one toast (which silently lost the file). The original File is retained so retry is real.
  • _peekFilesRestore marks an orphan (no path, not in flight) as failed, so it presents as something to act on rather than as frozen progress.
  • The send guard names the file and says what to do. "Wait for upload to finish" is a lie when nothing is transferring — it is what kept the user waiting on an upload that was never going to complete.
  • The >12-file summary row surfaces a failed count; past that threshold chips are collapsed, so a blocker could otherwise sit behind a row that only reported progress.
  • Server: sweep orphan .chunked-* dirs by mtime. The in-flight map is in memory, so every restart orphaned its tmpdirs with no record left to purge them by.
  • Mobile: 44px-tall chip controls, per the project rule.

Verification

e2e/upload-chip-escape.spec.ts drives the real functions against a stalled upload, because the bug only exists while a transfer is in flight — a test using an instantly-completing file passes against the broken code.

pre-fix post-fix
in-flight chip can be removed
closing the peek does not strand it
failed upload keeps chip + retry

Control run with product code reverted to the parent commit and the spec kept: all three fail. Against the fix: 9/9 pass on desktop, mobile and ios-safari. npm run lint:spa clean (0 errors, verified the gate still fails on an injected undefined name); cargo check and cargo clippy -p amux-server clean.

Logged in frustrations.md as AMUX-85, plus AMUX-86 for a separate instrument defect found on the way in.

🤖 Generated with Claude Code

Doron Greenspan added 3 commits August 18, 2026 18:44
…e peek no longer strands one

Reported with its artifact: four EMPTY .chunked-* dirs in ~/.amux/uploads —
uploads that got /start and never wrote chunk 0. Attach pictures, close the
peek, reopen, and the composer is permanently unsendable: chips sit at 0% with
no remove control and Send refuses while any chip lacks a .path.

Two defects compounded.

1. CANCELLATION WAS INFERRED FROM ARRAY MEMBERSHIP. uploadAndAttach asked
   `peekFiles.indexOf(placeholder) >= 0` to decide whether the user had removed
   the chip. But _peekFilesStash SWAPS peekFiles WHOLESALE when the peek closes
   or switches worker — so a stash read as a removal. The transfer was
   abandoned (hence the empty tmpdirs) and the placeholder was left in the
   stashed array at 0% with nothing left to drive it. Cancellation is now an
   explicit flag on the object plus an AbortController, so the upload no longer
   cares which array it currently lives in: close the peek, switch worker,
   reopen, and it still lands on its path.

2. THE ESCAPE HATCH EXISTED ONLY IN THE SUCCESS STATE. The chip rendered its ×
   inside the `done` branch, so the one state where you need it most — an
   upload that never finished — had no remove control at all. The × is now
   rendered on every chip in every state, and a failed upload KEEPS its chip
   (with a retry) instead of being spliced out behind a single toast, which
   silently lost the file. The original File is retained so retry is real.

Also:
- _peekFilesRestore marks an orphan (no path, not in flight) as failed, so it
  presents as something to act on rather than as frozen progress.
- The send guard names the file and says what to do. "Wait for upload to
  finish" is a lie when nothing is transferring — it is what kept the user
  waiting on an upload that was never going to complete.
- The >12-file summary row surfaces a failed count; past that threshold chips
  are collapsed, so a blocker could sit behind a row that only reported
  progress.
- Server: sweep orphan .chunked-* dirs by mtime. The in-flight map is in
  memory, so every restart orphaned its tmpdirs with no record left to purge
  them by — they accumulated forever.
- Mobile: 44px-tall chip controls (project rule).

Verified: e2e/upload-chip-escape.spec.ts drives the real functions against a
STALLED upload, because the bug only exists while a transfer is in flight — a
test using an instantly-completing file passes against the broken code. Control
run: the same assertions fail on the pre-fix build (chip renders 0% with no ×;
placeholder ends {path:null,chunk:1} after the peek closes).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Amux-Session: amux
The three specs asserted on a chip rendered into a CLOSED overlay. Playwright
calls that chip visible, but a closed #peek-overlay takes no pointer events, so
the click landed on <body> and the failure read "x does not remove the chip" —
indistinguishable from the product bug the spec exists to catch. Open the peek
first (openPeek directly, per tab-customizer.spec.ts: the harness has no
sessions to click), and skip the first-run walkthrough whose backdrop covers
the page.

The stash test now switches workers via openPeek instead of calling
_peekFilesStash/_peekFilesRestore by hand, so it exercises the real path rather
than an imitation of it.

Control: with product code reverted to the parent commit and the spec kept, all
three fail (chip renders with no x; failed upload vanishes entirely). Against
the fix, 9/9 pass on desktop, mobile and ios-safari.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Amux-Session: amux
… that calls a deleted file 'changed' (AMUX-86)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Amux-Session: amux
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant