Skip to content

fix(gridfs): fence file-id ownership across uploads, finalize, abort and delete [WIP] - #12

Merged
rockyshi1993 merged 12 commits into
mainfrom
fix/gridfs-id-ownership-v01
Nov 12, 2026
Merged

rockyshi1993 merged 12 commits into
mainfrom
fix/gridfs-id-ownership-v01

Conversation

@rockyshi1993

@rockyshi1993 rockyshi1993 commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

GridFS V0.1 lifecycle repair (draft — not mergeable yet)

Base: main v3.5.3. Branch: fix/gridfs-id-ownership-v01.
Problems: MZ-15, MZ-16, MZ-18 (deterministically reproduced on original main, 4/4), plus MZ-04 TTL and MZ-14 matrix.

Production changes in this PR

  • Shared unique per-ID reservations in <bucket>.uploads across upload / uploadStream / resumable begin / delete / rename / known orphan cleanup.
  • Durable activeAppend claim and CAS transitions pending -> finalizing / pending -> aborting.
  • Prevent abort from removing finalize chunks; prevent delete from removing a replacement upload's chunks.
  • Rehash actual chunks on ambiguous finalize retry; never trust metadata SHA alone.
  • Migrate legacy createdAt TTL to seven-day idle expiresAt; active operations have no TTL to avoid stale writers resuming after ownership expires.
  • Preserve writePathPolicy entry gates and surface GRIDFS_UPLOAD_BUSY for ID contention.
  • Document coordinated old-writer drain requirement and crash-stale reservation limitations.

Regression coverage

  • Four original unsafe interleavings inverted into safe post-fix deterministic tests.
  • Existing GridFS roundtrip, stream, scan, abort/retry, policy and index tests retained.
  • Four additional scenarios: legacy TTL migration, index initialization while active append holds expiry unset, append/finalize exclusion, legacy bad metadata SHA rejected after actual chunk rehash.
  • Added targeted MongoDB 7.0.37 / 8.0.26 test workflow; added GridFS suites to full server matrix.

Verified evidence so far

  • Original reproduction CI: 4/4 unsafe baseline behaviors reproduced.
  • GridFS safety matrix run: MongoDB 8.0.26 32/32 pass for four test files (prior to adding the four additional scenarios).
  • Latest expanded matrix: queued / not yet accepted at time of this update.
  • Earlier release gate failed because CJS/ESM runtime artifacts exceeded the old budget by approximately 4–5 KiB; review-adjusted ceilings committed, without disabling the budget gate.
  • An older stream pre-open failure test referenced a createdAt TTL collision; after expiry migration this no longer errored and left a test stream waiting. The test has now been corrected to conflict with the new expiresAt TTL.

Release blockers — DO NOT MERGE

  • Latest expanded 8-case targeted GridFS regression matrix passes on MongoDB 7.0.37 and 8.0.26.
  • Full CI / release gate including Node 18 / 20 / 22 and Driver 6 / 7 completed successfully.
  • Review stale direct/delete/append/finalizing reservation recovery; no automatic lease expiry may let old writers resume.
  • Verify legacy session migration and worker-drain upgrade/rollback procedures.
  • Independent code review of error paths, ownership release and chunk cleanup.
  • Any failures fixed with new evidence and new CI results.

No PR merge and no npm publish are authorized by this draft.


Note

High Risk
Changes GridFS write/delete/consistency paths and requires coordinated writer drain for TTL migration; incorrect recovery or mixed-version deploys could corrupt or lose file data.

Overview
Introduces durable per-file-ID ownership in <bucket>.uploads, serializing upload, uploadStream, resumable append/finalize/abort, delete, rename, and orphan cleanup. Conflicting operations now surface GRIDFS_UPLOAD_BUSY instead of racing on chunks.

Resumable lifecycle adds activeAppend tokens, CAS transitions (pending → finalizing / aborting), and chunk re-verification on ambiguous finalize retries (metadata SHA alone is not trusted). ensureIndexes migrates legacy createdAt TTL to seven-day idle expiresAt for pending sessions only; active claims stay untimed.

An operator-only gridfs-recover.cjs plus EN/ZH runbooks support dry-run/apply recovery after verified writer drain. CI gains a GridFS MongoDB 7/8 matrix workflow, expanded integration suites, concurrency cancellation, and raised bundle budgets (~4–5 KiB) for the new code.

Reviewed by Cursor Bugbot for commit 32cfee1. Bugbot is set up for automated code reviews on this repo. Configure here.

@rockyshi1993 rockyshi1993 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent merge gate — unresolved P1 crash recovery. Do not merge yet.

Code evidence on current head: src/capabilities/gridfs/resumable.ts returns GRIDFS_UPLOAD_BUSY for status:'finalizing' when no files document exists, and for status:'aborting' unconditionally; src/capabilities/gridfs/ownership.ts only implements claim/release/cleanup and has no supported recovery/inspection for stale direct, delete, activeAppend, finalizing or aborting records. Since these intentionally have no TTL, a process crash or ambiguous network failure can strand an upload ID indefinitely. The approved V0.1 explicitly requires safe, audited stale owner recovery after the old writer is stopped. Add a documented recovery path, deterministic fail-after-CAS/crash tests, and test explicit idempotent abort/cleanup under a retained reservation. Also get exact-head latest Node18/20/22, MongoDB7/8, Driver6/7 and preflight CI verdicts; current newest jobs are still queued. Green MongoDB 8 initial 32/32 from an older SHA is insufficient for merge. No production release authorized.

