Repository navigation
fix(core): finish a push after a key change, keep waits at hand-over - #2255
Conversation
Untested by a new case: the conformance test covers a second pull that places a file.
aicayzer
left a comment
There was a problem hiding this comment.
Review: ready to merge
Summary. After a key change, a push now rebuilds the folder's copy once and finishes in the same run, sending each waiting write once and reporting every verdict; a watch keeps the server's wait across a hand-over. Reviewed in three rounds on the strongest model; safe to merge.
First draft: 3/5. 2 blocking issues and 1 smaller one in round 1; 1 blocking issue and 2 smaller ones in round 2; 1 blocking issue in round 3. All fixed.
Found.
- Round 1, blocking: a drain cut short by the view change lost every verdict it had settled, so a refused create went unreported (reproduced: exit 0, no refusal). Fixed with
drain::Cutinb513a5a01. - Round 1, blocking: the drain after the pull did not rebuild, against the new statement. Fixed in
a9c4dbd45. - Round 1: a later drain with no
Retry-Afterkept an older wait. Fixed in11fdee7f8. - Round 2, blocking: a write whose own read-back met the change was sent once and never reported; a refused placement in that position was sent again by the next push. Fixed in
98a8ff24dand7eac4cd28, which also stops the placement refusal record failinghydration_incompleteon an expired copy. - Round 2: a withdrawn verdict from the cut drain stayed in the report. Fixed in
235fa3950. - Round 2, session decision: after a rebuild at the drain after the pull, the push pulls once more, so the folder holds what the new view holds when the run ends.
4cf32efbb, statementfolders/view-changed-push-pulls-again. - Round 3, blocking: the drain after that second pull ran even when it placed nothing. Fixed in
bf5e21a8d.
Checked. Writes keep their idempotency keys across the rebuild; the rebuild runs under the command's writer role with no second writer; at most one rebuild per run, and a change after it ends the run copy_expired; the stored wait is capped and cannot hold a folder for good; absorb never drops a verdict whose write is still queued.
Not checked. A real server with a real key replacement, the live suites, and the Swift and Node bindings, which take the rebuild through the core and do not report rebuilt yet.
Summary
After a folder's key is replaced or narrowed, the server's read view for that key changes. The first
marfa folders pushafter the change stopped withcopy_expired(read_view_changed) once its first write was read back, leaving every write behind it waiting until the person ran another push. A push, and a send-only run, now hydrates the copy again and drains once more in the same run, so the waiting writes go out, each once.Separately, a folder watch forgot a server's
Retry-Afterwait when it handed its folder to a command and took it back, so it sent again at once and was refused again. The wait is now kept in the folder's store by every drain of the folder, the watch's or a command's, and a watch that takes the folder back waits out the most recent one.Changes
Folder::syncandFolder::send_onlydrain through a newdrain_rebuilding: where a drain endscopy_expiredwith the reasonread_view_changed, it hydrates and drains once more. Every drain of a push goes through it, the drain after the pull included. Only one rebuild per run: a later change ends the runcopy_expired. A failed hydration ends the run with its own error and exit code. Waiting writes stay queued in both cases, and idempotency keys are kept, so nothing is sent twice.drain::Cut), and the folder drain remembers the placement refusals in it and holds the refused files before it returns. The rebuild adds the partial report to the report of the drain after it, so the push's report and exit code include every verdict from both drains.settle_placementsis split:remember_refusalsreads only the report, and runs on the partial report too; giving way, which reads the server, runs only after a drain that ended well.SyncReportandSendReportcarryrebuilt, the hydration taken, andfolders pushreports it asrebuiltin JSON and as one line of text.copy_expired. That path is unchanged and now has a regression test.folder_send_again_at, wall-clock milliseconds), replaces it with a later answer, and clears it when a later drain has writes taken, or leaves writes undelivered, with no wait. A watch starts with the kept wait, capped at the 300-second most a drain keeps, so a wall clock set back cannot hold it.409 read_view_changedwas left out of the push's report, and a refused placement was not remembered, so the next push sent it again. The same holds for a write refused before it was sent whose read meets the change.hydration_incompleteand replaced the drain's owncopy_expired.copy_expired.folders/view-changed-push-pulls-again(new),folders/view-changed-push-rebuilds,folders/view-changed-push-once,folders/view-changed-rebuild-fails,folders/view-changed-watch-rebuildsandfolders/watch-rate-limit-handoverinconformance/spec/folders.md.folders.test.ts(a scripted server that refuses the read-back with409 read_view_changed, after a refused create, and at the placement read after the pull) andfolders-handover.test.ts(a wait named by the watch's own drain, and a later one named to a command's push).scriptFolderWritestakes areadinghook for the read door, andEdgeDoorone for the read of an edge by id.core/README.mddescribes both behaviors.Testing
copy_expiredwith exit 4, the rebuild-failure test gotcopy_expiredinstead ofserver, and the hand-over tests measured gaps of about 2 seconds where 8 and 4 seconds were named. All pass with the fix, and passed three runs in a row.400 validation_error, Beta created, Beta's read-back refused409 read_view_changed). Without the fix the push exited 0 and its report had no refusal for Alpha's create, while the queue held it refused. With the fix the report has Alpha'svalidation_errorrefusal, the exit is 0 (a refusal is a verdict, not a write left waiting), nothing is undelivered, and each create was sent once.409 read_view_changedon the read of the first placement it sent. Without the fix the push endedcopy_expiredwith exit 4. With the fix it rebuilds once, places both files with onePOST /edgeseach, exits 0, and the next push sends nothing and does not rebuild.retry_after_secondsnow clears the stored wait. It kept the older wait before the fix.cargo fmt,cargo clippy -p marfa-core -p marfa-cli --all-targets -- -D warnings, andcargo test -p marfa-core -p marfa-cliall pass.folders,folders-handover,folders-send-only,folders-watch-everyandread-viewpass against the built binary (170 tests).conformance/src/utils/(spec-form,spec-statements,spec-citations) andci/core-contract.test.tspass,tsc --noEmitpasses forconformance, and Prettier is clean.drain.rs: an accepted write and a refused write whose read-back is refusedread_view_changed, and a write refused unsent whose read meets the change. Before the fix each reportedanswered0 and no verdict; each has a witness run where the read is answered.403 edge_permission_deniedwhose owed read is refusedread_view_changedat the drain after the pull. On a binary without the verdict fix, the next push sent the placement again (twoPOST /edges). With the verdict fix but not the settings-version fix, the push endedhydration_incomplete. With both, the push exits 0, rebuilds, reports the refusal, and the next push sends no placement.folder/sync.rs: a verdict an earlier drain told for a write a later drain withdrew leaves the report. It stayed with the filter removed.Three.mdwas not written. With it,Three.mdis written and placed in the same push,Two.mdkeeps its bytes, and the next push reports itlostand sends no create.read-view-live,edge-move-live), and the Swift and Node bindings' folder tests. The bindings compile and get the same rebuild through the core, but they do not reportrebuiltyet. A key replacement that also changes the key the refusal record names clears that record by design, so a refused placement is tried again under the new key; the scripted server keeps the key, so that case is not exercised here.Notes for reviewers
On the extra pull: a file whose row the new key cannot read is not removed by it. A bound file pins its row, a hydration keeps a pin whose row the key cannot read (
device/pin-gone-kept), and the pull leaves a row the copy lost for the scan to report (folders/lost-row-unchanged-held). So the visible effect of the extra pull is writing and placing what the new view holds in the same push; the unreadable item's file stays, as before.Review depth: full, it rebuilds the folder's copy with writes waiting.
The rebuild is limited to
read_view_changed. Othercopy_expiredreasons (instance_changed, a credential ended, an invalid view) still end the run, because rebuilding against another instance or without a credential is not safe to do unasked.Known limit: where the rebuild or the drain after it fails, the run ends with that error, and the verdicts the first drain settled are not in any report. They are in the queue, the placement refusals among them are remembered, and the refused files are held.
Docs for the release pass:
core/README.mdnow describes the rebuild and the kept wait, andfolders push --jsonhas a newrebuiltfield.Checklist