fix(file-upload): fail closed when the dedup existence check errors

[stepandra]

Duplicate uploads fail when the blob existence check returns an error, without rewriting the existing blob. A blob reported missing is still restored. This update strengthens the evidence for the concurrent retry path and corrects documentation; it does not change the already-reviewed fail-closed implementation.

What changed in this update

• Added a deterministic conflict-retry regression using real Local blob I/O and an injected23505 state conflict. It proves exactly one provisional store and successful delete of the same non-winner key, real provisional absence, unchanged winner bytes, two lookups and the exact client error.
• Narrowed the documented boundary to returned BlobStore.exists errors. Local filesystem existence semantics are unchanged. Logs use error level via Log.critical; presence is a check-time guarantee, not permanent availability.
• Integrated current main and regenerated codemap from the final source/test. Two generations are byte-identical. No hand-edited counters and no Local/transaction rewrite.

Exact verification

Published candidate: 18ef44018d787b01d79e796864d4eb60f258c444; integrated main: 7bd77c0fd88a4b56091fc015388df85837e97d96.

• New exact regression: 1 example,0 failures.
• Full ContentDedup: 41 examples,0 failures, repeated on final integrated candidate.
• Criteria runtime:7 exact fixtures; spec-check, spec-drift, lint (No hints), codemap-check and repeat-generation passed.
• New-delta expectation guard passed. Whole-PR guard still identifies the original fail-closed expectation change covered by the existing maintainer expectations-approved decision; that is not blanket approval of other changes.

Limits / remaining review

This tests a deterministic injected state conflict with real filesystem operations, not a PostgreSQL race. Failed best-effort deletion can still leave a provisional object; no universal cleanup guarantee is added. No S3/provider behavior is claimed.

Full test-all --require-all and testbed have not run: the host5432 database is not owned by this session, several test fixtures hardcode that port, and no active isolated test runtime was available. Those gates remain required; missing dependencies are not skipped-green. Draft CI is not full verification.

Historical red-first evidence remains a separate maintainer disposition: review the genuine private receipt if available, or accept a clearly labelled after-the-fact reproduction. No receipt or historical approval was fabricated. The current fixed test passing is not a new failing C1.

This PR remains draft, auto-merge off, until the remaining evidence/full-gate review is resolved. Applications require no code/data migration for this narrow finish. Source of truth: Changes001/020 and the reviewed fragment. Merge/release/deploy remain maintainer actions.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[NickSeagull]

Please reconcile these four points:

1. The helper comment and test preamble still say to re-store after an exists error. Update them to match the fail-closed branch; keep the known-missing self-heal.
2. C3 promises no store call, but its test only checks the returned key. A redundant write would pass. Check store calls or narrow C3.
3. C1 promises a specific generic error, but the test only looks for verify and excludes injected. Assert the promised message or revise the criterion.
4. Narrow the release note's “nothing is written”: the existing concurrent path can write a provisional blob before the check fails. This is a wording fix, not a request to change that path.

[NickSeagull]

The expectation guard failed because we're changing an existing test expectation here. I approved the fail-closed behavior and applied expectations-approved so the guard can pass on its next run.

[stepandra]

@NickSeagull — fresh remediation scope/decision request for #908. Its live head still matches the audit. Auto-merge is off; please keep it off while these substantive issues and evidence gates are addressed. No fixing push has been made, and I will not turn auto-merge back on.

PR #908 — proposed narrow finish and historical evidence decision

This follow-up proposes clearer documentation and one focused regression test. A duplicate upload already fails when the blob existence check returns an error, without rewriting the existing blob. A blob reported missing is still restored. Applications need no code or data migration for this finish.

Please approve this exact scope (Gate 1 continuation)

1. Amend change020, change001 and fragment020 to say when BlobStore.exists returns Err, rather than promising a response to every unknown filesystem condition. State that this does not change BlobStore.Local.exists semantics.
2. Say error level via Log.critical. Replace permanent “never dangling” claims with a check-time promise. No logger, Local backend, transaction, or locking rewrite.
3. Preserve the exact client error Failed to verify stored file content. Please retry., Ok True zero existing-key stores, and Ok False self-healing for Pending and Confirmed matches. Keep all current expectations.
4. After approval and the required design review, add one deterministic conflict-retry regression in ContentDedupSpec: first lookup misses, injected state update returns 23505, retry finds the winner, and its existence check returns Err. Observe actual Local I/O. Assert one provisional store, one successful cleanup delete of that same provisional key, neither key equals the winner, original winner bytes unchanged, and the exact client error. This proves the retry path with an injected conflict and real filesystem, not a PostgreSQL race. Do not assert zero total stores or guaranteed cleanup if deletion itself fails.
5. Regenerate codemap after approved base integration; rerun the exact ContentDedup suite and required criteria, full-suite/acceptance and CI gates on the resulting candidate.

Proposed source of truth remains docs/changes/020-dedup-no-blind-rewrite.md; the scoped principles and checks belong there, not in a separate agent policy. No new public API or product constant changes. Change026 is not needed for this documentation/test-only continuation; if a new behavioral bug is found, stop and scope it separately.

Historical red evidence: separate decision required

The first published commit 11a84183b2493b20e708818e265d6cf79a02d9c5 contains spec, test and fix together. Public history does not establish a red-only commit. It also cannot establish that a private receipt never existed.

Please choose:

• Existing evidence: author supplies the genuine private committed-red receipt for maintainer verification; record its identity and acceptance without publishing local security material.
• Post-hoc evidence: explicitly accept the historical deviation and request a clearly labelled after-the-fact red reproduction on the original pre-fix base, followed by green on the current candidate. It is new regression evidence, not proof of historical red-first compliance. It requires an authorized isolated worktree/base and no published-history rewrite.

The current fixed head must not be represented as a new failing C1. Your expectations-approved comment authorizes the narrow expectation change; it is not this new Gate 1 continuation or final Gate 2 approval. “Sounds good … principles” is not being treated as either gate.

Suggested explicit reply: “Approve #908 Gate 1 continuation for the five items above, retaining returned-Err/missing-heal semantics and no Local rewrite. For the historical red gap choose [existing receipt / labelled post-hoc evidence], with [receipt/required follow-up].” A maintainer must separately resolve any machine-gate incompatibility; no approval record or red receipt will be fabricated.

No new product implementation or race test has been completed in this remediation cycle. Prior audit: 40 ContentDedup examples passed at d6f8566a6baa5943c5e4d4c270ee72e2c48df666; that is bounded historical verification, not fresh candidate CI. Publication, merge, release and deployment remain separate permissions.

Current preparation result

The narrow spec001/spec020/fragment020 wording amendments are prepared locally. Structural spec-check, spec-drift, criteria-locator resolution, fragment validation and diff checks passed. No new runtime or race test was run; no product implementation, commit, source push, regenerated candidate or new CI success is claimed. The historical red-evidence decision and explicit scoped continuation approval remain open.

Generated codemap sync

I am separately running the upstream codemap generator and repeat-generation checks at this PR's current published head. This addresses the generated-file request only; it is not approval or completion of the substantive audit remediation. Auto-merge remains off. I will not push a codemap-only merge-unblocking update while the serious findings/required decisions above remain unresolved. Generated results will need regeneration again after source/base integration.