⛔ closed · #400 · open-legal-products/mike ← open-legal-products/mike · opened 10d ago by amal66 · closed 7d ago · self · +2,420-658 across 46 files · ↗ on GitHub

From the PR description

Review fixes for #397 - every blocker and major, replicated live before and after

Summary

This branch sits on top of feat/direct-upload-sessions and fixes the findings from a full live review of #397: 2 blockers, all 16 majors, and 14 of the 18 minors. Every UI-reachable bug was replicated in a real browser against the running stack before the fix and re-recorded after; storage/SQL-level bugs were replicated in terminal transcripts (signed-URL params, forced deadlocks, sweeper predicates). The table below links each finding's evidence pair.

Constraints (read first)

Base is the PR head, not main. Everything here is a delta on feat/direct-upload-sessions; nothing was rebased onto main. If #397 itself gets rebased, this branch must be rebased with it (or its commits cherry-picked into the PR branch).
feat/direct-upload-sessions was never force-pushed or modified - fixes land as this child PR so the review delta stays visible.
The migration slot problem can recur. The consolidated migration is 20260828_02. A migration's filename is a position in an order that main owns: if more migrations land on main before #397 merges, this file must be renamed past them as the last act before merge. A header comment in the file says exactly this.
Not everything is Chrome-replicable. The two storage findings (empty-body CRC32 checksum, unbound size/type) live in signed-URL parameters; the deadlock, sweeper, and advisory-lock findings live in Postgres. Those are evidenced with terminal transcripts instead of browser videos - the table says which is which. Two recording caveats are flagged inline (M8's 30-minute client timeout was temporarily shortened to 20 s for the recordings only, in both the before and after runs; M11's fix is evidenced by unit test because its 120 s PUT timeout doesn't fit a watchable clip).
CI's MinIO cannot prove B1. MinIO tolerates the empty-body checksum that R2-class stores reject, so green CI is not evidence for production storage. The fix removes the checksum (verified against the installed SDK), but one real PUT against the actual R2 bucket before merge is still the decisive check - it needs credentials this environment doesn't have.

Base-case replication (feature works)

before-happy.gif / after-happy.gif below: log in → project → add a PDF → placeholder row → completed document, via the full session protocol (create → signed PUT → per-file complete → worker claim → processing → poll). Also exercised end-to-end at the API level in happy-path-session-protocol.txt.

The findings table

Evidence links point to the olp-pr/397-evidence-assets branch. B = blocker, M = major.

#	Bug (what a user experiences)	Replication	Fix summary	Fix verification
B1	Every presigned upload URL embeds an empty-body CRC32 the browser can't strip; checksum-validating stores (R2) reject every upload while MinIO/CI stay green	transcript - URL shows `x-amz-checksum-crc32=AAAAAA==`	Both S3 clients get `requestChecksumCalculation: "WHEN_REQUIRED"`	transcript - no checksum params; plus offline SDK tests
B2	The migration sorts before six files already on `main`; deployments skip the table-creating file, the next migration aborts, every upload 500s	transcript - live: `relation "upload_processing_jobs" does not exist`	Both files consolidated into transactional `20260828_02`, at the tip of the order	transcript - deployment selection now picks it up; clean re-apply
M1	Signed PUT binds neither size nor content type: a URL signed for a 1 KB PDF accepted 5 MB of `text/plain`	same transcript as B1 - both 200 OK	`ContentLength` + `signableHeaders {content-type, content-length}`; browser-set Content-Length must now match the manifest	after-transcript: declared PUT 200; oversized 403, wrong type 403
M2	A hung LibreOffice conversion parks a worker forever, and the 60 s heartbeat guarantees the lease is never stolen; 16 poisoned files kill the whole pool	code-verified (spawn has no timeout/kill; heartbeat unconditional) - not browser-replicable	Hard conversion deadline + SIGKILL; per-job wall-clock cap stops the heartbeat so the lease ages out and is stolen	unit test: fake `soffice` that sleeps; timeout rejects + profile dir cleaned
M3	The 30-min session TTL can never be extended: a 2 GB batch on a normal uplink is destroyed mid-upload with unexplained errors	before-m3.gif - second wave fails when the deadline passes mid-batch	New `extend_upload_session_expiry` RPC: each successful per-file complete slides the deadline (never shrinks), capped at 4 h absolute age	transcript - `expires_at` visibly advances per completion
M4	`POST /:id/urls` revokes a file the server is actively sealing (no lease check), and seal writes carry no status guard	code-verified - race window, not reliably filmable	`/urls` resets `verifying` only past the lease cutoff; all seal writes fenced with `.eq(status,'verifying')`, 0 rows = lost the race	route tests whose fake `lte` is now a real predicate - they fail against the unfixed code
M5	Errored sessions get `cleaned_at` stamped without any deletion, permanently hiding them from the object sweeper → orphaned 100 MB objects	transcript - sweeper matches 0 rows	`cleaned_at` only on `completed` (whose objects the worker already removed); guarded no-op writes preserve retention timestamps	after-transcript - sweeper matches 1 row
M6	Lock-order inversion between the queue and claim RPCs → deadlock on the documented completion-retry path	transcript - `ERROR: deadlock detected`, reproduced deterministically (upgraded from the review's "Likely")	Queue RPC re-ordered job→file to match claim; session read un-locked; claim candidates bounded (LIMIT 32) before the lateral fairness count	stress transcript - 150 adversarial concurrent iterations, 0 deadlocks
M7	One file's failed `complete` throws away every other file's result: 5 real documents existed server-side while the UI showed a dead batch	before-m7.gif - DB had 11 docs, table showed 6	Unconfirmed files no longer throw; polling proceeds and the server has the last word; full per-file accounting always returned	after-m7.gif - 5 completed rows visible, only the stuck file still pending
M8	The processing timeout brands every file failed - including files already completed (server truth on record: 2 completed, UI said 3 failed)	before-m8.gif ⚠ client timeout shortened to 20 s for the recording	On deadline: completed files stay completed; only non-terminal files report `processing_timeout`	after-m8.gif - same fault, error names only the stuck file (same 20 s caveat)
M9	Dropping >50 files (or >2 GB) uploads nothing - a regression; the base branch uploaded all of them	before-m9.gif - 51 files, zero uploaded	Oversized files become per-file errors; the rest chunk into ≤50-file/≤2 GB sequential sessions with one merged accounting; the table's own pre-flight raised to a 500-file mis-drop guard	after-m9.gif - all 51 documents land (DB count on record)
M10	On Office WebViews without `crypto.randomUUID` (the exact case the add-in's own `secureUuid.ts` exists for), every upload dies instantly with no message	before-m10.gif - `randomUUID` deleted in-page: silent unhandled TypeError	`createSecureUuid` moved to `frontend/src/shared/lib/` and used for clientIds; add-in re-exports it	after-m10.gif - same environment, upload completes
M11	A stalled storage PUT hangs the batch forever - no timeout, and no `signal` is plumbed so nothing can ever cancel	before-m11.gif - frozen "Uploading." with no cancel affordance	Every wrapper accepts `{ signal }`; PUT runs under `AbortSignal.any(caller, 120 s timeout)` - a stall becomes a retryable attempt	unit tests (timeout-as-retry, abort distinguishable) - a 120 s timeout doesn't fit a watchable clip
M12	16 in-process workers per replica by default: measured 161 claim RPCs per 10 s, idle, and 16 potential LibreOffice spawns on the API container	transcript	Default 4 (cap unchanged); docs + `.env.example` updated	after-transcript - 40 calls/10 s
M13	Unchecking one file inside a checked folder clears the entire folder's selection (regression from the base branch)	before-m13.gif - checkbox state dumps in the session log	Cleared folders' implied documents are materialized into the id set before the deselect is applied; server-mode row lookup fixed (`documents`→`docs`)	after-m13.gif - siblings keep their checkmarks
M14	Select-all inside a 3-file subfolder selects every document in the project - and that count feeds the delete confirmation	before-m14.gif	Both halves scoped to the viewed folder's subtree; header checkbox state derives from the same scoped set	after-m14.gif
M15	Raw browser internals shown to users: the warning banner literally said "Failed to fetch"	before-m15.gif	`userFacingApiError(...)` in all three upload paths, per the AGENTS.md redaction rule	after-m15.gif - intentional copy
M16	New Project with an over-limit batch: modal closes with zero feedback, project created with 0 of its 51 files (`.catch(() => [])`)	before-m16.gif - DB shows `M16 Silent Failure	0`	Outcomes inspected and surfaced; counts derived from completed outcomes; retry reuses the created project; NewTRModal gains a `role="alert"` error line

Minors fixed in passing (each in the commit that owns its file): m1 advisory-lock namespace collision (before/after transcripts), m2 hourly-limit clamp, m3 housekeeping-before-busy-check, m4 non-transactional second migration (absorbed by B2), m5 terminal re-stamping, m6 unbounded claim scan, m7 dead multi-file loop, m8 SSR store leak, m9 forked firstUploadResult, m10 raw relative import → @mike/* aliases, m11 add-in chat discarding completed files, m12 CI CORS parity (env-var CORS → the real policy file, plus a loud failure if the store can't apply it), m13 abort mislabeled as unconfirmed, m14 filename-list instead of the actionable limit, m15 duplicate-row guard, m16 breadcrumb-derived persistence key, m17 unhandled delete rejections, m18 row-checkbox a11y.

Design decisions (stated so they can be vetoed)

Outcomes, not throws. The client now returns a full per-file accounting in every non-exceptional case; UploadBatchError remains only for aborts, session-creation failures, and a later chunk failing after earlier chunks landed documents (it then carries the full accounting). This is the root-cause fix behind M7/M8/M9/M16 rather than four spot patches.
The server has the last word on unconfirmed files. A lost complete ack keeps polling instead of guessing "failed" - the honest cost is that a truly-lost file resolves via session expiry (bounded by the TTL) rather than instantly.
Chunking over rejection for >50-file selections, sequential sessions only; the DocTable pre-flight becomes a 500-file mis-drop guard (10 sessions max against the 50/hour cap). Rejected alternative: a "split your upload" warning - it preserves the base branch's behavior less faithfully.
refresh_upload_session_status does NOT early-return on terminal states (the review note suggested it): a requeued processing_failed file can legitimately resurrect an errored session, so the fix guards the write instead of the entry.
Worker default 4, not a separate worker process. Splitting the worker out of the API process is the better end state but a deployment-shape change; out of scope here.
Upload copy stays in the shared client (failedUploadMessage) - known layering compromise; both apps already consume it. Moving copy to the hosts is a clean follow-up.

Deliberately left open

q2 (verdict): a file whose job exhausted 3 attempts still cannot be re-queued without a new session - is "start over" the intended recovery, and should the UI say so?
q3: R2_PUBLIC_ENDPOINT_URL is process-global; the documented add-in dev setting reroutes the web app's uploads too.
q4: the poll loop still runs to its deadline after navigation (deliberate per the code comment on failure-path preservation; a cancel affordance can now be wired since signal is plumbed).
M14 residual (library): listLibraryDocumentIds has no folder parameter, so inside a library folder select-all covers loaded rows only (under-selection; previously it over-selected across folders). Proper fix: a folder_id param on GET /library/:kind/ids.
The remaining verdict minors not listed above (m6's fairness ordering beyond the LIMIT, the fixture-only add-in e2e gaps).

Testing performed

npm test --prefix backend - full suite green (916 passed / 27 skipped) after all fixes; npm run build-level tsc --noEmit clean.
npm test --prefix frontend - 105 files / 692 tests green; coverage ratchet clears (99.86 / 97.78 / 100 / 100); tsc --noEmit clean; lint 0 errors.
npm run typecheck --prefix word-addin clean, plus a real webpack --mode development build for the new aliases.
New regression tests: presign parameters (real SDK, offline), conversion timeout, config clamps, /urls lease fencing (fake lte upgraded to a real predicate - the new tests fail on unfixed code), M7/M8 outcome semantics, chunk splits, no-Authorization-header storage PUTs, signed-URL rotation, abort semantics, secureUuid fallback, duplicate-row guard, SSR store isolation, breadcrumb-rename persistence.
The consolidated migration applied cleanly to a database that had run the branch's original two migrations (idempotent re-run), and the fresh-selection ordering was verified against main's recorded-version model.
Everything in the table's evidence columns was performed against the live stack (Postgres 54322 / MinIO 9000 / backend 3001 / Next 3000) on this branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BpdjmF3BsrGfjVX6qEx2G2

review fixes: direct upload sessions - 2 blockers + 16 majors, live-replicated before/after