Repository navigation
feat(server)!: crash-safe storage, write preconditions, and honest deletion - #7
Open
beardthelion wants to merge 14 commits into
Open
beardthelion wants to merge 14 commits into
beardthelion wants to merge 14 commits into
Conversation
getStore() memoizes for the life of the process, which is correct for the server and leaves tests no way to install a fault-injecting store or a second driver. setStore/resetStore open that door for tests only. The seam is production code, so the guard is that production never reaches it. A grep for callers would be an absence claim proved by grep; the test walks the real import graph from src/index.ts and src/mcp/server.ts instead, and carries a positive control that the walk reached the modules it claims to cover. Verified load-bearing: planting a reference in src/handler.ts turns that named control red and nothing else. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Entry blobs now live at a path named by their own ciphertext hash, so an overwrite never mutates a blob the visible manifest still points at. Commit order is blobs, then the manifest that publishes them, then reclaim what the new manifest no longer references. A crash before the manifest write leaves orphans no reader can see; a crash after it leaves stale extras no reader can see. Reads fall back to the old key-derived path, so entries written before this need no migration. Two consequences the sweep forced out. A corrupt manifest used to start clean on the reasoning that the blobs survived and a re-push would rebuild it; reclaiming blobs makes that destructive, so an unreadable manifest now refuses the write. And deterministic ciphertext means two entries can share one blob, so reclaim checks the live hash set before removing anything. The sweep injects a fault at every mutating call, evidences the plant landed at that index, and asserts the visible state is either the previous one or the new one and never torn. Verified red on the pre-change code at three tests, green after. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
A push may now carry, per entry key, the ciphertext hash it believes that key holds (or null for "should not exist"). The comparison runs inside the namespace lock against the manifest the write would actually mutate, before any projection, and collects every disagreeing key so one round trip tells the caller everything that moved under it. Disagreement is a 409 carrying those conflicts. The window this guards is the caller's own turn, which is why the base is per entry rather than a namespace version: a single-key write should not be refused because an unrelated key changed. A request with no base is accepted unconditionally, so existing clients are untouched, and the hashes view advertises the capability: without that a client cannot tell a server that enforces this from one that ignores an unknown field, and would report a guarantee it is not getting. DELETE reached upsert outside the catch that maps typed errors, so a conflict there would have surfaced as a 500. It now takes a base on the query, shape- checked separately since the body parser never sees a DELETE. Verified: removing the comparison turns exactly the five refusal tests red, and every pre-existing test passes unchanged. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Whether a delete removes the bytes is a property of the store, and a client cannot see which driver a deployment runs. So BlobStore declares it and the server reports it where a client already looks: the hashes view and every write response. fs and s3 erase. A store that keeps history does not, and a client that knows can refuse a scan mode that would let a secret land somewhere it can never be removed from. The interface change is pinned by a @ts-expect-error on a store missing the attribute: if the requirement is ever relaxed the directive goes unused and the type check fails. Verified by relaxing it and watching that happen. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
The health route is unauthenticated, so everything it says is public. It echoed the store's description, which on a driver whose label carries a URL or an owner would hand that to anyone who asks. It now returns liveness and the service name and nothing else, pinned by an exact-equality assertion. Reachability is still worth knowing, so it moved to startup: one write, read back, compare and remove under a reserved prefix, before the socket binds. A store we cannot reach now stops the process instead of answering 200 over it. The failure detail is the error's class, never its message, because a store error commonly carries an endpoint, a bucket and an object path, and that path carries a namespace slug. The disjointness control asserts the probe prefix against the paths the builders actually produce. Asserting that the namespace validators reject it would prove the wrong property and could not fail: tenants supply namespaces, never paths. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
An operator could not tell which account was refused or why: the only output was a startup line and a catch-all that dumped the raw error. Every refusal now emits one JSON line carrying timestamp, owner, code, status and route class. The field set is an allowlist, not a denylist. On a crypto-blind server the space of things that must never reach a log is open-ended, so a denylist only catches what someone thought to forbid; five fields cannot carry any of it because there is nowhere for it to go. The type has no index signature, so the compiler enforces it too. A namespace slug is deliberately absent: it reads as opaque but is a hash of a low-entropy namespace, so it is a stable per-tenant identifier anyone can reverse by dictionary. Logging happens once, where the response leaves handleRequest, so the code recorded is the code the caller received and a future refusal branch cannot be added without being covered. The catch-all now logs the error's class rather than its message, because a store error carries an endpoint, a bucket and an object path, and that path carries a slug. resetRateLimit exists because bun shares one process across test files and a test that exhausts a bucket would otherwise refuse requests in every later suite. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Three reviewers over the source diff. What they found: resetRateLimit duplicated _reset, which already existed and was already used by the rate-limit suite. Removed mine and moved the better explanation onto the one that was there. The blobstore module header still described one blob per entry key, which is the layout this branch replaced, and entryPath's docstring read as the current way to store an entry rather than the legacy one. A reader landing on either would have written new entries the old way. A code comment cited R12, a requirement id in docs/plans, which is in .git/info/exclude. That reference resolves to nothing for anyone reading the repo, so it now states the property instead. The QuotaError and StaleBaseError mapping was duplicated across the PUT and DELETE branches. Both reviewers verified folding it into the outer catch would be behavior-preserving today; it goes in a helper instead, so a future read path that threw QuotaError cannot silently answer 413 rather than 500. Also: prev now reuses checksumsFrom rather than rebuilding it, the unreachable half of the mutated disjunct is gone, a double branch on the same condition is one guard, and the readManifest comment no longer claims the refusal is write-only when reads take the same path. One tradeoff taken deliberately. The legacy blob sweep now runs only for keys the pre-write manifest knew, since only those can have a legacy blob. On s3 the unguarded version was a network round trip per touched key on every write, forever, while holding the namespace and owner locks. What is given up is sweeping a crash-orphaned legacy blob for a key absent from the manifest, which today is only swept in the narrow case where that key happens to be written again. Verified the sweep is still covered: removing it turns both legacy tests red. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Nine reviewers went at the previous commits. Three findings were real bugs and two were my tests failing the bar I set for them. Reclaim ran inside the commit closure, so a transient store delete after the manifest was published turned a durable write into a 500 and skipped the owner usage write, leaving quota under-counting. It now runs after the write is durable and never throws: collecting garbage that is already invisible to every reader must not fail, or roll back, a write that landed. Reclaim was also driven from the keys a request touched, which cannot see the orphans that matter. A write that died before publishing left blobs no later request can name, and a delete whose collection failed removed the key from the manifest so nothing could name its hash again. Those bytes stayed forever, uncounted by quota, while the server advertised erasure: 'erases'. BlobStore gains list() and reclaim now sweeps the namespace's blob directory against the live hash set, which finds both. Manifest hashes form storage paths now, and a manifest is parsed JSON rather than validated input, so contentPath proves the digest is bare hex before building a path. A malformed percent escape threw out of handleRequest entirely, past the envelope, the security headers and the log line, falsifying the comment saying no refusal branch escapes coverage. On the tests: assertConsistent compared two maps getData fills on consecutive lines behind one guard, so its missing-blob half could not fail; it now compares against the manifest. The corrupt-manifest control read the legacy path seed() never writes, so it asserted null equals null. The sweep reused one namespace, so later indices never reached a mutating call and it covered half the sequence; it re-seeds per index and pins the count. The reclaim guard, the s3 erasure declaration, the log's owner and route defaults, and the traversal check had no coverage at all. Verified by mutation rather than by claim. Seven mutations that previously left the suite green now fail it: reclaim's live check dropped, reclaim removed, s3 erasure flipped to retains, the handler guard removed, route hardcoded, owner default changed, and the digest check removed. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Round 2 of review, against the previous commit. Six of its seven fixes held under mutation; two things did not. Making contentPath throw was right for the write path and wrong for the read one: getData called it unguarded, three lines above its own comment about skipping rather than 500ing, so a single non-digest hash took the entire namespace's read with it where before it skipped one entry. It now skips that entry, with a control proving the healthy ones still read. The store-seam walk change was inert. Following import() adds no reach today because bin/memlawb.ts's only dynamic targets are roots the walk already had, and the floor of 20 sat below the real count of 25, so neither half could fail. The count is now exact and the comment no longer claims something untrue: a dropped root turns it red. Also from that round: reclaim resolves the store inside its try so "never throws" is structural rather than nearly-true, its failure line names the namespace so an operator can act on it, and its doc records what it costs and that it is single-instance for the same reason the lock is. s3's list had no coverage at all while the same commit asserted s3's erasure, which is backwards for the driver the hosted service runs. It now has a fake client proving pagination follows the continuation token and stops. Not covered, stated rather than implied: the nsSlug field on the reclaim failure line. That path writes to stderr directly rather than through the injectable sink. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
One predicate now decides whether a base hash is well formed, and both write verbs use it. Before, only DELETE checked the shape, so the same malformed value answered 400 there and 409 on PUT: the garbage reached the manifest comparison, never matched, and reported a conflict. A caller reading "someone wrote under you" would re-read and retry the same bad value forever. An unreadable manifest gets its own error and a 503 with code manifest_unreadable, on reads as well as writes. Refusing is right, but a generic 500 tells a caller to retry what no retry can fix and leaves an operator unable to tell it from any other fault. The rate limiter now runs before the refusal branches, keyed on the caller when there is one and a shared anonymous bucket when there is not. The unknown-route and unauthorized branches sat ahead of it, and every refusal writes a log line, so an unauthenticated caller could turn a trivially cheap request into unbounded log volume on the machine holding every tenant's ciphertext. Keying everything on the shared bucket would have let that abuse throttle real accounts, which is why the key is a named rule with its own test rather than an inline expression. The startup probe carries a deadline. Neither adapter sets a socket timeout, so a hung connect left startup pending forever and the failure line this module exists to produce was never printed. Smaller: the full view carries erasure like the other two surfaces, so a client that only pulls can still see it; a key deleted and re-added in one request is reported only as accepted, since naming it in both arrays tells a client mirroring deleted to drop a file the same response stored; the probe's byte-comparison branch, s3 list pagination, and the context defaults now have tests; and both files that install a store override reset it in afterEach, because bun shares one process and a failure before the inline reset leaked a stub into every later suite. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
The README's API table still advertised a `/health` field this branch removed and a PUT body without `base`, and RELEASE-0.1 still listed the store round trip as pending work on `/health` when it moved to a startup probe. The README is the published contract, so a reader building a monitor against it would key on a field the server no longer sends. Also documents what the write path gained: the optional `base` precondition and its 409, the `supports` and `erasure` fields, and the 503 a namespace answers when its index cannot be parsed. Test headers cited plan identifiers that resolve only inside docs/plans, which is in .git/info/exclude. For anyone reading this repository those pointed at nothing, so each header now states the property in plain English instead. BREAKING CHANGE: `GET /health` no longer returns `store`, and a namespace whose manifest cannot be parsed now answers 503 `manifest_unreadable` where it previously served an empty view. Without this footer release-please would cut a patch for a changed contract, because `bump-patch-for-minor-pre-major` is set. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
Every other test drives one layer. This drives the stack the way a deployment does: AES-GCM in the client, HTTP on a real port, the content-addressed store on disk. It covers what layer-local tests structurally cannot, a change that is correct in memory.ts and wrong once ciphertext, the wire format and the storage layout have to agree on the same bytes. Twelve cases: the push/pull/modify/delete lifecycle, client-side delta, no plaintext or passphrase on disk under the new layout, a wrong passphrase failing to read, the shipped client (which sends no base and reads neither supports nor erasure) still round-tripping against a server that enforces preconditions, the 409 refusal over the wire with the competing write surviving decryptably, delete actually removing bytes, liveness-only health, a corrupt index answering 503 rather than looking empty, and a budgeted caller getting a retry hint and recovering. Gaps closed alongside it. Reclaim failures go through an injectable sink like refusals do, so the namespace they name is asserted rather than assumed; that field is the only thing making the line actionable. The filesystem listing's absent-directory and temp-file branches are covered, both of which reclaim hits on ordinary writes. The server's delta short-circuit had no test at all: removing it survived the whole suite, and it now fails against a store that counts writes. A rate-limited request to a memory route was logged as route 'other', because the throttle ran before the route was classified. Verified by mutation rather than by claim: a client that stops encrypting fails six e2e cases including the ciphertext-at-rest walk, and disabling the precondition, reclaim, the corrupt-index refusal, or leaking the store label through health each fail it too. Also smoke-tested against a separately spawned server process: two entries pushed and pulled through real encryption, one deleted, zero plaintext on disk. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
beardthelion
force-pushed
the
feat/service-server-core
branch
2 times, most recently
from
September 4, 2026 03:17
55cf845 to
2358ce6
Compare
This was referenced Sep 4, 2026
This test failed in CI and passed everywhere else, which is the shape of a shared-state race rather than a defect. The rate limiter is one in-memory bucket keyed by owner, and under open auth every test file in the process shares that owner. The test sent requests until one came back 429, so its correctness depended on the bucket having room when it started and not being emptied by another file part way through, across a 240-request window it did not control. In CI the first request was already rate limited, so it logged one line and the assertion that there should be several failed. Both halves are now driven deliberately: one request for an ordinary refusal, then the bucket is spent directly rather than through the server, then one request for the rate-limited one. That does not make the race less likely, it removes the loop the race lived in. Worth recording that I could not reproduce the failure locally. Bun schedules test files independently, and with two files it will run one to completion before loading the other, so no small harness recreates the interleaving that nineteen files produce on a loaded runner. The diagnosis rests on the CI evidence, which is unambiguous: one logged line means the first request was already refused. What remains is a single request immediately after a reset. If another file empties the bucket inside that window this now fails on the 404 assertion, which names the real condition, instead of on a line count that reads like a logging bug. The assertions still bite: suppressing the rate-limited log line, and adding a field outside the allowlist, each turn this test red. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
House style takes commas, colons or separate sentences rather than em dashes. This comment is the only place on this branch that carried one. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Server-side groundwork. On its own this changes nothing a user of memlawb can see, and that is deliberate: it is the work whose cost rises sharply once real ciphertext and real deployed clients exist, so it is cheapest to do before either does.
What changes
A crash can no longer leave a namespace torn. Entry blobs move to content-addressed paths, so an overwrite never mutates a blob the visible manifest still points at. The commit writes blobs, then the manifest that publishes them, then collects what the new manifest no longer references. A crash before the manifest write leaves orphans no reader can see; a crash after it leaves stale extras no reader can see.
Collection sweeps the namespace's blob directory rather than the keys a request touched, because the orphans that matter are the ones no key can reach: a write that died before publishing, or a delete whose collection failed and removed the key from the manifest.
BlobStoregainslist()for that. Collection runs after the write is durable and never throws, since failing to collect garbage must not fail a write that already landed.Writes can carry a precondition. A push may include, per entry key, the ciphertext hash it believes that key holds, or null for "should not exist". Disagreement is a
409 stale_base_versionnaming what each key actually holds. Omitting it writes unconditionally, so existing clients are untouched, and the hashes view advertises the capability so a client can tell a server that enforces this from one that ignores an unknown field.It is per entry rather than a namespace version on purpose: a single-key write should not be refused because an unrelated key changed. The window it guards is the caller's own turn, not the moment between its last read and its write.
Deletion is honest. The store declares whether its delete actually removes bytes, and the server reports that where a client already looks. Combined with the collection above,
erasure: 'erases'is now a property rather than a claim.Operational surfaces.
/healthis unauthenticated, so it now reports liveness and nothing else; it used to echo the store's label, which on a driver whose label carries a URL or an owner handed that to anyone. Reachability moved to a startup probe, bounded by a deadline, that refuses to bind the socket rather than serve over a store it cannot reach. Every refusal emits one JSON line whose fields are a fixed allowlist, so an operator can attribute a rejection to an account and a reason with nowhere for plaintext, entry keys, or tokens to land.Breaking
GET /healthno longer returnsstore.503 manifest_unreadableon reads and writes, where it previously served an empty view. It used to start clean on the reasoning that the blobs survived and a re-push would rebuild the index. Collecting unreferenced blobs makes that destructive, so it refuses instead.Squash with a
!in the title, or release-please cuts a patch for a changed contract.Not in scope
Nothing here touches packaging, the sibling products, or storage on a node. The client still sends no precondition and reads neither new response field, so the lost-update window it closes is not yet closed for the shipped client.
authorizeNamespaceis untouched.The pre-content-addressing read path and its collection are retained so existing entries stay readable. Since nothing has been tagged or published, deleting that path outright is available and would be simpler, but that is destructive if any self-hosted instance holds older data, so it is left in.
On the testing
Every guard here was checked by mutation rather than by assertion. That was not ceremony: the first round of review found three correctness bugs and two of my own guards that could not fail, including one comparing a value to itself and another reading a path the fixture never wrote. Removing collection's shared-blob check, removing collection entirely, and flipping the s3 driver's erasure declaration all left the suite green at one point. They do not now.
tests/e2e.test.tsdrives the whole stack the way a deployment does, with real encryption in the client, HTTP on a real port, and the store on disk. A client that stops encrypting fails six of its cases, including the ciphertext-at-rest walk. Also smoke-tested against a separately spawned server process: entries pushed and pulled through real encryption, one deleted, zero plaintext on disk.124 tests, 19 files.
bunx biome ci,bun run type-checkandbun testall pass.