Copy link
Copy Markdown
Contributor Author

Merge gate update: P1 crash/ambiguous-write recovery is being addressed in draft PR #13 on isolated branch fix/gridfs-stale-ownership-recovery-v01 (base is this PR's feature branch, NOT main). Added an operator-quiesced audited recovery utility, English/Chinese rollout/rollback runbook, deterministic post-CAS crash/recovery regression tests, and full/targeted CI triggers. The main PR remains Draft and MUST NOT be merged until the follow-up repair has independently passed the exact-HEAD Node18/20/22, MongoDB7/8, applicable Driver6/7 and release-preflight gates, and its recovery invariant is reviewed. No npm/tag publishing or Knowkit change.

rockyshi1993 commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor Author

Merge gate — verified recovery follow-up is still pending

The P1 crash-recovery follow-up remains draft PR #13, based on PR #12's feature branch (not on main). Current child HEAD: 460b9580d76f89fbb7c23f17ee60b6c7797b3248.

New independent-review fixes in #13:

  1. Atomically rotate recovery.token on resume and reject two recovering executors using the same snapshot.
  2. Resume requires explicit confirmation that the prior recovery executor, not only the GridFS writer fleet, was stopped.
  3. Stale ordinary writer may no longer release a recovering owner document, even with the original ownerToken: release now also matches the expected original state.
  4. Verify a visible direct upload's actual SHA if metadata advertises one; mismatches fail closed. Interrupted rename requires a complete visible file.
  5. New deterministic rejection, concurrent CAS and integrity tests; recovery test matrix now has 18 authored scenarios, not yet claimed as passing.
  6. CI workflows have per-PR concurrency cancellation and explicit job timeouts. Legacy runs created before the workflow change can still occupy runners.

Latest HEAD checks remain queued, not passed: child CI and MongoDB 7/8 safety matrix. Earlier canceled/green jobs belong to different SHAs and cannot serve as the merge gate. There is no verified Node18/20/22 + MongoDB7/8 + applicable Driver6/7 + release preflight verdict for 460b9580 yet.

Strict sequence: independent review and all child exact-HEAD checks green -> merge #13 into fix/gridfs-id-ownership-v01 -> verify complete parent PR #12 test matrix on its NEW head -> mark #12 ready, squash-merge to main with expected_head_sha -> verify main SHA and stop; no npm release or tags. A failed, skipped, queued, or canceled mandatory job blocks the merge.

)

Add operator-controlled audited recovery, deterministic crash tests, CAS recovery takeover, and stale writer release fencing.
@rockyshi1993
rockyshi1993 marked this pull request as ready for review November 12, 2026 05:06
@rockyshi1993
rockyshi1993 merged commit a2c63cc into main Nov 12, 2026
3 of 9 checks passed

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 32cfee1. Configure here.

nextChunk: 0,
createdAt: now,
updatedAt: now,
expiresAt: new Date(now.getTime() + 604800 * 1000),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Begin wipes chunks after stale preflight

High Severity

beginResumableUpload treats the preflight files/uploads reads as proof that no file exists, then deletes every chunk for that id after inserting the session. A concurrent upload/uploadStream can finish and release its reservation during those awaits, so this deleteMany can strip chunks from a now-visible file and leave a corrupt files document.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 32cfee1. Configure here.

if (!transitioned.matchedCount) throw uploadBusy('finalizeResumableUpload', ctx, uploadId);
// Only the winner of this CAS may inspect/commit files. Abort will now
// fail its pending -> aborting CAS instead of deleting validated chunks.
session = await requireSession(ctx, uploadId, 'finalizeResumableUpload');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finalize conflict leaves session frozen

Medium Severity

After the pending -> finalizing CAS, committedFile errors such as GRIDFS_FILE_EXISTS or GRIDFS_INTEGRITY_MISMATCH propagate without restoring pending. Chunk-validation failures do roll back. The leftover finalizing row then makes abortResumableUpload and delete return GRIDFS_UPLOAD_BUSY, so a conflicting visible file cannot be cleared through the public API.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 32cfee1. Configure here.

}
} finally {
// Rename never removes chunks and is safe to unlock after driver settles.
await releaseOwner(ctx, owner, 'rename');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename release overwrites the real result

Medium Severity

renameFile always awaits releaseOwner in finally. If that release throws, it replaces a successful rename or a GRIDFS_FILE_NOT_FOUND with GRIDFS_UPLOAD_BUSY. Upload already treats post-commit release failure as a warning and still returns success; rename does not, so callers see failure while the filename change may already have landed and the reservation stays stuck.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 32cfee1. Configure here.

@rockyshi1993
rockyshi1993 deleted the fix/gridfs-id-ownership-v01 branch November 12, 2026 05:32
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