Uh oh!
There was an error while loading. Please reload this page.
Add typed maintenance diagnostics to the contract - #85
Merged
YellowSnnowmann merged 6 commits intoAug 23, 2026
Merged
Conversation
A host asking "how much is stored" or "how far behind is the pipeline" has no way to ask it. `MemoryMaintenance::doctor` answers a `MaintenanceReport`, whose `findings: Vec<String>` is written for an operator to read; a caller computing an idle time from it would be parsing prose back into numbers. So OpenHuman does not ask. It reaches past the contract into `store::chunks::store::with_connection` and queries TinyCortex's tables directly — seventeen sites across eight files, every one of them a read-only aggregate. That is the largest single group of direct engine references blocking openhuman#5560, and none of it needs a SQLite handle to cross a bus: the answers are counts, timestamps and one failure row. Three methods on the existing `Maintenance` family rather than a nineteenth capability, because `doctor` already is this family's diagnostics door: store_stats() -> StoreStats queue_stats(kind) -> QueueStats latest_queue_failure() -> Option<QueueFailure> Defaulted to empty rather than `Unsupported`. A caller asking a diagnostic can act on "nothing reported" and cannot act on an error, and a trait method that answers `Unsupported` is worse than the direct call it replaces — it moves the failure from compile time to run time. The TinyCortex engine implements all three for real, which is the half that makes the addition worth anything. `queue_stats` is one statement with one `now`. Six round trips would count at six instants, and an idle-time calculation built on that reads as a stall that never happened. `eligible_now` is deliberately separate from `ready`: deferred work is a healthy backlog, and conflating them reports it as a stall too. `rusqlite` joins this adapter at the exact version `tinymemory-core` pins. It carries a `links` key through `libsqlite3-sys`, so a mismatch is a hard cargo error rather than a silent duplicate; matching the pin unifies onto the bundled copy already in the graph. The test ingests a document and requires the numbers to move. Its first version asserted against `MemoryCore::store`, which writes a memory entry rather than a chunk, so it compared zero to zero and proved nothing. Deleting the engine's `store_stats` now fails it, which is the point: an engine that silently inherits the defaults reports an empty store forever, and that is indistinguishable from a healthy quiet one.
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: Warning Your free Security trial is over. An organization admin can activate billing to continue. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Migrating the first caller onto `latest_queue_failure` found the shape wrong. OpenHuman does not show the newest failure unconditionally — it first asks whether anything has succeeded since, and withholds the failure when something has, because a queue that recovered is not a queue that is broken. It reads both on one connection deliberately. The comment above that call says so, and names the review that caught it: with two reads a job can settle in between, and the decision flips on which side of the gap it lands. Serving the failure alone would have handed that caller two round trips where it had one, so the migration would have reintroduced a race through a refactor — the worst way to lose a fix, because nothing fails at the seam that lost it. So `QueueFailure` carries `last_success_ms`, read inside the same `with_connection` as the failure itself, and only when the failure is timestamped — without one there is nothing to compare against and the caller has to surface it either way. It is deliberately not `QueueStats::last_completed_ms`. That one counts a failure as progress, because a queue failing fast is not a queue that stalled. Supersession needs the opposite reading: only a success clears a failure. Same column, two different questions, so two different fields rather than one field that quietly answers the wrong one. The test drives a real failure through `mark_failed_typed` rather than writing the row by hand, because the query filters on `failure_reason IS NOT NULL` and only the typed path fills it — a hand-written row would prove the filter matches the test's own fixture rather than what the engine persists. It asserts `>=` on the two timestamps: both come from the wall clock and can land on the same millisecond, and what is under test is that they arrive together and are comparable, not the resolution of the clock.
Migrating OpenHuman's `pipeline_status` found two more numbers it reads from the engine directly, and both belong in the snapshot they are read beside rather than in a call of their own. `failed_unrecoverable` separates an alert from a shrug. Transient failures self-heal on the next attempt, and a caller escalating on `failed` alone pages someone for a queue that is already recovering. Counted in the same statement as `failed`, because two reads can land either side of a retry and report more unrecoverable failures than there are failures. `chunks_with_structure` is the numerator of the extraction-coverage figure the host displays. It is a count and not the ratio, because a ratio is only meaningful against the denominator it was measured with — and the code this replaces took the two with separate statements, so a write landing between them could produce a coverage above 100%. Read together they cannot. While writing the caller for the idle-time calculation, `last_completed_ms` turned out to disagree with its own documentation: the field says a settle, the SQL said `status = 'done'`. The host is explicit about which it needs — `completed_at_ms` is stamped on failure as well as success, so a queue failing as fast as it can run is making progress in the only sense idle time measures. Filtering to successes would have reported it as stalled, the opposite diagnosis, and the field's own doc had already committed to the right rule. Now the SQL does too, and the test fails without it. Supersession still wants the other reading, and has its own field for it.
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
The host used to answer "is the queue stuck?" from its own SQL, and its test planted rows to prove the two shapes that must not read as stalled. That query lives here now, so the test does too — otherwise the migration deletes a guard rather than moving it. The rule worth guarding is the one a caller cannot apply for itself. A job backing off after a transient failure stays `ready` with its next attempt scheduled forward; the caller sees counts, not schedules, so it cannot tell that apart from work nothing is picking up. Counting deferred jobs as runnable reports a queue behaving correctly as one that has stopped, and something escalates on it. So `eligible_now` excludes them and `ready` does not, and the test drives a real `mark_deferred` to prove the split rather than asserting the subset relation, which held before the distinction existed. The `kind` doc now also says what an unrecognised kind answers. The first caller passes a job kind this engine happens to have; the next driver will not have it, and "no jobs of a kind I never enqueue" has to be a count rather than an error or that caller has to special-case every driver. `Cargo.lock` picks up the `rusqlite` entry from the adapter's new dependency.
6 tasks
They were served and unreachable. `MemoryService` answered `StoreStats`, `QueueStats` and `LatestQueueFailure`, but neither of the two places that make a served method callable had heard of them: the module manifest, which decides what the loader publishes, and `tinymemory_bus::METHODS`, which is the list a host compiles against. Nothing links the three. The service derives its members from the `#[tinybus::interface]` block, the manifest lists them by hand, and the bus crate lists them again — so a method present in one and missing from the others produces no compile error anywhere in this crate. The two tests that caught it exist for exactly that, and they caught it: "these methods are served but not declared in the manifest, so no host can call them", then "served here but absent from tinymemory-bus". Which makes the omission the precise failure the contract addition set out to avoid. Adding a capability a caller reaches only to be refused moves the failure from compile time to run time, and that is worse than the direct call it replaces; shipping one that cannot be reached at all is the same mistake with the refusal removed. `METHODS` is a fixed-length array, so its length is the one part of this the compiler does check: 89 becomes 92, and the two prose counts that quote it follow. Found because the module is its own workspace and `cargo fmt --all` at the root stops at that boundary — the same reason CI runs a separate job for it. The root suite was green throughout.
Fourth and last list that has to agree. `EXPECTED_METHODS` in the loader E2E is what a real `dlopen`'ed module is checked against, and it is written by hand like the other three — so the same omission reaches it, and `cargo test --lib` does not run the target that would say so. That is the whole shape of this defect. A served method has to appear in the interface block, the manifest, `tinymemory_bus::METHODS` and this list, nothing derives any of them from any other, and only the array length is compiler- checked. Four hand-written lists, one compile-time guard between them. All twelve loader tests now pass against the built cdylib, one process per test. `every_declared_method_is_actually_routed` is the one that matters: the methods are reachable through a real host loading a real module, which is what "served" was supposed to mean two commits ago.
YellowSnnowmann
commented
Aug 23, 2026
ContributorAuthor
The failure is in the container build, before any code from this repo runs: That build context is Worth being precise about what the evidence does not show, though: the job fails on all four runs of this branch and passed on
Two real defects came out of chasing it, both in the same blind spot: the three new methods were served but absent from the module manifest, from |
YellowSnnowmann
marked this pull request as ready for review
August 23, 2026 15:35
How this change flows2 changed behaviours across 1 relationship. The code graph does not know these behaviours yet — normal for newly added code, and a cold index otherwise. 11 further behaviours left out to keep the diagram readable. flowchart LR
n0["MemoryService<br/>changed"]:::changed
n1["...embers_are_exactly_the_published_contract<br/>changed"]:::changed
n1 -->|uses| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 23, 2026
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 freeto 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.
Summary
Maintenancefamily:store_stats,queue_stats,latest_queue_failure.Problem
A host asking "how much is stored" or "how far behind is the pipeline" has no way to ask it.
MemoryMaintenance::doctoranswers aMaintenanceReport, whosefindings: Vec<String>is written for an operator to read — a caller computing an idle time from it would be parsing prose back into numbers.So OpenHuman does not ask. It reaches past the contract into
store::chunks::store::with_connectionand queries TinyCortex's tables directly: seventeen sites across eight files, every one a read-only aggregate — chunk counts, job counts by status, a newest timestamp, one failure row. That is the largest single group blocking openhuman#5560, and none of it needs a SQLite handle to cross a bus.Solution
Three methods on
Maintenancerather than a nineteenth capability, becausedoctoralready is this family's diagnostics door:Defaulted to empty, not
Unsupported. A caller asking a diagnostic can act on "nothing reported" and cannot act on an error. More importantly, a trait method that answersUnsupportedis worse than the direct call it replaces — it moves the failure from compile time to run time. The TinyCortex engine implements all three for real, which is the half that makes the addition worth anything.queue_statsis one statement with onenow. Six round trips would count at six instants, and an idle-time calculation built on that reads as a stall that never happened.eligible_nowis separate fromreadyfor the same reason: deferred work is a healthy backlog, and conflating them reports it as a stall.most_recent_chunk_msisOption.MAXover an empty table is SQLNULL, and "no chunks" must stay distinguishable from a chunk stamped at the epoch.rusqlitejoins this adapter at the exact versiontinymemory-corepins. It carries alinkskey throughlibsqlite3-sys, so a mismatch is a hard cargo error rather than a silent duplicate; matching the pin unifies onto the bundled copy already in the graph. The SQL lives in the adapter because adapting TinyCortex's schema is what this crate is for — the schema is not something the contract can describe.Testing
maintenance_diagnostics_read_the_store_rather_than_their_defaultsingests a document and requires the numbers to move.Its first version asserted against
MemoryCore::store, which writes a memory entry rather than a chunk, so it compared zero to zero and proved nothing — it failed on the first run and that is why it now ingests instead.Mutation-checked: deleting the engine's
store_statsmakes it inherit the trait default and the test fails withgot 0 after writing to 0. That is the failure worth guarding, because an engine silently inheriting the defaults reports an empty store forever, which is indistinguishable from a healthy quiet one.Validation
cargo fmt --all -- --check— cleancargo check --all-targets --all-features— cleancargo test -p tinymemory-tinycortex --test full_provider_conformance— 9 passed, 0 failedcrates/tinymemory-modulechecked in its own workspace — cleanRelated
Unblocks the diagnostics group of openhuman#5560. The remaining groups (the engine handle itself, chunk writes and transactions, the re-embed queue) need their own contract work; this is the one where the host's usage was already read-only and already expressible.