mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 13:31:26 -10:00
beardthelion
2837f5efb3
* fix(installtxn): put back an install a killed commit left behind
CommitDir publishes by two renames: the live target moves into a workspace
backup, then the staged copy is renamed into place. Neither is journaled, so a
process killed between them left the target absent with the install's only copy
retained in a workspace nothing ever read. plugins.Load and skills.Load
enumerate directories, so the extension simply disappeared, while the lockfile
went on listing it. Recovering needed one fact the transaction never wrote
down: which install a backup belonged to. CommitDir now records that before it
moves anything, and Recover puts the backup back when the target is absent.
Recovery is a second data-loss surface, so it is bounded on every side. It
refuses a name that is not a single element inside the install root, never
replaces a live target, identifies a workspace by the name StageDir gives one
rather than by contents alone, and leaves intact anything it cannot attribute.
Every path that takes the install lock recovers, not just the two that install.
Recovering on the install path alone is worse than not recovering: a removal
takes the not-present branch, drops the lockfile entry and reports success
while the backup it never looked at stays on disk, and the next install
republishes it, reinstating an extension the user deleted. Removal and the
terminalpet install hold the same lock and now do the same thing first.
Recovery stays an explicit call rather than a side effect of Lock, matching how
the other staged-swap transactions here invoke their repair pass and keeping a
filesystem mutation visible at the sites that cause it.
* fix(installtxn): retire a backup the publish rename already superseded
Recover skipped any workspace whose target was occupied, which left the backup
of a commit that was killed after its publish rename but before its cleanup.
Removing that install then deleted the live target and its lockfile entry but
not the skipped backup, and the next recovery read the absent target as an
interrupted swap and published the stale tree again. plugins.Load and
skills.Load enumerate directories, so the removed extension was loadable again
with nothing in the lockfile naming it.
The target already records how far the commit got, so recovery reads it as the
phase rather than carrying a phase file. Absent means the swap never finished
and the backup is put back as before. Present means the publish rename
committed, so the backup beside it is superseded and its workspace is retired.
The live install is never replaced or removed either way, and the guards that
skip a workspace with no backup, an unreadable or missing marker, or a recorded
name that is not a single element inside the install root are unchanged. The
retire path removes the workspace directly rather than through
cleanupWorkspace, which refuses one holding a previous precisely because it
cannot tell a superseded backup from one still owed a restore.
Reading the target that way is only safe once nothing can leave a partial tree
there. rollback deleted the failed install in place before restoring, so a
process killed partway through that delete left a husk at the target while the
backup was still the only complete copy, and recovery would have taken the husk
for a committed publish and deleted the last good tree. The renames are now
ordered so the target is never partial: the failed install moves aside into the
workspace, the backup moves back to the target, and only then is the set aside
tree removed. A first install has no backup to protect and recorded no target,
so it still deletes in place.
The move aside can fail too, and then the failed install stays live at the
target with the backup still the only copy of what it replaced. Rollback drops
the workspace marker on that path, which leaves a workspace nothing can
attribute, and recovery already leaves those alone.
* fix(installtxn): reconcile both halves of an interrupted commit
Recovery inferred three facts from incidental filesystem shape: target
presence stood in for the transaction phase, a public dot prefix plus two
ordinary filenames stood in for ownership, and a void best effort call stood
in for both "nothing to recover" and "recovery failed".
CommitDir swaps the trees and only then publishes the lockfile, so a kill
between the two leaves exactly what a kill after both leaves. A phase bit
written after the publish only inverts which side of the write the ambiguity
falls on, so Recover now asks the caller which of the two trees its published
metadata records. plugins and skills answer from the recorded hash; the
terminalpet install publishes no metadata and passes nil, where a live target
is the committed one. A backup the metadata still records goes back over the
target through the same move aside ordering rollback uses.
Recover returns an error, and every caller that takes the install lock aborts
on it before reading the lockfile, inspecting the target, installing over it,
or reporting a successful removal. Malformed, legacy, and unattributable
workspaces are still skipped without error; a workspace we own and could not
resolve is reported, and every workspace is processed before returning.
Ownership is now proven rather than inferred. The workspace marker carries a
magic and version first line, so a user authored skill directory that happens
to be named with the workspace prefix and to hold a previous directory is left
alone. A recorded name carrying the workspace prefix is refused too, since it
names another transaction rather than an install. Retirement clears the backup
before the workspace around it: os.RemoveAll unlinks the marker first, so a
failure inside the backup used to cost the workspace the attribution that gets
it reported at all.
* fix(installtxn): do not read a target we could not probe as absent
recoverWorkspace read any os.Lstat failure on the target as nothing being
there, which enters the restore branch: the one branch that moves a tree,
taken on the strength of a question that never got an answer. Only a
not-exist error means the swap did not finish; every other probe failure
leaves the phase unknown, so it is reported and the workspace is left as it
was.
The same distinction the reconcilers already draw between a missing file and
a file that could not be read, applied to the probe that decides which branch
recovery takes at all.
* fix(installtxn): ask the reconciler about the name that resolved the path
recoverableTarget trims the recorded name before resolving the path, but the
raw line went to the reconciler, so the two halves of one decision ran on two
different values. A marker recording "demo " resolves dir/demo and then asks
the lockfile about "demo ". The lookup misses, a miss proves the publish never
ran, and the committed target is replaced with the tree it superseded. Found
by CodeRabbit on the round-2 push; the test observes that replacement before
the fix.
Also fixes the two CI failures on the new tests, neither of which was a
product defect:
macOS and Windows: the skills recovery matrix compared the lock source against
the path the test handed the installer, while Install records
canonicalSource(source). t.TempDir returns /var on macOS where the recorded
value is /private/var, and an 8.3 short name on Windows where it is the long
one. The expectation now comes from the same resolver the installer uses.
Windows: the two plugins failure-injection tests make a directory unwritable to
force a rename or removal to fail. Windows does not block either that way and
root ignores the bits, so the injected failure never fired and the tests
asserted against a success they never meant to produce. They now skip there, as
the installtxn and skills equivalents already did.
* fix(installtxn): stop recovery acting on trees the lockfile never described
An adversarial pass over the recovery protocol, driven end to end through the
built binary rather than package tests, found that the protocol still acted on
evidence it did not have.
The one that loses data: a missing lock entry was read as proof the publish
never ran, so recovery replaced whatever was at the target with the retained
backup. A missing entry proves no such thing. The entry can be lost after a
publish that did run, since a truncated or deleted lockfile reads as an empty
one, and a directory with no entry is a state these packages already support
(Load returns it, Remove handles it). Worse, the tree at the target may have
nothing to do with the transaction: `zero tools make foo` scaffolds into the
plugins root, and a hand-written skill directory is an ordinary thing to find
in the skills root. Both were destroyed and replaced by a stale backup, and the
install reported success. Reproduced through the CLI both ways before the fix.
A missing entry is now PhaseUnknown, which stops every caller and touches
nothing.
The rest are the same class as the target probe fixed in 005fff72, a probe
whose failure was read as a fact:
- A marker that could not be read was skipped as somebody else's content, so
the only copy of an install stayed stranded while the caller was told there
was nothing to do. Present but unreadable is now reported.
- The retained backup was probed with Stat, which follows symlinks and never
checked the type, so a symlink there published content from outside the
install root and a regular file published a file where a directory belongs.
Only a directory is a tree this package set aside.
- An entry at the target was read as an install at the target. A dangling
symlink satisfies Lstat, and treating it as live retired the only real copy.
- Two workspaces claiming one install were acted on in turn, which published
one over the other and deleted the rest. Nothing ranks two claims, so both
are reported.
- A stale `failed` tree from an earlier rollback made the restore rename fail
for good, and since every caller aborts on a recovery error that wedged every
install and removal in the root. The workspace is proven ours, so the stale
tree is cleared first.
- A recorded name carrying a NUL or longer than a path element can be reached
Lstat and came back with an error that is not not-exist, reported on every
pass forever with no way for a caller to clear it. Such a name is not an
install to recover and is skipped.
Unresolved-transaction errors now name the workspace directory, because the
only remedy is to delete it by hand and nothing in the CLI can reach it.
Left alone deliberately: an interrupted RemoveDir still writes no marker, which
the PR body already discloses as a pre-existing residual.