mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
xarmian
2c8ddffcb0
* fix(store): cover documents and comment bodies in the attachment reference walks (BUG-2614, BUG-2615)
Two defects with one shape: a content surface that carries `pad-attachment:`
references was missing from a walk meant to cover every such surface. Both were
found by Codex during BUG-2415 and both predate it.
BUG-2614 — the orphan GC could reclaim a live reference. AttachmentReferenced
counted items and comments; documents.content was never scanned, and neither
document write path stamped. An attachment referenced only from a document was
therefore both invisible to the sweep's scan AND unprotected by the stamp that
covers references landing mid-sweep.
The filing asked whether the documents surface is dead enough to delete instead
of widening the scan. Evidence says widen, and I am not making the deletion
call inside a bug fix: /workspaces/{ws}/documents has full CRUD mounted and
authenticated today (list/create/get/patch/delete plus restore, versions and
activity), so a direct API consumer can still write one. It IS legacy — the
route block says "v1 — will be replaced by items in Phase 2" and no first-party
client reaches it (zero references in the web API client and in cmd/pad) — and
production carries 4 document rows, all soft-deleted, none referencing an
attachment, newest touched 2026-04-27. "Reachable but unused by us" is not
dead, and the conservative fix is a few lines. Retiring the surface belongs
with the Phase 2 migration, deliberately.
CreateDocument had no transaction, so it gains one: the stamp has to commit
atomically with the content carrying the reference or it cannot serialize
against a concurrent claim, which is the whole point. UpdateDocument already
had a transaction and only needed the call — and only when content is actually
written, since a metadata-only PATCH neither adds nor keeps a reference and
must not vouch for one.
BUG-2615 — the bundle import's remap rewrote item content and fields but not
comment bodies, so an imported comment kept the SOURCE workspace's ids: broken
references in the destination, and the rehydrated rows they should point at
left referenced by nothing. Bundles do carry comments (export.go exports them,
ImportWorkspace re-inserts them); they carry no documents, so this stays scoped
to comments.
The remap also now stamps what the rewrites point AT. ImportWorkspace already
stamps each comment body at insert, but the body still holds the source ids
then and the remap runs later in the handler, so those stamps land on nothing
that ends up referenced — leaving a fresh clone referenced only by text the
transaction just wrote and carrying no stamp, which is exactly the shape the
never-attached claim reclaims. The REWRITTEN TEXTS are passed rather than every
id in the map, so a clone nothing references is not vouched for and does not
survive an extra GC window.
Seven negative controls, one mutation at a time, each failing exactly the test
that covers it: the documents scan leg, each of the two stamps, the comment
write-back (at store and end-to-end level), the remap stamp, and an over-broad
stamp-the-whole-map variant that the precision test catches. Per the standing
bar out of BUG-2301, every regression test here was RUN against the unfixed
code and observed to fail — including the end-to-end bundle fixture the filing
asked for, whose item deliberately carries no reference so that the items walk
alone cannot rescue it.
* fix(store): stamp before the remap's content writes, and state the caller precondition (BUG-2615)
Codex round 2, two P1s.
The first is mine and is a straight violation of the protocol I was mirroring:
I stamped AFTER the item and comment UPDATEs. stampAttachmentRefsTx's own
contract says to call it before the content statement, for two reasons that
both bite here. On Postgres the stamp row-locks the attachment rows for the
rest of the transaction, so a concurrent GC claim blocks and re-evaluates
against the fresh stamp — stamping last instead lets a claim delete the target
while the rewritten text is still uncommitted, after which the stamp matches
zero rows and the transaction commits a dangling reference. And every other
writer takes attachments before content rows, so writing content first inverts
the lock order and deadlocks against them. The texts are known as soon as both
scans finish, so the stamp simply moves up.
The second — the scan-then-write over comments has no row lock and no
old-value predicate, so a concurrent edit committed in between is clobbered —
is real as a shape but not reachable at the only call site, and is NOT fixed
here. The bundle import runs this against a workspace it has just created,
which no other session can reach yet: there is no concurrent writer to lose an
edit to, and no contention for the long transaction to hold up. The
pre-existing items walk has the identical shape, so this is a property of the
function rather than of the comment leg. Adding row locks or a compare-and-swap
would be machinery for an unreachable window.
What that argument does require is that the precondition stop being tribal
knowledge, since it is about the CALLER and the next caller is exactly who
would break it. It is now stated at the top of the function, where someone
adding a second call site reads it, rather than in this message.
Also declined, both pre-existing and neither introduced here: the one-transaction
scan of the whole population (same reasoning — one caller, fresh workspace), and
document slug allocation outside the create transaction, which predates the
transaction existing at all and yields a spurious unique-violation rather than
partial state.
NOT COVERED BY A TEST, stated rather than implied: the stamp ORDERING. The
existing guard asserts the stamp is present and fails without it, but it reads
end state, so it cannot distinguish before-the-writes from after. Proving the
order needs a concurrent-session Postgres instrument of the kind BUG-2409 used;
that is not built here. The ordering rests on the reasoning above and on the
contract documented at stampAttachmentRefsTx.
* docs(store): make the scanned-surface set an explicit contract (BUG-2614)
Codex round 3 P2. Both comments a maintainer reads still described the scan as
covering items and comment bodies — AttachmentReferenced's doc, and the
orphan-GC sweep's "Item content references the attachment" branch — so the
change that added documents left the two artifacts that explain it stale. Same
class as the sentinel comment on BUG-2301: the code was right and the text
someone acts on was not.
They now also say the thing neither said before, which is why this defect
happened twice: the SET of scanned surfaces is the contract. Any surface that
persists user-authored text containing a `pad-attachment:` token has to be
listed there, and adding one without adding it here silently makes its
references invisible to the GC. Comments (IDEA-1650) and documents (BUG-2614)
were both found after the fact, which is the argument for writing the rule down
rather than the two instances.
Round 3's P1 — restore paths do not re-stamp, so a reference reclaimed while
archived is dangling after restore — is filed as BUG-2629, not fixed here. It
is pre-existing and uniform: RestoreItem does not stamp either, so fixing only
RestoreDocument would leave the larger hole open while making documents
inconsistently better-protected. The filing records the asymmetry that decides
its priority: items are usually shielded by the claim's own item_id IS NULL
predicate, while a document-referenced attachment has no document_id column to
be shielded by and is always claimable.
* docs(store): mark the unstamped rename cascades in place, pointing at BUG-2629 (BUG-2614)
Codex raised the title-rename cascade's missing stamp in two separate rounds
despite being told it was filed. Being raised twice is the signal that the
disposition was only in a bug tracker and not where a reader of this code
meets the problem — the same correction BUG-2301 ended on.
Both sites now carry it: documents.go::updateLinksInTx and
wiki_links.go::cascadeTitleRename, each naming BUG-2629, why it is not fixed
here (uniform across both surfaces, so half-fixing makes them inconsistent),
and why it is the weakest member of that family (the cascade rewrites link text
in content whose references were already stamped and are still visible to the
scan, so a genuinely new reference needs a title containing a pad-attachment
token).
Comments only.