Guard file uploads with remote object preconditions#119
Conversation
Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
| if (remoteHash !== expected.hash) { | ||
| if (remoteHash === (await hashBytes(bytes))) { | ||
| return { ok: true, kind: "done" }; | ||
| } | ||
| return { | ||
| ok: false, | ||
| concurrent: true, | ||
| failure: { path, message: REMOTE_DRIFT_MESSAGE }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
kind:"done" path has no test coverage
The branch where remoteHash !== expected.hash but remoteHash === hashBytes(bytes) — meaning the remote already holds our bytes from a previous orphaned upload — returns { ok: true, kind: "done" } and marks the action as completed. This is the "safely recognize identical orphaned uploads during retries" feature called out in the PR summary. It is not exercised by any test: the new #110 test only exercises the remoteMatches fallback after a failed conditional PUT, not this pre-PUT short-circuit. A targeted test (second-pass retry where the prior pass uploaded the file but lost the manifest CAS) would pin this behaviour against future refactors.
There was a problem hiding this comment.
@AysajanE Could you address this comment please?
Signed-off-by: Aysajan Eziz <aeziz@northset.ai>
|
@AysajanE Thanks for the contribution 🙏 |
Summary
Tests
Limitations
Verification
npm test && npm run lint && npm run buildexited 0 on this exact head (583e6c3) in a network-off container.No workflow or CI files are modified in this change.
Commands, environment, and hashes: receipt M-1056 — checkable in ~30 seconds without trusting us.
Self-run by the contributor, not maintainer verification.
AI-assisted and reviewed by Northset; I take responsibility for this submission.
Greptile Summary
This PR guards file uploads against concurrent remote writes by computing a conditional PUT header (
ifMatchfor existing objects,ifAbsentfor new ones) before every push, and stops a pass from uploading a stale manifest as soon as any file precondition fails.putConditiondoes a pre-flight GET to obtain the current ETag; if the remote content already matches the local bytes (orphaned upload from a previous pass) it returnsdoneand skips the PUT entirely, making retries cheaper.remoteMatchesis called on a 412 from a conditional PUT to distinguish a genuine concurrent writer from an idempotent re-upload, keeping the#87progress-recording behaviour intact for server errors while still flagging real races.executed.concurrentgate insyncOnceaborts the manifest upload immediately on detection, closing the window where a losing pass could upload a manifest that disagrees with the winning pass's file object (#110).Confidence Score: 5/5
Safe to merge — the new conditional PUT logic is correctly anchored to the manifest snapshot, the manifest CAS remains the final safety net, and all three changed files have direct test coverage for the new paths.
The putCondition ETag-fetch chain is correctly sequenced. The done path and remoteMatches idempotency check are each exercised end-to-end by the new tests. The concurrent flag propagates correctly through executeAction and executeSyncPlan and gates the manifest upload in syncOnce. The #84 test update is essential and correct. No logical gaps were found in the failure classification or loop-break behaviour.
No files require special attention.
Important Files Changed
putCondition(ETag-based pre-flight GET for existing files, ifAbsent for new ones) andremoteMatches(idempotency check after a 412); concurrent flag propagation and early loop exit are correct; all failure classifications look sound.executeSyncPlanand adds the early-return gate that prevents a manifest upload when any file precondition already detected a concurrent writer; change is minimal and correct.Reviews (3): Last reviewed commit: "test: cover sync retry outcomes" | Re-trigger Greptile